Partial implementation of subscription restrictions.

Partial implementation of subscription restrictions.

Currently, chrome requires that app developers provide a "sender_id" tag in the
manifest. This id is generated by the developer console and is not standard
for all browsers.

In the future, the app developer will be able to specify a public key for their
service, which will be registered with the push service and which the push
service can use to validate app services requesting to send messages.

BUG=583753
Committed: https://crrev.com/71f5a08bb30153b56e8a3f9b447264a54b6d9c12
Cr-Commit-Position: refs/heads/master@{#379045}

[harkness, 2016-02-17 15:53:09]

harkness@chromium.org changed reviewers:
+ peter@chromium.org

[Michael van Ouwerkerk, 2016-02-17 15:54:20]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-17 15:55:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1701313002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1701313002/1

[commit-bot: I haz the power, 2016-02-17 16:13:33]

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

[commit-bot: I haz the power, 2016-02-17 16:13:34]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Peter Beverloo, 2016-02-17 16:15:36]

Thanks! Most significant comments in content_push_subscription_options.h.

Two other points:

  (1) We need to validate the public key somewhere, maybe twice. A naive check
Blink can do is to verify that len(key)==65 and key[0]==0x04.

  (2) We can increase test coverage on the Blink side a bit. Notably, testing
the type-checks of the PushSubscriptionOptions IDL would be good. (E.g. pass a
string or a number rather than an ArrayBuffer.)

If we check validity, we'll also want some basic sanity tests there. Perhaps
that could be extracted in a small class w/ unit tests?

https://codereview.chromium.org/1701313002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/platform_notification_service_browsertest.cc
(right):

https://codereview.chromium.org/1701313002/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_browsertest.cc:185:
command_line->AppendSwtich(switches::kEnableExperimentalWebPlatformFeatures);
AppendSwitch

https://codereview.chromium.org/1701313002/diff/1/chrome/test/data/push_messa...
File chrome/test/data/push_messaging/push_test.js (right):

https://codereview.chromium.org/1701313002/diff/1/chrome/test/data/push_messa...
chrome/test/data/push_messaging/push_test.js:16: using_public_key: false,
This boolean is not Web exposed ;-).

https://codereview.chromium.org/1701313002/diff/1/content/public/common/conte...
File content/public/common/content_push_subscription_options.h (right):

https://codereview.chromium.org/1701313002/diff/1/content/public/common/conte...
content/public/common/content_push_subscription_options.h:16: struct
CONTENT_EXPORT ContentPushSubscriptionOptions {
nit: no need to prefix with "Content", it's namespaced. (As is
blink::PushSubscriptionOptions, by the way.)

https://codereview.chromium.org/1701313002/diff/1/content/public/common/conte...
content/public/common/content_push_subscription_options.h:22: bool
user_visible_only;
Please make sure that we assign default values to scalar members, right now
they'll be initialized with garbage.

(Either in the constructor's initialization list, or in-line in the class
declaration.)

https://codereview.chromium.org/1701313002/diff/1/content/public/common/conte...
content/public/common/content_push_subscription_options.h:26: bool
using_public_key;
Why is it significant for the browser process to know this? This also relates to
the branch you have in push_messaging_message_filter.cc:280 - why would we store
them differently?

Theoretically a Web developer can pass the applicationServerKey in the
gcm_sender_id field of the manifest (which is of type string).

From the GCM point of view, we give it an opaque identifier. The //chrome layer
and the GCM Driver don't care whether this is a sender Id or a public key, it's
the server that verifies this.

This would change how we validate the input in the browser process a bit. Right
now we assume that whatever we get is valid and use it, but we can tighten that
up a little bit. The two options are:

  (1) A sender id, e.g. parseable as a number.
  (2) An application server key, which must be an uncompressed P-256 EC point of
65 bytes.

https://codereview.chromium.org/1701313002/diff/1/content/public/common/conte...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1701313002/diff/1/content/public/common/conte...
content/public/common/content_switches.cc:474: const char
kEnableSubscriptionRestrictions[] =
Right now, we're hiding the feature behind *two* command line flags:

--enable-experimental-web-platform-features (for everything except...)
--enable-subscription-restrictions (using a different key name for storing
user-data in the Service Worker database)

Let's drop the latter?

https://codereview.chromium.org/1701313002/diff/1/content/renderer/push_messa...
File content/renderer/push_messaging/push_messaging_dispatcher.cc (right):

https://codereview.chromium.org/1701313002/diff/1/content/renderer/push_messa...
content/renderer/push_messaging/push_messaging_dispatcher.cc:75:
switches::kEnableExperimentalWebPlatformFeatures) ||
No need to check for the command line flag— we can rely on Blink passing us an
empty applicationServerKey if the feature is disabled.

(When it does give us that value, we either have a bug in the bindings
generator, or the renderer has been compromised. I don't think we have to be
overly defensive for the first case, and in the latter case all bets are off
anyway.)

https://codereview.chromium.org/1701313002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/1701313002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/push_messaging/PushManager.cpp:42:
webOptions.applicationServerKey = WTF::String(static_cast<const
char*>(options.appServicePublicKey()->data()));
This isn't safe because the |applicationServerKey| may contain NULL bytes, which
would early-terminate the string. We need to use the length (byteLength())
explicitly.

I think we have two options here—

  (1) Store and pass the key as a WebVector<uint8_t>.
  (2) Creating a UTF16 string out of the binary data so that we can convert it
back to a UTF8 string (i.e. std::string) later on.


webOptions.applicationServerKey = WebString::fromUTF8(
    static_cast<const char*>(options.applicationServerKey()->data(),
                             options.applicationServerKey()->byteLength()));

https://codereview.chromium.org/1701313002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/push_messaging/PushSubscriptionOptions.idl
(right):

https://codereview.chromium.org/1701313002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/push_messaging/PushSubscriptionOptions.idl:9:
ArrayBuffer appServicePublicKey;
applicationServerKey

https://codereview.chromium.org/1701313002/diff/1/third_party/WebKit/public/p...
File
third_party/WebKit/public/platform/modules/push_messaging/WebPushSubscriptionOptions.h
(right):

https://codereview.chromium.org/1701313002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/modules/push_messaging/WebPushSubscriptionOptions.h:23:
// subscription.
Since it expects a specific format (uncompressed EC points), it would be good to
detail this in the comment:

  // P-256 public key, in uncompressed form, of the app server that can send
  // push messages to this subscription.

[Michael van Ouwerkerk, 2016-02-17 16:20:42]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-17 16:21:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1701313002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1701313002/1

[commit-bot: I haz the power, 2016-02-17 16:25:20]

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

[commit-bot: I haz the power, 2016-02-17 16:25:21]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[harkness, 2016-02-18 10:45:15]

I agree completely about adding more tests and format checking for the input,
but I thought you were getting nervous about the size of this patch. I'm happy
to either do those in this patch or in a subsequent one, just let me know which
you would prefer.

https://codereview.chromium.org/1701313002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/platform_notification_service_browsertest.cc
(right):

https://codereview.chromium.org/1701313002/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_browsertest.cc:185:
command_line->AppendSwtich(switches::kEnableExperimentalWebPlatformFeatures);
On 2016/02/17 16:15:35, Peter Beverloo wrote:
> AppendSwitch

Actually, I meant to revert this file. I'll do that now.

https://codereview.chromium.org/1701313002/diff/1/chrome/test/data/push_messa...
File chrome/test/data/push_messaging/push_test.js (right):

https://codereview.chromium.org/1701313002/diff/1/chrome/test/data/push_messa...
chrome/test/data/push_messaging/push_test.js:16: using_public_key: false,
On 2016/02/17 16:15:35, Peter Beverloo wrote:
> This boolean is not Web exposed ;-).

Done.

https://codereview.chromium.org/1701313002/diff/1/content/public/common/conte...
File content/public/common/content_push_subscription_options.h (right):

https://codereview.chromium.org/1701313002/diff/1/content/public/common/conte...
content/public/common/content_push_subscription_options.h:16: struct
CONTENT_EXPORT ContentPushSubscriptionOptions {
On 2016/02/17 16:15:35, Peter Beverloo wrote:
> nit: no need to prefix with "Content", it's namespaced. (As is
> blink::PushSubscriptionOptions, by the way.)

Done.

https://codereview.chromium.org/1701313002/diff/1/content/public/common/conte...
content/public/common/content_push_subscription_options.h:22: bool
user_visible_only;
On 2016/02/17 16:15:35, Peter Beverloo wrote:
> Please make sure that we assign default values to scalar members, right now
> they'll be initialized with garbage.
> 
> (Either in the constructor's initialization list, or in-line in the class
> declaration.)

Done.

https://codereview.chromium.org/1701313002/diff/1/content/public/common/conte...
content/public/common/content_push_subscription_options.h:26: bool
using_public_key;
On 2016/02/17 16:15:35, Peter Beverloo wrote:
> Why is it significant for the browser process to know this? This also relates
to
> the branch you have in push_messaging_message_filter.cc:280 - why would we
store
> them differently?
> 
> Theoretically a Web developer can pass the applicationServerKey in the
> gcm_sender_id field of the manifest (which is of type string).
> 
> From the GCM point of view, we give it an opaque identifier. The //chrome
layer
> and the GCM Driver don't care whether this is a sender Id or a public key,
it's
> the server that verifies this.
> 
> This would change how we validate the input in the browser process a bit.
Right
> now we assume that whatever we get is valid and use it, but we can tighten
that
> up a little bit. The two options are:
> 
>   (1) A sender id, e.g. parseable as a number.
>   (2) An application server key, which must be an uncompressed P-256 EC point
of
> 65 bytes.

I was thinking that we would do verification also in the renderer process (I've
got a TODO in the code to add that check), but given that we don't trust that,
maybe a single check in the browser process is the better place. However, I do
think that it's useful to have this flag in the renderer process, because we can
skip loading the manifest entirely if the developer gives us an
applicationServerKey.

https://codereview.chromium.org/1701313002/diff/1/content/public/common/conte...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1701313002/diff/1/content/public/common/conte...
content/public/common/content_switches.cc:474: const char
kEnableSubscriptionRestrictions[] =
On 2016/02/17 16:15:36, Peter Beverloo wrote:
> Right now, we're hiding the feature behind *two* command line flags:
> 
> --enable-experimental-web-platform-features (for everything except...)
> --enable-subscription-restrictions (using a different key name for storing
> user-data in the Service Worker database)
> 
> Let's drop the latter?

Done.

https://codereview.chromium.org/1701313002/diff/1/content/renderer/push_messa...
File content/renderer/push_messaging/push_messaging_dispatcher.cc (right):

https://codereview.chromium.org/1701313002/diff/1/content/renderer/push_messa...
content/renderer/push_messaging/push_messaging_dispatcher.cc:75:
switches::kEnableExperimentalWebPlatformFeatures) ||
On 2016/02/17 16:15:36, Peter Beverloo wrote:
> No need to check for the command line flag— we can rely on Blink passing us an
> empty applicationServerKey if the feature is disabled.
> 
> (When it does give us that value, we either have a bug in the bindings
> generator, or the renderer has been compromised. I don't think we have to be
> overly defensive for the first case, and in the latter case all bets are off
> anyway.)

Done.

https://codereview.chromium.org/1701313002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/1701313002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/push_messaging/PushManager.cpp:42:
webOptions.applicationServerKey = WTF::String(static_cast<const
char*>(options.appServicePublicKey()->data()));
On 2016/02/17 16:15:36, Peter Beverloo wrote:
> This isn't safe because the |applicationServerKey| may contain NULL bytes,
which
> would early-terminate the string. We need to use the length (byteLength())
> explicitly.
> 
> I think we have two options here—
> 
>   (1) Store and pass the key as a WebVector<uint8_t>.
>   (2) Creating a UTF16 string out of the binary data so that we can convert it
> back to a UTF8 string (i.e. std::string) later on.
> 
> 
> webOptions.applicationServerKey = WebString::fromUTF8(
>     static_cast<const char*>(options.applicationServerKey()->data(),
>                              options.applicationServerKey()->byteLength()));

Done.

https://codereview.chromium.org/1701313002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/push_messaging/PushSubscriptionOptions.idl
(right):

https://codereview.chromium.org/1701313002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/push_messaging/PushSubscriptionOptions.idl:9:
ArrayBuffer appServicePublicKey;
On 2016/02/17 16:15:36, Peter Beverloo wrote:
> applicationServerKey

Done.

https://codereview.chromium.org/1701313002/diff/1/third_party/WebKit/public/p...
File
third_party/WebKit/public/platform/modules/push_messaging/WebPushSubscriptionOptions.h
(right):

https://codereview.chromium.org/1701313002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/modules/push_messaging/WebPushSubscriptionOptions.h:23:
// subscription.
On 2016/02/17 16:15:36, Peter Beverloo wrote:
> Since it expects a specific format (uncompressed EC points), it would be good
to
> detail this in the comment:
> 
>   // P-256 public key, in uncompressed form, of the app server that can send
>   // push messages to this subscription.

Done.

[Peter Beverloo, 2016-02-18 11:39:43]

https://codereview.chromium.org/1701313002/diff/1/content/public/common/conte...
File content/public/common/content_push_subscription_options.h (right):

https://codereview.chromium.org/1701313002/diff/1/content/public/common/conte...
content/public/common/content_push_subscription_options.h:26: bool
using_public_key;
On 2016/02/18 10:45:14, harkness wrote:
> On 2016/02/17 16:15:35, Peter Beverloo wrote:
> > Why is it significant for the browser process to know this? This also
relates
> to
> > the branch you have in push_messaging_message_filter.cc:280 - why would we
> store
> > them differently?
> > 
> > Theoretically a Web developer can pass the applicationServerKey in the
> > gcm_sender_id field of the manifest (which is of type string).
> > 
> > From the GCM point of view, we give it an opaque identifier. The //chrome
> layer
> > and the GCM Driver don't care whether this is a sender Id or a public key,
> it's
> > the server that verifies this.
> > 
> > This would change how we validate the input in the browser process a bit.
> Right
> > now we assume that whatever we get is valid and use it, but we can tighten
> that
> > up a little bit. The two options are:
> > 
> >   (1) A sender id, e.g. parseable as a number.
> >   (2) An application server key, which must be an uncompressed P-256 EC
point
> of
> > 65 bytes.
> 
> I was thinking that we would do verification also in the renderer process
(I've
> got a TODO in the code to add that check), but given that we don't trust that,
> maybe a single check in the browser process is the better place. However, I do
> think that it's useful to have this flag in the renderer process, because we
can
> skip loading the manifest entirely if the developer gives us an
> applicationServerKey. 

Yes, but why is this flag significant for that? The following implication will
exist anywhere in our renderer code:

|using_public_key == !options.applicationServerKey.isEmpty()|

If the value doesn't have to leave the renderer, I don't think we have to
include it in this file.

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

https://codereview.chromium.org/1701313002/diff/40001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:282: :
kPushSenderIdServiceWorkerKey;
Why would we store the values differently?

Theoretically a Web developer can already pass a applicationServerKey in the
gcm_sender_id field of the manifest (which is of type string). I don't know why
that wouldn't have to work?

https://codereview.chromium.org/1701313002/diff/40001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:309: // This
sender_info in options will be ignored; instead it will be fetched
This is not correct anymore, if |options.sender_info| is set the developer
subscribed with an explicit applicationServerKey, and we should be using that.

This raises an interesting question that I asked on the GitHub issue as well:
what if it's different? :-)

https://codereview.chromium.org/1701313002/diff/40001/content/child/push_mess...
File content/child/push_messaging/push_provider.cc (right):

https://codereview.chromium.org/1701313002/diff/40001/content/child/push_mess...
content/child/push_messaging/push_provider.cc:82: content_options.sender_info =
options.applicationServerKey.utf8();
This would be a bug already: the |sender_info| is set to the
applicationServerKey and |using_public_key| would be set to FALSE (the default
value).

https://codereview.chromium.org/1701313002/diff/40001/content/renderer/push_m...
File content/renderer/push_messaging/push_messaging_dispatcher.cc (right):

https://codereview.chromium.org/1701313002/diff/40001/content/renderer/push_m...
content/renderer/push_messaging/push_messaging_dispatcher.cc:7: #include
"base/command_line.h"
nit: unused

https://codereview.chromium.org/1701313002/diff/40001/content/renderer/push_m...
content/renderer/push_messaging/push_messaging_dispatcher.cc:11: #include
"content/public/common/content_switches.h"
nit: unused

https://codereview.chromium.org/1701313002/diff/40001/content/renderer/push_m...
content/renderer/push_messaging/push_messaging_dispatcher.cc:50: // and the
client provided a public key, don't load the manifest.
Perhaps phrase this like "If the developer provided an application server key in
|options|, skip fetching the manifest"?

That availability of the data is toggled based on a command line flag doesn't
really matter here.

https://codereview.chromium.org/1701313002/diff/40001/content/renderer/push_m...
content/renderer/push_messaging/push_messaging_dispatcher.cc:95: //
TODO(harkness) Need to check that appServicePublicKey has a valid
nit: applicationServerKey

Would you want to do the check here or in Blink?

https://codereview.chromium.org/1701313002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/push_messaging/PushSubscriptionOptions.idl
(right):

https://codereview.chromium.org/1701313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushSubscriptionOptions.idl:9:
ArrayBuffer applicationServerKey;
Per the spec, this has to be of type BufferSource, which is an ArrayBuffer *or*
an ArrayBufferView.

We have the typedef in CommonDefinitions.idl already, so you should be able to
use it directly. You could extract the IDL value -> (Web)String conversion to a
helper method that abstracts this away from the main flow.

String toString(const ArrayBufferOrArrayBufferView& applicationServerKey) {
  if (applicationServerKey.isArrayBuffer()) { ... }
  else if (applicationServerKey.isArrayBufferView()) { ... }
  else { Oops!(); }
}

https://codereview.chromium.org/1701313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushSubscriptionOptions.idl:9:
ArrayBuffer applicationServerKey;
As discussed, we'll want to hide this behind a feature flag annotation until the
feature can be shipped, e.g. [RuntimeEnabled=PushSubscriptionRestrictions],
whereas PushSubscriptionRestrictions can be enabled with the Enable Experimental
Web Platform Features command-line flag.

This gives us four possible values for the `applicationServerKey` property in
C++:

  (1) Feature flag disabled, developer does not provide `applicationServerKey`:
NULL.
  (2) Feature flag disabled, developer provides `applicationServerKey`: NULL.
  (3) Feature flag enabled, developer does not provide `applicationServerKey`:
NULL.
  (4) Feature flag enabled, developer provides `applicationServerKey`:
BufferSource value.

[harkness, 2016-02-22 15:40:43]

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

https://codereview.chromium.org/1701313002/diff/40001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:282: :
kPushSenderIdServiceWorkerKey;
On 2016/02/18 11:39:43, Peter Beverloo wrote:
> Why would we store the values differently?
> 
> Theoretically a Web developer can already pass a applicationServerKey in the
> gcm_sender_id field of the manifest (which is of type string). I don't know
why
> that wouldn't have to work?

Done.

https://codereview.chromium.org/1701313002/diff/40001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:309: // This
sender_info in options will be ignored; instead it will be fetched
On 2016/02/18 11:39:43, Peter Beverloo wrote:
> This is not correct anymore, if |options.sender_info| is set the developer
> subscribed with an explicit applicationServerKey, and we should be using that.
> 
> This raises an interesting question that I asked on the GitHub issue as well:
> what if it's different? :-)

I updated the comment to be correct. My assumption in the implementation is that
I will only look in the registration if I don't already have a sender_info.
However, that means that if there's a bad sender_info in the registration, I'm
not updating it. Essentially, that would mean that the only way to get rid of an
outdated registration would be to unregister and re-register with the new valid
ID. The only drawback we could do would be to overwrite any time there is a
register, but that's a lot of extra IO work for us to do if we do it on every
registration.

https://codereview.chromium.org/1701313002/diff/40001/content/child/push_mess...
File content/child/push_messaging/push_provider.cc (right):

https://codereview.chromium.org/1701313002/diff/40001/content/child/push_mess...
content/child/push_messaging/push_provider.cc:82: content_options.sender_info =
options.applicationServerKey.utf8();
On 2016/02/18 11:39:43, Peter Beverloo wrote:
> This would be a bug already: the |sender_info| is set to the
> applicationServerKey and |using_public_key| would be set to FALSE (the default
> value).

I've removed the flag on the SubscriptionOptions, so essentially the code is a
lot dumber now, and just passes everything through, only checking validity at
the edges.

https://codereview.chromium.org/1701313002/diff/40001/content/renderer/push_m...
File content/renderer/push_messaging/push_messaging_dispatcher.cc (right):

https://codereview.chromium.org/1701313002/diff/40001/content/renderer/push_m...
content/renderer/push_messaging/push_messaging_dispatcher.cc:7: #include
"base/command_line.h"
On 2016/02/18 11:39:43, Peter Beverloo wrote:
> nit: unused

Done.

https://codereview.chromium.org/1701313002/diff/40001/content/renderer/push_m...
content/renderer/push_messaging/push_messaging_dispatcher.cc:11: #include
"content/public/common/content_switches.h"
On 2016/02/18 11:39:43, Peter Beverloo wrote:
> nit: unused

Done.

https://codereview.chromium.org/1701313002/diff/40001/content/renderer/push_m...
content/renderer/push_messaging/push_messaging_dispatcher.cc:50: // and the
client provided a public key, don't load the manifest.
On 2016/02/18 11:39:43, Peter Beverloo wrote:
> Perhaps phrase this like "If the developer provided an application server key
in
> |options|, skip fetching the manifest"?
> 
> That availability of the data is toggled based on a command line flag doesn't
> really matter here.

Updated the comment, and I went ahead and refactored this to only load the
manifest if the server key wasn't provided.

https://codereview.chromium.org/1701313002/diff/40001/content/renderer/push_m...
content/renderer/push_messaging/push_messaging_dispatcher.cc:95: //
TODO(harkness) Need to check that appServicePublicKey has a valid
On 2016/02/18 11:39:43, Peter Beverloo wrote:
> nit: applicationServerKey
> 
> Would you want to do the check here or in Blink?

Done.

https://codereview.chromium.org/1701313002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/push_messaging/PushSubscriptionOptions.idl
(right):

https://codereview.chromium.org/1701313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushSubscriptionOptions.idl:9:
ArrayBuffer applicationServerKey;
On 2016/02/18 11:39:43, Peter Beverloo wrote:
> Per the spec, this has to be of type BufferSource, which is an ArrayBuffer
*or*
> an ArrayBufferView.
> 
> We have the typedef in CommonDefinitions.idl already, so you should be able to
> use it directly. You could extract the IDL value -> (Web)String conversion to
a
> helper method that abstracts this away from the main flow.
> 
> String toString(const ArrayBufferOrArrayBufferView& applicationServerKey) {
>   if (applicationServerKey.isArrayBuffer()) { ... }
>   else if (applicationServerKey.isArrayBufferView()) { ... }
>   else { Oops!(); }
> }

Should be good now?

[Peter Beverloo, 2016-02-23 14:28:29]

Thanks!

Browser side mostly lg, there are a few cleanups that we can do (e.g. remove
RegisterData.user_visible, pass the new struct to Core::RegisterUI), but I'm OK
doing those later.

We should address the Web exposed side of this CL before landing it, however.
Would you mind taking a look?

https://codereview.chromium.org/1701313002/diff/60001/chrome/test/data/push_m...
File chrome/test/data/push_messaging/push_test.js (right):

https://codereview.chromium.org/1701313002/diff/60001/chrome/test/data/push_m...
chrome/test/data/push_messaging/push_test.js:17: // this value will need to be
updated.
Which is now? :)

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

https://codereview.chromium.org/1701313002/diff/60001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:303:
CheckForExistingRegistration(data, PushSubscriptionOptions());
Your comment doesn't match your code :-). s/PushSubscriptionOptions()/options/?

https://codereview.chromium.org/1701313002/diff/60001/content/renderer/push_m...
File content/renderer/push_messaging/push_messaging_dispatcher.cc (right):

https://codereview.chromium.org/1701313002/diff/60001/content/renderer/push_m...
content/renderer/push_messaging/push_messaging_dispatcher.cc:55: } else {
nit: Chrome prefers early returns, so you could rewrite this as:

===============
  if (options.applicationServerKey.isEmpty()) {
    ...
    return;
  }

  ...
===============

https://codereview.chromium.org/1701313002/diff/60001/content/renderer/push_m...
content/renderer/push_messaging/push_messaging_dispatcher.cc:79:
content_options.sender_info =
nit: Since this has got a default value of empty already, you could get rid of
the ternary by writing this as:

===============
  if (!manifest.gcm_sender_id.is_null()) {
    content_options.sender_info =
        base::UTF16ToUTF8(manifest.gcm_sender_id.string());
  }
===============

https://codereview.chromium.org/1701313002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/1701313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.cpp:48: // ID or a
65 byte unencrypted key.
Developers will have two ways in Chrome to provide their keys.

(1) The new standardized way (SubscriptionOptions.applicationServerKey). This
*must* be a valid point on the P-256 curve, i.e. a 65-byte sequence starting
with 0x04.

https://github.com/w3c/push-api/pull/182/files

Accepting sender Ids in this property as well would mean that we violate the
spec, and also limit our own ability to use this as a carrot for moving to the
Web Push protocol.

Put differently, we shouldn't accept a sender Id here.

(2) The existing, proprietary way (`gcm_sender_id` in the manifest). This is an
opaque string that we just forward to GCM.

We already don't care about this value, and I think there's no reason to start
caring now either.

What does matter is that we give (1) precedence over (2), and that's exactly
what this CL does in the PushMessagingMessageFilter :-).

Sorry that this is confusing.

https://codereview.chromium.org/1701313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.cpp:60: return
String();
ASSERT_NOT_REACHED();

(The ArrayBufferOrArrayBufferView type should ensure that we never reach this.)

https://codereview.chromium.org/1701313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.cpp:65: return
String();
Per the specification:

>> If the `applicationServerKey` value is invalid, reject `promise` with
>> an `InvalidAccessError` and terminate these steps.

Right now we silently fail. Instead, we should throw an InvalidAccessError. You
can do this by rejecting the promise like we do on line 81. (Rejected promises
effectively are thrown exceptions.)

This means that in we might have to separate the concern of (1)
ArrayBufferOrArrayBufferView to WebString, (2) validating that the
`applicationServerKey`, when set, is valid.

https://codereview.chromium.org/1701313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.cpp:72: if
(options.hasApplicationServerKey()) {
nit: no need for brackets on one-line statements.

https://codereview.chromium.org/1701313002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushManager.h (right):

https://codereview.chromium.org/1701313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.h:34: static String
toString(const ArrayBufferOrArrayBufferView& applicationServerKey);
Two thoughts:

  (1) We could avoid exposing this altogether if we were to test the
toWebPushSubscriptionOptions entry point instead. That would further allow us to
have basic test coverage of the `userVisibleOnly` boolean, and potentially other
future values.

  (2) If you would like to expose this, let's pick a clear name. Right now, this
reads as "PushManager::toString(..)", which causes ambiguity with
`pushManager.toString()` (which is completely valid JavaScript), while it does
something completely different.

https://codereview.chromium.org/1701313002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushManagerTest.cpp
(right):

https://codereview.chromium.org/1701313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManagerTest.cpp:44: uint8_t
senderKey[kValidKeyLength];
Perhaps rewrite it like this:

===============
uint8_t senderKey[kValidKeyLength];
memset(senderKey, 0, sizeof(senderKey));

senderKey[0] = 0x04;

// ....
=====

That allows you to avoid the loops and just rely on memset() instead.

https://codereview.chromium.org/1701313002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1701313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:184:
SubscriptionRestrictions status=experimental
nit: Perhaps prefix with Push to indicate grouping?

[harkness, 2016-02-26 11:44:27]

https://codereview.chromium.org/1701313002/diff/60001/content/renderer/push_m...
File content/renderer/push_messaging/push_messaging_dispatcher.cc (right):

https://codereview.chromium.org/1701313002/diff/60001/content/renderer/push_m...
content/renderer/push_messaging/push_messaging_dispatcher.cc:55: } else {
On 2016/02/23 14:28:29, Peter Beverloo wrote:
> nit: Chrome prefers early returns, so you could rewrite this as:
> 
> ===============
>   if (options.applicationServerKey.isEmpty()) {
>     ...
>     return;
>   }
> 
>   ...
> ===============

Actually, in this case, I feel like the different format makes the code less
readable. Do you feel strongly about the change?

https://codereview.chromium.org/1701313002/diff/60001/content/renderer/push_m...
content/renderer/push_messaging/push_messaging_dispatcher.cc:79:
content_options.sender_info =
On 2016/02/23 14:28:29, Peter Beverloo wrote:
> nit: Since this has got a default value of empty already, you could get rid of
> the ternary by writing this as:
> 
> ===============
>   if (!manifest.gcm_sender_id.is_null()) {
>     content_options.sender_info =
>         base::UTF16ToUTF8(manifest.gcm_sender_id.string());
>   }
> ===============

Done.

https://codereview.chromium.org/1701313002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/1701313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.cpp:48: // ID or a
65 byte unencrypted key.
On 2016/02/23 14:28:29, Peter Beverloo wrote:
> Developers will have two ways in Chrome to provide their keys.
> 
> (1) The new standardized way (SubscriptionOptions.applicationServerKey). This
> *must* be a valid point on the P-256 curve, i.e. a 65-byte sequence starting
> with 0x04.
> 
> https://github.com/w3c/push-api/pull/182/files
> 
> Accepting sender Ids in this property as well would mean that we violate the
> spec, and also limit our own ability to use this as a carrot for moving to the
> Web Push protocol.
> 
> Put differently, we shouldn't accept a sender Id here.
> 
> (2) The existing, proprietary way (`gcm_sender_id` in the manifest). This is
an
> opaque string that we just forward to GCM.
> 
> We already don't care about this value, and I think there's no reason to start
> caring now either.
> 
> What does matter is that we give (1) precedence over (2), and that's exactly
> what this CL does in the PushMessagingMessageFilter :-).
> 
> Sorry that this is confusing.

Done.

https://codereview.chromium.org/1701313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.cpp:60: return
String();
On 2016/02/23 14:28:29, Peter Beverloo wrote:
> ASSERT_NOT_REACHED();
> 
> (The ArrayBufferOrArrayBufferView type should ensure that we never reach
this.)

Done.

https://codereview.chromium.org/1701313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.cpp:65: return
String();
On 2016/02/23 14:28:29, Peter Beverloo wrote:
> Per the specification:
> 
> >> If the `applicationServerKey` value is invalid, reject `promise` with
> >> an `InvalidAccessError` and terminate these steps.
> 
> Right now we silently fail. Instead, we should throw an InvalidAccessError.
You
> can do this by rejecting the promise like we do on line 81. (Rejected promises
> effectively are thrown exceptions.)
> 
> This means that in we might have to separate the concern of (1)
> ArrayBufferOrArrayBufferView to WebString, (2) validating that the
> `applicationServerKey`, when set, is valid.

Done.

https://codereview.chromium.org/1701313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.cpp:72: if
(options.hasApplicationServerKey()) {
On 2016/02/23 14:28:29, Peter Beverloo wrote:
> nit: no need for brackets on one-line statements.

Done.

https://codereview.chromium.org/1701313002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushManagerTest.cpp
(right):

https://codereview.chromium.org/1701313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManagerTest.cpp:44: uint8_t
senderKey[kValidKeyLength];
On 2016/02/23 14:28:29, Peter Beverloo wrote:
> Perhaps rewrite it like this:
> 
> ===============
> uint8_t senderKey[kValidKeyLength];
> memset(senderKey, 0, sizeof(senderKey));
> 
> senderKey[0] = 0x04;
> 
> // ....
> =====
> 
> That allows you to avoid the loops and just rely on memset() instead.

Done.

https://codereview.chromium.org/1701313002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1701313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:184:
SubscriptionRestrictions status=experimental
On 2016/02/23 14:28:29, Peter Beverloo wrote:
> nit: Perhaps prefix with Push to indicate grouping?

Done.

[Peter Beverloo, 2016-02-26 16:02:34]

code lgtm % nits and the bug in push_messaging_message_filter.cc.

https://codereview.chromium.org/1701313002/diff/60001/content/renderer/push_m...
File content/renderer/push_messaging/push_messaging_dispatcher.cc (right):

https://codereview.chromium.org/1701313002/diff/60001/content/renderer/push_m...
content/renderer/push_messaging/push_messaging_dispatcher.cc:55: } else {
On 2016/02/26 11:44:26, harkness wrote:
> On 2016/02/23 14:28:29, Peter Beverloo wrote:
> > nit: Chrome prefers early returns, so you could rewrite this as:
> > 
> > ===============
> >   if (options.applicationServerKey.isEmpty()) {
> >     ...
> >     return;
> >   }
> > 
> >   ...
> > ===============
> 
> Actually, in this case, I feel like the different format makes the code less
> readable. Do you feel strongly about the change?

No :-)

https://codereview.chromium.org/1701313002/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/1701313002/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:103:
command_line->AppendSwitch(
Please document in a comment for what feature this is needed.

https://codereview.chromium.org/1701313002/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:104:
switches::kEnableExperimentalWebPlatformFeatures);
nit:

InProcessBrowserTest::SetUpCommandLine(command_line);

https://codereview.chromium.org/1701313002/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.h (right):

https://codereview.chromium.org/1701313002/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.h:29: #include
"content/public/common/push_subscription_options.h"
micro nit: could forward declare this

https://codereview.chromium.org/1701313002/diff/80001/chrome/test/data/push_m...
File chrome/test/data/push_messaging/push_test.js (right):

https://codereview.chromium.org/1701313002/diff/80001/chrome/test/data/push_m...
chrome/test/data/push_messaging/push_test.js:10: // NIST P-256 public key made
available to layout tests. Must be an uncompressed
Please update the comment (this isn't a layout test).

https://codereview.chromium.org/1701313002/diff/80001/chrome/test/data/push_m...
chrome/test/data/push_messaging/push_test.js:145: applicationServerKey[0] =
0x05;
nit: I'd prefer to make a copy, because right now this would cause the second
call to subscribePush to fail (potential pit-fall for future tests).

subscribePush();
subscribePushBadKey();
subscribePush();

Proposal:

var invalidApplicationServerKey = Uint8Array.from(applicationServerKey);
invalidApplicationServerKey[0] = 0x05;

https://codereview.chromium.org/1701313002/diff/80001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/1701313002/diff/80001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:303:
CheckForExistingRegistration(data, PushSubscriptionOptions());
You seem to have missed my previous comment about this line:

> Your comment doesn't match your code :-).
> s/PushSubscriptionOptions()/options/?

https://codereview.chromium.org/1701313002/diff/80001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_push_messaging_service.cc
(right):

https://codereview.chromium.org/1701313002/diff/80001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_push_messaging_service.cc:12:
#include "content/public/common/push_subscription_options.h"
nit: double include

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.cpp:46:
ExceptionState& exceptionState)
nit: align this argument with the first one

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.cpp:68: "The
provided applicationServerKey is not valid.");
micro nit: newline not necessary

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.cpp:73: const
PushSubscriptionOptions& options, ExceptionState& exceptionState)
nit: arguments on different lines, or don't line break at all.

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushManager.h (right):

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.h:29:
ExceptionState&);
nit: forward declare

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.h:40: static String
toString(const ArrayBufferOrArrayBufferView& applicationServerKey,
nit 1: this should be internal to PushManager.cpp.
nit 2: please name this something different from toString()

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushManagerTest.cpp
(right):

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManagerTest.cpp:17: const
char kValidKeyMarker = 0x04;
Please do not indent code in namespaces.

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManagerTest.cpp:23:
std::memset(senderKey, 0, sizeof(senderKey));
nit: no std::

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManagerTest.cpp:33:
EXPECT_EQ(0, std::memcmp(output.applicationServerKey.utf8().c_str(), senderKey,
kValidKeyLength));
ASSERT_EQ on the length to make sure we don't access invalid memory?

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManagerTest.cpp:33:
EXPECT_EQ(0, std::memcmp(output.applicationServerKey.utf8().c_str(), senderKey,
kValidKeyLength));
STL is not yet allowed in these parts of Blink, unfortunately. Just compare
|output.applicationServerKey| against WebString::fromUTF8()?

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManagerTest.cpp:68: } //
anonymous namespace
micro nit: we usually just use "// namespace"

[harkness, 2016-02-26 17:12:05]

https://codereview.chromium.org/1701313002/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/1701313002/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:103:
command_line->AppendSwitch(
On 2016/02/26 16:02:33, Peter Beverloo wrote:
> Please document in a comment for what feature this is needed.

Done.

https://codereview.chromium.org/1701313002/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:104:
switches::kEnableExperimentalWebPlatformFeatures);
On 2016/02/26 16:02:32, Peter Beverloo wrote:
> nit:
> 
> InProcessBrowserTest::SetUpCommandLine(command_line);

Done.

https://codereview.chromium.org/1701313002/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.h (right):

https://codereview.chromium.org/1701313002/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.h:29: #include
"content/public/common/push_subscription_options.h"
On 2016/02/26 16:02:33, Peter Beverloo wrote:
> micro nit: could forward declare this

Done.

https://codereview.chromium.org/1701313002/diff/80001/chrome/test/data/push_m...
File chrome/test/data/push_messaging/push_test.js (right):

https://codereview.chromium.org/1701313002/diff/80001/chrome/test/data/push_m...
chrome/test/data/push_messaging/push_test.js:10: // NIST P-256 public key made
available to layout tests. Must be an uncompressed
On 2016/02/26 16:02:33, Peter Beverloo wrote:
> Please update the comment (this isn't a layout test).

Done.

https://codereview.chromium.org/1701313002/diff/80001/chrome/test/data/push_m...
chrome/test/data/push_messaging/push_test.js:145: applicationServerKey[0] =
0x05;
On 2016/02/26 16:02:33, Peter Beverloo wrote:
> nit: I'd prefer to make a copy, because right now this would cause the second
> call to subscribePush to fail (potential pit-fall for future tests).
> 
> subscribePush();
> subscribePushBadKey();
> subscribePush();
> 
> Proposal:
> 
> var invalidApplicationServerKey = Uint8Array.from(applicationServerKey);
> invalidApplicationServerKey[0] = 0x05;

Done.

https://codereview.chromium.org/1701313002/diff/80001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/1701313002/diff/80001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:303:
CheckForExistingRegistration(data, PushSubscriptionOptions());
On 2016/02/26 16:02:33, Peter Beverloo wrote:
> You seem to have missed my previous comment about this line:
> 
> > Your comment doesn't match your code :-).
> > s/PushSubscriptionOptions()/options/?

Done.

https://codereview.chromium.org/1701313002/diff/80001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_push_messaging_service.cc
(right):

https://codereview.chromium.org/1701313002/diff/80001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_push_messaging_service.cc:12:
#include "content/public/common/push_subscription_options.h"
On 2016/02/26 16:02:33, Peter Beverloo wrote:
> nit: double include

Done.

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.cpp:46:
ExceptionState& exceptionState)
On 2016/02/26 16:02:33, Peter Beverloo wrote:
> nit: align this argument with the first one

Done.

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.cpp:68: "The
provided applicationServerKey is not valid.");
On 2016/02/26 16:02:33, Peter Beverloo wrote:
> micro nit: newline not necessary

Done.

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.cpp:73: const
PushSubscriptionOptions& options, ExceptionState& exceptionState)
On 2016/02/26 16:02:33, Peter Beverloo wrote:
> nit: arguments on different lines, or don't line break at all.

Done.

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushManager.h (right):

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.h:29:
ExceptionState&);
On 2016/02/26 16:02:33, Peter Beverloo wrote:
> nit: forward declare

Done.

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.h:40: static String
toString(const ArrayBufferOrArrayBufferView& applicationServerKey,
On 2016/02/26 16:02:33, Peter Beverloo wrote:
> nit 1: this should be internal to PushManager.cpp.
> nit 2: please name this something different from toString()

Done.

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushManagerTest.cpp
(right):

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManagerTest.cpp:17: const
char kValidKeyMarker = 0x04;
On 2016/02/26 16:02:33, Peter Beverloo wrote:
> Please do not indent code in namespaces.

gah, evil git cl format!

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManagerTest.cpp:23:
std::memset(senderKey, 0, sizeof(senderKey));
On 2016/02/26 16:02:33, Peter Beverloo wrote:
> nit: no std::

Done.

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManagerTest.cpp:33:
EXPECT_EQ(0, std::memcmp(output.applicationServerKey.utf8().c_str(), senderKey,
kValidKeyLength));
On 2016/02/26 16:02:33, Peter Beverloo wrote:
> ASSERT_EQ on the length to make sure we don't access invalid memory?

Done.

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManagerTest.cpp:33:
EXPECT_EQ(0, std::memcmp(output.applicationServerKey.utf8().c_str(), senderKey,
kValidKeyLength));
On 2016/02/26 16:02:33, Peter Beverloo wrote:
> STL is not yet allowed in these parts of Blink, unfortunately. Just compare
> |output.applicationServerKey| against WebString::fromUTF8()?

Done.

https://codereview.chromium.org/1701313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManagerTest.cpp:68: } //
anonymous namespace
On 2016/02/26 16:02:33, Peter Beverloo wrote:
> micro nit: we usually just use "// namespace"

Done.

[harkness, 2016-02-26 17:16:00]

harkness@chromium.org changed reviewers:
+ avi@chromium.org, mkwst@chromium.org

[harkness, 2016-02-26 17:16:01]

avi@chromium.org: Please review changes in
content/public/common/push_subscription_options.h

mkwst@chromium.org: Please review changes in
content/common/push_messaging_messages.h

[Mike West, 2016-03-01 19:44:12]

Moving the IPC's existing bool and string into a struct LGTM.

[Avi (use Gerrit), 2016-03-01 20:02:59]

lgtm

works for me

https://codereview.chromium.org/1701313002/diff/100001/chrome/browser/push_me...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/1701313002/diff/100001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:103: // Enable
experiemntal features for subscription restrictions.
typo: experimental

[Peter Beverloo, 2016-03-02 18:07:58]

The CQ bit was checked by peter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-02 18:08:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1701313002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1701313002/100001

[commit-bot: I haz the power, 2016-03-02 18:30:33]

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

[commit-bot: I haz the power, 2016-03-02 18:30:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[harkness, 2016-03-03 15:25:04]

The CQ bit was checked by harkness@chromium.org

[harkness, 2016-03-03 15:25:05]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
avi@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1701313002/#ps120001
(title: "Fixed missing include")

[commit-bot: I haz the power, 2016-03-03 15:25:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1701313002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1701313002/120001

[commit-bot: I haz the power, 2016-03-03 15:54:27]

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

[commit-bot: I haz the power, 2016-03-03 15:54:29]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[harkness, 2016-03-03 17:08:25]

The CQ bit was checked by harkness@chromium.org

[harkness, 2016-03-03 17:08:26]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
avi@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1701313002/#ps140001
(title: "Fix module export build issue")

[commit-bot: I haz the power, 2016-03-03 17:08:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1701313002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1701313002/140001

[commit-bot: I haz the power, 2016-03-03 18:45:30]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-03-03 18:47:33]

Description was changed from

==========
Partial implementation of subscription restrictions.

Currently, chrome requires that app developers provide a "sender_id" tag in the
manifest. This id is generated by the developer console and is not standard
for all browsers.

In the future, the app developer will be able to specify a public key for their
service, which will be registered with the push service and which the push
service can use to validate app services requesting to send messages.

BUG=583753
==========

to

==========
Partial implementation of subscription restrictions.

Currently, chrome requires that app developers provide a "sender_id" tag in the
manifest. This id is generated by the developer console and is not standard
for all browsers.

In the future, the app developer will be able to specify a public key for their
service, which will be registered with the push service and which the push
service can use to validate app services requesting to send messages.

BUG=583753

Committed: https://crrev.com/71f5a08bb30153b56e8a3f9b447264a54b6d9c12
Cr-Commit-Position: refs/heads/master@{#379045}
==========

[commit-bot: I haz the power, 2016-03-03 18:47:34]

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

[benwells, 2016-03-07 23:46:18]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1770763003/ by benwells@chromium.org.

The reason for reverting is: Sorry about the delay in reverting ...

This is causing many memory errors on DrMemory.

I think there is an error in the patch, in push_messaging_dispatcher.cc. The
same WebPush...Callbacks* can be added twice to |subscription_callbacks_|, in
the new DidGetManifest and in the already existing DoSubscribe.

As |subscription_callbacks_| is declared with IDMapOwnPointer, this will cause
callbacks to be freed twice.

First build where it failed with example errors:

https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Browser%2...

Sample output:

UNADDRESSABLE ACCESS beyond heap bounds: writing 0x09880330-0x09880334 4 byte(s)
# 0 modules.dll!blink::WebCallbacks<>::`scalar deleting destructor'
# 1 content.dll!IDMap<>::Releaser<>::release_all                              
[base\id_map.h:250]
# 2 content.dll!content::PushMessagingDispatcher::~PushMessagingDispatcher    
[content\renderer\push_messaging\push_messaging_dispatcher.cc:27]
# 3 content.dll!content::PushMessagingDispatcher::`scalar deleting destructor'
# 4 content.dll!content::RenderFrameObserver::OnDestruct                      
[content\public\renderer\render_frame_observer.cc:33]
# 5 content.dll!content::RenderFrameImpl::`vector deleting destructor'
# 6 content.dll!content::RenderFrameImpl::frameDetached                       
[content\renderer\render_frame_impl.cc:2740]
# 7 blink_web.dll!blink::FrameLoaderClientImpl::detached                      
[third_party\webkit\source\web\frameloaderclientimpl.cpp:383]
# 8 webcore_shared.dll!blink::Frame::detach                                   
[third_party\webkit\source\core\frame\frame.cpp:98]
# 9 webcore_shared.dll!blink::LocalFrame::detach                              
[third_party\webkit\source\core\frame\localframe.cpp:357]
#10 webcore_shared.dll!blink::Page::willBeDestroyed                           
[third_party\webkit\source\core\page\page.cpp:573]
#11 base.dll!base::debug::TaskAnnotator::RunTask                              
[base\debug\task_annotator.cc:51]
#12 scheduler.dll!scheduler::TaskQueueManager::ProcessTaskFromWorkQueue       
[components\scheduler\base\task_queue_manager.cc:288]
#13 scheduler.dll!scheduler::TaskQueueManager::DoWork                         
[components\scheduler\base\task_queue_manager.cc:200]
#14 scheduler.dll!base::internal::Invoker<>::Run                              
[base\bind_internal.h:354]
#15 base.dll!base::debug::TaskAnnotator::RunTask                              
[base\debug\task_annotator.cc:51]
#16 base.dll!base::MessageLoop::RunTask                                       
[base\message_loop\message_loop.cc:476]
#17 base.dll!base::MessageLoop::DeferOrRunPendingTask                         
[base\message_loop\message_loop.cc:485]
#18 base.dll!base::MessageLoop::DoWork                                        
[base\message_loop\message_loop.cc:597]
#19 base.dll!base::MessagePumpDefault::Run                                    
[base\message_loop\message_pump_default.cc:33]
#20 base.dll!base::MessageLoop::RunHandler                                    
[base\message_loop\message_loop.cc:440]
#21 base.dll!base::RunLoop::Run                                               
[base\run_loop.cc:35]
#22 base.dll!base::MessageLoop::Run                                           
[base\message_loop\message_loop.cc:293]
#23 content.dll!content::RendererMain                                         
[content\renderer\renderer_main.cc:219]
#24 content.dll!content::RunNamedProcessTypeMain                              
[content\app\content_main_runner.cc:395]
#25 content.dll!content::ContentMainRunnerImpl::Run                           
[content\app\content_main_runner.cc:766]
#26 content.dll!content::ContentMain                                          
[content\app\content_main.cc:19]
#27 content::LaunchTests                                                      
[content\public\test\test_launcher.cc:505]
#28 LaunchChromeTests                                                         
[chrome\test\base\chrome_test_launcher.cc:128]
#29 main                                                                      
[chrome\test\base\browser_tests_main.cc:21]
Note: @0:03:41.960 in thread 1944
Note: next higher malloc: 0x098804a0-0x098804c0
Note: prev lower malloc:  0x09880248-0x09880310
Note: instruction: mov    $0x679f7ef4 -> (%esi)
The report came from the
`PushMessagingBrowserTest.GrantAlreadyGrantedPermissionDoesNotUnsubscribe` test.
.