Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2930013005:
[Tab Metrics] Measure FP, FCP and FMP for Foreground Tab during Session Restore (Closed)
Description[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}
Committed: https://chromium.googlesource.com/chromium/src/+/2fd90c9e304afdadb3b95cd810df485098793e4c
Patch Set 1 #
Total comments: 11
Patch Set 2 : Revise SessionRestorePageLoadMetricsObserver #
Total comments: 4
Patch Set 3 : Simplify conditions to track foreground tabs during session restore. #Patch Set 4 : Prune cases where a tab is loaded only after user switches to it. #
Total comments: 5
Patch Set 5 : Add a unittest and remove #if when adding the observer. #
Total comments: 10
Patch Set 6 : Reset the initial-foreground-tab-changed flag for each session restore. #
Total comments: 6
Patch Set 7 : Rename and revise SessionRestoreForegroundTabPageLoadMetricsObserver #Patch Set 8 : Remove unnecessary DCHECK(tab_manager) #Patch Set 9 : Add browsertest for SessionRestorePageLoadMetricsObserver. #
Total comments: 12
Patch Set 10 : Change the comment on initial_session_restore_foreground_tab_changed_. #
Total comments: 2
Patch Set 11 : Filter out some corner-case page loads in session restore. #Patch Set 12 : Fix unittests after revising the observer's conditions in previous patches. #Patch Set 13 : Disable the observer when browser-side navigation is enabled. #Patch Set 14 : Fix browser tests on CHROMEOS #Patch Set 15 : Add missing includes on CHROMEOS #Patch Set 16 : Fix tab_manager null on Android. #
Total comments: 2
Patch Set 17 : Revise session restore start signal. #
Total comments: 2
Patch Set 18 : Revert session restore signal and revise Session Restore Observer conditions. #Patch Set 19 : Remove content::IsBrowserSideNavigationEnabled() from unit tests. #
Total comments: 29
Patch Set 20 : Revise after changes in signals and started_in_foreground bug fixes. #Patch Set 21 : Use NavigationHandle's RestoreType in OnStart() #
Total comments: 3
Patch Set 22 : Add is_used_for_session_restore in Browser. #Patch Set 23 : Correct the position of DCHECK in OnStart(). #
Total comments: 41
Patch Set 24 : Filter out navigations which are not from last session. #Patch Set 25 : Use RestoreOrigin for WebContents. #Patch Set 26 : Add some missing includes and move the Observer constructor to the top #Patch Set 27 : Add RemoveFromWebContents() to RestoreOrigin. #
Total comments: 2
Patch Set 28 : Use new API to track session restore tabs #
Total comments: 2
Patch Set 29 : Refine tests. #Patch Set 30 : Rebase #Patch Set 31 : Exclude this observer from Android #Patch Set 32 : Refine tests #
Total comments: 57
Patch Set 33 : Address some comments #Patch Set 34 : Change testing URLs. #Patch Set 35 : Fix a typo #
Total comments: 12
Patch Set 36 : Fix compilation on ChromeOS #Patch Set 37 : Address some comments #Patch Set 38 : Fix ASAN tests #
Total comments: 10
Patch Set 39 : Refine tests #Patch Set 40 : Address reviewers comments #Patch Set 41 : Remove Stop() #Patch Set 42 : Refine test #
Total comments: 12
Patch Set 43 : Address Bryan comments #
Total comments: 2
Messages
Total messages: 211 (114 generated)
Description was changed from ========== Functions for checking is-during-session-restore and page load metrics of foreground tabs during session restore. An interface to know whether we are in session restore or not was added: TabManager::IsDuringSessionRestore() The start and end of session restore are determined by TabManager's whose lifetime is equal to session restore. Page load metrics of foreground tabs during session restore is tracked by a new PageLoadMetricsObserver. BUG= ========== to ========== Functions for checking is-during-session-restore and page load metrics of foreground tabs during session restore. An interface to know whether we are in session restore or not was added: TabManager::IsDuringSessionRestore() The start and end of session restore are determined by TabManager's whose lifetime is equal to session restore. Page load metrics of foreground tabs during session restore is tracked by a new PageLoadMetricsObserver. BUG= ==========
ducbui@google.com changed reviewers: + fmeawad@chromium.org
PTAL
The subject should be limited to 80 characters, also file a BUG and associate it to the CL. Reading the code now.
Overall the cl looks good, left some comments. https://codereview.chromium.org/2930013005/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h (right): https://codereview.chromium.org/2930013005/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h:13: // it started This comment is confusing, I would simplify it to the first line only. https://codereview.chromium.org/2930013005/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h:14: class SessionRestoreForegroundTabPageLoadMetricsObserver I think the observer should be for all situations regarding the session restore, it does not make sense that we would have an observer for foreground tab and another for background tab. Maybe add a method like: isForegroundTab similar to isDuringSessionRestore https://codereview.chromium.org/2930013005/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2930013005/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.h:157: void StartSessionRestore() { is_during_session_restore_ = true; } nit: SessionRestoreStarted since we are not actually starting it, but instead observing the start https://codereview.chromium.org/2930013005/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.h:159: void EndSessionRestore() { is_during_session_restore_ = false; } ditto
Description was changed from ========== Functions for checking is-during-session-restore and page load metrics of foreground tabs during session restore. An interface to know whether we are in session restore or not was added: TabManager::IsDuringSessionRestore() The start and end of session restore are determined by TabManager's whose lifetime is equal to session restore. Page load metrics of foreground tabs during session restore is tracked by a new PageLoadMetricsObserver. BUG= ========== to ========== Is-during-session-restore check + metrics of foreground tabs in session restore An interface to know whether we are in session restore or not was added: TabManager::IsDuringSessionRestore() The start and end of session restore are determined by TabManager's whose lifetime is equal to session restore. Page load metrics of foreground tabs during session restore is tracked by a new PageLoadMetricsObserver. BUG= ==========
Description was changed from ========== Is-during-session-restore check + metrics of foreground tabs in session restore An interface to know whether we are in session restore or not was added: TabManager::IsDuringSessionRestore() The start and end of session restore are determined by TabManager's whose lifetime is equal to session restore. Page load metrics of foreground tabs during session restore is tracked by a new PageLoadMetricsObserver. BUG= ========== to ========== Is-during-session-restore check + metrics of foreground tabs in session restore An interface to know whether we are in session restore or not was added: TabManager::IsDuringSessionRestore() The start and end of session restore are determined by TabManager's whose lifetime is equal to session restore. Page load metrics of foreground tabs during session restore is tracked by a new PageLoadMetricsObserver. BUG=e in session restore or not was added: TabManager::IsDuringSessionRestore() The start and end of session restore are determined by TabManager's whose lifetime is equal to session restore. Page load metrics of foreground tabs during session restore is tracked by a new PageLoadMetricsObserver. BUG=731901 ==========
Description was changed from ========== Is-during-session-restore check + metrics of foreground tabs in session restore An interface to know whether we are in session restore or not was added: TabManager::IsDuringSessionRestore() The start and end of session restore are determined by TabManager's whose lifetime is equal to session restore. Page load metrics of foreground tabs during session restore is tracked by a new PageLoadMetricsObserver. BUG=e in session restore or not was added: TabManager::IsDuringSessionRestore() The start and end of session restore are determined by TabManager's whose lifetime is equal to session restore. Page load metrics of foreground tabs during session restore is tracked by a new PageLoadMetricsObserver. BUG=731901 ========== to ========== [Tab Metrics] Measure FP, FCP and FMP for Foreground Tab during Session Restore The CL involves: - Informing the TabManager when a session restore start/ends - Creating a SessionRestorePageLoadMetricsObserver to record FP, FCP and FMP for foreground tab during session restore. BUG=731901 ==========
https://codereview.chromium.org/2930013005/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.cc:68: if (tab_manager) Are we expecting to always have a tab_manager, if so then this should be a DCHECK instead, if not, then maybe we are initializing this too early.
On 2017/06/09 21:22:27, fmeawad wrote: > https://codereview.chromium.org/2930013005/diff/1/chrome/browser/page_load_me... > File > chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.cc > (right): > > https://codereview.chromium.org/2930013005/diff/1/chrome/browser/page_load_me... > chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.cc:68: > if (tab_manager) > Are we expecting to always have a tab_manager, if so then this should be a > DCHECK instead, if not, then maybe we are initializing this too early. GetTabManager() is expected to have a non-nullptr return on WIN, MAC_OSX and LINUX. That is to say, as long as we support tab manager on that platform, the return should not be nullptr. Maybe we don't want to add this observer to page load tracker on other platforms, like android?
https://codereview.chromium.org/2930013005/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.cc:68: if (tab_manager) On 2017/06/09 21:22:27, fmeawad wrote: > Are we expecting to always have a tab_manager, if so then this should be a > DCHECK instead, if not, then maybe we are initializing this too early. I think the check is necessary because the tab_manager is null on non-supported platforms such as Android (see https://cs.chromium.org/chromium/src/chrome/browser/browser_process.h?type=cs...). https://codereview.chromium.org/2930013005/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h (right): https://codereview.chromium.org/2930013005/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h:13: // it started On 2017/06/09 21:02:54, fmeawad wrote: > This comment is confusing, I would simplify it to the first line only. Done. https://codereview.chromium.org/2930013005/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h:14: class SessionRestoreForegroundTabPageLoadMetricsObserver On 2017/06/09 21:02:54, fmeawad wrote: > I think the observer should be for all situations regarding the session restore, > it does not make sense that we would have an observer for foreground tab and > another for background tab. Maybe add a method like: isForegroundTab similar to > isDuringSessionRestore I changed the class name into a more general one SessionRestorePageLoadMetricsObserver and revised its methods accordingly. https://codereview.chromium.org/2930013005/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2930013005/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.h:157: void StartSessionRestore() { is_during_session_restore_ = true; } On 2017/06/09 21:02:54, fmeawad wrote: > nit: SessionRestoreStarted since we are not actually starting it, but instead > observing the start I change the name to MarkSessionRestoreStarted() to distinguish it with a check. https://codereview.chromium.org/2930013005/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.h:159: void EndSessionRestore() { is_during_session_restore_ = false; } On 2017/06/09 21:02:54, fmeawad wrote: > ditto Done.
https://codereview.chromium.org/2930013005/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.cc:68: if (tab_manager) On 2017/06/10 00:40:16, ducbui wrote: > On 2017/06/09 21:22:27, fmeawad wrote: > > Are we expecting to always have a tab_manager, if so then this should be a > > DCHECK instead, if not, then maybe we are initializing this too early. > > I think the check is necessary because the tab_manager is null on non-supported > platforms such as Android (see > https://cs.chromium.org/chromium/src/chrome/browser/browser_process.h?type=cs...). I see, then maybe exclude this code on non-supported devices using ifdefs? If this is a common pattern then leave it as is, otherwise use ifdefs
On 2017/06/12 14:56:40, fmeawad wrote: > https://codereview.chromium.org/2930013005/diff/1/chrome/browser/page_load_me... > File > chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.cc > (right): > > https://codereview.chromium.org/2930013005/diff/1/chrome/browser/page_load_me... > chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.cc:68: > if (tab_manager) > On 2017/06/10 00:40:16, ducbui wrote: > > On 2017/06/09 21:22:27, fmeawad wrote: > > > Are we expecting to always have a tab_manager, if so then this should be a > > > DCHECK instead, if not, then maybe we are initializing this too early. > > > > I think the check is necessary because the tab_manager is null on > non-supported > > platforms such as Android (see > > > https://cs.chromium.org/chromium/src/chrome/browser/browser_process.h?type=cs...). > > I see, then maybe exclude this code on non-supported devices using ifdefs? If > this is a common pattern then leave it as is, otherwise use ifdefs Sorry I missed lpy@ feedback, I think he said it better.
zhenw@chromium.org changed reviewers: + zhenw@chromium.org
https://codereview.chromium.org/2930013005/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:28: return CONTINUE_OBSERVING; I think you should return STOP_OBSERVING when this page starts in background? And when OnHidden() is called, also just return STOP_OBSERVING? (And no need to override OnShown). The above suggests to just observe a very simple use case: A tab starts in foreground until it is loaded. Once that tab becomes background, stops observing. My reasoning here is that it is very complicated when a tab changes its visibility: 1. We only care about the tab starting in foreground, as suggested by your UMA name containing "ForegroundTab". 2. When a foreground tab turns into background, the browser may not behave as you expect, e.g., a tab will not paint when invisible. Maybe there are other things, too? So you may not get FP/FCP/FMP or you get affected numbers. https://codereview.chromium.org/2930013005/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:69: if (IsDuringSessionRestore() && is_foreground_tab_) { Instead of calling IsDuringSessionRestore() when receiving FP/FCP/FMP, you may want to check that when SessionRestorePageLoadMetricsObserver is constructed and have a SessionRestorePageLoadMetricsObserver::CreateIfNeeded similar to other observers.
https://codereview.chromium.org/2930013005/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:69: if (IsDuringSessionRestore() && is_foreground_tab_) { On 2017/06/12 16:35:26, Zhen Wang wrote: > Instead of calling IsDuringSessionRestore() when receiving FP/FCP/FMP, you may > want to check that when SessionRestorePageLoadMetricsObserver is constructed and > have a SessionRestorePageLoadMetricsObserver::CreateIfNeeded similar to other > observers. Oh, And when I say "check that when SessionRestorePageLoadMetricsObserver is constructed", I actually mean check that in CreateIfNeeded and not creating it if not need to. Sorry for the confusion...
On 2017/06/12 16:35:26, Zhen Wang wrote: > https://codereview.chromium.org/2930013005/diff/20001/chrome/browser/page_loa... > File > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc > (right): > > https://codereview.chromium.org/2930013005/diff/20001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:28: > return CONTINUE_OBSERVING; > I think you should return STOP_OBSERVING when this page starts in background? > > And when OnHidden() is called, also just return STOP_OBSERVING? (And no need to > override OnShown). I tried this approach but a foreground tab can become background, then foreground again. Should we record the metrics of the tab in this case? > The above suggests to just observe a very simple use case: A tab starts in > foreground until it is loaded. Once that tab becomes background, stops > observing. > > My reasoning here is that it is very complicated when a tab changes its > visibility: > 1. We only care about the tab starting in foreground, as suggested by your UMA > name containing "ForegroundTab". > 2. When a foreground tab turns into background, the browser may not behave as > you expect, e.g., a tab will not paint when invisible. Maybe there are other > things, too? So you may not get FP/FCP/FMP or you get affected numbers. I agree that things get complicated when tabs change their visibility and I could not figure out how to do in these cases. In a short discussion last week, lpy@ suggested that I might record the metrics for all tabs which are or become foreground during session restore. Because we want to measure the delay for a user to see the web page, how about using the metrics foreground-to-[FP,FCP,FMP] (http://crrev.com/2930583002)?
On 2017/06/12 17:02:47, ducbui wrote: > On 2017/06/12 16:35:26, Zhen Wang wrote: > > > https://codereview.chromium.org/2930013005/diff/20001/chrome/browser/page_loa... > > File > > > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc > > (right): > > > > > https://codereview.chromium.org/2930013005/diff/20001/chrome/browser/page_loa... > > > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:28: > > return CONTINUE_OBSERVING; > > I think you should return STOP_OBSERVING when this page starts in background? > > > > And when OnHidden() is called, also just return STOP_OBSERVING? (And no need > to > > override OnShown). > > I tried this approach but a foreground tab can become background, then > foreground again. > Should we record the metrics of the tab in this case? I think we should ignore this one. Fadi can make the judgement call on this. :) > > > The above suggests to just observe a very simple use case: A tab starts in > > foreground until it is loaded. Once that tab becomes background, stops > > observing. > > > > My reasoning here is that it is very complicated when a tab changes its > > visibility: > > 1. We only care about the tab starting in foreground, as suggested by your UMA > > name containing "ForegroundTab". > > 2. When a foreground tab turns into background, the browser may not behave as > > you expect, e.g., a tab will not paint when invisible. Maybe there are other > > things, too? So you may not get FP/FCP/FMP or you get affected numbers. > > I agree that things get complicated when tabs change their visibility and I > could not > figure out how to do in these cases. In a short discussion last week, lpy@ > suggested that I > might record the metrics for all tabs which are or become foreground during > session restore. I think you are mixing 2 metrics into one: 1. ForegroundTab.FP/FCP/FMP 2. TabSwitchLoadTime We do want to capture both. In this CL, I think you are adding the 1st one. The 2nd one is when a tab was started as background, and then turns into foreground. That is a different metric. > Because we want to measure the delay for a user to see the web page, how about > using the metrics foreground-to-[FP,FCP,FMP] (http://crrev.com/2930583002) Kunihiko's CL does not capture if it is in session restore. As a side note, when replying comments next time, try to reply it by replying from the code (not from email/message), so it is easier to read. :)
On 2017/06/12 16:41:03, Zhen Wang wrote: > https://codereview.chromium.org/2930013005/diff/20001/chrome/browser/page_loa... > File > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc > (right): > > https://codereview.chromium.org/2930013005/diff/20001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:69: > if (IsDuringSessionRestore() && is_foreground_tab_) { > On 2017/06/12 16:35:26, Zhen Wang wrote: > > Instead of calling IsDuringSessionRestore() when receiving FP/FCP/FMP, you may > > want to check that when SessionRestorePageLoadMetricsObserver is constructed > and > > have a SessionRestorePageLoadMetricsObserver::CreateIfNeeded similar to other > > observers. > > Oh, And when I say "check that when SessionRestorePageLoadMetricsObserver is > constructed", I actually mean check that in CreateIfNeeded and not creating it > if not need to. Sorry for the confusion... That's a good point and it will save some processing time. However, should I record metrics in the case that a user opens a new tab A, then reopens closed tabs, then gets back to the newly opened tab A which finally gets loaded on the foreground? In this case, the tab A was created before the session restore, but get loaded on foreground during the session restore.
panicker@chromium.org changed reviewers: + panicker@chromium.org
https://codereview.chromium.org/2930013005/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:58: if (IsDuringSessionRestore() && is_foreground_tab_) { Do FCP, FMP even fire on background tabs? Once we move to the updated signal (swap time) it will not fire unless pixels were shipped to screen. How about checking / updating the signal and removing all the complexity of mainting shown / hidden etc? (Here's the CL that updates web perf APIs, we also need to update the UMAs etc https://codereview.chromium.org/2932593002/ )
The CQ bit was checked by fmeawad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chrisha@chromium.org changed reviewers: + chrisha@chromium.org
Overall this is moving in the right direction, but it's slightly more complicated: multiple session restores can be ongoing simultaneously, as session restores are tied to a browser. I think we'll need to decorate them with an ID of sorts, so that we can associate tabs to a session restore and track simultaneous session restores independently... https://codereview.chromium.org/2930013005/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right): https://codereview.chromium.org/2930013005/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:133: #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX) OS_CHROMEOS as well? https://codereview.chromium.org/2930013005/diff/60001/chrome/browser/sessions... File chrome/browser/sessions/tab_loader.cc (right): https://codereview.chromium.org/2930013005/diff/60001/chrome/browser/sessions... chrome/browser/sessions/tab_loader.cc:112: tab_manager->MarkSessionRestoreStarted(); This will fail if their are multiple session restores started across multiple profiles that overlap. We need a counter for the number of concurrent session restores that are ongoing. More likely, we'll need to tag WebContents with a kind of session restore ID so we know that which session restore a tab is associated with...
> complicated: multiple session restores can be ongoing simultaneously, as session > restores are tied to a browser. Tied to a *profile*, that is.
Description was changed from ========== [Tab Metrics] Measure FP, FCP and FMP for Foreground Tab during Session Restore The CL involves: - Informing the TabManager when a session restore start/ends - Creating a SessionRestorePageLoadMetricsObserver to record FP, FCP and FMP for foreground tab during session restore. BUG=731901 ========== to ========== [Tab Metrics] Measure FP, FCP and FMP for Foreground Tab during Session Restore The CL involves: - Informing the TabManager when a session restore start/ends - Creating a SessionRestorePageLoadMetricsObserver to record FP, FCP and FMP for foreground tab during session restore. This CL depends on another one which must be landed first: Add signals that mark the start and end of session restore (https://codereview.chromium.org/2935183002/) BUG=731901 ==========
Description was changed from ========== [Tab Metrics] Measure FP, FCP and FMP for Foreground Tab during Session Restore The CL involves: - Informing the TabManager when a session restore start/ends - Creating a SessionRestorePageLoadMetricsObserver to record FP, FCP and FMP for foreground tab during session restore. This CL depends on another one which must be landed first: Add signals that mark the start and end of session restore (https://codereview.chromium.org/2935183002/) BUG=731901 ========== to ========== [Tab Metrics] Measure FP, FCP and FMP for Foreground Tab during Session Restore The CL involves: - Informing the TabManager when a session restore start/ends - Creating a SessionRestorePageLoadMetricsObserver to record FP, FCP and FMP for foreground tab during session restore. This CL depends on another one which must be landed first: Add signals that mark the start and end of session restore (https://codereview.chromium.org/2935183002/) BUG=731901 ==========
Description was changed from ========== [Tab Metrics] Measure FP, FCP and FMP for Foreground Tab during Session Restore The CL involves: - Informing the TabManager when a session restore start/ends - Creating a SessionRestorePageLoadMetricsObserver to record FP, FCP and FMP for foreground tab during session restore. This CL depends on another one which must be landed first: Add signals that mark the start and end of session restore (https://codereview.chromium.org/2935183002/) BUG=731901 ========== to ========== [Tab Metrics] Measure FP, FCP and FMP for Foreground Tab during Session Restore The CL involves: - Informing the TabManager when a session restore start/ends - Creating a SessionRestorePageLoadMetricsObserver to record FP, FCP and FMP for foreground tab during session restore. This CL depends on another one which must be landed first: Add signals that mark the start and end of session restore (https://crrev.com/2935183002/) BUG=731901 ==========
https://codereview.chromium.org/2930013005/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right): https://codereview.chromium.org/2930013005/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:133: #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX) On 2017/06/15 19:44:56, chrisha wrote: > OS_CHROMEOS as well? TabManager seems not supported on CHROMEOS. BrowserProcessImpl::GetTabManager() returns non-null only if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX) (https://cs.chromium.org/chromium/src/chrome/browser/browser_process_impl.cc?l...) Does OS_LINUX include OS_CHROMEOS?
https://codereview.chromium.org/2930013005/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right): https://codereview.chromium.org/2930013005/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:133: #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX) It is supported on ChromeOS, so yes I suppose OS_LINUX must subsume OS_CHROMEOS. I dislike the fragility of replicating the #ifdef logic here as well. Can we make the code compile on all platforms and make CreateIfNeeded return null when not necessary?
https://codereview.chromium.org/2930013005/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right): https://codereview.chromium.org/2930013005/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:133: #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX) On 2017/06/16 16:52:35, chrisha wrote: > It is supported on ChromeOS, so yes I suppose OS_LINUX must subsume OS_CHROMEOS. > > I dislike the fragility of replicating the #ifdef logic here as well. Can we > make the code compile on all platforms and make CreateIfNeeded return null when > not necessary? That's a good point. I would remove the #if and make CreateIfNeeded() return null when TabManager is not supported (i.e., BrowserProcess::GetTabManager() returns null).
Hi, I created a unittest for the PageLoadMetricsObserver. I also added a dependency to the session restore signal CL https://crrev.com/2935183002/ into the new patchset. PTAL. Thanks.
https://codereview.chromium.org/2930013005/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h (right): https://codereview.chromium.org/2930013005/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h:21: // Record page load metrics of foreground tabs during session restore Missing period. https://codereview.chromium.org/2930013005/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h:30: ObservePolicy OnStart(content::NavigationHandle* navigation_handle, These functions are all implementations of the PageLoadMetricsObserver interface? Then please comment with // PageLoadMetricsObserver implementation: https://codereview.chromium.org/2930013005/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2930013005/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer_unittest.cc:101: // No paint timings recorded because the initial active tab was changed Missing period. https://codereview.chromium.org/2930013005/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer_unittest.cc:120: // Set the tab to background before the PageLoadMetricsObserver was created Ditto, for all comments in this file. https://codereview.chromium.org/2930013005/diff/80001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager.cc (right): https://codereview.chromium.org/2930013005/diff/80001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.cc:799: initial_active_tab_changed_ = true; This logic seems frail. If this is specifically tied to session restore, then it seems like we should be resetting this when OnSessionRestoreStarted is called, so that it tracks the initial foreground tab associated with a session restore. Note that session restore can be invoked at anytime, not just at browser startup. The more I think about this, the more I like sky's idea of session restore itself exposing an observer interface, and then tracking all of this state directly in the PageLoadMetricObserver, which would also be a SessionRestoreObserver and a TabStripModelObserver. Why do we need TabManager as the middle man? Other than for these metrics, TabManager doesn't explicitly care that the initially visible tab related to a session restore has since been changed away from... The session restore observer should be able to share everything about an individual session restore, including when it starts, the tabs it is restoring (either as WebContents, or TabStripModels and tab IDs), and when it ends. There is already something like this: SessionRestoreStatsCollector. We could be adding these metrics directly to that class, or alternatively, abstracting out a SessionRestoreObserver helper from that class, and having two separate clients.
This patch set addresses chrisha@'s comments and syncs with the depending CL (SessionRestore signals) which has just landed. chrisha@: Could you PTAL? Thanks. https://codereview.chromium.org/2930013005/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h (right): https://codereview.chromium.org/2930013005/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h:21: // Record page load metrics of foreground tabs during session restore On 2017/06/21 15:00:25, chrisha wrote: > Missing period. Done. https://codereview.chromium.org/2930013005/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h:30: ObservePolicy OnStart(content::NavigationHandle* navigation_handle, On 2017/06/21 15:00:25, chrisha wrote: > These functions are all implementations of the PageLoadMetricsObserver > interface? Then please comment with > > // PageLoadMetricsObserver implementation: Done. https://codereview.chromium.org/2930013005/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2930013005/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer_unittest.cc:101: // No paint timings recorded because the initial active tab was changed On 2017/06/21 15:00:26, chrisha wrote: > Missing period. Done. https://codereview.chromium.org/2930013005/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer_unittest.cc:120: // Set the tab to background before the PageLoadMetricsObserver was created On 2017/06/21 15:00:26, chrisha wrote: > Ditto, for all comments in this file. Done. https://codereview.chromium.org/2930013005/diff/80001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager.cc (right): https://codereview.chromium.org/2930013005/diff/80001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.cc:799: initial_active_tab_changed_ = true; On 2017/06/21 15:00:26, chrisha wrote: > This logic seems frail. If this is specifically tied to session restore, then it > seems like we should be resetting this when OnSessionRestoreStarted is called, > so that it tracks the initial foreground tab associated with a session restore. > > Note that session restore can be invoked at anytime, not just at browser > startup. Acknowledged. I reset the flag for each session restore. > > The more I think about this, the more I like sky's idea of session restore > itself exposing an observer interface, and then tracking all of this state > directly in the PageLoadMetricObserver, which would also be a > SessionRestoreObserver and a TabStripModelObserver. Why do we need TabManager as > the middle man? Other than for these metrics, TabManager doesn't explicitly care > that the initially visible tab related to a session restore has since been > changed away from... > > The session restore observer should be able to share everything about an > individual session restore, including when it starts, the tabs it is restoring > (either as WebContents, or TabStripModels and tab IDs), and when it ends. > > There is already something like this: SessionRestoreStatsCollector. We could be > adding these metrics directly to that class, or alternatively, abstracting out a > SessionRestoreObserver helper from that class, and having two separate clients.
lgtm
lpy@chromium.org changed reviewers: + lpy@chromium.org
session_restore_foreground_tab_page_load_metrics_observer.h -> session_restore_page_load_metrics_observer.h https://codereview.chromium.org/2930013005/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h (right): https://codereview.chromium.org/2930013005/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h:22: class SessionRestoreForegroundTabPageLoadMetricsObserver SessionRestorePageLoadMetricsObserver https://codereview.chromium.org/2930013005/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h:52: static resource_coordinator::TabManager* GetTabManager(); Why do we need this method instead of inlining into call site?
https://codereview.chromium.org/2930013005/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h (right): https://codereview.chromium.org/2930013005/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h:22: class SessionRestoreForegroundTabPageLoadMetricsObserver On 2017/06/27 19:57:55, lpy wrote: > SessionRestorePageLoadMetricsObserver +1
https://codereview.chromium.org/2930013005/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h (right): https://codereview.chromium.org/2930013005/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h:22: class SessionRestoreForegroundTabPageLoadMetricsObserver On 2017/06/27 20:26:09, fmeawad wrote: > On 2017/06/27 19:57:55, lpy wrote: > > SessionRestorePageLoadMetricsObserver > > +1 Done. https://codereview.chromium.org/2930013005/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h:52: static resource_coordinator::TabManager* GetTabManager(); On 2017/06/27 19:57:55, lpy wrote: > Why do we need this method instead of inlining into call site? Acknowledged. I am fine with your comment which makes the code shorter. I wrote those functions to clarify my intentions, but they seems unnecessary here.
https://codereview.chromium.org/2930013005/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h (right): https://codereview.chromium.org/2930013005/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_foreground_tab_page_load_metrics_observer.h:22: class SessionRestoreForegroundTabPageLoadMetricsObserver On 2017/06/27 21:04:13, ducbui wrote: > On 2017/06/27 20:26:09, fmeawad wrote: > > On 2017/06/27 19:57:55, lpy wrote: > > > SessionRestorePageLoadMetricsObserver > > > > +1 > > Done. I also removed the DCHECK() because if we are not on TabManager-supported platforms, this function will return false.
lpy@chromium.org changed reviewers: + bmcquade@chromium.org, mpearson@chromium.org
lgtm Bryan, could you please take a look at the page load metrics observer? Mark, could you please take a look at histograms.xml?
histograms.xml lgtm
Please update the description as this CL no longer "Informing the TabManager when a session restore start/ends"
Description was changed from ========== [Tab Metrics] Measure FP, FCP and FMP for Foreground Tab during Session Restore The CL involves: - Informing the TabManager when a session restore start/ends - Creating a SessionRestorePageLoadMetricsObserver to record FP, FCP and FMP for foreground tab during session restore. This CL depends on another one which must be landed first: Add signals that mark the start and end of session restore (https://crrev.com/2935183002/) BUG=731901 ========== to ========== [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 tab during session restore. This CL depends on another: Add signals that mark the start and end of session restore (https://crrev.com/2935183002/) BUG=731901 ==========
Description was changed from ========== [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 tab during session restore. This CL depends on another: Add signals that mark the start and end of session restore (https://crrev.com/2935183002/) BUG=731901 ========== to ========== [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: Add signals that mark the start and end of session restore (https://crrev.com/2935183002/) BUG=731901 ==========
On 2017/06/28 00:25:18, fmeawad wrote: > Please update the description as this CL no longer "Informing the TabManager > when a session restore start/ends" Issue description updated.
On 2017/06/28 at 00:32:04, ducbui wrote: > On 2017/06/28 00:25:18, fmeawad wrote: > > Please update the description as this CL no longer "Informing the TabManager > > when a session restore start/ends" > > Issue description updated. This change looks very good, thanks for adding this code! My main question is around the relative timing of the conditions that decide whether we should construct the observer. You have: tab_manager->IsSessionRestoreLoadingTabs() && !tab_manager->HasSessionRestoreInitialForegroundTabChanged() SessionRestorePageLoadMetricsObserver::CreateIfNeeded() will be invoked as the main frame navigation for each tab is starting. So we just need to be sure that: 1. this is guaranteed to never be invoked before the tab manager session restore state has been properly updated to reflect that we are in session restore mode 2. these conditions are never met for navigations that happen after session restore is complete RE: 1, when does the initialization logic to indicate that IsSessionRestoreLoadingTabs is true and HasSessionRestoreInitialForegroundTabChanged is false run? Is this part of tab helper construction? Earlier? Later? RE: 2, when does the logic to flip either IsSessionRestoreLoadingTabs to false or HasSessionRestoreInitialForegroundTabChanged to true run? Given the potential subtleties around ordering here, and possibility for regression if ordering changes in the future, it would be good to add a browsertest in addition to the unittests, to verify expected order of execution holds in the future. Would it be possible to add a browsertest in page_load_metrics_browsertest.cc that verifies that (1) metrics are properly recorded for page loads during session restore, and (2) metrics are not recorded for page loads that are initiated after session restore is complete? Thanks!
> RE: 1, when does the initialization logic to indicate that > IsSessionRestoreLoadingTabs is true and > HasSessionRestoreInitialForegroundTabChanged is false run? Is this part of tab > helper construction? Earlier? Later? Session Restore just creates a bunch of windows and tabs, the loading will be handed over to tab loader, so SessionRestoreLoadingTabs will be true when we finish creating windows, and tab loader starts to navigate/load tabs. SessionRestoreInitialForegroundTabChanged is false by default, when tabs switch happens it will be set to true. And we don't want to track other foreground tabs in this version except the foreground tabs that is created by session restore. > RE: 2, when does the logic to flip either IsSessionRestoreLoadingTabs to false > or HasSessionRestoreInitialForegroundTabChanged to true run? The IsSessionRestoreLoadingTabs will return false when we finish loading tabs from session restore. SessionRestoreInitialForegroundTabChanged will be true when we switch tabs, which is tracked by TabStripModelObserver.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/resourc... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/resourc... chrome/browser/resource_coordinator/tab_manager.h:395: // Whether the user has switched from the initial foreground tab or not. True if initial foreground tab is switched away.
https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/resourc... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/resourc... chrome/browser/resource_coordinator/tab_manager.h:395: // Whether the user has switched from the initial foreground tab or not. On 2017/06/30 19:13:50, lpy wrote: > True if initial foreground tab is switched away. Done.
The new patch set addresses Bryan's comments. bmcquade@: Could you PTAL?
Thanks! These browsertests look excellent, thanks! Here are a couple comments - still working through the code. https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:43: SessionRestorePageLoadMetricsObserver::OnHidden( reading the comments in https://cs.chromium.org/chromium/src/ui/base/page_transition_types.h, it sounds like we can be even more confident that we're only measuring session restore navs if we also check the PageTransition value of the navigation. I see: // SessionRestore and undo tab close use this transition type too. PAGE_TRANSITION_RELOAD = 8, Given this, can we do something similar to https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/observe..., where we get the transition type in OnCommit(), and return STOP_OBSERVING if a navigation doesn't have the PAGE_TRANSITION_RELOAD transition type? Something like: // session restores use transition reload, so we only observe loads with a reload transition type. return ui::PageTransitionCoreTypeIs(transition, ui::PAGE_TRANSITION_RELOAD) ? CONTINUE_OBSERVING : STOP_OBSERVING; https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:52: UMA_HISTOGRAM_TIMES(internal::kHistogramSessionRestoreForegroundTabFirstPaint, let's use PAGE_LOAD_HISTOGRAM rather than UMA_HISTOGRAM_TIMES to be consistent with all other page load metrics observers (same for below) https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1059: class SessionRestorePageLoadMetricsBrowserTest thanks for all these tests! i just want to make sure none are flaky - can you run with: ./browser_tests --gtest_filter="SessionRestorePageLoadMetricsBrowserTest.*" --gtest_repeat=20 and just make sure none report failures when run repeatedly? https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1099: contents->GetController().LoadIfNecessary(); i notice we call this here, but we don't end up invoking LoadIfNecessary in all tests. Is the call to LoadIfNecessary needed? If so, do you know why it's not needed for nay of the other tests? https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1184: ExpectFirstPaintMetricsTotalCount(1); interesting, is this the desired behavior? if i stop a navigation and start a new page load to something else, that's no longer a session restore, in which case i'd expect zero metrics logged. perhaps if we add the PAGE_TRANSITION_RELOAD suggestion above, this would no longer get logged here, and we could correctly assert that zero metrics are logged?
https://codereview.chromium.org/2930013005/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:39: return started_in_foreground ? CONTINUE_OBSERVING : STOP_OBSERVING; let's add one more test, as an extra line of defense: // If the currently committed URL in the hosting tab is non-empty, it indicates that // there was a previous page load in this tab, and thus this load can't be a session // restore. if (!currently_committed_url.is_empty()) return STOP_OBSERVING;
Thanks for the great comments. I addressed them in a new patch set. https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:43: SessionRestorePageLoadMetricsObserver::OnHidden( On 2017/06/30 20:04:38, Bryan McQuade wrote: > reading the comments in > https://cs.chromium.org/chromium/src/ui/base/page_transition_types.h, it sounds > like we can be even more confident that we're only measuring session restore > navs if we also check the PageTransition value of the navigation. I see: > > // SessionRestore and undo tab close use this transition type too. > PAGE_TRANSITION_RELOAD = 8, > > Given this, can we do something similar to > https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/observe..., > where we get the transition type in OnCommit(), and return STOP_OBSERVING if a > navigation doesn't have the PAGE_TRANSITION_RELOAD transition type? > > Something like: > > // session restores use transition reload, so we only observe loads with a > reload transition type. > return ui::PageTransitionCoreTypeIs(transition, ui::PAGE_TRANSITION_RELOAD) ? > CONTINUE_OBSERVING : STOP_OBSERVING; Done. I agree that this condition will filter out new page loads in the initial foreground tab during session restore. https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:52: UMA_HISTOGRAM_TIMES(internal::kHistogramSessionRestoreForegroundTabFirstPaint, On 2017/06/30 20:04:38, Bryan McQuade wrote: > let's use PAGE_LOAD_HISTOGRAM rather than UMA_HISTOGRAM_TIMES to be consistent > with all other page load metrics observers (same for below) Done. https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1059: class SessionRestorePageLoadMetricsBrowserTest On 2017/06/30 20:04:38, Bryan McQuade wrote: > thanks for all these tests! i just want to make sure none are flaky - can you > run with: > > ./browser_tests --gtest_filter="SessionRestorePageLoadMetricsBrowserTest.*" > --gtest_repeat=20 > > and just make sure none report failures when run repeatedly? Done. https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1099: contents->GetController().LoadIfNecessary(); On 2017/06/30 20:04:38, Bryan McQuade wrote: > i notice we call this here, but we don't end up invoking LoadIfNecessary in all > tests. Is the call to LoadIfNecessary needed? If so, do you know why it's not > needed for nay of the other tests? WaitForTabsToLoad() is used only in the MultipleTabsSessionRestore test: the session restore restores multiple tabs and we test that the metrics of background tabs are not recorded. In the test I made a session restore with 2 tabs and we need to wait until all the tabs to be loaded before testing that no metrics were recorded. Since the background tabs do not paint anything, I did not wait for first-paint events to be fired by background tabs. This code is from SessionRestoreTest https://cs.chromium.org/chromium/src/chrome/browser/sessions/session_restore_... https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1184: ExpectFirstPaintMetricsTotalCount(1); On 2017/06/30 20:04:38, Bryan McQuade wrote: > interesting, is this the desired behavior? if i stop a navigation and start a > new page load to something else, that's no longer a session restore, in which > case i'd expect zero metrics logged. > > perhaps if we add the PAGE_TRANSITION_RELOAD suggestion above, this would no > longer get logged here, and we could correctly assert that zero metrics are > logged? Done. I agree that it would better to filter out the page loads in the initial foreground tabs. After adding the PAGE_TRANSTITION_RELOAD, these cases are filtered out. https://codereview.chromium.org/2930013005/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:39: return started_in_foreground ? CONTINUE_OBSERVING : STOP_OBSERVING; On 2017/06/30 20:13:08, Bryan McQuade wrote: > let's add one more test, as an extra line of defense: > > // If the currently committed URL in the hosting tab is non-empty, it indicates > that > // there was a previous page load in this tab, and thus this load can't be a > session > // restore. > if (!currently_committed_url.is_empty()) > return STOP_OBSERVING; Done.
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/06/30 at 22:03:51, ducbui wrote: > Thanks for the great comments. I addressed them in a new patch set. > > https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... > File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): > > https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:43: SessionRestorePageLoadMetricsObserver::OnHidden( > On 2017/06/30 20:04:38, Bryan McQuade wrote: > > reading the comments in > > https://cs.chromium.org/chromium/src/ui/base/page_transition_types.h, it sounds > > like we can be even more confident that we're only measuring session restore > > navs if we also check the PageTransition value of the navigation. I see: > > > > // SessionRestore and undo tab close use this transition type too. > > PAGE_TRANSITION_RELOAD = 8, > > > > Given this, can we do something similar to > > https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/observe..., > > where we get the transition type in OnCommit(), and return STOP_OBSERVING if a > > navigation doesn't have the PAGE_TRANSITION_RELOAD transition type? > > > > Something like: > > > > // session restores use transition reload, so we only observe loads with a > > reload transition type. > > return ui::PageTransitionCoreTypeIs(transition, ui::PAGE_TRANSITION_RELOAD) ? > > CONTINUE_OBSERVING : STOP_OBSERVING; > > Done. I agree that this condition will filter out new page loads in the initial foreground tab during session restore. > > https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:52: UMA_HISTOGRAM_TIMES(internal::kHistogramSessionRestoreForegroundTabFirstPaint, > On 2017/06/30 20:04:38, Bryan McQuade wrote: > > let's use PAGE_LOAD_HISTOGRAM rather than UMA_HISTOGRAM_TIMES to be consistent > > with all other page load metrics observers (same for below) > > Done. > > https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... > File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): > > https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1059: class SessionRestorePageLoadMetricsBrowserTest > On 2017/06/30 20:04:38, Bryan McQuade wrote: > > thanks for all these tests! i just want to make sure none are flaky - can you > > run with: > > > > ./browser_tests --gtest_filter="SessionRestorePageLoadMetricsBrowserTest.*" > > --gtest_repeat=20 > > > > and just make sure none report failures when run repeatedly? > > Done. > > https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1099: contents->GetController().LoadIfNecessary(); > On 2017/06/30 20:04:38, Bryan McQuade wrote: > > i notice we call this here, but we don't end up invoking LoadIfNecessary in all > > tests. Is the call to LoadIfNecessary needed? If so, do you know why it's not > > needed for nay of the other tests? > > WaitForTabsToLoad() is used only in the MultipleTabsSessionRestore test: the session restore restores multiple tabs and we test that the metrics of background tabs are not recorded. > In the test I made a session restore with 2 tabs and we need to wait until all the tabs to be loaded before testing that no metrics were recorded. Since the background tabs do not paint anything, I did not wait for first-paint events to be fired by background tabs. > > This code is from SessionRestoreTest https://cs.chromium.org/chromium/src/chrome/browser/sessions/session_restore_... > > https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1184: ExpectFirstPaintMetricsTotalCount(1); > On 2017/06/30 20:04:38, Bryan McQuade wrote: > > interesting, is this the desired behavior? if i stop a navigation and start a > > new page load to something else, that's no longer a session restore, in which > > case i'd expect zero metrics logged. > > > > perhaps if we add the PAGE_TRANSITION_RELOAD suggestion above, this would no > > longer get logged here, and we could correctly assert that zero metrics are > > logged? > > Done. I agree that it would better to filter out the page loads in the initial foreground tabs. After adding the PAGE_TRANSTITION_RELOAD, these cases are filtered out. > > https://codereview.chromium.org/2930013005/diff/180001/chrome/browser/page_lo... > File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): > > https://codereview.chromium.org/2930013005/diff/180001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:39: return started_in_foreground ? CONTINUE_OBSERVING : STOP_OBSERVING; > On 2017/06/30 20:13:08, Bryan McQuade wrote: > > let's add one more test, as an extra line of defense: > > > > // If the currently committed URL in the hosting tab is non-empty, it indicates > > that > > // there was a previous page load in this tab, and thus this load can't be a > > session > > // restore. > > if (!currently_committed_url.is_empty()) > > return STOP_OBSERVING; > > Done. Thanks! Looks like SessionRestorePageLoadMetricsBrowserTest.MultipleTabsSessionRestore SessionRestorePageLoadMetricsBrowserTest.LoadingAfterSessionRestore SessionRestorePageLoadMetricsBrowserTest.SingleTabSessionRestore SessionRestorePageLoadMetricsBrowserTest.MultipleSessionRestores are failing on some bots. Can you take a look at those failures?
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/07/01 18:13:41, Bryan McQuade wrote: > On 2017/06/30 at 22:03:51, ducbui wrote: > > Thanks for the great comments. I addressed them in a new patch set. > > > > > https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... > > File > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc > (right): > > > > > https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... > > > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:43: > SessionRestorePageLoadMetricsObserver::OnHidden( > > On 2017/06/30 20:04:38, Bryan McQuade wrote: > > > reading the comments in > > > https://cs.chromium.org/chromium/src/ui/base/page_transition_types.h, it > sounds > > > like we can be even more confident that we're only measuring session restore > > > navs if we also check the PageTransition value of the navigation. I see: > > > > > > // SessionRestore and undo tab close use this transition type too. > > > PAGE_TRANSITION_RELOAD = 8, > > > > > > Given this, can we do something similar to > > > > https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/observe..., > > > where we get the transition type in OnCommit(), and return STOP_OBSERVING if > a > > > navigation doesn't have the PAGE_TRANSITION_RELOAD transition type? > > > > > > Something like: > > > > > > // session restores use transition reload, so we only observe loads with a > > > reload transition type. > > > return ui::PageTransitionCoreTypeIs(transition, ui::PAGE_TRANSITION_RELOAD) > ? > > > CONTINUE_OBSERVING : STOP_OBSERVING; > > > > Done. I agree that this condition will filter out new page loads in the > initial foreground tab during session restore. > > > > > https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... > > > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:52: > UMA_HISTOGRAM_TIMES(internal::kHistogramSessionRestoreForegroundTabFirstPaint, > > On 2017/06/30 20:04:38, Bryan McQuade wrote: > > > let's use PAGE_LOAD_HISTOGRAM rather than UMA_HISTOGRAM_TIMES to be > consistent > > > with all other page load metrics observers (same for below) > > > > Done. > > > > > https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... > > File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc > (right): > > > > > https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... > > chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1059: class > SessionRestorePageLoadMetricsBrowserTest > > On 2017/06/30 20:04:38, Bryan McQuade wrote: > > > thanks for all these tests! i just want to make sure none are flaky - can > you > > > run with: > > > > > > ./browser_tests --gtest_filter="SessionRestorePageLoadMetricsBrowserTest.*" > > > --gtest_repeat=20 > > > > > > and just make sure none report failures when run repeatedly? > > > > Done. > > > > > https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... > > chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1099: > contents->GetController().LoadIfNecessary(); > > On 2017/06/30 20:04:38, Bryan McQuade wrote: > > > i notice we call this here, but we don't end up invoking LoadIfNecessary in > all > > > tests. Is the call to LoadIfNecessary needed? If so, do you know why it's > not > > > needed for nay of the other tests? > > > > WaitForTabsToLoad() is used only in the MultipleTabsSessionRestore test: the > session restore restores multiple tabs and we test that the metrics of > background tabs are not recorded. > > In the test I made a session restore with 2 tabs and we need to wait until all > the tabs to be loaded before testing that no metrics were recorded. Since the > background tabs do not paint anything, I did not wait for first-paint events to > be fired by background tabs. > > > > This code is from SessionRestoreTest > https://cs.chromium.org/chromium/src/chrome/browser/sessions/session_restore_... > > > > > https://codereview.chromium.org/2930013005/diff/160001/chrome/browser/page_lo... > > chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1184: > ExpectFirstPaintMetricsTotalCount(1); > > On 2017/06/30 20:04:38, Bryan McQuade wrote: > > > interesting, is this the desired behavior? if i stop a navigation and start > a > > > new page load to something else, that's no longer a session restore, in > which > > > case i'd expect zero metrics logged. > > > > > > perhaps if we add the PAGE_TRANSITION_RELOAD suggestion above, this would no > > > longer get logged here, and we could correctly assert that zero metrics are > > > logged? > > > > Done. I agree that it would better to filter out the page loads in the initial > foreground tabs. After adding the PAGE_TRANSTITION_RELOAD, these cases are > filtered out. > > > > > https://codereview.chromium.org/2930013005/diff/180001/chrome/browser/page_lo... > > File > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc > (right): > > > > > https://codereview.chromium.org/2930013005/diff/180001/chrome/browser/page_lo... > > > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:39: > return started_in_foreground ? CONTINUE_OBSERVING : STOP_OBSERVING; > > On 2017/06/30 20:13:08, Bryan McQuade wrote: > > > let's add one more test, as an extra line of defense: > > > > > > // If the currently committed URL in the hosting tab is non-empty, it > indicates > > > that > > > // there was a previous page load in this tab, and thus this load can't be a > > > session > > > // restore. > > > if (!currently_committed_url.is_empty()) > > > return STOP_OBSERVING; > > > > Done. > > Thanks! Looks like > > SessionRestorePageLoadMetricsBrowserTest.MultipleTabsSessionRestore > SessionRestorePageLoadMetricsBrowserTest.LoadingAfterSessionRestore > SessionRestorePageLoadMetricsBrowserTest.SingleTabSessionRestore > SessionRestorePageLoadMetricsBrowserTest.MultipleSessionRestores > > are failing on some bots. Can you take a look at those failures? The bots failed because they had browser-side navigation enabled. When the feature is enabled, our assumptions fail because: 1. TabLoader was created after the creation of SessionRestorePageLoadMetricsObserver, i.e., after page navigation. Since we determine the start of session restore in the constructor of TabLoader, our metrics observer determines session restore is not happening. When browser-side navigation is enabled, NavigatorImpl::NavigateToEntry() behaves differently on the condition at https://cs.chromium.org/chromium/src/content/browser/frame_host/navigator_imp... 2. When OnStart() is called, all tabs are in background because the navigation is during NavigationControllerImpl::SetActive(). Therefore, the flag started_in_background is always false and our session restore observer always stops observing. There are 2 options: 1. Revise the code to work with browser-side navigation (e.g., mark the start of session restore at the constructor of SessionRestore). This will require a major change to the code. 2. Disable the metrics and the tests when browser-side navigation is enabled. I currently use this option.
https://codereview.chromium.org/2930013005/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2930013005/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1195: if (!content::IsBrowserSideNavigationEnabled()) { Somehow the PageLoadMetricsWaiter does not see the first meaningful paint and the test hangs here, so I disable this test when browser-side navigation is enabled.
Great, thanks for investigating! We should figure out how to get this working with browser-side navigation enabled, since the team working on that project is planning to ship it very soon (quite possibly, in Chrome 61, so it'd be enabled by default for any Chrome versions this new observer ships in). On Mon, Jul 3, 2017 at 7:38 PM <ducbui@google.com> wrote: > > > https://codereview.chromium.org/2930013005/diff/300001/chrome/browser/page_lo... > File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc > (right): > > > https://codereview.chromium.org/2930013005/diff/300001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1195: > if (!content::IsBrowserSideNavigationEnabled()) { > Somehow the PageLoadMetricsWaiter does not see the first meaningful > paint and the test hangs here, so I disable this test when browser-side > navigation is enabled. > > https://codereview.chromium.org/2930013005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2930013005/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2930013005/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1195: if (!content::IsBrowserSideNavigationEnabled()) { On 2017/07/03 at 23:38:44, ducbui wrote: > Somehow the PageLoadMetricsWaiter does not see the first meaningful paint and the test hangs here, so I disable this test when browser-side navigation is enabled. Ah, I missed these browser-side navigation checks. We should probably not have any logic that specifically targets browser side nav or not - that suggests something else is broken. Browser-side navigation will be shipping very soon so we should make sure this observer works when it is enabled.
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I tried to move the start of session restore to the constructor of SessionRestoreImpl and revised all the tests. https://codereview.chromium.org/2930013005/diff/320001/chrome/browser/session... File chrome/browser/sessions/session_restore.cc (right): https://codereview.chromium.org/2930013005/diff/320001/chrome/browser/session... chrome/browser/sessions/session_restore.cc:135: observer.OnSessionRestoreStartedLoadingTabs(); I move the start of session restore to here. Thereby, we will have multiple start signals but still only one session restore stop signal, so we still can cover the start of the first session restore to the last session restore.
On 2017/07/04 05:34:39, ducbui wrote: > I tried to move the start of session restore to the constructor of > SessionRestoreImpl and revised all the tests. P.S. I put a session-restore-start emulation to all unit tests while browser tests are mostly unchanged. > https://codereview.chromium.org/2930013005/diff/320001/chrome/browser/session... > File chrome/browser/sessions/session_restore.cc (right): > > https://codereview.chromium.org/2930013005/diff/320001/chrome/browser/session... > chrome/browser/sessions/session_restore.cc:135: > observer.OnSessionRestoreStartedLoadingTabs(); > I move the start of session restore to here. Thereby, we will have multiple > start signals but still only one session restore stop signal, so we still can > cover the start of the first session restore to the last session restore.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2930013005/diff/320001/chrome/browser/session... File chrome/browser/sessions/session_restore.cc (right): https://codereview.chromium.org/2930013005/diff/320001/chrome/browser/session... chrome/browser/sessions/session_restore.cc:135: observer.OnSessionRestoreStartedLoadingTabs(); On 2017/07/04 05:34:39, ducbui wrote: > I move the start of session restore to here. Thereby, we will have multiple > start signals but still only one session restore stop signal, so we still can > cover the start of the first session restore to the last session restore. The revision of the session restore start signal does not work because it causes multi stop signals as well (I thought there would be only one stop signal). Some tests failed because of this: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/07/04 15:28:25, ducbui wrote: > https://codereview.chromium.org/2930013005/diff/320001/chrome/browser/session... > File chrome/browser/sessions/session_restore.cc (right): > > https://codereview.chromium.org/2930013005/diff/320001/chrome/browser/session... > chrome/browser/sessions/session_restore.cc:135: > observer.OnSessionRestoreStartedLoadingTabs(); > On 2017/07/04 05:34:39, ducbui wrote: > > I move the start of session restore to here. Thereby, we will have multiple > > start signals but still only one session restore stop signal, so we still can > > cover the start of the first session restore to the last session restore. > > The revision of the session restore start signal does not work because it causes > multi stop signals as well (I thought there would be only one stop signal). Some > tests failed because of this: > https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... Another try: I moved the check IsSessionRestoreLoadingTabs() to OnCommit() in the Observer because TabLoader is always created before OnCommit(). This method needs no changes to the original session restore signals.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/07/04 19:25:15, ducbui wrote: > On 2017/07/04 15:28:25, ducbui wrote: > > > https://codereview.chromium.org/2930013005/diff/320001/chrome/browser/session... > > File chrome/browser/sessions/session_restore.cc (right): > > > > > https://codereview.chromium.org/2930013005/diff/320001/chrome/browser/session... > > chrome/browser/sessions/session_restore.cc:135: > > observer.OnSessionRestoreStartedLoadingTabs(); > > On 2017/07/04 05:34:39, ducbui wrote: > > > I move the start of session restore to here. Thereby, we will have multiple > > > start signals but still only one session restore stop signal, so we still > can > > > cover the start of the first session restore to the last session restore. > > > > The revision of the session restore start signal does not work because it > causes > > multi stop signals as well (I thought there would be only one stop signal). > Some > > tests failed because of this: > > > https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... > > Another try: I moved the check IsSessionRestoreLoadingTabs() to OnCommit() in > the Observer because TabLoader is always created before OnCommit(). This method > needs no changes to the original session restore signals. It seems moving the checks to OnCommit() works well with browser-side navigation. Bryan: could you PTAL?
I sent an email with more details, but I also had one other comment to share. https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:40: // If browser-side navigation enabled, started_in_foreground is always false. this shouldn't be the case, in fact i know it's not always false when browser-side nav is enabled - we have metrics that show otherwise. it may sometimes be false when it should be true - that seems like a bug. which tests fail due to this when browser side nav is enabled? i'd like to understand & fix this.
https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:40: // If browser-side navigation enabled, started_in_foreground is always false. On 2017/07/06 17:38:51, Bryan McQuade wrote: > this shouldn't be the case, in fact i know it's not always false when > browser-side nav is enabled - we have metrics that show otherwise. it may > sometimes be false when it should be true - that seems like a bug. which tests > fail due to this when browser side nav is enabled? i'd like to understand & fix > this. I ran session restore with 1 foreground tab in full Chrome. When PlzNavigate is enabled, the flag started_in_foreground was false. When PlzNavigate is disabled, started_in_foreground was true as expected. Same result when I did session restore with 2 foreground tabs on 2 browser windows. I think the started_in_foreground flag is not correct for foreground tabs in session restore.
https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:40: // If browser-side navigation enabled, started_in_foreground is always false. On 2017/07/06 18:57:08, ducbui wrote: > On 2017/07/06 17:38:51, Bryan McQuade wrote: > > this shouldn't be the case, in fact i know it's not always false when > > browser-side nav is enabled - we have metrics that show otherwise. it may > > sometimes be false when it should be true - that seems like a bug. which tests > > fail due to this when browser side nav is enabled? i'd like to understand & > fix > > this. > > I ran session restore with 1 foreground tab in full Chrome. When PlzNavigate is > enabled, the flag started_in_foreground was false. When PlzNavigate is disabled, > started_in_foreground was true as expected. > > Same result when I did session restore with 2 foreground tabs on 2 browser > windows. > > I think the started_in_foreground flag is not correct for foreground tabs in > session restore. I thought this should have been fixed in http://crbug.com/725347, where foreground info comes from CreateParams. If not fixed, maybe it is specific to session restore? You can debug this by following where |CreateParams.initially_hidden| is set. Related to session restore, it is probably here: https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_tabrestore.cc?...
Yes, that's right, thanks! We did not plumb CreateParams through all possible call paths, and it's possible that session restore is one of the paths that was not correctly plumbed. If we can plumb the create params for session restore as well I think that will help. Thanks, Bryan On Fri, Jul 7, 2017 at 5:09 PM <zhenw@chromium.org> wrote: > > > https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... > File > > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc > (right): > > > https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... > > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:40: > // If browser-side navigation enabled, started_in_foreground is always > false. > On 2017/07/06 18:57:08, ducbui wrote: > > On 2017/07/06 17:38:51, Bryan McQuade wrote: > > > this shouldn't be the case, in fact i know it's not always false > when > > > browser-side nav is enabled - we have metrics that show otherwise. > it may > > > sometimes be false when it should be true - that seems like a bug. > which tests > > > fail due to this when browser side nav is enabled? i'd like to > understand & > > fix > > > this. > > > > I ran session restore with 1 foreground tab in full Chrome. When > PlzNavigate is > > enabled, the flag started_in_foreground was false. When PlzNavigate is > disabled, > > started_in_foreground was true as expected. > > > > Same result when I did session restore with 2 foreground tabs on 2 > browser > > windows. > > > > I think the started_in_foreground flag is not correct for foreground > tabs in > > session restore. > > I thought this should have been fixed in http://crbug.com/725347, where > foreground info comes from CreateParams. > > If not fixed, maybe it is specific to session restore? You can debug > this by following where |CreateParams.initially_hidden| is set. Related > to session restore, it is probably here: > > https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_tabrestore.cc?... > > https://codereview.chromium.org/2930013005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/07/07 21:13:43, Bryan McQuade wrote: > Yes, that's right, thanks! > > We did not plumb CreateParams through all possible call paths, and it's > possible that session restore is one of the paths that was not correctly > plumbed. > > If we can plumb the create params for session restore as well I think that > will help. > > Thanks, > Bryan > > On Fri, Jul 7, 2017 at 5:09 PM <mailto:zhenw@chromium.org> wrote: > > > > > > > > https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... > > File > > > > > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc > > (right): > > > > > > > https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... > > > > > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:40: > > // If browser-side navigation enabled, started_in_foreground is always > > false. > > On 2017/07/06 18:57:08, ducbui wrote: > > > On 2017/07/06 17:38:51, Bryan McQuade wrote: > > > > this shouldn't be the case, in fact i know it's not always false > > when > > > > browser-side nav is enabled - we have metrics that show otherwise. > > it may > > > > sometimes be false when it should be true - that seems like a bug. > > which tests > > > > fail due to this when browser side nav is enabled? i'd like to > > understand & > > > fix > > > > this. > > > > > > I ran session restore with 1 foreground tab in full Chrome. When > > PlzNavigate is > > > enabled, the flag started_in_foreground was false. When PlzNavigate is > > disabled, > > > started_in_foreground was true as expected. > > > > > > Same result when I did session restore with 2 foreground tabs on 2 > > browser > > > windows. > > > > > > I think the started_in_foreground flag is not correct for foreground > > tabs in > > > session restore. > > > > I thought this should have been fixed in http://crbug.com/725347, where > > foreground info comes from CreateParams. > > > > If not fixed, maybe it is specific to session restore? You can debug > > this by following where |CreateParams.initially_hidden| is set. Related > > to session restore, it is probably here: > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_tabrestore.cc?... > > > > https://codereview.chromium.org/2930013005/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I have just filed a bug https://bugs.chromium.org/p/chromium/issues/detail?id=740252
bmcquade@chromium.org changed reviewers: + csharrison@chromium.org
Some initial comments, mostly nits and one question about navigation matching. https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:28: ? base::MakeUnique<SessionRestorePageLoadMetricsObserver>() #include "base/memory/ptr_utils.h" https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:41: if (!currently_committed_url.is_empty()) #include "url/gurl.h" https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:53: DCHECK(tab_manager); #include "base/logging.h" https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:58: return ui::PageTransitionCoreTypeIs(navigation_handle->GetPageTransition(), #include "content/public/browser/navigation_handle.h" https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:58: return ui::PageTransitionCoreTypeIs(navigation_handle->GetPageTransition(), #include "ui/base/page_transition_types.h" https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:63: tab_manager->IsSessionRestoreLoadingTabs() && Do these checks cover all session restore loads and no others? I'm nervous for e.g. cases when the current tab is reloaded when other tabs in the restore are loading. Tab manager keeps track of navigation handles, I wonder if there is something you could do to communicate with tab manager to see if this navigation is associated with the current restore. https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.h (right): https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.h:11: #include "chrome/browser/resource_coordinator/tab_manager.h" nit: not needed in the header https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.h:25: SessionRestorePageLoadMetricsObserver() {} nit: Chromium style discourages inline constructors https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.h:33: // PageLoadMetricsObserver implementation: nit: page_load_metrics::PageLoadMetricsObserver: https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.h:37: nit: remove whitespace from between these overriden methods. https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.h:57: DISALLOW_COPY_AND_ASSIGN(SessionRestorePageLoadMetricsObserver); #include "base/macros.h" https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.h:58: }; nit: add a newline after this line.
csharrison@: Could you PTAL? Thanks. https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:28: ? base::MakeUnique<SessionRestorePageLoadMetricsObserver>() On 2017/07/17 20:18:36, Charlie Harrison wrote: > #include "base/memory/ptr_utils.h" Done. https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:41: if (!currently_committed_url.is_empty()) On 2017/07/17 20:18:36, Charlie Harrison wrote: > #include "url/gurl.h" Done. https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:53: DCHECK(tab_manager); On 2017/07/17 20:18:36, Charlie Harrison wrote: > #include "base/logging.h" Done. https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:58: return ui::PageTransitionCoreTypeIs(navigation_handle->GetPageTransition(), On 2017/07/17 20:18:36, Charlie Harrison wrote: > #include "ui/base/page_transition_types.h" Done. https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:58: return ui::PageTransitionCoreTypeIs(navigation_handle->GetPageTransition(), On 2017/07/17 20:18:36, Charlie Harrison wrote: > #include "content/public/browser/navigation_handle.h" Done. https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:63: tab_manager->IsSessionRestoreLoadingTabs() && On 2017/07/17 20:18:36, Charlie Harrison wrote: > Do these checks cover all session restore loads and no others? I'm nervous for > e.g. cases when the current tab is reloaded when other tabs in the restore are > loading. If the current tab is reloaded during session restore, SessionRestorePageLoadMetricsObserver::OnStart() will stop observing. > Tab manager keeps track of navigation handles, I wonder if there is something > you could do to communicate with tab manager to see if this navigation is > associated with the current restore. Since we consider page loads during all session restores across all profiles, I don't know whether we need to associate the current navigation with the current restore. https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.h (right): https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.h:11: #include "chrome/browser/resource_coordinator/tab_manager.h" On 2017/07/17 20:18:36, Charlie Harrison wrote: > nit: not needed in the header Done. https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.h:25: SessionRestorePageLoadMetricsObserver() {} On 2017/07/17 20:18:36, Charlie Harrison wrote: > nit: Chromium style discourages inline constructors Done. https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.h:33: // PageLoadMetricsObserver implementation: On 2017/07/17 20:18:37, Charlie Harrison wrote: > nit: page_load_metrics::PageLoadMetricsObserver: Done. https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.h:37: On 2017/07/17 20:18:36, Charlie Harrison wrote: > nit: remove whitespace from between these overriden methods. Done. https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.h:57: DISALLOW_COPY_AND_ASSIGN(SessionRestorePageLoadMetricsObserver); On 2017/07/17 20:18:36, Charlie Harrison wrote: > #include "base/macros.h" Done. https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.h:58: }; On 2017/07/17 20:18:37, Charlie Harrison wrote: > nit: add a newline after this line. Done.
https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:63: tab_manager->IsSessionRestoreLoadingTabs() && On 2017/07/18 at 16:03:45, ducbui wrote: > On 2017/07/17 20:18:36, Charlie Harrison wrote: > > Do these checks cover all session restore loads and no others? I'm nervous for > > e.g. cases when the current tab is reloaded when other tabs in the restore are > > loading. > > If the current tab is reloaded during session restore, SessionRestorePageLoadMetricsObserver::OnStart() will stop observing. > > > Tab manager keeps track of navigation handles, I wonder if there is something > > you could do to communicate with tab manager to see if this navigation is > > associated with the current restore. > > Since we consider page loads during all session restores across all profiles, I don't know whether we need to associate the current navigation with the current restore. I share Charles' concern with using per-tab-manager boolean state timing to determine if a page load started due to session restore. We already saw this was fragile when we discovered that browser tests no longer worked when plznavigate was enabled. I think it's worthwhile to investigate Charles' great suggestion to see if we can ask the tab manager whether a given NavigationHandle is associated with session restore. We may conclude that this is infeasible, but I think it's worth spending the time to read the code & convince ourselves that's the case. Let's see whether it's feasible to add a: bool TabManager::IsNavigationDueToSessionRestore(NavigationHandle* handle); method (or something similar). I'd have much greater confidence in such a method, which internally should be testing whether the given NavigationHandle is in some member collection of session-restored navigations, than the current use of a boolean and assumption that any navigations observed by page load metrics while the boolean is true are due to session restore. FWIW I'm also really uncomfortable with doing such a test in OnCommit - that just seems like the wrong time to be doing such a lookup. This is something that should be tested at the time the navigation is started, in OnStart.
In the new patch set, I leverage the RestoreType of a NavigationHandle in the observer's OnStart(). Could you PTAL? https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:63: tab_manager->IsSessionRestoreLoadingTabs() && On 2017/07/18 16:44:02, Bryan McQuade wrote: > On 2017/07/18 at 16:03:45, ducbui wrote: > > On 2017/07/17 20:18:36, Charlie Harrison wrote: > > > Do these checks cover all session restore loads and no others? I'm nervous > for > > > e.g. cases when the current tab is reloaded when other tabs in the restore > are > > > loading. > > > > If the current tab is reloaded during session restore, > SessionRestorePageLoadMetricsObserver::OnStart() will stop observing. > > > > > Tab manager keeps track of navigation handles, I wonder if there is > something > > > you could do to communicate with tab manager to see if this navigation is > > > associated with the current restore. > > > > Since we consider page loads during all session restores across all profiles, > I don't know whether we need to associate the current navigation with the > current restore. > > I share Charles' concern with using per-tab-manager boolean state timing to > determine if a page load started due to session restore. We already saw this was > fragile when we discovered that browser tests no longer worked when plznavigate > was enabled. > > I think it's worthwhile to investigate Charles' great suggestion to see if we > can ask the tab manager whether a given NavigationHandle is associated with > session restore. We may conclude that this is infeasible, but I think it's worth > spending the time to read the code & convince ourselves that's the case. > > Let's see whether it's feasible to add a: > bool TabManager::IsNavigationDueToSessionRestore(NavigationHandle* handle); > method (or something similar). I'd have much greater confidence in such a > method, which internally should be testing whether the given NavigationHandle is > in some member collection of session-restored navigations, than the current use > of a boolean and assumption that any navigations observed by page load metrics > while the boolean is true are due to session restore. > > FWIW I'm also really uncomfortable with doing such a test in OnCommit - that > just seems like the wrong time to be doing such a lookup. This is something that > should be tested at the time the navigation is started, in OnStart. Thanks for the great suggestion. I added an condition which uses the RestoreType of a NavigationHandle to check whether the navigation was initiated by a session restore or not.
Great, the restore type seems like a good thing to test! Only thing I'd ask is to verify that there are no other sources of restores that will cause this observer to log. Let's see if we can figure out the various sources that set the restore type and verify that only the session restore case will be triggered in your OnStart. https://codereview.chromium.org/2930013005/diff/400001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/400001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:51: navigation_handle->GetRestoreType() == content::RestoreType::NONE) we probably want to exclude CURRENT_SESSION as well given the comment says "// The entry has been restored from the current session. This is used when // the user issues 'reopen closed tab'." - that's not a session restore, right?
ducbui@google.com changed reviewers: + sky@chromium.org
+sky@ to reviewers. Could you PTAL? Thanks. https://codereview.chromium.org/2930013005/diff/400001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/400001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:51: navigation_handle->GetRestoreType() == content::RestoreType::NONE) On 2017/07/19 13:12:59, Bryan McQuade wrote: > we probably want to exclude CURRENT_SESSION as well given the comment says "// > The entry has been restored from the current session. This is used when > // the user issues 'reopen closed tab'." - that's not a session restore, > right? We cannot use RestoreType of NavigationHandle to check here because we can reopen tabs/windows from a last session but we excluded them from session restore. Moreover, since we stop the observer whenever there is any foreground tab reloads or navigates away, we effectively exclude all non-restore navigations. I added an attribute to Browser to check whether the browser is used for session restore or not. This attributes can exclude the cases where the user reopens tabs from last session. I also added a browser test for this case. The new attribute is_used_for_session_restore of Browser is set in the following 2 points: 1. SessionRestoreImpl::CreateRestoredBrowser(): the only point that creates a restored browser. 2. SessionRestoreImpl::ProcessSessionWindows(): the only point that uses an existing browser for session restored tabs.
Looks good but I am concerned our unit test harness is not going to be good enough for you ATM :( I think it might be workable for the multi-tab case with a bit of extra logic in the harness to register our tab helper for new tabs. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:56: if (!currently_committed_url.is_empty() || !started_in_foreground || nit: I would prefer if this method looked like this: if (!started_in_foreground || !currently_committed_url.is_empty()) return STOP_OBSERVING; Browser* browser = ... ... if (!is_in_session_restored_browser) return STOP_OBSERVING; DCHECK(...); return CONTINUE_OBSERVING; This way we can early return without doing an O(n) lookup for the common case where this isn't the first URL loaded in the contents. If there is a session restore of n tabs then we could hit quadratic behavior here. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:75: return ui::PageTransitionCoreTypeIs(navigation_handle->GetPageTransition(), Can you comment why this isn't DCHECKable? I guess it's because the session restore could fail so we attach an observer to subsequent non-reloads. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:112: SessionRestorePageLoadMetricsObserver::SessionRestorePageLoadMetricsObserver() { nit: let's put this at the top of the file, so method order is consistent. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:31: std::unique_ptr<SessionRestorePageLoadMetricsObserver> observer = #include <memory> https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:34: tracker->AddObserver(std::move(observer)); #include <utility> for std::move https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:47: restored_tabs_.emplace_back(web_contents(), false, false, false); Would you mind documenting these "false" args e.g. restored_tabs_.emplace_back(web_contents(), false /* is_active */, false /* is_app */, false /* is_pinned */); https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:53: restored_tabs_.clear(); Is this really needed? https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:79: TabLoader::RestoreTabs(restored_tabs_, base::TimeTicks()); #include "base/time/time.h" https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:87: content::WebContentsTester::For(contents)->TestSetIsLoading(false); Does WebContents::Stop work in this case? It seems better than just resetting the loading state. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:91: void NavigateAndCommitWithReloadTransition(content::WebContents* contents, These days it is preferable to use content::NavigationSimulator to do these sorts of tests. If possible it would be nice to use that. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:93: contents->GetController().LoadURL( #include <string> #include "content/public/browser/web_contents.h" #include "content/public/browser/navigation_controller.h" #include "ui/base/page_transition_types.h" https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:95: GURL loaded_url(url); nit: #include "url/gurl.h" https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:106: std::vector<RestoredTab> restored_tabs_; #include <vector> https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:107: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:120: TEST_F(SessionRestorePageLoadMetricsObserverTest, InitialForegroundTabChanged) { Hmm, overall I'm not sure that the PLM observer test harness is robust enough for multiple tab cases. Internally, it stores and accesses only a single metrics web contents observer, and doesn't create new ones for new tabs. Can you create a test that exercises this? e.g. Close all tabs then start a session restore expecting metrics to arrive for a new tab. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:134: // Start loading tab 0 but do not finish loading it. nit: Start loading tab 0 but switch tabs before any paint events occur. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:192: // Clear committed URL so the next session restore starts from an emtpy URL. s/emtpy/empty https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1121: std::unique_ptr<PageLoadMetricsWaiter> CreatePageLoadMetricsWaiter( #include <memory> https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1123: return base::MakeUnique<PageLoadMetricsWaiter>(web_contents); #include "base/memory/ptr_util.h"
I assume you're asking for a review of the miscellaneous files not under page_load_metrics or resource_coordinator. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/ui/brow... File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/ui/brow... chrome/browser/ui/browser.h:246: is_used_for_session_restore_ = is_used_for_session_restore; How does this differ from is_session_restore_ above?
sky@: yes, I made some changes to files session_restore.cc and tab_loader.h under your session_restore/. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/ui/brow... File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/ui/brow... chrome/browser/ui/browser.h:246: is_used_for_session_restore_ = is_used_for_session_restore; On 2017/07/21 14:48:09, sky wrote: > How does this differ from is_session_restore_ above? The is_session_restore_ above is used on Mac only to indicate that session restore is in progress because when Mac window shows it inhibits animation during session restore. It was added in this CL: https://codereview.chromium.org/7809013 The new is_used_for_session_restore_ indicates not only that a new browser is created for session restore but also an existing browser is being used to hold restored tabs. I want to track foreground tabs created by session restore and adding this property to browser is sufficient.
On 2017/07/21 15:19:58, ducbui wrote: > sky@: yes, I made some changes to files session_restore.cc and tab_loader.h > under your session_restore/. > > https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/ui/brow... > File chrome/browser/ui/browser.h (right): > > https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/ui/brow... > chrome/browser/ui/browser.h:246: is_used_for_session_restore_ = > is_used_for_session_restore; > On 2017/07/21 14:48:09, sky wrote: > > How does this differ from is_session_restore_ above? > > The is_session_restore_ above is used on Mac only to indicate that session > restore is in progress because when Mac window shows it inhibits animation > during session restore. It was added in this CL: > https://codereview.chromium.org/7809013 > > The new is_used_for_session_restore_ indicates not only that a new browser is > created for session restore but also an existing browser is being used to hold > restored tabs. I want to track foreground tabs created by session restore and > adding this property to browser is sufficient. Can you elaborate on why you need this at the browser level? Restore is really at the WebContents level. How come you aren't tracking it there?
On 2017/07/21 at 17:50:12, sky wrote: > On 2017/07/21 15:19:58, ducbui wrote: > > sky@: yes, I made some changes to files session_restore.cc and tab_loader.h > > under your session_restore/. > > > > https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/ui/brow... > > File chrome/browser/ui/browser.h (right): > > > > https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/ui/brow... > > chrome/browser/ui/browser.h:246: is_used_for_session_restore_ = > > is_used_for_session_restore; > > On 2017/07/21 14:48:09, sky wrote: > > > How does this differ from is_session_restore_ above? > > > > The is_session_restore_ above is used on Mac only to indicate that session > > restore is in progress because when Mac window shows it inhibits animation > > during session restore. It was added in this CL: > > https://codereview.chromium.org/7809013 > > > > The new is_used_for_session_restore_ indicates not only that a new browser is > > created for session restore but also an existing browser is being used to hold > > restored tabs. I want to track foreground tabs created by session restore and > > adding this property to browser is sufficient. > > Can you elaborate on why you need this at the browser level? Restore is really at the WebContents level. How come you aren't tracking it there? RE: a webcontents-level API, this is what prerender does as well. They provide a static method: static bool prerender::PrerenderContents::FromWebContents(content::WebContents*); I wonder if we could provide something similar for session restore. Not knowing the internals of session restore, it's not totally clear to me whether tracking this at the WC level or NavigationHandle level makes more sense. From a page load standpoint, having the ability to inquire as to whether a given NavHandle was initiated via session restore makes the most sense to me. A WebContents API for this would have to know about navigations - if for example navigation A is initiated in a WebContents for session restore, then a user cancels that nav and causes a commit to nav B, the WebContents state needs to be updated such that it no longer reports that it's in session restore mode as a result of this navigation B. Given this, it does seem to me that session restore tracking is at the navigation level... I'll try to look more at the internals of session restore next week to see if I see any paths forward here.
https://codereview.chromium.org/2930013005/diff/400001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/400001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:51: navigation_handle->GetRestoreType() == content::RestoreType::NONE) On 2017/07/21 at 04:38:05, ducbui wrote: > On 2017/07/19 13:12:59, Bryan McQuade wrote: > > we probably want to exclude CURRENT_SESSION as well given the comment says "// > > The entry has been restored from the current session. This is used when > > // the user issues 'reopen closed tab'." - that's not a session restore, > > right? > > We cannot use RestoreType of NavigationHandle to check here because we can reopen tabs/windows from a last session but we excluded them from session restore. Moreover, since we stop the observer whenever there is any foreground tab reloads or navigates away, we effectively exclude all non-restore navigations. > > I added an attribute to Browser to check whether the browser is used for session restore or not. This attributes can exclude the cases where the user reopens tabs from last session. I also added a browser test for this case. > > The new attribute is_used_for_session_restore of Browser is set in the following 2 points: > 1. SessionRestoreImpl::CreateRestoredBrowser(): the only point that creates a restored browser. > 2. SessionRestoreImpl::ProcessSessionWindows(): the only point that uses an existing browser for session restored tabs. Thanks! Can you give a little more detail on why we can't use RestoreType? I don't fully understand what you mean by "we can reopen tabs/windows from a last session but we excluded them from session restore". Can you clarify / elaborate on what this means?
On 2017/07/24 18:30:02, Bryan McQuade wrote: > https://codereview.chromium.org/2930013005/diff/400001/chrome/browser/page_lo... > File > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc > (right): > > https://codereview.chromium.org/2930013005/diff/400001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:51: > navigation_handle->GetRestoreType() == content::RestoreType::NONE) > On 2017/07/21 at 04:38:05, ducbui wrote: > > On 2017/07/19 13:12:59, Bryan McQuade wrote: > > > we probably want to exclude CURRENT_SESSION as well given the comment says > "// > > > The entry has been restored from the current session. This is used when > > > // the user issues 'reopen closed tab'." - that's not a session restore, > > > right? > > > > We cannot use RestoreType of NavigationHandle to check here because we can > reopen tabs/windows from a last session but we excluded them from session > restore. Moreover, since we stop the observer whenever there is any foreground > tab reloads or navigates away, we effectively exclude all non-restore > navigations. > > > > I added an attribute to Browser to check whether the browser is used for > session restore or not. This attributes can exclude the cases where the user > reopens tabs from last session. I also added a browser test for this case. > > > > The new attribute is_used_for_session_restore of Browser is set in the > following 2 points: > > 1. SessionRestoreImpl::CreateRestoredBrowser(): the only point that creates a > restored browser. > > 2. SessionRestoreImpl::ProcessSessionWindows(): the only point that uses an > existing browser for session restored tabs. > > Thanks! Can you give a little more detail on why we can't use RestoreType? I > don't fully understand what you mean by "we can reopen tabs/windows from a last > session but we excluded them from session restore". Can you clarify / elaborate > on what this means? RestoreType::LAST_SESSION can be used by both session restore and by "Reopen closed tabs" (Ctrl+Shift+T). Typically, Ctrl+Shift+T opens tabs from current session (as described in the comment in the restore_type.h) but in some cases it opens tabs from last session. For example, when the user opens a new window then Ctrl+Shift+T, a new window with tabs from last session will be opened. Although this case uses RestoreType::LAST_SESSION, it is not counted as session restore (cases included in session restore is in this doc: https://docs.google.com/document/d/1sS1WO2rp6M8g1Uco1GMsyqr5wsuJjQzA9RTgrNZQT...).
On 2017/07/24 at 19:00:05, ducbui wrote: > On 2017/07/24 18:30:02, Bryan McQuade wrote: > > https://codereview.chromium.org/2930013005/diff/400001/chrome/browser/page_lo... > > File > > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc > > (right): > > > > https://codereview.chromium.org/2930013005/diff/400001/chrome/browser/page_lo... > > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:51: > > navigation_handle->GetRestoreType() == content::RestoreType::NONE) > > On 2017/07/21 at 04:38:05, ducbui wrote: > > > On 2017/07/19 13:12:59, Bryan McQuade wrote: > > > > we probably want to exclude CURRENT_SESSION as well given the comment says > > "// > > > > The entry has been restored from the current session. This is used when > > > > // the user issues 'reopen closed tab'." - that's not a session restore, > > > > right? > > > > > > We cannot use RestoreType of NavigationHandle to check here because we can > > reopen tabs/windows from a last session but we excluded them from session > > restore. Moreover, since we stop the observer whenever there is any foreground > > tab reloads or navigates away, we effectively exclude all non-restore > > navigations. > > > > > > I added an attribute to Browser to check whether the browser is used for > > session restore or not. This attributes can exclude the cases where the user > > reopens tabs from last session. I also added a browser test for this case. > > > > > > The new attribute is_used_for_session_restore of Browser is set in the > > following 2 points: > > > 1. SessionRestoreImpl::CreateRestoredBrowser(): the only point that creates a > > restored browser. > > > 2. SessionRestoreImpl::ProcessSessionWindows(): the only point that uses an > > existing browser for session restored tabs. > > > > Thanks! Can you give a little more detail on why we can't use RestoreType? I > > don't fully understand what you mean by "we can reopen tabs/windows from a last > > session but we excluded them from session restore". Can you clarify / elaborate > > on what this means? > > RestoreType::LAST_SESSION can be used by both session restore and by "Reopen closed tabs" (Ctrl+Shift+T). Typically, Ctrl+Shift+T opens tabs from current session (as described in the comment in the restore_type.h) but in some cases it opens tabs from last session. For example, when the user opens a new window then Ctrl+Shift+T, a new window with tabs from last session will be opened. Although this case uses RestoreType::LAST_SESSION, it is not counted as session restore (cases included in session restore is in this doc: https://docs.google.com/document/d/1sS1WO2rp6M8g1Uco1GMsyqr5wsuJjQzA9RTgrNZQT...). Thanks! That makes sense. I interpreted your previous message as indicating that we couldn't use RestoreType at all (you said "We cannot use RestoreType of NavigationHandle to check here..."), but it sounds like maybe you meant that RestoreType can be used, but isn't sufficient on its own. If we use RestoreType in combination with your checks in CreateIfNeeded, this seems like it should be sufficient to restrict to tabs inserted via session restore. Do you see any navigations that we would incorrectly classify (either a page load from session restore that we fail to track, or a page load that isn't a session restore that we accidentially track) if we use the following: 1. In OnStart check for LAST_SESSION restoretype on the NavigationHandle 2. In CreateIfNeeded, check for tab_manager && tab_manager->IsSessionRestoreLoadingTabs() && !tab_manager->HasSessionRestoreInitialForegroundTabChanged() Would this combination be sufficient to track only page loads from session restore?
On 2017/07/24 at 19:16:46, Bryan McQuade wrote: > On 2017/07/24 at 19:00:05, ducbui wrote: > > On 2017/07/24 18:30:02, Bryan McQuade wrote: > > > https://codereview.chromium.org/2930013005/diff/400001/chrome/browser/page_lo... > > > File > > > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc > > > (right): > > > > > > https://codereview.chromium.org/2930013005/diff/400001/chrome/browser/page_lo... > > > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:51: > > > navigation_handle->GetRestoreType() == content::RestoreType::NONE) > > > On 2017/07/21 at 04:38:05, ducbui wrote: > > > > On 2017/07/19 13:12:59, Bryan McQuade wrote: > > > > > we probably want to exclude CURRENT_SESSION as well given the comment says > > > "// > > > > > The entry has been restored from the current session. This is used when > > > > > // the user issues 'reopen closed tab'." - that's not a session restore, > > > > > right? > > > > > > > > We cannot use RestoreType of NavigationHandle to check here because we can > > > reopen tabs/windows from a last session but we excluded them from session > > > restore. Moreover, since we stop the observer whenever there is any foreground > > > tab reloads or navigates away, we effectively exclude all non-restore > > > navigations. > > > > > > > > I added an attribute to Browser to check whether the browser is used for > > > session restore or not. This attributes can exclude the cases where the user > > > reopens tabs from last session. I also added a browser test for this case. > > > > > > > > The new attribute is_used_for_session_restore of Browser is set in the > > > following 2 points: > > > > 1. SessionRestoreImpl::CreateRestoredBrowser(): the only point that creates a > > > restored browser. > > > > 2. SessionRestoreImpl::ProcessSessionWindows(): the only point that uses an > > > existing browser for session restored tabs. > > > > > > Thanks! Can you give a little more detail on why we can't use RestoreType? I > > > don't fully understand what you mean by "we can reopen tabs/windows from a last > > > session but we excluded them from session restore". Can you clarify / elaborate > > > on what this means? > > > > RestoreType::LAST_SESSION can be used by both session restore and by "Reopen closed tabs" (Ctrl+Shift+T). Typically, Ctrl+Shift+T opens tabs from current session (as described in the comment in the restore_type.h) but in some cases it opens tabs from last session. For example, when the user opens a new window then Ctrl+Shift+T, a new window with tabs from last session will be opened. Although this case uses RestoreType::LAST_SESSION, it is not counted as session restore (cases included in session restore is in this doc: https://docs.google.com/document/d/1sS1WO2rp6M8g1Uco1GMsyqr5wsuJjQzA9RTgrNZQT...). > > Thanks! That makes sense. I interpreted your previous message as indicating that we couldn't use RestoreType at all (you said "We cannot use RestoreType of NavigationHandle to check here..."), but it sounds like maybe you meant that RestoreType can be used, but isn't sufficient on its own. If we use RestoreType in combination with your checks in CreateIfNeeded, this seems like it should be sufficient to restrict to tabs inserted via session restore. Do you see any navigations that we would incorrectly classify (either a page load from session restore that we fail to track, or a page load that isn't a session restore that we accidentially track) if we use the following: > > 1. In OnStart check for LAST_SESSION restoretype on the NavigationHandle > 2. In CreateIfNeeded, check for tab_manager && tab_manager->IsSessionRestoreLoadingTabs() && > !tab_manager->HasSessionRestoreInitialForegroundTabChanged() > > Would this combination be sufficient to track only page loads from session restore? Apologies, I'm still paging all of this back in. My previous suggestion may not work. IIRC Duc, you found that IsSessionRestoreLoadingTabs didn't work correctly in CreateIfNeeded when browser-side navigation is enabled. Is that right? If so, then an alternative approach may be needed. Earlier in the thread, Scott suggested scoping something at the WebContents level. One thing we could consider is, in session_restore.cc, where we call chrome::AddRestoredTab(...), we could tag the returned WebContents with a WebContentsUserData indicating that the tab was created for session restore. We could then query for that particular WebContentsUserData in the page load metrics observer. We could also RemoveUserData the WCUD when session restore completes for that tab. I'm not super familiar with WebContentsUserData so I'm not certain this is the best approach. Charles and others on thread may be able to help decide if this approach would work.
+1 to Bryan said. SupportsUserData is intended for associating arbitrary data with a WebContents, which I believe is example what you want here. -Scott On Mon, Jul 24, 2017 at 12:33 PM, <bmcquade@chromium.org> wrote: > On 2017/07/24 at 19:16:46, Bryan McQuade wrote: > > On 2017/07/24 at 19:00:05, ducbui wrote: > > > On 2017/07/24 18:30:02, Bryan McQuade wrote: > > > > > https://codereview.chromium.org/2930013005/diff/400001/ > chrome/browser/page_load_metrics/observers/session_ > restore_page_load_metrics_observer.cc > > > > File > > > > > chrome/browser/page_load_metrics/observers/session_ > restore_page_load_metrics_observer.cc > > > > (right): > > > > > > > > > https://codereview.chromium.org/2930013005/diff/400001/ > chrome/browser/page_load_metrics/observers/session_ > restore_page_load_metrics_observer.cc#newcode51 > > > > > chrome/browser/page_load_metrics/observers/session_ > restore_page_load_metrics_observer.cc:51: > > > > navigation_handle->GetRestoreType() == content::RestoreType::NONE) > > > > On 2017/07/21 at 04:38:05, ducbui wrote: > > > > > On 2017/07/19 13:12:59, Bryan McQuade wrote: > > > > > > we probably want to exclude CURRENT_SESSION as well given the > comment > says > > > > "// > > > > > > The entry has been restored from the current session. This is > used > when > > > > > > // the user issues 'reopen closed tab'." - that's not a session > restore, > > > > > > right? > > > > > > > > > > We cannot use RestoreType of NavigationHandle to check here > because we > can > > > > reopen tabs/windows from a last session but we excluded them from > session > > > > restore. Moreover, since we stop the observer whenever there is any > foreground > > > > tab reloads or navigates away, we effectively exclude all non-restore > > > > navigations. > > > > > > > > > > I added an attribute to Browser to check whether the browser is > used for > > > > session restore or not. This attributes can exclude the cases where > the > user > > > > reopens tabs from last session. I also added a browser test for this > case. > > > > > > > > > > The new attribute is_used_for_session_restore of Browser is set in > the > > > > following 2 points: > > > > > 1. SessionRestoreImpl::CreateRestoredBrowser(): the only point > that > creates a > > > > restored browser. > > > > > 2. SessionRestoreImpl::ProcessSessionWindows(): the only point > that uses > an > > > > existing browser for session restored tabs. > > > > > > > > Thanks! Can you give a little more detail on why we can't use > RestoreType? > I > > > > don't fully understand what you mean by "we can reopen tabs/windows > from a > last > > > > session but we excluded them from session restore". Can you clarify / > elaborate > > > > on what this means? > > > > > > RestoreType::LAST_SESSION can be used by both session restore and by > "Reopen > closed tabs" (Ctrl+Shift+T). Typically, Ctrl+Shift+T opens tabs from > current > session (as described in the comment in the restore_type.h) but in some > cases it > opens tabs from last session. For example, when the user opens a new > window then > Ctrl+Shift+T, a new window with tabs from last session will be opened. > Although > this case uses RestoreType::LAST_SESSION, it is not counted as session > restore > (cases included in session restore is in this doc: > https://docs.google.com/document/d/1sS1WO2rp6M8g1Uco1GMsyqr5wsuJj > QzA9RTgrNZQTxA/edit). > > > > Thanks! That makes sense. I interpreted your previous message as > indicating > that we couldn't use RestoreType at all (you said "We cannot use > RestoreType of > NavigationHandle to check here..."), but it sounds like maybe you meant > that > RestoreType can be used, but isn't sufficient on its own. If we use > RestoreType > in combination with your checks in CreateIfNeeded, this seems like it > should be > sufficient to restrict to tabs inserted via session restore. Do you see any > navigations that we would incorrectly classify (either a page load from > session > restore that we fail to track, or a page load that isn't a session restore > that > we accidentially track) if we use the following: > > > > 1. In OnStart check for LAST_SESSION restoretype on the NavigationHandle > > 2. In CreateIfNeeded, check for tab_manager && > tab_manager->IsSessionRestoreLoadingTabs() && > > !tab_manager->HasSessionRestoreInitialForegroundTabChanged() > > > > Would this combination be sufficient to track only page loads from > session > restore? > > Apologies, I'm still paging all of this back in. My previous suggestion > may not > work. IIRC Duc, you found that IsSessionRestoreLoadingTabs didn't work > correctly > in CreateIfNeeded when browser-side navigation is enabled. Is that right? > If so, > then an alternative approach may be needed. > > Earlier in the thread, Scott suggested scoping something at the WebContents > level. One thing we could consider is, in session_restore.cc, where we call > chrome::AddRestoredTab(...), we could tag the returned WebContents with a > WebContentsUserData indicating that the tab was created for session > restore. We > could then query for that particular WebContentsUserData in the page load > metrics observer. We could also RemoveUserData the WCUD when session > restore > completes for that tab. > > I'm not super familiar with WebContentsUserData so I'm not certain this is > the > best approach. Charles and others on thread may be able to help decide if > this > approach would work. > > https://codereview.chromium.org/2930013005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thank you for great suggestions! In the new patch set, I added a restore origin to WebContents. This restore origin is added when a WebContents is created by session restore and is removed when the restored WebContents finishes loading. With this restore origin, we can know whether the tab was restored by session restore or not. Another approach is to add more values to RestoreType of NavigationHandle to distinguish tab restore by session restore or not. However, it will not be easy for TabManager to detect whether an initial foreground tab change occurs on restored tabs or not, because the ActiveTabChanged() callback provides with only WebContents while getting NavigationHandle from WebContents is hard. This is necessary in case: When the user clicks "Restore" after a browser crash, a session restore starts, then the active tab is changed to a restored tab. BTW, we landed a new patch which fixed the problem of Session Restore signal when browser-side navigation is enable (https://chromium-review.googlesource.com/c/564343/), so IsSessionRestoreLoadingTabs() now works well in all cases. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:56: if (!currently_committed_url.is_empty() || !started_in_foreground || On 2017/07/21 13:59:33, Charlie Harrison wrote: > nit: I would prefer if this method looked like this: > > if (!started_in_foreground || !currently_committed_url.is_empty()) > return STOP_OBSERVING; > > Browser* browser = ... > ... > if (!is_in_session_restored_browser) > return STOP_OBSERVING; > > DCHECK(...); > return CONTINUE_OBSERVING; > > This way we can early return without doing an O(n) lookup for the common case > where this isn't the first URL loaded in the contents. If there is a session > restore of n tabs then we could hit quadratic behavior here. Ack. In the new patch set, I add RestoreOrigin into WebContents instead of Browser. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:75: return ui::PageTransitionCoreTypeIs(navigation_handle->GetPageTransition(), On 2017/07/21 13:59:33, Charlie Harrison wrote: > Can you comment why this isn't DCHECKable? I guess it's because the session > restore could fail so we attach an observer to subsequent non-reloads. Acknowledged. I put this into DCHECK in the new patch set. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:112: SessionRestorePageLoadMetricsObserver::SessionRestorePageLoadMetricsObserver() { On 2017/07/21 13:59:33, Charlie Harrison wrote: > nit: let's put this at the top of the file, so method order is consistent. Done. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1121: std::unique_ptr<PageLoadMetricsWaiter> CreatePageLoadMetricsWaiter( On 2017/07/21 13:59:34, Charlie Harrison wrote: > #include <memory> Done. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1123: return base::MakeUnique<PageLoadMetricsWaiter>(web_contents); On 2017/07/21 13:59:34, Charlie Harrison wrote: > #include "base/memory/ptr_util.h" Done.
https://codereview.chromium.org/2930013005/diff/520001/chrome/browser/session... File chrome/browser/sessions/session_restore.h (right): https://codereview.chromium.org/2930013005/diff/520001/chrome/browser/session... chrome/browser/sessions/session_restore.h:35: class RestoreOrigin : public content::WebContentsUserData<RestoreOrigin> { I think the enum makes sense if it's going to be around for the lifetime of the WebContents, but that's not the case. You remove it at some point. How about adding a function OnWillRestoreTab() to SessionRestoreObserver. This way TabLoader can locally track whatever it needs to.
https://codereview.chromium.org/2930013005/diff/520001/chrome/browser/session... File chrome/browser/sessions/session_restore.h (right): https://codereview.chromium.org/2930013005/diff/520001/chrome/browser/session... chrome/browser/sessions/session_restore.h:35: class RestoreOrigin : public content::WebContentsUserData<RestoreOrigin> { On 2017/07/25 16:48:00, sky wrote: > I think the enum makes sense if it's going to be around for the lifetime of the > WebContents, but that's not the case. You remove it at some point. How about > adding a function OnWillRestoreTab() to SessionRestoreObserver. This way > TabLoader can locally track whatever it needs to. +1 to this idea.
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
lgtm https://codereview.chromium.org/2930013005/diff/540001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/540001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:58: !started_in_foreground) { Is this check still needed?
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by ducbui@google.com
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Bryan: Could you PTAL? I updated this CL to use per-tab methods which track session-restore state (https://chromium-review.googlesource.com/c/588168) and restored-in-foreground condition (https://chromium-review.googlesource.com/c/599507) for each tab. After you confirm the approach in the SessionRestorePageLoadMetricsObserver is correct, I will complete Charlie's comments on the unit tests. I am still working on the unit test harness to observe multiple tabs. Thanks! https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:31: std::unique_ptr<SessionRestorePageLoadMetricsObserver> observer = On 2017/07/21 13:59:33, Charlie Harrison wrote: > #include <memory> Done. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:34: tracker->AddObserver(std::move(observer)); On 2017/07/21 13:59:33, Charlie Harrison wrote: > #include <utility> for std::move Done. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:47: restored_tabs_.emplace_back(web_contents(), false, false, false); On 2017/07/21 13:59:33, Charlie Harrison wrote: > Would you mind documenting these "false" args e.g. > restored_tabs_.emplace_back(web_contents(), false /* is_active */, false /* > is_app */, false /* is_pinned */); Done. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:53: restored_tabs_.clear(); On 2017/07/21 13:59:33, Charlie Harrison wrote: > Is this really needed? No, I will remove it. Each RestoredTab element, which does not hold dynamically-allocated memory, in the restore_tabs_ vector will be deallocated in the Test class destructor. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:79: TabLoader::RestoreTabs(restored_tabs_, base::TimeTicks()); On 2017/07/21 13:59:33, Charlie Harrison wrote: > #include "base/time/time.h" Done. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:87: content::WebContentsTester::For(contents)->TestSetIsLoading(false); On 2017/07/21 13:59:33, Charlie Harrison wrote: > Does WebContents::Stop work in this case? It seems better than just resetting > the loading state. WebContents::Stop() seems not sufficient. It crashed MultipleSessionRestoresWithoutInitialForegroundTabChange test because TabLoader has not received stop or destroy signal from the tab. This caused duplicated registration in TabLoader's NotificationRegistrar. I will add WebContents::Stop() in addition to TestSetIsLoading(false). https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:93: contents->GetController().LoadURL( On 2017/07/21 13:59:33, Charlie Harrison wrote: > #include <string> > > #include "content/public/browser/web_contents.h" > #include "content/public/browser/navigation_controller.h" > #include "ui/base/page_transition_types.h" Done. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:95: GURL loaded_url(url); On 2017/07/21 13:59:33, Charlie Harrison wrote: > nit: #include "url/gurl.h" Done. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:106: std::vector<RestoredTab> restored_tabs_; On 2017/07/21 13:59:33, Charlie Harrison wrote: > #include <vector> Done. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:107: }; On 2017/07/21 13:59:33, Charlie Harrison wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:120: TEST_F(SessionRestorePageLoadMetricsObserverTest, InitialForegroundTabChanged) { On 2017/07/21 13:59:33, Charlie Harrison wrote: > Hmm, overall I'm not sure that the PLM observer test harness is robust enough > for multiple tab cases. Internally, it stores and accesses only a single metrics > web contents observer, and doesn't create new ones for new tabs. > > Can you create a test that exercises this? e.g. Close all tabs then start a > session restore expecting metrics to arrive for a new tab. Acknowledged. I will add a PageLoadMetricsObserverTester to record metrics of multiple web contents. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:134: // Start loading tab 0 but do not finish loading it. On 2017/07/21 13:59:33, Charlie Harrison wrote: > nit: Start loading tab 0 but switch tabs before any paint events occur. Done. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:192: // Clear committed URL so the next session restore starts from an emtpy URL. On 2017/07/21 13:59:33, Charlie Harrison wrote: > s/emtpy/empty Done. https://codereview.chromium.org/2930013005/diff/540001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/540001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:58: !started_in_foreground) { On 2017/08/03 19:48:59, fmeawad wrote: > Is this check still needed? Yes, it is needed in case that a tab is created in foreground but then navigates in background. This case might not happen in practice because a tab typically navigates *shortly* after it is created in foreground. However, in tests, the computer can switch away from a foreground tab so quickly even before the tab navigates.
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== [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: Add signals that mark the start and end of session restore (https://crrev.com/2935183002/) BUG=731901 ========== to ========== [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 ==========
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I addressed Charlie's comments for the unit tests. Could you PTAL? Thanks! https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:91: void NavigateAndCommitWithReloadTransition(content::WebContents* contents, On 2017/07/21 13:59:33, Charlie Harrison wrote: > These days it is preferable to use content::NavigationSimulator to do these > sorts of tests. If possible it would be nice to use that. Done. https://codereview.chromium.org/2930013005/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:120: TEST_F(SessionRestorePageLoadMetricsObserverTest, InitialForegroundTabChanged) { On 2017/08/03 20:41:27, ducbui wrote: > On 2017/07/21 13:59:33, Charlie Harrison wrote: > > Hmm, overall I'm not sure that the PLM observer test harness is robust enough > > for multiple tab cases. Internally, it stores and accesses only a single > metrics > > web contents observer, and doesn't create new ones for new tabs. > > > > Can you create a test that exercises this? e.g. Close all tabs then start a > > session restore expecting metrics to arrive for a new tab. > > Acknowledged. I will add a PageLoadMetricsObserverTester to record metrics of > multiple web contents. Done. I use an additional PageLoadMetricsObserverTester to record metrics of a second tab. I also add a test case MultipleForegroundTabsLoadedInSessionRestore to check UMA recorded multiple tabs.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I have a few suggestions and some nits. Overall I like the simplicity in the new observer, and I really really appreciate the tests. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:7: #include <base/debug/stack_trace.h> nit: remove stack_trace https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:9: #include "base/memory/ptr_util.h" nit: unused. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:10: #include "base/metrics/histogram_macros.h" nit: unused (you're just using our own custom macros). https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:14: #include "chrome/browser/sessions/session_restore.h" nit: is it used? https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:17: #include "url/gurl.h" nit: unused. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:55: return STOP_OBSERVING; Note: OnHidden() is called immediately when the tab is backgrounded. However, timing updates are dispatched to this class asynchronously and batched at ~1s intervals. So, it is possible for the following sequence to occur: 1. First paint in foreground (in renderer) 2. OnHidden() 3. Timestamp from (1) is dispatched to this class, which is no longer observing updates Up to you whether you want to handle this case, just making sure you're aware of it. If you do want to handle it, you can remove this method and instead gate the metrics logging code with: if (page_load_metrics::WasStartedInForegroundOptionalEventInForeground(event, info)) { PAGE_LOAD_HISTOGRAM(...); } https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:62: timing.paint_timing->first_paint.value()); #include page_load_metrics.mojom.h https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.h (right): https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.h:8: #include <memory> nit: unused. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:8: #include <string> nit: is it used? https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:9: #include <utility> nit: is it used? https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:25: #include "content/public/common/referrer.h" nit: is it used? https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:34: : public page_load_metrics::PageLoadMetricsObserverTestHarness { Optional: The test harness is just a dead-simple wrapper around a PageLoadMetricsObserverTester. If you wanted, you could just make this test harness extensible to multiple tabs and just inherit directly from ChromeRenderViewHostTestHarness. This way, multi-tab loading situations would not have two paths, one using test harness methods and one using custom code. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:36: void RegisterObservers(page_load_metrics::PageLoadTracker* tracker) override { optional: This method could be static. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:115: DISALLOW_COPY_AND_ASSIGN(SessionRestorePageLoadMetricsObserverTest); #include "base/macros.h" https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:140: std::unique_ptr<content::WebContents> test_contents(CreateTestWebContents()); nit: maybe name it second_contents? test_contents vs. web_contents() does not help my understanding of the test. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:141: std::unique_ptr<page_load_metrics::PageLoadMetricsObserverTester> tester( I'm very happy the *Tester class was useful to you! https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:142: new page_load_metrics::PageLoadMetricsObserverTester( nit: please use base::MakeUnique to avoid bare new. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:144: base::BindRepeating( #include base/bind and base/bind_helpers (for base::Unretained) https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1157: std::unique_ptr<PageLoadMetricsWaiter> CreatePageLoadMetricsWaiter( nit: I would just remove this method, it doesn't gain you much for the additional layer of abstraction. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1187: "/page_load_metrics/page_with_css.html"); page_with_css is not very aptly named, it does a lot of needless computation to ensure that we spend more than 1ms of time parsing css. Can we choose another site? Why doesn't title1.html work? https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1301: active_web_contents->Stop(); I'm not quite sure why this is needed? Shouldn't loading a new URL in the contents automatically stop the previous navigation? How does it affect paint metrics? https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1308: // No metrics recorded because it navigates away during session restore. s/it/the tab https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right): https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:142: #if !defined(OS_ANDROID) While you're here could you also gate the #includes by this #ifdef? That way we don't have to pull in the session restore observer on android, and the android observer on desktop.
Thank you for the comments! There are 2 remaining unaddressed comments about Stop() and title1.html in the browser test. I am doing further investigation to resolve them. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:7: #include <base/debug/stack_trace.h> On 2017/08/07 14:30:07, Charlie Harrison wrote: > nit: remove stack_trace Done. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:9: #include "base/memory/ptr_util.h" On 2017/08/07 14:30:07, Charlie Harrison wrote: > nit: unused. Done. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:10: #include "base/metrics/histogram_macros.h" On 2017/08/07 14:30:08, Charlie Harrison wrote: > nit: unused (you're just using our own custom macros). Done. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:14: #include "chrome/browser/sessions/session_restore.h" On 2017/08/07 14:30:07, Charlie Harrison wrote: > nit: is it used? Acknowledged. It is not used so I will remove it. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:17: #include "url/gurl.h" On 2017/08/07 14:30:07, Charlie Harrison wrote: > nit: unused. Done. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:55: return STOP_OBSERVING; On 2017/08/07 14:30:08, Charlie Harrison wrote: > Note: OnHidden() is called immediately when the tab is backgrounded. However, > timing updates are dispatched to this class asynchronously and batched at ~1s > intervals. > > So, it is possible for the following sequence to occur: > 1. First paint in foreground (in renderer) > 2. OnHidden() > 3. Timestamp from (1) is dispatched to this class, which is no longer observing > updates > > Up to you whether you want to handle this case, just making sure you're aware of > it. If you do want to handle it, you can remove this method and instead gate the > metrics logging code with: > if (page_load_metrics::WasStartedInForegroundOptionalEventInForeground(event, > info)) { > PAGE_LOAD_HISTOGRAM(...); > } Thank you for the suggestion! I did not aware of this issue. I will remove the OnStart() and use WasStartedInForegroundOptionalEventInForeground() to check the foreground state of the tab. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:62: timing.paint_timing->first_paint.value()); On 2017/08/07 14:30:08, Charlie Harrison wrote: > #include page_load_metrics.mojom.h Done. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.h (right): https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.h:8: #include <memory> On 2017/08/07 14:30:08, Charlie Harrison wrote: > nit: unused. Done. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:8: #include <string> On 2017/08/07 14:30:08, Charlie Harrison wrote: > nit: is it used? Acknowledged. I will remove it. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:9: #include <utility> On 2017/08/07 14:30:08, Charlie Harrison wrote: > nit: is it used? Acknowledged. I will remove it. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:25: #include "content/public/common/referrer.h" On 2017/08/07 14:30:08, Charlie Harrison wrote: > nit: is it used? Acknowledged. I will remove it. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:34: : public page_load_metrics::PageLoadMetricsObserverTestHarness { On 2017/08/07 14:30:08, Charlie Harrison wrote: > Optional: The test harness is just a dead-simple wrapper around a > PageLoadMetricsObserverTester. > > If you wanted, you could just make this test harness extensible to multiple tabs > and just inherit directly from ChromeRenderViewHostTestHarness. > > This way, multi-tab loading situations would not have two paths, one using test > harness methods and one using custom code. Thank you for the suggestion, I change to class to support multiple tabs. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:36: void RegisterObservers(page_load_metrics::PageLoadTracker* tracker) override { On 2017/08/07 14:30:08, Charlie Harrison wrote: > optional: This method could be static. Done. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:115: DISALLOW_COPY_AND_ASSIGN(SessionRestorePageLoadMetricsObserverTest); On 2017/08/07 14:30:08, Charlie Harrison wrote: > #include "base/macros.h" Done. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:140: std::unique_ptr<content::WebContents> test_contents(CreateTestWebContents()); On 2017/08/07 14:30:08, Charlie Harrison wrote: > nit: maybe name it second_contents? test_contents vs. web_contents() does not > help my understanding of the test. Acknowledged. I will change it to second_contents. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:141: std::unique_ptr<page_load_metrics::PageLoadMetricsObserverTester> tester( On 2017/08/07 14:30:08, Charlie Harrison wrote: > I'm very happy the *Tester class was useful to you! It is indeed very useful. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:142: new page_load_metrics::PageLoadMetricsObserverTester( On 2017/08/07 14:30:08, Charlie Harrison wrote: > nit: please use base::MakeUnique to avoid bare new. Done. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:144: base::BindRepeating( On 2017/08/07 14:30:08, Charlie Harrison wrote: > #include base/bind and base/bind_helpers (for base::Unretained) Done. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1157: std::unique_ptr<PageLoadMetricsWaiter> CreatePageLoadMetricsWaiter( On 2017/08/07 14:30:08, Charlie Harrison wrote: > nit: I would just remove this method, it doesn't gain you much for the > additional layer of abstraction. Acknowledged. I will remove it. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1187: "/page_load_metrics/page_with_css.html"); On 2017/08/07 14:30:08, Charlie Harrison wrote: > page_with_css is not very aptly named, it does a lot of needless computation to > ensure that we spend more than 1ms of time parsing css. > > Can we choose another site? Why doesn't title1.html work? Using "/title1.html" yields the first paint and first contentful paint, but no first meaningful paint, so it makes PageLoadMetricsWaiter timeout. I checked which timing is updated on PageLoadMetricsWaiter::OnTimingUpdated(). The first meaningful paint does happen when I use more complicated pages like the page with css. Currently I do not have any clue to explain and will do further investigation about this issue. I checked FirstMeaningfulPaintDetector but it seems to work normally. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1301: active_web_contents->Stop(); On 2017/08/07 14:30:08, Charlie Harrison wrote: > I'm not quite sure why this is needed? Shouldn't loading a new URL in the > contents automatically stop the previous navigation? > > How does it affect paint metrics? Acknowledged. I tried to use ui_test_utils::NavigateToURL() but there are only first paint and first contentful pain while not first meaningful paint (even with the page with CSS). I will do further debugging on this issue. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1308: // No metrics recorded because it navigates away during session restore. On 2017/08/07 14:30:08, Charlie Harrison wrote: > s/it/the tab Done. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right): https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:142: #if !defined(OS_ANDROID) On 2017/08/07 14:30:08, Charlie Harrison wrote: > While you're here could you also gate the #includes by this #ifdef? That way we > don't have to pull in the session restore observer on android, and the android > observer on desktop. Done. Thanks for the suggestion!
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1187: "/page_load_metrics/page_with_css.html"); On 2017/08/08 16:29:48, ducbui wrote: > On 2017/08/07 14:30:08, Charlie Harrison wrote: > > page_with_css is not very aptly named, it does a lot of needless computation > to > > ensure that we spend more than 1ms of time parsing css. > > > > Can we choose another site? Why doesn't title1.html work? > > Using "/title1.html" yields the first paint and first contentful paint, but no > first meaningful paint, so it makes PageLoadMetricsWaiter timeout. I checked > which timing is updated on PageLoadMetricsWaiter::OnTimingUpdated(). The first > meaningful paint does happen when I use more complicated pages like the page > with css. > > Currently I do not have any clue to explain and will do further investigation > about this issue. I checked FirstMeaningfulPaintDetector but it seems to work > normally. Hm maybe FMP needs > 1 layout to work correctly? In any case I would still prefer if we could use another URL, maybe main_frame_with_iframe with a query param would work. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1301: active_web_contents->Stop(); On 2017/08/08 16:29:48, ducbui wrote: > On 2017/08/07 14:30:08, Charlie Harrison wrote: > > I'm not quite sure why this is needed? Shouldn't loading a new URL in the > > contents automatically stop the previous navigation? > > > > How does it affect paint metrics? > > Acknowledged. > I tried to use ui_test_utils::NavigateToURL() but there are only first paint > and first contentful pain while not first meaningful paint (even with the page > with CSS). > I will do further debugging on this issue. I see. Don't let this block you too much if it turns out to be difficult. If that's the case let's just beef up this comment and maybe add a TODO(ducbui) next to it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1187: "/page_load_metrics/page_with_css.html"); On 2017/08/08 17:00:29, Charlie Harrison wrote: > On 2017/08/08 16:29:48, ducbui wrote: > > On 2017/08/07 14:30:08, Charlie Harrison wrote: > > > page_with_css is not very aptly named, it does a lot of needless computation > > to > > > ensure that we spend more than 1ms of time parsing css. > > > > > > Can we choose another site? Why doesn't title1.html work? > > > > Using "/title1.html" yields the first paint and first contentful paint, but no > > first meaningful paint, so it makes PageLoadMetricsWaiter timeout. I checked > > which timing is updated on PageLoadMetricsWaiter::OnTimingUpdated(). The first > > meaningful paint does happen when I use more complicated pages like the page > > with css. > > > > Currently I do not have any clue to explain and will do further investigation > > about this issue. I checked FirstMeaningfulPaintDetector but it seems to work > > normally. > > Hm maybe FMP needs > 1 layout to work correctly? In any case I would still > prefer if we could use another URL, maybe main_frame_with_iframe with a query > param would work. Heuristics in FirstMeaningfulPaintDetector.cpp skip the first meaningful paint candidate which is generally the first contentful paint, so the page might need to have at least 2 paints to have the first meaningful paint. I changed the testing URLs to main_frame_with_iframe.html and iframes.html which also work. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1301: active_web_contents->Stop(); On 2017/08/08 17:00:29, Charlie Harrison wrote: > On 2017/08/08 16:29:48, ducbui wrote: > > On 2017/08/07 14:30:08, Charlie Harrison wrote: > > > I'm not quite sure why this is needed? Shouldn't loading a new URL in the > > > contents automatically stop the previous navigation? > > > > > > How does it affect paint metrics? > > > > Acknowledged. > > I tried to use ui_test_utils::NavigateToURL() but there are only first paint > > and first contentful pain while not first meaningful paint (even with the page > > with CSS). > > I will do further debugging on this issue. > > I see. Don't let this block you too much if it turns out to be difficult. If > that's the case let's just beef up this comment and maybe add a TODO(ducbui) > next to it. I added a TODO for this issue. I tried other APIs for loading URLs but they did not work. When I changed the testing URL to "/page_load_metrics/iframes.html", it worked without the Stop() without browser-side navigation but did not produce the first meaningful paint with browser-side navigation enabled. I think I need to debug all the first meaningful-paint code to resolve this issue.
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM with nits https://codereview.chromium.org/2930013005/diff/680001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/680001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:42: started_in_foreground) perf nit: Both tab_manager methods require map lookups, let's start off by checking started_in_foreground for a fast early exit. https://codereview.chromium.org/2930013005/diff/680001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2930013005/diff/680001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:62: for (size_t i = 0; i < tabs().size(); ++i) Can you just tabs_.clear()? https://codereview.chromium.org/2930013005/diff/680001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:80: testers_.emplace_back(base::MakeUnique< optional: this gets formatted rather badly, I'd prefer just pulling out the tester into a local and std::moving it here. https://codereview.chromium.org/2930013005/diff/680001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:151: std::unique_ptr<WebContents> web_contents_; nit: not used https://codereview.chromium.org/2930013005/diff/680001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2930013005/diff/680001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1125: class SessionRestorePageLoadMetricsBrowserTest Optional: There's enough tests here to warrant pulling this out into a separate file. No need to do it in this CL, but I would welcome it as a follow up. https://codereview.chromium.org/2930013005/diff/680001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right): https://codereview.chromium.org/2930013005/diff/680001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:41: #if !defined(OS_ANDROID) nit: platform specific includes go in a separate section below standard includes: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
sky@: Could you PTAL chrome/browser/sessions/session_restore.h? Thanks! https://codereview.chromium.org/2930013005/diff/680001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/680001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:42: started_in_foreground) On 2017/08/09 02:42:05, Charlie Harrison wrote: > perf nit: Both tab_manager methods require map lookups, let's start off by > checking started_in_foreground for a fast early exit. Done. https://codereview.chromium.org/2930013005/diff/680001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2930013005/diff/680001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:62: for (size_t i = 0; i < tabs().size(); ++i) On 2017/08/09 02:42:05, Charlie Harrison wrote: > Can you just tabs_.clear()? Done. https://codereview.chromium.org/2930013005/diff/680001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:80: testers_.emplace_back(base::MakeUnique< On 2017/08/09 02:42:05, Charlie Harrison wrote: > optional: this gets formatted rather badly, I'd prefer just pulling out the > tester into a local and std::moving it here. Done. https://codereview.chromium.org/2930013005/diff/680001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:151: std::unique_ptr<WebContents> web_contents_; On 2017/08/09 02:42:05, Charlie Harrison wrote: > nit: not used Acknowledged. Thank you for spotting this. https://codereview.chromium.org/2930013005/diff/680001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2930013005/diff/680001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1125: class SessionRestorePageLoadMetricsBrowserTest On 2017/08/09 02:42:05, Charlie Harrison wrote: > Optional: There's enough tests here to warrant pulling this out into a separate > file. No need to do it in this CL, but I would welcome it as a follow up. Ack, I agree to separate these tests, but the change will requite some refactoring effort since the tests depend on PageLoadMetricsWaiter which is local in this test. I think it would better to do it in another CL. https://codereview.chromium.org/2930013005/diff/680001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right): https://codereview.chromium.org/2930013005/diff/680001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:41: #if !defined(OS_ANDROID) On 2017/08/09 02:42:05, Charlie Harrison wrote: > nit: platform specific includes go in a separate section below standard > includes: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done.
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM
https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:55: return STOP_OBSERVING; On 2017/08/07 at 14:30:08, Charlie Harrison wrote: > Note: OnHidden() is called immediately when the tab is backgrounded. However, timing updates are dispatched to this class asynchronously and batched at ~1s intervals. > > So, it is possible for the following sequence to occur: > 1. First paint in foreground (in renderer) > 2. OnHidden() > 3. Timestamp from (1) is dispatched to this class, which is no longer observing updates > > Up to you whether you want to handle this case, just making sure you're aware of it. If you do want to handle it, you can remove this method and instead gate the metrics logging code with: > if (page_load_metrics::WasStartedInForegroundOptionalEventInForeground(event, info)) { > PAGE_LOAD_HISTOGRAM(...); > } Good point, yes. There are corner case timing issues with either - some observers use the OnHidden approach, others the WasStartedInForegroundOptionalEventInForeground. In practice we don't see significant differences between the 2, so I think either is ok. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1169: auto waiter = CreatePageLoadMetricsWaiter(web_contents); generally we prefer to create the waiter before navigating, and call Wait() after navigating. otherwise there's the potential for races. so i'm not sure i recomment adding helpers like this - better to inline in each test method instead. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1192: "/page_load_metrics/main_frame_with_iframe.html"); can we use title2.html to be consistent? https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1301: active_web_contents->Stop(); On 2017/08/07 at 14:30:08, Charlie Harrison wrote: > I'm not quite sure why this is needed? Shouldn't loading a new URL in the contents automatically stop the previous navigation? > > How does it affect paint metrics? agree - if you have to call this method, something else seems wrong here, and we should figure out what's going on. https://codereview.chromium.org/2930013005/diff/740001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/740001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:18: "TabManager.Experimental.SessionRestore.ForegroundTab.FirstPaint"; any reason not to use the existing naming scheme PageLoad.Clients.SessionRestore? using the same naming convention allows others using the UMA dash to easily find Pageload breakout metrics since they are under the same prefix. If you put them in TabManager.* people won't be able to find them easily, if they are interested in session restore metrics. https://codereview.chromium.org/2930013005/diff/740001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2930013005/diff/740001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:122: testers_[i]->SimulateTimingUpdate(timing_); this is odd. we never dispatch the same timing update to all webcontents like this in real code. we should only dispatch the timing update to the tester it's intended for. i worry that doing this makes it less likely that you're exercising the real behavior in your tests. could you instead target the dispatch to the specific webcontents you want that timing to be sent to? So you'd perhaps update testers_ to be a map from WebContents* to the associated tester for that WC, add a param to this method that takes a WebContents*, and then dispatch via the map. https://codereview.chromium.org/2930013005/diff/740001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:260: StopLoading(web_contents()); does loading not stop on its own when the page being loaded fires the load event? i'm still a bit surprised this is needed. https://codereview.chromium.org/2930013005/diff/740001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2930013005/diff/740001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1165: auto waiter = base::MakeUnique<PageLoadMetricsWaiter>(web_contents); let's not do this - it's likely to be racy. please first instantiate the waiter, than initiate the navigation, then wait. i'll probably follow this change with code that prohibits this use of the waiter class, as it's not intended. https://codereview.chromium.org/2930013005/diff/740001/chrome/browser/session... File chrome/browser/sessions/session_restore.h (right): https://codereview.chromium.org/2930013005/diff/740001/chrome/browser/session... chrome/browser/sessions/session_restore.h:125: FRIEND_TEST_ALL_PREFIXES(SessionRestorePageLoadMetricsObserverTest, eek, it's slightly concerning to me that we need to call private methods from tests - it's usually a hint that we may not be testing the real production behavior directly. is this standard practice for session restore tests? i don't know the code well enough to make a firm call here, but this is slightly worrying to me.
https://codereview.chromium.org/2930013005/diff/740001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/740001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:18: "TabManager.Experimental.SessionRestore.ForegroundTab.FirstPaint"; On 2017/08/09 15:41:34, Bryan McQuade wrote: > any reason not to use the existing naming scheme > PageLoad.Clients.SessionRestore? using the same naming convention allows others > using the UMA dash to easily find Pageload breakout metrics since they are under > the same prefix. If you put them in TabManager.* people won't be able to find > them easily, if they are interested in session restore metrics. I argue the opposite since currently we have a set of metrics that starts with TabManager.SessionRestore.* but I am OK with either.
On 2017/08/09 at 16:09:28, fmeawad wrote: > https://codereview.chromium.org/2930013005/diff/740001/chrome/browser/page_lo... > File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): > > https://codereview.chromium.org/2930013005/diff/740001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:18: "TabManager.Experimental.SessionRestore.ForegroundTab.FirstPaint"; > On 2017/08/09 15:41:34, Bryan McQuade wrote: > > any reason not to use the existing naming scheme > > PageLoad.Clients.SessionRestore? using the same naming convention allows others > > using the UMA dash to easily find Pageload breakout metrics since they are under > > the same prefix. If you put them in TabManager.* people won't be able to find > > them easily, if they are interested in session restore metrics. > > I argue the opposite since currently we have a set of metrics that starts with > TabManager.SessionRestore.* but I am OK with either. Ok, I am fine with using different naming if there's a good reason to do so. SGTM, thanks!
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1169: auto waiter = CreatePageLoadMetricsWaiter(web_contents); On 2017/08/09 15:41:34, Bryan McQuade wrote: > generally we prefer to create the waiter before navigating, and call Wait() > after navigating. otherwise there's the potential for races. so i'm not sure i > recomment adding helpers like this - better to inline in each test method > instead. Ack. We should ensure to create the Waiter before any navigation. I added SessionRestoreFirstMeaningfulPaintWaiter to create a Waiter right after the creation of a restored tab and before any of its navigations. https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1192: "/page_load_metrics/main_frame_with_iframe.html"); On 2017/08/09 15:41:34, Bryan McQuade wrote: > can we use title2.html to be consistent? Acknowledged. I will change this to title2.html. https://codereview.chromium.org/2930013005/diff/740001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2930013005/diff/740001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:122: testers_[i]->SimulateTimingUpdate(timing_); On 2017/08/09 15:41:34, Bryan McQuade wrote: > this is odd. we never dispatch the same timing update to all webcontents like > this in real code. we should only dispatch the timing update to the tester it's > intended for. i worry that doing this makes it less likely that you're > exercising the real behavior in your tests. could you instead target the > dispatch to the specific webcontents you want that timing to be sent to? So > you'd perhaps update testers_ to be a map from WebContents* to the associated > tester for that WC, add a param to this method that takes a WebContents*, and > then dispatch via the map. Done. https://codereview.chromium.org/2930013005/diff/740001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:260: StopLoading(web_contents()); On 2017/08/09 15:41:35, Bryan McQuade wrote: > does loading not stop on its own when the page being loaded fires the load > event? i'm still a bit surprised this is needed. Acknowledged. This is unnecessary and I will remove it. https://codereview.chromium.org/2930013005/diff/740001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2930013005/diff/740001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1165: auto waiter = base::MakeUnique<PageLoadMetricsWaiter>(web_contents); On 2017/08/09 15:41:35, Bryan McQuade wrote: > let's not do this - it's likely to be racy. please first instantiate the waiter, > than initiate the navigation, then wait. > > i'll probably follow this change with code that prohibits this use of the waiter > class, as it's not intended. Done. I added SessionRestoreFirstMeaningfulPaintWaiter to add the Waiter for the restored tabs when they are created. https://codereview.chromium.org/2930013005/diff/740001/chrome/browser/session... File chrome/browser/sessions/session_restore.h (right): https://codereview.chromium.org/2930013005/diff/740001/chrome/browser/session... chrome/browser/sessions/session_restore.h:125: FRIEND_TEST_ALL_PREFIXES(SessionRestorePageLoadMetricsObserverTest, On 2017/08/09 15:41:35, Bryan McQuade wrote: > eek, it's slightly concerning to me that we need to call private methods from > tests - it's usually a hint that we may not be testing the real production > behavior directly. is this standard practice for session restore tests? i don't > know the code well enough to make a firm call here, but this is slightly > worrying to me. I will remove these because OnWillRestoreTab() is made public in a related CL (https://chromium-review.googlesource.com/c/601545).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Bryan: Could you PTAL once more? I believe I addressed all of your important comments. Thanks! https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2930013005/diff/620001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1301: active_web_contents->Stop(); On 2017/08/09 15:41:34, Bryan McQuade wrote: > On 2017/08/07 at 14:30:08, Charlie Harrison wrote: > > I'm not quite sure why this is needed? Shouldn't loading a new URL in the > contents automatically stop the previous navigation? > > > > How does it affect paint metrics? > > agree - if you have to call this method, something else seems wrong here, and we > should figure out what's going on. I removed the Stop() and added some calculation to distinguish the first paints of the first navigation and the second navigation to test that no metrics of the second navigation are recorded.
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! These tests are great! A couple small changes requested then we are good to commit. https://codereview.chromium.org/2930013005/diff/820001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2930013005/diff/820001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:162: ASSERT_NO_FATAL_FAILURE(SimulateTimingUpdateForTab(web_contents())); Interesting, I have not used ASSERT_NO_FATAL_FAILURE before. Why is it needed here? I would expect the ASSERT_TRUE() inside SimulateTimingUpdateForTab to be sufficient w/o the need to also use ASSERT_NO_FATAL_FAILURE here. Just so I can better understand, can you share why you are using ASSERT_NO_FATAL_FAILURE here? Does this help to show which call site of SimulateTimingUpdateForTab failed in the stack trace printed on the console? If so, that seems helpful - good idea! https://codereview.chromium.org/2930013005/diff/820001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2930013005/diff/820001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1201: class SessionRestoreFirstMeaningfulPaintWaiter : public SessionRestoreObserver { let's call this SessionRestorePaintWater and wait for all types of paints we expect https://codereview.chromium.org/2930013005/diff/820001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1214: waiter->AddPageExpectation(TimingField::FIRST_MEANINGFUL_PAINT); we should include all TimingFields we're testing for here. So also add an expectation for FIRST_PAINT and FIRST_CONTENTFUL_PAINT. https://codereview.chromium.org/2930013005/diff/820001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1219: void WaitForForegroundTabs() { it strikes me that if OnWillRestoreTab isn't called for the tab we expect it to be, we'll silently fail to wait on that tab. So I'd like to make the tests also assert the number of tabs they expect to wait on. Let's change this function signature to take an int num_expected_foreground_tabs argument, and then count the number of tabs we wait on in the loop below, and EXPECT_EQ(num_expected_foreground_tabs, num_actual_foreground_tabs); after the for loop is complete. https://codereview.chromium.org/2930013005/diff/820001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1309: ASSERT_NO_FATAL_FAILURE(WaitForTabsToLoad(new_browser)); is the call to WaitForTabsToLoad needed here? seems like the paint waiter should be sufficient? https://codereview.chromium.org/2930013005/diff/820001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1326: ui_test_utils::NavigateToURL(new_browser, GetTestURL()); is the purpose of this test to verify that this particular navigation is not counted as a session restore? if so, you can change your code above: ui_test_utils::NavigateToURL(browser(), GetTestURL2()); to instead call NavigateToUntrackedUrl(); because this nav is not to an http/https URL, it won't be counted, and it will allow you to be certain that the navigation is never counted. so you no longer need the EXPECT_LE and related code - you can just verify that no histograms were logged at the end of this test.
https://codereview.chromium.org/2930013005/diff/820001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2930013005/diff/820001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer_unittest.cc:162: ASSERT_NO_FATAL_FAILURE(SimulateTimingUpdateForTab(web_contents())); On 2017/08/10 12:36:05, Bryan McQuade wrote: > Interesting, I have not used ASSERT_NO_FATAL_FAILURE before. Why is it needed > here? I would expect the ASSERT_TRUE() inside SimulateTimingUpdateForTab to be > sufficient w/o the need to also use ASSERT_NO_FATAL_FAILURE here. Just so I can > better understand, can you share why you are using ASSERT_NO_FATAL_FAILURE here? > Does this help to show which call site of SimulateTimingUpdateForTab failed in > the stack trace printed on the console? If so, that seems helpful - good idea! The function SimulateTimingUpdateForTab() uses an ASSERT_EQ whose failure does not propagate to the call sites. ASSERT_NO_FATAL_FAILURE will stop the test if the called function has an ASSERT failure, without it the test will continue. ASSERT_NO_FATAL_FAILURE indeed shows which call sites where SimulateTimingUpdateForTab failed. (https://github.com/google/googletest/issues/23) Thanks to sky@ to tell me to use ASSERT_NO_FATAL_FAILURE in a similar case in my recent CL. https://chromium-review.googlesource.com/c/601545/11/chrome/browser/sessions/... https://codereview.chromium.org/2930013005/diff/820001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right): https://codereview.chromium.org/2930013005/diff/820001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1201: class SessionRestoreFirstMeaningfulPaintWaiter : public SessionRestoreObserver { On 2017/08/10 12:36:05, Bryan McQuade wrote: > let's call this SessionRestorePaintWater and wait for all types of paints we > expect Done. https://codereview.chromium.org/2930013005/diff/820001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1214: waiter->AddPageExpectation(TimingField::FIRST_MEANINGFUL_PAINT); On 2017/08/10 12:36:05, Bryan McQuade wrote: > we should include all TimingFields we're testing for here. So also add an > expectation for FIRST_PAINT and FIRST_CONTENTFUL_PAINT. Done. https://codereview.chromium.org/2930013005/diff/820001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1219: void WaitForForegroundTabs() { On 2017/08/10 12:36:05, Bryan McQuade wrote: > it strikes me that if OnWillRestoreTab isn't called for the tab we expect it to > be, we'll silently fail to wait on that tab. So I'd like to make the tests also > assert the number of tabs they expect to wait on. Let's change this function > signature to take an int num_expected_foreground_tabs argument, and then count > the number of tabs we wait on in the loop below, and > EXPECT_EQ(num_expected_foreground_tabs, num_actual_foreground_tabs); after the > for loop is complete. Done. https://codereview.chromium.org/2930013005/diff/820001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1309: ASSERT_NO_FATAL_FAILURE(WaitForTabsToLoad(new_browser)); On 2017/08/10 12:36:05, Bryan McQuade wrote: > is the call to WaitForTabsToLoad needed here? seems like the paint waiter should > be sufficient? Acknowledged. I is unnecessary and I will remove it. https://codereview.chromium.org/2930013005/diff/820001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:1326: ui_test_utils::NavigateToURL(new_browser, GetTestURL()); On 2017/08/10 12:36:05, Bryan McQuade wrote: > is the purpose of this test to verify that this particular navigation is not > counted as a session restore? if so, you can change your code above: > ui_test_utils::NavigateToURL(browser(), GetTestURL2()); > to instead call > NavigateToUntrackedUrl(); > > because this nav is not to an http/https URL, it won't be counted, and it will > allow you to be certain that the navigation is never counted. so you no longer > need the EXPECT_LE and related code - you can just verify that no histograms > were logged at the end of this test. Yes, this test case checks that the second navigation should not have any UMA recorded because it is out of session restore. Done. Thank you for the suggestion!
The CQ bit was checked by ducbui@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ducbui@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from chrisha@chromium.org, mpearson@chromium.org, lpy@chromium.org, fmeawad@chromium.org, csharrison@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2930013005/#ps840001 (title: "Address Bryan comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 840001, "attempt_start_ts": 1502392642529330,
"parent_rev": "c0b9b583846d5fa37fe71e751076a2e903cc83e3", "commit_rev":
"f151b48d33647f93c9703fbd6eae497ff550faae"}
CQ is committing da patch.
Bot data: {"patchset_id": 840001, "attempt_start_ts": 1502392642529330,
"parent_rev": "446cad57bb512c52d39a94cd7e469ef6e94dfdd5", "commit_rev":
"2fd90c9e304afdadb3b95cd810df485098793e4c"}
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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} Committed: https://chromium.googlesource.com/chromium/src/+/2fd90c9e304afdadb3b95cd810df... ==========
Message was sent while issue was closed.
Committed patchset #43 (id:840001) as https://chromium.googlesource.com/chromium/src/+/2fd90c9e304afdadb3b95cd810df...
Message was sent while issue was closed.
https://codereview.chromium.org/2930013005/diff/840001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/840001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:40: return (started_in_foreground && just noticed - any reason we stopped testing for RELOAD transition type and/or a valid restore type? seems like that would only serve to make the metric more accurate.
Message was sent while issue was closed.
https://codereview.chromium.org/2930013005/diff/840001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2930013005/diff/840001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:40: return (started_in_foreground && On 2017/08/12 14:10:36, Bryan McQuade wrote: > just noticed - any reason we stopped testing for RELOAD transition type and/or a > valid restore type? seems like that would only serve to make the metric more > accurate. I think the test may be redundant because a tab is set to be in session restore only in OnWillRestoreTab() whose execution implies that the transition type is RELOAD and the restore type is from last session. I put some DCHECKs to test RELOAD transition and valid restore types; the DCHECKs have no failure in tests (https://chromium-review.googlesource.com/c/612592). Therefore, I believe that current filtering conditions may fail only in very corner cases. I will change the DCHECKs to the conditions in the observer if you think they are necessary.
Message was sent while issue was closed.
Thanks! DCHECK seems like the right thing here. if they start to fire we can investigate why, and possibly promote them to non-DCHECKs. On Sun, Aug 13, 2017 at 7:56 PM <ducbui@google.com> wrote: > > > https://codereview.chromium.org/2930013005/diff/840001/chrome/browser/page_lo... > File > > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc > (right): > > > https://codereview.chromium.org/2930013005/diff/840001/chrome/browser/page_lo... > > chrome/browser/page_load_metrics/observers/session_restore_page_load_metrics_observer.cc:40: > return (started_in_foreground && > On 2017/08/12 14:10:36, Bryan McQuade wrote: > > just noticed - any reason we stopped testing for RELOAD transition > type and/or a > > valid restore type? seems like that would only serve to make the > metric more > > accurate. > > I think the test may be redundant because a tab is set to be in session > restore > only in OnWillRestoreTab() whose execution implies that the transition > type is > RELOAD and the restore type is from last session. > > I put some DCHECKs to test RELOAD transition and valid restore types; > the > DCHECKs have no failure in tests > (https://chromium-review.googlesource.com/c/612592). Therefore, I > believe > that current filtering conditions may fail only in very corner cases. > > I will change the DCHECKs to the conditions in the observer if you think > they > are necessary. > > https://codereview.chromium.org/2930013005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
