Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1373363005:
Add a delay to Site Engagement tracking after navigation. (Closed)
DescriptionAdd a delay to Site Engagement tracking after navigation.
This CL adds a timer to the Site Engagement Service that delays
input tracking for 10 seconds after a navigation. This will prevent
immediately closed tabs from accumulating engagement.
This CL also adds a 5 second delay after showing a tab to
prevent rapid tab switching from accumulating excess engagement.
BUG=464234
Committed: https://crrev.com/43716890b5ad407479926caff884c3737e9e5908
Cr-Commit-Position: refs/heads/master@{#353010}
Patch Set 1 #
Total comments: 10
Patch Set 2 : address comments, refactor #
Total comments: 8
Patch Set 3 : address comments #
Total comments: 2
Patch Set 4 : address comment #
Total comments: 4
Patch Set 5 : address comment #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 16 (4 generated)
calamity@chromium.org changed reviewers: + dominickn@chromium.org
https://codereview.chromium.org/1373363005/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_helper.cc (left): https://codereview.chromium.org/1373363005/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_helper.cc:188: // static Is there a particular reason for removing this function? Arguably https://codereview.chromium.org/1373453002 should allow this value to be changed via field trial - I'll add that if you agree (and in that case, the variable name should stay the same). https://codereview.chromium.org/1373363005/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1373363005/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_helper.cc:195: input_tracker_.StartTracking(web_contents()->GetRenderViewHost()); Is it correct that WasShown() should immediately trigger input tracking? If you navigate somewhere and have a pop up trigger and immediately closed, then input tracking would start immediately. Additionally, I've managed to trigger DidNavigate prior to WasShown in testing (often when opening the browser to the NTP and then navigating somewhere from there). I think WasShown() should set up the navigation timer, perhaps with a smaller timeout. Then you could rename the navigation timer to "engagament timer" or something like that. https://codereview.chromium.org/1373363005/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_helper.h (right): https://codereview.chromium.org/1373363005/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_helper.h:77: bool callbacks_added_; Nit: rename to is_tracking_ to match the function name. https://codereview.chromium.org/1373363005/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_helper.h:88: bool ShouldRecordEngagement(); Nit: remove this declaration since the function is gone. https://codereview.chromium.org/1373363005/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_service_browsertest.cc (right): https://codereview.chromium.org/1373363005/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service_browsertest.cc:331: EXPECT_FALSE(navigation_timer->IsRunning()); I think we need a test for tracking after a navigation to a new page in an existing tab where tracking is already enabled/disabled. Also a test where you open a background tab with tracking enabled (make sure tracking continues) would be good.
Patchset #2 (id:10001) has been deleted
Okay, I refactored the whole thing. Sorry about that. We can just use the same timer in the input tracker. I don't think it's too confusing and it makes the timer logic easier to reason about. LMK what you think. https://codereview.chromium.org/1373363005/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_helper.cc (left): https://codereview.chromium.org/1373363005/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_helper.cc:188: // static On 2015/10/06 00:04:41, dominickn wrote: > Is there a particular reason for removing this function? Arguably > https://codereview.chromium.org/1373453002 should allow this value to be changed > via field trial - I'll add that if you agree (and in that case, the variable > name should stay the same). Yeah, ok. https://codereview.chromium.org/1373363005/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1373363005/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_helper.cc:195: input_tracker_.StartTracking(web_contents()->GetRenderViewHost()); On 2015/10/06 00:04:41, dominickn wrote: > Is it correct that WasShown() should immediately trigger input tracking? If you > navigate somewhere and have a pop up trigger and immediately closed, then input > tracking would start immediately. Additionally, I've managed to trigger > DidNavigate prior to WasShown in testing (often when opening the browser to the > NTP and then navigating somewhere from there). > > I think WasShown() should set up the navigation timer, perhaps with a smaller > timeout. Then you could rename the navigation timer to "engagament timer" or > something like that. Agreed. https://codereview.chromium.org/1373363005/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_helper.h (right): https://codereview.chromium.org/1373363005/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_helper.h:77: bool callbacks_added_; On 2015/10/06 00:04:41, dominickn wrote: > Nit: rename to is_tracking_ to match the function name. Done. https://codereview.chromium.org/1373363005/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_helper.h:88: bool ShouldRecordEngagement(); On 2015/10/06 00:04:41, dominickn wrote: > Nit: remove this declaration since the function is gone. Done. https://codereview.chromium.org/1373363005/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_service_browsertest.cc (right): https://codereview.chromium.org/1373363005/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service_browsertest.cc:331: EXPECT_FALSE(navigation_timer->IsRunning()); On 2015/10/06 00:04:42, dominickn wrote: > I think we need a test for tracking after a navigation to a new page in an > existing tab where tracking is already enabled/disabled. > > Also a test where you open a background tab with tracking enabled (make sure > tracking continues) would be good. Done. https://codereview.chromium.org/1373363005/diff/30001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1373363005/diff/30001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:110: return; Is this check necessary?
https://codereview.chromium.org/1373363005/diff/30001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1373363005/diff/30001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:110: return; On 2015/10/06 03:39:56, calamity wrote: > Is this check necessary? web_contents() can return null (e.g. the web contents is in the process of being destroyed). This is to catch the race where the input timer fires and callbacks are added just as the web contents is being destructed. https://codereview.chromium.org/1373363005/diff/30001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:182: } I think this means that the pause timer is reset to 0 each time the render view host changes, including if the pause timer is already running (with time left on the clock). I think is_tracking_ should replace callbacks_added_ (i.e. only true if they input is truly being recorded). Then you could have: if (pause_timer_.IsRunning()) { host_ = new_host; } else if (is_tracking_) { input_tracker_.StopTracking(); input_tracker_.StartTracking(new_host, 0); } Then you only need one bool instead of two in the class. https://codereview.chromium.org/1373363005/diff/30001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_helper.h (right): https://codereview.chromium.org/1373363005/diff/30001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.h:88: bool callbacks_added_; I don't think both of these bools are necessary. Can you use pause_timer_.IsRunning() in StartTracking(), and then remove callbacks_added_ and replace it with is_tracking_? Also see other comment.
https://codereview.chromium.org/1373363005/diff/30001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1373363005/diff/30001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:110: return; On 2015/10/06 04:11:45, dominickn wrote: > On 2015/10/06 03:39:56, calamity wrote: > > Is this check necessary? > > web_contents() can return null (e.g. the web contents is in the process of being > destroyed). This is to catch the race where the input timer fires and callbacks > are added just as the web contents is being destructed. Acknowledged. https://codereview.chromium.org/1373363005/diff/30001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:182: } On 2015/10/06 04:11:45, dominickn wrote: > I think this means that the pause timer is reset to 0 each time the render view > host changes, including if the pause timer is already running (with time left on > the clock). I think is_tracking_ should replace callbacks_added_ (i.e. only true > if they input is truly being recorded). Then you could have: > > if (pause_timer_.IsRunning()) { > host_ = new_host; > } > else if (is_tracking_) { > input_tracker_.StopTracking(); > input_tracker_.StartTracking(new_host, 0); > } > > Then you only need one bool instead of two in the class. I think it's useful to have a straightforward way to reason about the Start/Stop state of the InputTracker as well as the callback state. The states of the InputTracker are getting a bit hairy. I guess I could make IsTracking() { return pause_timer_.IsRunning() || callbacks_added_; } if you want to remove the is_tracking_ bool? I fixed the RVH switch timer issue and wrote a test.
https://codereview.chromium.org/1373363005/diff/30001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1373363005/diff/30001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:182: } On 2015/10/07 04:23:50, calamity wrote: > On 2015/10/06 04:11:45, dominickn wrote: > > I think this means that the pause timer is reset to 0 each time the render > view > > host changes, including if the pause timer is already running (with time left > on > > the clock). I think is_tracking_ should replace callbacks_added_ (i.e. only > true > > if they input is truly being recorded). Then you could have: > > > > if (pause_timer_.IsRunning()) { > > host_ = new_host; > > } > > else if (is_tracking_) { > > input_tracker_.StopTracking(); > > input_tracker_.StartTracking(new_host, 0); > > } > > > > Then you only need one bool instead of two in the class. > > I think it's useful to have a straightforward way to reason about the Start/Stop > state of the InputTracker as well as the callback state. The states of the > InputTracker are getting a bit hairy. > > I guess I could make IsTracking() { return pause_timer_.IsRunning() || > callbacks_added_; } if you want to remove the is_tracking_ bool? > > I fixed the RVH switch timer issue and wrote a test. I'm mainly concerned that IsTracking() and StartTracking() imply that we're tracking input, when in fact we may or may not be (particularly the latter now that I think about it). I acknowledge that the internal state is getting a bit confusing. Perhaps the states should be: IsTracking: callbacks are registered, waiting for input IsActive: as above, but also true when the timer is going Remove Tracking from Start/Pause/Stop. I think that clarifies the state, and makes the naming more consistent with what's actually going on in the tracker (it's "started", but not necessarily "tracking"). https://codereview.chromium.org/1373363005/diff/50001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1373363005/diff/50001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:92: RemoveCallbacks(); I'm not sure it's safe to implement it this way: the old host may have been shut down so the host_ pointer may be invalid at this point (see https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...)
https://codereview.chromium.org/1373363005/diff/30001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1373363005/diff/30001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:182: } On 2015/10/07 05:38:50, dominickn wrote: > On 2015/10/07 04:23:50, calamity wrote: > > On 2015/10/06 04:11:45, dominickn wrote: > > > I think this means that the pause timer is reset to 0 each time the render > > view > > > host changes, including if the pause timer is already running (with time > left > > on > > > the clock). I think is_tracking_ should replace callbacks_added_ (i.e. only > > true > > > if they input is truly being recorded). Then you could have: > > > > > > if (pause_timer_.IsRunning()) { > > > host_ = new_host; > > > } > > > else if (is_tracking_) { > > > input_tracker_.StopTracking(); > > > input_tracker_.StartTracking(new_host, 0); > > > } > > > > > > Then you only need one bool instead of two in the class. > > > > I think it's useful to have a straightforward way to reason about the > Start/Stop > > state of the InputTracker as well as the callback state. The states of the > > InputTracker are getting a bit hairy. > > > > I guess I could make IsTracking() { return pause_timer_.IsRunning() || > > callbacks_added_; } if you want to remove the is_tracking_ bool? > > > > I fixed the RVH switch timer issue and wrote a test. > > I'm mainly concerned that IsTracking() and StartTracking() imply that we're > tracking input, when in fact we may or may not be (particularly the latter now > that I think about it). I acknowledge that the internal state is getting a bit > confusing. Perhaps the states should be: > > IsTracking: callbacks are registered, waiting for input > IsActive: as above, but also true when the timer is going > Remove Tracking from Start/Pause/Stop. > > I think that clarifies the state, and makes the naming more consistent with > what's actually going on in the tracker (it's "started", but not necessarily > "tracking"). Agreed. Done. https://codereview.chromium.org/1373363005/diff/50001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1373363005/diff/50001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:92: RemoveCallbacks(); On 2015/10/07 05:38:50, dominickn wrote: > I'm not sure it's safe to implement it this way: the old host may have been shut > down so the host_ pointer may be invalid at this point (see > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...) Good catch. Fixed.
lgtm https://codereview.chromium.org/1373363005/diff/70001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1373363005/diff/70001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:106: is_active_ = false; Nit: assign is_active_ to false last https://codereview.chromium.org/1373363005/diff/70001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_helper.h (right): https://codereview.chromium.org/1373363005/diff/70001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.h:34: void DidNavigateMainFrame( Nit: why are these public now?
https://codereview.chromium.org/1373363005/diff/70001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1373363005/diff/70001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:106: is_active_ = false; On 2015/10/07 20:11:03, dominickn wrote: > Nit: assign is_active_ to false last Done. https://codereview.chromium.org/1373363005/diff/70001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_helper.h (right): https://codereview.chromium.org/1373363005/diff/70001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.h:34: void DidNavigateMainFrame( On 2015/10/07 20:11:03, dominickn wrote: > Nit: why are these public now? I needed a couple in tests. In general, I think it's preferable to override at the same (or higher) access level as the superclass.
The CQ bit was checked by calamity@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org Link to the patchset: https://codereview.chromium.org/1373363005/#ps90001 (title: "address comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1373363005/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1373363005/90001
Message was sent while issue was closed.
Committed patchset #5 (id:90001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/43716890b5ad407479926caff884c3737e9e5908 Cr-Commit-Position: refs/heads/master@{#353010} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
