Push API: Grace - allow one in ten pushes to show no notification.

Push API: Grace - allow one in ten pushes to show no notification.

For developers who have opted in to showing a user-visible UI change on
every push (in exchange for showing the user a less scary permission
prompt), we currently[1] enforce that each push shows a notification.

This patch allows one in ten pushes to show no notification, so
occasional bugs in developer push handlers don't needlessly spam the
user.

Specifically, it keeps track of whether the last (up to) 10 push
messages showed a notification (ignoring pushes didn't show a
notification but were exempt, e.g. due to https://crrev.com/866443003)
and only shows a forced notification if one of those (up to) 10 also
failed to show a notification.

[1]: (since https://crrev.com/842233003)

BUG=437277
Committed: https://crrev.com/8b5b00d6cc8f02139c1d15209612ab4126de5add
Cr-Commit-Position: refs/heads/master@{#314851}

[johnme, 2015-02-04 14:00:34]

johnme@chromium.org changed reviewers:
+ avi@chromium.org, mvanouwerkerk@chromium.org

[johnme, 2015-02-04 14:00:35]

avi: please review content/public/browser/push_messaging_service.{h,cc}
I added static methods to PushMessagingService as recommended by jochen/michaeln
in https://codereview.chromium.org/875573002/

mvanouwerkerk: please review overall patch, especially
chrome/browser/services/gcm/push_messaging_*

[Michael van Ouwerkerk, 2015-02-04 15:51:33]

https://codereview.chromium.org/883743002/diff/20001/content/public/browser/p...
File content/public/browser/push_messaging_service.cc (right):

https://codereview.chromium.org/883743002/diff/20001/content/public/browser/p...
content/public/browser/push_messaging_service.cc:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
Doing this much work in static methods seems permitted by [1] but only rarely.
I'll leave this to a public content owner to review whether this is appropriate.

[1] http://www.chromium.org/developers/content-module/content-api

https://codereview.chromium.org/883743002/diff/20001/content/public/browser/p...
content/public/browser/push_messaging_service.cc:36:
ServiceWorkerContextWrapper* service_worker_context_wrapper,
Shouldn't this be a scoped_refptr? Otherwise this wrapper may well be gone by
now. In fact this looks like an implicit conversion from scoped_refptr to raw,
not quite what you want.

https://codereview.chromium.org/883743002/diff/20001/content/public/browser/p...
content/public/browser/push_messaging_service.cc:46:
ServiceWorkerContextWrapper* service_worker_context_wrapper,
Ditto.

https://codereview.chromium.org/883743002/diff/20001/content/public/browser/p...
File content/public/browser/push_messaging_service.h (right):

https://codereview.chromium.org/883743002/diff/20001/content/public/browser/p...
content/public/browser/push_messaging_service.h:37: // Worker registration.
Stored data is deleted when the associated registraton
s/registraton/registration/

[Avi (use Gerrit), 2015-02-04 16:16:57]

I must admit to being rather confused as to the whole UserData thing used here
and implemented in https://codereview.chromium.org/875573002/. First, we already
_have_ UserData: base/supports_user_data.h. Second, how does this fit in to how
content calls back to chrome? I haven't been involved in API changes in a while,
but that was usually through ContentClient. My brain is a bit fuzzy here.

https://codereview.chromium.org/883743002/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/883743002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:298: // as the
Service Worker can't talk to it, even with navigator.connect.
rewrap the comment to stay under 80 columns

https://codereview.chromium.org/883743002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:333: // off the
beginning once the history length is exceeded.
:(

I'd much rather have a type like std::bitset or std::vector<ShownEnumType> or
even a std::deque<bool> than the magical "1 = shown, 0 = not shown".

https://codereview.chromium.org/883743002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:368: char new_entry =
notification_shown ? '1' : '0';
:( // see above

[johnme, 2015-02-04 17:57:41]

Thanks, addressed review comments.

avi wrote:
> I must admit to being rather confused as to the whole UserData
> thing used here and implemented in
> https://codereview.chromium.org/875573002/. First, we already
> _have_ UserData: base/supports_user_data.h. Second, how does
> this fit in to how content calls back to chrome? I haven't been
> involved in API changes in a while, but that was usually through
> ContentClient. My brain is a bit fuzzy here.

ServiceWorkerStorage has GetUserData, StoreUserData and ClearUserData methods
(added by https://crbug.com/435519) which allow content layer code to associate
string:string key:value pairs with SW registrations.

Unlike SupportsUserData, these gets persisted to the SW database, and hence
survives browser restarts (though in hindsight, the SW methods should probably
have been named differently).

https://codereview.chromium.org/875573002/ was my initial attempt at allowing
chrome-layer code to save data using the same mechanism, but I've abandoned that
since jochen/michaeln thought it was too general-purpose, and prefered a more
tightly scoped API. Hence instead I've added these methods to
PushMessagingService to allow chrome-layer to get/set the
notifications_shown_by_last_few_pushes key only.

https://codereview.chromium.org/883743002/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/883743002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:298: // as the
Service Worker can't talk to it, even with navigator.connect.
On 2015/02/04 16:16:57, Avi wrote:
> rewrap the comment to stay under 80 columns

Done.

https://codereview.chromium.org/883743002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:333: // off the
beginning once the history length is exceeded.
On 2015/02/04 16:16:57, Avi wrote:
> :(
> 
> I'd much rather have a type like std::bitset or std::vector<ShownEnumType> or
> even a std::deque<bool> than the magical "1 = shown, 0 = not shown".

Done (I switched to std::bitset since it can easily be converted to/from
std::string, which is the only datatype the Service Worker database's user data
mechanism currently supports).

https://codereview.chromium.org/883743002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:368: char new_entry =
notification_shown ? '1' : '0';
On 2015/02/04 16:16:57, Avi wrote:
> :( // see above

Done.

https://codereview.chromium.org/883743002/diff/20001/content/public/browser/p...
File content/public/browser/push_messaging_service.cc (right):

https://codereview.chromium.org/883743002/diff/20001/content/public/browser/p...
content/public/browser/push_messaging_service.cc:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2015/02/04 15:51:33, Michael van Ouwerkerk wrote:
> Doing this much work in static methods seems permitted by [1] but only rarely.
> I'll leave this to a public content owner to review whether this is
appropriate.
> 
> [1] http://www.chromium.org/developers/content-module/content-api

Acknowledged. I'm doing this based on jochen/michaeln's comments in
https://codereview.chromium.org/875573002/ (and the precedent of a fair amount
of existing .cc files in content/public/browser), but alternatives welcome.

https://codereview.chromium.org/883743002/diff/20001/content/public/browser/p...
content/public/browser/push_messaging_service.cc:36:
ServiceWorkerContextWrapper* service_worker_context_wrapper,
On 2015/02/04 15:51:33, Michael van Ouwerkerk wrote:
> Shouldn't this be a scoped_refptr? Otherwise this wrapper may well be gone by
> now. In fact this looks like an implicit conversion from scoped_refptr to raw,
> not quite what you want.

Done (yeah, I deliberately used scoped_refptr<ServiceWorkerContextWrapper> below
to make sure the wrapper wouldn't be destroyed while thread hopping, then forgot
to update these - oops).

https://codereview.chromium.org/883743002/diff/20001/content/public/browser/p...
content/public/browser/push_messaging_service.cc:46:
ServiceWorkerContextWrapper* service_worker_context_wrapper,
On 2015/02/04 15:51:33, Michael van Ouwerkerk wrote:
> Ditto.

Done.

https://codereview.chromium.org/883743002/diff/20001/content/public/browser/p...
File content/public/browser/push_messaging_service.h (right):

https://codereview.chromium.org/883743002/diff/20001/content/public/browser/p...
content/public/browser/push_messaging_service.h:37: // Worker registration.
Stored data is deleted when the associated registraton
On 2015/02/04 15:51:33, Michael van Ouwerkerk wrote:
> s/registraton/registration/

Done.

[Avi (use Gerrit), 2015-02-04 21:24:22]

lgtm

https://codereview.chromium.org/883743002/diff/40001/content/public/browser/p...
File content/public/browser/push_messaging_service.cc (right):

https://codereview.chromium.org/883743002/diff/40001/content/public/browser/p...
content/public/browser/push_messaging_service.cc:12: static const char
kNotificationsShownServiceWorkerKey[] =
Don't use static in this way; use an anonymous namespace.

[Michael van Ouwerkerk, 2015-02-05 11:23:20]

lgtm

[johnme, 2015-02-05 11:33:29]

New patchsets have been uploaded after l-g-t-m from
avi@chromium.org,mvanouwerkerk@chromium.org

[johnme, 2015-02-05 11:34:26]

Thanks :)

https://codereview.chromium.org/883743002/diff/40001/content/public/browser/p...
File content/public/browser/push_messaging_service.cc (right):

https://codereview.chromium.org/883743002/diff/40001/content/public/browser/p...
content/public/browser/push_messaging_service.cc:12: static const char
kNotificationsShownServiceWorkerKey[] =
On 2015/02/04 21:24:22, Avi wrote:
> Don't use static in this way; use an anonymous namespace.

Done.

[johnme, 2015-02-05 11:34:31]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2015-02-05 11:35:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/883743002/60001

[commit-bot: I haz the power, 2015-02-05 11:45:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-05 11:45:47]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[johnme, 2015-02-05 12:42:48]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-02-05 12:43:03]

The CQ bit was unchecked by johnme@chromium.org

[johnme, 2015-02-05 18:06:20]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2015-02-05 18:07:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/883743002/100001

[commit-bot: I haz the power, 2015-02-05 19:16:52]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-05 19:18:39]

Patchset 6 (id:??) landed as
https://crrev.com/8b5b00d6cc8f02139c1d15209612ab4126de5add
Cr-Commit-Position: refs/heads/master@{#314851}