Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1338603002:
Implement a site engagement score based on time-on-site. (Closed)
DescriptionImplement a site engagement score based on time-on-site.
Time spent on site is an important indicator of how important an origin
is to a user. This CL detects time on site by registering callbacks to
handle user input, using any mouse or keypress events as a proxy for
a user spending time on the site. Input events are checked for at
regular intervals, with site engagement updated if input has been
present in each interval.
BUG=464234
Committed: https://crrev.com/f3226a4f280b20eb00024d724b9e70ec1edc2e33
Cr-Commit-Position: refs/heads/master@{#351465}
Committed: https://crrev.com/5eceefb54758e691c7857c4feff111f7db33ef6d
Cr-Commit-Position: refs/heads/master@{#351704}
Patch Set 1 #Patch Set 2 : Compile unit tests #
Total comments: 16
Patch Set 3 : Remove outdated test #Patch Set 4 : Addressing reviewer feedback #Patch Set 5 : Removing references to navigation #Patch Set 6 : More web contents validity checking #
Total comments: 10
Patch Set 7 : Addressing reviewer feedback #Patch Set 8 : Adding browser test. Refactoring to use double scores #Patch Set 9 : Factoring input checking into a new class #Patch Set 10 : Fix partial staging issue #Patch Set 11 : Restore unit tests to test navigations #
Total comments: 29
Patch Set 12 : Moving tests to interactive ui, adding user input tests #Patch Set 13 : Fixing rebase issue #Patch Set 14 : Update types used in browser test #
Total comments: 17
Patch Set 15 : Addressing reviewer feedback #
Total comments: 4
Patch Set 16 : Checking callbacks added, reducing mouse events listened for #Patch Set 17 : Adding tracing, fixing indentation #
Total comments: 12
Patch Set 18 : Rebase #Patch Set 19 : Adding a TestSiteEngagementHelper #
Total comments: 2
Patch Set 20 : Removing observer interface #
Total comments: 2
Patch Set 21 : Remove flaky input simulation tests and combine callback and timer testing #Dependent Patchsets: Messages
Total messages: 71 (17 generated)
dominickn@chromium.org changed reviewers: + calamity@chromium.org
PTAL, thanks!
On 2015/09/11 03:19:26, dominickn wrote: > PTAL, thanks! FYI: if you're okay with this design, I will add a new browser test to check that mouse clicks, movements, and keypresses all increment engagement as intended. We also need to work out if the thresholds are sensible now.
Also, all references to navigation should be changed (particularly in the test file). https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:17: const int kSecondsBetweenUserInputCheck = 10; We're going to want some noise on top of this. Should check with security people. https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:41: base::Unretained(this)); Probably want a WeakPtrFactory for this. https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:74: return; These two checks feel like they should be done on navigation rather than on timer fire. https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:105: callbacks_added_ = true; This only needs re-add the callbacks if they were already there, right? https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:128: mouse_event_callback_); It's probably worth making an Add/RemoveCallback(host) private method to do this. https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:129: callbacks_added_ = false; This should move outside the condition. https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:176: score.AddPoints(SiteEngagementScore::kNavigationPoints); This constant name needs to change. kUserInputPoints I guess. https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.h:117: // Update the karma score of the origin matching |url| for user navigation. Update.
Thanks! https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:41: base::Unretained(this)); On 2015/09/11 07:19:56, calamity wrote: > Probably want a WeakPtrFactory for this. I can't use a WeakPtrFactory because weak pointers can't be used for functions with non-void return values (compile failure). https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:74: return; On 2015/09/11 07:19:56, calamity wrote: > These two checks feel like they should be done on navigation rather than on > timer fire. Done. https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:105: callbacks_added_ = true; On 2015/09/11 07:19:56, calamity wrote: > This only needs re-add the callbacks if they were already there, right? Done. https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:128: mouse_event_callback_); On 2015/09/11 07:19:56, calamity wrote: > It's probably worth making an Add/RemoveCallback(host) private method to do > this. Done. https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:129: callbacks_added_ = false; On 2015/09/11 07:19:56, calamity wrote: > This should move outside the condition. Done. https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:176: score.AddPoints(SiteEngagementScore::kNavigationPoints); On 2015/09/11 07:19:56, calamity wrote: > This constant name needs to change. kUserInputPoints I guess. Done. https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.h:117: // Update the karma score of the origin matching |url| for user navigation. On 2015/09/11 07:19:56, calamity wrote: > Update. Done.
https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1338603002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:41: base::Unretained(this)); On 2015/09/11 08:03:37, dominickn wrote: > On 2015/09/11 07:19:56, calamity wrote: > > Probably want a WeakPtrFactory for this. > > I can't use a WeakPtrFactory because weak pointers can't be used for functions > with non-void return values (compile failure). Lame. But this makes sense. The comment in bind_internal.h says that it just wants you to have a return value so there's no undefined behavior. You can get around this by making a private static method that looks like bool HandleInputEvent(WeakPtr<SiteEngagementHelper> p, const NativeWebKeyboardEvent&) and then just check if |p| is true for still alive-ness.
As discussed offline, this complicates the callbacks a bit, and it's probably sufficient to check the validity of web_contents() (returns nullptr if the web_contents isn't alive any more)
dominickn@chromium.org changed reviewers: + benwells@chromium.org
lgtm
Thanks! @benwells: PTAL. Still to work out are how many engagement points should be awarded over what time periods, what the maximums should be, etc.
https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.cc (left): https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:45: service->HandleNavigation(url); Why is this now removed? I am guessing you're equating the initial engagement on a site with navigation, but I *think* they are different events and should be tracked separately. https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:39: RegisterInputCallbacks(web_contents->GetRenderViewHost()); Should the callbacks be registered here? Won't they be registered in WasShown? If they do need to eb registered here, shouldn't the timer be started too? If there is a strong relationship between timer running and callbacks registered, maybe these should be grouped together into functions like BeginMonitoring and EndMonitoring, which would control the timer and callback registration. https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:64: if (!callbacks_added_) { I wonder if this could be encapsulated into some sort of ScopedInputTracker, which in its constructor sets up the listening and in its destructor kills it. That way you wouldn't need callbacks_added_, the existence of your ScopedInputTracker would be signal enough. https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:98: if (ignore_page_) I don't really like having this timer running all the time. Can we just start it after movement / when Chrome isn't idle? https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_unittest.cc (left): https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_unittest.cc:327: TEST_F(SiteEngagementServiceTest, ScoreIncrementsOnPageRequest) { This test makes sure the whole system works together. You need to replace it with something (or if navigation tracking stays, which I think it should, also add something else to ensure the user input stuff works properly as a whole).
https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.cc (left): https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:45: service->HandleNavigation(url); Why is this now removed? I am guessing you're equating the initial engagement on a site with navigation, but I *think* they are different events and should be tracked separately. https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:39: RegisterInputCallbacks(web_contents->GetRenderViewHost()); Should the callbacks be registered here? Won't they be registered in WasShown? If they do need to eb registered here, shouldn't the timer be started too? If there is a strong relationship between timer running and callbacks registered, maybe these should be grouped together into functions like BeginMonitoring and EndMonitoring, which would control the timer and callback registration. https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:64: if (!callbacks_added_) { I wonder if this could be encapsulated into some sort of ScopedInputTracker, which in its constructor sets up the listening and in its destructor kills it. That way you wouldn't need callbacks_added_, the existence of your ScopedInputTracker would be signal enough. https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:98: if (ignore_page_) I don't really like having this timer running all the time. Can we just start it after movement / when Chrome isn't idle? https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_unittest.cc (left): https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_unittest.cc:327: TEST_F(SiteEngagementServiceTest, ScoreIncrementsOnPageRequest) { This test makes sure the whole system works together. You need to replace it with something (or if navigation tracking stays, which I think it should, also add something else to ensure the user input stuff works properly as a whole).
PTAL. Still working on a full test for this - the timing aspect of it is very tricky to test accurately... https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.cc (left): https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:45: service->HandleNavigation(url); On 2015/09/16 00:23:32, benwells wrote: > Why is this now removed? I am guessing you're equating the initial engagement on > a site with navigation, but I *think* they are different events and should be > tracked separately. calamity@ wanted to remove navigation tracking entirely, but following today's meetings I agree that it's important to keep around. https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:39: RegisterInputCallbacks(web_contents->GetRenderViewHost()); On 2015/09/16 00:23:32, benwells wrote: > Should the callbacks be registered here? Won't they be registered in WasShown? > > If they do need to eb registered here, shouldn't the timer be started too? > > If there is a strong relationship between timer running and callbacks > registered, maybe these should be grouped together into functions like > BeginMonitoring and EndMonitoring, which would control the timer and callback > registration. There isn't a strong relationship between timer running and callbacks registered at the moment: callbacks currently unregister themselves at first input to reduce the overhead of watching for user input. This may change slightly if we decide to score different input types differently (and this have to listen for particular types), but I quite like having the callbacks unregister themselves when they're not needed. https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:64: if (!callbacks_added_) { On 2015/09/16 00:23:32, benwells wrote: > I wonder if this could be encapsulated into some sort of ScopedInputTracker, > which in its constructor sets up the listening and in its destructor kills it. > That way you wouldn't need callbacks_added_, the existence of your > ScopedInputTracker would be signal enough. I'm concerned whether the additional clarity comes at the overhead of continually constructing and destructing the object. https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:98: if (ignore_page_) On 2015/09/16 00:23:32, benwells wrote: > I don't really like having this timer running all the time. Can we just start it > after movement / when Chrome isn't idle? I have changed this design such that the timer is now run after an input callback is fired. When the timer finishes, the input callbacks are re-registered. The difference is that instead of checking for user input in discrete windows, this now enforces that user input must occur some amount of time apart to record more engagement. https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_unittest.cc (left): https://codereview.chromium.org/1338603002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_unittest.cc:327: TEST_F(SiteEngagementServiceTest, ScoreIncrementsOnPageRequest) { On 2015/09/16 00:23:32, benwells wrote: > This test makes sure the whole system works together. You need to replace it > with something (or if navigation tracking stays, which I think it should, also > add something else to ensure the user input stuff works properly as a whole). I'm working on adding a browser/interactive UI test as well, probably in a separate CL. Do you want it as part of this CL?
PTAL - now with a browser test. Instead of testing the timing aspect, I'm manually generating events. This is much more robust than trying to test the waiting and callback registration.
Input tracking is now factored out into a separate inner class in site engagement helper.
https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:17: double gSecondsBetweenUserInputCheck = 10; The more common style seems to be g_seconds_between_user_input_check. https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:46: helper_->RecordUserInput(); Does this mean the first input event can get recorded as soon as tracking is started? I thought we wanted it to happen at the end of the first timer period. https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:123: record_engagement_ = params.url.SchemeIsHTTPOrHTTPS(); Needing this bool is a bit unfortunate. Ideally, we'd be able to ask the input tracker whether or not it's tracking. Can we use WebContents::FromRenderViewHost() to get the WC and then use WC::GetLastCommittedURL() to check the origin on the input tracker side? Then we can have an IsTracking() method on InputTracker. https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:125: if (record_engagement_) { Invert and early return. https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:166: void SiteEngagementHelper::SetSecondsBetweenUserInputCheck(double seconds) { SetSecondsBetweenUserInputCheckForTestsing https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:169: // static https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.h (right): https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.h:36: static void DisableCallbackRegistrationForTesting(); If these are for testing and the test class is friended, these may as well be private. https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:172: ui::PageTransition transition) { Why is |transition| here? https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_browsertest.cc (right): https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:31: switches::kDisableSiteEngagementService); Why is this necessary? It probably needs a comment. https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:32: SiteEngagementHelper::SetSecondsBetweenUserInputCheck(10); This time change doesn't affect anything at the moment right? https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:33: SiteEngagementHelper::DisableCallbackRegistrationForTesting(); // This prevents other browser tests from generating spurious events on our web contents. (Is that right?) https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:93: HandleKeyPress(helper, ui::VKEY_J); There needs to be some test for the timer accumulation. You can pass a MockTimer to the InputTracker to manually Fire the timer to trigger the score write. It would also be good to write a test for when the timer is supposed to be running (e.g show, hide, idling, after navigate, and various sequences of these)
PTAL - now with additional interactive ui test checking user input and timer triggering. https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:17: double gSecondsBetweenUserInputCheck = 10; On 2015/09/22 02:57:20, calamity wrote: > The more common style seems to be g_seconds_between_user_input_check. Done. https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:46: helper_->RecordUserInput(); On 2015/09/22 02:57:21, calamity wrote: > Does this mean the first input event can get recorded as soon as tracking is > started? I thought we wanted it to happen at the end of the first timer period. I can put in a one-shot timer to add that effect back. https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:123: record_engagement_ = params.url.SchemeIsHTTPOrHTTPS(); On 2015/09/22 02:57:21, calamity wrote: > Needing this bool is a bit unfortunate. Ideally, we'd be able to ask the input > tracker whether or not it's tracking. > > Can we use WebContents::FromRenderViewHost() to get the WC and then use > WC::GetLastCommittedURL() to check the origin on the input tracker side? > > Then we can have an IsTracking() method on InputTracker. The issue I have with this is that it's the Helper which definitely knows what the origin is. The input tracker may be sleeping or disabled during a navigation, and it's up to the helper to tell the tracker that we've navigated and it's time to wake up. https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:125: if (record_engagement_) { On 2015/09/22 02:57:21, calamity wrote: > Invert and early return. Done. https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:166: void SiteEngagementHelper::SetSecondsBetweenUserInputCheck(double seconds) { On 2015/09/22 02:57:20, calamity wrote: > SetSecondsBetweenUserInputCheckForTestsing I'm assuming that this may be Finch'ed at some point in the future, so I left off the ForTesting. https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:169: On 2015/09/22 02:57:20, calamity wrote: > // static Done. https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.h (right): https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.h:36: static void DisableCallbackRegistrationForTesting(); On 2015/09/22 02:57:21, calamity wrote: > If these are for testing and the test class is friended, these may as well be > private. Done. https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:172: ui::PageTransition transition) { On 2015/09/22 02:57:21, calamity wrote: > Why is |transition| here? It's for when/if we want to count different navigation types differently (generated versus direct). https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_browsertest.cc (right): https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:31: switches::kDisableSiteEngagementService); On 2015/09/22 02:57:21, calamity wrote: > Why is this necessary? It probably needs a comment. Done. https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:32: SiteEngagementHelper::SetSecondsBetweenUserInputCheck(10); On 2015/09/22 02:57:21, calamity wrote: > This time change doesn't affect anything at the moment right? With the mock timer in for the test, not any more. https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:33: SiteEngagementHelper::DisableCallbackRegistrationForTesting(); On 2015/09/22 02:57:21, calamity wrote: > // This prevents other browser tests from generating spurious events on our web > contents. > > (Is that right?) It was primarily to address the case where I'm running the tests on my machine, and any input over the test window gets detected. Running the tests with xvfb solves this, but I'd like to keep the safety measure there because I can foresee it being an issue.. https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:93: HandleKeyPress(helper, ui::VKEY_J); On 2015/09/22 02:57:21, calamity wrote: > There needs to be some test for the timer accumulation. You can pass a MockTimer > to the InputTracker to manually Fire the timer to trigger the score write. > > It would also be good to write a test for when the timer is supposed to be > running (e.g show, hide, idling, after navigate, and various sequences of these) > Done.
I haven't done a full review yet, but fwiw I like the new structure.
https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:46: helper_->RecordUserInput(); On 2015/09/23 00:06:44, dominickn wrote: > On 2015/09/22 02:57:21, calamity wrote: > > Does this mean the first input event can get recorded as soon as tracking is > > started? I thought we wanted it to happen at the end of the first timer > period. > > I can put in a one-shot timer to add that effect back. Hmm, maybe we can save that for a follow-up, to keep this CL slightly less hairy. Although, you may to rename timer_ to pause_timer_ or similar to make adding the one-shot easier. https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:123: record_engagement_ = params.url.SchemeIsHTTPOrHTTPS(); On 2015/09/23 00:06:44, dominickn wrote: > On 2015/09/22 02:57:21, calamity wrote: > > Needing this bool is a bit unfortunate. Ideally, we'd be able to ask the input > > tracker whether or not it's tracking. > > > > Can we use WebContents::FromRenderViewHost() to get the WC and then use > > WC::GetLastCommittedURL() to check the origin on the input tracker side? > > > > Then we can have an IsTracking() method on InputTracker. > > The issue I have with this is that it's the Helper which definitely knows what > the origin is. The input tracker may be sleeping or disabled during a > navigation, and it's up to the helper to tell the tracker that we've navigated > and it's time to wake up. Acknowledged. https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:166: void SiteEngagementHelper::SetSecondsBetweenUserInputCheck(double seconds) { On 2015/09/23 00:06:44, dominickn wrote: > On 2015/09/22 02:57:20, calamity wrote: > > SetSecondsBetweenUserInputCheckForTestsing > > I'm assuming that this may be Finch'ed at some point in the future, so I left > off the ForTesting. Acknowledged. https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:172: ui::PageTransition transition) { On 2015/09/23 00:06:44, dominickn wrote: > On 2015/09/22 02:57:21, calamity wrote: > > Why is |transition| here? > > It's for when/if we want to count different navigation types differently > (generated versus direct). Acknowledged. https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_browsertest.cc (right): https://codereview.chromium.org/1338603002/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:33: SiteEngagementHelper::DisableCallbackRegistrationForTesting(); On 2015/09/23 00:06:44, dominickn wrote: > On 2015/09/22 02:57:21, calamity wrote: > > // This prevents other browser tests from generating spurious events on our > web > > contents. > > > > (Is that right?) > > It was primarily to address the case where I'm running the tests on my machine, > and any input over the test window gets detected. Running the tests with xvfb > solves this, but I'd like to keep the safety measure there because I can foresee > it being an issue.. Acknowledged. https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.h (right): https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.h:71: // gSecondsBetweenUserInputCheck seconds. nit: update var https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:210: std::map<GURL, int> SiteEngagementService::GetScoreMap() { I think your rebase is going to want to change this to a double as well.. https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_browsertest.cc (right): https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:330: timer->Fire(); Simulate an extra event before the timer fires to ensure that PauseTracking() works correctly and further input isn't recorded. https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:336: GURL url("https://www.google.com/"); nit: url1 https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:343: SiteEngagementHelper* helper = CreateHelper(web_contents, true); Wait, doesn't this reintroduce the flakiness you were worried about? I thought the plan was to just to call HandleMouseEvent/HandleKeyPress to activate/deactivate the timer. https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:373: // Timer should still be stopped on re-focus. Hmm. So does this mean that showing a popup in a new tab, which the user then closes to return to the original site nets a second engagement event? I guess adding the StartTracking 1-shot timer will fix this. https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:374: browser()->tab_strip_model()->GetWebContentsAt(0)->WasShown(); May as well call TasbStripModel::ActivateTabAt(0). Or would that require more run loop madness (if so, nevermind).
https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.h (right): https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.h:71: // gSecondsBetweenUserInputCheck seconds. On 2015/09/24 03:50:53, calamity wrote: > nit: update var Done. https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:210: std::map<GURL, int> SiteEngagementService::GetScoreMap() { On 2015/09/24 03:50:53, calamity wrote: > I think your rebase is going to want to change this to a double as well.. Done. https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_browsertest.cc (right): https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:330: timer->Fire(); On 2015/09/24 03:50:53, calamity wrote: > Simulate an extra event before the timer fires to ensure that PauseTracking() > works correctly and further input isn't recorded. Done, though I'm not sure we can properly wait and observe this (which is why I didn't have it in before). https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:336: GURL url("https://www.google.com/"); On 2015/09/24 03:50:53, calamity wrote: > nit: url1 Done. https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:343: SiteEngagementHelper* helper = CreateHelper(web_contents, true); On 2015/09/24 03:50:53, calamity wrote: > Wait, doesn't this reintroduce the flakiness you were worried about? > > I thought the plan was to just to call HandleMouseEvent/HandleKeyPress to > activate/deactivate the timer. I've added another test which does just call Handle functions, but because these functions directly call the callback rather than dispatching user input, they can't test the input during timer running case (that's done below now). I'm less concerned now because all of these tests have been moved to interactive_ui_tests and I diagnosed why content::SimulateKeyPress wasn't doing anything. However, it remains true that running these last tests outside of xvfb and clicking or moving the mouse induces bad results. We're going to have to think about getting tests for this on Android as well in a follow up CL... https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:373: // Timer should still be stopped on re-focus. On 2015/09/24 03:50:53, calamity wrote: > Hmm. So does this mean that showing a popup in a new tab, which the user then > closes to return to the original site nets a second engagement event? > > I guess adding the StartTracking 1-shot timer will fix this. This case will be handled by adding a 1-shot timer to start the tracking (later CL) https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:374: browser()->tab_strip_model()->GetWebContentsAt(0)->WasShown(); On 2015/09/24 03:50:53, calamity wrote: > May as well call TasbStripModel::ActivateTabAt(0). Or would that require more > run loop madness (if so, nevermind). Done.
https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_browsertest.cc (right): https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:330: timer->Fire(); On 2015/09/24 06:52:07, dominickn wrote: > On 2015/09/24 03:50:53, calamity wrote: > > Simulate an extra event before the timer fires to ensure that PauseTracking() > > works correctly and further input isn't recorded. > > Done, though I'm not sure we can properly wait and observe this (which is why I > didn't have it in before). Hmm. True. Can you just add a test accessor to callback_added_ and then make sure that's false? https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:343: SiteEngagementHelper* helper = CreateHelper(web_contents, true); On 2015/09/24 06:52:07, dominickn wrote: > On 2015/09/24 03:50:53, calamity wrote: > > Wait, doesn't this reintroduce the flakiness you were worried about? > > > > I thought the plan was to just to call HandleMouseEvent/HandleKeyPress to > > activate/deactivate the timer. > > I've added another test which does just call Handle functions, but because these > functions directly call the callback rather than dispatching user input, they > can't test the input during timer running case (that's done below now). Ah, right. Because the callbacks are only called by the WebContents system. > > I'm less concerned now because all of these tests have been moved to > interactive_ui_tests and I diagnosed why content::SimulateKeyPress wasn't doing > anything. However, it remains true that running these last tests outside of xvfb > and clicking or moving the mouse induces bad results. This is acceptable. > > We're going to have to think about getting tests for this on Android as well in > a follow up CL... https://codereview.chromium.org/1338603002/diff/260001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:373: // Timer should still be stopped on re-focus. On 2015/09/24 06:52:07, dominickn wrote: > On 2015/09/24 03:50:53, calamity wrote: > > Hmm. So does this mean that showing a popup in a new tab, which the user then > > closes to return to the original site nets a second engagement event? > > > > I guess adding the StartTracking 1-shot timer will fix this. > > This case will be handled by adding a 1-shot timer to start the tracking (later > CL) Acknowledged.
lgtm
jdduke@chromium.org changed reviewers: + jdduke@chromium.org
https://codereview.chromium.org/1338603002/diff/280001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1338603002/diff/280001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:61: event.type == blink::WebInputEvent::MouseMove || I'm not sure MouseMove's without an active button should qualify as engagement (on many platforms mousemoves are noisy and rapid). If the sampling here is intentionally lossy and sparse (once per 10 seconds), just listening to a mousedown is probably sufficient?
(Sorry for the late drive-by, just trying to understand the system better :) https://codereview.chromium.org/1338603002/diff/280001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1338603002/diff/280001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:138: FOR_EACH_OBSERVER(Observer, observer_list_, OnInputRecorded(this)); Any idea what the potential costs are here? Is it O(microseconds), O(milliseconds)? If it's potentially expensive could we add trace instrumentation for this function?
On 2015/09/24 15:13:26, jdduke wrote: > https://codereview.chromium.org/1338603002/diff/280001/chrome/browser/engagem... > File chrome/browser/engagement/site_engagement_helper.cc (right): > > https://codereview.chromium.org/1338603002/diff/280001/chrome/browser/engagem... > chrome/browser/engagement/site_engagement_helper.cc:61: event.type == > blink::WebInputEvent::MouseMove || > I'm not sure MouseMove's without an active button should qualify as engagement > (on many platforms mousemoves are noisy and rapid). If the sampling here is > intentionally lossy and sparse (once per 10 seconds), just listening to a > mousedown is probably sufficient? I agree mouse move's shouldn't count as engagement. Gestures on android should (e.g. scrolling the page) but on desktop just moving the mouse over a page shouldn't count.
On 2015/09/24 22:56:07, benwells wrote: > On 2015/09/24 15:13:26, jdduke wrote: > > > https://codereview.chromium.org/1338603002/diff/280001/chrome/browser/engagem... > > File chrome/browser/engagement/site_engagement_helper.cc (right): > > > > > https://codereview.chromium.org/1338603002/diff/280001/chrome/browser/engagem... > > chrome/browser/engagement/site_engagement_helper.cc:61: event.type == > > blink::WebInputEvent::MouseMove || > > I'm not sure MouseMove's without an active button should qualify as engagement > > (on many platforms mousemoves are noisy and rapid). If the sampling here is > > intentionally lossy and sparse (once per 10 seconds), just listening to a > > mousedown is probably sufficient? > > I agree mouse move's shouldn't count as engagement. Gestures on android should > (e.g. scrolling the page) but on desktop just moving the mouse over a page > shouldn't count. Yep, I expect if/when we integrate gestures, you'll need only listen to Tap + ScrollBegin gestures. Of course, that excludes a whole different class of events (which is raw touch events that are handled by the page). For that, I suppose you could just listen to touchstartm, which might be sufficient here (we always forward touch events through the RWH). I think CrOS/Aura already has the concept of a "presence" monitor that monitors input interaction on a page, but I don't know much about it. Might be worth just sanity checking that to see if it's something we ought to be leveraging (though i don't think it works on Android...).
RE the aura presence monitor: do you of someone I can contact to talk to about it? I've done a few passes through and hadn't found anything that exactly fit our needs yet. : ) https://codereview.chromium.org/1338603002/diff/280001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1338603002/diff/280001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:61: event.type == blink::WebInputEvent::MouseMove || On 2015/09/24 15:13:25, jdduke wrote: > I'm not sure MouseMove's without an active button should qualify as engagement > (on many platforms mousemoves are noisy and rapid). If the sampling here is > intentionally lossy and sparse (once per 10 seconds), just listening to a > mousedown is probably sufficient? Done, though I think it's also important to listen to wheel events here for the case where people are reading a long document, and scrolling through it without clicking or typing. https://codereview.chromium.org/1338603002/diff/280001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:138: FOR_EACH_OBSERVER(Observer, observer_list_, OnInputRecorded(this)); On 2015/09/24 15:17:35, jdduke wrote: > Any idea what the potential costs are here? Is it O(microseconds), > O(milliseconds)? > > If it's potentially expensive could we add trace instrumentation for this > function? Do you mean the observer or the cost of recording user input? For the observer, that's only used for testing. There will be a follow-up CL posted today adding UMA metrics to try and work out how often input is recorded, how quickly users accumulate engagement, and whether we need adjust how often we're capturing input. These should hopefully help us fine-tune what we're doing.
On 2015/09/25 00:08:57, dominickn wrote: > RE the aura presence monitor: do you of someone I can contact to talk to about > it? I've done a few passes through and hadn't found anything that exactly fit > our needs yet. : ) > Took me a while to remember the name, it's the UserActivityDetector: https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/user_activ... > https://codereview.chromium.org/1338603002/diff/280001/chrome/browser/engagem... > File chrome/browser/engagement/site_engagement_helper.cc (right): > > https://codereview.chromium.org/1338603002/diff/280001/chrome/browser/engagem... > chrome/browser/engagement/site_engagement_helper.cc:61: event.type == > blink::WebInputEvent::MouseMove || > On 2015/09/24 15:13:25, jdduke wrote: > > I'm not sure MouseMove's without an active button should qualify as engagement > > (on many platforms mousemoves are noisy and rapid). If the sampling here is > > intentionally lossy and sparse (once per 10 seconds), just listening to a > > mousedown is probably sufficient? > > Done, though I think it's also important to listen to wheel events here for the > case where people are reading a long document, and scrolling through it without > clicking or typing. > Yep, wheel is fine. > https://codereview.chromium.org/1338603002/diff/280001/chrome/browser/engagem... > chrome/browser/engagement/site_engagement_helper.cc:138: > FOR_EACH_OBSERVER(Observer, observer_list_, OnInputRecorded(this)); > On 2015/09/24 15:17:35, jdduke wrote: > > Any idea what the potential costs are here? Is it O(microseconds), > > O(milliseconds)? > > > > If it's potentially expensive could we add trace instrumentation for this > > function? > > Do you mean the observer or the cost of recording user input? For the observer, > that's only used for testing. > > There will be a follow-up CL posted today adding UMA metrics to try and work out > how often input is recorded, how quickly users accumulate engagement, and > whether we need adjust how often we're capturing input. These should hopefully > help us fine-tune what we're doing. The cost of everything in this function. A couple UMA entries is fine, I guess the question mostly comes down to the cost of SiteEngagementService::AddPoints(const GURL& url, double points)? We'll want some trace instrumentation regardless I think. Perhaps add a trace entry at the top of this function (or at some top level in the SiteEngagementService with category "input", unless you have a different category for the service).
On 2015/09/25 00:19:44, jdduke wrote: > On 2015/09/25 00:08:57, dominickn wrote: > > RE the aura presence monitor: do you of someone I can contact to talk to about > > it? I've done a few passes through and hadn't found anything that exactly fit > > our needs yet. : ) > > > > Took me a while to remember the name, it's the UserActivityDetector: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/user_activ... Thanks for pointing this out to me. I'm currently experimenting to confirm, but judging from the code and the log it looks like a feature that is only available with X11 on Linux/Aura, so it doesn't fit our needs here. > The cost of everything in this function. A couple UMA entries is fine, I guess > the question mostly comes down to the cost of > SiteEngagementService::AddPoints(const GURL& url, double points)? We'll want > some trace instrumentation regardless I think. Perhaps add a trace entry at the > top of this function (or at some top level in the SiteEngagementService with > category "input", unless you have a different category for the service). I've added a TRACE_EVENT0 to the top of RecordUserInput().
On 2015/09/25 00:19:44, jdduke wrote: > On 2015/09/25 00:08:57, dominickn wrote: > > RE the aura presence monitor: do you of someone I can contact to talk to about > > it? I've done a few passes through and hadn't found anything that exactly fit > > our needs yet. : ) > > > > Took me a while to remember the name, it's the UserActivityDetector: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/user_activ... Thanks for pointing this out to me. I'm currently experimenting to confirm, but judging from the code and the log it looks like a feature that is only available with X11 on Linux/Aura, so it doesn't fit our needs here. > The cost of everything in this function. A couple UMA entries is fine, I guess > the question mostly comes down to the cost of > SiteEngagementService::AddPoints(const GURL& url, double points)? We'll want > some trace instrumentation regardless I think. Perhaps add a trace entry at the > top of this function (or at some top level in the SiteEngagementService with > category "input", unless you have a different category for the service). I've added a TRACE_EVENT0 to the top of RecordUserInput().
https://codereview.chromium.org/1338603002/diff/320001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.h (right): https://codereview.chromium.org/1338603002/diff/320001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.h:22: // origins based on the time spent on site. Nit: this comment is too restrictive. https://codereview.chromium.org/1338603002/diff/320001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.h:25: // some discrete time unit. If there is user input, then record a positive site Nit: move this into the InputTracker class level comment. https://codereview.chromium.org/1338603002/diff/320001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.h:29: // for time on site: e.g. watching videos. Nit: I'd remove this TODO as we don't have clear ideas about what to do. https://codereview.chromium.org/1338603002/diff/320001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.h:117: scoped_ptr<InputTracker> input_tracker_; Is there any need for this to be a scoped_ptr, versus a plain old member? https://codereview.chromium.org/1338603002/diff/320001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_browsertest.cc (right): https://codereview.chromium.org/1338603002/diff/320001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:54: SiteEngagementHelper* CreateHelper(content::WebContents* web_contents, Since you create the helper here, can you have a TestSiteEngagementHelper instead of using a regalar one? That would let you move most of the weird test only stuff out of the regular SiteEngagementHelper.
Patchset #19 (id:360001) has been deleted
https://codereview.chromium.org/1338603002/diff/320001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.h (right): https://codereview.chromium.org/1338603002/diff/320001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.h:22: // origins based on the time spent on site. On 2015/09/29 00:24:12, benwells wrote: > Nit: this comment is too restrictive. Done. https://codereview.chromium.org/1338603002/diff/320001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.h:25: // some discrete time unit. If there is user input, then record a positive site On 2015/09/29 00:24:12, benwells wrote: > Nit: move this into the InputTracker class level comment. Done. https://codereview.chromium.org/1338603002/diff/320001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.h:29: // for time on site: e.g. watching videos. On 2015/09/29 00:24:12, benwells wrote: > Nit: I'd remove this TODO as we don't have clear ideas about what to do. Done. https://codereview.chromium.org/1338603002/diff/320001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.h:117: scoped_ptr<InputTracker> input_tracker_; On 2015/09/29 00:24:12, benwells wrote: > Is there any need for this to be a scoped_ptr, versus a plain old member? Done. https://codereview.chromium.org/1338603002/diff/320001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_browsertest.cc (right): https://codereview.chromium.org/1338603002/diff/320001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:54: SiteEngagementHelper* CreateHelper(content::WebContents* web_contents, On 2015/09/29 00:24:12, benwells wrote: > Since you create the helper here, can you have a TestSiteEngagementHelper > instead of using a regalar one? That would let you move most of the weird test > only stuff out of the regular SiteEngagementHelper. Done, though it does make this test pretty ugly.
https://codereview.chromium.org/1338603002/diff/380001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_browsertest.cc (right): https://codereview.chromium.org/1338603002/diff/380001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:31: class Observer { Why do you need all this observer stuff still? There is only ever one, and all it ever does is quit the message loop, so you should be able to do this in a much simpler way.
Thanks for the feedback. I've now sacrificed the observer and moved the quit closure directly into the TestSiteEngagementHelper. https://codereview.chromium.org/1338603002/diff/380001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_browsertest.cc (right): https://codereview.chromium.org/1338603002/diff/380001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_browsertest.cc:31: class Observer { On 2015/09/29 01:48:39, benwells wrote: > Why do you need all this observer stuff still? There is only ever one, and all > it ever does is quit the message loop, so you should be able to do this in a > much simpler way. Done.
lgtm
The CQ bit was checked by dominickn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from calamity@chromium.org Link to the patchset: https://codereview.chromium.org/1338603002/#ps400001 (title: "Removing observer interface")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1338603002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1338603002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by dominickn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1338603002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1338603002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by dominickn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1338603002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1338603002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by dominickn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1338603002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1338603002/400001
On 2015/09/29 08:10:00, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1338603002/400001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1338603002/400001 The repeated android_clang_dbg_recipe failures seem to stem from an issue with the CQ: https://code.google.com/p/chromium/issues/detail?id=537033
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
https://codereview.chromium.org/1338603002/diff/320001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1338603002/diff/320001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:38: if (!settings) Can settings legitimately be null? None of the callers appear to check the return value for null. https://codereview.chromium.org/1338603002/diff/400001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1338603002/diff/400001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:55: const blink::WebMouseEvent& event) { So, WebMouseEvents won't actually include wheel events. Only WebMouseWheelEvents will have type "WebInputEvent::MouseWheel". One thought here would be to hook into the more generic "OnUserGesture" signal, which is fired from RenderWidgetHostImpl::FilterInputEvent. Then you need only add a WebContentsObserver and listen to DidGetUserGesture. That should get you both mouse click and gesture hooks, what do you think?
https://codereview.chromium.org/1338603002/diff/320001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1338603002/diff/320001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:38: if (!settings) On 2015/09/29 15:20:36, jdduke wrote: > Can settings legitimately be null? None of the callers appear to check the > return value for null. I think it would be a fairly oblique case where settings is null. However, it's a possibility. This method is used in a lot of places outside of this CL. I'll put up another one to clean up this usage. https://codereview.chromium.org/1338603002/diff/400001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1338603002/diff/400001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_helper.cc:55: const blink::WebMouseEvent& event) { On 2015/09/29 15:20:36, jdduke wrote: > So, WebMouseEvents won't actually include wheel events. Only WebMouseWheelEvents > will have type "WebInputEvent::MouseWheel". > > One thought here would be to hook into the more generic "OnUserGesture" signal, > which is fired from RenderWidgetHostImpl::FilterInputEvent. Then you need only > add a WebContentsObserver and listen to DidGetUserGesture. > > That should get you both mouse click and gesture hooks, what do you think? I had a look at DidGetUserGesture as the first thing for this CL, but the comments for the method in web_contents_observer.h and render_widget_host_impl.h are very misleading (one claims that it is fired when the next navigation is triggered by a user gesture, and the other claims it fires for mouse clicks and enter/space keys). That's why I didn't even try and use it. Perhaps those comments should be tidied up. On testing, DidGetUserGesture() fires when you start scrolling on Android, but it doesn't trigger again until you take your finger off the touch screen and tap again elsewhere. However, I think it's much noisier than we want: DidGetUserGesture() is called on every single click or tap. With our current design, we deregister the listening callbacks for a set period once we have handled an event, so there is zero overhead most of the time. If we override DidGetUserGesture(), that's no longer an option, and we'll need to manually ignore excess clicks and taps, incurring more overhead. I think having the ability to register and deregister callbacks is a better design.
The CQ bit was checked by dominickn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1338603002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1338603002/400001
Message was sent while issue was closed.
Committed patchset #20 (id:400001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/f3226a4f280b20eb00024d724b9e70ec1edc2e33 Cr-Commit-Position: refs/heads/master@{#351465}
Message was sent while issue was closed.
On 2015/09/30 00:22:52, dominickn wrote: > https://codereview.chromium.org/1338603002/diff/320001/chrome/browser/engagem... > File chrome/browser/engagement/site_engagement_service.cc (right): > > https://codereview.chromium.org/1338603002/diff/320001/chrome/browser/engagem... > chrome/browser/engagement/site_engagement_service.cc:38: if (!settings) > On 2015/09/29 15:20:36, jdduke wrote: > > Can settings legitimately be null? None of the callers appear to check the > > return value for null. > > I think it would be a fairly oblique case where settings is null. However, it's > a possibility. This method is used in a lot of places outside of this CL. I'll > put up another one to clean up this usage. > > https://codereview.chromium.org/1338603002/diff/400001/chrome/browser/engagem... > File chrome/browser/engagement/site_engagement_helper.cc (right): > > https://codereview.chromium.org/1338603002/diff/400001/chrome/browser/engagem... > chrome/browser/engagement/site_engagement_helper.cc:55: const > blink::WebMouseEvent& event) { > On 2015/09/29 15:20:36, jdduke wrote: > > So, WebMouseEvents won't actually include wheel events. Only > WebMouseWheelEvents > > will have type "WebInputEvent::MouseWheel". > > > > One thought here would be to hook into the more generic "OnUserGesture" > signal, > > which is fired from RenderWidgetHostImpl::FilterInputEvent. Then you need only > > add a WebContentsObserver and listen to DidGetUserGesture. > > > > That should get you both mouse click and gesture hooks, what do you think? > > I had a look at DidGetUserGesture as the first thing for this CL, but the > comments for the method in web_contents_observer.h and render_widget_host_impl.h > are very misleading (one claims that it is fired when the next navigation is > triggered by a user gesture, and the other claims it fires for mouse clicks and > enter/space keys). That's why I didn't even try and use it. Perhaps those > comments should be tidied up. > > On testing, DidGetUserGesture() fires when you start scrolling on Android, but > it doesn't trigger again until you take your finger off the touch screen and tap > again elsewhere. However, I think it's much noisier than we want: > DidGetUserGesture() is called on every single click or tap. With our current > design, we deregister the listening callbacks for a set period once we have > handled an event, so there is zero overhead most of the time. If we override > DidGetUserGesture(), that's no longer an option, and we'll need to manually > ignore excess clicks and taps, incurring more overhead. I think having the > ability to register and deregister callbacks is a better design. Are you in such a rush to land this that we couldn't this further?
Message was sent while issue was closed.
This feature is disabled by default, and this CL is blocking others which will let us start to experiment and get UMA data from the dev channel. Without some solid metrics, it's a bit difficult to reason about the implementation - I think this discussion will be ongoing well beyond this CL. This is very much the early days for site engagement. I'll make sure to keep you in the loop - thanks for the helpful questions so far. :)
Message was sent while issue was closed.
A revert of this CL (patchset #20 id:400001) has been created in https://codereview.chromium.org/1376143003/ by phoglund@chromium.org. The reason for reverting is: Appears to break SiteEngagementServiceBrowserTest.SimulateInput on at least Mac 10.10: see https://build.chromium.org/p/chromium.mac/builders/Mac10.10%20Tests/builds/3034..
Message was sent while issue was closed.
On 2015/09/30 07:43:31, phoglund_chrome wrote: > A revert of this CL (patchset #20 id:400001) has been created in > https://codereview.chromium.org/1376143003/ by mailto:phoglund@chromium.org. > > The reason for reverting is: Appears to break > SiteEngagementServiceBrowserTest.SimulateInput on at least Mac 10.10: see > https://build.chromium.org/p/chromium.mac/builders/Mac10.10%20Tests/builds/3034.. Seems the input simulation doesn't work and times out on Mac 10.10, even though it passed all the trybots? I'll investigate tomorrow...
Patchset #21 (id:420001) has been deleted
The CQ bit was checked by dominickn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from calamity@chromium.org, benwells@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1338603002/#ps440001 (title: "Remove flaky input simulation tests and combine callback and timer testing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1338603002/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1338603002/440001
Message was sent while issue was closed.
Committed patchset #21 (id:440001)
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/5eceefb54758e691c7857c4feff111f7db33ef6d Cr-Commit-Position: refs/heads/master@{#351704} |
