Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2788413003:
Add SiteEngagementService::GetAllDetails(), to return detailed scores. (Closed)
|
Created:
3 years, 8 months ago by Wez Modified:
3 years, 8 months ago CC:
chromium-reviews, dominickn+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, arv+watch_chromium.org, darin (slow to review) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd SiteEngagementService::GetAllDetails(), to return detailed scores.
The new API provides details of the (decayed) base score, and any bonus
scores e.g. due to granted permissions, in addition to the total score.
It will be used by the important sites implementation to avoid the need
for querying the bonus factors separately from the site engagement.
This CL makes the following changes:
* Renames SiteEngagement(Info->Details).
* Add base & bonus score fields to SiteEngagementDetails.
* Adds SiteEngagementScore::GetDetails() and renames GetScore() to
GetTotalScore(), for clarity.
* Adds SiteEngagementService::GetAllDetails(), which returns details for
all sites with non-zero engagement score (i.e. including sites which
have no direct engagement, but receive a bonus).
* Adds some unit tests, for details contents and the WebUI score format.
* Some minor cleanups (e.g. std::string() -> ResourceIdentifier() where
appropriate).
BUG=703848
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2788413003
Cr-Commit-Position: refs/heads/master@{#463772}
Committed: https://chromium.googlesource.com/chromium/src/+/9be0aad26957e8d00dd879fc1a188c0738dc3255
Patch Set 1 #
Total comments: 13
Patch Set 2 : Rename score->total_score and GetScore->GetTotal #
Total comments: 13
Patch Set 3 : Tweak names, add std::move() in Mojo get path #
Total comments: 3
Patch Set 4 : Add some unit tests #Patch Set 5 : Address review comments #Patch Set 6 : Cleanups #
Total comments: 12
Patch Set 7 : Revert some callsites to just engagement settings, cleanups #Patch Set 8 : Fix notifications permission logic & test #
Total comments: 4
Dependent Patchsets: Messages
Total messages: 40 (23 generated)
Description was changed from ========== Rename SiteEngagement(Info->Details) and add base & bonus score details. BUG=703848 ========== to ========== Rename SiteEngagement(Info->Details) and add base & bonus score details. BUG=703848 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by wez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
wez@chromium.org changed reviewers: + dominickn@chromium.org
Pre-review: Please take a look at the naming of the scores fields in SiteEngagementDetails (formerly ..Info) and let me know if they look suitable. I'll send along separate CLs for the WebUI and Important Sites changes.
Looks pretty good! https://codereview.chromium.org/2788413003/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_details.mojom (right): https://codereview.chromium.org/2788413003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_details.mojom:11: double score; I'd call this total_score to be explicit. Possibly also put it last in the struct. https://codereview.chromium.org/2788413003/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/2788413003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:151: for (const auto& site : *engagement_settings) { A nuance here is that sites with only a notification bonus score (and no actual engagement) won't appear in the engagement_settings list. We'll need to also iterate through the notification content setting. For now, this method can fetch a list of all URLs with notification permission, and then iterate over the engagement content setting, removing origins from the permission lists as they are seen. Then at the end we add all the remaining origins in the permission list to the vector with their bonus engagement. WDYT?
https://codereview.chromium.org/2788413003/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_details.mojom (right): https://codereview.chromium.org/2788413003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_details.mojom:11: double score; On 2017/04/06 01:47:16, dominickn wrote: > I'd call this total_score to be explicit. Possibly also put it last in the > struct. Renamed, but left it where it is; it seems more natural to me to have the "brief" information at the top. https://codereview.chromium.org/2788413003/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/2788413003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:151: for (const auto& site : *engagement_settings) { On 2017/04/06 01:47:16, dominickn wrote: > A nuance here is that sites with only a notification bonus score (and no actual > engagement) won't appear in the engagement_settings list. We'll need to also > iterate through the notification content setting. > > For now, this method can fetch a list of all URLs with notification permission, > and then iterate over the engagement content setting, removing origins from the > permission lists as they are seen. Then at the end we add all the remaining > origins in the permission list to the vector with their bonus engagement. WDYT? IIUC correctly what you're saying is basically: set<URL> notification_permission_urls = ... set<URL> engagement_content_settings_urls = ... set<URL> unengaged_urls = engagement_content_settings_urls - notification_permission_urls; for (engagement_content_settings_urls) result.insert(GetURLScore(url).GetDetails()) for (unengaged_urls) result.insert(SomehowGetBonusEngagement) Two concerns: 1. What is the mechanism for getting the "bonus" for un-engaged URLs? If we call GetScoreDetails() then that'll implicitly create a new SiteEngagementScore in-memory - is that desirable? 2. This seems a lot of logic (most importantly a lot of memory-churn) to calculate a result. :-/ Ideally we'd have a single internal API that gets the set of URLs to process, and then have GetScoreMap, GetScoreDetails etc call that to determine what to do; I'll see if that will work. https://codereview.chromium.org/2788413003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:166: score_map[info.origin] = info.score; Note to self; this also leads to loads of memory churn; see if we can std::move(info.origin), otherwise revert to calling GetTotalScore(url) rather than going via Details. https://codereview.chromium.org/2788413003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:262: mojom::SiteEngagementDetails SiteEngagementService::GetScoreDetails( Note to self: This clashes with GetScoreDetails() above; rename one or the other. https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_score.cc (right): https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_score.cc:283: double SiteEngagementScore::GetTotal() const { Renamed this to GetTotal, for clarity, and rearranged things to avoid the churn of calling GetDetails() under the hood. https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:674: double new_score = proportion_remaining * engagement_score.GetTotal(); This logic looks wrong; we're grabbing the *total* score, decaying it and Reset()ing based on that - should we actually be grabbing the underlying raw_score? or the decayed score? Or the base score? Why do we even have decay-handling logic in here, rather than in SiteEngagementScore itself?
Thanks wez! https://codereview.chromium.org/2788413003/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/2788413003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:151: for (const auto& site : *engagement_settings) { On 2017/04/08 01:52:53, Wez wrote: > On 2017/04/06 01:47:16, dominickn wrote: > > A nuance here is that sites with only a notification bonus score (and no > actual > > engagement) won't appear in the engagement_settings list. We'll need to also > > iterate through the notification content setting. > > > > For now, this method can fetch a list of all URLs with notification > permission, > > and then iterate over the engagement content setting, removing origins from > the > > permission lists as they are seen. Then at the end we add all the remaining > > origins in the permission list to the vector with their bonus engagement. > WDYT? > > IIUC correctly what you're saying is basically: > > set<URL> notification_permission_urls = ... > set<URL> engagement_content_settings_urls = ... > > set<URL> unengaged_urls = engagement_content_settings_urls - > notification_permission_urls; > > for (engagement_content_settings_urls) > result.insert(GetURLScore(url).GetDetails()) > > for (unengaged_urls) > result.insert(SomehowGetBonusEngagement) This is what I had in mind (slight difference to yours): set<URL> notification_permission_urls = ... set<URL> engagement_content_settings_urls = ... for (engagement_content_settings_urls) { result.insert(GetURLScore(url).GetDetails()) notification_permission_urls -= url } for (notification_permission_urls) { // if the URL had any non-bonus engagement it's caught in the first for loop result.insert(GetURLScore(url).GetDetails()) } > > Two concerns: > 1. What is the mechanism for getting the "bonus" for un-engaged URLs? If we call > GetScoreDetails() then that'll implicitly create a new SiteEngagementScore > in-memory - is that desirable? That should be fine. GetScoreDetails will be infrequently called: 1. when the important sites dialog is triggered (user clears browsing data) 2. when UMA stats are calculated (on startup and once per hour after that) 3. chrome://site-engagement page on load 4. tests > 2. This seems a lot of logic (most importantly a lot of memory-churn) to > calculate a result. :-/ > > Ideally we'd have a single internal API that gets the set of URLs to process, > and then have GetScoreMap, GetScoreDetails etc call that to determine what to > do; I'll see if that will work. Ideally, yes, there would be a single internal API that gets the set of URLs to process. But the only consumer of that method should be GetScoreDetails (we should remove GetScoreMap because it should be supplanted completed by GetScoreDetails). There shouldn't be any other uses that don't require scores attached. https://codereview.chromium.org/2788413003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:166: score_map[info.origin] = info.score; On 2017/04/08 01:52:53, Wez wrote: > Note to self; this also leads to loads of memory churn; see if we can > std::move(info.origin), otherwise revert to calling GetTotalScore(url) rather > than going via Details. Even better: we should be able to remove this method once important sites and tests are ported to use GetScoreDetails. :) https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_score.cc (right): https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_score.cc:283: double SiteEngagementScore::GetTotal() const { On 2017/04/08 01:52:53, Wez wrote: > Renamed this to GetTotal, for clarity, and rearranged things to avoid the churn > of calling GetDetails() under the hood. I'm not sure that GetTotal is more clear than GetScore (plus the comment in the header file can clarify). But I'm not overly fussy on this. https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_score.cc:423: double SiteEngagementScore::BonusIfHasNotifications() const { Nit: I'd call this BonusIfCanShowNotifications() https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:674: double new_score = proportion_remaining * engagement_score.GetTotal(); On 2017/04/08 01:52:53, Wez wrote: > This logic looks wrong; we're grabbing the *total* score, decaying it and > Reset()ing based on that - should we actually be grabbing the underlying > raw_score? or the decayed score? Or the base score? You're right, this should be the underlying raw_score that's decayed. The bonus concept has become more prominent since it now includes origins with notification permission (way more than previously, which was just whether or not a site was installed). When this was first implemented, the bonus wasn't really hit that often so it wasn't important. Using the raw_score will stay true to the original intention of the proportion decay. > Why do we even have decay-handling logic in here, rather than in > SiteEngagementScore itself? Legacy holdover from before SiteEngagementScore existed (and when the decay was much simpler - a linear decay of 5 per week). :) Makes sense to me to move this into a SiteEngagementScore::Decay(base::Time now) method, which does everything except commit. https://codereview.chromium.org/2788413003/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/2788413003/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:161: std::vector<mojom::SiteEngagementDetails> scores = GetAllDetails(); Nit: you could probably just do for (const auto& info : GetAllDetails()) ? https://codereview.chromium.org/2788413003/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/2788413003/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.h:10: #include <set> #include <vector>
The CQ bit was checked by wez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2788413003/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/2788413003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:151: for (const auto& site : *engagement_settings) { On 2017/04/10 00:58:11, dominickn wrote: > On 2017/04/08 01:52:53, Wez wrote: > > On 2017/04/06 01:47:16, dominickn wrote: > > > A nuance here is that sites with only a notification bonus score (and no > > actual > > > engagement) won't appear in the engagement_settings list. We'll need to also > > > iterate through the notification content setting. > > > > > > For now, this method can fetch a list of all URLs with notification > > permission, > > > and then iterate over the engagement content setting, removing origins from > > the > > > permission lists as they are seen. Then at the end we add all the remaining > > > origins in the permission list to the vector with their bonus engagement. > > WDYT? > > > > IIUC correctly what you're saying is basically: > > > > set<URL> notification_permission_urls = ... > > set<URL> engagement_content_settings_urls = ... > > > > set<URL> unengaged_urls = engagement_content_settings_urls - > > notification_permission_urls; > > > > for (engagement_content_settings_urls) > > result.insert(GetURLScore(url).GetDetails()) > > > > for (unengaged_urls) > > result.insert(SomehowGetBonusEngagement) > > This is what I had in mind (slight difference to yours): > > set<URL> notification_permission_urls = ... > set<URL> engagement_content_settings_urls = ... > > for (engagement_content_settings_urls) { > result.insert(GetURLScore(url).GetDetails()) > notification_permission_urls -= url > } > > for (notification_permission_urls) { > // if the URL had any non-bonus engagement it's caught in the first for loop > result.insert(GetURLScore(url).GetDetails()) > } That makes sense; what I've actually done in the latest patch is collate a set<GURL>, since all we use those content-settings fetches for is gathering the URLs to process, so that we basically insert URLs from the two sources into a single set and iterate that. > > > > Two concerns: > > 1. What is the mechanism for getting the "bonus" for un-engaged URLs? If we > call > > GetScoreDetails() then that'll implicitly create a new SiteEngagementScore > > in-memory - is that desirable? > > That should be fine. GetScoreDetails will be infrequently called: > > 1. when the important sites dialog is triggered (user clears browsing data) > 2. when UMA stats are calculated (on startup and once per hour after that) OK, the startup-time enumeration means we should be a little careful about cost. > 3. chrome://site-engagement page on load The page refreshes every second or two, though. > 4. tests > > 2. This seems a lot of logic (most importantly a lot of memory-churn) to > > calculate a result. :-/ > > > > Ideally we'd have a single internal API that gets the set of URLs to process, > > and then have GetScoreMap, GetScoreDetails etc call that to determine what to > > do; I'll see if that will work. > > Ideally, yes, there would be a single internal API that gets the set of URLs to > process. But the only consumer of that method should be GetScoreDetails (we > should remove GetScoreMap because it should be supplanted completed by > GetScoreDetails). There shouldn't be any other uses that don't require scores > attached. There are several other calls that iterate over the URLs - the latest patch replaces all the call-sites with the new set-collating implementation. WDYT? https://codereview.chromium.org/2788413003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:166: score_map[info.origin] = info.score; On 2017/04/10 00:58:11, dominickn wrote: > On 2017/04/08 01:52:53, Wez wrote: > > Note to self; this also leads to loads of memory churn; see if we can > > std::move(info.origin), otherwise revert to calling GetTotalScore(url) rather > > than going via Details. > > Even better: we should be able to remove this method once important sites and > tests are ported to use GetScoreDetails. :) Acknowledged. https://codereview.chromium.org/2788413003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:262: mojom::SiteEngagementDetails SiteEngagementService::GetScoreDetails( On 2017/04/08 01:52:53, Wez wrote: > Note to self: This clashes with GetScoreDetails() above; rename one or the > other. Renamed the two in the latest patch. https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_score.cc (right): https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_score.cc:283: double SiteEngagementScore::GetTotal() const { On 2017/04/10 00:58:11, dominickn wrote: > On 2017/04/08 01:52:53, Wez wrote: > > Renamed this to GetTotal, for clarity, and rearranged things to avoid the > churn > > of calling GetDetails() under the hood. > > I'm not sure that GetTotal is more clear than GetScore (plus the comment in the > header file can clarify). But I'm not overly fussy on this. The class is SiteEngagementScore, so SiteEngagementScore::GetTotal() seems reasonably intuitive, and more obvious than Site...::GetScore(). If you prefer GetTotalScore() though then happy to update to that. https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_score.cc:418: if (days_since_shortcut_launch <= kMaxDaysSinceShortcutLaunch) nit: While we'll never hit this in practice, this code would have had unexpected behaviour in January 1970 (and it does behave unexpectedly in tests with SimpleTestClock...). https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_score.cc:423: double SiteEngagementScore::BonusIfHasNotifications() const { On 2017/04/10 00:58:11, dominickn wrote: > Nit: I'd call this BonusIfCanShowNotifications() Done. https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:674: double new_score = proportion_remaining * engagement_score.GetTotal(); On 2017/04/10 00:58:11, dominickn wrote: > On 2017/04/08 01:52:53, Wez wrote: > > This logic looks wrong; we're grabbing the *total* score, decaying it and > > Reset()ing based on that - should we actually be grabbing the underlying > > raw_score? or the decayed score? Or the base score? > > You're right, this should be the underlying raw_score that's decayed. The bonus > concept has become more prominent since it now includes origins with > notification permission (way more than previously, which was just whether or not > a site was installed). When this was first implemented, the bonus wasn't really > hit that often so it wasn't important. > > Using the raw_score will stay true to the original intention of the proportion > decay. > > > Why do we even have decay-handling logic in here, rather than in > > SiteEngagementScore itself? > > Legacy holdover from before SiteEngagementScore existed (and when the decay was > much simpler - a linear decay of 5 per week). :) > > Makes sense to me to move this into a SiteEngagementScore::Decay(base::Time now) > method, which does everything except commit. That sounds great, but reading through the comment above, I wasn't able to grok why we have both this "explicit" decay logic and separate "automatic" logic (as per the comment), rather than just letting the automatic decay do its job. It almost sounds like |raw_score_| should be replaced with direct calculation based on the remaining URLs for the origin, in the history. For now I've added a ::raw_score() getter to Score and used it here - let's get the Important Sites issue fixed and come back to clean this up. https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.h:125: // TODO(wez): Remove this! AFAICT the only callers of this are: 1. to calculate the total engagement score across all sites. 2. to get data to pass to record site engagement metrics. 3. for important sites to use to grab scores. Since #3 will be switched to use GetAllDetails() anyway, is it acceptable to switch the other callers to use that as well? The argument against doing so would be that GetAllDetails() is considerably more expensive, since it allocates internal buffers for a GURL for each entry. How often do we expect #1 and #2 to be called? https://codereview.chromium.org/2788413003/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/2788413003/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:161: std::vector<mojom::SiteEngagementDetails> scores = GetAllDetails(); On 2017/04/10 00:58:11, dominickn wrote: > Nit: you could probably just do for (const auto& info : GetAllDetails()) ? Updated this to use the std::set<GURL> helper.
The CQ bit was checked by wez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2788413003/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/2788413003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:151: for (const auto& site : *engagement_settings) { On 2017/04/10 03:45:28, Wez wrote: > On 2017/04/10 00:58:11, dominickn wrote: > > On 2017/04/08 01:52:53, Wez wrote: > > > On 2017/04/06 01:47:16, dominickn wrote: > > > > A nuance here is that sites with only a notification bonus score (and no > > > actual > > > > engagement) won't appear in the engagement_settings list. We'll need to > also > > > > iterate through the notification content setting. > > > > > > > > For now, this method can fetch a list of all URLs with notification > > > permission, > > > > and then iterate over the engagement content setting, removing origins > from > > > the > > > > permission lists as they are seen. Then at the end we add all the > remaining > > > > origins in the permission list to the vector with their bonus engagement. > > > WDYT? > > > > > > IIUC correctly what you're saying is basically: > > > > > > set<URL> notification_permission_urls = ... > > > set<URL> engagement_content_settings_urls = ... > > > > > > set<URL> unengaged_urls = engagement_content_settings_urls - > > > notification_permission_urls; > > > > > > for (engagement_content_settings_urls) > > > result.insert(GetURLScore(url).GetDetails()) > > > > > > for (unengaged_urls) > > > result.insert(SomehowGetBonusEngagement) > > > > This is what I had in mind (slight difference to yours): > > > > set<URL> notification_permission_urls = ... > > set<URL> engagement_content_settings_urls = ... > > > > for (engagement_content_settings_urls) { > > result.insert(GetURLScore(url).GetDetails()) > > notification_permission_urls -= url > > } > > > > for (notification_permission_urls) { > > // if the URL had any non-bonus engagement it's caught in the first for loop > > result.insert(GetURLScore(url).GetDetails()) > > } > > That makes sense; what I've actually done in the latest patch is collate a > set<GURL>, since all we use those content-settings fetches for is gathering the > URLs to process, so that we basically insert URLs from the two sources into a > single set and iterate that. > > > > > > > Two concerns: > > > 1. What is the mechanism for getting the "bonus" for un-engaged URLs? If we > > call > > > GetScoreDetails() then that'll implicitly create a new SiteEngagementScore > > > in-memory - is that desirable? > > > > That should be fine. GetScoreDetails will be infrequently called: > > > > 1. when the important sites dialog is triggered (user clears browsing data) > > 2. when UMA stats are calculated (on startup and once per hour after that) > > OK, the startup-time enumeration means we should be a little careful about cost. It's done using content::PostAfterStartupTask, so this should avoid any startup critical paths and get called eventually. > > 3. chrome://site-engagement page on load > > The page refreshes every second or two, though. Yep, but it's an internal / dev-focused page rather than being obviously end-user exposed, so I'm comfortable with this. > > 4. tests > > > 2. This seems a lot of logic (most importantly a lot of memory-churn) to > > > calculate a result. :-/ > > > > > > Ideally we'd have a single internal API that gets the set of URLs to > process, > > > and then have GetScoreMap, GetScoreDetails etc call that to determine what > to > > > do; I'll see if that will work. > > > > Ideally, yes, there would be a single internal API that gets the set of URLs > to > > process. But the only consumer of that method should be GetScoreDetails (we > > should remove GetScoreMap because it should be supplanted completed by > > GetScoreDetails). There shouldn't be any other uses that don't require scores > > attached. > > There are several other calls that iterate over the URLs - the latest patch > replaces all the call-sites with the new set-collating implementation. > > WDYT? https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_score.cc (right): https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_score.cc:283: double SiteEngagementScore::GetTotal() const { On 2017/04/10 03:45:28, Wez wrote: > On 2017/04/10 00:58:11, dominickn wrote: > > On 2017/04/08 01:52:53, Wez wrote: > > > Renamed this to GetTotal, for clarity, and rearranged things to avoid the > > churn > > > of calling GetDetails() under the hood. > > > > I'm not sure that GetTotal is more clear than GetScore (plus the comment in > the > > header file can clarify). But I'm not overly fussy on this. > > The class is SiteEngagementScore, so SiteEngagementScore::GetTotal() seems > reasonably intuitive, and more obvious than Site...::GetScore(). If you prefer > GetTotalScore() though then happy to update to that. Not fussed, it can stay. https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:674: double new_score = proportion_remaining * engagement_score.GetTotal(); On 2017/04/10 03:45:28, Wez wrote: > On 2017/04/10 00:58:11, dominickn wrote: > > On 2017/04/08 01:52:53, Wez wrote: > > > This logic looks wrong; we're grabbing the *total* score, decaying it and > > > Reset()ing based on that - should we actually be grabbing the underlying > > > raw_score? or the decayed score? Or the base score? > > > > You're right, this should be the underlying raw_score that's decayed. The > bonus > > concept has become more prominent since it now includes origins with > > notification permission (way more than previously, which was just whether or > not > > a site was installed). When this was first implemented, the bonus wasn't > really > > hit that often so it wasn't important. > > > > Using the raw_score will stay true to the original intention of the proportion > > decay. > > > > > Why do we even have decay-handling logic in here, rather than in > > > SiteEngagementScore itself? > > > > Legacy holdover from before SiteEngagementScore existed (and when the decay > was > > much simpler - a linear decay of 5 per week). :) > > > > Makes sense to me to move this into a SiteEngagementScore::Decay(base::Time > now) > > method, which does everything except commit. > > That sounds great, but reading through the comment above, I wasn't able to grok > why we have both this "explicit" decay logic and separate "automatic" logic (as > per the comment), rather than just letting the automatic decay do its job. > > It almost sounds like |raw_score_| should be replaced with direct calculation > based on the remaining URLs for the origin, in the history. > > For now I've added a ::raw_score() getter to Score and used it here - let's get > the Important Sites issue fixed and come back to clean this up. Privacy required us to explicitly decay the engagement score when the user cleared their history. This means that you avoid the situation where a user has cleared their history, but the engagement remains high (i.e. "revealing" information they've cleared) So this decay is proportionate to how many history items they removed, and is separate to the automatic time-based decay we already do. We don't want to decay based on URLs remaining in the history all the time though, since there are single-page apps and the like that will only ever generate one history entry (and vice-versa multi-page sites which generate oodles of history entries). Time-based decay is what we want in general. Anyway, circling back to this later SGTM. https://codereview.chromium.org/2788413003/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/2788413003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:67: for (const auto& site : content_settings_list) { Nit: no braces https://codereview.chromium.org/2788413003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:76: if (site.setting == CONTENT_SETTING_ALLOW) Should this be != ? https://codereview.chromium.org/2788413003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:349: void SiteEngagementService::CleanupEngagementScores( This cleanup method doesn't need to iterate over origins with notification permission. Its only purpose is to clean up the engagement content setting of sites with 0 raw scores, and rescale things in there if Chrome hasn't been used in a while (so you don't get a huge loss if you go on holiday). Notifications is always a flat boost, so it's not necessary to capture origins which only have notification permission (and no raw engagement) in this method. https://codereview.chromium.org/2788413003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:610: for (const GURL& origin : GetEngagementOriginsFromContentSettings(profile_)) { An origin can't possibly get maximum daily engagement unless it has an entry in the engagement content setting (max daily = 15), so it's probably not necessary to do this here.
https://codereview.chromium.org/2788413003/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_unittest.cc (right): https://codereview.chromium.org/2788413003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_unittest.cc:1854: url2, url2, CONTENT_SETTINGS_TYPE_NOTIFICATIONS, std::string(), Nit: this second url2 should be GURL(). Notifications doesn't use the secondary pattern. https://codereview.chromium.org/2788413003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_unittest.cc:1860: url3, url3, CONTENT_SETTINGS_TYPE_NOTIFICATIONS, std::string(), Ditto
The CQ bit was checked by wez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by wez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Rename SiteEngagement(Info->Details) and add base & bonus score details. BUG=703848 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add SiteEngagementService::GetAllDetails(), to return detailed scores. The new API provides details of the (decayed) base score, and any bonus scores e.g. due to granted permissions, in addition to the total score. It will be used by the important sites implementation to avoid the need for querying the bonus factors separately from the site engagement. This CL makes the following changes: * Renames SiteEngagement(Info->Details). * Add base & bonus score fields to SiteEngagementDetails. * Adds SiteEngagementScore::GetDetails() and renames GetScore() to GetTotalScore(), for clarity. * Adds SiteEngagementService::GetAllDetails(), which returns details for all sites with non-zero engagement score (i.e. including sites which have no direct engagement, but receive a bonus). * Adds some unit tests, for details contents and the WebUI score format. * Some minor cleanups (e.g. std::string() -> ResourceIdentifier() where appropriate). BUG=703848 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2788413003/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/2788413003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:151: for (const auto& site : *engagement_settings) { On 2017/04/10 04:41:26, dominickn wrote: > On 2017/04/10 03:45:28, Wez wrote: > > On 2017/04/10 00:58:11, dominickn wrote: > > > On 2017/04/08 01:52:53, Wez wrote: > > > > On 2017/04/06 01:47:16, dominickn wrote: > > > > > A nuance here is that sites with only a notification bonus score (and no > > > > actual > > > > > engagement) won't appear in the engagement_settings list. We'll need to > > also > > > > > iterate through the notification content setting. > > > > > > > > > > For now, this method can fetch a list of all URLs with notification > > > > permission, > > > > > and then iterate over the engagement content setting, removing origins > > from > > > > the > > > > > permission lists as they are seen. Then at the end we add all the > > remaining > > > > > origins in the permission list to the vector with their bonus > engagement. > > > > WDYT? > > > > > > > > IIUC correctly what you're saying is basically: > > > > > > > > set<URL> notification_permission_urls = ... > > > > set<URL> engagement_content_settings_urls = ... > > > > > > > > set<URL> unengaged_urls = engagement_content_settings_urls - > > > > notification_permission_urls; > > > > > > > > for (engagement_content_settings_urls) > > > > result.insert(GetURLScore(url).GetDetails()) > > > > > > > > for (unengaged_urls) > > > > result.insert(SomehowGetBonusEngagement) > > > > > > This is what I had in mind (slight difference to yours): > > > > > > set<URL> notification_permission_urls = ... > > > set<URL> engagement_content_settings_urls = ... > > > > > > for (engagement_content_settings_urls) { > > > result.insert(GetURLScore(url).GetDetails()) > > > notification_permission_urls -= url > > > } > > > > > > for (notification_permission_urls) { > > > // if the URL had any non-bonus engagement it's caught in the first for > loop > > > result.insert(GetURLScore(url).GetDetails()) > > > } > > > > That makes sense; what I've actually done in the latest patch is collate a > > set<GURL>, since all we use those content-settings fetches for is gathering > the > > URLs to process, so that we basically insert URLs from the two sources into a > > single set and iterate that. > > > > > > > > > > Two concerns: > > > > 1. What is the mechanism for getting the "bonus" for un-engaged URLs? If > we > > > call > > > > GetScoreDetails() then that'll implicitly create a new SiteEngagementScore > > > > in-memory - is that desirable? > > > > > > That should be fine. GetScoreDetails will be infrequently called: > > > > > > 1. when the important sites dialog is triggered (user clears browsing data) > > > 2. when UMA stats are calculated (on startup and once per hour after that) > > > > OK, the startup-time enumeration means we should be a little careful about > cost. > > It's done using content::PostAfterStartupTask, so this should avoid any startup > critical paths and get called eventually. > > > > 3. chrome://site-engagement page on load > > > > The page refreshes every second or two, though. > > Yep, but it's an internal / dev-focused page rather than being obviously > end-user exposed, so I'm comfortable with this. > > > > 4. tests > > > > 2. This seems a lot of logic (most importantly a lot of memory-churn) to > > > > calculate a result. :-/ > > > > > > > > Ideally we'd have a single internal API that gets the set of URLs to > > process, > > > > and then have GetScoreMap, GetScoreDetails etc call that to determine what > > to > > > > do; I'll see if that will work. > > > > > > Ideally, yes, there would be a single internal API that gets the set of URLs > > to > > > process. But the only consumer of that method should be GetScoreDetails (we > > > should remove GetScoreMap because it should be supplanted completed by > > > GetScoreDetails). There shouldn't be any other uses that don't require > scores > > > attached. > > > > There are several other calls that iterate over the URLs - the latest patch > > replaces all the call-sites with the new set-collating implementation. > > > > WDYT? Acknowledged. https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/2788413003/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:674: double new_score = proportion_remaining * engagement_score.GetTotal(); On 2017/04/10 04:41:26, dominickn wrote: > On 2017/04/10 03:45:28, Wez wrote: > > On 2017/04/10 00:58:11, dominickn wrote: > > > On 2017/04/08 01:52:53, Wez wrote: > > > > This logic looks wrong; we're grabbing the *total* score, decaying it and > > > > Reset()ing based on that - should we actually be grabbing the underlying > > > > raw_score? or the decayed score? Or the base score? > > > > > > You're right, this should be the underlying raw_score that's decayed. The > > bonus > > > concept has become more prominent since it now includes origins with > > > notification permission (way more than previously, which was just whether or > > not > > > a site was installed). When this was first implemented, the bonus wasn't > > really > > > hit that often so it wasn't important. > > > > > > Using the raw_score will stay true to the original intention of the > proportion > > > decay. > > > > > > > Why do we even have decay-handling logic in here, rather than in > > > > SiteEngagementScore itself? > > > > > > Legacy holdover from before SiteEngagementScore existed (and when the decay > > was > > > much simpler - a linear decay of 5 per week). :) > > > > > > Makes sense to me to move this into a SiteEngagementScore::Decay(base::Time > > now) > > > method, which does everything except commit. > > > > That sounds great, but reading through the comment above, I wasn't able to > grok > > why we have both this "explicit" decay logic and separate "automatic" logic > (as > > per the comment), rather than just letting the automatic decay do its job. > > > > It almost sounds like |raw_score_| should be replaced with direct calculation > > based on the remaining URLs for the origin, in the history. > > > > For now I've added a ::raw_score() getter to Score and used it here - let's > get > > the Important Sites issue fixed and come back to clean this up. > > Privacy required us to explicitly decay the engagement score when the user > cleared their history. This means that you avoid the situation where a user has > cleared their history, but the engagement remains high (i.e. "revealing" > information they've cleared) So this decay is proportionate to how many history > items they removed, and is separate to the automatic time-based decay we already > do. The double-decay stuff was cofusing; I realised it was just that in starting from GetScore() there is already decay included, hence the need for the complex compensation logic. So using raw_score() here is only correct if you remove the compensation. > Anyway, circling back to this later SGTM. Yup, I'll switch back to GetTotalScore() and add a TODO to cleanup. https://codereview.chromium.org/2788413003/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/2788413003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:67: for (const auto& site : content_settings_list) { On 2017/04/10 04:41:26, dominickn wrote: > Nit: no braces Done, though note that other areas of Chromium tend to use braces all the time, to mitigate potential security issues due to typos. :) https://codereview.chromium.org/2788413003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:76: if (site.setting == CONTENT_SETTING_ALLOW) On 2017/04/10 04:41:26, dominickn wrote: > Should this be != ? Yup; and of course my simple test wouldn't catch that... *facepalm* https://codereview.chromium.org/2788413003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:349: void SiteEngagementService::CleanupEngagementScores( On 2017/04/10 04:41:26, dominickn wrote: > This cleanup method doesn't need to iterate over origins with notification > permission. Its only purpose is to clean up the engagement content setting of > sites with 0 raw scores, and rescale things in there if Chrome hasn't been used > in a while (so you don't get a huge loss if you go on holiday). Notifications is > always a flat boost, so it's not necessary to capture origins which only have > notification permission (and no raw engagement) in this method. Done. https://codereview.chromium.org/2788413003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.cc:610: for (const GURL& origin : GetEngagementOriginsFromContentSettings(profile_)) { On 2017/04/10 04:41:26, dominickn wrote: > An origin can't possibly get maximum daily engagement unless it has an entry in > the engagement content setting (max daily = 15), so it's probably not necessary > to do this here. Done. https://codereview.chromium.org/2788413003/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_unittest.cc (right): https://codereview.chromium.org/2788413003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_unittest.cc:1854: url2, url2, CONTENT_SETTINGS_TYPE_NOTIFICATIONS, std::string(), On 2017/04/10 04:49:26, dominickn wrote: > Nit: this second url2 should be GURL(). Notifications doesn't use the secondary > pattern. This was actually cribbed from the tests above. :P Done. https://codereview.chromium.org/2788413003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_unittest.cc:1860: url3, url3, CONTENT_SETTINGS_TYPE_NOTIFICATIONS, std::string(), On 2017/04/10 04:49:26, dominickn wrote: > Ditto Done.
wez@chromium.org changed reviewers: + dbeam@chromium.org, dcheng@chromium.org
dcheng: Review mojom rename & JSON overlay, plz. dbeam: Review WebUI & resources changes, plz.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Thanks wez for all the work here! chrome/browser/engagement/ lgtm
mojo lgtm https://codereview.chromium.org/2788413003/diff/140001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.h (right): https://codereview.chromium.org/2788413003/diff/140001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.h:149: mojom::SiteEngagementDetails GetDetails() const; FWIW, this seems potentially dicey. I think this is internally relying on the fact that there's an implicitly defined copy constructor. It'll probably break if someone ever adds a move-only field to the mojo struct.
lgtm but C++ -> JS tests are kind funky and flaky, IMO i'd recommend testing the handlers and JS separately. https://codereview.chromium.org/2788413003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc (right): https://codereview.chromium.org/2788413003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc:93: ""), can you `git cl format` this? is it already?
On 2017/04/11 15:57:48, Dan Beam wrote: > lgtm but C++ -> JS tests are kind funky and flaky, IMO I take it you mean in general, not just these specific tests look flaky to you. ;) > i'd recommend testing the handlers and JS separately. clamity@ has a replacement for the WebUI browser-test, to use the JS framework for that instead, I believe, which seems to be the new preferred way.
https://codereview.chromium.org/2788413003/diff/140001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.h (right): https://codereview.chromium.org/2788413003/diff/140001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.h:149: mojom::SiteEngagementDetails GetDetails() const; On 2017/04/11 05:28:06, dcheng wrote: > FWIW, this seems potentially dicey. I think this is internally relying on the > fact that there's an implicitly defined copy constructor. It'll probably break > if someone ever adds a move-only field to the mojo struct. Checked w/ yzshen: Intention is that Mojo structs behave similarly to C++ structs, i.e. unless someone decides to add something that would break pass-by-value, pass-by-value should be valid. https://codereview.chromium.org/2788413003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc (right): https://codereview.chromium.org/2788413003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/engagement/site_engagement_ui_browsertest.cc:93: ""), On 2017/04/11 15:57:48, Dan Beam wrote: > can you `git cl format` this? is it already? Yes, this is already git cl formatted :-/
The CQ bit was checked by wez@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/11 19:56:49, Wez wrote: > On 2017/04/11 15:57:48, Dan Beam wrote: > > lgtm but C++ -> JS tests are kind funky and flaky, IMO > > I take it you mean in general, not just these specific tests look flaky to you. > ;) no, these probably are as well. most things that do browser<->renderer communication flake after a while. they at least have more moving parts than isolated, same-process tests. > > > i'd recommend testing the handlers and JS separately. > > clamity@ has a replacement for the WebUI browser-test, to use the JS framework > for that instead, I believe, which seems to be the new preferred way. clamy@ or calamity@?
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1491940634681570,
"parent_rev": "1580e6ff2376812aef62b6e4ca840a5399f0cea2", "commit_rev":
"9be0aad26957e8d00dd879fc1a188c0738dc3255"}
Message was sent while issue was closed.
Description was changed from ========== Add SiteEngagementService::GetAllDetails(), to return detailed scores. The new API provides details of the (decayed) base score, and any bonus scores e.g. due to granted permissions, in addition to the total score. It will be used by the important sites implementation to avoid the need for querying the bonus factors separately from the site engagement. This CL makes the following changes: * Renames SiteEngagement(Info->Details). * Add base & bonus score fields to SiteEngagementDetails. * Adds SiteEngagementScore::GetDetails() and renames GetScore() to GetTotalScore(), for clarity. * Adds SiteEngagementService::GetAllDetails(), which returns details for all sites with non-zero engagement score (i.e. including sites which have no direct engagement, but receive a bonus). * Adds some unit tests, for details contents and the WebUI score format. * Some minor cleanups (e.g. std::string() -> ResourceIdentifier() where appropriate). BUG=703848 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add SiteEngagementService::GetAllDetails(), to return detailed scores. The new API provides details of the (decayed) base score, and any bonus scores e.g. due to granted permissions, in addition to the total score. It will be used by the important sites implementation to avoid the need for querying the bonus factors separately from the site engagement. This CL makes the following changes: * Renames SiteEngagement(Info->Details). * Add base & bonus score fields to SiteEngagementDetails. * Adds SiteEngagementScore::GetDetails() and renames GetScore() to GetTotalScore(), for clarity. * Adds SiteEngagementService::GetAllDetails(), which returns details for all sites with non-zero engagement score (i.e. including sites which have no direct engagement, but receive a bonus). * Adds some unit tests, for details contents and the WebUI score format. * Some minor cleanups (e.g. std::string() -> ResourceIdentifier() where appropriate). BUG=703848 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2788413003 Cr-Commit-Position: refs/heads/master@{#463772} Committed: https://chromium.googlesource.com/chromium/src/+/9be0aad26957e8d00dd879fc1a18... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/9be0aad26957e8d00dd879fc1a18... |
