Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1476503004:
[page_load_metrics] User Initiated Abort Tracking (Observer version) (Closed)
|
Created:
5 years ago by Charlie Harrison Modified:
5 years ago CC:
asvitkine+watch_chromium.org, blundell+watchlist_chromium.org, chromium-reviews, csharrison+watch_chromium.org, droger+watchlist_chromium.org, loading-reviews+metrics_chromium.org, Randy Smith (Not in Mondays), sdefresne+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@new_observer Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[page_load_metrics] User Initiated Abort Tracking (Observer version)
Brief implementation design document here:
https://docs.google.com/a/chromium.org/document/d/18qyICfGXWyBzl40B2Ky_qSlSRJLufrnZoaLUz6bEUQA/edit?usp=sharing
BUG=517209
Committed: https://crrev.com/a50c920204e0053a0bdb356d0783a5191a3bb8f3
Cr-Commit-Position: refs/heads/master@{#365617}
Patch Set 1 #Patch Set 2 : fixes #Patch Set 3 : add design doc link #Patch Set 4 : Fix rebase #Patch Set 5 : #Patch Set 6 : #
Total comments: 4
Patch Set 7 : ScopedVector => std::vector #
Total comments: 13
Patch Set 8 : Bryan review + fix #Patch Set 9 : switch case #Patch Set 10 : Update for EventOccurredInForeground #
Total comments: 20
Patch Set 11 : Bryan review #
Total comments: 11
Patch Set 12 : bryan review #Patch Set 13 : Don't clear provisional loads on RenderProcessGone #
Total comments: 21
Patch Set 14 : kinuko review #
Total comments: 1
Patch Set 15 : Only eagerly log on renderer crash #
Total comments: 1
Patch Set 16 : Quick early return fix #Patch Set 17 : DCHECK(status != still running) #Patch Set 18 : rebase on #365097 #
Total comments: 14
Patch Set 19 : Alexei review #
Total comments: 4
Patch Set 20 : Alexei nits #
Total comments: 1
Patch Set 21 : const methods #Patch Set 22 : Remove dcheck for STILL_RUNNING #Messages
Total messages: 60 (14 generated)
Description was changed from ========== [page_load_metrics] User Initiated Abort Tracking (Observer version) BUG= ========== to ========== [page_load_metrics] User Initiated Abort Tracking (Observer version) Brief implementation design document here: https://docs.google.com/document/d/110xLqjh9pRBfUevRZYgJevyBoq36uulzkUvTYhDve... BUG=517209 ==========
csharrison@chromium.org changed reviewers: + bmcquade@chromium.org
This is based on the new observer API. This can sit on this until that CL settles.
Description was changed from ========== [page_load_metrics] User Initiated Abort Tracking (Observer version) Brief implementation design document here: https://docs.google.com/document/d/110xLqjh9pRBfUevRZYgJevyBoq36uulzkUvTYhDve... BUG=517209 ========== to ========== [page_load_metrics] User Initiated Abort Tracking (Observer version) Brief implementation design document here: https://docs.google.com/a/chromium.org/document/d/18qyICfGXWyBzl40B2Ky_qSlSRJ... BUG=517209 ==========
csharrison@chromium.org changed reviewers: + kinuko@chromium.org
Adding kinuko@ to give rdsmith@ a break.
Adding kinuko@ to give rdsmith@ a break.
looking good https://codereview.chromium.org/1476503004/diff/100001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1476503004/diff/100001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:336: ScopedVector<PageLoadTracker> aborted_provisional_loads_; nit: vector<scoped_ptr<>> should work now https://codereview.chromium.org/1476503004/diff/100001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_util.h (right): https://codereview.chromium.org/1476503004/diff/100001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_util.h:28: base::TimeDelta GetBackgroundDelta(const PageLoadExtraInfo& info); nit: I feel the method name and comment are a bit vague to be a public utility method; what 'background delta'? What 'simpler metric comparisons' means? If we always compare the return value to a given timing value, could this method be instead: bool IsAfterBackgrounded(const TimeDelta& timing) and could it do the comparison within the method?
https://codereview.chromium.org/1476503004/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1476503004/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:35: if (abort_type == UserAbortType::ABORT_RELOAD) { nit: should we do this in a switch/case? should be marginally more efficient. though you do have to include break statements in each case, which makes it slightly longer. in general i like to prefer switch/case when it's possible to use it. kinuko what are your thoughts here? https://codereview.chromium.org/1476503004/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:50: if (abort_type == UserAbortType::ABORT_RELOAD) { i know it'd be odd, but if extra_info.has_commit is false and the timing object is non-empty, do you want to log the 'BeforeCommit' histograms in that case? https://codereview.chromium.org/1476503004/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:50: if (abort_type == UserAbortType::ABORT_RELOAD) { same suggestion for switch/case https://codereview.chromium.org/1476503004/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.cc (right): https://codereview.chromium.org/1476503004/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.cc:37: if (EventOccurredInForeground(timing.load_event_start, extra_info)) { I agree that we should do this, but can you make sure adam is aware of this change? https://codereview.chromium.org/1476503004/diff/120001/components/page_load_m... File components/page_load_metrics/browser/BUILD.gn (right): https://codereview.chromium.org/1476503004/diff/120001/components/page_load_m... components/page_load_metrics/browser/BUILD.gn:24: "//ui/base:base", in google3 at least, when the stuff after the colon was the same as the stuff right before, you could omit the colon. not sure if that's also the case here, but can you check, and if so, let's shorten to just '//ui/base' https://codereview.chromium.org/1476503004/diff/120001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1476503004/diff/120001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:137: if (has_commit_) let's call all of the Record functions unconditionally for consistency, and keep the policy about whether to log anything if has_commit_ is false in each record function (so let's add 'if (!has_commit) return;' to RecordRappor(...) and remove the check for has_commit_ here) https://codereview.chromium.org/1476503004/diff/120001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_util.cc (right): https://codereview.chromium.org/1476503004/diff/120001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_util.cc:21: bool EventOccurredInForeground(const base::TimeDelta& event, nice - this seems like a great addition to these utils.
On 2015/12/09 20:35:24, Bryan McQuade wrote: > https://codereview.chromium.org/1476503004/diff/120001/chrome/browser/page_lo... > File > chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc > (right): > > https://codereview.chromium.org/1476503004/diff/120001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:35: > if (abort_type == UserAbortType::ABORT_RELOAD) { > nit: should we do this in a switch/case? should be marginally more efficient. > though you do have to include break statements in each case, which makes it > slightly longer. in general i like to prefer switch/case when it's possible to > use it. kinuko what are your thoughts here? > > https://codereview.chromium.org/1476503004/diff/120001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:50: > if (abort_type == UserAbortType::ABORT_RELOAD) { > i know it'd be odd, but if extra_info.has_commit is false and the timing object > is non-empty, do you want to log the 'BeforeCommit' histograms in that case? > > https://codereview.chromium.org/1476503004/diff/120001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:50: > if (abort_type == UserAbortType::ABORT_RELOAD) { > same suggestion for switch/case > > https://codereview.chromium.org/1476503004/diff/120001/chrome/browser/page_lo... > File > chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.cc > (right): > > https://codereview.chromium.org/1476503004/diff/120001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.cc:37: > if (EventOccurredInForeground(timing.load_event_start, extra_info)) { > I agree that we should do this, but can you make sure adam is aware of this > change? > > https://codereview.chromium.org/1476503004/diff/120001/components/page_load_m... > File components/page_load_metrics/browser/BUILD.gn (right): > > https://codereview.chromium.org/1476503004/diff/120001/components/page_load_m... > components/page_load_metrics/browser/BUILD.gn:24: "//ui/base:base", > in google3 at least, when the stuff after the colon was the same as the stuff > right before, you could omit the colon. not sure if that's also the case here, > but can you check, and if so, let's shorten to just '//ui/base' > > https://codereview.chromium.org/1476503004/diff/120001/components/page_load_m... > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > (right): > > https://codereview.chromium.org/1476503004/diff/120001/components/page_load_m... > components/page_load_metrics/browser/metrics_web_contents_observer.cc:137: if > (has_commit_) > let's call all of the Record functions unconditionally for consistency, and keep > the policy about whether to log anything if has_commit_ is false in each record > function (so let's add 'if (!has_commit) return;' to RecordRappor(...) and > remove the check for has_commit_ here) > > https://codereview.chromium.org/1476503004/diff/120001/components/page_load_m... > File components/page_load_metrics/browser/page_load_metrics_util.cc (right): > > https://codereview.chromium.org/1476503004/diff/120001/components/page_load_m... > components/page_load_metrics/browser/page_load_metrics_util.cc:21: bool > EventOccurredInForeground(const base::TimeDelta& event, > nice - this seems like a great addition to these utils. Sorry I tried to do something fancy locally but it accidentally got pushed up here. EventOccurredInForeground change will land soon with Adam's patch and I'll use that.
Adam's patch is ready to go, so I'll merge with that asap. https://codereview.chromium.org/1476503004/diff/100001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1476503004/diff/100001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:336: ScopedVector<PageLoadTracker> aborted_provisional_loads_; On 2015/12/09 04:57:51, kinuko wrote: > nit: vector<scoped_ptr<>> should work now Done. https://codereview.chromium.org/1476503004/diff/100001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_util.h (right): https://codereview.chromium.org/1476503004/diff/100001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_util.h:28: base::TimeDelta GetBackgroundDelta(const PageLoadExtraInfo& info); On 2015/12/09 04:57:51, kinuko wrote: > nit: I feel the method name and comment are a bit vague to be a public utility > method; what 'background delta'? What 'simpler metric comparisons' means? > > If we always compare the return value to a given timing value, could this method > be instead: > > bool IsAfterBackgrounded(const TimeDelta& timing) > > and could it do the comparison within the method? Good point. ricea is fixing this in his CL here https://codereview.chromium.org/1507813002/ I'll merge with his change once that gets settled. https://codereview.chromium.org/1476503004/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1476503004/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:35: if (abort_type == UserAbortType::ABORT_RELOAD) { On 2015/12/09 20:35:23, Bryan McQuade wrote: > nit: should we do this in a switch/case? should be marginally more efficient. > though you do have to include break statements in each case, which makes it > slightly longer. in general i like to prefer switch/case when it's possible to > use it. kinuko what are your thoughts here? I'll hold off on this until kinuko@ chimes in. I'm fine either way though. https://codereview.chromium.org/1476503004/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:50: if (abort_type == UserAbortType::ABORT_RELOAD) { On 2015/12/09 20:35:23, Bryan McQuade wrote: > i know it'd be odd, but if extra_info.has_commit is false and the timing object > is non-empty, do you want to log the 'BeforeCommit' histograms in that case? The timing object should always be empty for provisional loads. I can add a DCHECK. https://codereview.chromium.org/1476503004/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.cc (right): https://codereview.chromium.org/1476503004/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/stale_while_revalidate_metrics_observer.cc:37: if (EventOccurredInForeground(timing.load_event_start, extra_info)) { On 2015/12/09 20:35:23, Bryan McQuade wrote: > I agree that we should do this, but can you make sure adam is aware of this > change? Sorry this shouldn't be included in this change. https://codereview.chromium.org/1476503004/diff/120001/components/page_load_m... File components/page_load_metrics/browser/BUILD.gn (right): https://codereview.chromium.org/1476503004/diff/120001/components/page_load_m... components/page_load_metrics/browser/BUILD.gn:24: "//ui/base:base", On 2015/12/09 20:35:23, Bryan McQuade wrote: > in google3 at least, when the stuff after the colon was the same as the stuff > right before, you could omit the colon. not sure if that's also the case here, > but can you check, and if so, let's shorten to just '//ui/base' Done. https://codereview.chromium.org/1476503004/diff/120001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1476503004/diff/120001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:137: if (has_commit_) On 2015/12/09 20:35:23, Bryan McQuade wrote: > let's call all of the Record functions unconditionally for consistency, and keep > the policy about whether to log anything if has_commit_ is false in each record > function (so let's add 'if (!has_commit) return;' to RecordRappor(...) and > remove the check for has_commit_ here) Done.
(Let me do one more review after this one's rebased on top of adam's one) https://codereview.chromium.org/1476503004/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1476503004/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:35: if (abort_type == UserAbortType::ABORT_RELOAD) { On 2015/12/09 22:46:43, csharrison wrote: > On 2015/12/09 20:35:23, Bryan McQuade wrote: > > nit: should we do this in a switch/case? should be marginally more efficient. > > though you do have to include break statements in each case, which makes it > > slightly longer. in general i like to prefer switch/case when it's possible to > > use it. kinuko what are your thoughts here? > > I'll hold off on this until kinuko@ chimes in. I'm fine either way though. Either would work, but yeah I think I also prefer switch/case in this case (and it'd be more common in chromium code too)
Updated switch case and also migrated away from GetBackgroundDelta to EventOccurredInForeground. Randy would be happy :)
https://codereview.chromium.org/1476503004/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/page_load_metrics_observers_unittest.cc (right): https://codereview.chromium.org/1476503004/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/page_load_metrics_observers_unittest.cc:53: tracker->AddObserver( I'd like to have a separate aborts_page_load_metrics_observer_unittest.cc for this, perhaps with a common base test class that supports testing of observers. I don't want us to put each observer's tests in a single file. We can fix this in a follow up CL though. Can you add a TODO for it and/or file a bug? Let's try to fix it soon so subsequent observers can easily add their own unittest.cc files. https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:214: time_to_abort = abort_time_ - navigation_start_; can we DCHECK that abort_time_ is not zero when abort_type_ is not ABORT_NONE? https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:237: // signal). Note that this only occurs for provisional loads. While this 'this only occurs for provisional loads' -> 'abort type can only be ABORT_OTHER for provisional loads' (assuming i'm understanding correctly) https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:239: // methods. See https://goo.gl/WKRG98. navigating to https://goo.gl/WKRG98 requires me to be logged in with a chromium.org acct. can we make the document public? https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:241: (timestamp - abort_time_).InMilliseconds() < 100) { below we have std::min(abort_time_, timestamp); which suggests it's valid for timestamp to be before abort_time_, but here we're subtracting them which implies that timestamp should be after abort_time_. Which is correct? https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:248: if (!background_time_.is_null() && can we use info.first_background_time instead of background_time_ here and throughout this function? https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:578: plt->NotifyAbort(abort_type, timestamp); I'm finding following the logic in NotifyAbort a bit confusing to follow, and I think it's because it's trying to handle both the initial abort case as well as the updated abort case. IIUC, we only update abort reasons for PageLoadTrackers in aborted_provisional_loads_. So I'd propose that we have two methods, one to record the initial reason for non-aborted loads (NotifyAbort), and a separate to update the reason (UpdateAbortType). NotifyAbort might look like: DCHECK(abort_type != ABORT_NONE); DCHECK(abort_type_ == ABORT_NONE); DCHECK(abort_time_.is_zero()); abort_type_ = abort_type; abort_time_ = timestamp; And UpdateAbortType might look like: DCHECK(abort_type != ABORT_NONE); DCHECK(abort_type != ABORT_OTHER); DCHECK(abort_type_ == ABORT_OTHER); // is it ever valid to update an abort reason other than ABORT_OTHER? DCHECK(!abort_time_.is_zero()); abort_type_ = abort_type; abort_time_ = std::min(abort_time_, timestamp); // TODO(csharrison): add a short comment explaining why an updated abort reason could ever have a timestamp earlier than the previously recorded timestamp. The policy about whether to update the abort reason should IMO live at the call site. Something like: for (const auto& plt : aborted_provisional_loads_) { const base::TimeTicks& abort_time = plt->abort_time(); DCHECK_GE(timestamp, abort_time); // If we got a better signal than ABORT_OTHER in the last 100ms, treat it // as ... rest of comment here. if ((timestamp - abort_time_).InMilliseconds() < 100) { plt->UpdateAbortType(abort_type); } } https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:11: #include "base/memory/scoped_vector.h" now that we're using std::vector directly, can we remove this include? https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:79: "PageLoad.Timing2.Provisional.NavigationToFirstBackground"; I prefer the name 'BeforeCommit' rather than 'Provisional' here (and treating BeforeCommit as a suffix like we do elsewhere, so 'PageLoad.Timing2.NavigationToFirstBackground.BeforeCommit' since I think more people will understand what that means than 'Provisional' - you have to understand chrome's navigation flow pretty well to understand what a provisional load is, whereas I think we can explain once what a commit is and when it happens, and then people logically can understand what 'BeforeCommit' means. I know we have the PageLoad.Events UMAs which use Provisional and Committed - those are more internal UMA for our team so I'm less concerned about using familiar naming, but feel free to rename them to use 'BeforeCommit' and 'AfterCommit' if you want for consistency. https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_util.h (right): https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_util.h:32: bool EventOccurredInForeground(const base::TimeDelta& event, this naming is slightly misleading to the non-expert reader in that it sounds like it's about an instant in time occurring in foreground, when in reality it's determining whether the entirety of the duration from navigation start to that event happened in the foreground (e.g. if a page starts foregrounded, then goes background, then foregrounds again, and the event happens at that time, we'd return false from this function, even though technically the instantaneous event did happen in the foreground). since this is a utility that may be used by all clients, let's try to make the name as clear as possible. can we rename to more clearly convey that it's the duration from nav start to the event, not just the event itself, that happened in the foreground? PeriodFromNavigationToEventOccurredInForeground? A little long, but clear. Can we shorten it and still retain the clarity? InForegroundFromNavigationToEvent?
csharrison@chromium.org changed reviewers: + asvitkine@chromium.org
Adding Alexei now that this has settled a little. https://codereview.chromium.org/1476503004/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/page_load_metrics_observers_unittest.cc (right): https://codereview.chromium.org/1476503004/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/page_load_metrics_observers_unittest.cc:53: tracker->AddObserver( On 2015/12/10 17:04:23, Bryan McQuade wrote: > I'd like to have a separate aborts_page_load_metrics_observer_unittest.cc for > this, perhaps with a common base test class that supports testing of observers. > I don't want us to put each observer's tests in a single file. We can fix this > in a follow up CL though. Can you add a TODO for it and/or file a bug? Let's try > to fix it soon so subsequent observers can easily add their own unittest.cc > files. I have a bug filed for this. I'll do it asap after this change lands. https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:214: time_to_abort = abort_time_ - navigation_start_; On 2015/12/10 17:04:23, Bryan McQuade wrote: > can we DCHECK that abort_time_ is not zero when abort_type_ is not ABORT_NONE? Done. https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:237: // signal). Note that this only occurs for provisional loads. While this On 2015/12/10 17:04:23, Bryan McQuade wrote: > 'this only occurs for provisional loads' -> 'abort type can only be ABORT_OTHER > for provisional loads' (assuming i'm understanding correctly) Done. https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:239: // methods. See https://goo.gl/WKRG98. On 2015/12/10 17:04:23, Bryan McQuade wrote: > navigating to https://goo.gl/WKRG98 requires me to be logged in with a > http://chromium.org acct. can we make the document public? Done. https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:241: (timestamp - abort_time_).InMilliseconds() < 100) { On 2015/12/10 17:04:23, Bryan McQuade wrote: > below we have std::min(abort_time_, timestamp); which suggests it's valid for > timestamp to be before abort_time_, but here we're subtracting them which > implies that timestamp should be after abort_time_. Which is correct? It is valid for timestamp < abort_time. In this case we want to replace it, so it's okay for the subtraction to be negative and succeed. I'll clarify with a comment. https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:248: if (!background_time_.is_null() && On 2015/12/10 17:04:23, Bryan McQuade wrote: > can we use info.first_background_time instead of background_time_ here and > throughout this function? Done. https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:578: plt->NotifyAbort(abort_type, timestamp); On 2015/12/10 17:04:23, Bryan McQuade wrote: > I'm finding following the logic in NotifyAbort a bit confusing to follow, and I > think it's because it's trying to handle both the initial abort case as well as > the updated abort case. > > IIUC, we only update abort reasons for PageLoadTrackers in > aborted_provisional_loads_. > > So I'd propose that we have two methods, one to record the initial reason for > non-aborted loads (NotifyAbort), and a separate to update the reason > (UpdateAbortType). > > NotifyAbort might look like: > DCHECK(abort_type != ABORT_NONE); > DCHECK(abort_type_ == ABORT_NONE); > DCHECK(abort_time_.is_zero()); > abort_type_ = abort_type; > abort_time_ = timestamp; > > And UpdateAbortType might look like: > DCHECK(abort_type != ABORT_NONE); > DCHECK(abort_type != ABORT_OTHER); > DCHECK(abort_type_ == ABORT_OTHER); // is it ever valid to update an abort > reason other than ABORT_OTHER? > DCHECK(!abort_time_.is_zero()); > abort_type_ = abort_type; > abort_time_ = std::min(abort_time_, timestamp); // TODO(csharrison): add a > short comment explaining why an updated abort reason could ever have a timestamp > earlier than the previously recorded timestamp. > > The policy about whether to update the abort reason should IMO live at the call > site. Something like: > for (const auto& plt : aborted_provisional_loads_) { > const base::TimeTicks& abort_time = plt->abort_time(); > DCHECK_GE(timestamp, abort_time); > // If we got a better signal than ABORT_OTHER in the last 100ms, treat it > // as ... rest of comment here. > if ((timestamp - abort_time_).InMilliseconds() < 100) { > plt->UpdateAbortType(abort_type); > } > } This isn't quite true. We can notify aborts multiple times, so you can't DCHECK(abort_type_ == ABORT_NONE). You can early return, however. Other than that this suggestion sounds good. Done. https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:11: #include "base/memory/scoped_vector.h" On 2015/12/10 17:04:23, Bryan McQuade wrote: > now that we're using std::vector directly, can we remove this include? Done. Also removing observer_list. https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:79: "PageLoad.Timing2.Provisional.NavigationToFirstBackground"; On 2015/12/10 17:04:23, Bryan McQuade wrote: > I prefer the name 'BeforeCommit' rather than 'Provisional' here (and treating > BeforeCommit as a suffix like we do elsewhere, so > 'PageLoad.Timing2.NavigationToFirstBackground.BeforeCommit' since I think > more people will understand what that means than 'Provisional' - you have to > understand chrome's navigation flow pretty well to understand what a provisional > load is, whereas I think we can explain once what a commit is and when it > happens, and then > people logically can understand what 'BeforeCommit' means. > > I know we have the PageLoad.Events UMAs which use Provisional and Committed - > those are more internal UMA for our team so I'm less concerned about using > familiar naming, but feel free to rename them to use 'BeforeCommit' and > 'AfterCommit' if you want for consistency. Done. Switched it to using BeforeCommit / AfterCommit.BeforePaint.
rdsmith@chromium.org changed reviewers: + rdsmith@chromium.org
> Randy would be happy :) You seem to have mistaken me for someone who has the ability to be satisfied :-} :-}. More seriously, yes, this is goodness, and thank you. But Bryan raises a point that was tickling in the back of my mind too, so I wanted to at least mention that. https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_util.h (right): https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_util.h:32: bool EventOccurredInForeground(const base::TimeDelta& event, On 2015/12/10 17:04:23, Bryan McQuade wrote: > this naming is slightly misleading to the non-expert reader in that it sounds > like it's about an instant in time occurring in foreground, when in reality it's > determining whether the entirety of the duration from navigation start to that > event happened in the foreground (e.g. if a page starts foregrounded, then goes > background, then foregrounds again, and the event happens at that time, we'd > return false from this function, even though technically the instantaneous event > did happen in the foreground). since this is a utility that may be used by all > clients, let's try to make the name as clear as possible. can we rename to more > clearly convey that it's the duration from nav start to the event, not just the > event itself, that happened in the foreground? > PeriodFromNavigationToEventOccurredInForeground? A little long, but clear. Can > we shorten it and still retain the clarity? InForegroundFromNavigationToEvent? Agreed with Bryan's basic point; neutral on suggested name.
On 2015/12/10 18:13:02, rdsmith wrote: > > Randy would be happy :) > > You seem to have mistaken me for someone who has the ability to be satisfied :-} > :-}. > > More seriously, yes, this is goodness, and thank you. But Bryan raises a point > that was tickling in the back of my mind too, so I wanted to at least mention > that. > > https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... > File components/page_load_metrics/browser/page_load_metrics_util.h (right): > > https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... > components/page_load_metrics/browser/page_load_metrics_util.h:32: bool > EventOccurredInForeground(const base::TimeDelta& event, > On 2015/12/10 17:04:23, Bryan McQuade wrote: > > this naming is slightly misleading to the non-expert reader in that it sounds > > like it's about an instant in time occurring in foreground, when in reality > it's > > determining whether the entirety of the duration from navigation start to that > > event happened in the foreground (e.g. if a page starts foregrounded, then > goes > > background, then foregrounds again, and the event happens at that time, we'd > > return false from this function, even though technically the instantaneous > event > > did happen in the foreground). since this is a utility that may be used by all > > clients, let's try to make the name as clear as possible. can we rename to > more > > clearly convey that it's the duration from nav start to the event, not just > the > > event itself, that happened in the foreground? > > PeriodFromNavigationToEventOccurredInForeground? A little long, but clear. Can > > we shorten it and still retain the clarity? InForegroundFromNavigationToEvent? > > Agreed with Bryan's basic point; neutral on suggested name. Oops I completely missed that comment. What about "DurationOccurredInForeground(const TimeDelta& eventDuration)" ?
On 2015/12/10 19:03:57, csharrison wrote: > On 2015/12/10 18:13:02, rdsmith wrote: > > > Randy would be happy :) > > > > You seem to have mistaken me for someone who has the ability to be satisfied > :-} > > :-}. > > > > More seriously, yes, this is goodness, and thank you. But Bryan raises a > point > > that was tickling in the back of my mind too, so I wanted to at least mention > > that. > > > > > https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... > > File components/page_load_metrics/browser/page_load_metrics_util.h (right): > > > > > https://codereview.chromium.org/1476503004/diff/180001/components/page_load_m... > > components/page_load_metrics/browser/page_load_metrics_util.h:32: bool > > EventOccurredInForeground(const base::TimeDelta& event, > > On 2015/12/10 17:04:23, Bryan McQuade wrote: > > > this naming is slightly misleading to the non-expert reader in that it > sounds > > > like it's about an instant in time occurring in foreground, when in reality > > it's > > > determining whether the entirety of the duration from navigation start to > that > > > event happened in the foreground (e.g. if a page starts foregrounded, then > > goes > > > background, then foregrounds again, and the event happens at that time, we'd > > > return false from this function, even though technically the instantaneous > > event > > > did happen in the foreground). since this is a utility that may be used by > all > > > clients, let's try to make the name as clear as possible. can we rename to > > more > > > clearly convey that it's the duration from nav start to the event, not just > > the > > > event itself, that happened in the foreground? > > > PeriodFromNavigationToEventOccurredInForeground? A little long, but clear. > Can > > > we shorten it and still retain the clarity? > InForegroundFromNavigationToEvent? > > > > Agreed with Bryan's basic point; neutral on suggested name. > > Oops I completely missed that comment. What about > "DurationOccurredInForeground(const TimeDelta& eventDuration)" ? Sure, I think that works. The fact that the name includes 'duration' indicates to me that it's not talking about an instant in time, and the logical duration to infer is the time from nav start. Let's also clarify in the comment that the function determines whether the period from navigation start through the end of the duration occurs entirely in the foreground.
this looks really good. let's rename EventOccurredInForeground as you suggested and then i think we are good. thanks! https://codereview.chromium.org/1476503004/diff/200001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1476503004/diff/200001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:91: return ABORT_NEW_NAVIGATION; can we add a DCHECK(ui::PageTransitionIsNewNavigation(transition)); in the else case just to be sure there aren't any surprising cases here? or: ... else if (ui::PageTransitionIsNewNavigation(transition)) return ABORT_NEW_NAVIGATION; NOTREACHED(); return ABORT_OTHER; https://codereview.chromium.org/1476503004/diff/200001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:560: NotifyAbortAllLoads(ABORT_CLOSE); does this happen due to a nav to a different domain in the same tab, in which case maybe this should use ABORT_NEW_NAVIGATION? i guess we should characterize a crash as ABORT_CLOSE (or maybe ABORT_OTHER) and a cross-domain navigation as ABORT_NEW_NAVIGATION. not sure if we can differentiate those here though. can you think of a way for us to distinguish between the two cases? https://codereview.chromium.org/1476503004/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1476503004/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:31574: +<histogram name="PageLoad.Timing2.NavigationToFirstBackgroundBeforePaint" can move this down to your suffixes below. we should rename from 'PageLoadAbortConditions' to something more generic, since these aren't abort conditions, but happen to use the same suffixes. (are histogram suffix groupings significant? if so, we could create a separate suffixes entry with a different name, but my sense is that the grouping doesn't actually matter) https://codereview.chromium.org/1476503004/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:31650: +<histogram name="PageLoad.Timing2.Provisional.NavigationToFirstBackground" same
Let me know what you think. https://codereview.chromium.org/1476503004/diff/200001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1476503004/diff/200001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:91: return ABORT_NEW_NAVIGATION; On 2015/12/10 21:21:14, Bryan McQuade wrote: > can we add a DCHECK(ui::PageTransitionIsNewNavigation(transition)); in the else > case just to be sure there aren't any surprising cases here? > > or: > ... > else if (ui::PageTransitionIsNewNavigation(transition)) > return ABORT_NEW_NAVIGATION; > NOTREACHED(); > return ABORT_OTHER; Done. https://codereview.chromium.org/1476503004/diff/200001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:560: NotifyAbortAllLoads(ABORT_CLOSE); On 2015/12/10 21:21:14, Bryan McQuade wrote: > does this happen due to a nav to a different domain in the same tab, in which > case maybe this should use ABORT_NEW_NAVIGATION? > > i guess we should characterize a crash as ABORT_CLOSE (or maybe ABORT_OTHER) and > a cross-domain navigation as ABORT_NEW_NAVIGATION. not sure if we can > differentiate those here though. can you think of a way for us to distinguish > between the two cases? I think in practice this rarely occurs for cross-domain navigations. I actually have an alternate proposal: just ignore this event. 1. It is rarely user initiated 2. We aren't always sure what's happening (although inspecting the termination status gives us some indication). 3. Doing anything fancy could hamper other results. I.e. cross domain case / browser shutdown case (our code will handle this fine) I think clearing the loads is still smart, but we don't need to notify an abort. https://codereview.chromium.org/1476503004/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1476503004/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:31574: +<histogram name="PageLoad.Timing2.NavigationToFirstBackgroundBeforePaint" On 2015/12/10 21:21:14, Bryan McQuade wrote: > can move this down to your suffixes below. we should rename from > 'PageLoadAbortConditions' to something more generic, since these aren't abort > conditions, but happen to use the same suffixes. > > (are histogram suffix groupings significant? if so, we could create a separate > suffixes entry with a different name, but my sense is that the grouping doesn't > actually matter) Done. Oops https://codereview.chromium.org/1476503004/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:31650: +<histogram name="PageLoad.Timing2.Provisional.NavigationToFirstBackground" On 2015/12/10 21:21:14, Bryan McQuade wrote: > same Done.
Let me know what you think.
https://codereview.chromium.org/1476503004/diff/200001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1476503004/diff/200001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:560: NotifyAbortAllLoads(ABORT_CLOSE); On 2015/12/10 22:00:54, csharrison wrote: > On 2015/12/10 21:21:14, Bryan McQuade wrote: > > does this happen due to a nav to a different domain in the same tab, in which > > case maybe this should use ABORT_NEW_NAVIGATION? > > > > i guess we should characterize a crash as ABORT_CLOSE (or maybe ABORT_OTHER) > and > > a cross-domain navigation as ABORT_NEW_NAVIGATION. not sure if we can > > differentiate those here though. can you think of a way for us to distinguish > > between the two cases? > > I think in practice this rarely occurs for cross-domain navigations. I actually > have an alternate proposal: just ignore this event. > 1. It is rarely user initiated > 2. We aren't always sure what's happening (although inspecting the termination > status gives us some indication). > 3. Doing anything fancy could hamper other results. I.e. cross domain case / > browser shutdown case (our code will handle this fine) > > I think clearing the loads is still smart, but we don't need to notify an abort. +1 agree with proposal to clear the loads but not call NotifyAbortAllLoads here. Maybe add a short comment to explain why we don't call it here? https://codereview.chromium.org/1476503004/diff/200001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:562: provisional_loads_.clear(); actually, if the RenderProcess for the currently committed load goes away, should we be clearing out the provisional loads at that point? Seems like they could potentially still continue to load in a different render process? Not totally sure.
I wrote a small comment on my previous patch above the implementation of RenderProcessGone. https://codereview.chromium.org/1476503004/diff/200001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1476503004/diff/200001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:562: provisional_loads_.clear(); On 2015/12/10 23:02:58, Bryan McQuade wrote: > actually, if the RenderProcess for the currently committed load goes away, > should we be clearing out the provisional loads at that point? Seems like they > could potentially still continue to load in a different render process? Not > totally sure. Good call, you're right.
LGTM - just one question. Really nice change! https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:567: aborted_provisional_loads_.clear(); I'm also wondering if aborted_provisional_loads_ should be cleared here. For instance if the following happens: 1. WebContents starts a navigation to page http://a.com/ 2. the navigation commits 3. user clicks and navigates to http://a.com/subpage 4. user decides to start a new navigation to http://b.com/ before a.com/subpage commits 5. DidFinishNavigation gets called for the a.com/subpage navigation, which did not commit, and it gets added to aborted_provisional_loads_ 6. RenderProcessGone gets called for a.com (the first navigation). we clear a.com/subpage from aborted_provisional_loads_ and it gets recorded as ABORT_OTHER. 7. b.com commits, at which point we would have recorded the abort reason for a.com/subpage as ABORT_NEW_NAVIGATION, but because we cleared aborted_provisional_loads_ in RenderProcessGone, we don't get a chance to do this I'm not sure what the order of operations is and whether the above specific case is possible. There may also be other cases where it's desirable to clear aborted_provisional_loads_ here, but I'm not sure. LMK though if there are cases where it's important to clear them here.
https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:567: aborted_provisional_loads_.clear(); On 2015/12/11 01:33:39, Bryan McQuade wrote: > I'm also wondering if aborted_provisional_loads_ should be cleared here. For > instance if the following happens: > 1. WebContents starts a navigation to page http://a.com/ > 2. the navigation commits > 3. user clicks and navigates to http://a.com/subpage > 4. user decides to start a new navigation to http://b.com/ before a.com/subpage > commits > 5. DidFinishNavigation gets called for the a.com/subpage navigation, which did > not commit, and it gets added to aborted_provisional_loads_ > 6. RenderProcessGone gets called for a.com (the first navigation). we clear > a.com/subpage from aborted_provisional_loads_ and it gets recorded as > ABORT_OTHER. > 7. b.com commits, at which point we would have recorded the abort reason for > a.com/subpage as ABORT_NEW_NAVIGATION, but because we cleared > aborted_provisional_loads_ in RenderProcessGone, we don't get a chance to do > this > > I'm not sure what the order of operations is and whether the above specific case > is possible. There may also be other cases where it's desirable to clear > aborted_provisional_loads_ here, but I'm not sure. LMK though if there are cases > where it's important to clear them here. I think something like this is possible. However, in this case I think we should not clear committed_load_ either! If a.com commits but never paints and we move to a new renderer before the next navigation commits, then we're in the same place. Maybe we should just remove this call completely. The only problem would be IPCs from the renderer's new navigation updating the committed load. I think the solution would be to ABORT_OTHER the committed load and put it into aborted_provisional_loads_, which we'll rename aborted_loads_. WDYT? I'd also be fine with just clearing the committed_load_ for simplicity though.
lgtm % nits below I defer the detailed abort logic review to Bryan. https://codereview.chromium.org/1476503004/diff/240001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1476503004/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:57: default: If we do return instead of break in above cases and also do following we don't need to use default (so that we could get warning when the enum's updated without updating this switch/case): ... case ABORT_NONE: case ABORT_LAST: NOTREACHED(); } NOTREACHED(); Also we can make this if/else into if/early-return pattern https://codereview.chromium.org/1476503004/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:84: default: ditto, using default could be error-prone https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:231: DCHECK(abort_type != ABORT_NONE); nit: DCHECK_NE will give more informative debug output https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:244: DCHECK(abort_type_ == ABORT_OTHER); nit: DCHECK_NE and DCHECK_EQ https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:583: // cause of the abort (Some ABORT_OTHER signals occur before the true signal). nit: Some -> some https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:587: for (const auto& plt : aborted_provisional_loads_) { What stands for plt? https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_observer.h (right): https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.h:45: ABORT_LAST_ENTRY nit: looks like this is the naming policy of this component so it's fine, but LAST could be confusing as many modules use it to indicate the last valid enum value (e.g. ABORT_LAST = ABORT_OTHER). _MAX or _COUNT is more popular for the boundary (non-valid) value. https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_util.h (right): https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_util.h:32: bool DurationOccurredInForeground(const base::TimeDelta& event, I have a feeling that this change is not really relevant in this cl, could we do this in a separate patch?
https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_util.h (right): https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_util.h:32: bool DurationOccurredInForeground(const base::TimeDelta& event, On 2015/12/11 12:24:27, kinuko wrote: > I have a feeling that this change is not really relevant in this cl, could we do > this in a separate patch? Some more nit comments in case you want to land this patch as quickly as possible: 'Input' in the comment is a bit unclear, if it's about input event can we clearly refer to the param name |event|? Also calling a TimeDelta value 'Duration' gives me a bit of uneasy feeling, though I can't think of a better name either... Among the candidates I think I prefer InForegroundFromNavigationToEvent slightly better than others.
On 2015/12/11 01:43:55, csharrison wrote: > https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > (right): > > https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... > components/page_load_metrics/browser/metrics_web_contents_observer.cc:567: > aborted_provisional_loads_.clear(); > On 2015/12/11 01:33:39, Bryan McQuade wrote: > > I'm also wondering if aborted_provisional_loads_ should be cleared here. For > > instance if the following happens: > > 1. WebContents starts a navigation to page http://a.com/ > > 2. the navigation commits > > 3. user clicks and navigates to http://a.com/subpage > > 4. user decides to start a new navigation to http://b.com/ before > a.com/subpage > > commits > > 5. DidFinishNavigation gets called for the a.com/subpage navigation, which did > > not commit, and it gets added to aborted_provisional_loads_ > > 6. RenderProcessGone gets called for a.com (the first navigation). we clear > > a.com/subpage from aborted_provisional_loads_ and it gets recorded as > > ABORT_OTHER. > > 7. b.com commits, at which point we would have recorded the abort reason for > > a.com/subpage as ABORT_NEW_NAVIGATION, but because we cleared > > aborted_provisional_loads_ in RenderProcessGone, we don't get a chance to do > > this > > > > I'm not sure what the order of operations is and whether the above specific > case > > is possible. There may also be other cases where it's desirable to clear > > aborted_provisional_loads_ here, but I'm not sure. LMK though if there are > cases > > where it's important to clear them here. > > I think something like this is possible. However, in this case I think we should > not clear committed_load_ either! If a.com commits but never paints and we move > to a new renderer before the next navigation commits, then we're in the same > place. Maybe we should just remove this call completely. > > The only problem would be IPCs from the renderer's new navigation updating the > committed load. I think the solution would be to ABORT_OTHER the committed load > and put it into aborted_provisional_loads_, which we'll rename aborted_loads_. > WDYT? I'd also be fine with just clearing the committed_load_ for simplicity > though. Interesting - why not clear the committed load? The page was done loading as soon as the render process went away so it seems useful to clear the committed load, which will log page load metrics for the committed load at the earliest point possible. Can discuss in person to work through the details.
Bryan: I'm not sure this is possible, but if a cross-site navigation crashed the renderer before the committed load painted, then we wouldn't want to lose information that a navigation triggered it and it is a valid abort. Clearing the committed load could possibly mask that abort. I don't know if it is worth the trouble though, as this seems like an exceedingly rare circumstance. https://codereview.chromium.org/1476503004/diff/240001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1476503004/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:57: default: On 2015/12/11 12:24:27, kinuko wrote: > If we do return instead of break in above cases and also do following we don't > need to use default (so that we could get warning when the enum's updated > without updating this switch/case): > > ... > case ABORT_NONE: > case ABORT_LAST: > NOTREACHED(); > } > NOTREACHED(); > > Also we can make this if/else into if/early-return pattern Done. https://codereview.chromium.org/1476503004/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:84: default: On 2015/12/11 12:24:27, kinuko wrote: > ditto, using default could be error-prone Done. https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:231: DCHECK(abort_type != ABORT_NONE); On 2015/12/11 12:24:27, kinuko wrote: > nit: DCHECK_NE will give more informative debug output Done. https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:244: DCHECK(abort_type_ == ABORT_OTHER); On 2015/12/11 12:24:27, kinuko wrote: > nit: DCHECK_NE and DCHECK_EQ Done. https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:583: // cause of the abort (Some ABORT_OTHER signals occur before the true signal). On 2015/12/11 12:24:27, kinuko wrote: > nit: Some -> some Done. https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:587: for (const auto& plt : aborted_provisional_loads_) { On 2015/12/11 12:24:27, kinuko wrote: > What stands for plt? PageLoadTracker. I'll modify it to be "tracker" to be more clear. https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_observer.h (right): https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_observer.h:45: ABORT_LAST_ENTRY On 2015/12/11 12:24:27, kinuko wrote: > nit: looks like this is the naming policy of this component so it's fine, but > LAST could be confusing as many modules use it to indicate the last valid enum > value (e.g. ABORT_LAST = ABORT_OTHER). _MAX or _COUNT is more popular for the > boundary (non-valid) value. Noted. I think LAST_ENTRY is common in the net stack, so I was encouraged to use it. I probably won't change it in this CL though but I'll keep it in mind. https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_util.h (right): https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_util.h:32: bool DurationOccurredInForeground(const base::TimeDelta& event, On 2015/12/11 13:16:45, kinuko wrote: > On 2015/12/11 12:24:27, kinuko wrote: > > I have a feeling that this change is not really relevant in this cl, could we > do > > this in a separate patch? > > Some more nit comments in case you want to land this patch as quickly as > possible: 'Input' in the comment is a bit unclear, if it's about input event can > we clearly refer to the param name |event|? Also calling a TimeDelta value > 'Duration' gives me a bit of uneasy feeling, though I can't think of a better > name either... Among the candidates I think I prefer > InForegroundFromNavigationToEvent slightly better than others. I'm fine doing this in another patch. We can figure out the best name there.
https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_util.h (right): https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_util.h:32: bool DurationOccurredInForeground(const base::TimeDelta& event, On 2015/12/11 12:24:27, kinuko wrote: > I have a feeling that this change is not really relevant in this cl, could we do > this in a separate patch? Yeah - doing this in a separate change sounds fine to me. Sorry Charles for asking to tack this on to the current change. To simplify merging to 48, let's land the rename in a patch after this one (that way we don't have to merge the rename). Charles, is that ok with you?
https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_util.h (right): https://codereview.chromium.org/1476503004/diff/240001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_util.h:32: bool DurationOccurredInForeground(const base::TimeDelta& event, On 2015/12/11 14:13:42, Bryan McQuade wrote: > On 2015/12/11 12:24:27, kinuko wrote: > > I have a feeling that this change is not really relevant in this cl, could we > do > > this in a separate patch? > > Yeah - doing this in a separate change sounds fine to me. Sorry Charles for > asking to tack this on to the current change. To simplify merging to 48, let's > land the rename in a patch after this one (that way we don't have to merge the > rename). Charles, is that ok with you? Yep, my most recent patch set does not include this.
https://codereview.chromium.org/1476503004/diff/260001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1476503004/diff/260001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:562: void MetricsWebContentsObserver::RenderProcessGone( just following on our discussion here, it seems like we are only hooking this function to eagerly clean up in cases where the render process crashed. in cases where the render process did not crash, it seems like we want to rely on the logic in DidFinishNavigation (or the destructor) to clean up the committed_load_. given this, perhaps we should do something like: if (status == base::TERMINATION_STATUS_NORMAL_TERMINATION) return; at the entry to this function, and then we focus this function on doing cleanup in the specific case where the render process crashed and thus the current navigation is terminated without a next navigation necessarily having committed. i think this is mostly unrelated to this change, except i'm truly unsure if we want to be clearing aborted_provisional_loads_ in all cases in this function so it becomes more of an issue now. probably best for us to discuss in person. will come by shortly.
Bryan: I made the change. Now we only eagerly log in the case of a crashed renderer.
LGTM https://codereview.chromium.org/1476503004/diff/280001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1476503004/diff/280001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:566: status == base::TERMINATION_STATUS_STILL_RUNNING) { I wonder if it's ever valid to get TERMINATION_STATUS_STILL_RUNNING in RenderProcessGone. If we don't think it's valid, maybe it's better to DCHECK_NE(status, TERMINATION_STATUS_STILL_RUNNING); at entry to this function. I'm honestly not sure how this function should interpret that status code, since it implies the render process is not gone, which is contradictory to the method name... That said I'm fine either way.
On 2015/12/11 17:00:22, Bryan McQuade wrote: > LGTM > > https://codereview.chromium.org/1476503004/diff/280001/components/page_load_m... > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > (right): > > https://codereview.chromium.org/1476503004/diff/280001/components/page_load_m... > components/page_load_metrics/browser/metrics_web_contents_observer.cc:566: > status == base::TERMINATION_STATUS_STILL_RUNNING) { > I wonder if it's ever valid to get TERMINATION_STATUS_STILL_RUNNING in > RenderProcessGone. If we don't think it's valid, maybe it's better to > DCHECK_NE(status, TERMINATION_STATUS_STILL_RUNNING); at entry to this function. > I'm honestly not sure how this function should interpret that status code, since > it implies the render process is not gone, which is contradictory to the method > name... > > That said I'm fine either way. This change LGTM, thanks for the fixes! Alexei, this is ready for your review when you have cycles.
https://codereview.chromium.org/1476503004/diff/340001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.h (right): https://codereview.chromium.org/1476503004/diff/340001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.h:11: "PageLoad.AbortTiming.ForwardBackNavigation.BeforeCommit"; Don't define constants in header files. Instead, if you want to expose them, put their definition in the .cc file and declare them in the header. See for example chrome_switches.cc/h. Also, I'm not a big fun of dumping these in the global namespace. Can you put them inside a namespace or a class? If you're exposing these only for testing, I suggest putting them in an "internal" namespace. https://codereview.chromium.org/1476503004/diff/340001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.h:38: // page_load_metrics::PageLoadMetricsObserver implementation: Nit: I think for new code, the comment convention is: // page_load_metrics::PageLoadMetricsObserver: https://codereview.chromium.org/1476503004/diff/340001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.h:42: }; Nit: Add private: and DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/1476503004/diff/340001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1476503004/diff/340001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:88: else if (transition & ui::PAGE_TRANSITION_FORWARD_BACK) Nit: No need for else if the above if already returns. https://codereview.chromium.org/1476503004/diff/340001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1476503004/diff/340001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:238: void UpdateAbort(UserAbortType abort_type, const base::TimeTicks& timestamp); Nit: I think base::TimeTicks is meant to be passed by value. Check the header comments. https://codereview.chromium.org/1476503004/diff/340001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:334: // Tracks aborted provisional loads for a little bit longer than usual (one Nit: Add empty lines around the new field you're adding.
Thanks for the review! https://codereview.chromium.org/1476503004/diff/340001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.h (right): https://codereview.chromium.org/1476503004/diff/340001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.h:11: "PageLoad.AbortTiming.ForwardBackNavigation.BeforeCommit"; On 2015/12/14 at 22:14:40, Alexei Svitkine (slow) wrote: > Don't define constants in header files. > > Instead, if you want to expose them, put their definition in the .cc file and declare them in the header. See for example chrome_switches.cc/h. > > Also, I'm not a big fun of dumping these in the global namespace. Can you put them inside a namespace or a class? If you're exposing these only for testing, I suggest putting them in an "internal" namespace. I used the extern pattern for this case, and added an internal namespace. Can you point me to a resource that explains this rule? I couldn't find it in the style guide. https://codereview.chromium.org/1476503004/diff/340001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.h:38: // page_load_metrics::PageLoadMetricsObserver implementation: On 2015/12/14 at 22:14:40, Alexei Svitkine (slow) wrote: > Nit: I think for new code, the comment convention is: > > // page_load_metrics::PageLoadMetricsObserver: Done https://codereview.chromium.org/1476503004/diff/340001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.h:42: }; On 2015/12/14 at 22:14:40, Alexei Svitkine (slow) wrote: > Nit: Add private: and DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/1476503004/diff/340001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1476503004/diff/340001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:88: else if (transition & ui::PAGE_TRANSITION_FORWARD_BACK) On 2015/12/14 at 22:14:40, Alexei Svitkine (slow) wrote: > Nit: No need for else if the above if already returns. Done. https://codereview.chromium.org/1476503004/diff/340001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1476503004/diff/340001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:238: void UpdateAbort(UserAbortType abort_type, const base::TimeTicks& timestamp); On 2015/12/14 at 22:14:40, Alexei Svitkine (slow) wrote: > Nit: I think base::TimeTicks is meant to be passed by value. Check the header comments. Fixed all occurrences. https://codereview.chromium.org/1476503004/diff/340001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:334: // Tracks aborted provisional loads for a little bit longer than usual (one On 2015/12/14 at 22:14:40, Alexei Svitkine (slow) wrote: > Nit: Add empty lines around the new field you're adding. Done.
lgtm https://codereview.chromium.org/1476503004/diff/340001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.h (right): https://codereview.chromium.org/1476503004/diff/340001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.h:11: "PageLoad.AbortTiming.ForwardBackNavigation.BeforeCommit"; On 2015/12/14 23:16:30, csharrison wrote: > On 2015/12/14 at 22:14:40, Alexei Svitkine (slow) wrote: > > Don't define constants in header files. > > > > Instead, if you want to expose them, put their definition in the .cc file and > declare them in the header. See for example chrome_switches.cc/h. > > > > Also, I'm not a big fun of dumping these in the global namespace. Can you put > them inside a namespace or a class? If you're exposing these only for testing, I > suggest putting them in an "internal" namespace. > > I used the extern pattern for this case, and added an internal namespace. Can > you point me to a resource that explains this rule? I couldn't find it in the > style guide. I think putting them in the header can cause linker errors as multiple .cc files will end up defining the same symbols when more than one file includes the header. See: http://stackoverflow.com/questions/2328671/constant-variables-not-working-in-... So it might not be covered in the style guide since it's not a matter of style, but a matter of functionality. https://codereview.chromium.org/1476503004/diff/360001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1476503004/diff/360001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:229: UserAbortType abort_type() { return abort_type_; } Nit: const https://codereview.chromium.org/1476503004/diff/360001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:230: const base::TimeTicks& abort_time() { return abort_time_; } Nit: const
I'm a bit confused what you meant with your nits. Do you want me to return a const UserAbortType / TimeTicks by value? That's what I did. I'm not sure I see the benefit of returning a const enum, though. https://codereview.chromium.org/1476503004/diff/340001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.h (right): https://codereview.chromium.org/1476503004/diff/340001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.h:11: "PageLoad.AbortTiming.ForwardBackNavigation.BeforeCommit"; On 2015/12/15 at 16:42:33, Alexei Svitkine (slow) wrote: > On 2015/12/14 23:16:30, csharrison wrote: > > On 2015/12/14 at 22:14:40, Alexei Svitkine (slow) wrote: > > > Don't define constants in header files. > > > > > > Instead, if you want to expose them, put their definition in the .cc file and > > declare them in the header. See for example chrome_switches.cc/h. > > > > > > Also, I'm not a big fun of dumping these in the global namespace. Can you put > > them inside a namespace or a class? If you're exposing these only for testing, I > > suggest putting them in an "internal" namespace. > > > > I used the extern pattern for this case, and added an internal namespace. Can > > you point me to a resource that explains this rule? I couldn't find it in the > > style guide. > > I think putting them in the header can cause linker errors as multiple .cc files will end up defining the same symbols when more than one file includes the header. See: http://stackoverflow.com/questions/2328671/constant-variables-not-working-in-... > > So it might not be covered in the style guide since it's not a matter of style, but a matter of functionality. Ah okay that makes sense. I have to file a bug for other places in the code where I've done this. I think this hasn't broken anything yet because we aren't including from multiple translation units yet. Thanks for the help. https://codereview.chromium.org/1476503004/diff/360001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1476503004/diff/360001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:229: UserAbortType abort_type() { return abort_type_; } On 2015/12/15 at 16:42:33, Alexei Svitkine (slow) wrote: > Nit: const Done. https://codereview.chromium.org/1476503004/diff/360001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:230: const base::TimeTicks& abort_time() { return abort_time_; } On 2015/12/15 at 16:42:33, Alexei Svitkine (slow) wrote: > Nit: const Done.
https://codereview.chromium.org/1476503004/diff/380001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1476503004/diff/380001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:229: const UserAbortType abort_type() { return abort_type_; } I meant: UserAbortType abort_type() const { return abort_type_; } To indicate that the call isn't modifying the object.
Updated. Thanks for the clarification. Sorry about that!
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, bmcquade@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1476503004/#ps370014 (title: "const methods")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1476503004/370014 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1476503004/370014
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
csharrison@chromium.org changed reviewers: + sadrul@chromium.org
+sadrul for ui/base/page_transition_types.h added to DEPS
On 2015/12/15 21:09:17, csharrison wrote: > +sadrul for ui/base/page_transition_types.h added to DEPS lgtm
On 2015/12/16 at 18:55:52, sadrul wrote: > On 2015/12/15 21:09:17, csharrison wrote: > > +sadrul for ui/base/page_transition_types.h added to DEPS > > lgtm Thanks. Looks like we can still have TERMINATION_STATUS_STILL_RUNNING when the render process dies. Will push a fix.
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, bmcquade@chromium.org, asvitkine@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1476503004/#ps410001 (title: "Remove dcheck for STILL_RUNNING")
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1476503004/410001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1476503004/410001
Message was sent while issue was closed.
Description was changed from ========== [page_load_metrics] User Initiated Abort Tracking (Observer version) Brief implementation design document here: https://docs.google.com/a/chromium.org/document/d/18qyICfGXWyBzl40B2Ky_qSlSRJ... BUG=517209 ========== to ========== [page_load_metrics] User Initiated Abort Tracking (Observer version) Brief implementation design document here: https://docs.google.com/a/chromium.org/document/d/18qyICfGXWyBzl40B2Ky_qSlSRJ... BUG=517209 ==========
Message was sent while issue was closed.
Committed patchset #22 (id:410001)
Message was sent while issue was closed.
Description was changed from ========== [page_load_metrics] User Initiated Abort Tracking (Observer version) Brief implementation design document here: https://docs.google.com/a/chromium.org/document/d/18qyICfGXWyBzl40B2Ky_qSlSRJ... BUG=517209 ========== to ========== [page_load_metrics] User Initiated Abort Tracking (Observer version) Brief implementation design document here: https://docs.google.com/a/chromium.org/document/d/18qyICfGXWyBzl40B2Ky_qSlSRJ... BUG=517209 Committed: https://crrev.com/a50c920204e0053a0bdb356d0783a5191a3bb8f3 Cr-Commit-Position: refs/heads/master@{#365617} ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/a50c920204e0053a0bdb356d0783a5191a3bb8f3 Cr-Commit-Position: refs/heads/master@{#365617} |
