Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1357403003:
Separate page load metrics for backgrounded pages (Closed)
|
Created:
5 years, 3 months ago by Charlie Harrison Modified:
5 years, 2 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org, clamy Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSeparate page load metrics for backgrounded pages
BUG=382542
Committed: https://crrev.com/1fb23d444bce951a4c169e0e06589fc2c147ec2d
Cr-Commit-Position: refs/heads/master@{#351587}
Patch Set 1 #Patch Set 2 : Used DidFinishLoad instead of DidStopLoading #
Total comments: 4
Patch Set 3 : Keep track of all provisional navigation handles + general refactor #
Total comments: 20
Patch Set 4 : Bryan review notes: change provisional vector to map #
Total comments: 18
Patch Set 5 : Update for Navigation API changes #Patch Set 6 : minor changes per Bryan review #
Total comments: 3
Patch Set 7 : Histogram BG suffix and updated bg logic + test #Patch Set 8 : Only log first background time #
Total comments: 4
Patch Set 9 : Alexei review #Patch Set 10 : Can't record raw delta before we have nav_start. Be more lazy. #
Total comments: 21
Patch Set 11 : Bryan review, added csharrison to histogram OWNERS #Patch Set 12 : Added a started_in_foreground boolean to PageLoadTracker for clarity #
Total comments: 1
Patch Set 13 : Added new histograms to replace the corrupted old ones #
Total comments: 2
Patch Set 14 : DCHECK nit #
Dependent Patchsets: Messages
Total messages: 36 (4 generated)
csharrison@chromium.org changed reviewers: + asvitkine@chromium.org, bmcquade@chromium.org, ttuttle@chromium.org
The change should accurately keep track of whether a web contents had a page load in the background or foreground. PTAL Bryan. Tuttle we need you cause you're a committer. asvitkine could you please review the histograms? Timing metrics are heavily skewed for backgrounded tabs, because layout and paint are deferred on them until foreground.
@clamy, I was originally using DidFinishNavigation for tracking when the document load finishes. I noticed in your change you substituted DidFinishNavigation with DidStopLoading, but I need the render_frame_host to check if it's the main frame. My understanding is that these calls will roughly coincide with calls to DidStopLoading / DidFinishNavigation.
https://codereview.chromium.org/1357403003/diff/20001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1357403003/diff/20001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:57: in_foreground_(false) {} can webcontents ever start foregrounded (such that we would not get an initial WasShown callback)?
clamy@chromium.org changed reviewers: + clamy@chromium.org
@csharrison: DidStopLoading will only be called when the entire page has stopped loading. In a normal navigation, this corresponds to the time the main frame stopped loading (which happens after all subframes stopped loading as well). I have some additional comments, see below. I don't think the patch as it is implemented currently will handle all navigation edge cases. https://codereview.chromium.org/1357403003/diff/20001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1357403003/diff/20001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:96: current_navigation_stayed_in_foreground_ = in_foreground_; This could break down in certain edge cases. You make the assumption that there is always one navigation in the WebContents at the same time. However this is not true. Here is what can happen: 1) You start on the page slow.com with the tab in the background. 2) A javascript navigation to slow.com/foo starts. -> current_navigation_stayed_in_foreground_ is false. 3) The tab gets in the foreground before slow.com commits. 4) You type in the omnibox and start a second concurrent navigation to slowaswell.com. It's cross-process so it can go on at the same time as the navigation to slow.com/foo. -> current_navigation_stayed_in_foreground_ is true. 5) The navigation to slow.com/foo commits first. This cancels the navigation to slowaswell.com. However current_navigation_stayed_in_foreground_ is still true, while this navigation actually started in the background. If you want to do things cleanly, I don't think you can escape tracking the 2 possible NavigationHandles and whether they stayed in foreground all the time separately, and only get to a general value for the tab once one of them commits (which with my patch corresponds to DidFinishNavigation). Not to mention that there is a special case in which you will see a commit from a same-document navigation, but it will not cancel an ongoing cross-site navigation.
https://codereview.chromium.org/1357403003/diff/20001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1357403003/diff/20001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:96: current_navigation_stayed_in_foreground_ = in_foreground_; On 2015/09/22 23:54:31, clamy wrote: > This could break down in certain edge cases. You make the assumption that there > is always one navigation in the WebContents at the same time. However this is > not true. Here is what can happen: > 1) You start on the page http://slow.com with the tab in the background. > 2) A javascript navigation to slow.com/foo starts. -> > current_navigation_stayed_in_foreground_ is false. > 3) The tab gets in the foreground before http://slow.com commits. > 4) You type in the omnibox and start a second concurrent navigation to > http://slowaswell.com. It's cross-process so it can go on at the same time as the > navigation to slow.com/foo. -> current_navigation_stayed_in_foreground_ is true. > 5) The navigation to slow.com/foo commits first. This cancels the navigation to > http://slowaswell.com. However current_navigation_stayed_in_foreground_ is still true, > while this navigation actually started in the background. > > If you want to do things cleanly, I don't think you can escape tracking the 2 > possible NavigationHandles and whether they stayed in foreground all the time > separately, and only get to a general value for the tab once one of them commits > (which with my patch corresponds to DidFinishNavigation). Not to mention that > there is a special case in which you will see a commit from a same-document > navigation, but it will not cancel an ongoing cross-site navigation. Similarly, we can have the following case: 1) You start a prerender to foo.com. The prerender contents is in background. 2) You start a request to foo.com which means your prerender WebContents become the actual WebContents for the tab. The prerender has not finished loading but it has committed the navigation. current_navigation_stayed_in_foreground_ is false. 3) You start a cross-site navigation to bar.com from the omnibox. current_navigation_stayed_in_foreground_ becomes true. 4) You cancel it before it commits. current_navigation_stayed_in_foreground_ is unchanged. 5) The prerender navigation to foo.com finally finishes -> current_navigation_stayed_in_foreground_ is still true when it should have been false.
On 2015/09/23 00:06:18, clamy wrote: > https://codereview.chromium.org/1357403003/diff/20001/components/page_load_me... > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > (right): > > https://codereview.chromium.org/1357403003/diff/20001/components/page_load_me... > components/page_load_metrics/browser/metrics_web_contents_observer.cc:96: > current_navigation_stayed_in_foreground_ = in_foreground_; > On 2015/09/22 23:54:31, clamy wrote: > > This could break down in certain edge cases. You make the assumption that > there > > is always one navigation in the WebContents at the same time. However this is > > not true. Here is what can happen: > > 1) You start on the page http://slow.com with the tab in the background. > > 2) A javascript navigation to slow.com/foo starts. -> > > current_navigation_stayed_in_foreground_ is false. > > 3) The tab gets in the foreground before http://slow.com commits. > > 4) You type in the omnibox and start a second concurrent navigation to > > http://slowaswell.com. It's cross-process so it can go on at the same time as > the > > navigation to slow.com/foo. -> current_navigation_stayed_in_foreground_ is > true. > > 5) The navigation to slow.com/foo commits first. This cancels the navigation > to > > http://slowaswell.com. However current_navigation_stayed_in_foreground_ is > still true, > > while this navigation actually started in the background. > > > > If you want to do things cleanly, I don't think you can escape tracking the 2 > > possible NavigationHandles and whether they stayed in foreground all the time > > separately, and only get to a general value for the tab once one of them > commits > > (which with my patch corresponds to DidFinishNavigation). Not to mention that > > there is a special case in which you will see a commit from a same-document > > navigation, but it will not cancel an ongoing cross-site navigation. > > Similarly, we can have the following case: > 1) You start a prerender to http://foo.com. The prerender contents is in background. > 2) You start a request to http://foo.com which means your prerender WebContents become > the actual WebContents for the tab. The prerender has not finished loading but > it has committed the navigation. current_navigation_stayed_in_foreground_ is > false. > 3) You start a cross-site navigation to http://bar.com from the omnibox. > current_navigation_stayed_in_foreground_ becomes true. > 4) You cancel it before it commits. current_navigation_stayed_in_foreground_ is > unchanged. > 5) The prerender navigation to http://foo.com finally finishes -> > current_navigation_stayed_in_foreground_ is still true when it should have been > false. Thanks for the extremely detailed reply! I'll update the patch today with fixes, and those scenarios will make great test cases :)
@clamy does DidFinishLoad wait until all subframes are loaded? I thought it corresponded to the onload event which doesn't wait for iframes?
https://codereview.chromium.org/1357403003/diff/20001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1357403003/diff/20001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:57: in_foreground_(false) {} On 2015/09/22 23:27:47, Bryan McQuade wrote: > can webcontents ever start foregrounded (such that we would not get an initial > WasShown callback)? I couldn't find a case where we don't get the WasShown callback, but it's worth investigating. I could put a DCHECK in WasHidden to assert that we were foregrounded.
On 2015/09/23 22:15:24, csharrison wrote: > https://codereview.chromium.org/1357403003/diff/20001/components/page_load_me... > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > (right): > > https://codereview.chromium.org/1357403003/diff/20001/components/page_load_me... > components/page_load_metrics/browser/metrics_web_contents_observer.cc:57: > in_foreground_(false) {} > On 2015/09/22 23:27:47, Bryan McQuade wrote: > > can webcontents ever start foregrounded (such that we would not get an initial > > WasShown callback)? > > I couldn't find a case where we don't get the WasShown callback, but it's worth > investigating. I could put a DCHECK in WasHidden to assert that we were > foregrounded. Also seems like we're getting more than 2 provisional loads in some of those browsertests. Will investigate.
On 2015/09/23 22:36:30, csharrison wrote: > On 2015/09/23 22:15:24, csharrison wrote: > > > https://codereview.chromium.org/1357403003/diff/20001/components/page_load_me... > > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > > (right): > > > > > https://codereview.chromium.org/1357403003/diff/20001/components/page_load_me... > > components/page_load_metrics/browser/metrics_web_contents_observer.cc:57: > > in_foreground_(false) {} > > On 2015/09/22 23:27:47, Bryan McQuade wrote: > > > can webcontents ever start foregrounded (such that we would not get an > initial > > > WasShown callback)? > > > > I couldn't find a case where we don't get the WasShown callback, but it's > worth > > investigating. I could put a DCHECK in WasHidden to assert that we were > > foregrounded. > > Also seems like we're getting more than 2 provisional loads in some of those > browsertests. Will investigate. The problem with these tests are we get 3 provisional navigations, usually 1. "" an empty url, caused by opening a new window for the download 2. "about:blank" 3. The real download / navigation Would it be wrong to filter out blank urls / about:blank from our logic? Can they ever be redirects?
https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (left): https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:76: if (navigation_handle->IsInMainFrame() && !navigation_handle->IsSamePage()) why did the IsSamePage test go away? https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:60: page_load_stayed_in_foreground_(in_foreground) {} also need to set initial values for has_commit_ and has_finished_, else the memory for those fields will be uninitialized in release builds (and maybe in debug builds). https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:131: committed_load_(nullptr) {} scoped_ptr has a default constructor which initializes to nullptr, so you can omit this. https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:156: for (auto iter = provisional_loads_.begin(); if there is a real 1:1 relationship between NavigationHandle and a PageLoadTracker then I'd prefer to use a map<const NavigationHandle*, PageLoadTracker> (or, better, a ScopedPtrMap or ScopedPtrHashMap: https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/sc...) since it both makes accessing the right PageLoadTracker more straightforward and more clearly declares the relationship between the NavigationHandle and a PageLoadTracker. Even if a NavigationHandle can change for a PageLoadTracker (in-page navigation) it may still be good to use a map and just update the entry as a given PageLoadTracker's NavigationHandle changes. If you switch to a map, are you able to stop storing the NavigationHandle as a member of a PageLoadTracker? If so, that also seems like a nice reason to make the change (less for the PageLoadTracker to know about). https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:160: committed_load_.reset(new PageLoadTracker(*iter)); I'd rather avoid allocating a new instance of the same PageLoadTracker via the implicit copy constructor. If non-POD types get added to PageLoadTracker in the future, this can stop compiling. If you switch to the ScopedPtrMap for provisional_loads_, you get that for free. Then your logic here becomes something like: if (!IsRelevantNavigation(navigation_handle)) { committed_load_.reset(); return; } committed_load_ = provisional_loads_.take_and_erase(navigation_handle); // make sure we found a match for the navigation_handle in our // provisional_loads_ map. DCHECK(committed_load_); I think this should work but might require some small changes. https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:175: provisional_loads_.push_back( based on the dcheck, it sounds like we can have up to 2 provisional loads at a time, since you will DCHECK that there are zero or one provisional loads, then add another one. Is that right? If there can be 2, can you add a short comment to explain the case where that can happen? https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:32: PageLoadTracker(content::NavigationHandle* navigation_handle, one concern with holding on to a NavigationHandle is that in-page navigations may cause a single 'page load' to have more than one NavigationHandle (unless the same handle is re-used for in-page navigations). https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:37: void Finish(); I know the term 'finish' is used in the WCO API but I find it relatively vague. What does it mean for a page load to 'finish'? Does it mean the page reached the load event? I think it's worth adding a comment here and above DidFinishLoad to describe the points during a web page's lifetime where this method gets called. https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:51: content::NavigationHandle* navigation_handle_; if you don't expect this value to change after construction, you can declare the pointer as const: content::NavigationHandle* const navigation_handle_; this becomes a hint to readers that the member is set in the constructor and won't change, and also enforces that so you can safely make that assumption throughout your own code. https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:59: PageLoadTiming timing_; should add DISALLOW_COPY_AND_ASSIGN(PageLoadTracker); here https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:77: void DidFinishLoad(content::RenderFrameHost* render_frame_host, related to above comment, let's add a comment explaining what 'Finish' means. Is this reaching the load event, or something else?
DidFinishLoad is also called when all subframes have finished loading. I'm not sure there is a callback for just the main frame, but we need to change this if we don't want to wait for subframes. https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:60: page_load_stayed_in_foreground_(in_foreground) {} On 2015/09/24 15:14:11, Bryan McQuade wrote: > also need to set initial values for has_commit_ and has_finished_, else the > memory for those fields will be uninitialized in release builds (and maybe in > debug builds). Done. https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:131: committed_load_(nullptr) {} On 2015/09/24 15:14:11, Bryan McQuade wrote: > scoped_ptr has a default constructor which initializes to nullptr, so you can > omit this. Done. https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:156: for (auto iter = provisional_loads_.begin(); On 2015/09/24 15:14:11, Bryan McQuade wrote: > if there is a real 1:1 relationship between NavigationHandle and a > PageLoadTracker then I'd prefer to use a map<const NavigationHandle*, > PageLoadTracker> (or, better, a ScopedPtrMap or ScopedPtrHashMap: > https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/sc...) > since it both makes accessing the right PageLoadTracker more straightforward and > more clearly declares the relationship between the NavigationHandle and a > PageLoadTracker. > > Even if a NavigationHandle can change for a PageLoadTracker (in-page navigation) > it may still be good to use a map and just update the entry as a given > PageLoadTracker's NavigationHandle changes. > > If you switch to a map, are you able to stop storing the NavigationHandle as a > member of a PageLoadTracker? If so, that also seems like a nice reason to make > the change (less for the PageLoadTracker to know about). I considered this, but the PageLoadTracker has a longer lifetime than the NavigationHandle, making the NavigationHandle a weird key for a map. I agree it would make things simpler, though the PageLoadTracker might still need to hold on to the NavigationHandle if we ever want it to know things like it's URL. https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:175: provisional_loads_.push_back( On 2015/09/24 15:14:11, Bryan McQuade wrote: > based on the dcheck, it sounds like we can have up to 2 provisional loads at a > time, since you will DCHECK that there are zero or one provisional loads, then > add another one. Is that right? If there can be 2, can you add a short comment > to explain the case where that can happen? Sure, this is documented in clamy's replies. There seem to be cases in our browser tests where we have 3 provisional loads, actually. I'm still investigating that. https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:32: PageLoadTracker(content::NavigationHandle* navigation_handle, On 2015/09/24 15:14:11, Bryan McQuade wrote: > one concern with holding on to a NavigationHandle is that in-page navigations > may cause a single 'page load' to have more than one NavigationHandle (unless > the same handle is re-used for in-page navigations). In this case, we don't know that an in-page is really in-page until it commits, in which case we delete the PageLoadTracker. Before commit we keep track of all navigations separately. https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:51: content::NavigationHandle* navigation_handle_; On 2015/09/24 15:14:11, Bryan McQuade wrote: > if you don't expect this value to change after construction, you can declare the > pointer as const: > content::NavigationHandle* const navigation_handle_; > > this becomes a hint to readers that the member is set in the constructor and > won't change, and also enforces that so you can safely make that assumption > throughout your own code. Done. https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:59: PageLoadTiming timing_; On 2015/09/24 15:14:11, Bryan McQuade wrote: > should add DISALLOW_COPY_AND_ASSIGN(PageLoadTracker); here Done.
https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:156: for (auto iter = provisional_loads_.begin(); On 2015/09/24 15:48:21, csharrison wrote: > On 2015/09/24 15:14:11, Bryan McQuade wrote: > > if there is a real 1:1 relationship between NavigationHandle and a > > PageLoadTracker then I'd prefer to use a map<const NavigationHandle*, > > PageLoadTracker> (or, better, a ScopedPtrMap or ScopedPtrHashMap: > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/sc...) > > since it both makes accessing the right PageLoadTracker more straightforward > and > > more clearly declares the relationship between the NavigationHandle and a > > PageLoadTracker. > > > > Even if a NavigationHandle can change for a PageLoadTracker (in-page > navigation) > > it may still be good to use a map and just update the entry as a given > > PageLoadTracker's NavigationHandle changes. > > > > If you switch to a map, are you able to stop storing the NavigationHandle as a > > member of a PageLoadTracker? If so, that also seems like a nice reason to make > > the change (less for the PageLoadTracker to know about). > > I considered this, but the PageLoadTracker has a longer lifetime than the > NavigationHandle, making the NavigationHandle a weird key for a map. I agree it > would make things simpler, though the PageLoadTracker might still need to hold > on to the NavigationHandle if we ever want it to know things like it's URL. Ah, I see. Let's chat about this in person (Monday?). https://codereview.chromium.org/1357403003/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:175: provisional_loads_.push_back( On 2015/09/24 15:48:21, csharrison wrote: > On 2015/09/24 15:14:11, Bryan McQuade wrote: > > based on the dcheck, it sounds like we can have up to 2 provisional loads at a > > time, since you will DCHECK that there are zero or one provisional loads, then > > add another one. Is that right? If there can be 2, can you add a short comment > > to explain the case where that can happen? > > Sure, this is documented in clamy's replies. There seem to be cases in our > browser tests where we have 3 provisional loads, actually. I'm still > investigating that. ah, ok, cool. let's just make sure to add a comment in the code explaining it, so future readers of the code can understand the dcheck.
https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (left): https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:97: if (!current_timing_) should we still continue to return if committed_load_ is null? https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:68: if (!has_finished_) let's chat about this and some possible alternatives a bit next week. onload is probably a decent signal for many pages, but there are others where the entire page load, from a user standpoint, happens entirely after onload. if we can come up with a mechanism that doesn't care about onload i think i'd prefer that. one alternative would be to log metrics observed up until the time the web contents got hidden, and then stop recording metrics for that page. that way, we log metrics for what the user experienced only. let's chat more next week. https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:161: provisional_loads_.erase(navigation_handle); should we also call committed_load_.reset(); here to force logging of the previous tracker if any? Perhaps instead we should just do that unconditionally above the call to IsRelevantNavigation(...) along with a comment just to be super explicit that we want to destruct the previous one if any in order to trigger recording of histograms. https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:201: for (auto kv : provisional_loads_) { auto is awesome but subtle. if you just say 'auto' it's equivalent to just typing the type of the object (non-const, non-reference) which can cause a copy constructor to be invoked in some cases as you iterate each element. just to be sure to avoid that, i like to do: const auto& kv : provisional_loads_ or if operating on non-const data: auto& kv : provisional_loads_ which avoids a copy in many cases https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:17: for forward-declarations, I tend to not see these newlines in code (though I'm less familiar with chromium code). Can you look at other chromium code to get a sense for whether newlines are common in forward declarations? https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:42: void RecordTimingHistograms(); can we make RecordTimingHistograms private, since it's only called by the destructor.
Thanks! A few drop-by comments. The 3 provisional load you are seeing is likely due to you not erasing a failed one. It _is not_ possible to have 3 NavigationHandles for the main frame. https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:17: On 2015/09/24 18:21:52, Bryan McQuade wrote: > for forward-declarations, I tend to not see these newlines in code (though I'm > less familiar with chromium code). Can you look at other chromium code to get a > sense for whether newlines are common in forward declarations? Both are acceptable. https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:73: content::NavigationHandle* navigation_handle) override; I notice that you do not implement the DidFinishNavigation method, while tracking NavigationHandles. This is wrong, as you may end up tracking NavigationHandles that have been deleted when the provisional load fails (DidCommitNavigation is not called in those cases). Also, this patch should land soon: https://codereview.chromium.org/1350673003/ which will remove the DidCommitNavigation function in favor of DidFinishNavigation. Unless your patch lands first, you will need to rebase upon it.
On 2015/09/24 18:35:59, clamy wrote: > Thanks! A few drop-by comments. > > The 3 provisional load you are seeing is likely due to you not erasing a failed > one. It _is not_ possible to have 3 NavigationHandles for the main frame. > > https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... > File components/page_load_metrics/browser/metrics_web_contents_observer.h > (right): > > https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... > components/page_load_metrics/browser/metrics_web_contents_observer.h:17: > On 2015/09/24 18:21:52, Bryan McQuade wrote: > > for forward-declarations, I tend to not see these newlines in code (though I'm > > less familiar with chromium code). Can you look at other chromium code to get > a > > sense for whether newlines are common in forward declarations? > > Both are acceptable. > > https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... > components/page_load_metrics/browser/metrics_web_contents_observer.h:73: > content::NavigationHandle* navigation_handle) override; > I notice that you do not implement the DidFinishNavigation method, while > tracking NavigationHandles. This is wrong, as you may end up tracking > NavigationHandles that have been deleted when the provisional load fails > (DidCommitNavigation is not called in those cases). > > Also, this patch should land soon: https://codereview.chromium.org/1350673003/ > which will remove the DidCommitNavigation function in favor of > DidFinishNavigation. Unless your patch lands first, you will need to rebase upon > it. @clamy thanks for the reassurance about NavigationHandles. That is certainly a relief :) Looks like your patch is going to get in before mine so I'll just update DidFinishNavigation to take care of failure conditions.
https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:68: if (!has_finished_) On 2015/09/24 18:21:52, Bryan McQuade wrote: > let's chat about this and some possible alternatives a bit next week. onload is > probably a decent signal for many pages, but there are others where the entire > page load, from a user standpoint, happens entirely after onload. if we can come > up with a mechanism that doesn't care about onload i think i'd prefer that. one > alternative would be to log metrics observed up until the time the web contents > got hidden, and then stop recording metrics for that page. that way, we log > metrics for what the user experienced only. let's chat more next week. Sounds good. I'm hesitant about logging partial structs before background unless we name them something else. Otherwise they will be conflated with failed/aborted loads. https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:161: provisional_loads_.erase(navigation_handle); On 2015/09/24 18:21:52, Bryan McQuade wrote: > should we also call > committed_load_.reset(); > here to force logging of the previous tracker if any? > > Perhaps instead we should just do that unconditionally above the call to > IsRelevantNavigation(...) along with a comment just to be super explicit that we > want to destruct the previous one if any in order to trigger recording of > histograms. I'm not sure. Can't a same-page navigation commit before a page has fully loaded? Best to wait as long as possible to log so we don't log early. https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:201: for (auto kv : provisional_loads_) { On 2015/09/24 18:21:52, Bryan McQuade wrote: > auto is awesome but subtle. if you just say 'auto' it's equivalent to just > typing the type of the object (non-const, non-reference) which can cause a copy > constructor to be invoked in some cases as you iterate each element. just to be > sure to avoid that, i like to do: > const auto& kv : provisional_loads_ > or if operating on non-const data: > auto& kv : provisional_loads_ > which avoids a copy in many cases Yeah good idea. https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:42: void RecordTimingHistograms(); On 2015/09/24 18:21:52, Bryan McQuade wrote: > can we make RecordTimingHistograms private, since it's only called by the > destructor. This was originally a conscious choice I made in case the WebContentsObserver wanted to be eager about RecordingHistograms but I'm convinced now that's a bad choice. https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:73: content::NavigationHandle* navigation_handle) override; On 2015/09/24 18:35:58, clamy wrote: > I notice that you do not implement the DidFinishNavigation method, while > tracking NavigationHandles. This is wrong, as you may end up tracking > NavigationHandles that have been deleted when the provisional load fails > (DidCommitNavigation is not called in those cases). > > Also, this patch should land soon: https://codereview.chromium.org/1350673003/ > which will remove the DidCommitNavigation function in favor of > DidFinishNavigation. Unless your patch lands first, you will need to rebase upon > it. Good point. I was hesitating to add DidFinishNavigation due to your change.
https://codereview.chromium.org/1357403003/diff/100001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1357403003/diff/100001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:55: base::TimeDelta::FromMinutes(10), 100); Nit: No ; The caller will supply their own after the macro. https://codereview.chromium.org/1357403003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1357403003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:29710: +<histogram name="PageLoad.Timing.BG.NavigationToDOMContentLoadedEventFired" Consider using histogram_suffixes feature to permute these metrics with the extra "BG" string. See example/doc at top of this file for that. Maybe put it at the end of the name to match the suffix convention.
Here's the updated CL. We log a timestamp when a page is backgrounded for the first time, and compare timestamps to see if we should bucket it in the BG histogram or not. https://codereview.chromium.org/1357403003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1357403003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:29710: +<histogram name="PageLoad.Timing.BG.NavigationToDOMContentLoadedEventFired" On 2015/09/28 18:27:21, Alexei Svitkine wrote: > Consider using histogram_suffixes feature to permute these metrics with the > extra "BG" string. See example/doc at top of this file for that. > > Maybe put it at the end of the name to match the suffix convention. Thanks for this suggestion, that's cleaner.
https://codereview.chromium.org/1357403003/diff/140001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1357403003/diff/140001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:58: background_time = in_foreground ? base::TimeDelta::Max() : base::TimeDelta(); Nit: Make this just an if, since base::TimeDelta is the default value. https://codereview.chromium.org/1357403003/diff/140001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:189: } Nit: Add empty line after. https://codereview.chromium.org/1357403003/diff/140001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1357403003/diff/140001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:17: Nit: I don't think it's convention to add empty lines around namespaces for fwd declarations. https://codereview.chromium.org/1357403003/diff/140001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:51: base::TimeDelta background_time; Nit: _
On 2015/09/28 21:40:35, Alexei Svitkine wrote: > https://codereview.chromium.org/1357403003/diff/140001/components/page_load_m... > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > (right): > > https://codereview.chromium.org/1357403003/diff/140001/components/page_load_m... > components/page_load_metrics/browser/metrics_web_contents_observer.cc:58: > background_time = in_foreground ? base::TimeDelta::Max() : base::TimeDelta(); > Nit: Make this just an if, since base::TimeDelta is the default value. > > https://codereview.chromium.org/1357403003/diff/140001/components/page_load_m... > components/page_load_metrics/browser/metrics_web_contents_observer.cc:189: } > Nit: Add empty line after. > > https://codereview.chromium.org/1357403003/diff/140001/components/page_load_m... > File components/page_load_metrics/browser/metrics_web_contents_observer.h > (right): > > https://codereview.chromium.org/1357403003/diff/140001/components/page_load_m... > components/page_load_metrics/browser/metrics_web_contents_observer.h:17: > Nit: I don't think it's convention to add empty lines around namespaces for fwd > declarations. > > https://codereview.chromium.org/1357403003/diff/140001/components/page_load_m... > components/page_load_metrics/browser/metrics_web_contents_observer.h:51: > base::TimeDelta background_time; > Nit: _ Done! @clamy claims that either style is fine for fwd declarations, but I changed it anyways. I think the whitespace isn't needed there.
lgtm
https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (left): https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:86: void MetricsWebContentsObserver::RenderProcessGone( if removing this causes us to log metrics less eagerly, i think it's worth keeping it and explicitly invoking committed_load_.reset() here instead, with a short comment to explain. https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:102: base::TimeDelta background_delta = Let's be a little more explicit about the handling of the special max value. Something like: background_delta = background_time_.is_max() ? TimeDelta::Max() : background_time_ - timing.navigation_start; https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:161: let's move the DCHECK from below up here: DCHECK(finished_nav); https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:166: if (!navigation_handle->HasCommitted()) what's the difference between testing !HasCommitted and IsErrorPage? should we test both or perhaps add a DCHECK(IsErrorPage()) when HasCommitted is false here, just to sanity check our assumptions? https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:169: if (!IsRelevantNavigation(navigation_handle)) thinking about this more, I do think we should log and discard the current committed PageLoadTracker a bit more eagerly when we get a new non-in-page commit. I feel uneasy potentially holding on to data that should be logged until later, since we're then logging that data potentially long after the nav completes. As you point out, we should not discard and log on an in-page navigation, but I feel that we should discard and log for all main frame non-in-page navs. similar to how you do: if (!navigation_handle->HasCommitted()) return; can we add: // Do not treat in-page navigations as new page loads. if (navigation_handle->IsSamePage()) return; // Trigger recording of timing histograms for the previous PageLoadTracker, if any. committed_load_.reset(); if (!IsRelevantNavigation(navigation_handle)) return; committed_load_ = finished_nav.Pass(); committed_load_->Commit(); This makes sure we log more eagerly, and I like being explicit about the force-log step by calling reset(), rather than implicitly during the assignment from finished_nav.Pass(); https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:179: void MetricsWebContentsObserver::DidStartNavigation( let's put DidStartNavigation impl before DidFinishNavigation. https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:206: const PageLoadTiming& timing) { we used to have: if (!current_timing) return; which protected against getting an OnTimingUpdated when we weren't tracking metrics. Should we have something similar to make sure we don't end up calling committed_load_->UpdateTiming(...); on a null pointer, below? e.g.: if (!committed_load_) return; https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:29: PageLoadTracker(bool in_foreground); nit: single-arg constructor should be explicit https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:47: base::Time background_time_; let's use a TimeTicks here, just to be clear that we really prefer to operate in terms of monotonic clocks. We can convert to base::Time as we need to in the code, but let's have our members using monotonic times as much as possible. Eventually it'd be nice if we could convert navigation_start to a monotonic time as well, but that's a bigger change that spans from blink back to browser process, so can get to it later. https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:67: void DidStartNavigation( let's move DidStartNavigation's decl above DidFinishNavigation.
https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc (right): https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc:89: observer_->DidFinishLoad(web_contents()->GetMainFrame(), we probably no longer need these calls to DidFinishLoad, since you removed the logic related to finish https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc:313: } could we add one more test where some metrics occur before backgrounding, and some after? (to test the background_time_ logic) https://codereview.chromium.org/1357403003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1357403003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:29654: +<histogram name="PageLoad.Timing.BG.NavigationToDOMContentLoadedEventFired" do these names need to be updated (with BG at the end)? https://codereview.chromium.org/1357403003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:29656: + <owner>bmcquade@chromium.org</owner> btw you should make yourself an owner on all of these metrics as well
https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:102: base::TimeDelta background_delta = On 2015/09/29 00:32:19, Bryan McQuade wrote: > Let's be a little more explicit about the handling of the special max value. > Something like: > > background_delta = background_time_.is_max() ? TimeDelta::Max() : > background_time_ - timing.navigation_start; Actually, there are two cases we should handle explicitly, just so other readers of the code remember them as well: * background_time_ is max (webcontents was never backgrounded): delta should be max * background_time_ is zero (webcontents was backgrounded when constructed): delta should be zero The logic around this is just complex enough that it might be good to factor into a helper method in an anonymous namespace, but it's up to you.
Unfortunately TimeTicks has no Max(), so I changed the logic slightly so that no background was a null TimeTicks. https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc (right): https://codereview.chromium.org/1357403003/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc:313: } On 2015/09/29 00:43:01, Bryan McQuade wrote: > could we add one more test where some metrics occur before backgrounding, and > some after? (to test the background_time_ logic) It's already in the most updated patch :) https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (left): https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:86: void MetricsWebContentsObserver::RenderProcessGone( On 2015/09/29 00:32:19, Bryan McQuade wrote: > if removing this causes us to log metrics less eagerly, i think it's worth > keeping it and explicitly invoking committed_load_.reset() here instead, with a > short comment to explain. Yeah I think it could. Done. https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:102: base::TimeDelta background_delta = On 2015/09/29 00:32:19, Bryan McQuade wrote: > Let's be a little more explicit about the handling of the special max value. > Something like: > > background_delta = background_time_.is_max() ? TimeDelta::Max() : > background_time_ - timing.navigation_start; Good idea. Done. https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:161: On 2015/09/29 00:32:19, Bryan McQuade wrote: > let's move the DCHECK from below up here: > DCHECK(finished_nav); Done. https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:166: if (!navigation_handle->HasCommitted()) On 2015/09/29 00:32:19, Bryan McQuade wrote: > what's the difference between testing !HasCommitted and IsErrorPage? should we > test both or perhaps add a DCHECK(IsErrorPage()) when HasCommitted is false > here, just to sanity check our assumptions? IsErrorPage is true when HasCommitted is true and the navigation resulted in an error page. If we toss !HasCommitted and !IsErrorPage (in IsRelevantNavigation) then we don't log UMA for error'ed navigations. https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:169: if (!IsRelevantNavigation(navigation_handle)) On 2015/09/29 00:32:19, Bryan McQuade wrote: > thinking about this more, I do think we should log and discard the current > committed PageLoadTracker a bit more eagerly when we get a new non-in-page > commit. I feel uneasy potentially holding on to data that should be logged until > later, since we're then logging that data potentially long after the nav > completes. > > As you point out, we should not discard and log on an in-page navigation, but I > feel that we should discard and log for all main frame non-in-page navs. > > similar to how you do: > if (!navigation_handle->HasCommitted()) > return; > > can we add: > // Do not treat in-page navigations as new page loads. > if (navigation_handle->IsSamePage()) > return; > > // Trigger recording of timing histograms for the previous PageLoadTracker, if > any. > committed_load_.reset(); > > if (!IsRelevantNavigation(navigation_handle)) > return; > > committed_load_ = finished_nav.Pass(); > committed_load_->Commit(); > > This makes sure we log more eagerly, and I like being explicit about the > force-log step by calling reset(), rather than implicitly during the assignment > from finished_nav.Pass(); I have no objections to that! Done. https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:179: void MetricsWebContentsObserver::DidStartNavigation( On 2015/09/29 00:32:19, Bryan McQuade wrote: > let's put DidStartNavigation impl before DidFinishNavigation. Done. https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:206: const PageLoadTiming& timing) { On 2015/09/29 00:32:19, Bryan McQuade wrote: > we used to have: > if (!current_timing) > return; > > which protected against getting an OnTimingUpdated when we weren't tracking > metrics. > > Should we have something similar to make sure we don't end up calling > committed_load_->UpdateTiming(...); on a null pointer, below? e.g.: > if (!committed_load_) > return; I removed it under the assumption that DidStartNavigation would be called before any IPCs get sent. I think it'd be a good idea to keep it just in case though. https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:29: PageLoadTracker(bool in_foreground); On 2015/09/29 00:32:19, Bryan McQuade wrote: > nit: single-arg constructor should be explicit Done. https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:47: base::Time background_time_; On 2015/09/29 00:32:19, Bryan McQuade wrote: > let's use a TimeTicks here, just to be clear that we really prefer to operate in > terms of monotonic clocks. We can convert to base::Time as we need to in the > code, but let's have our members using monotonic times as much as possible. > Eventually it'd be nice if we could convert navigation_start to a monotonic time > as well, but that's a bigger change that spans from blink back to browser > process, so can get to it later. Done. https://codereview.chromium.org/1357403003/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:67: void DidStartNavigation( On 2015/09/29 00:32:19, Bryan McQuade wrote: > let's move DidStartNavigation's decl above DidFinishNavigation. Done.
LGTM https://codereview.chromium.org/1357403003/diff/220001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1357403003/diff/220001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:224: if (!committed_load_) re: your previous comment, if you do think committed_load_ should always be non-null in this case, let's also add a DCHECK(committed_load_); to validate that.
On 2015/09/29 18:05:57, Bryan McQuade wrote: > LGTM > > https://codereview.chromium.org/1357403003/diff/220001/components/page_load_m... > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > (right): > > https://codereview.chromium.org/1357403003/diff/220001/components/page_load_m... > components/page_load_metrics/browser/metrics_web_contents_observer.cc:224: if > (!committed_load_) > re: your previous comment, if you do think committed_load_ should always be > non-null in this case, let's also add a DCHECK(committed_load_); to validate > that. I'm not 100% confident, so I think it should stay. I think things could get racy especially when render processes die or other times we eagerly reset the committed load to log metrics. @ttuttle: friendly ping
LGTM with one nit. https://codereview.chromium.org/1357403003/diff/240001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1357403003/diff/240001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:36: DCHECK_IMPLIES( Whoa, I didn't know this was a thing. Cool. https://codereview.chromium.org/1357403003/diff/240001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:164: DCHECK(provisional_loads_.size() < 2); DCHECK_GT(2, provisional_loads_.size()); (It'll give you the actual value of .size() in the error instead of just "expected true, got false".)
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, bmcquade@chromium.org, ttuttle@chromium.org Link to the patchset: https://codereview.chromium.org/1357403003/#ps260001 (title: "DCHECK nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1357403003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1357403003/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/1fb23d444bce951a4c169e0e06589fc2c147ec2d Cr-Commit-Position: refs/heads/master@{#351587} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
