New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 731901 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Email to this user bounced
Closed: Aug 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 3
Type: Feature

Blocking:
issue 731899



Sign in to add a comment

[Tab Metrics] Track FirstPaint, FirstContentfulPaint, and FirstMeaningfulPaint for foreground tab during session restore

Project Member Reported by fmea...@chromium.org, Jun 9 2017

Issue description

Currently we track some notion of paint for foreground tab during session restore, but we want to improve it to report most metrics tracked by the page load metrics system instead.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 2 2017

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

commit 16a6e80782a15c9b228f84b77d4c0ae02a846fa4
Author: Duc Bui <ducbui@google.com>
Date: Wed Aug 02 16:22:53 2017

[TabManager] Keep track of session-restoring tabs.

We need to keep track of tabs which are being restored by session restore (instead of other mechanisms like reopening closed tabs).

This CL adds new API to SessionRestoreObserver and makes TabManager to keep track of the session-restoring tabs.

Bug:  731901 
Change-Id: I0d415a689d2b8e49b58cb2af6623ad7fad7ea50a
Reviewed-on: https://chromium-review.googlesource.com/588168
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Zhen Wang <zhenw@chromium.org>
Commit-Queue: Duc Bui <ducbui@google.com>
Cr-Commit-Position: refs/heads/master@{#491395}
[modify] https://crrev.com/16a6e80782a15c9b228f84b77d4c0ae02a846fa4/chrome/browser/resource_coordinator/tab_manager.cc
[modify] https://crrev.com/16a6e80782a15c9b228f84b77d4c0ae02a846fa4/chrome/browser/resource_coordinator/tab_manager.h
[modify] https://crrev.com/16a6e80782a15c9b228f84b77d4c0ae02a846fa4/chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc
[modify] https://crrev.com/16a6e80782a15c9b228f84b77d4c0ae02a846fa4/chrome/browser/resource_coordinator/tab_manager_web_contents_data.h
[modify] https://crrev.com/16a6e80782a15c9b228f84b77d4c0ae02a846fa4/chrome/browser/resource_coordinator/tab_manager_web_contents_data_unittest.cc
[modify] https://crrev.com/16a6e80782a15c9b228f84b77d4c0ae02a846fa4/chrome/browser/sessions/session_restore.cc
[modify] https://crrev.com/16a6e80782a15c9b228f84b77d4c0ae02a846fa4/chrome/browser/sessions/session_restore.h
[modify] https://crrev.com/16a6e80782a15c9b228f84b77d4c0ae02a846fa4/chrome/browser/sessions/session_restore_observer.h
[modify] https://crrev.com/16a6e80782a15c9b228f84b77d4c0ae02a846fa4/chrome/browser/sessions/session_restore_observer_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Aug 3 2017

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

commit 7f858a6c614f3d69437468301b68338d9b6c64b9
Author: Duc Bui <ducbui@google.com>
Date: Thu Aug 03 19:53:02 2017

Keep track of restored-in-foreground tabs.

Since we want to record UMA for session-restored tabs only when they are
initially foreground, we need to track restored-in-foreground tabs. This
CL adds tracking of tabs which are created by session restore and
initially in foreground.

IsTabRestoredInForeground() API added by this CL will be used in
SessionRestorePageLoadMetricsObserver to measure FP, FCP and FMP for
foreground tabs during session restore (http://crrev.com/2930013005/).

started_in_foreground in PageLoadMetricsObserver::OnStart() cannot be
used to determine whether a tab is initially foreground after it is
created by session restore or not. For example, when browser-side
navigation is disabled and a restored tab is created in background,
switching to it before it loads will yield started_in_foreground as
true.

Bug:  731901 
Change-Id: I58c6c0330bf6121e78d9d9b9cad72729faeb8885
Reviewed-on: https://chromium-review.googlesource.com/599507
Reviewed-by: Zhen Wang <zhenw@chromium.org>
Reviewed-by: Fadi Meawad <fmeawad@chromium.org>
Commit-Queue: Duc Bui <ducbui@google.com>
Cr-Commit-Position: refs/heads/master@{#491821}
[modify] https://crrev.com/7f858a6c614f3d69437468301b68338d9b6c64b9/chrome/browser/resource_coordinator/tab_manager.cc
[modify] https://crrev.com/7f858a6c614f3d69437468301b68338d9b6c64b9/chrome/browser/resource_coordinator/tab_manager.h
[modify] https://crrev.com/7f858a6c614f3d69437468301b68338d9b6c64b9/chrome/browser/resource_coordinator/tab_manager_unittest.cc
[modify] https://crrev.com/7f858a6c614f3d69437468301b68338d9b6c64b9/chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc
[modify] https://crrev.com/7f858a6c614f3d69437468301b68338d9b6c64b9/chrome/browser/resource_coordinator/tab_manager_web_contents_data.h
[modify] https://crrev.com/7f858a6c614f3d69437468301b68338d9b6c64b9/chrome/browser/resource_coordinator/tab_manager_web_contents_data_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 9 2017

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

commit 70c5972eb6758f200696f5a3548d474ee8f9f832
Author: Duc Bui <ducbui@google.com>
Date: Wed Aug 09 15:25:09 2017

Fix OnWillRestoreTab() function call order

OnWillRestoreTab() is intended to be called before any navigations of a
restored WebContents but the original CL
(https://chromium-review.googlesource.com/c/588168) places this function
far from the creation of restored tabs. With browser-side navigation enabled,
the first navigation happens before OnWillRestoreTab(), while without
browser-side navigation, OnWillRestoreTab() is called before the first
navigation as expected. This makes some browser tests in
SessionRestorePageLoadMetricsObserver (https://crrev.com/2930013005) failed
when browser-side navigation is enabled.

This CL moves the call of OnWillRestoreTab() to right after the creation of a
restored WebContents, before any of its navigations.

Bug:  731901 
Change-Id: I8d95fb14b141caffa95ada64838bde363aa10f00
Reviewed-on: https://chromium-review.googlesource.com/601545
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Zhen Wang <zhenw@chromium.org>
Reviewed-by: Fadi Meawad <fmeawad@chromium.org>
Commit-Queue: Duc Bui <ducbui@google.com>
Cr-Commit-Position: refs/heads/master@{#492992}
[modify] https://crrev.com/70c5972eb6758f200696f5a3548d474ee8f9f832/chrome/browser/sessions/session_restore.cc
[modify] https://crrev.com/70c5972eb6758f200696f5a3548d474ee8f9f832/chrome/browser/sessions/session_restore.h
[add] https://crrev.com/70c5972eb6758f200696f5a3548d474ee8f9f832/chrome/browser/sessions/session_restore_observer_browsertest.cc
[modify] https://crrev.com/70c5972eb6758f200696f5a3548d474ee8f9f832/chrome/browser/ui/browser_live_tab_context.cc
[modify] https://crrev.com/70c5972eb6758f200696f5a3548d474ee8f9f832/chrome/browser/ui/browser_tabrestore.cc
[modify] https://crrev.com/70c5972eb6758f200696f5a3548d474ee8f9f832/chrome/browser/ui/browser_tabrestore.h
[modify] https://crrev.com/70c5972eb6758f200696f5a3548d474ee8f9f832/chrome/test/BUILD.gn

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 10 2017

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

commit 2fd90c9e304afdadb3b95cd810df485098793e4c
Author: ducbui <ducbui@google.com>
Date: Thu Aug 10 19:24:11 2017

[Tab Metrics] Measure FP, FCP and FMP for Foreground Tabs during Session Restore

The CL involves:
- Creating a SessionRestorePageLoadMetricsObserver to record FP, FCP and FMP
for foreground tabs during session restore.

This CL depends on another:
Keep track of session-restoring tabs (https://chromium-review.googlesource.com/c/588168).
Keep track of restored-in-foreground tabs (https://chromium-review.googlesource.com/c/599507).
Add signals that mark the start and end of session restore
(https://crrev.com/2935183002/).

BUG= 731901 

Review-Url: https://codereview.chromium.org/2930013005
Cr-Commit-Position: refs/heads/master@{#493494}

[modify] https://crrev.com/2fd90c9e304afdadb3b95cd810df485098793e4c/chrome/browser/BUILD.gn
[add] https://crrev.com/2fd90c9e304afdadb3b95cd810df485098793e4c/chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc
[add] https://crrev.com/2fd90c9e304afdadb3b95cd810df485098793e4c/chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.h
[add] https://crrev.com/2fd90c9e304afdadb3b95cd810df485098793e4c/chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/2fd90c9e304afdadb3b95cd810df485098793e4c/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
[modify] https://crrev.com/2fd90c9e304afdadb3b95cd810df485098793e4c/chrome/browser/page_load_metrics/page_load_metrics_initialize.cc
[modify] https://crrev.com/2fd90c9e304afdadb3b95cd810df485098793e4c/chrome/test/BUILD.gn
[modify] https://crrev.com/2fd90c9e304afdadb3b95cd810df485098793e4c/tools/metrics/histograms/histograms.xml

Project Member

Comment 5 by bugdroid1@chromium.org, Aug 15 2017

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

commit 0c11773b9e0e7b6cc580c6e824aa80e03a5e6826
Author: Duc Bui <ducbui@google.com>
Date: Tue Aug 15 17:51:27 2017

[Tab Metrics] Enhance SessionRestorePageLoadMetricsObserver

This CL add more conditions to SessionRestorePageLoadMetricsObserver to
check and filter out corner cases.

Bug:  731901 
Change-Id: I5464f293dc9e6c3b58dfeeca9e22ab3a248e2361
Reviewed-on: https://chromium-review.googlesource.com/612592
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Commit-Queue: Duc Bui <ducbui@google.com>
Cr-Commit-Position: refs/heads/master@{#494439}
[modify] https://crrev.com/0c11773b9e0e7b6cc580c6e824aa80e03a5e6826/chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc
[modify] https://crrev.com/0c11773b9e0e7b6cc580c6e824aa80e03a5e6826/chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.h
[modify] https://crrev.com/0c11773b9e0e7b6cc580c6e824aa80e03a5e6826/chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Aug 17 2017

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

commit 844e26c176db3c589099d36b2677d25088726bb5
Author: Duc Bui <ducbui@google.com>
Date: Thu Aug 17 05:55:25 2017

[Tab Metrics] Fix flaky browser tests SessionRestorePageLoadMetricsBrowserTest

Some tests in SessionRestorePageLoadMetricsBrowserTest fail:
RestoreForeignSession and MultipleTabsSessionRestore.
https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/62231
https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/62288


Reason for the failure: PageLoadMetricsWaiter created for a tab must have the
tab committed where did_add_observer_ becomes true. If the tab has not committed
before the Waiter is destroyed, did_add_observer_ is still false, and
DCHECK(did_add_observer_) in its destructor will fail.

The above two tests are the only one that load a tab in background but the
tests did not wait for the background tab to commit.

This CL add WaitForTabsToLoad() to the tests to ensure all restored tabs get
committed before the tests end.

Bug:  731901 
Change-Id: Idfdfc1ad6c9596f5abe7d3cc05315e0f74efabee
Reviewed-on: https://chromium-review.googlesource.com/614467
Reviewed-by: Fadi Meawad <fmeawad@chromium.org>
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Commit-Queue: Duc Bui <ducbui@google.com>
Cr-Commit-Position: refs/heads/master@{#495087}
[modify] https://crrev.com/844e26c176db3c589099d36b2677d25088726bb5/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc

Comment 7 by holte@chromium.org, Aug 22 2017

Components: -Internals>Metrics
Labels: Hotlist-Metrics

Comment 8 by ducbui@google.com, Aug 25 2017

Status: Fixed (was: Started)

Sign in to add a comment