Push API: Store push registration IDs for GetRegistration etc

Push API: Store push registration IDs for GetRegistration etc

Tests will be added by the use-case patches enabled by this, e.g. when
GetRegistration (https://crbug.com/401422) is implemented, it will come
with tests ensuring that the registration is properly stored.

BUG=429212
Committed: https://crrev.com/ecf45b63ca76adfd846146137e5620f0a895f92e
Cr-Commit-Position: refs/heads/master@{#307270}

[johnme, 2014-12-05 19:19:06]

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

[johnme, 2014-12-05 19:19:06]

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

[johnme, 2014-12-05 19:19:06]

mvanouwerkerk: please review overall patch, especially:
- content/browser/push_messaging/push_messaging_message_filter.h
- content/browser/push_messaging/push_messaging_message_filter.cc

avi: please approve changes to:
- content/public/common/push_messaging_status.h
- content/public/common/push_messaging_status.cc

isherman: please approve changes to:
- tools/metrics/histograms/histograms.xml

Thanks all!

[johnme, 2014-12-05 19:19:06]

mvanouwerkerk: please review overall patch, especially:
- content/browser/push_messaging/push_messaging_message_filter.h
- content/browser/push_messaging/push_messaging_message_filter.cc

avi: please approve changes to:
- content/public/common/push_messaging_status.h
- content/public/common/push_messaging_status.cc

isherman: please approve changes to:
- tools/metrics/histograms/histograms.xml

Thanks all!

[Ilya Sherman, 2014-12-05 20:41:49]

histograms.xml lgtm

[Avi (use Gerrit), 2014-12-05 20:50:23]

https://codereview.chromium.org/758403003/diff/40001/content/public/common/pu...
File content/public/common/push_messaging_status.h (right):

https://codereview.chromium.org/758403003/diff/40001/content/public/common/pu...
content/public/common/push_messaging_status.h:34: // Registration suceeded, but
we failed to persist it.
s/suceeded/succeeded/

https://codereview.chromium.org/758403003/diff/40001/content/public/common/pu...
content/public/common/push_messaging_status.h:35:
PUSH_REGISTRATION_STATUS_STORAGE_ERROR = 7,
I'm curious: if a registration succeeded but wasn't persisted, is it a useful
registration? What is the caller supposed to do upon getting this reply?

[johnme, 2014-12-05 21:56:33]

Replies to avi's comments - thanks.

https://codereview.chromium.org/758403003/diff/40001/content/public/common/pu...
File content/public/common/push_messaging_status.h (right):

https://codereview.chromium.org/758403003/diff/40001/content/public/common/pu...
content/public/common/push_messaging_status.h:34: // Registration suceeded, but
we failed to persist it.
On 2014/12/05 20:50:22, Avi wrote:
> s/suceeded/succeeded/

Done.

https://codereview.chromium.org/758403003/diff/40001/content/public/common/pu...
content/public/common/push_messaging_status.h:35:
PUSH_REGISTRATION_STATUS_STORAGE_ERROR = 7,
On 2014/12/05 20:50:22, Avi wrote:
> I'm curious: if a registration succeeded but wasn't persisted, is it a useful
> registration? What is the caller supposed to do upon getting this reply?

As well as it being one of the outcomes we log to UMA (to detect failure modes),
the caller will bubble up the error to JavaScript. The spec says[1] to fire a
DOMException whose `name` is "AbortError" for an errors that occur during
registration. But we'll set the `message`[2] of the DOMException to the string
produced by content::PushRegistrationStatusToString.

Thus a web developer who encounters this error
a) might be better able to guess at the cause,
b) can include it on stackoverflow and bug reports etc,
c) they or we will then be able to grep through the chromium source to find the
relevant code.

[1]:
https://w3c.github.io/push-api/#widl-PushRegistrationManager-register-Promise...

[2]: http://heycam.github.io/webidl/#dfn-exception-message

[Avi (use Gerrit), 2014-12-05 22:02:55]

lgtm

Hm. OK.

[Michael van Ouwerkerk, 2014-12-08 14:36:55]

not lgtm due to threading mismatch, sorry John :-)

https://codereview.chromium.org/758403003/diff/60001/content/browser/push_mes...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/758403003/diff/60001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.cc:47: static const
char kPushRegistrationIdServiceWorkerKey[] =
Nit: move this inside the anonymous namespace.

https://codereview.chromium.org/758403003/diff/60001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.cc:244:
base::Bind(&PushMessagingMessageFilter::DoPersistRegistration,
Nit: rename DoPersistRegistration to PersistRegistrationOnIO, it seems easier to
read the reason for this thread jump. Then DidPersistRegistration would become
DidPersistRegistrationOnIO.

https://codereview.chromium.org/758403003/diff/60001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.cc:270: if
(service_worker_status == SERVICE_WORKER_OK) {
Nit: omit curly braces for these one-liners.

https://codereview.chromium.org/758403003/diff/60001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.cc:279: // Called
from both UI and IO threads.
Nit: don't be overly prescriptive. How about: "May be called from both UI and IO
threads."

https://codereview.chromium.org/758403003/diff/60001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.cc:292:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
This is very dodgy, because the service must only be accessed on the UI thread.
I just committed a check for that so the call to service() on the next line
should be DCHECKing now.

https://codereview.chromium.org/758403003/diff/60001/content/browser/push_mes...
File content/browser/push_messaging/push_messaging_message_filter.h (right):

https://codereview.chromium.org/758403003/diff/60001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.h:101: // Should
only be used for asynchronous calls to ServiceWorkerStorage on the
Please don't be overly prescriptive. You can call to ServiceWorkerStorage, but
also do anything else on the IO thread. How about this:

"Should only be used for asynchronous calls on the IO thread to dependencies
that might outlive this class e.g. ServiceWorkerStorage."

[johnme, 2014-12-08 16:16:33]

Addressed Michael's review comments - thanks!

https://codereview.chromium.org/758403003/diff/60001/content/browser/push_mes...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/758403003/diff/60001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.cc:47: static const
char kPushRegistrationIdServiceWorkerKey[] =
On 2014/12/08 14:36:54, Michael van Ouwerkerk wrote:
> Nit: move this inside the anonymous namespace.

Done.

https://codereview.chromium.org/758403003/diff/60001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.cc:244:
base::Bind(&PushMessagingMessageFilter::DoPersistRegistration,
On 2014/12/08 14:36:54, Michael van Ouwerkerk wrote:
> Nit: rename DoPersistRegistration to PersistRegistrationOnIO, it seems easier
to
> read the reason for this thread jump. Then DidPersistRegistration would become
> DidPersistRegistrationOnIO.

Done.

https://codereview.chromium.org/758403003/diff/60001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.cc:270: if
(service_worker_status == SERVICE_WORKER_OK) {
On 2014/12/08 14:36:54, Michael van Ouwerkerk wrote:
> Nit: omit curly braces for these one-liners.

Done.

https://codereview.chromium.org/758403003/diff/60001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.cc:279: // Called
from both UI and IO threads.
On 2014/12/08 14:36:54, Michael van Ouwerkerk wrote:
> Nit: don't be overly prescriptive. How about: "May be called from both UI and
IO
> threads."

Done.

https://codereview.chromium.org/758403003/diff/60001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.cc:292:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
On 2014/12/08 14:36:54, Michael van Ouwerkerk wrote:
> This is very dodgy, because the service must only be accessed on the UI
thread.
> I just committed a check for that so the call to service() on the next line
> should be DCHECKing now.

Good spot, done (I now get it on the UI thread, and pass it along).

https://codereview.chromium.org/758403003/diff/60001/content/browser/push_mes...
File content/browser/push_messaging/push_messaging_message_filter.h (right):

https://codereview.chromium.org/758403003/diff/60001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.h:101: // Should
only be used for asynchronous calls to ServiceWorkerStorage on the
On 2014/12/08 14:36:54, Michael van Ouwerkerk wrote:
> Please don't be overly prescriptive. You can call to ServiceWorkerStorage, but
> also do anything else on the IO thread. How about this:
> 
> "Should only be used for asynchronous calls on the IO thread to dependencies
> that might outlive this class e.g. ServiceWorkerStorage."

Done.

[Michael van Ouwerkerk, 2014-12-08 16:18:12]

lgtm thanks!

[johnme, 2014-12-08 16:18:30]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2014-12-08 16:19:31]

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

[commit-bot: I haz the power, 2014-12-08 16:22:35]

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

[commit-bot: I haz the power, 2014-12-08 16:22:36]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[johnme, 2014-12-08 16:33:01]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2014-12-08 16:33:41]

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

[commit-bot: I haz the power, 2014-12-08 17:34:17]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-08 17:35:10]

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