Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(72)

Issue 2748103011: Grant origins engagement for having interactions on their notifications. (Closed)

Created:
3 years, 9 months ago by dominickn
Modified:
3 years, 9 months ago
CC:
chromium-reviews, mlamouri+watch-notifications_chromium.org, Peter Beverloo, dominickn+watch_chromium.org, awdf+watch_chromium.org, agrieve+watch_chromium.org, chrome-apps-syd-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Grant origins engagement for having interactions on their notifications. This CL implements support for interactions on persistent (i.e. ServiceWorker-based) notifications to generate site engagement. A new engagement type and points bucket is added for notification interactions. In the future it would be better to have notification observers so the site engagement service itself can directly observe notifications without needing to expose HandleNotificationInteraction() publicly. Several tests are added and extended to ensure that the correct behaviour is exhibited on Android and desktop when users: - click on a notification (increase engagement) - click on an action button in a notification (increase engagement) - click on the settings button (no action) - close the notification (no action) This CL also corrects some errors in histograms.xml as well as adding the new engagement type. BUG=679336 Review-Url: https://codereview.chromium.org/2748103011 Cr-Commit-Position: refs/heads/master@{#458021} Committed: https://chromium.googlesource.com/chromium/src/+/b8a53b39667bab5aa73c5526b33bfdb04ea5351b

Patch Set 1 #

Total comments: 1

Patch Set 2 : Histograms #

Patch Set 3 : Remove some unused includes #

Total comments: 8

Patch Set 4 : Address comments #

Patch Set 5 : Unfriend #

Unified diffs Side-by-side diffs Delta from patch set Stats (+268 lines, -46 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/engagement/SiteEngagementService.java View 2 chunks +8 lines, -0 lines 0 comments Download
M chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java View 1 2 3 18 chunks +55 lines, -3 lines 0 comments Download
M chrome/browser/engagement/site_engagement_metrics.h View 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/browser/engagement/site_engagement_observer.h View 1 chunk +4 lines, -3 lines 0 comments Download
M chrome/browser/engagement/site_engagement_score.h View 4 chunks +9 lines, -10 lines 0 comments Download
M chrome/browser/engagement/site_engagement_score.cc View 1 2 3 3 chunks +29 lines, -23 lines 0 comments Download
M chrome/browser/engagement/site_engagement_service.h View 1 2 3 4 2 chunks +5 lines, -0 lines 0 comments Download
M chrome/browser/engagement/site_engagement_service.cc View 1 2 3 4 4 chunks +20 lines, -3 lines 0 comments Download
M chrome/browser/engagement/site_engagement_service_android.cc View 2 chunks +5 lines, -0 lines 0 comments Download
M chrome/browser/engagement/site_engagement_service_unittest.cc View 3 chunks +55 lines, -0 lines 0 comments Download
M chrome/browser/notifications/platform_notification_service_impl.h View 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/notifications/platform_notification_service_impl.cc View 1 2 3 chunks +12 lines, -0 lines 0 comments Download
M chrome/browser/notifications/platform_notification_service_interactive_uitest.cc View 16 chunks +56 lines, -2 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 1 chunk +4 lines, -2 lines 0 comments Download

Messages

Total messages: 32 (23 generated)
dominickn
benwells: PTAL at engagement peter: PTAL at notifications dfalcantara: PTAL at SiteEngagementServiceAndroid Thanks! https://codereview.chromium.org/2748103011/diff/1/chrome/browser/engagement/site_engagement_score.h File ...
3 years, 9 months ago (2017-03-16 06:36:55 UTC) #4
dominickn
And +isherman for histograms.xml
3 years, 9 months ago (2017-03-16 07:03:38 UTC) #8
Peter Beverloo
LGTM! Thank you! :) https://codereview.chromium.org/2748103011/diff/40001/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java (right): https://codereview.chromium.org/2748103011/diff/40001/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java#newcode106 chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:106: ThreadUtils.runOnUiThreadBlocking(new Runnable() { Optional comment: ...
3 years, 9 months ago (2017-03-16 12:24:14 UTC) #13
gone
SiteEngagementServiceAndroid lgtm
3 years, 9 months ago (2017-03-16 17:01:59 UTC) #14
Ilya Sherman
LGTM assuming the corrections to histograms.xml are fixing something that was always wrong, as opposed ...
3 years, 9 months ago (2017-03-16 22:22:35 UTC) #15
dominickn
Thanks! https://codereview.chromium.org/2748103011/diff/40001/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java (right): https://codereview.chromium.org/2748103011/diff/40001/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java#newcode106 chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:106: ThreadUtils.runOnUiThreadBlocking(new Runnable() { On 2017/03/16 12:24:14, Peter Beverloo ...
3 years, 9 months ago (2017-03-16 23:25:03 UTC) #16
benwells
lgtm
3 years, 9 months ago (2017-03-20 02:50:32 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2748103011/80001
3 years, 9 months ago (2017-03-20 02:52:09 UTC) #29
commit-bot: I haz the power
3 years, 9 months ago (2017-03-20 07:20:56 UTC) #32
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/b8a53b39667bab5aa73c5526b33b...

Powered by Google App Engine
This is Rietveld 408576698