Implement and ship PushManager.supportedContentEncodings

Implement and ship PushManager.supportedContentEncodings

This static property can be used by developers to check which
Content-Encoding values are supported by the implementation for the
encrypted message payloads that a push message can carry.

Right now we only support "aesgcm", which corresponds to
draft-ietf-webpush-encryption-05. Support for the latest version of the
encryption scheme, identified by "aes128gcm" and required by the
specification, is tracked by crbug.com/679789.

This CL also exposes the PushManager, PushSubscription,
PushSubscriptionOptions and extensions to the ServiceWorkerRegistration
interfaces in all workers, matching availability of SWRegistration.

Specification:
https://w3c.github.io/push-api/#dom-pushmanager-supportedcontentencodings

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/jNg1Zth2XrI/discussion

BUG=718020
Review-Url: https://codereview.chromium.org/2863443002
Cr-Commit-Position: refs/heads/master@{#472146}
Committed: https://chromium.googlesource.com/chromium/src/+/60b0f2c0bbf2fbb96aa09a781f5adac7981832df

[Peter Beverloo, 2017-05-03 16:26:00]

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

[commit-bot: I haz the power, 2017-05-03 16:26:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 18:28:44]

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

[commit-bot: I haz the power, 2017-05-03 18:28:45]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Peter Beverloo, 2017-05-05 13:05:36]

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

[commit-bot: I haz the power, 2017-05-05 13:05:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Beverloo, 2017-05-05 13:14:43]

Description was changed from

==========
Implement and ship PushManager.supportedContentEncodings

This static property can be used by developers to check which
Content-Encoding values are supported by the implementation for the
encrypted message payloads that a push message can carry.

Right now we only support "aesgcm", which corresponds to
draft-ietf-webpush-encryption-05. Support for the latest version of the
encryption scheme, identified by "aes128gcm" and required by the
specification, is tracked by crbug.com/679789.

This CL also exposes the PushManager, PushSubscription,
PushSubscriptionOptions and extensions to the ServiceWorkerRegistration
interfaces in all workers, matching availability of SWRegistration.

Specification:
https://w3c.github.io/push-api/#dom-pushmanager-supportedcontentencodings

Intent to Implement and Ship:
PENDING

BUG=718020
==========

to

==========
Implement and ship PushManager.supportedContentEncodings

This static property can be used by developers to check which
Content-Encoding values are supported by the implementation for the
encrypted message payloads that a push message can carry.

Right now we only support "aesgcm", which corresponds to
draft-ietf-webpush-encryption-05. Support for the latest version of the
encryption scheme, identified by "aes128gcm" and required by the
specification, is tracked by crbug.com/679789.

This CL also exposes the PushManager, PushSubscription,
PushSubscriptionOptions and extensions to the ServiceWorkerRegistration
interfaces in all workers, matching availability of SWRegistration.

Specification:
https://w3c.github.io/push-api/#dom-pushmanager-supportedcontentencodings

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/jNg1Zth2XrI/discus...

BUG=718020
==========

[Peter Beverloo, 2017-05-05 13:18:58]

peter@chromium.org changed reviewers:
+ bashi@chromium.org, haraken@chromium.org, jochen@chromium.org,
johnme@chromium.org

[Peter Beverloo, 2017-05-05 13:18:59]

+johnme for push
+jochen for webexposed
+jochen/haraken/bashi for bindings change

https://codereview.chromium.org/2863443002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2863443002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:24:
v8::Local<v8::Object> holder = info.Holder();
SaveSameObject uses |holder| (see lines 27-43 below), but this now introduces a
use of [SaveSameObject] on a static attribute.

Looking at FunctionCallbackInfo<>::Holder(), Ross and I suspect that the holder
ends up being the prototype object as opposed to the instance object, which
should be fine in this case. However, neither of us is sure.

This does work as expected, but verifying this would be most appreciated!

[haraken, 2017-05-05 14:04:22]

haraken@chromium.org changed reviewers:
+ yukishiino@chromium.org

[haraken, 2017-05-05 14:04:23]

+yukishiino

I was thinking that [SaveSameObject] is just a performance optimization (i.e.,
we should not use it to guarantee correctness).

In this particular case, a right solution would be to change the C++
implementation of |supportedContentEncodings| so that it always returns the same
object (instead of relying on [SaveSameObject])... ?

[Peter Beverloo, 2017-05-05 14:23:53]

On 2017/05/05 14:04:23, haraken wrote:
> +yukishiino
> 
> I was thinking that [SaveSameObject] is just a performance optimization (i.e.,
> we should not use it to guarantee correctness).
> 
> In this particular case, a right solution would be to change the C++
> implementation of |supportedContentEncodings| so that it always returns the
same
> object (instead of relying on [SaveSameObject])... ?

We'd have to return a ScriptValue (since the Vector<String> return type would be
converted to a new v8::Value on each invocation), but where would we store it in
a way that its lifetime is correctly managed?

A new Supplementable on the ExecutionContext would be an option, solely for
holding this value, but that'd be a lot more complexity for (eventually) the
same result.

[Peter Beverloo, 2017-05-05 14:24:07]

On 2017/05/05 14:23:53, Peter Beverloo wrote:
> On 2017/05/05 14:04:23, haraken wrote:
> > +yukishiino
> > 
> > I was thinking that [SaveSameObject] is just a performance optimization
(i.e.,
> > we should not use it to guarantee correctness).
> > 
> > In this particular case, a right solution would be to change the C++
> > implementation of |supportedContentEncodings| so that it always returns the
> same
> > object (instead of relying on [SaveSameObject])... ?
> 
> We'd have to return a ScriptValue (since the Vector<String> return type would
be
> converted to a new v8::Value on each invocation), but where would we store it
in
> a way that its lifetime is correctly managed?
> 
> A new Supplementable on the ExecutionContext would be an option, solely for
> holding this value, but that'd be a lot more complexity for (eventually) the
> same result.

s/Supplementable/Supplement/

[haraken, 2017-05-05 14:39:58]

On 2017/05/05 14:24:07, Peter Beverloo wrote:
> On 2017/05/05 14:23:53, Peter Beverloo wrote:
> > On 2017/05/05 14:04:23, haraken wrote:
> > > +yukishiino
> > > 
> > > I was thinking that [SaveSameObject] is just a performance optimization
> (i.e.,
> > > we should not use it to guarantee correctness).
> > > 
> > > In this particular case, a right solution would be to change the C++
> > > implementation of |supportedContentEncodings| so that it always returns
the
> > same
> > > object (instead of relying on [SaveSameObject])... ?
> > 
> > We'd have to return a ScriptValue (since the Vector<String> return type
would
> be
> > converted to a new v8::Value on each invocation), but where would we store
it
> in
> > a way that its lifetime is correctly managed?
> > 
> > A new Supplementable on the ExecutionContext would be an option, solely for
> > holding this value, but that'd be a lot more complexity for (eventually) the
> > same result.
> 
> s/Supplementable/Supplement/

Yeah, you're right.

We should probably change the IDL compiler so that the generated code calls the
C++ implementation only at the first time and returns the cached result from the
second time. (i.e., make [SaveSameObject] the default behavior)

+yukishiino What is preventing us from making [SaveSameObject] the default
behavior?

[commit-bot: I haz the power, 2017-05-05 14:53:43]

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

[commit-bot: I haz the power, 2017-05-05 14:53:44]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Peter Beverloo, 2017-05-05 15:40:38]

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

[commit-bot: I haz the power, 2017-05-05 15:41:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 17:34:15]

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

[commit-bot: I haz the power, 2017-05-05 17:34:16]

Dry run: This issue passed the CQ dry run.

[Yuki, 2017-05-08 07:48:20]

On 2017/05/05 14:39:58, haraken wrote:
> On 2017/05/05 14:24:07, Peter Beverloo wrote:
> > On 2017/05/05 14:23:53, Peter Beverloo wrote:
> > > On 2017/05/05 14:04:23, haraken wrote:
> > > > +yukishiino
> > > > 
> > > > I was thinking that [SaveSameObject] is just a performance optimization
> > (i.e.,
> > > > we should not use it to guarantee correctness).
> > > > 
> > > > In this particular case, a right solution would be to change the C++
> > > > implementation of |supportedContentEncodings| so that it always returns
> the
> > > same
> > > > object (instead of relying on [SaveSameObject])... ?
> > > 
> > > We'd have to return a ScriptValue (since the Vector<String> return type
> would
> > be
> > > converted to a new v8::Value on each invocation), but where would we store
> it
> > in
> > > a way that its lifetime is correctly managed?
> > > 
> > > A new Supplementable on the ExecutionContext would be an option, solely
for
> > > holding this value, but that'd be a lot more complexity for (eventually)
the
> > > same result.
> > 
> > s/Supplementable/Supplement/
> 
> Yeah, you're right.
> 
> We should probably change the IDL compiler so that the generated code calls
the
> C++ implementation only at the first time and returns the cached result from
the
> second time. (i.e., make [SaveSameObject] the default behavior)
> 
> +yukishiino What is preventing us from making [SaveSameObject] the default
> behavior?

[SaveSameObject] requires a private property, and we (or I) don't want to abuse
private properties.  Private properties increase the binary size.  Currently we
don't have a good way to retrieve the right private property's key without using
unrecommended V8 APIs.  We may want to fix these issues around private
properties and make them robust infrastructure (in future).

If C++ implementation knows the reference to the returned object,
ToV8(impl->obj()) is the best way so far, I think, because we don't need any
additional mechanism or resources.  [SameObject] adds a test to check if the
returned object changes or not.  So, there should be no worry.

[haraken, 2017-05-08 07:52:31]

On 2017/05/08 07:48:20, Yuki(ooo_til_may8) wrote:
> On 2017/05/05 14:39:58, haraken wrote:
> > On 2017/05/05 14:24:07, Peter Beverloo wrote:
> > > On 2017/05/05 14:23:53, Peter Beverloo wrote:
> > > > On 2017/05/05 14:04:23, haraken wrote:
> > > > > +yukishiino
> > > > > 
> > > > > I was thinking that [SaveSameObject] is just a performance
optimization
> > > (i.e.,
> > > > > we should not use it to guarantee correctness).
> > > > > 
> > > > > In this particular case, a right solution would be to change the C++
> > > > > implementation of |supportedContentEncodings| so that it always
returns
> > the
> > > > same
> > > > > object (instead of relying on [SaveSameObject])... ?
> > > > 
> > > > We'd have to return a ScriptValue (since the Vector<String> return type
> > would
> > > be
> > > > converted to a new v8::Value on each invocation), but where would we
store
> > it
> > > in
> > > > a way that its lifetime is correctly managed?
> > > > 
> > > > A new Supplementable on the ExecutionContext would be an option, solely
> for
> > > > holding this value, but that'd be a lot more complexity for (eventually)
> the
> > > > same result.
> > > 
> > > s/Supplementable/Supplement/
> > 
> > Yeah, you're right.
> > 
> > We should probably change the IDL compiler so that the generated code calls
> the
> > C++ implementation only at the first time and returns the cached result from
> the
> > second time. (i.e., make [SaveSameObject] the default behavior)
> > 
> > +yukishiino What is preventing us from making [SaveSameObject] the default
> > behavior?
> 
> [SaveSameObject] requires a private property, and we (or I) don't want to
abuse
> private properties.  Private properties increase the binary size.  Currently
we
> don't have a good way to retrieve the right private property's key without
using
> unrecommended V8 APIs.  We may want to fix these issues around private
> properties and make them robust infrastructure (in future).
> 
> If C++ implementation knows the reference to the returned object,
> ToV8(impl->obj()) is the best way so far, I think, because we don't need any
> additional mechanism or resources.  [SameObject] adds a test to check if the
> returned object changes or not.  So, there should be no worry.

If the attribute returns a simple ScriptWrappable object, we can just make sure
that the C++ implementation returns the same C++ object assuming that toV8()
returns the same V8 wrapper for the C++ object.

The problem is that |supportedContentEncodings| is returning Vector<String>.
toV8Sequence returns a new v8 value every time it converts the vector to a v8
array.

[Yuki, 2017-05-08 09:04:07]

On 2017/05/08 07:52:31, haraken wrote:
> On 2017/05/08 07:48:20, Yuki(ooo_til_may8) wrote:
> > On 2017/05/05 14:39:58, haraken wrote:
> > > On 2017/05/05 14:24:07, Peter Beverloo wrote:
> > > > On 2017/05/05 14:23:53, Peter Beverloo wrote:
> > > > > On 2017/05/05 14:04:23, haraken wrote:
> > > > > > +yukishiino
> > > > > > 
> > > > > > I was thinking that [SaveSameObject] is just a performance
> optimization
> > > > (i.e.,
> > > > > > we should not use it to guarantee correctness).
> > > > > > 
> > > > > > In this particular case, a right solution would be to change the C++
> > > > > > implementation of |supportedContentEncodings| so that it always
> returns
> > > the
> > > > > same
> > > > > > object (instead of relying on [SaveSameObject])... ?
> > > > > 
> > > > > We'd have to return a ScriptValue (since the Vector<String> return
type
> > > would
> > > > be
> > > > > converted to a new v8::Value on each invocation), but where would we
> store
> > > it
> > > > in
> > > > > a way that its lifetime is correctly managed?
> > > > > 
> > > > > A new Supplementable on the ExecutionContext would be an option,
solely
> > for
> > > > > holding this value, but that'd be a lot more complexity for
(eventually)
> > the
> > > > > same result.
> > > > 
> > > > s/Supplementable/Supplement/
> > > 
> > > Yeah, you're right.
> > > 
> > > We should probably change the IDL compiler so that the generated code
calls
> > the
> > > C++ implementation only at the first time and returns the cached result
from
> > the
> > > second time. (i.e., make [SaveSameObject] the default behavior)
> > > 
> > > +yukishiino What is preventing us from making [SaveSameObject] the default
> > > behavior?
> > 
> > [SaveSameObject] requires a private property, and we (or I) don't want to
> abuse
> > private properties.  Private properties increase the binary size.  Currently
> we
> > don't have a good way to retrieve the right private property's key without
> using
> > unrecommended V8 APIs.  We may want to fix these issues around private
> > properties and make them robust infrastructure (in future).
> > 
> > If C++ implementation knows the reference to the returned object,
> > ToV8(impl->obj()) is the best way so far, I think, because we don't need any
> > additional mechanism or resources.  [SameObject] adds a test to check if the
> > returned object changes or not.  So, there should be no worry.
> 
> If the attribute returns a simple ScriptWrappable object, we can just make
sure
> that the C++ implementation returns the same C++ object assuming that toV8()
> returns the same V8 wrapper for the C++ object.
> 
> The problem is that |supportedContentEncodings| is returning Vector<String>.
> toV8Sequence returns a new v8 value every time it converts the vector to a v8
> array.

Chatted offline with haraken@.

Our conclusion is that

a) This CL should use [SaveSameObject] for now.

b) The bindings code generator can automatically determine if we should apply
[SaveSameObject] or not.  So we can get rid of [SaveSameObject] manually wrriten
in *.idl files.

[Yuki, 2017-05-08 09:07:08]

LGTM on my side.

[haraken, 2017-05-08 09:22:26]

bindings LGTM

[johnme, 2017-05-08 14:36:17]

push_messaging lgtm with nits

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

https://codereview.chromium.org/2863443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.cpp:47: return
Vector<String>({"aesgcm"});
Nit: should this be an AtomicString?

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

https://codereview.chromium.org/2863443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.idl:11:
[SameObject, SaveSameObject]
Nit: why are these on a different line, but subscribe below isn't wrapped?

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

https://codereview.chromium.org/2863443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushSubscription.idl:13:
Exposed=(Window,Worker),
Would it be easy to extend http/tests/push_messaging/interfaces.html to cover
workers (in addition to SWs and documents)?

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

https://codereview.chromium.org/2863443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/ServiceWorkerRegistrationPush.idl:8:
Exposed=(Window,Worker),
Oddly, pushManager was already exposed on ServiceWorkerRegistration in
webexposed/global-interface-listing-dedicated-worker-expected.txt. Perhaps
Exposed on partial interfaces doesn't do anything? If so consider removing it
and/or replacing it with a comment.

[Peter Beverloo, 2017-05-08 15:00:12]

Thanks John, PTAL - a few questions

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

https://codereview.chromium.org/2863443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.cpp:47: return
Vector<String>({"aesgcm"});
On 2017/05/08 14:36:16, johnme wrote:
> Nit: should this be an AtomicString?

That seems a bit excessive. This method is only being called once per
thread(/context) as we cache the result in the bindings code. We wouldn't gain
much.

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

https://codereview.chromium.org/2863443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.idl:11:
[SameObject, SaveSameObject]
On 2017/05/08 14:36:16, johnme wrote:
> Nit: why are these on a different line, but subscribe below isn't wrapped?

Admittedly because I like being able to copy/paste the IDL defined in the
specification. The additions below are implementation-specific as they amend
which arguments are being passed to the implementation function. Do you feel
strongly?

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

https://codereview.chromium.org/2863443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushSubscription.idl:13:
Exposed=(Window,Worker),
On 2017/05/08 14:36:16, johnme wrote:
> Would it be easy to extend http/tests/push_messaging/interfaces.html to cover
> workers (in addition to SWs and documents)?

I'd argue in the opposite direction - what do you think about removing that test
altogether? It's redundant with the webexposed interface listings.

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

https://codereview.chromium.org/2863443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/ServiceWorkerRegistrationPush.idl:8:
Exposed=(Window,Worker),
On 2017/05/08 14:36:16, johnme wrote:
> Oddly, pushManager was already exposed on ServiceWorkerRegistration in
> webexposed/global-interface-listing-dedicated-worker-expected.txt. Perhaps
> Exposed on partial interfaces doesn't do anything? If so consider removing it
> and/or replacing it with a comment.

IDLCompiler.md details that due to the way we implement partial interfaces
(basically merging it), a subset of interface-level annotations will be migrated
to the individual members. [Exposed] is not part of that, so based on that,
proven by it being in that file, implies that it's indeed the case.

However, per the WebIDL spec, having [Exposed] on a partial interface is
perfectly fine, so we should keep it. I guess we just don't carry it over
because there's no demand for the feature yet. Since it's valid, a comment feels
fragile since it'd bound to go out of date when this does get implemented.

[johnme, 2017-05-08 15:09:27]

still lgtm :)

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

https://codereview.chromium.org/2863443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.cpp:47: return
Vector<String>({"aesgcm"});
On 2017/05/08 15:00:11, Peter Beverloo wrote:
> On 2017/05/08 14:36:16, johnme wrote:
> > Nit: should this be an AtomicString?
> 
> That seems a bit excessive. This method is only being called once per
> thread(/context) as we cache the result in the bindings code. We wouldn't gain
> much.

Acknowledged.

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

https://codereview.chromium.org/2863443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.idl:11:
[SameObject, SaveSameObject]
On 2017/05/08 15:00:11, Peter Beverloo wrote:
> On 2017/05/08 14:36:16, johnme wrote:
> > Nit: why are these on a different line, but subscribe below isn't wrapped?
> 
> Admittedly because I like being able to copy/paste the IDL defined in the
> specification. The additions below are implementation-specific as they amend
> which arguments are being passed to the implementation function. Do you feel
> strongly?

Acknowledged.

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

https://codereview.chromium.org/2863443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushSubscription.idl:13:
Exposed=(Window,Worker),
On 2017/05/08 15:00:11, Peter Beverloo wrote:
> On 2017/05/08 14:36:16, johnme wrote:
> > Would it be easy to extend http/tests/push_messaging/interfaces.html to
cover
> > workers (in addition to SWs and documents)?
> 
> I'd argue in the opposite direction - what do you think about removing that
test
> altogether? It's redundant with the webexposed interface listings.

Fair. The test goes a little further, e.g. exercises constructors. But I agree
that the webexposed ones tend to be clearer.

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

https://codereview.chromium.org/2863443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/ServiceWorkerRegistrationPush.idl:8:
Exposed=(Window,Worker),
On 2017/05/08 15:00:11, Peter Beverloo wrote:
> On 2017/05/08 14:36:16, johnme wrote:
> > Oddly, pushManager was already exposed on ServiceWorkerRegistration in
> > webexposed/global-interface-listing-dedicated-worker-expected.txt. Perhaps
> > Exposed on partial interfaces doesn't do anything? If so consider removing
it
> > and/or replacing it with a comment.
> 
> IDLCompiler.md details that due to the way we implement partial interfaces
> (basically merging it), a subset of interface-level annotations will be
migrated
> to the individual members. [Exposed] is not part of that, so based on that,
> proven by it being in that file, implies that it's indeed the case.
> 
> However, per the WebIDL spec, having [Exposed] on a partial interface is
> perfectly fine, so we should keep it. I guess we just don't carry it over
> because there's no demand for the feature yet. Since it's valid, a comment
feels
> fragile since it'd bound to go out of date when this does get implemented.

Ok. Mind filing a bug to mark ServiceWorkerRegistrationNotifications.idl and
ServiceWorkerRegistrationSync.idl as exposed on (Window,Worker) as well?

[Peter Beverloo, 2017-05-08 15:53:15]

Thanks!

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

https://codereview.chromium.org/2863443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushSubscription.idl:13:
Exposed=(Window,Worker),
On 2017/05/08 15:09:27, johnme wrote:
> On 2017/05/08 15:00:11, Peter Beverloo wrote:
> > On 2017/05/08 14:36:16, johnme wrote:
> > > Would it be easy to extend http/tests/push_messaging/interfaces.html to
> cover
> > > workers (in addition to SWs and documents)?
> > 
> > I'd argue in the opposite direction - what do you think about removing that
> test
> > altogether? It's redundant with the webexposed interface listings.
> 
> Fair. The test goes a little further, e.g. exercises constructors. But I agree
> that the webexposed ones tend to be clearer.

https://codereview.chromium.org/2869793003/

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

https://codereview.chromium.org/2863443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/ServiceWorkerRegistrationPush.idl:8:
Exposed=(Window,Worker),
On 2017/05/08 15:09:27, johnme wrote:
> On 2017/05/08 15:00:11, Peter Beverloo wrote:
> > On 2017/05/08 14:36:16, johnme wrote:
> > > Oddly, pushManager was already exposed on ServiceWorkerRegistration in
> > > webexposed/global-interface-listing-dedicated-worker-expected.txt. Perhaps
> > > Exposed on partial interfaces doesn't do anything? If so consider removing
> it
> > > and/or replacing it with a comment.
> > 
> > IDLCompiler.md details that due to the way we implement partial interfaces
> > (basically merging it), a subset of interface-level annotations will be
> migrated
> > to the individual members. [Exposed] is not part of that, so based on that,
> > proven by it being in that file, implies that it's indeed the case.
> > 
> > However, per the WebIDL spec, having [Exposed] on a partial interface is
> > perfectly fine, so we should keep it. I guess we just don't carry it over
> > because there's no demand for the feature yet. Since it's valid, a comment
> feels
> > fragile since it'd bound to go out of date when this does get implemented.
> 
> Ok. Mind filing a bug to mark ServiceWorkerRegistrationNotifications.idl and
> ServiceWorkerRegistrationSync.idl as exposed on (Window,Worker) as well?

https://codereview.chromium.org/2866023003

[Peter Beverloo, 2017-05-16 14:20:07]

jochen - PTAL for webexposed OWNERS, the I2I&S passed :)

[jochen (gone - plz use gerrit), 2017-05-16 14:25:14]

lgtm

[Peter Beverloo, 2017-05-16 14:36:56]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2017-05-16 14:36:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, johnme@chromium.org, jochen@chromium.org,
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2863443002/#ps60001
(title: "Implement and ship PushManager.supportedContentEncodings")

[commit-bot: I haz the power, 2017-05-16 14:37:38]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 16:14:02]

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

[commit-bot: I haz the power, 2017-05-16 16:14:03]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Beverloo, 2017-05-16 16:34:52]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2017-05-16 16:35:32]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 17:30:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494952492221830,
"parent_rev": "a9935effb606cac69554673147c9bfc6484b0b9c", "commit_rev":
"60b0f2c0bbf2fbb96aa09a781f5adac7981832df"}

[commit-bot: I haz the power, 2017-05-16 17:30:32]

Description was changed from

==========
Implement and ship PushManager.supportedContentEncodings

This static property can be used by developers to check which
Content-Encoding values are supported by the implementation for the
encrypted message payloads that a push message can carry.

Right now we only support "aesgcm", which corresponds to
draft-ietf-webpush-encryption-05. Support for the latest version of the
encryption scheme, identified by "aes128gcm" and required by the
specification, is tracked by crbug.com/679789.

This CL also exposes the PushManager, PushSubscription,
PushSubscriptionOptions and extensions to the ServiceWorkerRegistration
interfaces in all workers, matching availability of SWRegistration.

Specification:
https://w3c.github.io/push-api/#dom-pushmanager-supportedcontentencodings

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/jNg1Zth2XrI/discus...

BUG=718020
==========

to

==========
Implement and ship PushManager.supportedContentEncodings

This static property can be used by developers to check which
Content-Encoding values are supported by the implementation for the
encrypted message payloads that a push message can carry.

Right now we only support "aesgcm", which corresponds to
draft-ietf-webpush-encryption-05. Support for the latest version of the
encryption scheme, identified by "aes128gcm" and required by the
specification, is tracked by crbug.com/679789.

This CL also exposes the PushManager, PushSubscription,
PushSubscriptionOptions and extensions to the ServiceWorkerRegistration
interfaces in all workers, matching availability of SWRegistration.

Specification:
https://w3c.github.io/push-api/#dom-pushmanager-supportedcontentencodings

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/jNg1Zth2XrI/discus...

BUG=718020

Review-Url: https://codereview.chromium.org/2863443002
Cr-Commit-Position: refs/heads/master@{#472146}
Committed:
https://chromium.googlesource.com/chromium/src/+/60b0f2c0bbf2fbb96aa09a781f5a...
==========

[commit-bot: I haz the power, 2017-05-16 17:30:33]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/60b0f2c0bbf2fbb96aa09a781f5a...