Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1384213002:
Page Abort Events for relevant navigations (Closed)
|
Created:
5 years, 2 months ago by Charlie Harrison Modified:
5 years, 2 months ago CC:
chromium-reviews, asanka, benjhayden+dwatch_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPage Abort Events for relevant navigations
BUG=544152
Committed: https://crrev.com/7a32ec9ce6c2522c65b3e9be5ecad54b5285bb0a
Cr-Commit-Position: refs/heads/master@{#354778}
Patch Set 1 #Patch Set 2 : Fixed tests + added a test #
Total comments: 1
Patch Set 3 : More events + bg/fg metrics #
Total comments: 7
Patch Set 4 : new enum per Randy, updated histograms #
Total comments: 7
Patch Set 5 : Mirror histogram for backgrounded events. New test. #
Total comments: 30
Patch Set 6 : Bryan+Randy review changes #Patch Set 7 : More errors, minimum histogram bucket_num is 3 #
Total comments: 5
Patch Set 8 : Added new histogram suffix, comments, name changes #
Total comments: 20
Patch Set 9 : Bryan review. Postponing BG/Background decision for now #
Total comments: 2
Patch Set 10 : Comments/name changes. Don't track non html mimes #
Total comments: 9
Patch Set 11 : IsRelevantNavigation moved, Alexei review #Patch Set 12 : one last little error condition #
Total comments: 14
Patch Set 13 : Alexei review: comments, histogram changes, unit test constructor #
Messages
Total messages: 47 (5 generated)
csharrison@chromium.org changed reviewers: + bmcquade@chromium.org, ttuttle@chromium.org
Looks like this was a mistake in PageLoadEvents. We need a separate counter for relevant page loads aborted. In the meantime, I fixed the previous count to not count loads we explicitly stop tracking.
csharrison@chromium.org changed reviewers: + rdsmith@chromium.org - ttuttle@chromium.org
+ randy
https://codereview.chromium.org/1384213002/diff/20001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1384213002/diff/20001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:55: PAGE_LOAD_ABORTED_BEFORE_FIRST_LAYOUT_RELEVANT, can you add a comment for this one? what does relevant indicate here?
On 2015/10/05 17:46:00, Bryan McQuade wrote: > https://codereview.chromium.org/1384213002/diff/20001/components/page_load_me... > File components/page_load_metrics/browser/metrics_web_contents_observer.h > (right): > > https://codereview.chromium.org/1384213002/diff/20001/components/page_load_me... > components/page_load_metrics/browser/metrics_web_contents_observer.h:55: > PAGE_LOAD_ABORTED_BEFORE_FIRST_LAYOUT_RELEVANT, > can you add a comment for this one? what does relevant indicate here? Let me know if the comments I added suffice. I added a couple more metrics to track too.
First set of comments. Part of these is my being confused, so I'm going to get that confusion resolved before I put gobs more time into the review :-}. https://codereview.chromium.org/1384213002/diff/40001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1384213002/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:85: // Don't log foreground time if we started foregrounded. I'm confused by this sentence. If we started foregrounded and never go background, foreground_time_ will be null and (presumably, haven't read the paired code yet) won't be logged. But if we started foreground, go background, and then go foreground again, foregrount_time_ will not be null. So shouldn't we be testing started_in_foreground_? (This may just be me bering confused by a comment that's not in the right place, i.e. not near the code it refers to?) https://codereview.chromium.org/1384213002/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:102: bool valid_timing_descendent = timing_.navigation_start.is_null() || nit, suggestion: I dislike creating temporary variables for single use. To the extent that documentation is needed, I'd rather have a comment. But up to you. Part of this is because I don't understand what "valid_timing_descendent" means; whether you accept the above suggestion or not, could you add a comment explaining that? https://codereview.chromium.org/1384213002/diff/40001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1384213002/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:67: PAGE_LOAD_BAD_IPC, I'd like the comments for these instances to make clear what overlap they have with the other elements in the list; tracking what's disjoint, what's subset, and what's just overlapping is getting hard. Suggestion (up to you): Make separate histograms (and separate enums) where each histogram has the property that each load shows up in only one bucket. It might make the bucket descriptions a bit confusing, but I think it would make analysis and comparison much easier. https://codereview.chromium.org/1384213002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1384213002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:29948: + user first backgrounds the tab. I presume no entry is made in this histogram if the tab stays in the foreground? Also, what happens if we start in background, foreground, then background? https://codereview.chromium.org/1384213002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:29958: + user first foregrounds an initially backgrounded tab. Mirror set of questions (stays in background, start in foreground, background, then foreground).
https://codereview.chromium.org/1384213002/diff/40001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1384213002/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:85: // Don't log foreground time if we started foregrounded. On 2015/10/06 19:24:00, rdsmith wrote: > I'm confused by this sentence. If we started foregrounded and never go > background, foreground_time_ will be null and (presumably, haven't read the > paired code yet) won't be logged. But if we started foreground, go background, > and then go foreground again, foregrount_time_ will not be null. So shouldn't > we be testing started_in_foreground_? > > (This may just be me bering confused by a comment that's not in the right place, > i.e. not near the code it refers to?) You're right this code could be more clear. Later we test !foreground_time_.is_null() && !started_in_foreground_, but I'll move some of that logic here. https://codereview.chromium.org/1384213002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1384213002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:29948: + user first backgrounds the tab. On 2015/10/06 19:24:00, rdsmith wrote: > I presume no entry is made in this histogram if the tab stays in the foreground? > Also, what happens if we start in background, foreground, then background? No entry if tab stays in foreground. Start in background => foreground => background should not log anything either, I'll update these to say something like "time to first background from initially foregrounded tab."
Randy was right about the enums. I updated them so they are much clearer and as disjoint as possible. I ended up marking the PageLoadEvent enum obsolete because not only was it not split up, we had corrupted data.
overall this looks good - just a couple high level questions from me. i'd be interested in discussing these a bit more in person on friday to hear what you think. https://codereview.chromium.org/1384213002/diff/60001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1384213002/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:35: enum ProvisionalLoadEvent { nice - i like segmenting into separate enums. https://codereview.chromium.org/1384213002/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:37: PROVISIONAL_LOAD_ABORTED, is it interesting/useful to segment these by foreground vs background? have not given it a lot of thought - just want to make sure we make an informed decision one way or the other. https://codereview.chromium.org/1384213002/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:60: COMMITTED_LOAD_ABORTED_BEFORE_FIRST_LAYOUT, WDYT about having 2 histograms for this enum, one for foreground and one for background, like we do for the timing histograms? I haven't thought about it in detail but on first glance given that we have a few cases where there are both FG and BG enum entries I wonder if we should just segment at the histogram level. When doing analysis we can always merge the counts from both histograms if needed. https://codereview.chromium.org/1384213002/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:99: base::TimeTicks background_time_; propose calling these first_background_time_ and first_foreground_time_ - WDYT?
https://codereview.chromium.org/1384213002/diff/60001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1384213002/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:37: PROVISIONAL_LOAD_ABORTED, On 2015/10/08 01:50:26, Bryan McQuade wrote: > is it interesting/useful to segment these by foreground vs background? have not > given it a lot of thought - just want to make sure we make an informed decision > one way or the other. I think it does make sense. I'll make the change. https://codereview.chromium.org/1384213002/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:60: COMMITTED_LOAD_ABORTED_BEFORE_FIRST_LAYOUT, On 2015/10/08 01:50:26, Bryan McQuade wrote: > WDYT about having 2 histograms for this enum, one for foreground and one for > background, like we do for the timing histograms? I haven't thought about it in > detail but on first glance given that we have a few cases where there are both > FG and BG enum entries I wonder if we should just segment at the histogram > level. When doing analysis we can always merge the counts from both histograms > if needed. I like that idea! We can still keep two enums, but record 4 histograms. I'll see if I can make the code / tests concise with this change. https://codereview.chromium.org/1384213002/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:99: base::TimeTicks background_time_; On 2015/10/08 01:50:26, Bryan McQuade wrote: > propose calling these first_background_time_ and first_foreground_time_ - WDYT? Good idea.
Thanks for the change! This is IMO much better, but I still have a couple of questions, probably the most important one of which is around the difference between COMMITED_LOAD_STARTED and the other buckets in that histogram. https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:90: // but at the start of the navigation. Scanning the rest of the file for the usage of the second argument of RecordCommittedEvent, this comment seems off to me--the rest of the file look like it's checking not if the page is in the background at the time of the event, but if it's been backgrounded at any point since it started. I'm also confused by why we want to do things differently for this statistic. My primary concern is that it makes this number incomparable with the other numbers in the histogram. But I'm also not certain why we want to know whether or not the page was started in the foreground; what are we going to do with that information? https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:97: // previous one (if the previous one had a navigation start at all). This mostly just duplicates what the code says. That can be useful, but more useful is saying *why* a timing struct is valid iff it has the same navigation start as a previous one is more useful. Maybe "Two timing structures cannot refer to the same navigation if they indicate that a navigation started at different times, so a new timing struct with an different start time from an earlier struct is considered invalid."? https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:105: RecordCommittedEvent(COMMITTED_LOAD_BAD_IPC, HasBackgrounded()); I'm a bit uncomfortable within recording this event in a routine that doesn't, on the surface, have anything to do with IPC. Maybe pull this into the caller and conditionalize it on the routine return value? (Though I see the caller isn't obviously IPC related either, but it's closer.) https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:117: // this solution is the best we have right now. I continue to be confused by background_delta when I run into it. It isn't being added in this CL, but can I ask for a comment clarification anyway? I don't feel like the current comment is very illuminating. Maybe "The time between navigation and first backgrounding. If never backgrounded, this is TimeDelta::Max(), if started in the background, it is null. There may be slight innacuracies due to interprocess timestamps ..."? Also, a suggestion: I find the value/max/null sorta confusing and inconsistent. Maybe just make it 0 if the page started in the background? I think that's valid and consistent with the other values (0 == there was no time between navigation and first backgrounding). https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:182: UMA_HISTOGRAM_ENUMERATION("PageLoad.Events.Provisional.BG", event, I'd like to get rid of "BG" and use "Background" and "Foreground". The custom in Chromium is to avoid abbreviations (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin...). https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:49: // inconsistencies between versions. Should this comment be before the above enum and referring to both enums (or duplicated)? https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:64: COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT, I'm slightly confused about these enums. Is it true that #(COMMITTED_LOAD_STARTED) == #(COMMITTED_LOAD_ABORTED_BEFORE_FIRST_LAYOUT) + #(COMMITTED_LOAD_SUCCESSUL_FIRST_LAYOUT) + #(loads that failed but weren't aborted before first layout)? If so, I'm not sure what "sum them to find the total number of committed loads that we end up tracking" means? Is there some other stat where that sum will be useful? If so, is there any way we can make the useful number #(COMMITTED_LOAD_STARTED) so it's all in one place? Sorry if my suggestions for clarity are more confusing--I don't quite yet understand the relationship between these enums.
https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:90: // but at the start of the navigation. On 2015/10/08 19:07:11, rdsmith wrote: > Scanning the rest of the file for the usage of the second argument of > RecordCommittedEvent, this comment seems off to me--the rest of the file look > like it's checking not if the page is in the background at the time of the > event, but if it's been backgrounded at any point since it started. > > I'm also confused by why we want to do things differently for this statistic. > My primary concern is that it makes this number incomparable with the other > numbers in the histogram. But I'm also not certain why we want to know whether > or not the page was started in the foreground; what are we going to do with that > information? I think it adds a little more context to the backgrounded navigations. A navigation started in the background signals a different user intent than one started in the foreground, then backgrounded. IMO not special casing this makes the metrics not that useful, because commit-time does not signal anything special to the user. https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:97: // previous one (if the previous one had a navigation start at all). On 2015/10/08 19:07:11, rdsmith wrote: > This mostly just duplicates what the code says. That can be useful, but more > useful is saying *why* a timing struct is valid iff it has the same navigation > start as a previous one is more useful. Maybe "Two timing structures cannot > refer to the same navigation if they indicate that a navigation started at > different times, so a new timing struct with an different start time from an > earlier struct is considered invalid."? Done. https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:105: RecordCommittedEvent(COMMITTED_LOAD_BAD_IPC, HasBackgrounded()); On 2015/10/08 19:07:10, rdsmith wrote: > I'm a bit uncomfortable within recording this event in a routine that doesn't, > on the surface, have anything to do with IPC. Maybe pull this into the caller > and conditionalize it on the routine return value? (Though I see the caller > isn't obviously IPC related either, but it's closer.) Done. https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:117: // this solution is the best we have right now. On 2015/10/08 19:07:10, rdsmith wrote: > I continue to be confused by background_delta when I run into it. It isn't > being added in this CL, but can I ask for a comment clarification anyway? I > don't feel like the current comment is very illuminating. Maybe "The time > between navigation and first backgrounding. If never backgrounded, this is > TimeDelta::Max(), if started in the background, it is null. There may be slight > innacuracies due to interprocess timestamps ..."? > > Also, a suggestion: I find the value/max/null sorta confusing and inconsistent. > Maybe just make it 0 if the page started in the background? I think that's > valid and consistent with the other values (0 == there was no time between > navigation and first backgrounding). > See what you're saying. Can I update this in my RAPPOR CL where I made some other changes already (pulled it out into its own method, and added much better comments). https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:182: UMA_HISTOGRAM_ENUMERATION("PageLoad.Events.Provisional.BG", event, On 2015/10/08 19:07:10, rdsmith wrote: > I'd like to get rid of "BG" and use "Background" and "Foreground". The custom > in Chromium is to avoid abbreviations > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin...). Is it worth it to have to deprecate the PageLoad.Timing2.* metrics for this change? https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:49: // inconsistencies between versions. On 2015/10/08 19:07:11, rdsmith wrote: > Should this comment be before the above enum and referring to both enums (or > duplicated)? It's duplicated as is, and I think that's the clearest. If you think I should make one of the comments refer to both enums then I'm willing to be convinced. https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:64: COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT, On 2015/10/08 19:07:11, rdsmith wrote: > I'm slightly confused about these enums. Is it true that > #(COMMITTED_LOAD_STARTED) == #(COMMITTED_LOAD_ABORTED_BEFORE_FIRST_LAYOUT) + > #(COMMITTED_LOAD_SUCCESSUL_FIRST_LAYOUT) + #(loads that failed but weren't > aborted before first layout)? If so, I'm not sure what "sum them to find the > total number of committed loads that we end up tracking" means? Is there some > other stat where that sum will be useful? If so, is there any way we can make > the useful number #(COMMITTED_LOAD_STARTED) so it's all in one place? > > Sorry if my suggestions for clarity are more confusing--I don't quite yet > understand the relationship between these enums. #(loads that failed but weren't aborted before first layout) is a subset of #(COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT), they got a first layout after all. So COMMITTED_LOAD_STARTED = COMMITTED_LOAD_ABORTED_BEFORE_FIRST_LAYOUT + COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT. The only reason I wanted this was to track how many navigations started in the background vs foreground.
I really like the separation into 2 enums. Thinking we might actually want 3: * ways that provisional loads complete (as you have it) * events that occur during the lifetime of committed loads * error counters (would just have one enum entry for now) Let's chat about it more in our sync happening shortly. https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:35: enum ProvisionalLoadEvent { general question here: do we want to track all provisional loads, or just those that meet our criteria for tracking (e.g. http/https only, etc?). https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:39: PROVISIONAL_LOAD_FAILED_NON_ABORT, i noticed in cries's message that some provisional loads don't commit because they result in downloads or other things. do we need an enum entry for those? seems like those aren't failures, but aren't going to commit either. or maybe we just don't count those? https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:57: enum CommittedLoadEvent { do we want to track all committed loads, or just the ones that meet our tracking criteria (http/https only etc)? https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:64: COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT, On 2015/10/08 21:27:02, csharrison wrote: > On 2015/10/08 19:07:11, rdsmith wrote: > > I'm slightly confused about these enums. Is it true that > > #(COMMITTED_LOAD_STARTED) == #(COMMITTED_LOAD_ABORTED_BEFORE_FIRST_LAYOUT) + > > #(COMMITTED_LOAD_SUCCESSUL_FIRST_LAYOUT) + #(loads that failed but weren't > > aborted before first layout)? If so, I'm not sure what "sum them to find the > > total number of committed loads that we end up tracking" means? Is there some > > other stat where that sum will be useful? If so, is there any way we can make > > the useful number #(COMMITTED_LOAD_STARTED) so it's all in one place? > > > > Sorry if my suggestions for clarity are more confusing--I don't quite yet > > understand the relationship between these enums. > > #(loads that failed but weren't aborted before first layout) is a subset of > #(COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT), they got a first layout after all. > > So COMMITTED_LOAD_STARTED = COMMITTED_LOAD_ABORTED_BEFORE_FIRST_LAYOUT + > COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT. The only reason I wanted this was to > track how many navigations started in the background vs foreground. Given ksakamoto's work on time to first text and time to first paint, I think it'd be better to have these target the first paint metrics, even though we don't have them yet. My guess is that once we have reliable first paint metrics, first layout will be less interesting. Want to rename these to FIRST_PAINT rather than first layout with a TODO in the code to start logging them once we have first paint instrumented? Or just omit first paint/first layout for now? https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:69: COMMITTED_LOAD_BAD_IPC, wondering if perhaps we should move errors to their own enum and track them in a separate histogram. maybe move this to PageLoadMetricsError enum and call it BAD_TIMING_IPC?
https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:35: enum ProvisionalLoadEvent { On 2015/10/09 14:24:35, Bryan McQuade wrote: > general question here: do we want to track all provisional loads, or just those > that meet our criteria for tracking (e.g. http/https only, etc?). Talked about this offline: we can't know if it meets our criteria until commit, so might as well track all of them. https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:39: PROVISIONAL_LOAD_FAILED_NON_ABORT, On 2015/10/09 14:24:35, Bryan McQuade wrote: > i noticed in cries's message that some provisional loads don't commit because > they result in downloads or other things. do we need an enum entry for those? > seems like those aren't failures, but aren't going to commit either. or maybe we > just don't count those? Will look into this. https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:57: enum CommittedLoadEvent { On 2015/10/09 14:24:35, Bryan McQuade wrote: > do we want to track all committed loads, or just the ones that meet our tracking > criteria (http/https only etc)? I think we should only track the ones we care about, the rest aren't affecting our histograms. https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:64: COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT, On 2015/10/09 14:24:35, Bryan McQuade wrote: > On 2015/10/08 21:27:02, csharrison wrote: > > On 2015/10/08 19:07:11, rdsmith wrote: > > > I'm slightly confused about these enums. Is it true that > > > #(COMMITTED_LOAD_STARTED) == #(COMMITTED_LOAD_ABORTED_BEFORE_FIRST_LAYOUT) + > > > #(COMMITTED_LOAD_SUCCESSUL_FIRST_LAYOUT) + #(loads that failed but weren't > > > aborted before first layout)? If so, I'm not sure what "sum them to find > the > > > total number of committed loads that we end up tracking" means? Is there > some > > > other stat where that sum will be useful? If so, is there any way we can > make > > > the useful number #(COMMITTED_LOAD_STARTED) so it's all in one place? > > > > > > Sorry if my suggestions for clarity are more confusing--I don't quite yet > > > understand the relationship between these enums. > > > > #(loads that failed but weren't aborted before first layout) is a subset of > > #(COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT), they got a first layout after all. > > > > So COMMITTED_LOAD_STARTED = COMMITTED_LOAD_ABORTED_BEFORE_FIRST_LAYOUT + > > COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT. The only reason I wanted this was to > > track how many navigations started in the background vs foreground. > > Given ksakamoto's work on time to first text and time to first paint, I think > it'd be better to have these target the first paint metrics, even though we > don't have them yet. My guess is that once we have reliable first paint metrics, > first layout will be less interesting. Want to rename these to FIRST_PAINT > rather than first layout with a TODO in the code to start logging them once we > have first paint instrumented? Or just omit first paint/first layout for now? adding a TODO to add them to the code once we have first paint. https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:69: COMMITTED_LOAD_BAD_IPC, On 2015/10/09 14:24:35, Bryan McQuade wrote: > wondering if perhaps we should move errors to their own enum and track them in a > separate histogram. maybe move this to PageLoadMetricsError enum and call it > BAD_TIMING_IPC? Done, but with a caveat that histograms can't be that small. I added some more. PTAL.
I think I'm ok with the COMMITTED_LOAD_START exception as long as it's documented clearly (both in the enum and in histograms.xml). https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:117: // this solution is the best we have right now. On 2015/10/08 21:27:02, csharrison wrote: > On 2015/10/08 19:07:10, rdsmith wrote: > > I continue to be confused by background_delta when I run into it. It isn't > > being added in this CL, but can I ask for a comment clarification anyway? I > > don't feel like the current comment is very illuminating. Maybe "The time > > between navigation and first backgrounding. If never backgrounded, this is > > TimeDelta::Max(), if started in the background, it is null. There may be > slight > > innacuracies due to interprocess timestamps ..."? > > > > Also, a suggestion: I find the value/max/null sorta confusing and > inconsistent. > > Maybe just make it 0 if the page started in the background? I think that's > > valid and consistent with the other values (0 == there was no time between > > navigation and first backgrounding). > > > > See what you're saying. Can I update this in my RAPPOR CL where I made some > other changes already (pulled it out into its own method, and added much better > comments). Sure, SG. https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:182: UMA_HISTOGRAM_ENUMERATION("PageLoad.Events.Provisional.BG", event, On 2015/10/08 21:27:02, csharrison wrote: > On 2015/10/08 19:07:10, rdsmith wrote: > > I'd like to get rid of "BG" and use "Background" and "Foreground". The custom > > in Chromium is to avoid abbreviations > > > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin...). > > Is it worth it to have to deprecate the PageLoad.Timing2.* metrics for this > change? I think I'd say "No, but I still think we should do it right for PageLoad.Events.Provisional.*". https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:49: // inconsistencies between versions. On 2015/10/08 21:27:02, csharrison wrote: > On 2015/10/08 19:07:11, rdsmith wrote: > > Should this comment be before the above enum and referring to both enums (or > > duplicated)? > > It's duplicated as is, and I think that's the clearest. If you think I should > make one of the comments refer to both enums then I'm willing to be convinced. Whoops, I'm sorry, missed the duplication. Please ignore this comment. https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:64: COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT, On 2015/10/08 21:27:02, csharrison wrote: > On 2015/10/08 19:07:11, rdsmith wrote: > > I'm slightly confused about these enums. Is it true that > > #(COMMITTED_LOAD_STARTED) == #(COMMITTED_LOAD_ABORTED_BEFORE_FIRST_LAYOUT) + > > #(COMMITTED_LOAD_SUCCESSUL_FIRST_LAYOUT) + #(loads that failed but weren't > > aborted before first layout)? If so, I'm not sure what "sum them to find the > > total number of committed loads that we end up tracking" means? Is there some > > other stat where that sum will be useful? If so, is there any way we can make > > the useful number #(COMMITTED_LOAD_STARTED) so it's all in one place? > > > > Sorry if my suggestions for clarity are more confusing--I don't quite yet > > understand the relationship between these enums. > > #(loads that failed but weren't aborted before first layout) is a subset of > #(COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT), they got a first layout after all. So I take that to mean that there's no way that a load can fail before first layout other than being aborted? That wasn't clear to me--I think of abort as a particular type of failure. > So COMMITTED_LOAD_STARTED = COMMITTED_LOAD_ABORTED_BEFORE_FIRST_LAYOUT + > COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT. The only reason I wanted this was to > track how many navigations started in the background vs foreground. So that equality is accurate *except* that the foreground/background split is different for LOAD_STARTED and the other two, right? If we're actually doing to do that, I think that difference has to be reflected in the comments and in the histogram entry description--otherwise people are going to be very confused reading the graph (CLS will be > CLABFL + CLSFL on the foreground graph, and < on the background graph, right?)
basically looks good - just a few small things https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:39: PROVISIONAL_LOAD_FAILED_NON_ABORT, On 2015/10/09 22:30:41, csharrison wrote: > On 2015/10/09 14:24:35, Bryan McQuade wrote: > > i noticed in cries's message that some provisional loads don't commit because > > they result in downloads or other things. do we need an enum entry for those? > > seems like those aren't failures, but aren't going to commit either. or maybe > we > > just don't count those? > > Will look into this. Thanks! Just want to get clarity on this one before we commit the change. https://codereview.chromium.org/1384213002/diff/120001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1384213002/diff/120001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:90: // but at the start of the navigation. let's add a little more detail to explain why we choose to look at background at nav start, rather than at any time up to the commit. https://codereview.chromium.org/1384213002/diff/120001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:105: } can we add an else along with a new error enum entry to count how often this happens? https://codereview.chromium.org/1384213002/diff/120001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:301: UMA_HISTOGRAM_ENUMERATION("PageLoad.Events.Error", Is there a name we could use to convey that these errors aren't errors happening in the page, but errors internal to the page load metrics implementation? Maybe InternalError, InternalTrackingError, or InternalMetricsError?
Just got confirmation from Charlie Reis that downloads, 204s, etc. will call DidFinishNavigation, but HasCommitted() will be false. Unfortunately this comes with ERR_ABORTED error because the user agent aborts the navigation in blink. I opened an issue at https://crbug.com/542369 tracking this. I'm unsure if this is intended behavior. https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1384213002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:64: COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT, On 2015/10/12 15:45:22, rdsmith wrote: > On 2015/10/08 21:27:02, csharrison wrote: > > On 2015/10/08 19:07:11, rdsmith wrote: > > > I'm slightly confused about these enums. Is it true that > > > #(COMMITTED_LOAD_STARTED) == #(COMMITTED_LOAD_ABORTED_BEFORE_FIRST_LAYOUT) + > > > #(COMMITTED_LOAD_SUCCESSUL_FIRST_LAYOUT) + #(loads that failed but weren't > > > aborted before first layout)? If so, I'm not sure what "sum them to find > the > > > total number of committed loads that we end up tracking" means? Is there > some > > > other stat where that sum will be useful? If so, is there any way we can > make > > > the useful number #(COMMITTED_LOAD_STARTED) so it's all in one place? > > > > > > Sorry if my suggestions for clarity are more confusing--I don't quite yet > > > understand the relationship between these enums. > > > > #(loads that failed but weren't aborted before first layout) is a subset of > > #(COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT), they got a first layout after all. > > So I take that to mean that there's no way that a load can fail before first > layout other than being aborted? That wasn't clear to me--I think of abort as a > particular type of failure. > > > So COMMITTED_LOAD_STARTED = COMMITTED_LOAD_ABORTED_BEFORE_FIRST_LAYOUT + > > COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT. The only reason I wanted this was to > > track how many navigations started in the background vs foreground. > > So that equality is accurate *except* that the foreground/background split is > different for LOAD_STARTED and the other two, right? If we're actually doing to > do that, I think that difference has to be reflected in the comments and in the > histogram entry description--otherwise people are going to be very confused > reading the graph (CLS will be > CLABFL + CLSFL on the foreground graph, and < > on the background graph, right?) We were treating all failures post commit as aborts. Perhaps we should change the name to COMMITTED_LOAD_FAILED_BEFORE_*. Regarding LOAD_STARTED. You are right, I'll edit the comments. https://codereview.chromium.org/1384213002/diff/120001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1384213002/diff/120001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:90: // but at the start of the navigation. On 2015/10/12 16:39:22, Bryan McQuade wrote: > let's add a little more detail to explain why we choose to look at background at > nav start, rather than at any time up to the commit. Done. https://codereview.chromium.org/1384213002/diff/120001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:105: } On 2015/10/12 16:39:23, Bryan McQuade wrote: > can we add an else along with a new error enum entry to count how often this > happens? We do this one frame up the stack, per Randy's review. This coincides with ERR_BAD_TIMING_IPC.
looks good once these are addressed. sorry for review latency on this one. https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:185: UMA_HISTOGRAM_ENUMERATION("PageLoad.Events.Provisional.Background", event, let's use 'BG' here rather than 'Background' for consistency with the others, unless there's a reason we can't do that. https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:199: UMA_HISTOGRAM_ENUMERATION("PageLoad.Events.Committed.Background", event, same - .BG for consistency https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:255: return; is it ever valid for HasCommitted() to be false and GetNetErrorCode() to be net::OK? if so, do we need a new provisional load event to handle that case? if not, let's log an InternalError in an else clause just to make sure we aren't hitting that case. https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:36: // An aborted provisional load has error net::ERR_ABORTED. Let's add a comment noting that Downloads and 204s are currently also included in this case, with a link to the bug you opened. https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:54304: + <int value="3" label="Received a bad timing IPC"/> can remove this one (since it moved to the error enum) https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:56977: +<enum name="ErrorLoadEvent" type="int"> rename to InternalErrorLoadEvent https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:67215: + <int value="6" label="Aborted load before first layout (relevant page load)"/> looks like these can be removed since we've abandoned the PageLoadEvent enum. let's also add an <obsolete> tag to this enum. https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:69323: + <int value="0" label="Provisional load aborted"/> let's add '(also includes downloads and 204s. crbug.com/542369)' so people using the UMA dash are also aware of this. https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:78210: +<histogram_suffixes name="PageLoadBackgrounded2" separator="."> Can we use the existing 'BG' for these event cases as well?
Randy, care to make your case for Background > BG for the new events? https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:185: UMA_HISTOGRAM_ENUMERATION("PageLoad.Events.Provisional.Background", event, On 2015/10/15 03:47:36, Bryan McQuade wrote: > let's use 'BG' here rather than 'Background' for consistency with the others, > unless there's a reason we can't do that. Haha I agree with you, though it's going against Randy's wishes. I'll give him time to respond before I make the change. https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:255: return; On 2015/10/15 03:47:36, Bryan McQuade wrote: > is it ever valid for HasCommitted() to be false and GetNetErrorCode() to be > net::OK? if so, do we need a new provisional load event to handle that case? if > not, let's log an InternalError in an else clause just to make sure we aren't > hitting that case. After looking through the navigation code, I'm not sure if this is possible, or if it is an error if it is :P. I'll keep investigating. https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:36: // An aborted provisional load has error net::ERR_ABORTED. On 2015/10/15 03:47:36, Bryan McQuade wrote: > Let's add a comment noting that Downloads and 204s are currently also included > in this case, with a link to the bug you opened. Done. In my next abort CL I'll distinguish UserAborts from this more general kind. https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:54304: + <int value="3" label="Received a bad timing IPC"/> On 2015/10/15 03:47:36, Bryan McQuade wrote: > can remove this one (since it moved to the error enum) Good catch. Done. https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:56977: +<enum name="ErrorLoadEvent" type="int"> On 2015/10/15 03:47:36, Bryan McQuade wrote: > rename to InternalErrorLoadEvent Done. https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:67215: + <int value="6" label="Aborted load before first layout (relevant page load)"/> On 2015/10/15 03:47:36, Bryan McQuade wrote: > looks like these can be removed since we've abandoned the PageLoadEvent enum. > let's also add an <obsolete> tag to this enum. Done. https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:69323: + <int value="0" label="Provisional load aborted"/> On 2015/10/15 03:47:36, Bryan McQuade wrote: > let's add '(also includes downloads and 204s. crbug.com/542369)' so people using > the UMA dash are also aware of this. Done. https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:78210: +<histogram_suffixes name="PageLoadBackgrounded2" separator="."> On 2015/10/15 03:47:36, Bryan McQuade wrote: > Can we use the existing 'BG' for these event cases as well? Same as other comment, per Randy.
On 2015/10/15 14:06:48, csharrison wrote: > Randy, care to make your case for Background > BG for the new events? I prefer consistency here overall. Let's either rename everything to Background, or use BG everywhere. I don't have a strong preference either way. I'm ok with losing the old histogram data for the *.BG histograms if we do a rename. > > https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > (right): > > https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... > components/page_load_metrics/browser/metrics_web_contents_observer.cc:185: > UMA_HISTOGRAM_ENUMERATION("PageLoad.Events.Provisional.Background", event, > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > let's use 'BG' here rather than 'Background' for consistency with the others, > > unless there's a reason we can't do that. > > Haha I agree with you, though it's going against Randy's wishes. I'll give him > time to respond before I make the change. > > https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... > components/page_load_metrics/browser/metrics_web_contents_observer.cc:255: > return; > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > is it ever valid for HasCommitted() to be false and GetNetErrorCode() to be > > net::OK? if so, do we need a new provisional load event to handle that case? > if > > not, let's log an InternalError in an else clause just to make sure we aren't > > hitting that case. > > After looking through the navigation code, I'm not sure if this is possible, or > if it is an error if it is :P. I'll keep investigating. > > https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... > File components/page_load_metrics/browser/metrics_web_contents_observer.h > (right): > > https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... > components/page_load_metrics/browser/metrics_web_contents_observer.h:36: // An > aborted provisional load has error net::ERR_ABORTED. > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > Let's add a comment noting that Downloads and 204s are currently also included > > in this case, with a link to the bug you opened. > > Done. In my next abort CL I'll distinguish UserAborts from this more general > kind. > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:54304: + <int value="3" label="Received > a bad timing IPC"/> > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > can remove this one (since it moved to the error enum) > > Good catch. Done. > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:56977: +<enum name="ErrorLoadEvent" > type="int"> > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > rename to InternalErrorLoadEvent > > Done. > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:67215: + <int value="6" label="Aborted > load before first layout (relevant page load)"/> > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > looks like these can be removed since we've abandoned the PageLoadEvent enum. > > let's also add an <obsolete> tag to this enum. > > Done. > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:69323: + <int value="0" > label="Provisional load aborted"/> > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > let's add '(also includes downloads and 204s. crbug.com/542369)' so people > using > > the UMA dash are also aware of this. > > Done. > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:78210: +<histogram_suffixes > name="PageLoadBackgrounded2" separator="."> > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > Can we use the existing 'BG' for these event cases as well? > > Same as other comment, per Randy.
https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:255: return; Sounds good. I'd propose adding an error enum (any maybe a DCHECK) even if it's currently not possible, just in case code changes in the future such that it becomes possible.
https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:255: return; On 2015/10/15 14:12:41, Bryan McQuade wrote: > Sounds good. I'd propose adding an error enum (any maybe a DCHECK) even if it's > currently not possible, just in case code changes in the future such that it > becomes possible. According to clamy, this will signal a user stopping a navigation post-PlzNavigate. It should not fire pre-PlzNavigate.
https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:255: return; On 2015/10/15 15:13:49, csharrison wrote: > On 2015/10/15 14:12:41, Bryan McQuade wrote: > > Sounds good. I'd propose adding an error enum (any maybe a DCHECK) even if > it's > > currently not possible, just in case code changes in the future such that it > > becomes possible. > > According to clamy, this will signal a user stopping a navigation > post-PlzNavigate. It should not fire pre-PlzNavigate. great, thanks for investigating that. what do you think we should do in the meantime? maybe add a DCHECK and a comment noting that we do expect this to start firing eventually as part of plznavigate, and want to update our code to handle that case once it happens. maybe also file a bug against plznavigate folks about this so they remember to help us update this bit of code once the plznav changes are ready.
On 2015/10/15 14:09:52, Bryan McQuade wrote: > On 2015/10/15 14:06:48, csharrison wrote: > > Randy, care to make your case for Background > BG for the new events? > > I prefer consistency here overall. Let's either rename everything to Background, > or use BG everywhere. I don't have a strong preference either way. I'm ok with > losing the old histogram data for the *.BG histograms if we do a rename. I have no objection to bringing us entirely into agreement with the style guide, but I would object reasonably strongly to further divergence from it. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin... If either of you really don't want Background/Foreground, could you stop by my desk so that I can beat you into submission ^W^W^W^W discuss it with you in a calm and rational fashion? :-} > > > > > > https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... > > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > > (right): > > > > > https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... > > components/page_load_metrics/browser/metrics_web_contents_observer.cc:185: > > UMA_HISTOGRAM_ENUMERATION("PageLoad.Events.Provisional.Background", event, > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > let's use 'BG' here rather than 'Background' for consistency with the > others, > > > unless there's a reason we can't do that. > > > > Haha I agree with you, though it's going against Randy's wishes. I'll give him > > time to respond before I make the change. > > > > > https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... > > components/page_load_metrics/browser/metrics_web_contents_observer.cc:255: > > return; > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > is it ever valid for HasCommitted() to be false and GetNetErrorCode() to be > > > net::OK? if so, do we need a new provisional load event to handle that case? > > if > > > not, let's log an InternalError in an else clause just to make sure we > aren't > > > hitting that case. > > > > After looking through the navigation code, I'm not sure if this is possible, > or > > if it is an error if it is :P. I'll keep investigating. > > > > > https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... > > File components/page_load_metrics/browser/metrics_web_contents_observer.h > > (right): > > > > > https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... > > components/page_load_metrics/browser/metrics_web_contents_observer.h:36: // An > > aborted provisional load has error net::ERR_ABORTED. > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > Let's add a comment noting that Downloads and 204s are currently also > included > > > in this case, with a link to the bug you opened. > > > > Done. In my next abort CL I'll distinguish UserAborts from this more general > > kind. > > > > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > > File tools/metrics/histograms/histograms.xml (right): > > > > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > > tools/metrics/histograms/histograms.xml:54304: + <int value="3" > label="Received > > a bad timing IPC"/> > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > can remove this one (since it moved to the error enum) > > > > Good catch. Done. > > > > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > > tools/metrics/histograms/histograms.xml:56977: +<enum name="ErrorLoadEvent" > > type="int"> > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > rename to InternalErrorLoadEvent > > > > Done. > > > > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > > tools/metrics/histograms/histograms.xml:67215: + <int value="6" > label="Aborted > > load before first layout (relevant page load)"/> > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > looks like these can be removed since we've abandoned the PageLoadEvent > enum. > > > let's also add an <obsolete> tag to this enum. > > > > Done. > > > > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > > tools/metrics/histograms/histograms.xml:69323: + <int value="0" > > label="Provisional load aborted"/> > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > let's add '(also includes downloads and 204s. crbug.com/542369)' so people > > using > > > the UMA dash are also aware of this. > > > > Done. > > > > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > > tools/metrics/histograms/histograms.xml:78210: +<histogram_suffixes > > name="PageLoadBackgrounded2" separator="."> > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > Can we use the existing 'BG' for these event cases as well? > > > > Same as other comment, per Randy.
On 2015/10/15 17:31:35, rdsmith wrote: > On 2015/10/15 14:09:52, Bryan McQuade wrote: > > On 2015/10/15 14:06:48, csharrison wrote: > > > Randy, care to make your case for Background > BG for the new events? > > > > I prefer consistency here overall. Let's either rename everything to > Background, > > or use BG everywhere. I don't have a strong preference either way. I'm ok with > > losing the old histogram data for the *.BG histograms if we do a rename. > > I have no objection to bringing us entirely into agreement with the style guide, > but I would object reasonably strongly to further divergence from it. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin... > > If either of you really don't want Background/Foreground, could you stop by my > desk so that I can beat you into submission ^W^W^W^W discuss it with you in a > calm and rational fashion? :-} > > > > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... > > > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... > > > components/page_load_metrics/browser/metrics_web_contents_observer.cc:185: > > > UMA_HISTOGRAM_ENUMERATION("PageLoad.Events.Provisional.Background", event, > > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > > let's use 'BG' here rather than 'Background' for consistency with the > > others, > > > > unless there's a reason we can't do that. > > > > > > Haha I agree with you, though it's going against Randy's wishes. I'll give > him > > > time to respond before I make the change. > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... > > > components/page_load_metrics/browser/metrics_web_contents_observer.cc:255: > > > return; > > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > > is it ever valid for HasCommitted() to be false and GetNetErrorCode() to > be > > > > net::OK? if so, do we need a new provisional load event to handle that > case? > > > if > > > > not, let's log an InternalError in an else clause just to make sure we > > aren't > > > > hitting that case. > > > > > > After looking through the navigation code, I'm not sure if this is possible, > > or > > > if it is an error if it is :P. I'll keep investigating. > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... > > > File components/page_load_metrics/browser/metrics_web_contents_observer.h > > > (right): > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... > > > components/page_load_metrics/browser/metrics_web_contents_observer.h:36: // > An > > > aborted provisional load has error net::ERR_ABORTED. > > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > > Let's add a comment noting that Downloads and 204s are currently also > > included > > > > in this case, with a link to the bug you opened. > > > > > > Done. In my next abort CL I'll distinguish UserAborts from this more general > > > kind. > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > > > File tools/metrics/histograms/histograms.xml (right): > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > > > tools/metrics/histograms/histograms.xml:54304: + <int value="3" > > label="Received > > > a bad timing IPC"/> > > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > > can remove this one (since it moved to the error enum) > > > > > > Good catch. Done. > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > > > tools/metrics/histograms/histograms.xml:56977: +<enum name="ErrorLoadEvent" > > > type="int"> > > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > > rename to InternalErrorLoadEvent > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > > > tools/metrics/histograms/histograms.xml:67215: + <int value="6" > > label="Aborted > > > load before first layout (relevant page load)"/> > > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > > looks like these can be removed since we've abandoned the PageLoadEvent > > enum. > > > > let's also add an <obsolete> tag to this enum. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > > > tools/metrics/histograms/histograms.xml:69323: + <int value="0" > > > label="Provisional load aborted"/> > > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > > let's add '(also includes downloads and 204s. crbug.com/542369)' so people > > > using > > > > the UMA dash are also aware of this. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > > > tools/metrics/histograms/histograms.xml:78210: +<histogram_suffixes > > > name="PageLoadBackgrounded2" separator="."> > > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > > Can we use the existing 'BG' for these event cases as well? > > > > > > Same as other comment, per Randy. BG/Background: tossed that to time to first text owners. I imagine they'll be with with it. Bryan: I'd prefer to just log when that occurs. I'm not sure it's an error case that should be DCHECKed, and even if it were, I don't think here would be the place for it. It should go in the navigation code. If we get hits on it we can notify the PlzNavigate team.
sure, I'm fine with logging in that else case. that sounds better than a dcheck. On Thu, Oct 15, 2015 at 2:09 PM <csharrison@chromium.org> wrote: > On 2015/10/15 17:31:35, rdsmith wrote: > > On 2015/10/15 14:09:52, Bryan McQuade wrote: > > > On 2015/10/15 14:06:48, csharrison wrote: > > > > Randy, care to make your case for Background > BG for the new events? > > > > > > I prefer consistency here overall. Let's either rename everything to > > Background, > > > or use BG everywhere. I don't have a strong preference either way. I'm > > ok > with > > > losing the old histogram data for the *.BG histograms if we do a > rename. > > > I have no objection to bringing us entirely into agreement with the style > guide, > > but I would object reasonably strongly to further divergence from it. > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin... > > > If either of you really don't want Background/Foreground, could you stop > > by my > > desk so that I can beat you into submission ^W^W^W^W discuss it with you > > in a > > calm and rational fashion? :-} > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... > > > > File > > components/page_load_metrics/browser/metrics_web_contents_observer.cc > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... > > > > > > > components/page_load_metrics/browser/metrics_web_contents_observer.cc:185: > > > > UMA_HISTOGRAM_ENUMERATION("PageLoad.Events.Provisional.Background", > > event, > > > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > > > let's use 'BG' here rather than 'Background' for consistency with > > the > > > others, > > > > > unless there's a reason we can't do that. > > > > > > > > Haha I agree with you, though it's going against Randy's wishes. I'll > > give > > him > > > > time to respond before I make the change. > > > > > > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... > > > > > > > components/page_load_metrics/browser/metrics_web_contents_observer.cc:255: > > > > return; > > > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > > > is it ever valid for HasCommitted() to be false and > > GetNetErrorCode() to > > be > > > > > net::OK? if so, do we need a new provisional load event to handle > > that > > case? > > > > if > > > > > not, let's log an InternalError in an else clause just to make sure > > we > > > aren't > > > > > hitting that case. > > > > > > > > After looking through the navigation code, I'm not sure if this is > possible, > > > or > > > > if it is an error if it is :P. I'll keep investigating. > > > > > > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... > > > > File > > components/page_load_metrics/browser/metrics_web_contents_observer.h > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/components/page_load_m... > > > > > > components/page_load_metrics/browser/metrics_web_contents_observer.h:36: > // > > An > > > > aborted provisional load has error net::ERR_ABORTED. > > > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > > > Let's add a comment noting that Downloads and 204s are currently > > also > > > included > > > > > in this case, with a link to the bug you opened. > > > > > > > > Done. In my next abort CL I'll distinguish UserAborts from this more > general > > > > kind. > > > > > > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > > > > File tools/metrics/histograms/histograms.xml (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > > > > tools/metrics/histograms/histograms.xml:54304: + <int value="3" > > > label="Received > > > > a bad timing IPC"/> > > > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > > > can remove this one (since it moved to the error enum) > > > > > > > > Good catch. Done. > > > > > > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > > > > tools/metrics/histograms/histograms.xml:56977: +<enum > name="ErrorLoadEvent" > > > > type="int"> > > > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > > > rename to InternalErrorLoadEvent > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > > > > tools/metrics/histograms/histograms.xml:67215: + <int value="6" > > > label="Aborted > > > > load before first layout (relevant page load)"/> > > > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > > > looks like these can be removed since we've abandoned the > > PageLoadEvent > > > enum. > > > > > let's also add an <obsolete> tag to this enum. > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > > > > tools/metrics/histograms/histograms.xml:69323: + <int value="0" > > > > label="Provisional load aborted"/> > > > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > > > let's add '(also includes downloads and 204s. crbug.com/542369)' > so > people > > > > using > > > > > the UMA dash are also aware of this. > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/1384213002/diff/140001/tools/metrics/histogra... > > > > tools/metrics/histograms/histograms.xml:78210: +<histogram_suffixes > > > > name="PageLoadBackgrounded2" separator="."> > > > > On 2015/10/15 03:47:36, Bryan McQuade wrote: > > > > > Can we use the existing 'BG' for these event cases as well? > > > > > > > > Same as other comment, per Randy. > > BG/Background: tossed that to time to first text owners. I imagine they'll > be > with with it. > Bryan: I'd prefer to just log when that occurs. I'm not sure it's an error > case > that should be DCHECKed, and even if it were, I don't think here would be > the > place for it. It should go in the navigation code. If we get hits on it we > can > notify the PlzNavigate team. > > https://codereview.chromium.org/1384213002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
So we covered most of this offline, but: * I now understand that there isn't any conceptual different between COMMITTED_LOAD_STARTED and the other enums in that histogram. They're all segmented Background/Foreground by whether or not they were at any point in the background between when they were started and the event in question. I'd encourage the comments to make that commonality clear--I'm sorry I pushed you off track on that. (Feel free to include a sentence pointing out that for the STARTED event "at any time between start and event" means "when it started" :-}.) * Keep comments about when things are recorded out of histograms.xml. If they're useful for understanding the code, they're appropriate in .h/.cc file (my guess would be that the comment about the difference between recording time and event time is based place at point of recording). * As noted below, I'm a bit concerned by the use of the "provisional" description when we're talking about committed loads, but that may be moot since you're going to rewrite the comment. * I feel like there are a couple of places where you imply that Background/Foreground separation is determined based on the BG/FG state at the actual commit event when it's actually determined by whether the tab ever went background; would you clean that up? Sorry to be such a pedant; thanks for bearing with me. https://codereview.chromium.org/1384213002/diff/160001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1384213002/diff/160001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:68: // background/foreground specifies whether the provisional load started nit, suggestion: "provisional" -> "eventually committed"? The whole point of this enum is that we're only tracking stuff that eventually results in commit, right? https://codereview.chromium.org/1384213002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1384213002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:30118: + the actual commit event. This is because we don't have all the data to The rest aren't segmented by actual commit event, but by whether they were ever in the background, right?
PTAL. I added an additional check to block non html mimetypes to reflect what our renderframeobserver does. Otherwise we track the page but get no IPCs.
csharrison@chromium.org changed reviewers: + asvitkine@chromium.org
Alexei, take a look at histograms? Thanks!
On 2015/10/16 13:09:18, csharrison wrote: > Alexei, take a look at histograms? Thanks! Oh, forgot to mention: We got the go-ahead from time to first text owners on the name deprecation.
lgtm - just one request https://codereview.chromium.org/1384213002/diff/180001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1384213002/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:346: std::string mime = web_contents()->GetContentsMimeType(); GetContentsMimeType only returns the correct value after a certain point in the navigation. If we happened to try to call IsRelevantNavigation from somewhere else in the code, we might get an empty string here, or possibly the mime type of the prior nav, and it would not be immediately obvious at the call site that that could happen (though tests should fail, but i'd like it to be more apparent to the caller). So I think it's important that callers of this method are aware of the input params and take care to think about whether they can get valid values for those params at the time they call. I'd propose making IsRelevantNavigation a non-class method in the anonymous namespace above that takes all params required for it to make policy decisions: bool IsRelevantNavigation(content::NavigationHandle* navigation_handle, const std::string& browser_url, const std::string& mime_type); That way the caller is responsible for providing valid values, forcing them to make sure they can get the right url and mime type at the point of invocation.
On 2015/10/16 15:01:29, Bryan McQuade wrote: > lgtm - just one request > > https://codereview.chromium.org/1384213002/diff/180001/components/page_load_m... > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > (right): > > https://codereview.chromium.org/1384213002/diff/180001/components/page_load_m... > components/page_load_metrics/browser/metrics_web_contents_observer.cc:346: > std::string mime = web_contents()->GetContentsMimeType(); > GetContentsMimeType only returns the correct value after a certain point in the > navigation. If we happened to try to call IsRelevantNavigation from somewhere > else in the code, we might get an empty string here, or possibly the mime type > of the prior nav, and it would not be immediately obvious at the call site that > that could happen (though tests should fail, but i'd like it to be more apparent > to the caller). So I think it's important that callers of this method are aware > of the input params and take care to think about whether they can get valid > values for those params at the time they call. > > I'd propose making IsRelevantNavigation a non-class method in the anonymous > namespace above that takes all params required for it to make policy decisions: > > bool IsRelevantNavigation(content::NavigationHandle* navigation_handle, > const std::string& browser_url, > const std::string& mime_type); > > That way the caller is responsible for providing valid values, forcing them to > make sure they can get the right url and mime type at the point of invocation. The mime is valid after a committed main frame navigation, I believe. Why don't we just DCHECK these assumptions in the method?
On 2015/10/16 15:11:12, csharrison wrote: > On 2015/10/16 15:01:29, Bryan McQuade wrote: > > lgtm - just one request > > > > > https://codereview.chromium.org/1384213002/diff/180001/components/page_load_m... > > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > > (right): > > > > > https://codereview.chromium.org/1384213002/diff/180001/components/page_load_m... > > components/page_load_metrics/browser/metrics_web_contents_observer.cc:346: > > std::string mime = web_contents()->GetContentsMimeType(); > > GetContentsMimeType only returns the correct value after a certain point in > the > > navigation. If we happened to try to call IsRelevantNavigation from somewhere > > else in the code, we might get an empty string here, or possibly the mime type > > of the prior nav, and it would not be immediately obvious at the call site > that > > that could happen (though tests should fail, but i'd like it to be more > apparent > > to the caller). So I think it's important that callers of this method are > aware > > of the input params and take care to think about whether they can get valid > > values for those params at the time they call. > > > > I'd propose making IsRelevantNavigation a non-class method in the anonymous > > namespace above that takes all params required for it to make policy > decisions: > > > > bool IsRelevantNavigation(content::NavigationHandle* navigation_handle, > > const std::string& browser_url, > > const std::string& mime_type); > > > > That way the caller is responsible for providing valid values, forcing them to > > make sure they can get the right url and mime type at the point of invocation. > > The mime is valid after a committed main frame navigation, I believe. Why don't > we just DCHECK these assumptions in the method? You can add a DCHECK, but I expect that it may sometimes be valid for the field to be empty after a commit if we don't know the mime type of the resource being navigated to. Not totally sure though. Either way I think this is the kind of method that's better served being static, so it's explicit to callers what the required state is, and they can make sure to provide it at the call site.
On 2015/10/16 15:15:34, Bryan McQuade wrote: > On 2015/10/16 15:11:12, csharrison wrote: > > On 2015/10/16 15:01:29, Bryan McQuade wrote: > > > lgtm - just one request > > > > > > > > > https://codereview.chromium.org/1384213002/diff/180001/components/page_load_m... > > > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/1384213002/diff/180001/components/page_load_m... > > > components/page_load_metrics/browser/metrics_web_contents_observer.cc:346: > > > std::string mime = web_contents()->GetContentsMimeType(); > > > GetContentsMimeType only returns the correct value after a certain point in > > the > > > navigation. If we happened to try to call IsRelevantNavigation from > somewhere > > > else in the code, we might get an empty string here, or possibly the mime > type > > > of the prior nav, and it would not be immediately obvious at the call site > > that > > > that could happen (though tests should fail, but i'd like it to be more > > apparent > > > to the caller). So I think it's important that callers of this method are > > aware > > > of the input params and take care to think about whether they can get valid > > > values for those params at the time they call. > > > > > > I'd propose making IsRelevantNavigation a non-class method in the anonymous > > > namespace above that takes all params required for it to make policy > > decisions: > > > > > > bool IsRelevantNavigation(content::NavigationHandle* navigation_handle, > > > const std::string& browser_url, > > > const std::string& mime_type); > > > > > > That way the caller is responsible for providing valid values, forcing them > to > > > make sure they can get the right url and mime type at the point of > invocation. > > > > The mime is valid after a committed main frame navigation, I believe. Why > don't > > we just DCHECK these assumptions in the method? > > You can add a DCHECK, but I expect that it may sometimes be valid for the field > to be empty after a commit if we don't know the mime type of the resource being > navigated to. Not totally sure though. Either way I think this is the kind of > method that's better served being static, so it's explicit to callers what the > required state is, and they can make sure to provide it at the call site. That said, I'm fine leaving it as-is and adding a DCHECK that the mime type is not empty if you prefer. (maybe also a DCHECK that the browser url is not empty?)
https://codereview.chromium.org/1384213002/diff/180001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1384213002/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:346: std::string mime = web_contents()->GetContentsMimeType(); nit: since GetContentsMimeType returns a const std::string&, let's change the type here to const std::string& as well to avoid the string copy
Can you associate a crbug with this, given it's not a trivial change? https://codereview.chromium.org/1384213002/diff/180001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1384213002/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:111: return !(started_in_foreground_ && background_time_.is_null()); Nit: !(a && b) -> !a || !b https://codereview.chromium.org/1384213002/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:315: ERR_IPC_FROM_WRONG_FRAME, ERR_LAST_ENTRY); Please make a helper function for this histogram, with the enum as the param. Each macro expands to a bunch of machine code, so this way you reduce the bloat. https://codereview.chromium.org/1384213002/diff/180001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc (right): https://codereview.chromium.org/1384213002/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc:125: }; Nit: DISALLOW_COPY_AND_ASSIGN()
Added a crbug. Originally this CL was a trivial bug fix but it spun into some new features. https://codereview.chromium.org/1384213002/diff/180001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1384213002/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:111: return !(started_in_foreground_ && background_time_.is_null()); On 2015/10/16 15:43:14, Alexei Svitkine wrote: > Nit: !(a && b) -> !a || !b Done. https://codereview.chromium.org/1384213002/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:315: ERR_IPC_FROM_WRONG_FRAME, ERR_LAST_ENTRY); On 2015/10/16 15:43:15, Alexei Svitkine wrote: > Please make a helper function for this histogram, with the enum as the param. > Each macro expands to a bunch of machine code, so this way you reduce the bloat. Done. Good call. https://codereview.chromium.org/1384213002/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:346: std::string mime = web_contents()->GetContentsMimeType(); On 2015/10/16 15:01:29, Bryan McQuade wrote: > GetContentsMimeType only returns the correct value after a certain point in the > navigation. If we happened to try to call IsRelevantNavigation from somewhere > else in the code, we might get an empty string here, or possibly the mime type > of the prior nav, and it would not be immediately obvious at the call site that > that could happen (though tests should fail, but i'd like it to be more apparent > to the caller). So I think it's important that callers of this method are aware > of the input params and take care to think about whether they can get valid > values for those params at the time they call. > > I'd propose making IsRelevantNavigation a non-class method in the anonymous > namespace above that takes all params required for it to make policy decisions: > > bool IsRelevantNavigation(content::NavigationHandle* navigation_handle, > const std::string& browser_url, > const std::string& mime_type); > > That way the caller is responsible for providing valid values, forcing them to > make sure they can get the right url and mime type at the point of invocation. Done. https://codereview.chromium.org/1384213002/diff/180001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc (right): https://codereview.chromium.org/1384213002/diff/180001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc:125: }; On 2015/10/16 15:43:15, Alexei Svitkine wrote: > Nit: DISALLOW_COPY_AND_ASSIGN() Done.
lgtm % remaining comments https://codereview.chromium.org/1384213002/diff/220001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1384213002/diff/220001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:33: // If you add elements from this enum, make sure you update the enum Nit: "from" -> "to" https://codereview.chromium.org/1384213002/diff/220001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:37: // ProvisionalLoadEvents count all main frame navigations before they commit. Nit: Switch the order of the two paragraphs. I think having the description of the enum be at the start of the comment and the comment about UMA be the second paragraph will make it so it's less likely someone will miss the UMA paragraph. Same for the next enum. https://codereview.chromium.org/1384213002/diff/220001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:54: // Counts the number of successful commits. Nit: Add an empty line above. https://codereview.chromium.org/1384213002/diff/220001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:61: // If you add elements from this enum, make sure you update the enum Nit: "from" -> "to" https://codereview.chromium.org/1384213002/diff/220001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:89: // reflect actual errors that occur during a page load. Nit: Add the same comment about UMA histograms.xml here. https://codereview.chromium.org/1384213002/diff/220001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc (right): https://codereview.chromium.org/1384213002/diff/220001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc:65: num_errors_ = 0; Nit: Init these in the ctor instead. The ctor will be re-run for each test, so there shouldn't be any behavior difference and this will fix warnings (from some IDEs and lint tools) that you're not initializing variables in the ctor. https://codereview.chromium.org/1384213002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1384213002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:69634: + label="Provisional load stopped by user with no error. Should not occur Nit: Long description can go between the <int></int> tags. Suggest making the label= description shorter. See AndroidSigninPromoAction for an example.
Thanks Alexei! https://codereview.chromium.org/1384213002/diff/220001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1384213002/diff/220001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:33: // If you add elements from this enum, make sure you update the enum On 2015/10/16 16:48:54, Alexei Svitkine wrote: > Nit: "from" -> "to" Done. https://codereview.chromium.org/1384213002/diff/220001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:37: // ProvisionalLoadEvents count all main frame navigations before they commit. On 2015/10/16 16:48:53, Alexei Svitkine wrote: > Nit: Switch the order of the two paragraphs. I think having the description of > the enum be at the start of the comment and the comment about UMA be the second > paragraph will make it so it's less likely someone will miss the UMA paragraph. > Same for the next enum. Done. https://codereview.chromium.org/1384213002/diff/220001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:54: // Counts the number of successful commits. On 2015/10/16 16:48:53, Alexei Svitkine wrote: > Nit: Add an empty line above. Done. https://codereview.chromium.org/1384213002/diff/220001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:61: // If you add elements from this enum, make sure you update the enum On 2015/10/16 16:48:54, Alexei Svitkine wrote: > Nit: "from" -> "to" Done. https://codereview.chromium.org/1384213002/diff/220001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.h:89: // reflect actual errors that occur during a page load. On 2015/10/16 16:48:53, Alexei Svitkine wrote: > Nit: Add the same comment about UMA histograms.xml here. Done. https://codereview.chromium.org/1384213002/diff/220001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc (right): https://codereview.chromium.org/1384213002/diff/220001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc:65: num_errors_ = 0; On 2015/10/16 16:48:54, Alexei Svitkine wrote: > Nit: Init these in the ctor instead. > > The ctor will be re-run for each test, so there shouldn't be any behavior > difference and this will fix warnings (from some IDEs and lint tools) that > you're not initializing variables in the ctor. Great! Done. https://codereview.chromium.org/1384213002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1384213002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:69634: + label="Provisional load stopped by user with no error. Should not occur On 2015/10/16 16:48:54, Alexei Svitkine wrote: > Nit: Long description can go between the <int></int> tags. Suggest making the > label= description shorter. See AndroidSigninPromoAction for an example. Done.
lgtm
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmcquade@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1384213002/#ps240001 (title: "Alexei review: comments, histogram changes, unit test constructor")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1384213002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1384213002/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/7a32ec9ce6c2522c65b3e9be5ecad54b5285bb0a Cr-Commit-Position: refs/heads/master@{#354778} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
