New issue
Advanced search Search tips

Issue 817174 link

Starred by 1 user
Status: Verified
Owner:
charleszhao@chromium.org
Closed: Aug 9
Cc:
charleszhao@chromium.org
napper@chromium.org
michae...@chromium.org
pkasting@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

Freeze tab metrics while closing multiple tabs

Project Member Reported by michae...@chromium.org, Feb 28 2018 
The ForegroundedOrClosed event logged when closing a tab will include
metrics like MRUIndex and TotalTabCount.

When an entire browser window is closed, we want to record each tab's metrics at the same time. But currently, we log each event as each tab is closed, one by one. This is less accurate because the metrics logged by tabs closed later are affected by the tabs closed earlier.

For example, with these tabs in the tabstrip:

   A.example [MRUIndex: 1]
   B.example [MRUIndex: 3]
   C.example [MRUIndex: 2]
   D.example [MRUIndex: 0]

the MRU order is [D, A, C, B].

If all tabs are closed, we'd want to log each tab's MRUIndex as above. Instead, we log these events as tabs are closed right-to-left:

1. Log D.example [MRUIndex: 0]

With D.example removed, the MRU order becomes [A, C, B].

2. Log C.example [MRUIndex: 1]  # should be 2!

With C.example removed, the MRU order becomes [A, B].

3. Log B.example [MRUIndex: 1]  # should be 3!

With B.example removed, the MRU order becomes [A].

4. Log A.examtple [MRUIndex: 0] # should be 1!

Comment 1 by erikc...@chromium.org, May 7 2018 

Note: I'm in the process of working on a CL that will further break these metrics:
https://chromium-review.googlesource.com/c/chromium/src/+/1045790

When we close all tabs, we first make all changes to the TabStripModel's internal data structures, and then start sending observer notifications [to deal with re-entrancy]. This will make the current implementation report MRUIndex:0 for every tab.

Note that in the new implementation, like the current implementation, calls WillCloseAllTabs() before doing any of this. Listening to that seems like the easiest way to fix the implementation.

Comment 2 by michae...@chromium.org, May 7 2018

Cc: michae...@chromium.org
Owner: charleszhao@chromium.org
I think Charles has started looking at this. Charles -- let me know if you need additional guidance on improving these metrics.

Comment 3 by charleszhao@chromium.org, Jun 5 2018 

I have a draft CL here 
https://chromium-review.googlesource.com/c/chromium/src/+/1086552 

Which calculates and saves all MRUFeatures when WillCloseAllTabs() is invoked.

Will add more tests to ensure it doesn't includes extra bugs.
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 29 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2271b821df065ad7b7cac95f1e1a927f505fc281

commit 2271b821df065ad7b7cac95f1e1a927f505fc281
Author: Charles Zhao <charleszhao@chromium.org>
Date: Fri Jun 29 05:47:51 2018

Fixing MRUFeatures logging when closing_all happens.

(1) MRUFeatures are calculated and saved when WillCloseAllTabs() is
    invoked but returned when the WebContentsDestroyed is called.
(2) MRUFeatures is calculated in WebContentsDestroyed if not in
    closing_all mode.
(3) Change tab_strip_model to respond to WillCloseAllTabs,
    CloseAllTabsCanceled and CloseAllTabsFinished.

Bug:  817174 

Change-Id: Idea15b5c48666fbc0163ccba62ee37b23bb192f0
Reviewed-on: https://chromium-review.googlesource.com/1086552
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Commit-Queue: Charles . <charleszhao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571397}
[modify] https://crrev.com/2271b821df065ad7b7cac95f1e1a927f505fc281/chrome/browser/resource_coordinator/tab_activity_watcher.cc
[modify] https://crrev.com/2271b821df065ad7b7cac95f1e1a927f505fc281/chrome/browser/resource_coordinator/tab_activity_watcher.h
[modify] https://crrev.com/2271b821df065ad7b7cac95f1e1a927f505fc281/chrome/browser/resource_coordinator/tab_activity_watcher_unittest.cc
[modify] https://crrev.com/2271b821df065ad7b7cac95f1e1a927f505fc281/chrome/browser/ui/tabs/tab_strip_model.cc
[modify] https://crrev.com/2271b821df065ad7b7cac95f1e1a927f505fc281/chrome/browser/ui/tabs/tab_strip_model_observer.cc
[modify] https://crrev.com/2271b821df065ad7b7cac95f1e1a927f505fc281/chrome/browser/ui/tabs/tab_strip_model_observer.h
[modify] https://crrev.com/2271b821df065ad7b7cac95f1e1a927f505fc281/chrome/browser/ui/tabs/tab_strip_model_unittest.cc
[modify] https://crrev.com/2271b821df065ad7b7cac95f1e1a927f505fc281/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/2271b821df065ad7b7cac95f1e1a927f505fc281/chrome/browser/ui/views/frame/browser_view.h

Comment 5 by charleszhao@chromium.org, Aug 9

Status: Verified (was: Assigned)
►

Sign in to add a comment