Push API: Return cached registration if available.

Push API: Return cached registration if available.

This is the implementation for the following steps of the register
algorithm[1]:

    'If the webapp is already registered, run the following substeps:
     1. Retrieve the push registration associated with the webapp.
     2. If there is an error, reject promise with a DOMException whose
        name is "AbortError" and terminate these steps.
     3. When the request has been completed, resolve promise with a
        PushRegistration providing the details of the retrieved push
        registration.'

Currently the implementation assumes that errors can be recovered from
and attempts to do a fresh push registration if accessing storage
fails; I left a TODO for us to decide this case later.

Depends on https://codereview.chromium.org/785983002

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

BUG=401422, 429212
Committed: https://crrev.com/ce39d41c13db11a69564ba2299f7e83d3fa05c3c
Cr-Commit-Position: refs/heads/master@{#308135}

[johnme, 2014-12-08 18:56:41]

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

[johnme, 2014-12-08 18:56:41]

mvanouwerkerk@chromium.org: Please review overall patch.

avi@chromium.org: Please approve changes to
content/public/common/push_messaging_status.*

isherman@chromium.org: Please review changes to
tools/metrics/histograms/histograms.xml
(note that I'm renumbering a UMA enum, in order to keep related enum values
together; hopefully this is acceptable since the feature is still behind
--experimental-web-platform-features and has very little usage so far, but if
you'd rather I'd be happy to just at the new value at the end).

[Ilya Sherman, 2014-12-09 00:30:06]

On 2014/12/08 18:56:41, johnme wrote:
> mailto:isherman@chromium.org: Please review changes to
> tools/metrics/histograms/histograms.xml
> (note that I'm renumbering a UMA enum, in order to keep related enum values
> together; hopefully this is acceptable since the feature is still behind
> --experimental-web-platform-features and has very little usage so far, but if
> you'd rather I'd be happy to just at the new value at the end).

I'd prefer, if you renumber the enum, that you also rename the corresponding
histogram (and copy/rename the enum), so that old data isn't mixed in using the
incorrect labels.  However, it's up to you -- if you're quite confident that the
incorrect labels for old data won't cause any confusion, then renumbering is ok.

[Ilya Sherman, 2014-12-09 00:30:53]

https://codereview.chromium.org/778243002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/778243002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:53743: +  <int value="1"
label="Successful (from cache)"/>
Is the is a subcase of case 0, or complementary?  Please clarify in the labels.

[Avi (use Gerrit), 2014-12-09 06:22:52]

content/public lgtm

[Michael van Ouwerkerk, 2014-12-09 11:34:56]

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

https://codereview.chromium.org/778243002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_browsertest.cc:193: ASSERT_EQ("ok -
service worker registered", script_result);
It would be nice to check that when an assertion triggers, execution of the test
actually stops and the assertion message is surfaced.

[johnme, 2014-12-11 16:15:17]

Addressed review comments

On 2014/12/09 00:30:06, Ilya Sherman wrote:
> On 2014/12/08 18:56:41, johnme wrote:
> > mailto:isherman@chromium.org: Please review changes to
> > tools/metrics/histograms/histograms.xml
> > (note that I'm renumbering a UMA enum, in order to keep related enum values
> > together; hopefully this is acceptable since the feature is still behind
> > --experimental-web-platform-features and has very little usage so far, but
if
> > you'd rather I'd be happy to just at the new value at the end).
> 
> I'd prefer, if you renumber the enum, that you also rename the corresponding
> histogram (and copy/rename the enum), so that old data isn't mixed in using
the
> incorrect labels.  However, it's up to you -- if you're quite confident that
the
> incorrect labels for old data won't cause any confusion, then renumbering is
ok.

There is some old data[1]. It's not super useful, but I guess it would be a
shame to mangle it. I ended up not renumbering the enum, since I think the code
complexity cost of oddly numbered enum values is less than the complexity cost
of having two enums.

[1]:
https://uma.googleplex.com/histograms/json_dashboard.html?dayCount=7&endDate=...

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

https://codereview.chromium.org/778243002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_browsertest.cc:193: ASSERT_EQ("ok -
service worker registered", script_result);
On 2014/12/09 11:34:56, Michael van Ouwerkerk wrote:
> It would be nice to check that when an assertion triggers, execution of the
test
> actually stops and the assertion message is surfaced.

Assertion messages from helper messages are correctly surfaced, however
execution doesn't stop properly (it early-outs from the helper function, but not
the parent function). So I've changed these all to EXPECT instead of ASSERT to
avoid confusion.

https://codereview.chromium.org/778243002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/778243002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:53743: +  <int value="1"
label="Successful (from cache)"/>
On 2014/12/09 00:30:53, Ilya Sherman wrote:
> Is the is a subcase of case 0, or complementary?  Please clarify in the
labels.

Done.

[Michael van Ouwerkerk, 2014-12-11 17:12:53]

https://codereview.chromium.org/778243002/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/778243002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_browsertest.cc:150: void
LoadSubscope1TestPage() {
These methods repeat a lot of boilerplate and do not get called all that often.
How about instead adding an overload for loadTestPage that accepts a url, and
making loadTestPage call its overload?

https://codereview.chromium.org/778243002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_browsertest.cc:208: 0LL /*
service_worker_registration_id */);
Is the rest of this file doing something significantly different by using only
one L ? Let's make it all consistent.

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

https://codereview.chromium.org/778243002/diff/40001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.cc:202: GURL
push_endpoint(service()->GetPushEndpoint());
The service() method is UI thread only, we're on the IO thread here.

[johnme, 2014-12-11 18:32:34]

Addressed Michael's review comments v2

https://codereview.chromium.org/778243002/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/778243002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_browsertest.cc:150: void
LoadSubscope1TestPage() {
On 2014/12/11 17:12:53, Michael van Ouwerkerk wrote:
> These methods repeat a lot of boilerplate and do not get called all that
often.
> How about instead adding an overload for loadTestPage that accepts a url, and
> making loadTestPage call its overload?

Done (fixed the capitalization while I was at it ;)

https://codereview.chromium.org/778243002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_browsertest.cc:208: 0LL /*
service_worker_registration_id */);
On 2014/12/11 17:12:53, Michael van Ouwerkerk wrote:
> Is the rest of this file doing something significantly different by using only
> one L ? Let's make it all consistent.

Done.

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

https://codereview.chromium.org/778243002/diff/40001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.cc:202: GURL
push_endpoint(service()->GetPushEndpoint());
On 2014/12/11 17:12:53, Michael van Ouwerkerk wrote:
> The service() method is UI thread only, we're on the IO thread here.

Done (sigh, more thread hops 8-)

[Michael van Ouwerkerk, 2014-12-11 18:53:11]

lgtm thanks John!

[Ilya Sherman, 2014-12-11 23:38:55]

histograms lgtm -- thanks

[johnme, 2014-12-12 09:55:40]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2014-12-12 09:56:42]

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

[commit-bot: I haz the power, 2014-12-12 10:00:58]

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

[commit-bot: I haz the power, 2014-12-12 10:00:59]

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

[johnme, 2014-12-12 14:46:31]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2014-12-12 14:47:30]

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

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

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

[commit-bot: I haz the power, 2014-12-12 15:08:23]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[johnme, 2014-12-12 17:49:24]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2014-12-12 17:49:48]

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

[commit-bot: I haz the power, 2014-12-12 18:59:24]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-12 19:00:19]

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