Skip to content

Commit

Permalink
Extend WebStateListObserver::ChangeReason with CLOSED and INSERTED flags
Browse files Browse the repository at this point in the history
These flags are respectively passed from WillCloseWebStateAt and
WebStateInsertedAt callbacks. Previously mentioned callbacks used
CHANGE_REASON_USER_ACTION flag.

Also rename CHANGE_REASON_USER_ACTION to CHANGE_REASON_ACTIVATE to better
reflect the meaning of the flag.

This change is needed by Breadcrumbs feature which attaches steps to
reproduce to crashlogs. This way Breadcrumbs can log WebStateActivatedAt
events only for user triggered activations (all other activations are
not useful and only create noise in Breadcrumbs logs).

This CL does not have any functional changes to other features and all
clients of WebStateActivatedAt were updated to use new flags in addition
to CHANGE_REASON_USER_ACTION.

Bug: 1046231
Change-Id: Idc5685ee052e8f056200ea9e94b397a2909b4afa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2084152
Commit-Queue: Eugene But <eugenebut@chromium.org>
Auto-Submit: Eugene But <eugenebut@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746536}
  • Loading branch information
Eugene But authored and Commit Bot committed Mar 3, 2020
1 parent 4d148c1 commit eb7a7e2
Show file tree
Hide file tree
Showing 8 changed files with 37 additions and 20 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,9 @@
case WebStateListObserver::ChangeReason::CHANGE_REASON_REPLACED:
change_reason_string = "replaced";
break;
case WebStateListObserver::ChangeReason::CHANGE_REASON_USER_ACTION:
case WebStateListObserver::ChangeReason::CHANGE_REASON_ACTIVATED:
case WebStateListObserver::ChangeReason::CHANGE_REASON_CLOSED:
case WebStateListObserver::ChangeReason::CHANGE_REASON_INSERTED:
change_reason_string = "for user action";
break;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -522,8 +522,11 @@
web::WebState* new_web_state,
int active_index,
int reason) {
if (reason & WebStateListObserver::CHANGE_REASON_USER_ACTION)
if ((reason & WebStateListObserver::CHANGE_REASON_ACTIVATED) ||
(reason & WebStateListObserver::CHANGE_REASON_CLOSED) ||
(reason & WebStateListObserver::CHANGE_REASON_INSERTED)) {
RecordTabSwitched(old_web_state, new_web_state);
}
}

void TabUsageRecorderBrowserAgent::SessionRestorationFinished(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,11 @@
web::WebState* new_web_state,
int active_index,
int reason) {
if (!(reason & WebStateListObserver::CHANGE_REASON_USER_ACTION))
if (!((reason & WebStateListObserver::CHANGE_REASON_ACTIVATED) ||
(reason & WebStateListObserver::CHANGE_REASON_CLOSED) ||
(reason & WebStateListObserver::CHANGE_REASON_INSERTED))) {
return;
}

NSMutableSet<NSString*>* set = [NSMutableSet set];
if (active_index > 0) {
Expand Down
2 changes: 1 addition & 1 deletion ios/chrome/browser/web_state_list/web_state_list.h
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ class WebStateList {
// Makes the WebState at the specified index the active WebState.
//
// Assumes that the WebStateList is locked.
void ActivateWebStateAtImpl(int index);
void ActivateWebStateAtImpl(int index, int reason);

// Sets the opener of any WebState that reference the WebState at the
// specified index to null.
Expand Down
13 changes: 7 additions & 6 deletions ios/chrome/browser/web_state_list/web_state_list.mm
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,8 @@ bool WasOpenedBy(const web::WebState* opener,
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(ContainsIndex(index));
auto lock = LockForMutation();
return ActivateWebStateAtImpl(index);
return ActivateWebStateAtImpl(index,
WebStateListObserver::CHANGE_REASON_ACTIVATED);
}

base::AutoReset<bool> WebStateList::LockForMutation() {
Expand Down Expand Up @@ -295,7 +296,7 @@ bool WasOpenedBy(const web::WebState* opener,
SetOpenerOfWebStateAt(index, opener);

if (activating)
ActivateWebStateAtImpl(index);
ActivateWebStateAtImpl(index, WebStateListObserver::CHANGE_REASON_INSERTED);

return index;
}
Expand Down Expand Up @@ -417,7 +418,8 @@ bool WasOpenedBy(const web::WebState* opener,
// WebState is de-activated before closing them. This avoid sending
// one notification per WebState in the worst case (when the active
// WebState is the last one and no opener is set to any WebState).
web_state_list->ActivateWebStateAtImpl(kInvalidIndex);
web_state_list->ActivateWebStateAtImpl(
kInvalidIndex, WebStateListObserver::CHANGE_REASON_CLOSED);

// Close the WebStates from last to first.
while (!web_state_list->empty())
Expand All @@ -427,14 +429,13 @@ bool WasOpenedBy(const web::WebState* opener,
close_flags));
}

void WebStateList::ActivateWebStateAtImpl(int index) {
void WebStateList::ActivateWebStateAtImpl(int index, int reason) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(locked_);
DCHECK(ContainsIndex(index) || index == kInvalidIndex);
web::WebState* old_web_state = GetActiveWebState();
active_index_ = index;
NotifyIfActiveWebStateChanged(
old_web_state, WebStateListObserver::CHANGE_REASON_USER_ACTION);
NotifyIfActiveWebStateChanged(old_web_state, reason);
}

void WebStateList::AddObserver(WebStateListObserver* observer) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,11 @@
if (metric_collection_paused_)
return;
++activated_web_state_counter_;
if (!(reason & WebStateListObserver::CHANGE_REASON_USER_ACTION))
if (!((reason & WebStateListObserver::CHANGE_REASON_ACTIVATED) ||
(reason & WebStateListObserver::CHANGE_REASON_CLOSED) ||
(reason & WebStateListObserver::CHANGE_REASON_INSERTED))) {
return;
}

base::RecordAction(base::UserMetricsAction("MobileTabSwitched"));
}
Expand Down
19 changes: 13 additions & 6 deletions ios/chrome/browser/web_state_list/web_state_list_observer.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,20 @@ class WebStateListObserver {
// the active WebState change.
CHANGE_REASON_NONE = 0,

// Used to indicate the active WebState changed because it was replaced.
// Used to indicate the active WebState changed because active WebState was
// replaces.
CHANGE_REASON_REPLACED = 1 << 0,

// Used to indicate the active WebState changed due to a user action.
CHANGE_REASON_USER_ACTION = 1 << 1,
// Used to indicate the active WebState changed because it was activated.
CHANGE_REASON_ACTIVATED = 1 << 1,

// Used to indicate the active WebState changed because active WebState was
// closed.
CHANGE_REASON_CLOSED = 1 << 2,

// Used to indicate the active WebState changed because a new active
// WebState was inserted.
CHANGE_REASON_INSERTED = 1 << 3,
};

WebStateListObserver();
Expand Down Expand Up @@ -76,9 +85,7 @@ class WebStateListObserver {

// Invoked after |new_web_state| was activated at the specified index. Both
// WebState are either valid or null (if there was no selection or there is
// no selection). If |reason| has CHANGE_REASON_USER_ACTION set then the
// change is due to an user action. If |reason| has CHANGE_REASON_REPLACED
// set then the change is caused because the WebState was replaced.
// no selection). See ChangeReason enum for possible values for |reason|.
virtual void WebStateActivatedAt(WebStateList* web_state_list,
web::WebState* old_web_state,
web::WebState* new_web_state,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,7 @@

// Invoked after |newWebState| was activated at the specified index. Both
// WebState are either valid or null (if there was no selection or there is
// no selection). If |reason| has CHANGE_REASON_USER_ACTION set then the
// change is due to an user action. If |reason| has CHANGE_REASON_REPLACED
// set then the change is caused because the WebState was replaced.
// no selection). See ChangeReason enum for possible values for |reason|.
- (void)webStateList:(WebStateList*)webStateList
didChangeActiveWebState:(web::WebState*)newWebState
oldWebState:(web::WebState*)oldWebState
Expand Down

0 comments on commit eb7a7e2

Please sign in to comment.