Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2540183003:
Add UserInputTracker, which keeps track of recent user input events. (Closed)
|
Created:
4 years ago by Bryan McQuade Modified:
4 years ago Reviewers:
Charlie Harrison CC:
chromium-reviews, csharrison+watch_chromium.org, loading-reviews+metrics_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UserInputTracker, which keeps track of recent user input events.
UserInputTracker keeps track of recent user input events, and allows
for consuming of those events. This class will be used in a subsequent
change https://codereview.chromium.org/2545593003 to heuristically
determine if an action such as a navigation was due to a user
interaction, by checking to see if a user input event was received
shortly before that action.
BUG=633329
Committed: https://crrev.com/20cc4e8c0e57f1e675a6354ce8916c02d68b88c9
Cr-Commit-Position: refs/heads/master@{#436859}
Patch Set 1 #
Total comments: 10
Patch Set 2 : address some comments #Patch Set 3 : fix #Patch Set 4 : add comments and cleanups #Patch Set 5 : address comments #
Total comments: 38
Patch Set 6 : address comments #
Total comments: 6
Patch Set 7 : address comments #
Total comments: 2
Patch Set 8 : address comments #
Total comments: 2
Patch Set 9 : address comment #
Dependent Patchsets: Messages
Total messages: 58 (36 generated)
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add UserInputTracker, which will be used in a subsequent change to heuristically determine if an action such as a navigation was due to a user interaction, by checking to see if a user input event was received shortly before that action. BUG= ========== to ========== Add UserInputTracker, which keeps track of recent user input events. UserInputTracker keeps track of recent user input events, and allows for comsuing of those events. This class will be used in a subsequent change to heuristically determine if an action such as a navigation was due to a user interaction, by checking to see if a user input event was received shortly before that action. BUG= ==========
Description was changed from ========== Add UserInputTracker, which keeps track of recent user input events. UserInputTracker keeps track of recent user input events, and allows for comsuing of those events. This class will be used in a subsequent change to heuristically determine if an action such as a navigation was due to a user interaction, by checking to see if a user input event was received shortly before that action. BUG= ========== to ========== Add UserInputTracker, which keeps track of recent user input events. UserInputTracker keeps track of recent user input events, and allows for comsuing of those events. This class will be used in a subsequent change to heuristically determine if an action such as a navigation was due to a user interaction, by checking to see if a user input event was received shortly before that action. https://codereview.chromium.org/2545593003 is a follow up change that uses UserInputTracker to heuristically attribute user interactions based on user input events. BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add UserInputTracker, which keeps track of recent user input events. UserInputTracker keeps track of recent user input events, and allows for comsuing of those events. This class will be used in a subsequent change to heuristically determine if an action such as a navigation was due to a user interaction, by checking to see if a user input event was received shortly before that action. https://codereview.chromium.org/2545593003 is a follow up change that uses UserInputTracker to heuristically attribute user interactions based on user input events. BUG= ========== to ========== Add UserInputTracker, which keeps track of recent user input events. UserInputTracker keeps track of recent user input events, and allows for comsuing of those events. This class will be used in a subsequent change https://codereview.chromium.org/2545593003 to heuristically determine if an action such as a navigation was due to a user interaction, by checking to see if a user input event was received shortly before that action. BUG= ==========
bmcquade@chromium.org changed reviewers: + csharrison@chromium.org
PTAL, thanks!
high level notes, haven't looked at tests yet. - Can you document a bit how often input events come in? It's unclear how big event_times_ can get, or how fast it grows. If it grows too fast, we maybe should rate limit? - Can you include the code that uses this new class in this CL? It's hard to review this without knowing how it will be used, and switching between CLs is cumbersome. https://codereview.chromium.org/2540183003/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/user_input_tracker.cc (right): https://codereview.chromium.org/2540183003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/user_input_tracker.cc:47: return base::TimeTicks() + Can you document why this conversion is correct? https://codereview.chromium.org/2540183003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/user_input_tracker.cc:61: Should we trim event_times_ here too, to avoid it growing too big? https://codereview.chromium.org/2540183003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/user_input_tracker.cc:106: base::TimeTicks UserInputTracker::GetOldestAllowedEventTime() { Why don't we limit to 1s old instead of 2s old events? https://codereview.chromium.org/2540183003/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/user_input_tracker.h (right): https://codereview.chromium.org/2540183003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/user_input_tracker.h:29: // rapid succession, e.g. from a JavaScript tight loop, as all of the "e.g. JS code that dispatches new navigation requests in a tight loop can result in dozens of programmatically generated navigations being user initiated." You can cut some of the words here that are redundant, and the bit about 30ms is too much detail imo. https://codereview.chromium.org/2540183003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/user_input_tracker.h:62: std::set<base::TimeTicks> event_times_; Can this be a sorted vector instead?
On 2016/12/02 at 02:58:44, csharrison wrote: > high level notes, haven't looked at tests yet. > > - Can you document a bit how often input events come in? It's unclear how big event_times_ can get, or how fast it grows. If it grows too fast, we maybe should rate limit? > > - Can you include the code that uses this new class in this CL? It's hard to review this without knowing how it will be used, and switching between CLs is cumbersome. > > https://codereview.chromium.org/2540183003/diff/1/chrome/browser/page_load_me... > File chrome/browser/page_load_metrics/user_input_tracker.cc (right): > > https://codereview.chromium.org/2540183003/diff/1/chrome/browser/page_load_me... > chrome/browser/page_load_metrics/user_input_tracker.cc:47: return base::TimeTicks() + > Can you document why this conversion is correct? > > https://codereview.chromium.org/2540183003/diff/1/chrome/browser/page_load_me... > chrome/browser/page_load_metrics/user_input_tracker.cc:61: > Should we trim event_times_ here too, to avoid it growing too big? > > https://codereview.chromium.org/2540183003/diff/1/chrome/browser/page_load_me... > chrome/browser/page_load_metrics/user_input_tracker.cc:106: base::TimeTicks UserInputTracker::GetOldestAllowedEventTime() { > Why don't we limit to 1s old instead of 2s old events? > > https://codereview.chromium.org/2540183003/diff/1/chrome/browser/page_load_me... > File chrome/browser/page_load_metrics/user_input_tracker.h (right): > > https://codereview.chromium.org/2540183003/diff/1/chrome/browser/page_load_me... > chrome/browser/page_load_metrics/user_input_tracker.h:29: // rapid succession, e.g. from a JavaScript tight loop, as all of the > "e.g. JS code that dispatches new navigation requests in a tight loop can result in dozens of programmatically generated navigations being user initiated." You can cut some of the words here that are redundant, and the bit about 30ms is too much detail imo. > > https://codereview.chromium.org/2540183003/diff/1/chrome/browser/page_load_me... > chrome/browser/page_load_metrics/user_input_tracker.h:62: std::set<base::TimeTicks> event_times_; > Can this be a sorted vector instead? Will address comments, but re: merging the CLs, though we do tend to create very large CLs in Chrome org, that's not the norm that I've seen across other orgs. Typical guidance is to keep changes small to make reviewing faster and to minimize the chance that a bug gets missed during reviews. I intentionally split this out with those goals (in particular, the latter) in mind. I'll send you some internal docs that suggest this as a best practice as well. I did link to the overall CL so you can see how this class is used. If you do have high level API feedback re: how this code is used in the linked CL, I'm happy to chat about that offline and try to address those API/design considerations in this CL. But I'd like to split reviewing of the technical implementation details across the two CLs.
Ok that's fine if you have a strong preference, but would you mind setting it up as a proper dependent CL? This way I can be sure that there are no subtle differences in the dependent CLs definitions of the classes in this CL.
Sure, I'll set it up as a dependent CL. Do you happen to know if there's a way to run a change with a dependent CL against trybots? I've not found that to work, which is why I didn't set up a dependency. Note that the second CL isn't ready for review yet, but the dependency on UserInputTracker won't change there - there are just some metrics logging implementation details that need to be worked out. On Fri, Dec 2, 2016 at 9:30 AM <csharrison@chromium.org> wrote: > Ok that's fine if you have a strong preference, but would you mind setting > it up > as a proper dependent CL? This way I can be sure that there are no subtle > differences in the dependent CLs definitions of the classes in this CL. > > https://codereview.chromium.org/2540183003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! Addressed comments. The implementation got a bit more complex as a result of supporting rate limiting. LMK if you see any opportunities to simplify. https://codereview.chromium.org/2540183003/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/user_input_tracker.cc (right): https://codereview.chromium.org/2540183003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/user_input_tracker.cc:47: return base::TimeTicks() + On 2016/12/02 at 02:58:44, Charlie Harrison wrote: > Can you document why this conversion is correct? Done https://codereview.chromium.org/2540183003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/user_input_tracker.cc:61: On 2016/12/02 at 02:58:44, Charlie Harrison wrote: > Should we trim event_times_ here too, to avoid it growing too big? Yep - moved to the rate limiting model we talked about, with a DCHECK to verify we don't exceed the allowed capacity https://codereview.chromium.org/2540183003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/user_input_tracker.cc:106: base::TimeTicks UserInputTracker::GetOldestAllowedEventTime() { On 2016/12/02 at 02:58:44, Charlie Harrison wrote: > Why don't we limit to 1s old instead of 2s old events? The idea here is that if someone calls FindMostRecentUserInputEventBefore with a time slightly in the past, we'd like to be able to provide data for times at least kMaxEventAgeSeconds before any reasonable time they might provide. So we use a time that's 2x the period, effectively allowing times passed to FindMostRecentUserInputEventBefore up to 1 second in the past. https://codereview.chromium.org/2540183003/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/user_input_tracker.h (right): https://codereview.chromium.org/2540183003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/user_input_tracker.h:29: // rapid succession, e.g. from a JavaScript tight loop, as all of the On 2016/12/02 at 02:58:44, Charlie Harrison wrote: > "e.g. JS code that dispatches new navigation requests in a tight loop can result in dozens of programmatically generated navigations being user initiated." You can cut some of the words here that are redundant, and the bit about 30ms is too much detail imo. Done https://codereview.chromium.org/2540183003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/user_input_tracker.h:62: std::set<base::TimeTicks> event_times_; On 2016/12/02 at 02:58:44, Charlie Harrison wrote: > Can this be a sorted vector instead? Yeah - we discussed this & I made the switch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Probably won't get to this until monday, sorry.
This looks great. One thing that tripped me up is that FindAndConsumeInputEventsBefore(time) also consumes |time| *if an event was found before |time|*, but FindMostRecentUserInputEventBefore(time) does not include |time|. This is very confusing, and I would support more verbose names or a simpler api. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/user_input_tracker.cc (right): https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.cc:49: // WebInputEvent::timeStampSeconds is a double representing number of optional: This looks like its only used once, I'd personally prefer inlining in GetEventTime, but up to you. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.cc:65: const size_t UserInputTracker::kMaxEntries = 100; Why 100 and not a nice round number like 128 or 64? Is it to ensure an integer ClampMillis? https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.cc:93: base::TimeTicks time = RoundToRateLimitedOffset(GetEventTime(event)); Optional: Instead of rounding we could instead just check if the given event is > ClampMillis from the most recent event. It gives us less precision but it is simpler and doesn't require the complicated rounding code. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.cc:94: if (time <= std::max(most_recent_consumed_time_, GetOldestAllowedEventTime())) GetOldestAllowedEventTime calls the monotonic clock, which isn't cheap. Since we already call TimeTicks::Now() in RemoveOldInputEvents I'd like to find a way to only call it once in this function, maybe by expanding RemoveOldInputEvents? https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.cc:109: event_times_.insert(it, time); I am curious what the insertion pattern looks like in practice. Are we usually inserting at the ~end? https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.cc:157: void UserInputTracker::RemoveInputEventsUpTo(base::TimeTicks cutoff) { This looks like it removes input events up to *and including* cutoff. Optional if you want to clarify the name. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/user_input_tracker.h (right): https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.h:38: static const size_t kMaxEntries; s/kMaxEntries/kMaxTrackedEvents or something similar? kMaxEntries is very vague, considering that from the public interface, UserInputTracker is not a container. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.h:67: void RemoveOldInputEvents(); This one might need some documentation. What is "Old"? https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.h:70: std::vector<base::TimeTicks> event_times_; Please document that this vector should always be sorted. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/user_input_tracker_unittest.cc (right): https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. Copyright 2016 no (c) https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:10: Shouldn't these tests be in namespace page_load_metrics? https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:18: return t.ToInternalValue() / Use (t - base::TimeTicks()).InSecondsF() https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:26: type = blink::WebInputEvent::Char; Should we vary on this to ensure we aren't recording mouse moves, e.g.? https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:97: EXPECT_TRUE(tracker.FindAndConsumeInputEventsBefore(e1.GetTimeStamp())); Why is this always true? Shouldn't e1 == event_times_.begin()? Could this flake if rounding ends up being a no-op? https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:182: e2.GetTimeStampRounded() + base::TimeDelta::FromMilliseconds(1))); Add test: EXPECT_EQ(base::TimeTicks(), tracker.FidnMostRecentUserInputEventBefore(e2.GetTimeStampRounded()) ?
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/05 at 19:17:47, csharrison wrote: > This looks great. One thing that tripped me up is that > FindAndConsumeInputEventsBefore(time) also consumes |time| *if an event was found before |time|*, but FindMostRecentUserInputEventBefore(time) does not include |time|. This is very confusing, and I would support more verbose names or a simpler api. This is just a bug - FindAndConsume should just consume up to the most recently found time. > > https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... > File chrome/browser/page_load_metrics/user_input_tracker.cc (right): > > https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/user_input_tracker.cc:49: // WebInputEvent::timeStampSeconds is a double representing number of > optional: This looks like its only used once, I'd personally prefer inlining in GetEventTime, but up to you. > > https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/user_input_tracker.cc:65: const size_t UserInputTracker::kMaxEntries = 100; > Why 100 and not a nice round number like 128 or 64? Is it to ensure an integer ClampMillis? > > https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/user_input_tracker.cc:93: base::TimeTicks time = RoundToRateLimitedOffset(GetEventTime(event)); > Optional: Instead of rounding we could instead just check if the given event is > ClampMillis from the most recent event. It gives us less precision but it is simpler and doesn't require the complicated rounding code. > > https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/user_input_tracker.cc:94: if (time <= std::max(most_recent_consumed_time_, GetOldestAllowedEventTime())) > GetOldestAllowedEventTime calls the monotonic clock, which isn't cheap. Since we already call TimeTicks::Now() in RemoveOldInputEvents I'd like to find a way to only call it once in this function, maybe by expanding RemoveOldInputEvents? > > https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/user_input_tracker.cc:109: event_times_.insert(it, time); > I am curious what the insertion pattern looks like in practice. Are we usually inserting at the ~end? > > https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/user_input_tracker.cc:157: void UserInputTracker::RemoveInputEventsUpTo(base::TimeTicks cutoff) { > This looks like it removes input events up to *and including* cutoff. Optional if you want to clarify the name. > > https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... > File chrome/browser/page_load_metrics/user_input_tracker.h (right): > > https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/user_input_tracker.h:38: static const size_t kMaxEntries; > s/kMaxEntries/kMaxTrackedEvents or something similar? kMaxEntries is very vague, considering that from the public interface, UserInputTracker is not a container. > > https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/user_input_tracker.h:67: void RemoveOldInputEvents(); > This one might need some documentation. What is "Old"? > > https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/user_input_tracker.h:70: std::vector<base::TimeTicks> event_times_; > Please document that this vector should always be sorted. > > https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... > File chrome/browser/page_load_metrics/user_input_tracker_unittest.cc (right): > > https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. > Copyright 2016 > > no (c) > > https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:10: > Shouldn't these tests be in namespace page_load_metrics? > > https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:18: return t.ToInternalValue() / > Use (t - base::TimeTicks()).InSecondsF() > > https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:26: type = blink::WebInputEvent::Char; > Should we vary on this to ensure we aren't recording mouse moves, e.g.? > > https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:97: EXPECT_TRUE(tracker.FindAndConsumeInputEventsBefore(e1.GetTimeStamp())); > Why is this always true? Shouldn't e1 == event_times_.begin()? Could this flake if rounding ends up being a no-op? > > https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:182: e2.GetTimeStampRounded() + base::TimeDelta::FromMilliseconds(1))); > Add test: > EXPECT_EQ(base::TimeTicks(), tracker.FidnMostRecentUserInputEventBefore(e2.GetTimeStampRounded()) ?
Thanks for the thorough review! Addressed comments. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/user_input_tracker.cc (right): https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.cc:49: // WebInputEvent::timeStampSeconds is a double representing number of On 2016/12/05 at 19:17:46, Charlie Harrison wrote: > optional: This looks like its only used once, I'd personally prefer inlining in GetEventTime, but up to you. You're right, there's no benefit to factoring this out for reuse purposes, given it's only used one time. This was motivated by a desire to abstract away the strange implementation (base::TimeTicks() plus a time delta, in order to get a timedelta in monotonic time base). This ideally allows readers of the code to just skip right over 'GetTimeTicksFromSeconds' at the call site, since it's obvious from the name what that should do, making it easier to focus on the implementation details of the call site. This was motivated in part by your earlier comment asking to add a comment where it was used previously - felt it better to abstract the strange code away in a helper along with a comment to explain. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.cc:65: const size_t UserInputTracker::kMaxEntries = 100; On 2016/12/05 at 19:17:46, Charlie Harrison wrote: > Why 100 and not a nice round number like 128 or 64? Is it to ensure an integer ClampMillis? Ha, is 64 or 128 rounder than 100? I guess it depends on your base. :) I've not heard this suggestion before but I'm open to it. Is there a benefit to using a base16-based value like 64 or 128? Does it lead to better memory alignment? I can make the change - just want to understand the motivation. You point out the benefit to using 100 - it makes the clamp interval a round number that's easier for humans to reason about. I think that's reason to stick with a base10 value but I'm open to switching if there's a reason base16 will have a significant improvement on browser performance. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.cc:93: base::TimeTicks time = RoundToRateLimitedOffset(GetEventTime(event)); On 2016/12/05 at 19:17:46, Charlie Harrison wrote: > Optional: Instead of rounding we could instead just check if the given event is > ClampMillis from the most recent event. It gives us less precision but it is simpler and doesn't require the complicated rounding code. That's an interesting idea. It adds some additional complexity in that we need to call upper_bound then decrement the iterator if we're not at beginning in order to get the first event before or equal to, then see if it's within the clamp threshold (can't just look at events_.end() since the new time may not go at the end). I think it's complexity either way. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.cc:94: if (time <= std::max(most_recent_consumed_time_, GetOldestAllowedEventTime())) On 2016/12/05 at 19:17:46, Charlie Harrison wrote: > GetOldestAllowedEventTime calls the monotonic clock, which isn't cheap. Since we already call TimeTicks::Now() in RemoveOldInputEvents I'd like to find a way to only call it once in this function, maybe by expanding RemoveOldInputEvents? I'm mixed on this. I think the simplest way to do this would be to pass 'now' into GetOldestAllowedEventTime(). That brings a slight perf benefit, but it's weird to have to pass the current time into this method. Are we sure it's worth the added code complexity? We expect this method to be called relatively infrequently, on the order of the frequency with which a user might click a mouse button, click a key on the keyboard, or tap the screen of the device. Perhaps once every few hundred milliseconds during an active period. I've gone ahead & made the change but I'm not sure this is the right perf/code simplicity tradeoff. I don't know how long the call to clock_gettime takes, however. Do you have a sense for the cost of an inocation of clock_gettime with the monotonic clock? Are we talking nanos here? Assuming a few tens of nanos, we're looking at about 20/100000000 = 0.00002% CPU cycle savings assuming 1 event per 100ms. My sense is that's probably not enough to justify an optimization. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.cc:109: event_times_.insert(it, time); On 2016/12/05 at 19:17:46, Charlie Harrison wrote: > I am curious what the insertion pattern looks like in practice. Are we usually inserting at the ~end? Yes, it should be usually at the end, but no guarantee. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.cc:157: void UserInputTracker::RemoveInputEventsUpTo(base::TimeTicks cutoff) { On 2016/12/05 at 19:17:46, Charlie Harrison wrote: > This looks like it removes input events up to *and including* cutoff. Optional if you want to clarify the name. Ah, interesting, I'd though of 'UpTo' as being inclusive, but I can see that's not guaranteed. I renamed it. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/user_input_tracker.h (right): https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.h:38: static const size_t kMaxEntries; On 2016/12/05 at 19:17:46, Charlie Harrison wrote: > s/kMaxEntries/kMaxTrackedEvents or something similar? kMaxEntries is very vague, considering that from the public interface, UserInputTracker is not a container. Sure, renamed. This is only public for tests. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.h:67: void RemoveOldInputEvents(); On 2016/12/05 at 19:17:46, Charlie Harrison wrote: > This one might need some documentation. What is "Old"? Done. It's somewhat of an implementation detail, but added a comment. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.h:70: std::vector<base::TimeTicks> event_times_; On 2016/12/05 at 19:17:46, Charlie Harrison wrote: > Please document that this vector should always be sorted. Renamed to sorted_event_times_ https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/user_input_tracker_unittest.cc (right): https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2016/12/05 at 19:17:46, Charlie Harrison wrote: > Copyright 2016 > > no (c) Ah, sure. I fixed a couple other tests in the same directory that had the same pattern. Not really sure where we got that from. Perhaps it was the old copyright statement. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:10: On 2016/12/05 at 19:17:46, Charlie Harrison wrote: > Shouldn't these tests be in namespace page_load_metrics? I don't think so. Most of our tests don't follow this pattern (in the observers directory). https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:18: return t.ToInternalValue() / On 2016/12/05 at 19:17:46, Charlie Harrison wrote: > Use (t - base::TimeTicks()).InSecondsF() Ah, nice, much better, thanks! https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:26: type = blink::WebInputEvent::Char; On 2016/12/05 at 19:17:47, Charlie Harrison wrote: > Should we vary on this to ensure we aren't recording mouse moves, e.g.? Added a test. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:97: EXPECT_TRUE(tracker.FindAndConsumeInputEventsBefore(e1.GetTimeStamp())); On 2016/12/05 at 19:17:46, Charlie Harrison wrote: > Why is this always true? Shouldn't e1 == event_times_.begin()? Could this flake if rounding ends up being a no-op? Wow, this is just a straight up test bug, thanks for your attention to detail here. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:182: e2.GetTimeStampRounded() + base::TimeDelta::FromMilliseconds(1))); On 2016/12/05 at 19:17:47, Charlie Harrison wrote: > Add test: > EXPECT_EQ(base::TimeTicks(), tracker.FidnMostRecentUserInputEventBefore(e2.GetTimeStampRounded()) ? Done
Description was changed from ========== Add UserInputTracker, which keeps track of recent user input events. UserInputTracker keeps track of recent user input events, and allows for comsuing of those events. This class will be used in a subsequent change https://codereview.chromium.org/2545593003 to heuristically determine if an action such as a navigation was due to a user interaction, by checking to see if a user input event was received shortly before that action. BUG= ========== to ========== Add UserInputTracker, which keeps track of recent user input events. UserInputTracker keeps track of recent user input events, and allows for consuing of those events. This class will be used in a subsequent change https://codereview.chromium.org/2545593003 to heuristically determine if an action such as a navigation was due to a user interaction, by checking to see if a user input event was received shortly before that action. BUG= ==========
Description was changed from ========== Add UserInputTracker, which keeps track of recent user input events. UserInputTracker keeps track of recent user input events, and allows for consuing of those events. This class will be used in a subsequent change https://codereview.chromium.org/2545593003 to heuristically determine if an action such as a navigation was due to a user interaction, by checking to see if a user input event was received shortly before that action. BUG= ========== to ========== Add UserInputTracker, which keeps track of recent user input events. UserInputTracker keeps track of recent user input events, and allows for consuming of those events. This class will be used in a subsequent change https://codereview.chromium.org/2545593003 to heuristically determine if an action such as a navigation was due to a user interaction, by checking to see if a user input event was received shortly before that action. BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Responding to comments and found a few more nits. Probably want to do one more pass through tests after this one. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/user_input_tracker.cc (right): https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.cc:65: const size_t UserInputTracker::kMaxEntries = 100; On 2016/12/06 01:52:24, Bryan McQuade wrote: > On 2016/12/05 at 19:17:46, Charlie Harrison wrote: > > Why 100 and not a nice round number like 128 or 64? Is it to ensure an integer > ClampMillis? > > Ha, is 64 or 128 rounder than 100? I guess it depends on your base. :) > > I've not heard this suggestion before but I'm open to it. Is there a benefit to > using a base16-based value like 64 or 128? Does it lead to better memory > alignment? I can make the change - just want to understand the motivation. > > You point out the benefit to using 100 - it makes the clamp interval a round > number that's easier for humans to reason about. I think that's reason to stick > with a base10 value but I'm open to switching if there's a reason base16 will > have a significant improvement on browser performance. It isn't a huge deal. Aligning to a multiple of 64 bytes (a typical cache line size) makes some sense, as you won't be pulling unnecessary data into your cache when reading from the end of your buffer. TimeTicks is 8 bytes so that's 8 TimeTicks per cache line. Probably 100 is fine though, especially because you get the integer clamp :) https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.cc:93: base::TimeTicks time = RoundToRateLimitedOffset(GetEventTime(event)); On 2016/12/06 01:52:24, Bryan McQuade wrote: > On 2016/12/05 at 19:17:46, Charlie Harrison wrote: > > Optional: Instead of rounding we could instead just check if the given event > is > ClampMillis from the most recent event. It gives us less precision but it > is simpler and doesn't require the complicated rounding code. > > That's an interesting idea. It adds some additional complexity in that we need > to call upper_bound then decrement the iterator if we're not at beginning in > order to get the first event before or equal to, then see if it's within the > clamp threshold (can't just look at events_.end() since the new time may not go > at the end). I think it's complexity either way. Yeah I'm not sure, after going through the rest of the patch I'm in favor of how you did it. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.cc:94: if (time <= std::max(most_recent_consumed_time_, GetOldestAllowedEventTime())) On 2016/12/06 01:52:24, Bryan McQuade wrote: > On 2016/12/05 at 19:17:46, Charlie Harrison wrote: > > GetOldestAllowedEventTime calls the monotonic clock, which isn't cheap. Since > we already call TimeTicks::Now() in RemoveOldInputEvents I'd like to find a way > to only call it once in this function, maybe by expanding RemoveOldInputEvents? > > I'm mixed on this. I think the simplest way to do this would be to pass 'now' > into GetOldestAllowedEventTime(). That brings a slight perf benefit, but it's > weird to have to pass the current time into this method. Are we sure it's worth > the added code complexity? We expect this method to be called relatively > infrequently, on the order of the frequency with which a user might click a > mouse button, click a key on the keyboard, or tap the screen of the device. > Perhaps once every few hundred milliseconds during an active period. > > I've gone ahead & made the change but I'm not sure this is the right perf/code > simplicity tradeoff. I don't know how long the call to clock_gettime takes, > however. Do you have a sense for the cost of an inocation of clock_gettime with > the monotonic clock? Are we talking nanos here? Assuming a few tens of nanos, > we're looking at about 20/100000000 = 0.00002% CPU cycle savings assuming 1 > event per 100ms. My sense is that's probably not enough to justify an > optimization. Your call. I agree It is a bit awkward to thread the TimeTicks, and I would be fine reverting it to calling Now() twice. One thing you could do is have GetOldestAllowedEventTime to instead return a TimeDelta, and rename it something like GetOldEventThreshold or something. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.cc:109: event_times_.insert(it, time); On 2016/12/06 01:52:24, Bryan McQuade wrote: > On 2016/12/05 at 19:17:46, Charlie Harrison wrote: > > I am curious what the insertion pattern looks like in practice. Are we usually > inserting at the ~end? > > Yes, it should be usually at the end, but no guarantee. Acknowledged. https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/user_input_tracker_unittest.cc (right): https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:10: On 2016/12/06 01:52:25, Bryan McQuade wrote: > On 2016/12/05 at 19:17:46, Charlie Harrison wrote: > > Shouldn't these tests be in namespace page_load_metrics? > > I don't think so. Most of our tests don't follow this pattern (in the observers > directory). That was because originally the namespace was confined to the separate component, and there were concerns with sharing the namespace across both //components and //chrome. I think now that all code is in //chrome we can use the namespace in tests. Most other namespaces like content and net do this. https://codereview.chromium.org/2540183003/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/user_input_tracker.cc (right): https://codereview.chromium.org/2540183003/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/user_input_tracker.cc:114: DCHECK(sorted_event_times_.size() <= kMaxTrackedEvents); DCHECK_LE https://codereview.chromium.org/2540183003/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/user_input_tracker.cc:150: DCHECK(candidate < time); DCHECK_LT https://codereview.chromium.org/2540183003/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/user_input_tracker.h (right): https://codereview.chromium.org/2540183003/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/user_input_tracker.h:76: DISALLOW_COPY_AND_ASSIGN(UserInputTracker); include base/macros.h
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/user_input_tracker_unittest.cc (right): https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:10: On 2016/12/06 at 18:43:09, Charlie Harrison wrote: > On 2016/12/06 01:52:25, Bryan McQuade wrote: > > On 2016/12/05 at 19:17:46, Charlie Harrison wrote: > > > Shouldn't these tests be in namespace page_load_metrics? > > > > I don't think so. Most of our tests don't follow this pattern (in the observers > > directory). > > That was because originally the namespace was confined to the separate component, and there were concerns with sharing the namespace across both //components and //chrome. I think now that all code is in //chrome we can use the namespace in tests. Most other namespaces like content and net do this. Ah, ok, I went ahead and made this change. I took a look at how often this pattern is used in chrome/. There doesn't appear to be a convention one way or the other: git grep -l -E '^namespace [^{]' chrome/ | grep test.cc | wc -l 1008 git grep -L -E '^namespace [^{]' chrome/ | grep test.cc | wc -l 1062 So if my grep is correct, it's roughly 50/50, meaning no real convention in chrome/. It would be nice if there was a convention across the codebase, but it appears to vary a good bit. https://codereview.chromium.org/2540183003/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/user_input_tracker.cc (right): https://codereview.chromium.org/2540183003/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/user_input_tracker.cc:114: DCHECK(sorted_event_times_.size() <= kMaxTrackedEvents); On 2016/12/06 at 18:43:09, Charlie Harrison wrote: > DCHECK_LE Done https://codereview.chromium.org/2540183003/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/user_input_tracker.cc:150: DCHECK(candidate < time); On 2016/12/06 at 18:43:09, Charlie Harrison wrote: > DCHECK_LT Done https://codereview.chromium.org/2540183003/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/user_input_tracker.h (right): https://codereview.chromium.org/2540183003/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/user_input_tracker.h:76: DISALLOW_COPY_AND_ASSIGN(UserInputTracker); On 2016/12/06 at 18:43:09, Charlie Harrison wrote: > include base/macros.h Done
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/user_input_tracker.cc (right): https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.cc:94: if (time <= std::max(most_recent_consumed_time_, GetOldestAllowedEventTime())) On 2016/12/06 18:43:09, Charlie Harrison wrote: > On 2016/12/06 01:52:24, Bryan McQuade wrote: > > On 2016/12/05 at 19:17:46, Charlie Harrison wrote: > > > GetOldestAllowedEventTime calls the monotonic clock, which isn't cheap. > Since > > we already call TimeTicks::Now() in RemoveOldInputEvents I'd like to find a > way > > to only call it once in this function, maybe by expanding > RemoveOldInputEvents? > > > > I'm mixed on this. I think the simplest way to do this would be to pass 'now' > > into GetOldestAllowedEventTime(). That brings a slight perf benefit, but it's > > weird to have to pass the current time into this method. Are we sure it's > worth > > the added code complexity? We expect this method to be called relatively > > infrequently, on the order of the frequency with which a user might click a > > mouse button, click a key on the keyboard, or tap the screen of the device. > > Perhaps once every few hundred milliseconds during an active period. > > > > I've gone ahead & made the change but I'm not sure this is the right perf/code > > simplicity tradeoff. I don't know how long the call to clock_gettime takes, > > however. Do you have a sense for the cost of an inocation of clock_gettime > with > > the monotonic clock? Are we talking nanos here? Assuming a few tens of nanos, > > we're looking at about 20/100000000 = 0.00002% CPU cycle savings assuming 1 > > event per 100ms. My sense is that's probably not enough to justify an > > optimization. > > Your call. I agree It is a bit awkward to thread the TimeTicks, and I would be > fine reverting it to calling Now() twice. One thing you could do is have > GetOldestAllowedEventTime to instead return a TimeDelta, and rename it something > like GetOldEventThreshold or something. What do you think about this idea? https://codereview.chromium.org/2540183003/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/user_input_tracker_unittest.cc (right): https://codereview.chromium.org/2540183003/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:65: EXPECT_TRUE(tracker.FindAndConsumeInputEventsBefore(e.GetTimeStamp())); Won't this flake if e.GetTimeStamp() == e.GetTimeStampRounded()? Same throughout these tests. It seems like we should be testing with |after|.
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/user_input_tracker.cc (right): https://codereview.chromium.org/2540183003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/user_input_tracker.cc:94: if (time <= std::max(most_recent_consumed_time_, GetOldestAllowedEventTime())) On 2016/12/06 at 22:04:55, Charlie Harrison wrote: > On 2016/12/06 18:43:09, Charlie Harrison wrote: > > On 2016/12/06 01:52:24, Bryan McQuade wrote: > > > On 2016/12/05 at 19:17:46, Charlie Harrison wrote: > > > > GetOldestAllowedEventTime calls the monotonic clock, which isn't cheap. > > Since > > > we already call TimeTicks::Now() in RemoveOldInputEvents I'd like to find a > > way > > > to only call it once in this function, maybe by expanding > > RemoveOldInputEvents? > > > > > > I'm mixed on this. I think the simplest way to do this would be to pass 'now' > > > into GetOldestAllowedEventTime(). That brings a slight perf benefit, but it's > > > weird to have to pass the current time into this method. Are we sure it's > > worth > > > the added code complexity? We expect this method to be called relatively > > > infrequently, on the order of the frequency with which a user might click a > > > mouse button, click a key on the keyboard, or tap the screen of the device. > > > Perhaps once every few hundred milliseconds during an active period. > > > > > > I've gone ahead & made the change but I'm not sure this is the right perf/code > > > simplicity tradeoff. I don't know how long the call to clock_gettime takes, > > > however. Do you have a sense for the cost of an inocation of clock_gettime > > with > > > the monotonic clock? Are we talking nanos here? Assuming a few tens of nanos, > > > we're looking at about 20/100000000 = 0.00002% CPU cycle savings assuming 1 > > > event per 100ms. My sense is that's probably not enough to justify an > > > optimization. > > > > Your call. I agree It is a bit awkward to thread the TimeTicks, and I would be > > fine reverting it to calling Now() twice. One thing you could do is have > > GetOldestAllowedEventTime to instead return a TimeDelta, and rename it something > > like GetOldEventThreshold or something. > > What do you think about this idea? That is better. I've changed it. https://codereview.chromium.org/2540183003/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/user_input_tracker_unittest.cc (right): https://codereview.chromium.org/2540183003/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:65: EXPECT_TRUE(tracker.FindAndConsumeInputEventsBefore(e.GetTimeStamp())); On 2016/12/06 at 22:04:55, Charlie Harrison wrote: > Won't this flake if e.GetTimeStamp() == e.GetTimeStampRounded()? Same throughout these tests. It seems like we should be testing with |after|. Ah, good catch, yes, I've fixed all of these cases to use |after|. Thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LG aside from one small thing. https://codereview.chromium.org/2540183003/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/user_input_tracker.cc (right): https://codereview.chromium.org/2540183003/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/user_input_tracker.cc:84: RemoveOldInputEvents(); So, this function also calls TimeTicks::Now(). I would prefer expanding it out to use the same `now' as below. I also wouldn't mind removing this function entirely, and I think replacing it with its expansion everywhere is not unreadable. Up to you though.
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
https://codereview.chromium.org/2540183003/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/user_input_tracker.cc (right): https://codereview.chromium.org/2540183003/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/user_input_tracker.cc:84: RemoveOldInputEvents(); On 2016/12/06 at 22:34:39, Charlie Harrison wrote: > So, this function also calls TimeTicks::Now(). I would prefer expanding it out to use the same `now' as below. > > I also wouldn't mind removing this function entirely, and I think replacing it with its expansion everywhere is not unreadable. Up to you though. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add UserInputTracker, which keeps track of recent user input events. UserInputTracker keeps track of recent user input events, and allows for consuming of those events. This class will be used in a subsequent change https://codereview.chromium.org/2545593003 to heuristically determine if an action such as a navigation was due to a user interaction, by checking to see if a user input event was received shortly before that action. BUG= ========== to ========== Add UserInputTracker, which keeps track of recent user input events. UserInputTracker keeps track of recent user input events, and allows for consuming of those events. This class will be used in a subsequent change https://codereview.chromium.org/2545593003 to heuristically determine if an action such as a navigation was due to a user interaction, by checking to see if a user input event was received shortly before that action. BUG=633329 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM ship it! Is the dependent CL ready for review?
On 2016/12/07 04:09:58, Charlie Harrison wrote: > LGTM ship it! Is the dependent CL ready for review? Ha nvm just saw that you already published it.
On 2016/12/07 at 04:10:36, csharrison wrote: > On 2016/12/07 04:09:58, Charlie Harrison wrote: > > LGTM ship it! Is the dependent CL ready for review? > > Ha nvm just saw that you already published it. thx! yep, second change is ready, thanks!
The CQ bit was checked by bmcquade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481083918735000,
"parent_rev": "41e67bb837573a13da1df7acece3d8be23237a3a", "commit_rev":
"c3bbfe20765c6ac09abd034f599180468f84a8fe"}
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Add UserInputTracker, which keeps track of recent user input events. UserInputTracker keeps track of recent user input events, and allows for consuming of those events. This class will be used in a subsequent change https://codereview.chromium.org/2545593003 to heuristically determine if an action such as a navigation was due to a user interaction, by checking to see if a user input event was received shortly before that action. BUG=633329 ========== to ========== Add UserInputTracker, which keeps track of recent user input events. UserInputTracker keeps track of recent user input events, and allows for consuming of those events. This class will be used in a subsequent change https://codereview.chromium.org/2545593003 to heuristically determine if an action such as a navigation was due to a user interaction, by checking to see if a user input event was received shortly before that action. BUG=633329 Committed: https://crrev.com/20cc4e8c0e57f1e675a6354ce8916c02d68b88c9 Cr-Commit-Position: refs/heads/master@{#436859} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/20cc4e8c0e57f1e675a6354ce8916c02d68b88c9 Cr-Commit-Position: refs/heads/master@{#436859} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
