Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1984173002:
Log First User Interaction in Page Load Metrics (Closed)
|
Created:
4 years, 7 months ago by mushan1 Modified:
4 years, 6 months ago Reviewers:
jochen (gone - plz use gerrit), Wez, jam, Charlie Harrison, Alexei Svitkine (slow), Bryan McQuade, tdresser CC:
asvitkine+watch_chromium.org, blundell+watchlist_chromium.org, chromium-reviews, csharrison+watch_chromium.org, droger+watchlist_chromium.org, loading-reviews+metrics_chromium.org, sdefresne+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLog First User Interaction in Page Load Metrics
Learn more at: https://docs.google.com/document/d/1-OEDCDZPjWQHpmuGzaNxz9cRH4bEQXCo4x04ydazYtQ/edit
BUG=612422
Committed: https://crrev.com/48cac60dbb50219805cc59e04ad50022c0288bc7
Cr-Commit-Position: refs/heads/master@{#398128}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Plumbing RWH latency tracker instead of DidGetUserInteraction API #
Total comments: 15
Patch Set 3 : listen to raw input events in RenderWidgetHost #Patch Set 4 : Only record user interaction after first paint #
Total comments: 4
Patch Set 5 : Remove style change by formatter #
Total comments: 8
Patch Set 6 : Get navigation_start in OnCommit callback in FromGWSPLMO #
Total comments: 15
Patch Set 7 : Add comments for the 1000ms threshold #Patch Set 8 : Logic refinements #
Total comments: 2
Patch Set 9 : fix deps #Patch Set 10 : fix blimp/engine/feature/engine_render_widget_feature_unittest.cc #Patch Set 11 : fix unittest error because of register / unregister observers #Patch Set 12 : Add tests for from_gws PLMO #
Total comments: 12
Patch Set 13 : Change per Bryan's comment #Patch Set 14 : add histograms.xml #Patch Set 15 : Add asserts to ensure correct behavior of first user interaction time #Patch Set 16 : Add hacks to work around when time_to_abort is 0 #
Total comments: 15
Patch Set 17 : update fragments #Messages
Total messages: 78 (20 generated)
mushan@google.com changed reviewers: + asvitkine@google.com, csharrison@chromium.org, tdresser@chromium.org
I'm drafting a change to log aborts before user interaction in page load metrics. Please take a look. Thanks.
mushan@google.com changed reviewers: + bmcquade@chromium.org
https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... File components/page_load_metrics/browser/page_load_metrics_observer.h (right): https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... components/page_load_metrics/browser/page_load_metrics_observer.h:175: // web_contents It worries me a bit that these comments don't make it clear what kinds of input are being monitored. For example, most touch based games will _never_ trigger this. If touch events are preventDefaulted, they won't result in a user interaction. See rbyers.net/paint.html for an example of a page that you can interact with (drawing with touch), without causing this to register at all. https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... components/page_load_metrics/browser/page_load_metrics_observer.h:175: // web_contents nit:missing period.
On 2016/05/17 at 13:24:53, tdresser wrote: > https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... > File components/page_load_metrics/browser/page_load_metrics_observer.h (right): > > https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... > components/page_load_metrics/browser/page_load_metrics_observer.h:175: // web_contents > It worries me a bit that these comments don't make it clear what kinds of input are being monitored. > > For example, most touch based games will _never_ trigger this. If touch events are preventDefaulted, they won't result in a user interaction. > > See rbyers.net/paint.html for an example of a page that you can interact with (drawing with touch), without causing this to register at all. > > https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... > components/page_load_metrics/browser/page_load_metrics_observer.h:175: // web_contents > nit:missing period. I'll update the comments the record this in the design doc. Thanks for coming up with it!
On 2016/05/17 at 13:24:53, tdresser wrote: > https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... > File components/page_load_metrics/browser/page_load_metrics_observer.h (right): > > https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... > components/page_load_metrics/browser/page_load_metrics_observer.h:175: // web_contents > It worries me a bit that these comments don't make it clear what kinds of input are being monitored. > > For example, most touch based games will _never_ trigger this. If touch events are preventDefaulted, they won't result in a user interaction. > > See rbyers.net/paint.html for an example of a page that you can interact with (drawing with touch), without causing this to register at all. > > https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... > components/page_load_metrics/browser/page_load_metrics_observer.h:175: // web_contents > nit:missing period. I'll update the comments the record this in the design doc. Thanks for coming up with it!
Description was changed from ========== Log aborts before user interaction in page load metrics BUG=612422 ========== to ========== Log First User Interaction in Page Load Metrics Learn more at: https://docs.google.com/document/d/1-OEDCDZPjWQHpmuGzaNxz9cRH4bEQXCo4x04ydazY... BUG=612422 ==========
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
lgtm https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... components/page_load_metrics/browser/metrics_web_contents_observer.cc:370: if (user_interaction_time_.is_null()) { Nit: No {}'s https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... components/page_load_metrics/browser/metrics_web_contents_observer.cc:677: } else { Nit: Reverse the cond and turn the if else into just an if?
https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... components/page_load_metrics/browser/metrics_web_contents_observer.cc:676: if (type != blink::WebInputEvent::Type::Undefined) { based on the comment, do you want to ignore if type == undefined? let's not keep the empty if followed by else and just handle the one case we are adding code for https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... components/page_load_metrics/browser/metrics_web_contents_observer.h:95: ERR_USER_INTERACTION_WITH_NO_RELEVANT_LOAD this needs to be before ERR_LAST_ENTRY https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... File components/page_load_metrics/browser/page_load_metrics_observer.h (right): https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... components/page_load_metrics/browser/page_load_metrics_observer.h:74: const base::TimeDelta first_user_interaction_time; i'm actually not sure if this is a generally useful bit of information we want to be providing in this struct. in particular, if the first interaction happened before anything was painted (while the screen is blank), is that likely to be useful to most consumers? https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... components/page_load_metrics/browser/page_load_metrics_observer.h:175: // web_contents On 2016/05/17 at 13:24:53, tdresser wrote: > It worries me a bit that these comments don't make it clear what kinds of input are being monitored. > > For example, most touch based games will _never_ trigger this. If touch events are preventDefaulted, they won't result in a user interaction. > > See rbyers.net/paint.html for an example of a page that you can interact with (drawing with touch), without causing this to register at all. yeah, i agree - it seems the WCO hook we are overriding to get this signal isn't necessarily well understood or well supported, which is worrying. tim, is there a better canonical source of input events that we should be wiring through WebContentsObservers to replace the hook we are using currently? I'd like to make sure we're using a data source that input team feels is giving good data.
On 2016/05/18 15:21:50, Bryan McQuade wrote: > https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > (right): > > https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... > components/page_load_metrics/browser/metrics_web_contents_observer.cc:676: if > (type != blink::WebInputEvent::Type::Undefined) { > based on the comment, do you want to ignore if type == undefined? > > let's not keep the empty if followed by else and just handle the one case we are > adding code for > > https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... > File components/page_load_metrics/browser/metrics_web_contents_observer.h > (right): > > https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... > components/page_load_metrics/browser/metrics_web_contents_observer.h:95: > ERR_USER_INTERACTION_WITH_NO_RELEVANT_LOAD > this needs to be before ERR_LAST_ENTRY > > https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... > File components/page_load_metrics/browser/page_load_metrics_observer.h (right): > > https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... > components/page_load_metrics/browser/page_load_metrics_observer.h:74: const > base::TimeDelta first_user_interaction_time; > i'm actually not sure if this is a generally useful bit of information we want > to be providing in this struct. in particular, if the first interaction happened > before anything was painted (while the screen is blank), is that likely to be > useful to most consumers? > > https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... > components/page_load_metrics/browser/page_load_metrics_observer.h:175: // > web_contents > On 2016/05/17 at 13:24:53, tdresser wrote: > > It worries me a bit that these comments don't make it clear what kinds of > input are being monitored. > > > > For example, most touch based games will _never_ trigger this. If touch events > are preventDefaulted, they won't result in a user interaction. > > > > See rbyers.net/paint.html for an example of a page that you can interact with > (drawing with touch), without causing this to register at all. > > yeah, i agree - it seems the WCO hook we are overriding to get this signal isn't > necessarily well understood or well supported, which is worrying. tim, is there > a better canonical source of input events that we should be wiring through > WebContentsObservers to replace the hook we are using currently? I'd like to > make sure we're using a data source that input team feels is giving good data. Yeah, sorry I wasn't clearer about this earlier. I was confusing DidGetUserInteraction with the UserGestureIndicator (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). The UserGestureIndicator is used throughout blink to determine if the user has recently interacted with the page, but I don't think it's been plumbed out of blink. I'm not aware of any existing clean API to observe the event stream. Ideally we'd plumb something through from https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re..., or somewhere around there. I haven't thought too deeply about what's going to be the easiest place to plumb this from though. You could also plumb each event type separately from the input router. https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
On 2016/05/18 at 15:41:21, tdresser wrote: > On 2016/05/18 15:21:50, Bryan McQuade wrote: > > https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... > > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > > (right): > > > > https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... > > components/page_load_metrics/browser/metrics_web_contents_observer.cc:676: if > > (type != blink::WebInputEvent::Type::Undefined) { > > based on the comment, do you want to ignore if type == undefined? > > > > let's not keep the empty if followed by else and just handle the one case we are > > adding code for > > > > https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... > > File components/page_load_metrics/browser/metrics_web_contents_observer.h > > (right): > > > > https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... > > components/page_load_metrics/browser/metrics_web_contents_observer.h:95: > > ERR_USER_INTERACTION_WITH_NO_RELEVANT_LOAD > > this needs to be before ERR_LAST_ENTRY > > > > https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... > > File components/page_load_metrics/browser/page_load_metrics_observer.h (right): > > > > https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... > > components/page_load_metrics/browser/page_load_metrics_observer.h:74: const > > base::TimeDelta first_user_interaction_time; > > i'm actually not sure if this is a generally useful bit of information we want > > to be providing in this struct. in particular, if the first interaction happened > > before anything was painted (while the screen is blank), is that likely to be > > useful to most consumers? > > > > https://codereview.chromium.org/1984173002/diff/1/components/page_load_metric... > > components/page_load_metrics/browser/page_load_metrics_observer.h:175: // > > web_contents > > On 2016/05/17 at 13:24:53, tdresser wrote: > > > It worries me a bit that these comments don't make it clear what kinds of > > input are being monitored. > > > > > > For example, most touch based games will _never_ trigger this. If touch events > > are preventDefaulted, they won't result in a user interaction. > > > > > > See rbyers.net/paint.html for an example of a page that you can interact with > > (drawing with touch), without causing this to register at all. > > > > yeah, i agree - it seems the WCO hook we are overriding to get this signal isn't > > necessarily well understood or well supported, which is worrying. tim, is there > > a better canonical source of input events that we should be wiring through > > WebContentsObservers to replace the hook we are using currently? I'd like to > > make sure we're using a data source that input team feels is giving good data. > > Yeah, sorry I wasn't clearer about this earlier. I was confusing DidGetUserInteraction with the UserGestureIndicator (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > > The UserGestureIndicator is used throughout blink to determine if the user has recently interacted with the page, but I don't think it's been plumbed out of blink. > > I'm not aware of any existing clean API to observe the event stream. Ideally we'd plumb something through from https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re..., or somewhere around there. I haven't thought too deeply about what's going to be the easiest place to plumb this from though. > > You could also plumb each event type separately from the input router. > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... I tried to plumb RWH latency tracker in the new patchset. Please take a look.
General approach seems reasonable. Can you add some tests with HistogramTester? https://code.google.com/p/chromium/codesearch#chromium/src/base/test/histogra... https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:686: // ignore browser navigation or reload which comes with type Undefined Capitalization and end with . https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:274: // this navigation, to help instantiate the new PageLoadTracker. Awkward whitespace. https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:300: // vanish before we get signal about what caused the abort (new navigation, Awkward whitespace. https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... File components/page_load_metrics/browser/page_load_metrics_observer.h (right): https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... components/page_load_metrics/browser/page_load_metrics_observer.h:175: // web_contents Missing . https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.h:785: std::unique_ptr<RenderWidgetHostLatencyTracker> latency_tracker_; Why change this to unique_ptr?
On 2016/05/24 at 14:28:38, tdresser wrote: > General approach seems reasonable. > > Can you add some tests with HistogramTester? > > https://code.google.com/p/chromium/codesearch#chromium/src/base/test/histogra... > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > components/page_load_metrics/browser/metrics_web_contents_observer.cc:686: // ignore browser navigation or reload which comes with type Undefined > Capitalization and end with . > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > components/page_load_metrics/browser/metrics_web_contents_observer.h:274: // this navigation, to help instantiate the new PageLoadTracker. > Awkward whitespace. > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > components/page_load_metrics/browser/metrics_web_contents_observer.h:300: // vanish before we get signal about what caused the abort (new navigation, > Awkward whitespace. > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > File components/page_load_metrics/browser/page_load_metrics_observer.h (right): > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > components/page_load_metrics/browser/page_load_metrics_observer.h:175: // web_contents > Missing . > > https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_impl.h (right): > > https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_impl.h:785: std::unique_ptr<RenderWidgetHostLatencyTracker> latency_tracker_; > Why change this to unique_ptr? Got a build error if latency_tracker() returns the raw instance of RenderWidgetHostLatencyTracker: In file included from ../../content/browser/renderer_host/render_view_host_impl.h:23: ../../content/browser/renderer_host/render_widget_host_impl.h:470:12: error: calling a private constructor of class 'conte nt::RenderWidgetHostLatencyTracker' return latency_tracker_; ^ ../../content/browser/renderer_host/input/render_widget_host_latency_tracker.h:76:28: note: declared private here DISALLOW_COPY_AND_ASSIGN(RenderWidgetHostLatencyTracker);
On 2016/05/25 04:09:00, mushan wrote: > On 2016/05/24 at 14:28:38, tdresser wrote: > > General approach seems reasonable. > > > > Can you add some tests with HistogramTester? > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/test/histogra... > > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > (right): > > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > components/page_load_metrics/browser/metrics_web_contents_observer.cc:686: // > ignore browser navigation or reload which comes with type Undefined > > Capitalization and end with . > > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > File components/page_load_metrics/browser/metrics_web_contents_observer.h > (right): > > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > components/page_load_metrics/browser/metrics_web_contents_observer.h:274: // > this navigation, to help instantiate the new PageLoadTracker. > > Awkward whitespace. > > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > components/page_load_metrics/browser/metrics_web_contents_observer.h:300: // > vanish before we get signal about what caused the abort (new navigation, > > Awkward whitespace. > > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > File components/page_load_metrics/browser/page_load_metrics_observer.h > (right): > > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > components/page_load_metrics/browser/page_load_metrics_observer.h:175: // > web_contents > > Missing . > > > > > https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... > > File content/browser/renderer_host/render_widget_host_impl.h (right): > > > > > https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... > > content/browser/renderer_host/render_widget_host_impl.h:785: > std::unique_ptr<RenderWidgetHostLatencyTracker> latency_tracker_; > > Why change this to unique_ptr? > > Got a build error if latency_tracker() returns the raw instance of > RenderWidgetHostLatencyTracker: > > In file included from > ../../content/browser/renderer_host/render_view_host_impl.h:23: > ../../content/browser/renderer_host/render_widget_host_impl.h:470:12: error: > calling a private constructor of class 'conte > nt::RenderWidgetHostLatencyTracker' > return latency_tracker_; > ^ > ../../content/browser/renderer_host/input/render_widget_host_latency_tracker.h:76:28: > note: declared private here > DISALLOW_COPY_AND_ASSIGN(RenderWidgetHostLatencyTracker); You can return the object's address without making it a unique_ptr.
Thanks! Will review more soon but wanted to send one specific bit of feedback. https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:544: web_contents()->GetRenderViewHost()->GetWidget()); My understanding is that a RenderViewHost's lifetime may be shorter than a WebContents, so if the RVH associated with the web contents changes, and the RenderWidgetHost is bound to the RVH and not the WebContents, we'll stop getting events once the RVH changes. That said, my understanding of these relationships is not great, so I could very well be mistaken. My understanding is that an RVH is bound to a render process, which can change for a given WebContents if e.g. the user navigates between origins in the same tab (since we use different render processes for different origins). Anyway, it is worth digging into this to make sure this either is or is not an issue for us. Can you dig in further and see what you find? Or if another reviewer knows for sure, please reply & let us know.
https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:534: content::RenderWidgetHostImpl* rwhi = content::RenderWidgetHostImpl::From( since we're unlistening in the destructor, let's listen in the construct rather than in the CreateForWebContents method. We call the constructo in CreateForWebContents and in the unit test, so in the latter case, if we keep this code here, we'll attempt to unlisten in the destructor without having ever attached ourselves as a listener. https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:544: web_contents()->GetRenderViewHost()->GetWidget()); On 2016/05/25 at 19:02:47, Bryan McQuade wrote: > My understanding is that a RenderViewHost's lifetime may be shorter than a WebContents, so if the RVH associated with the web contents changes, and the RenderWidgetHost is bound to the RVH and not the WebContents, we'll stop getting events once the RVH changes. > > That said, my understanding of these relationships is not great, so I could very well be mistaken. My understanding is that an RVH is bound to a render process, which can change for a given WebContents if e.g. the user navigates between origins in the same tab (since we use different render processes for different origins). > > Anyway, it is worth digging into this to make sure this either is or is not an issue for us. Can you dig in further and see what you find? Or if another reviewer knows for sure, please reply & let us know. I think you can address this issue by implementing RenderViewHostChanged: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...
https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... File components/page_load_metrics/browser/page_load_metrics_observer.h (right): https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... components/page_load_metrics/browser/page_load_metrics_observer.h:74: const base::TimeDelta first_user_interaction_time; just copying an earlier comment i made forward since i didn't see it addressed (sorry if i missed any comments on it): i'm actually not sure if this is a generally useful bit of information we want to be providing in this struct. in particular, if the first interaction happened before anything was painted (while the screen is blank), is that likely to be useful to most consumers? perhaps all consumers should instead implement the OnUserInput callback to store just the times they are interested in, and we get rid of this TimeDelta in this struct? If we find that multiple observers are recording the same timestamp (e.g. first interaction after contentful paint) we can then consider adding it to this struct and refactoring at that time.
https://codereview.chromium.org/1984173002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1984173002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:39: const char kHistogramAbortForwardBackBeforeInteraction[] = we have a lot of AbortTiming metrics, and i'm not totally sure yet if this one is going to be useful, so while i'm fine with your additions to the fromgws code, let's leave this class alone for now. if the fromgws histograms start looking useful, we can consider adding equivalent metrics here. in particular i'm concerned about adding aborts before events that may never happen on some kinds of pages. i want to take a bit more time before deciding these are metrics we want in PageLoad.AbortTiming. https://codereview.chromium.org/1984173002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1984173002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:460: extra_info.first_user_interaction_time >= time_to_abort)) { if there first user interaction time is before paint, then there are no interactions after paint, I don't think we'll end up logging, even though your metric name "AfterPaint.BeforeInteraction" is satisfied (we aborted after paint but before any interactions after paint). do you instead want to record the time of the first interaction after paint? see related comment - i think we should record time of first interaction after paint in only this observer implementation for now. you'll need to set a boolean member to true in the OnFirstPaint callback then record the first interaction time when that boolean is true in the new OnInputEvent callback. https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:19: #include "content/browser/renderer_host/render_view_host_impl.h" My understanding is that you can't depend on content/browser from anywhere outside of content. See this code search: https://code.google.com/p/chromium/codesearch#search/&q=include%5C%20%5C%22co... There are a few exceptions but for the most part only other content code is allowed to depend on content/browser. In your case, I believe this means you'll need to add virtual AddInputEventCallback/RemoveInputEventCallback methods to RenderWidgetHost, which are then implemented in RenderWidgetHostImpl. https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:206: base::TimeTicks user_interaction_time_; let's remove this (see related comment with more detail) https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.h (right): https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.h:74: std::vector<InputEventCallback> input_event_callbacks_; can you use base::ObserverList here? the comments note that it's safer to use than vector<> since clients can safely remove themselves as observers during iteration, whereas that doesn't work correctly with vector<>s. https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.h:298: void ForwardTouchEventWithLatencyInfo(const blink::WebTouchEvent& touch_event, can you run git cl format in your client just to be sure that all of the style changes you made for this file are consistent with what the formatter expects? if git cl format makes any changes (including reverting to the file's previous style for these methods), you should keep whatever style changes it makes.
On 2016/05/25 at 12:39:43, tdresser wrote: > On 2016/05/25 04:09:00, mushan wrote: > > On 2016/05/24 at 14:28:38, tdresser wrote: > > > General approach seems reasonable. > > > > > > Can you add some tests with HistogramTester? > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/test/histogra... > > > > > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > > (right): > > > > > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > > components/page_load_metrics/browser/metrics_web_contents_observer.cc:686: // > > ignore browser navigation or reload which comes with type Undefined > > > Capitalization and end with . > > > > > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > > File components/page_load_metrics/browser/metrics_web_contents_observer.h > > (right): > > > > > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > > components/page_load_metrics/browser/metrics_web_contents_observer.h:274: // > > this navigation, to help instantiate the new PageLoadTracker. > > > Awkward whitespace. > > > > > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > > components/page_load_metrics/browser/metrics_web_contents_observer.h:300: // > > vanish before we get signal about what caused the abort (new navigation, > > > Awkward whitespace. > > > > > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > > File components/page_load_metrics/browser/page_load_metrics_observer.h > > (right): > > > > > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > > components/page_load_metrics/browser/page_load_metrics_observer.h:175: // > > web_contents > > > Missing . > > > > > > > > https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... > > > File content/browser/renderer_host/render_widget_host_impl.h (right): > > > > > > > > https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... > > > content/browser/renderer_host/render_widget_host_impl.h:785: > > std::unique_ptr<RenderWidgetHostLatencyTracker> latency_tracker_; > > > Why change this to unique_ptr? > > > > Got a build error if latency_tracker() returns the raw instance of > > RenderWidgetHostLatencyTracker: > > > > In file included from > > ../../content/browser/renderer_host/render_view_host_impl.h:23: > > ../../content/browser/renderer_host/render_widget_host_impl.h:470:12: error: > > calling a private constructor of class 'conte > > nt::RenderWidgetHostLatencyTracker' > > return latency_tracker_; > > ^ > > ../../content/browser/renderer_host/input/render_widget_host_latency_tracker.h:76:28: > > note: declared private here > > DISALLOW_COPY_AND_ASSIGN(RenderWidgetHostLatencyTracker); > > You can return the object's address without making it a unique_ptr. Done.
On 2016/05/25 at 20:47:04, bmcquade wrote: > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > components/page_load_metrics/browser/metrics_web_contents_observer.cc:534: content::RenderWidgetHostImpl* rwhi = content::RenderWidgetHostImpl::From( > since we're unlistening in the destructor, let's listen in the construct rather than in the CreateForWebContents method. We call the constructo in CreateForWebContents and in the unit test, so in the latter case, if we keep this code here, we'll attempt to unlisten in the destructor without having ever attached ourselves as a listener. Done. > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > components/page_load_metrics/browser/metrics_web_contents_observer.cc:544: web_contents()->GetRenderViewHost()->GetWidget()); > On 2016/05/25 at 19:02:47, Bryan McQuade wrote: > > My understanding is that a RenderViewHost's lifetime may be shorter than a WebContents, so if the RVH associated with the web contents changes, and the RenderWidgetHost is bound to the RVH and not the WebContents, we'll stop getting events once the RVH changes. > > > > That said, my understanding of these relationships is not great, so I could very well be mistaken. My understanding is that an RVH is bound to a render process, which can change for a given WebContents if e.g. the user navigates between origins in the same tab (since we use different render processes for different origins). > > > > Anyway, it is worth digging into this to make sure this either is or is not an issue for us. Can you dig in further and see what you find? Or if another reviewer knows for sure, please reply & let us know. > > I think you can address this issue by implementing RenderViewHostChanged: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... Done.
On 2016/05/25 at 20:59:23, bmcquade wrote: > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > File components/page_load_metrics/browser/page_load_metrics_observer.h (right): > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > components/page_load_metrics/browser/page_load_metrics_observer.h:74: const base::TimeDelta first_user_interaction_time; > just copying an earlier comment i made forward since i didn't see it addressed (sorry if i missed any comments on it): > > i'm actually not sure if this is a generally useful bit of information we want to be providing in this struct. in particular, if the first interaction happened before anything was painted (while the screen is blank), is that likely to be useful to most consumers? > > perhaps all consumers should instead implement the OnUserInput callback to store just the times they are interested in, and we get rid of this TimeDelta in this struct? If we find that multiple observers are recording the same timestamp (e.g. first interaction after contentful paint) we can then consider adding it to this struct and refactoring at that time. Done.
On 2016/05/25 at 21:32:30, bmcquade wrote: > https://codereview.chromium.org/1984173002/diff/20001/chrome/browser/page_loa... > File chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc (right): > > https://codereview.chromium.org/1984173002/diff/20001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:39: const char kHistogramAbortForwardBackBeforeInteraction[] = > we have a lot of AbortTiming metrics, and i'm not totally sure yet if this one is going to be useful, so while i'm fine with your additions to the fromgws code, let's leave this class alone for now. if the fromgws histograms start looking useful, we can consider adding equivalent metrics here. > > in particular i'm concerned about adding aborts before events that may never happen on some kinds of pages. i want to take a bit more time before deciding these are metrics we want in PageLoad.AbortTiming. Understand. I removed relative codes in this CL. > https://codereview.chromium.org/1984173002/diff/20001/chrome/browser/page_loa... > File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc (right): > > https://codereview.chromium.org/1984173002/diff/20001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:460: extra_info.first_user_interaction_time >= time_to_abort)) { > if there first user interaction time is before paint, then there are no interactions after paint, I don't think we'll end up logging, even though your metric name "AfterPaint.BeforeInteraction" is satisfied (we aborted after paint but before any interactions after paint). do you instead want to record the time of the first interaction after paint? see related comment - i think we should record time of first interaction after paint in only this observer implementation for now. you'll need to set a boolean member to true in the OnFirstPaint callback then record the first interaction time when that boolean is true in the new OnInputEvent callback. I'll follow up this soon in next patch. > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > components/page_load_metrics/browser/metrics_web_contents_observer.cc:19: #include "content/browser/renderer_host/render_view_host_impl.h" > My understanding is that you can't depend on content/browser from anywhere outside of content. See this code search: > https://code.google.com/p/chromium/codesearch#search/&q=include%5C%20%5C%22co... > > There are a few exceptions but for the most part only other content code is allowed to depend on content/browser. > > In your case, I believe this means you'll need to add virtual AddInputEventCallback/RemoveInputEventCallback methods to RenderWidgetHost, which are then implemented in RenderWidgetHostImpl. Done. Since we're actually plumbing to RenderWidgetHost now, I pulled out the callback trigger from latency_tracker. @Tim please let me know if you have any concern. > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > components/page_load_metrics/browser/metrics_web_contents_observer.h:206: base::TimeTicks user_interaction_time_; > let's remove this (see related comment with more detail) Done. > https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... > File content/browser/renderer_host/input/render_widget_host_latency_tracker.h (right): > > https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/render_widget_host_latency_tracker.h:74: std::vector<InputEventCallback> input_event_callbacks_; > can you use base::ObserverList here? the comments note that it's safer to use than vector<> since clients can safely remove themselves as observers during iteration, whereas that doesn't work correctly with vector<>s. Done. I made an RenderWidgetHost::InputEventObserver and let MWCO inherit it. > https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_impl.h (right): > > https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_impl.h:298: void ForwardTouchEventWithLatencyInfo(const blink::WebTouchEvent& touch_event, > can you run git cl format in your client just to be sure that all of the style changes you made for this file are consistent with what the formatter expects? if git cl format makes any changes (including reverting to the file's previous style for these methods), you should keep whatever style changes it makes. Done. Sorry for format errors in last patch.
On 2016/05/26 21:47:21, mushan wrote: > On 2016/05/25 at 21:32:30, bmcquade wrote: > > > https://codereview.chromium.org/1984173002/diff/20001/chrome/browser/page_loa... > > File > chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc > (right): > > > > > https://codereview.chromium.org/1984173002/diff/20001/chrome/browser/page_loa... > > > chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:39: > const char kHistogramAbortForwardBackBeforeInteraction[] = > > we have a lot of AbortTiming metrics, and i'm not totally sure yet if this one > is going to be useful, so while i'm fine with your additions to the fromgws > code, let's leave this class alone for now. if the fromgws histograms start > looking useful, we can consider adding equivalent metrics here. > > > > in particular i'm concerned about adding aborts before events that may never > happen on some kinds of pages. i want to take a bit more time before deciding > these are metrics we want in PageLoad.AbortTiming. > > Understand. I removed relative codes in this CL. > > > > https://codereview.chromium.org/1984173002/diff/20001/chrome/browser/page_loa... > > File > chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc > (right): > > > > > https://codereview.chromium.org/1984173002/diff/20001/chrome/browser/page_loa... > > > chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:460: > extra_info.first_user_interaction_time >= time_to_abort)) { > > if there first user interaction time is before paint, then there are no > interactions after paint, I don't think we'll end up logging, even though your > metric name "AfterPaint.BeforeInteraction" is satisfied (we aborted after paint > but before any interactions after paint). do you instead want to record the time > of the first interaction after paint? see related comment - i think we should > record time of first interaction after paint in only this observer > implementation for now. you'll need to set a boolean member to true in the > OnFirstPaint callback then record the first interaction time when that boolean > is true in the new OnInputEvent callback. > > I'll follow up this soon in next patch. > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > (right): > > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > components/page_load_metrics/browser/metrics_web_contents_observer.cc:19: > #include "content/browser/renderer_host/render_view_host_impl.h" > > My understanding is that you can't depend on content/browser from anywhere > outside of content. See this code search: > > > https://code.google.com/p/chromium/codesearch#search/&q=include%5C%20%5C%22co... > > > > There are a few exceptions but for the most part only other content code is > allowed to depend on content/browser. > > > > In your case, I believe this means you'll need to add virtual > AddInputEventCallback/RemoveInputEventCallback methods to RenderWidgetHost, > which are then implemented in RenderWidgetHostImpl. > > Done. Since we're actually plumbing to RenderWidgetHost now, I pulled out the > callback trigger from latency_tracker. @Tim please let me know if you have any > concern. > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > File components/page_load_metrics/browser/metrics_web_contents_observer.h > (right): > > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > components/page_load_metrics/browser/metrics_web_contents_observer.h:206: > base::TimeTicks user_interaction_time_; > > let's remove this (see related comment with more detail) > > Done. > > > > https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... > > File content/browser/renderer_host/input/render_widget_host_latency_tracker.h > (right): > > > > > https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... > > content/browser/renderer_host/input/render_widget_host_latency_tracker.h:74: > std::vector<InputEventCallback> input_event_callbacks_; > > can you use base::ObserverList here? the comments note that it's safer to use > than vector<> since clients can safely remove themselves as observers during > iteration, whereas that doesn't work correctly with vector<>s. > > Done. I made an RenderWidgetHost::InputEventObserver and let MWCO inherit it. > > > > https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... > > File content/browser/renderer_host/render_widget_host_impl.h (right): > > > > > https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... > > content/browser/renderer_host/render_widget_host_impl.h:298: void > ForwardTouchEventWithLatencyInfo(const blink::WebTouchEvent& touch_event, > > can you run git cl format in your client just to be sure that all of the > style changes you made for this file are consistent with what the formatter > expects? if git cl format makes any changes (including reverting to the file's > previous style for these methods), you should keep whatever style changes it > makes. > > Done. Sorry for format errors in last patch. The whitespace changes in these diffs make them hard to read. Can you leave the whitespace for code you aren't modifying untouched? If you want to submit a separate patch that just includes formatting tweaks, that would be fine.
On 2016/05/27 at 14:37:12, tdresser wrote: > On 2016/05/26 21:47:21, mushan wrote: > > On 2016/05/25 at 21:32:30, bmcquade wrote: > > > > > https://codereview.chromium.org/1984173002/diff/20001/chrome/browser/page_loa... > > > File > > chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc > > (right): > > > > > > > > https://codereview.chromium.org/1984173002/diff/20001/chrome/browser/page_loa... > > > > > chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:39: > > const char kHistogramAbortForwardBackBeforeInteraction[] = > > > we have a lot of AbortTiming metrics, and i'm not totally sure yet if this one > > is going to be useful, so while i'm fine with your additions to the fromgws > > code, let's leave this class alone for now. if the fromgws histograms start > > looking useful, we can consider adding equivalent metrics here. > > > > > > in particular i'm concerned about adding aborts before events that may never > > happen on some kinds of pages. i want to take a bit more time before deciding > > these are metrics we want in PageLoad.AbortTiming. > > > > Understand. I removed relative codes in this CL. > > > > > > > https://codereview.chromium.org/1984173002/diff/20001/chrome/browser/page_loa... > > > File > > chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc > > (right): > > > > > > > > https://codereview.chromium.org/1984173002/diff/20001/chrome/browser/page_loa... > > > > > chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:460: > > extra_info.first_user_interaction_time >= time_to_abort)) { > > > if there first user interaction time is before paint, then there are no > > interactions after paint, I don't think we'll end up logging, even though your > > metric name "AfterPaint.BeforeInteraction" is satisfied (we aborted after paint > > but before any interactions after paint). do you instead want to record the time > > of the first interaction after paint? see related comment - i think we should > > record time of first interaction after paint in only this observer > > implementation for now. you'll need to set a boolean member to true in the > > OnFirstPaint callback then record the first interaction time when that boolean > > is true in the new OnInputEvent callback. > > > > I'll follow up this soon in next patch. > > > > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > > (right): > > > > > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > > components/page_load_metrics/browser/metrics_web_contents_observer.cc:19: > > #include "content/browser/renderer_host/render_view_host_impl.h" > > > My understanding is that you can't depend on content/browser from anywhere > > outside of content. See this code search: > > > > > https://code.google.com/p/chromium/codesearch#search/&q=include%5C%20%5C%22co... > > > > > > There are a few exceptions but for the most part only other content code is > > allowed to depend on content/browser. > > > > > > In your case, I believe this means you'll need to add virtual > > AddInputEventCallback/RemoveInputEventCallback methods to RenderWidgetHost, > > which are then implemented in RenderWidgetHostImpl. > > > > Done. Since we're actually plumbing to RenderWidgetHost now, I pulled out the > > callback trigger from latency_tracker. @Tim please let me know if you have any > > concern. > > > > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > > File components/page_load_metrics/browser/metrics_web_contents_observer.h > > (right): > > > > > > > > https://codereview.chromium.org/1984173002/diff/20001/components/page_load_me... > > > components/page_load_metrics/browser/metrics_web_contents_observer.h:206: > > base::TimeTicks user_interaction_time_; > > > let's remove this (see related comment with more detail) > > > > Done. > > > > > > > https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... > > > File content/browser/renderer_host/input/render_widget_host_latency_tracker.h > > (right): > > > > > > > > https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... > > > content/browser/renderer_host/input/render_widget_host_latency_tracker.h:74: > > std::vector<InputEventCallback> input_event_callbacks_; > > > can you use base::ObserverList here? the comments note that it's safer to use > > than vector<> since clients can safely remove themselves as observers during > > iteration, whereas that doesn't work correctly with vector<>s. > > > > Done. I made an RenderWidgetHost::InputEventObserver and let MWCO inherit it. > > > > > > > https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... > > > File content/browser/renderer_host/render_widget_host_impl.h (right): > > > > > > > > https://codereview.chromium.org/1984173002/diff/20001/content/browser/rendere... > > > content/browser/renderer_host/render_widget_host_impl.h:298: void > > ForwardTouchEventWithLatencyInfo(const blink::WebTouchEvent& touch_event, > > > can you run git cl format in your client just to be sure that all of the > > style changes you made for this file are consistent with what the formatter > > expects? if git cl format makes any changes (including reverting to the file's > > previous style for these methods), you should keep whatever style changes it > > makes. > > > > Done. Sorry for format errors in last patch. > > The whitespace changes in these diffs make them hard to read. > Can you leave the whitespace for code you aren't modifying untouched? > > If you want to submit a separate patch that just includes formatting tweaks, that would be fine. Mushan, if the reformat is due to running git cl format as I suggested, I'm sorry - I didn't anticipate that this would end up changing existing code. I agree it's preferred not to mix formatting cleanup changes with implementation changes.
https://codereview.chromium.org/1984173002/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h (right): https://codereview.chromium.org/1984173002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h:112: base::TimeDelta first_user_interaction_time_; let's call this first_user_interaction_after_paint_ to be clearer https://codereview.chromium.org/1984173002/diff/60001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1984173002/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:380: observer->OnUserInput(event, navigation_start_ - base::TimeTicks::Now()); I'm inclined to not provide a timestamp in the callback, if it's really just the current time. Instead, you can compute this in our observer by storing the current TimeTicks and then taking a delta between that and navigation_start_ time in your observer. If we don't provide the TimeTicks for navigation_start time to observers currently, I'd be fine with doing so in PageLoadExtraInfo. Charles, wdyt? https://codereview.chromium.org/1984173002/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:521: this->RegisterInputEventObserver(web_contents->GetRenderViewHost()); you can exclude 'this->' when invoking member functions.
https://codereview.chromium.org/1984173002/diff/60001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1984173002/diff/60001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:380: observer->OnUserInput(event, navigation_start_ - base::TimeTicks::Now()); On 2016/05/27 at 15:03:31, Bryan McQuade wrote: > I'm inclined to not provide a timestamp in the callback, if it's really just the current time. Instead, you can compute this in our observer by storing the current TimeTicks and then taking a delta between that and navigation_start_ time in your observer. If we don't provide the TimeTicks for navigation_start time to observers currently, I'd be fine with doing so in PageLoadExtraInfo. Charles, wdyt? charles pointed out to me that you can call navigation_handle->GetNavigationStart() in your OnCommit function to get record this time. so no change needed to PageLoadExtraInfo.
Looks good, thanks for this addition. https://codereview.chromium.org/1984173002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1984173002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:477: skipped_user_interaction_time += Can you add a rationale for the magic number 1000? Maybe it would be a good idea to manually test the time it takes to complete a pull-to-refresh / forward-back action. https://codereview.chromium.org/1984173002/diff/80001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1984173002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:540: UnregisterInputEventObserver(web_contents()->GetRenderViewHost()); Hm this feels unsafe. I don't think we should be calling web_contents() in the destructor. I think the last possible place you can safely call this is in WebContentsDestroyed() or something. Can you verify?
On 2016/05/27 at 15:02:58, bmcquade wrote: > > Mushan, if the reformat is due to running git cl format as I suggested, I'm sorry - I didn't anticipate that this would end up changing existing code. I agree it's preferred not to mix formatting cleanup changes with implementation changes. Sorry it was because I invoked clang-format in my vim. I tried git cl format after revoked those changed. It works as expected. On 2016/05/27 at 15:03:31, bmcquade wrote: > https://codereview.chromium.org/1984173002/diff/60001/chrome/browser/page_loa... > File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h (right): > > https://codereview.chromium.org/1984173002/diff/60001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h:112: base::TimeDelta first_user_interaction_time_; > let's call this first_user_interaction_after_paint_ to be clearer Done. > https://codereview.chromium.org/1984173002/diff/60001/components/page_load_me... > File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): > > https://codereview.chromium.org/1984173002/diff/60001/components/page_load_me... > components/page_load_metrics/browser/metrics_web_contents_observer.cc:380: observer->OnUserInput(event, navigation_start_ - base::TimeTicks::Now()); > I'm inclined to not provide a timestamp in the callback, if it's really just the current time. Instead, you can compute this in our observer by storing the current TimeTicks and then taking a delta between that and navigation_start_ time in your observer. If we don't provide the TimeTicks for navigation_start time to observers currently, I'd be fine with doing so in PageLoadExtraInfo. Charles, wdyt? > > charles pointed out to me that you can call navigation_handle->GetNavigationStart() in your OnCommit function to get record this time. so no change needed to PageLoadExtraInfo. Done. > https://codereview.chromium.org/1984173002/diff/60001/components/page_load_me... > components/page_load_metrics/browser/metrics_web_contents_observer.cc:521: this->RegisterInputEventObserver(web_contents->GetRenderViewHost()); > you can exclude 'this->' when invoking member functions. Done.
The CQ bit was checked by mushan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1984173002/#ps100001 (title: "Get navigation_start in OnCommit callback in FromGWSPLMO")
The CQ bit was unchecked by mushan@google.com
https://codereview.chromium.org/1984173002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1984173002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:477: skipped_user_interaction_time += On 2016/05/27 at 18:14:34, csharrison wrote: > Can you add a rationale for the magic number 1000? Maybe it would be a good idea to manually test the time it takes to complete a pull-to-refresh / forward-back action. I use 1000 here because it takes definitely less than 1000ms to perform a pull-to-reload/forward_back. Also 1000ms is a very short time for a user to consume any content just revealed by his interaction. So it seems to be a good number as a baseline. https://codereview.chromium.org/1984173002/diff/80001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1984173002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:540: UnregisterInputEventObserver(web_contents()->GetRenderViewHost()); On 2016/05/27 at 18:14:34, csharrison wrote: > Hm this feels unsafe. I don't think we should be calling web_contents() in the destructor. I think the last possible place you can safely call this is in WebContentsDestroyed() or something. Can you verify? What do you think if we just remove this Unregister logic? Since if web_contents() are destroyed, there won't be further callbacks triggered. Could a page load metrics observer live longer than its web_contents?
https://codereview.chromium.org/1984173002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1984173002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:477: skipped_user_interaction_time += On 2016/05/27 at 18:51:24, mushan wrote: > On 2016/05/27 at 18:14:34, csharrison wrote: > > Can you add a rationale for the magic number 1000? Maybe it would be a good idea to manually test the time it takes to complete a pull-to-refresh / forward-back action. > > I use 1000 here because it takes definitely less than 1000ms to perform a pull-to-reload/forward_back. Also 1000ms is a very short time for a user to consume any content just revealed by his interaction. So it seems to be a good number as a baseline. Fair enough. It might be good to include some of this in the comment. Maybe append "1000ms is too short a time for a user to to consume any content revealed by the interaction." https://codereview.chromium.org/1984173002/diff/80001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1984173002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:540: UnregisterInputEventObserver(web_contents()->GetRenderViewHost()); On 2016/05/27 at 18:51:24, mushan wrote: > On 2016/05/27 at 18:14:34, csharrison wrote: > > Hm this feels unsafe. I don't think we should be calling web_contents() in the destructor. I think the last possible place you can safely call this is in WebContentsDestroyed() or something. Can you verify? > > What do you think if we just remove this Unregister logic? Since if web_contents() are destroyed, there won't be further callbacks triggered. > Could a page load metrics observer live longer than its web_contents? No, currently observers are scoped to the WebContentsObserver, which is scoped to the WebContents. I'm fine with removing the unregister logic if it's safe. Note that you'll need to be sure that the RenderWidgetHost does not outlive the WebContents. I'm not 100% sure of that.
Content still LGTM
https://codereview.chromium.org/1984173002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1984173002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:477: skipped_user_interaction_time += On 2016/05/27 at 18:58:24, csharrison wrote: > On 2016/05/27 at 18:51:24, mushan wrote: > > On 2016/05/27 at 18:14:34, csharrison wrote: > > > Can you add a rationale for the magic number 1000? Maybe it would be a good idea to manually test the time it takes to complete a pull-to-refresh / forward-back action. > > > > I use 1000 here because it takes definitely less than 1000ms to perform a pull-to-reload/forward_back. Also 1000ms is a very short time for a user to consume any content just revealed by his interaction. So it seems to be a good number as a baseline. > > Fair enough. It might be good to include some of this in the comment. Maybe append "1000ms is too short a time for a user to to consume any content revealed by the interaction." Done. https://codereview.chromium.org/1984173002/diff/80001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1984173002/diff/80001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:540: UnregisterInputEventObserver(web_contents()->GetRenderViewHost()); On 2016/05/27 at 18:58:24, csharrison wrote: > On 2016/05/27 at 18:51:24, mushan wrote: > > On 2016/05/27 at 18:14:34, csharrison wrote: > > > Hm this feels unsafe. I don't think we should be calling web_contents() in the destructor. I think the last possible place you can safely call this is in WebContentsDestroyed() or something. Can you verify? > > > > What do you think if we just remove this Unregister logic? Since if web_contents() are destroyed, there won't be further callbacks triggered. > > Could a page load metrics observer live longer than its web_contents? > > No, currently observers are scoped to the WebContentsObserver, which is scoped to the WebContents. I'm fine with removing the unregister logic if it's safe. Note that you'll need to be sure that the RenderWidgetHost does not outlive the WebContents. I'm not 100% sure of that. refer to https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... The case RenderWidgetHost outlives WebContents does not apply here (WebContents does not have its reference anyway). Removed the unregister logic.
Thanks LGTM but let's wait for Bryan's signoff.
Thanks! This is looking good. Let's add some tests in from_gws_..._unittest.cc to verify that your expected histograms get logged. you can add some code to simulate input events in PageLoadMetricsObserverTestHarness - perhaps a new method similar to void SimulateTimingUpdate(const PageLoadTiming& timing); something like void SimulateInputEvent(const WebInputEvent& event); https://codereview.chromium.org/1984173002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1984173002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:457: bool aborted_in_foreground = if i understand the previous code correctly, we actually never log aborts except if aborted_in_foreground is true. take a look and if that seems correct to you i think we can simplify like so: if (!WasAbortedInForeground(abort_type, time_to_abort, extra_info)) return; https://codereview.chromium.org/1984173002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:460: if (!extra_info.committed_url.is_empty()) { let's simplify a bit here as well: if (extra_info.committed_url.is_empty()) { LogProvisionalAborts(abort_type, time_to_abort); return; } // If we didn't receive any timing data but did commit, this is likely not a renderer-tracked navigation, so ignore it. if (timing.IsEmpty()) return; if (timing.first_paint.is_zero() || timing.first_paint >= time_to_abort) LogCommittedAbortsBeforePaint(abort_type, time_to_abort); else if (AbortedBeforeInteraction(abort_type)) LogAbortsAfterPaintBeforeInteraction(abort_type, time_to_abort); the AbortedBeforeInteraction() method should encapsulate the 1000ms logic. https://codereview.chromium.org/1984173002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:468: first_user_interaction_after_paint_.is_zero(); like for aborted_before_paint, it think we want to check that either first interaction after paint is zero, or it is greater than or equal to time_to_abort. https://codereview.chromium.org/1984173002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:629: if (first_paint_triggered_ && first_user_interaction_after_paint_.is_zero()) just to be thorough let's also add a check for && !navigation_start_.is_null() or if you think this should not be possible, inside the if you can DCHECK(!navigation_start_.is_null()); https://codereview.chromium.org/1984173002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h (right): https://codereview.chromium.org/1984173002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h:51: navigation_start_ = navigation_start; let's add DCHECK(navigation_start_.is_null()); at the beginning of this method, to verify that this only gets called once https://codereview.chromium.org/1984173002/diff/100001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1984173002/diff/100001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:34: class RenderViewHost; now that you include the header for this class you no longer need to forward declare it https://codereview.chromium.org/1984173002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1984173002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:1001: latency_tracker_.OnInputEvent(wheel_event, &wheel_with_latency.latency); can we add a helper method: void RenderWidgetHostImpl::DispatchInputEvent(const blink::WebInputEvent& event, ui::LatencyInfo* latency) { latency_tracker_.OnInputEvent(event, latency); FOR_EACH_OBSERVER(InputEventObserver, input_event_observers_, OnInputEvent(event)); } and invoke that where we currently call out to the latency_tracker_? That way we have slightly less repetitive code in each event method, removing the need to repeat both the call to the latency tracker and the InputEventObservers in each method. https://codereview.chromium.org/1984173002/diff/100001/content/public/browser... File content/public/browser/render_widget_host.h (right): https://codereview.chromium.org/1984173002/diff/100001/content/public/browser... content/public/browser/render_widget_host.h:252: virtual bool OnInputEvent(const blink::WebInputEvent&) = 0; does this need to return a boolean? it doesn't seem to be used currently. if we aren't doing anything with the return value then let's switch to void return type.
Thanks for the thorough review! I'll add cases in next patch. https://codereview.chromium.org/1984173002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1984173002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:457: bool aborted_in_foreground = On 2016/05/27 at 20:06:35, Bryan McQuade wrote: > if i understand the previous code correctly, we actually never log aborts except if aborted_in_foreground is true. take a look and if that seems correct to you i think we can simplify like so: > if (!WasAbortedInForeground(abort_type, time_to_abort, extra_info)) > return; Seems correct. Done. https://codereview.chromium.org/1984173002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:460: if (!extra_info.committed_url.is_empty()) { On 2016/05/27 at 20:06:35, Bryan McQuade wrote: > let's simplify a bit here as well: > > if (extra_info.committed_url.is_empty()) { > LogProvisionalAborts(abort_type, time_to_abort); > return; > } > > // If we didn't receive any timing data but did commit, this is likely not a renderer-tracked navigation, so ignore it. > if (timing.IsEmpty()) > return; > > if (timing.first_paint.is_zero() || timing.first_paint >= time_to_abort) > LogCommittedAbortsBeforePaint(abort_type, time_to_abort); > else if (AbortedBeforeInteraction(abort_type)) > LogAbortsAfterPaintBeforeInteraction(abort_type, time_to_abort); > > the AbortedBeforeInteraction() method should encapsulate the 1000ms logic. Done. It looks much more neat now. Thanks a lot! https://codereview.chromium.org/1984173002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:629: if (first_paint_triggered_ && first_user_interaction_after_paint_.is_zero()) On 2016/05/27 at 20:06:35, Bryan McQuade wrote: > just to be thorough let's also add a check for > && !navigation_start_.is_null() > > or if you think this should not be possible, inside the if you can DCHECK(!navigation_start_.is_null()); Added the DCHECK. https://codereview.chromium.org/1984173002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h (right): https://codereview.chromium.org/1984173002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h:51: navigation_start_ = navigation_start; On 2016/05/27 at 20:06:36, Bryan McQuade wrote: > let's add DCHECK(navigation_start_.is_null()); at the beginning of this method, to verify that this only gets called once Done. https://codereview.chromium.org/1984173002/diff/100001/components/page_load_m... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1984173002/diff/100001/components/page_load_m... components/page_load_metrics/browser/metrics_web_contents_observer.cc:34: class RenderViewHost; On 2016/05/27 at 20:06:36, Bryan McQuade wrote: > now that you include the header for this class you no longer need to forward declare it Removed. https://codereview.chromium.org/1984173002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1984173002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:1001: latency_tracker_.OnInputEvent(wheel_event, &wheel_with_latency.latency); On 2016/05/27 at 20:06:36, Bryan McQuade wrote: > can we add a helper method: > > void RenderWidgetHostImpl::DispatchInputEvent(const blink::WebInputEvent& event, ui::LatencyInfo* latency) { > latency_tracker_.OnInputEvent(event, latency); > FOR_EACH_OBSERVER(InputEventObserver, input_event_observers_, > OnInputEvent(event)); > } > > and invoke that where we currently call out to the latency_tracker_? > > That way we have slightly less repetitive code in each event method, removing the need to repeat both the call to the latency tracker and the InputEventObservers in each method. Done. https://codereview.chromium.org/1984173002/diff/100001/content/public/browser... File content/public/browser/render_widget_host.h (right): https://codereview.chromium.org/1984173002/diff/100001/content/public/browser... content/public/browser/render_widget_host.h:252: virtual bool OnInputEvent(const blink::WebInputEvent&) = 0; On 2016/05/27 at 20:06:36, Bryan McQuade wrote: > does this need to return a boolean? it doesn't seem to be used currently. if we aren't doing anything with the return value then let's switch to void return type. Done.
One more thing: some of the trybots are showing build failures. To fix, I think you'll need to update dependencies for targets depending on WebInputEvent to explicitly depend on //third_party/WebKit/public:blink_headers. I still don't fully understand layering and dependency expectations - I believe it's ok to depend on this target from components but I'm not 100% certain.
https://codereview.chromium.org/1984173002/diff/140001/components/page_load_m... File components/page_load_metrics/DEPS (right): https://codereview.chromium.org/1984173002/diff/140001/components/page_load_m... components/page_load_metrics/DEPS:11: "+third_party/WebKit/public/web/WebInputEvent.h", since we only depend on this in browser code, and I don't anticipate that changing, you can move this to components/page_load_metrics/browser/DEPS
mushan@google.com changed reviewers: - asvitkine@google.com
mushan@google.com changed reviewers: + jam@chromium.org
Added tests for FromGWSPLMO. PTAL https://codereview.chromium.org/1984173002/diff/140001/components/page_load_m... File components/page_load_metrics/DEPS (right): https://codereview.chromium.org/1984173002/diff/140001/components/page_load_m... components/page_load_metrics/DEPS:11: "+third_party/WebKit/public/web/WebInputEvent.h", On 2016/05/28 at 12:23:29, Bryan McQuade wrote: > since we only depend on this in browser code, and I don't anticipate that changing, you can move this to > components/page_load_metrics/browser/DEPS Done.
This change looks great, thanks! A couple small things then I think we are ready to commit. You'll need to add an owners reviewer for the blimp code as well. https://codereview.chromium.org/1984173002/diff/220001/blimp/engine/feature/e... File blimp/engine/feature/engine_render_widget_feature_unittest.cc (right): https://codereview.chromium.org/1984173002/diff/220001/blimp/engine/feature/e... blimp/engine/feature/engine_render_widget_feature_unittest.cc:99: void AddInputEventObserver(InputEventObserver* observer) override{}; formatting nit: add space between 'override' and '{}' https://codereview.chromium.org/1984173002/diff/220001/blimp/engine/feature/e... blimp/engine/feature/engine_render_widget_feature_unittest.cc:100: void RemoveInputEventObserver(InputEventObserver* observer) override{}; same https://codereview.chromium.org/1984173002/diff/220001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1984173002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:206: if (time_to_interaction.is_zero()) let's add: DCHECK(!time_to_abort.is_null()); at the beginning of this function https://codereview.chromium.org/1984173002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:217: // It's also too short a time for a user to consume any content i'm a little concerned about the fact that histograms with different names end up with subtly but possibly important differences in behavior. for example: PageLoad.Clients.FromGoogleSearch.AbortTiming.ForwardBackNavigation.AfterPaint.BeforeInteraction will only log when there's 1000ms passed, whereas PageLoad.Clients.FromGoogleSearch.AbortTiming.NewNavigation.AfterPaint.BeforeInteraction will log regardless of the delay. This makes comparing results in the two histograms a bit tricky, since they're really tracking quite different things. To be clear about this, I think we should give the histograms that incorporate the delay different names. Perhaps: PageLoad.Clients.FromGoogleSearch.AbortTiming.ForwardBackNavigation.AfterPaint.BeforeInteractionWith1sDelay Can we use this name or something similar? That way, the naming will make it really clear that these histograms are different. https://codereview.chromium.org/1984173002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:495: if (timing.first_paint.is_zero() || timing.first_paint >= time_to_abort) this code uses >= but your WasAbortedBeforeInteraction uses strictly greater than. Charles or Shivani can give guidance on which one is preferred, but we should be consistent. https://codereview.chromium.org/1984173002/diff/220001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/1984173002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc:28: void SimulateTimingWithFirstPaint() { nit: please add a newline between each function https://codereview.chromium.org/1984173002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc:35: void SimulateMouseEvent() { same https://codereview.chromium.org/1984173002/diff/220001/content/public/browser... File content/public/browser/render_widget_host.h (right): https://codereview.chromium.org/1984173002/diff/220001/content/public/browser... content/public/browser/render_widget_host.h:247: // Observer for all kinds of input events (but not input event acks). rather than 'for all kinds of input events' let's say 'for WebInputEvents'
This change looks great, thanks! A couple small things then I think we are ready to commit. You'll need to add an owners reviewer for the blimp code as well.
+jam for the blimp code review. The new cases passed in a few try targets but broke in some others. I'm still debugging it. https://codereview.chromium.org/1984173002/diff/220001/blimp/engine/feature/e... File blimp/engine/feature/engine_render_widget_feature_unittest.cc (right): https://codereview.chromium.org/1984173002/diff/220001/blimp/engine/feature/e... blimp/engine/feature/engine_render_widget_feature_unittest.cc:99: void AddInputEventObserver(InputEventObserver* observer) override{}; On 2016/05/31 at 20:33:28, Bryan McQuade wrote: > formatting nit: add space between 'override' and '{}' Changed. But weird that the format here is actually the result of git-cl format. https://codereview.chromium.org/1984173002/diff/220001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1984173002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:206: if (time_to_interaction.is_zero()) On 2016/05/31 at 20:33:29, Bryan McQuade wrote: > let's add: > DCHECK(!time_to_abort.is_null()); > at the beginning of this function Done. https://codereview.chromium.org/1984173002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:217: // It's also too short a time for a user to consume any content On 2016/05/31 at 20:33:29, Bryan McQuade wrote: > i'm a little concerned about the fact that histograms with different names end up with subtly but possibly important differences in behavior. > > for example: > PageLoad.Clients.FromGoogleSearch.AbortTiming.ForwardBackNavigation.AfterPaint.BeforeInteraction > will only log when there's 1000ms passed, whereas > PageLoad.Clients.FromGoogleSearch.AbortTiming.NewNavigation.AfterPaint.BeforeInteraction > will log regardless of the delay. This makes comparing results in the two histograms a bit tricky, since they're really tracking quite different things. > > To be clear about this, I think we should give the histograms that incorporate the delay different names. Perhaps: > PageLoad.Clients.FromGoogleSearch.AbortTiming.ForwardBackNavigation.AfterPaint.BeforeInteractionWith1sDelay > Can we use this name or something similar? That way, the naming will make it really clear that these histograms are different. Agree. I made it 'PageLoad.Clients.FromGoogleSearch.AbortTiming.ForwardBackNavigation.AfterPaint.Before1sDelayedInteraction' now. Also, I might misinterpret your words here. But to keep us in the same page, the logic here is, reload and forward_back will be logged EVEN before 1000ms passed of the first user interaction. Whereas other types of abort will only be logged exactly before the first user interaction. https://codereview.chromium.org/1984173002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:495: if (timing.first_paint.is_zero() || timing.first_paint >= time_to_abort) On 2016/05/31 at 20:33:29, Bryan McQuade wrote: > this code uses >= but your WasAbortedBeforeInteraction uses strictly greater than. > > Charles or Shivani can give guidance on which one is preferred, but we should be consistent. Change to >= for consistency.
bmcquade@chromium.org changed reviewers: + wez@chromium.org
bmcquade@chromium.org changed reviewers: - wez@chromium.org
Also adding wez@ as a blimp/ owner (I don't see jam in any blimp/... owners files).
Looks like some of your tests are failing: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
Mushan and I looked into the test failures on the bots. Mushan, I wasn't able to repro these failures in a debug build, but I'm able to repro them in release. If you switch to release builds, you should also be able to repro. Here's my gn configuration file for the release builds that allowed me to repro the failure - you may want to use the same: $ gn args out/rel # when editor pops up, change the file contents to: is_component_build = true enable_nacl = false symbol_level = 0 use_goma = true is_debug = false remove_webcore_debug_symbols = true # save file $ ninja -C out/rel unit_tests $ ./out/rel/unit_tests --gtest_filter='FromGWSPageLoadMetrics*' --single-process-tests That should repro the 3 failing tests. Let me know if this doesn't repro for you.
bmcquade@chromium.org changed reviewers: + wez@chromium.org
Actually adding wez for blimp review. Wez, this change shouldn't affect blimp functionality at all, but we had to update MockRenderWidgetHost in a blimp test as a result of adding new methods to RenderWidgetHost. Can you review the change to the blimp test and give lg if it seems ok to you?
On 2016/06/02 at 14:03:01, bmcquade wrote: > Actually adding wez for blimp review. > > Wez, this change shouldn't affect blimp functionality at all, but we had to update MockRenderWidgetHost in a blimp test as a result of adding new methods to RenderWidgetHost. Can you review the change to the blimp test and give lg if it seems ok to you? I debugged with Bryan and came to a conclusion. The fails are due to cases ran in release environment are too fast that time_to_abort logged in FromGWSObserver is set to 0. And 0 represents unset status for a TimeDelta. We added a few hacks to work around of it now. Once the change for Optional<TimeDelta> https://codereview.chromium.org/1837233002 is landed, we'll follow up and change these hacks back.
blimp/ LGTM
On 2016/06/02 at 19:47:52, wez wrote: > blimp/ LGTM Thank you for the quick review!
ok this is really close to done - there's some fragility that i want to think about, so i'lll try to follow up shortly, but we're basically ready to commit. https://codereview.chromium.org/1984173002/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h (right): https://codereview.chromium.org/1984173002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h:60: void SetFirstPaintTriggered(const bool first_paint_triggered) { let's add TODO(bmcquade): remove this method once we add Optional<TimeDelta> for first_paint, as it should no longer be needed. https://codereview.chromium.org/1984173002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h:147: FromGWSPageLoadMetricsLogger* GetLogger() { let's add TODO(bmcquade): remove this method once we add Optional<TimeDelta> for first_paint, as it should no longer be needed. https://codereview.chromium.org/1984173002/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/1984173002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc:40: logger_->SetFirstPaintTriggered(true); this is unfortunate but i'm not yet sure how to fix it. i'm concerned with having to force invocation of this method since it's not called in production code due to first_paint being zero. seems like we have a bug there and this is side stepping around it, so we're no longer really even testing production code flow. i will think about this a bit more & see what we can do. as you mention the Optional<> change will help but it's not landed yet and I am not sure we want to merge it to M52 so your change probably should not depend on it. https://codereview.chromium.org/1984173002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc:610: EXPECT_FALSE(logger_->HasUserInteractionAfterPaint()); Is it fair to say that the following check to verify the histogram is set also implicitly tests that there was no user interaction after paint? If so I'd rather that the observer tests just stick to verifying expected histogram effects rather than inspecting the internal state of the observer/loggger. None of the other observer tests reach into the state of the observers that I'm aware of, so I'd like to be consistent here if we can.
Ok, I'm going to try to fix up some of the tricky issues in a follow up change - let's go with what you've got in your change. Let's address these remaining comments then we are good to go, thanks! https://codereview.chromium.org/1984173002/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1984173002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:500: // TODO(bmcquade): change back to `else if` once Optional<TimeDelta> is landed let's update this to change back to else if once crbug.com/616901 is addressed https://codereview.chromium.org/1984173002/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h (right): https://codereview.chromium.org/1984173002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h:60: void SetFirstPaintTriggered(const bool first_paint_triggered) { On 2016/06/02 at 20:30:17, Bryan McQuade wrote: > let's add TODO(bmcquade): remove this method once we add Optional<TimeDelta> for first_paint, as it should no longer be needed. Actually, let's make it // TODO(bmcquade): remove SetFirstPaintTriggered as part of fixing crbug.com/616901 https://codereview.chromium.org/1984173002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h:113: bool HasUserInteractionAfterPaint() { related to comment below, let's remove this method. https://codereview.chromium.org/1984173002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.h:147: FromGWSPageLoadMetricsLogger* GetLogger() { On 2016/06/02 at 20:30:17, Bryan McQuade wrote: > let's add TODO(bmcquade): remove this method once we add Optional<TimeDelta> for first_paint, as it should no longer be needed. Same: // TODO(bmcquade): remove this as part of fixing crbug.com/616901 https://codereview.chromium.org/1984173002/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/1984173002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc:20: observer_ = new FromGWSPageLoadMetricsObserver(); we don't actually need observer_ beyond the scope of this function, so let's just take a local variable rather than keeping a member variable. https://codereview.chromium.org/1984173002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc:40: logger_->SetFirstPaintTriggered(true); On 2016/06/02 at 20:30:17, Bryan McQuade wrote: > this is unfortunate but i'm not yet sure how to fix it. i'm concerned with having to force invocation of this method since it's not called in production code due to first_paint being zero. seems like we have a bug there and this is side stepping around it, so we're no longer really even testing production code flow. i will think about this a bit more & see what we can do. as you mention the Optional<> change will help but it's not landed yet and I am not sure we want to merge it to M52 so your change probably should not depend on it. Looking at this more, given that these tests stub out PageLoadTiming timings, it seems like they should also allow for providing fake timings for the timing data recorded by calls to TimeTicks::Now(). But I don't want to block your change on yet more changes, especially given the desire to merge this to M52, so I think I'm ok with this for now as you've done it. Can you add // TODO(bmcquade): remove SetFirstPaintTriggered as part of fixing crbug.com/616901 https://codereview.chromium.org/1984173002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc:54: FromGWSPageLoadMetricsObserver* observer_; related to above, let's remove observer_ as a member. https://codereview.chromium.org/1984173002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc:55: FromGWSPageLoadMetricsLogger* logger_; // TODO(bmcquade): remove once crbug.com/616901 is addressed https://codereview.chromium.org/1984173002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc:610: EXPECT_FALSE(logger_->HasUserInteractionAfterPaint()); On 2016/06/02 at 20:30:17, Bryan McQuade wrote: > Is it fair to say that the following check to verify the histogram is set also implicitly tests that there was no user interaction after paint? If so I'd rather that the observer tests just stick to verifying expected histogram effects rather than inspecting the internal state of the observer/loggger. None of the other observer tests reach into the state of the observers that I'm aware of, so I'd like to be consistent here if we can. Given the desire to remove the logger_ variable, I'd like to also remove all of these EXPECT_FALSE(logger_->...); calls, if they are also covered by the following histogram tester calls.
Done the changes per bryan's comments. https://codereview.chromium.org/1984173002/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/1984173002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc:40: logger_->SetFirstPaintTriggered(true); On 2016/06/02 at 21:38:29, Bryan McQuade wrote: > On 2016/06/02 at 20:30:17, Bryan McQuade wrote: > > this is unfortunate but i'm not yet sure how to fix it. i'm concerned with having to force invocation of this method since it's not called in production code due to first_paint being zero. seems like we have a bug there and this is side stepping around it, so we're no longer really even testing production code flow. i will think about this a bit more & see what we can do. as you mention the Optional<> change will help but it's not landed yet and I am not sure we want to merge it to M52 so your change probably should not depend on it. > > Looking at this more, given that these tests stub out PageLoadTiming timings, it seems like they should also allow for providing fake timings for the timing data recorded by calls to TimeTicks::Now(). But I don't want to block your change on yet more changes, especially given the desire to merge this to M52, so I think I'm ok with this for now as you've done it. Can you add > // TODO(bmcquade): remove SetFirstPaintTriggered as part of fixing crbug.com/616901 Thanks a lot Bryan. I can take a look and make changes for it after this cl get committed. It really interests me and looks like a good ramp for me to extend the knowledge to chromium, if you feel like so. https://codereview.chromium.org/1984173002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc:610: EXPECT_FALSE(logger_->HasUserInteractionAfterPaint()); On 2016/06/02 at 21:38:29, Bryan McQuade wrote: > On 2016/06/02 at 20:30:17, Bryan McQuade wrote: > > Is it fair to say that the following check to verify the histogram is set also implicitly tests that there was no user interaction after paint? If so I'd rather that the observer tests just stick to verifying expected histogram effects rather than inspecting the internal state of the observer/loggger. None of the other observer tests reach into the state of the observers that I'm aware of, so I'd like to be consistent here if we can. > > Given the desire to remove the logger_ variable, I'd like to also remove all of these EXPECT_FALSE(logger_->...); calls, if they are also covered by the following histogram tester calls. I added this change initially to filter the root cause of the failing cases. As now we located that, I agree that the test for histogram is implicitly sufficient here.
Thanks! LGTM.
mushan@google.com changed reviewers: + jochen@chromium.org
jochen: PTAL for changes to render_widget_host.h We're adding the ability to log first user interaction in page load metrics. As for that, this is to add the ability to listen to input events to RenderWidgetHost. It provides an interface to the clients of RenderWidgetHost to register to input events without getting into details of the input event stack. This helps page load metrics framework get routed to input events in a green way as it is scoped to web contents. Need reviews for content/public, it'll be very appreciated if you can help.
Description was changed from ========== Log First User Interaction in Page Load Metrics Learn more at: https://docs.google.com/document/d/1-OEDCDZPjWQHpmuGzaNxz9cRH4bEQXCo4x04ydazY... BUG=612422 ========== to ========== Log First User Interaction in Page Load Metrics Learn more at: https://docs.google.com/document/d/1-OEDCDZPjWQHpmuGzaNxz9cRH4bEQXCo4x04ydazY... BUG=612422 ==========
lgtm
The CQ bit was checked by mushan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, tdresser@chromium.org, csharrison@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/1984173002/#ps320001 (title: "update fragments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1984173002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mushan@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1984173002/320001
Message was sent while issue was closed.
Description was changed from ========== Log First User Interaction in Page Load Metrics Learn more at: https://docs.google.com/document/d/1-OEDCDZPjWQHpmuGzaNxz9cRH4bEQXCo4x04ydazY... BUG=612422 ========== to ========== Log First User Interaction in Page Load Metrics Learn more at: https://docs.google.com/document/d/1-OEDCDZPjWQHpmuGzaNxz9cRH4bEQXCo4x04ydazY... BUG=612422 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Log First User Interaction in Page Load Metrics Learn more at: https://docs.google.com/document/d/1-OEDCDZPjWQHpmuGzaNxz9cRH4bEQXCo4x04ydazY... BUG=612422 ========== to ========== Log First User Interaction in Page Load Metrics Learn more at: https://docs.google.com/document/d/1-OEDCDZPjWQHpmuGzaNxz9cRH4bEQXCo4x04ydazY... BUG=612422 Committed: https://crrev.com/48cac60dbb50219805cc59e04ad50022c0288bc7 Cr-Commit-Position: refs/heads/master@{#398128} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/48cac60dbb50219805cc59e04ad50022c0288bc7 Cr-Commit-Position: refs/heads/master@{#398128}
Message was sent while issue was closed.
On 2016/06/06 at 21:24:47, commit-bot wrote: > Patchset 17 (id:??) landed as https://crrev.com/48cac60dbb50219805cc59e04ad50022c0288bc7 > Cr-Commit-Position: refs/heads/master@{#398128} mushan@ It seems you also need to update histograms.xml file for the added enum entry ERR_USER_INPUT_WITH_NO_RELEVANT_LOAD.
Message was sent while issue was closed.
On 2016/06/07 at 16:18:13, shivanisha wrote: > On 2016/06/06 at 21:24:47, commit-bot wrote: > > Patchset 17 (id:??) landed as https://crrev.com/48cac60dbb50219805cc59e04ad50022c0288bc7 > > Cr-Commit-Position: refs/heads/master@{#398128} > > mushan@ > It seems you also need to update histograms.xml file for the added enum entry ERR_USER_INPUT_WITH_NO_RELEVANT_LOAD. Thanks for the catch! Filed https://codereview.chromium.org/2067163002 to fix. |
