Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2737533003:
Grant bonus engagement to origins with notification permission. (Closed)
DescriptionGrant bonus engagement to origins with notification permission.
This CL implements an engagement bonus for origins which have been
granted notification permission.
BUG=679336
Review-Url: https://codereview.chromium.org/2737533003
Cr-Commit-Position: refs/heads/master@{#455893}
Committed: https://chromium.googlesource.com/chromium/src/+/fbb226e6e24e82cb0dcf4559510790e23d7665bc
Patch Set 1 #Patch Set 2 : Fix push messaging tests #
Total comments: 6
Patch Set 3 : Address comments #
Total comments: 4
Patch Set 4 : Address comments #
Messages
Total messages: 29 (20 generated)
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
dominickn@chromium.org changed reviewers: + benwells@chromium.org
PTAL, thanks!
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dominickn@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.
https://codereview.chromium.org/2737533003/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_score.cc (right): https://codereview.chromium.org/2737533003/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_score.cc:17: #include "chrome/browser/permissions/permission_manager.h" is this include used? https://codereview.chromium.org/2737533003/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service_unittest.cc (right): https://codereview.chromium.org/2737533003/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service_unittest.cc:494: GURL url3("http://www.google.com/maps"); Should this be a different origin? https://codereview.chromium.org/2737533003/diff/20001/chrome/browser/push_mes... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/2737533003/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/push_messaging_browsertest.cc:284: service->ResetScoreForURL(url, score); This name is a bit confusing - should we make it ResetBaseScoreForURL?
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
dominickn@chromium.org changed reviewers: + peter@chromium.org
Thanks! +peter: PTAL at push, thanks! https://codereview.chromium.org/2737533003/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_score.cc (right): https://codereview.chromium.org/2737533003/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_score.cc:17: #include "chrome/browser/permissions/permission_manager.h" On 2017/03/08 04:19:32, benwells wrote: > is this include used? It was, but that was unfeasible. Removed! https://codereview.chromium.org/2737533003/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service_unittest.cc (right): https://codereview.chromium.org/2737533003/diff/20001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service_unittest.cc:494: GURL url3("http://www.google.com/maps"); On 2017/03/08 04:19:33, benwells wrote: > Should this be a different origin? Derp, yes. https://codereview.chromium.org/2737533003/diff/20001/chrome/browser/push_mes... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/2737533003/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/push_messaging_browsertest.cc:284: service->ResetScoreForURL(url, score); On 2017/03/08 04:19:33, benwells wrote: > This name is a bit confusing - should we make it ResetBaseScoreForURL? Filed crbug.com/699381
lgtm
peter@chromium.org changed reviewers: + harkness@chromium.org
Sorry, missed this! Thank you for doing this! :) One implication of this is that the site engagement score of an origin that has notification permission granted will never drop down to zero. Instead, it'll drop down to five. In effect, if my calculation is correct, this means that sites that get no engagement through other means (e.g. push aggregators) will get 0.3 budget per day, thus a free silent push message per week, solely for having the permission granted. I have no concerns about that, so lgtm. +harkness to verify https://codereview.chromium.org/2737533003/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_score.cc (right): https://codereview.chromium.org/2737533003/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_score.cc:385: origin_, origin_, CONTENT_SETTINGS_TYPE_NOTIFICATIONS, Per NotificationPermissionContext::UpdateContentSetting and the DesktopNotificationProfileUtil, we store the content settings with [requesting origin, GURL()] as opposed to [requesting origin, embedding origin]. Here we use the origin twice. Does that matter? https://codereview.chromium.org/2737533003/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service_unittest.cc (right): https://codereview.chromium.org/2737533003/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service_unittest.cc:490: new SiteEngagementService(profile(), base::WrapUnique(clock))); nit: base::MakeUnique<> is preferred over raw `new` these days.
On 2017/03/09 15:41:12, Peter Beverloo wrote: > One implication of this is that the site engagement score of an origin that has > notification permission granted will never drop down to zero. Instead, it'll > drop down to five. > > In effect, if my calculation is correct, this means that sites that get no > engagement through other means (e.g. push aggregators) will get 0.3 budget per > day, thus a free silent push message per week, solely for having the permission > granted. > > I have no concerns about that, so lgtm. +harkness to verify Yup, your math is correct, they would get about 1 silent push per week. I think that's a reasonable level, given that the user has given a definite trust signal.
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Thanks! I'll be following up with a patch to increase engagement per interaction with a notification event too. https://codereview.chromium.org/2737533003/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_score.cc (right): https://codereview.chromium.org/2737533003/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_score.cc:385: origin_, origin_, CONTENT_SETTINGS_TYPE_NOTIFICATIONS, On 2017/03/09 15:41:12, Peter Beverloo wrote: > Per NotificationPermissionContext::UpdateContentSetting and the > DesktopNotificationProfileUtil, we store the content settings with [requesting > origin, GURL()] as opposed to [requesting origin, embedding origin]. > > Here we use the origin twice. Does that matter? I'm fairly sure it gets wiped in the content settings layer, but for safety I've switched to GURL() for the second argument. https://codereview.chromium.org/2737533003/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service_unittest.cc (right): https://codereview.chromium.org/2737533003/diff/40001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service_unittest.cc:490: new SiteEngagementService(profile(), base::WrapUnique(clock))); On 2017/03/09 15:41:12, Peter Beverloo wrote: > nit: base::MakeUnique<> is preferred over raw `new` these days. This is consistent with everything else in this file; we don't use MakeUnique because we want to keep a pointer to the clock to move the time around (the service takes ownership of the clock but we want to keep moving it from outside the service).
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.
The CQ bit was checked by dominickn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, benwells@chromium.org Link to the patchset: https://codereview.chromium.org/2737533003/#ps60001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489100046151320,
"parent_rev": "fede5b8ff09c1be5803be0a9f333f5e643d2877a", "commit_rev":
"fbb226e6e24e82cb0dcf4559510790e23d7665bc"}
Message was sent while issue was closed.
Description was changed from ========== Grant bonus engagement to origins with notification permission. This CL implements an engagement bonus for origins which have been granted notification permission. BUG=679336 ========== to ========== Grant bonus engagement to origins with notification permission. This CL implements an engagement bonus for origins which have been granted notification permission. BUG=679336 Review-Url: https://codereview.chromium.org/2737533003 Cr-Commit-Position: refs/heads/master@{#455893} Committed: https://chromium.googlesource.com/chromium/src/+/fbb226e6e24e82cb0dcf45595107... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/fbb226e6e24e82cb0dcf45595107... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
