Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1986033002:
Implement an observer interface for the site engagement service. (Closed)
DescriptionImplement an observer interface for the site engagement service.
This CL adds the ability for site engagement service clients to register
an observer on the site engagement service. The observer's
OnEngagementIncreased method is called every time engagement is
increased for a URL. The WebContents from which the increase was
triggered and the visible/hidden state of that WebContents is also
passed through to the observer.
BUG=606590
Committed: https://crrev.com/1dc78e80fe37a7afc9b68f07d53ec09a2179e2f9
Cr-Commit-Position: refs/heads/master@{#401206}
Patch Set 1 #Patch Set 2 : Fix CrOS and Android compile #Patch Set 3 : Squash race condition in unit test which leads to a leak #
Total comments: 16
Patch Set 4 : Addressing reviewer comments #Patch Set 5 : Convert to an observer interface #
Total comments: 5
Patch Set 6 : Address comments #
Total comments: 7
Patch Set 7 : Addressing comments #Patch Set 8 : Rebase, pass through WebContents and hidden state #Patch Set 9 : Rebase to fix leak #
Total comments: 7
Patch Set 10 : Addressing reviewer comments #
Messages
Total messages: 34 (13 generated)
Description was changed from ========== Implement a callback interface for the site engagement service. This CL adds the ability for site engagement service clients to register a callback with a URL and an engagement score or level. The callback is posted to the UI thread as soon as the URL's origin reaches the requested score. BUG=606590 ========== to ========== Implement a callback interface for the site engagement service. This CL adds the ability for site engagement service clients to register a callback with a URL and an engagement score or level. The callback is posted to the UI thread as soon as the URL's origin reaches the requested score. BUG=606590 ==========
dominickn@chromium.org changed reviewers: + benwells@chromium.org, calamity@chromium.org
PTAL, thanks!
First pass. https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_score.cc (right): https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_score.cc:123: return GetHiddenMediaPoints(); This will limit the way we modify the experimental numbers right? Can we calculate this as a static when we update from variations? https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:238: } else { nit: replace with early return. https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:297: engagement_callbacks_(), vectors should initialize fine without being in the list. https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:319: ProcessCallbacks(url, score.GetScore()); Does this also need to be run when scores are reset? https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:569: PostCallback(it->callback, url, score); Why do we post this rather than running immediately? https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:579: double score) { nit: alignment https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:613: void SiteEngagementService::RunAndDeleteCallback( This can be in the anonymous namespace. Doesn't need to be on the class. https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.h:110: // |url|'s origin reaches |level| or |score|. Callbacks for origins which have It's not obvious that this only happens when engagement goes above the specified score/level.
Thanks! https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_score.cc (right): https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_score.cc:123: return GetHiddenMediaPoints(); On 2016/05/19 03:28:22, calamity wrote: > This will limit the way we modify the experimental numbers right? Can we > calculate this as a static when we update from variations? Done. https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:238: } else { On 2016/05/19 03:28:22, calamity wrote: > nit: replace with early return. Done. https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:297: engagement_callbacks_(), On 2016/05/19 03:28:22, calamity wrote: > vectors should initialize fine without being in the list. Done. https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:319: ProcessCallbacks(url, score.GetScore()); On 2016/05/19 03:28:22, calamity wrote: > Does this also need to be run when scores are reset? I explicitly decided to not do that: this runs only when engagement is explicitly (not implicitly) earned/changed. https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:569: PostCallback(it->callback, url, score); On 2016/05/19 03:28:22, calamity wrote: > Why do we post this rather than running immediately? This is potentially run on user input (like scrolling), so I decided to post the task to avoid lag. This also lets us avoid having to stipulate to callers that their callback has to be fast enough in case it's run on user input. https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:579: double score) { On 2016/05/19 03:28:22, calamity wrote: > nit: alignment Done. https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:613: void SiteEngagementService::RunAndDeleteCallback( On 2016/05/19 03:28:22, calamity wrote: > This can be in the anonymous namespace. Doesn't need to be on the class. Done. https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/1986033002/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.h:110: // |url|'s origin reaches |level| or |score|. Callbacks for origins which have On 2016/05/19 03:28:22, calamity wrote: > It's not obvious that this only happens when engagement goes above the specified > score/level. I've made this comment more explicit - it's equal to or greater than.
Description was changed from ========== Implement a callback interface for the site engagement service. This CL adds the ability for site engagement service clients to register a callback with a URL and an engagement score or level. The callback is posted to the UI thread as soon as the URL's origin reaches the requested score. BUG=606590 ========== to ========== Implement an observer interface for the site engagement service. This CL adds the ability for site engagement service clients to register an observer on the site engagement service. The observer's OnEngagementIncreased method is called every time engagement is increased for a URL. BUG=606590 ==========
PTAL - this is now an observer interface for the reasons discussed F2F: * the primary consumer is the app banner service * using the previous callback interface, there was no way to unregister a callback, or indicate that the banner manager was changing which URLs it was interested in * the observer firehose pattern works better for app banners because the banner manager can simply return when it gets a URL or score it isn't interested in (or is otherwise not active).
https://codereview.chromium.org/1986033002/diff/80001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_score.h (right): https://codereview.chromium.org/1986033002/diff/80001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_score.h:43: POINTS_INCREMENT_LAST = HIDDEN_MEDIA_POINTS, This won't work if we add a new increment type to the bottom of this enum. We need either a function or a new enum that tells us exactly what the increment types are. https://codereview.chromium.org/1986033002/diff/80001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service_unittest.cc (right): https://codereview.chromium.org/1986033002/diff/80001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service_unittest.cc:125: void UpdateExpectations(const GURL& url, double score) { Unused.
Thanks! https://codereview.chromium.org/1986033002/diff/80001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_score.h (right): https://codereview.chromium.org/1986033002/diff/80001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_score.h:43: POINTS_INCREMENT_LAST = HIDDEN_MEDIA_POINTS, On 2016/05/24 08:09:32, calamity wrote: > This won't work if we add a new increment type to the bottom of this enum. We > need either a function or a new enum that tells us exactly what the increment > types are. Is there any reason to keep this enum append only? Even if we change Finch configs, there is a mechanism to target experiments to certain versions so that shouldn't be an issue. I've added a comment to make this more explicit. Otherwise, the increment stuff isn't really necessary in this CL - I kept it because it seems like we were missing the complement function for GetEngagementLevel. https://codereview.chromium.org/1986033002/diff/80001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service_unittest.cc (right): https://codereview.chromium.org/1986033002/diff/80001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service_unittest.cc:125: void UpdateExpectations(const GURL& url, double score) { On 2016/05/24 08:09:33, calamity wrote: > Unused. Done.
Ping
lgtm https://codereview.chromium.org/1986033002/diff/80001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_score.h (right): https://codereview.chromium.org/1986033002/diff/80001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_score.h:43: POINTS_INCREMENT_LAST = HIDDEN_MEDIA_POINTS, On 2016/05/25 07:21:32, dominickn wrote: > On 2016/05/24 08:09:32, calamity wrote: > > This won't work if we add a new increment type to the bottom of this enum. We > > need either a function or a new enum that tells us exactly what the increment > > types are. > > Is there any reason to keep this enum append only? Even if we change Finch > configs, there is a mechanism to target experiments to certain versions so that > shouldn't be an issue. I've added a comment to make this more explicit. > Otherwise, the increment stuff isn't really necessary in this CL - I kept it > because it seems like we were missing the complement function for > GetEngagementLevel. Guess not. This whole 3-way mapping is getting a bit messy though =S
https://codereview.chromium.org/1986033002/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_observer.h (right): https://codereview.chromium.org/1986033002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_observer.h:20: const GURL& url, can you use a url::Origin here instead? There is a separate effort going on to use that class instead of GURL when the urls in question should be origins. https://codereview.chromium.org/1986033002/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.h (right): https://codereview.chromium.org/1986033002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.h:35: POINTS_INCREMENT_FIRST = NAVIGATION_POINTS, Is it just me, or will it be awkward now to add more variations which increment points? Update: I just saw calamity's comments and can see this has already been addressed... but I'm not convinced that inserting values into this enum down the track will be a good idea. Seems like there is a potential for error there. You should at least mention in a comment that if anything is inserted field trials should be updated to target specific versions, but even that feels fraught to me. https://codereview.chromium.org/1986033002/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/1986033002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:147: double GetScoreForEngagementLevel(EngagementLevel level) const; This doesn't appear to be used or tested. Are you planning to use it? If not I don't think its worth adding for completeness sake alone.
Thanks! https://codereview.chromium.org/1986033002/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_observer.h (right): https://codereview.chromium.org/1986033002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_observer.h:20: const GURL& url, On 2016/06/01 01:44:45, benwells wrote: > can you use a url::Origin here instead? > > There is a separate effort going on to use that class instead of GURL when the > urls in question should be origins. Do you think that the observer should only be informed of the origin for which engagement increased, or the full URL which triggered that increase? For instance, for app banners, the particular PWA may be hosted under a path, and we want an engagement increase on that precise path before triggering the banner flow. If we reduce this parameter to just an origin, we'll lose that precision. Just a strawman, but I'd like to hear your thoughts. https://codereview.chromium.org/1986033002/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.h (right): https://codereview.chromium.org/1986033002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.h:35: POINTS_INCREMENT_FIRST = NAVIGATION_POINTS, On 2016/06/01 01:44:45, benwells wrote: > Is it just me, or will it be awkward now to add more variations which increment > points? > > Update: I just saw calamity's comments and can see this has already been > addressed... but I'm not convinced that inserting values into this enum down the > track will be a good idea. Seems like there is a potential for error there. > > You should at least mention in a comment that if anything is inserted field > trials should be updated to target specific versions, but even that feels > fraught to me. Acknowledged. I'll remove this. https://codereview.chromium.org/1986033002/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/1986033002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:147: double GetScoreForEngagementLevel(EngagementLevel level) const; On 2016/06/01 01:44:45, benwells wrote: > This doesn't appear to be used or tested. Are you planning to use it? If not I > don't think its worth adding for completeness sake alone. Acknowledged, I'll remove it.
lgtm (yurch, there was a merge in that last patchset!) https://codereview.chromium.org/1986033002/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_observer.h (right): https://codereview.chromium.org/1986033002/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_observer.h:20: const GURL& url, On 2016/06/01 02:54:13, dominickn wrote: > On 2016/06/01 01:44:45, benwells wrote: > > can you use a url::Origin here instead? > > > > There is a separate effort going on to use that class instead of GURL when the > > urls in question should be origins. > > Do you think that the observer should only be informed of the origin for which > engagement increased, or the full URL which triggered that increase? > > For instance, for app banners, the particular PWA may be hosted under a path, > and we want an engagement increase on that precise path before triggering the > banner flow. If we reduce this parameter to just an origin, we'll lose that > precision. Just a strawman, but I'd like to hear your thoughts. Hmm ... I hadn't considered that. It feels a bit strange to limit it to paths, as the engagement is for the origin as a whole, not any individual path. In the app banner case I'd be worried about subtle race conditions and other weirdness occuring, but it's hard to consider it without seeing code. Either way, feel free to leave it as is if you'll be passing the full url through, and change to an Origin if it will just be origins.
Description was changed from ========== Implement an observer interface for the site engagement service. This CL adds the ability for site engagement service clients to register an observer on the site engagement service. The observer's OnEngagementIncreased method is called every time engagement is increased for a URL. BUG=606590 ========== to ========== Implement an observer interface for the site engagement service. This CL adds the ability for site engagement service clients to register an observer on the site engagement service. The observer's OnEngagementIncreased method is called every time engagement is increased for a URL from a specified WebContents. BUG=606590 ==========
Patchset #8 (id:140001) has been deleted
Patchset #8 (id:160001) has been deleted
Patchset #8 (id:180001) has been deleted
Patchset #8 (id:200001) has been deleted
Description was changed from ========== Implement an observer interface for the site engagement service. This CL adds the ability for site engagement service clients to register an observer on the site engagement service. The observer's OnEngagementIncreased method is called every time engagement is increased for a URL from a specified WebContents. BUG=606590 ========== to ========== Implement an observer interface for the site engagement service. This CL adds the ability for site engagement service clients to register an observer on the site engagement service. The observer's OnEngagementIncreased method is called every time engagement is increased for a URL. The WebContents from which the increase was triggered and the visible/hidden state of that WebContents is also passed through to the observer. BUG=606590 ==========
Please take another look - this now passes through a WebContents*, so that observers can be told which WebContents triggered the engagement increase. They are also told if the increase happened while the WebContents was visible or not, so they don't have to do their own bookkeeping for that. Both of these features are useful for app banners, which only want to trigger on the foreground WebContents.
mostly good, just questions about hidden. Also, please keep rebases separate from other changes so it's easier to see what you've done versus the rebase. https://codereview.chromium.org/1986033002/diff/240001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_observer.h (right): https://codereview.chromium.org/1986033002/diff/240001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_observer.h:22: // was hidden (e.g. in the background). Why is the hidden field necessary? https://codereview.chromium.org/1986033002/diff/240001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1986033002/diff/240001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:247: void SiteEngagementService::AddPoints(const GURL& url, double points) { You could pass in the web contents and hidden here, and do the RecordMetrics and FOR_EACH_OBSERVER stuff. https://codereview.chromium.org/1986033002/diff/240001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:358: OnEngagementIncreased(web_contents, url, GetScore(url), false)); Is this always not hidden? What about if the navigation happens due to javascript?
On 2016/06/11 09:31:23, benwells wrote: > mostly good, just questions about hidden. > > Also, please keep rebases separate from other changes so it's easier to see what > you've done versus the rebase. > > https://codereview.chromium.org/1986033002/diff/240001/chrome/browser/engagem... > File chrome/browser/engagement/site_engagement_observer.h (right): > > https://codereview.chromium.org/1986033002/diff/240001/chrome/browser/engagem... > chrome/browser/engagement/site_engagement_observer.h:22: // was hidden (e.g. in > the background). > Why is the hidden field necessary? Oh btw I remembered why ... so no need to answer. It feels weird to pass this through though. > > https://codereview.chromium.org/1986033002/diff/240001/chrome/browser/engagem... > File chrome/browser/engagement/site_engagement_service.cc (right): > > https://codereview.chromium.org/1986033002/diff/240001/chrome/browser/engagem... > chrome/browser/engagement/site_engagement_service.cc:247: void > SiteEngagementService::AddPoints(const GURL& url, double points) { > You could pass in the web contents and hidden here, and do the RecordMetrics and > FOR_EACH_OBSERVER stuff. > > https://codereview.chromium.org/1986033002/diff/240001/chrome/browser/engagem... > chrome/browser/engagement/site_engagement_service.cc:358: > OnEngagementIncreased(web_contents, url, GetScore(url), false)); > Is this always not hidden? What about if the navigation happens due to > javascript?
Thanks! https://codereview.chromium.org/1986033002/diff/240001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_observer.h (right): https://codereview.chromium.org/1986033002/diff/240001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_observer.h:22: // was hidden (e.g. in the background). On 2016/06/11 09:31:23, benwells wrote: > Why is the hidden field necessary? Passing it through means that clients don't need to keep their own hidden state if they care - they can just use the one already computed by the site engagement helper. https://codereview.chromium.org/1986033002/diff/240001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1986033002/diff/240001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:247: void SiteEngagementService::AddPoints(const GURL& url, double points) { On 2016/06/11 09:31:23, benwells wrote: > You could pass in the web contents and hidden here, and do the RecordMetrics and > FOR_EACH_OBSERVER stuff. I did think about doing that. It felt kind of messy though - right now, AddPoints is a nice, self-contained, testable method, and adding the observer call in it makes it a lot less nice. The reduction in duplication didn't feel worth the extra complexity (and resulting changes to tests) in here. WDYT? https://codereview.chromium.org/1986033002/diff/240001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:358: OnEngagementIncreased(web_contents, url, GetScore(url), false)); On 2016/06/11 09:31:23, benwells wrote: > Is this always not hidden? What about if the navigation happens due to > javascript? The assumption right now is that navigation engagement is always in the foreground. I'll file a bug to investigate if that's really the case.
https://codereview.chromium.org/1986033002/diff/240001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_observer.h (right): https://codereview.chromium.org/1986033002/diff/240001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_observer.h:22: // was hidden (e.g. in the background). On 2016/06/11 15:05:50, dominickn wrote: > On 2016/06/11 09:31:23, benwells wrote: > > Why is the hidden field necessary? > > Passing it through means that clients don't need to keep their own hidden state > if they care - they can just use the one already computed by the site engagement > helper. Let's chat about this. I think it would be better for clients to keep track of this but don't feel super strongly. It will be easier to explain face to face.
Thanks! Have removed the hidden flag. The gypi file has a rebase, so I didn't upload a second patch just for that.
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/1986033002/#ps260001 (title: "Addressing reviewer comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1986033002/260001
Message was sent while issue was closed.
Description was changed from ========== Implement an observer interface for the site engagement service. This CL adds the ability for site engagement service clients to register an observer on the site engagement service. The observer's OnEngagementIncreased method is called every time engagement is increased for a URL. The WebContents from which the increase was triggered and the visible/hidden state of that WebContents is also passed through to the observer. BUG=606590 ========== to ========== Implement an observer interface for the site engagement service. This CL adds the ability for site engagement service clients to register an observer on the site engagement service. The observer's OnEngagementIncreased method is called every time engagement is increased for a URL. The WebContents from which the increase was triggered and the visible/hidden state of that WebContents is also passed through to the observer. BUG=606590 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Implement an observer interface for the site engagement service. This CL adds the ability for site engagement service clients to register an observer on the site engagement service. The observer's OnEngagementIncreased method is called every time engagement is increased for a URL. The WebContents from which the increase was triggered and the visible/hidden state of that WebContents is also passed through to the observer. BUG=606590 ========== to ========== Implement an observer interface for the site engagement service. This CL adds the ability for site engagement service clients to register an observer on the site engagement service. The observer's OnEngagementIncreased method is called every time engagement is increased for a URL. The WebContents from which the increase was triggered and the visible/hidden state of that WebContents is also passed through to the observer. BUG=606590 Committed: https://crrev.com/1dc78e80fe37a7afc9b68f07d53ec09a2179e2f9 Cr-Commit-Position: refs/heads/master@{#401206} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/1dc78e80fe37a7afc9b68f07d53ec09a2179e2f9 Cr-Commit-Position: refs/heads/master@{#401206} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
