[bindings] Introduce PostMessage extended keyword

[bindings] [bindings] Introduce PostMessage extended keyword 

This CL is prelude to removal of usage of custom bindings for IDLs using postMessage.
In that, it adds the support for the |PostMessage| extended keyword.

R=haraken@chromium.org, jsbell@chromium.org

BUG=345519
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195860

[vivekg, 2015-05-25 09:56:29]

vivekg@chromium.org changed reviewers:
+ vivekg@chromium.org

[vivekg, 2015-05-25 09:56:29]

This is a WIP patch to remove the custom bindings required for postMessage.
Ideally we can also remove the bindings/core/v8/PostMessage.h and embed this in
the template code itself. Anyways, this being a template method, I thought of
having it "as is". 

One more doubt: Some of the classes pass different |interfaceName| from the idl
name per se 

DedicatedWorkerGlobalScope => WorkerGlobalScope
Client => ServiceWorkerClient
CrossOriginServiceWorkerClient => ServiceWorkerClient

As per the generated code, the IDL interface name itself would be used here. 

Should we provide one more extended attribute such as
|AliasInterfaceName=WorkerGlobalScope| while defining postMessage in
DedicatedWorkerGlobalScope.idl? I think this is an overkill and adding one more
extended attribute would deviate from the standard web IDL. So in this case, is
it ok to use the same interfaceName as the IDL interface?

[vivekg, 2015-05-25 09:57:22]

Also this patch will be broken down in smaller pieces once we agree on the
approach.

[haraken, 2015-05-25 10:26:48]

https://codereview.chromium.org/1159653002/diff/1/Source/core/dom/MessagePort...
File Source/core/dom/MessagePort.idl (right):

https://codereview.chromium.org/1159653002/diff/1/Source/core/dom/MessagePort...
Source/core/dom/MessagePort.idl:34: [Custom=PostMessageCommon, RaisesException,
Measure] void postMessage(any message, optional sequence<Transferable>
transfer);

Nit: Given that this doesn't require custom bindings, I'd prefer [PostMessage]
rather than [Custom=PostMessage].

[haraken, 2015-05-25 10:27:38]

The approach looks good to me.

> One more doubt: Some of the classes pass different |interfaceName| from the
idl
> name per se 
> 
> DedicatedWorkerGlobalScope => WorkerGlobalScope
> Client => ServiceWorkerClient
> CrossOriginServiceWorkerClient => ServiceWorkerClient
> 
> As per the generated code, the IDL interface name itself would be used here. 
> 
> Should we provide one more extended attribute such as
> |AliasInterfaceName=WorkerGlobalScope| while defining postMessage in
> DedicatedWorkerGlobalScope.idl? I think this is an overkill and adding one
more
> extended attribute would deviate from the standard web IDL. So in this case,
is
> it ok to use the same interfaceName as the IDL interface?

Yeah, it would make sense to fix the core/ side and avoid introducing new IDL
extended attributes.

[vivekg, 2015-05-25 11:32:39]

On 2015/05/25 at 10:26:48, haraken wrote:
>
https://codereview.chromium.org/1159653002/diff/1/Source/core/dom/MessagePort...
> File Source/core/dom/MessagePort.idl (right):
> 
>
https://codereview.chromium.org/1159653002/diff/1/Source/core/dom/MessagePort...
> Source/core/dom/MessagePort.idl:34: [Custom=PostMessageCommon,
RaisesException, Measure] void postMessage(any message, optional
sequence<Transferable> transfer);
> 
> Nit: Given that this doesn't require custom bindings, I'd prefer [PostMessage]
rather than [Custom=PostMessage].

Done. Also have split the CL into just bindings change alone for now. The other
CLs will follow in continuation.

[haraken, 2015-05-25 11:37:45]

LGTM

https://codereview.chromium.org/1159653002/diff/20001/Source/bindings/templat...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/1159653002/diff/20001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:476:
postMessageMethodCommon("{{interface_name}}",
{{v8_class}}::toImpl(info.Holder()), info);

Nit: I'd rename this to postMessageImpl, but you can do that when removing
PostMessage.h (i.e., auto-generate postMessageImpl).

[vivekg, 2015-05-25 11:40:00]

https://codereview.chromium.org/1159653002/diff/20001/Source/bindings/templat...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/1159653002/diff/20001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:476:
postMessageMethodCommon("{{interface_name}}",
{{v8_class}}::toImpl(info.Holder()), info);
On 2015/05/25 11:37:45, haraken wrote:
> 
> Nit: I'd rename this to postMessageImpl, but you can do that when removing
> PostMessage.h (i.e., auto-generate postMessageImpl).

Sure, I will change it in the follow up CL. Thanks.

[vivekg, 2015-05-25 11:40:49]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2015-05-25 11:41:10]

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

[commit-bot: I haz the power, 2015-05-25 13:04:37]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195860

[scheib, 2015-06-23 18:12:31]

scheib@chromium.org changed reviewers:
+ scheib@chromium.org

[scheib, 2015-06-23 18:12:32]

Please document PostMessage on
https://www.chromium.org/blink/webidl/blink-idl-extended-attributes

[vivekg, 2015-06-23 19:17:07]

On 2015/06/23 at 18:12:32, scheib wrote:
> Please document PostMessage on
https://www.chromium.org/blink/webidl/blink-idl-extended-attributes

Done. PTAL
https://sites.google.com/a/chromium.org/dev/blink/webidl/blink-idl-extended-a...

[blink-reviews, 2015-06-23 20:04:16]

Thanks, though I don't understand from that documentation what is done
differently in the bindings. I'm reviewing a patch for someone and I still
don't understand what the affect of their adding [PostMessage] to a method
will have. I'll try to research more, but it would be best for
documentation to describe this or link to more information.

On Tue, Jun 23, 2015 at 12:17 PM, <vivekg@chromium.org> wrote:

> On 2015/06/23 at 18:12:32, scheib wrote:
>
>> Please document PostMessage on
>>
> https://www.chromium.org/blink/webidl/blink-idl-extended-attributes
>
> Done. PTAL
>
>
https://sites.google.com/a/chromium.org/dev/blink/webidl/blink-idl-extended-a...
>
> https://codereview.chromium.org/1159653002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[vivekg, 2015-06-24 00:57:35]

On 2015/06/23 at 20:04:16, blink-reviews wrote:
> Thanks, though I don't understand from that documentation what is done
> differently in the bindings. I'm reviewing a patch for someone and I still
> don't understand what the affect of their adding [PostMessage] to a method
> will have. I'll try to research more, but it would be best for
> documentation to describe this or link to more information.
> 

If you take a look at [1], earlier we needed a custom bindings code just to
invoke the postMessageMethodCommon which will extract
the transferable object. This code can be auto-generated with the help of
|PostMessage| attribute, see [2] for some more details.
This attribute reduces the author's work by eliminating a relaying custom
bindings. Hope this helps.

[1] https://codereview.chromium.org/1156973002
[2] https://codereview.chromium.org/1149623004

[scheib, 2015-06-24 03:52:20]

It helps somewhat? Expecting readers to find and read those patches isn't
reasonable, that's why we have documentation. I suggest the documentation
address:
+ Short explanation of why postMessage needs special handling, (seems to be
that transferable objects need handling, and exceptions). So, something
like "postMessage results in calls being made out of a worker to another
worker or main document. Transferable objects must be extracted for this,
..... e.g. arrays ..... e.g. passing message ports ... .
+ Indicate what the IDL looks like (done), and what the corresponding C++
interface must be.
https://sites.google.com/a/chromium.org/dev/blink/webidl/blink-idl-extended-a...
is an example of this.
+ Explain what those parameters are, what should an implementation do with
context, message, ports, and exceptionState?

A Blink developer who's never worked with postmessage or message ports
should be able to understand what these objects are for.


On Tue, Jun 23, 2015 at 5:57 PM, <vivekg@chromium.org> wrote:

> On 2015/06/23 at 20:04:16, blink-reviews wrote:
>
>> Thanks, though I don't understand from that documentation what is done
>> differently in the bindings. I'm reviewing a patch for someone and I still
>> don't understand what the affect of their adding [PostMessage] to a method
>> will have. I'll try to research more, but it would be best for
>> documentation to describe this or link to more information.
>>
>
>
> If you take a look at [1], earlier we needed a custom bindings code just to
> invoke the postMessageMethodCommon which will extract
> the transferable object. This code can be auto-generated with the help of
> |PostMessage| attribute, see [2] for some more details.
> This attribute reduces the author's work by eliminating a relaying custom
> bindings. Hope this helps.
>
> [1] https://codereview.chromium.org/1156973002
> [2] https://codereview.chromium.org/1149623004
>
> https://codereview.chromium.org/1159653002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[vivekg, 2015-06-24 04:01:01]

On 2015/06/24 at 03:52:20, scheib wrote:
> It helps somewhat? Expecting readers to find and read those patches isn't
> reasonable, that's why we have documentation. I suggest the documentation
> address:
> + Short explanation of why postMessage needs special handling, (seems to be
> that transferable objects need handling, and exceptions). So, something
> like "postMessage results in calls being made out of a worker to another
> worker or main document. Transferable objects must be extracted for this,
> ..... e.g. arrays ..... e.g. passing message ports ... .
> + Indicate what the IDL looks like (done), and what the corresponding C++
> interface must be.
>
https://sites.google.com/a/chromium.org/dev/blink/webidl/blink-idl-extended-a...
> is an example of this.
> + Explain what those parameters are, what should an implementation do with
> context, message, ports, and exceptionState?
> 
> A Blink developer who's never worked with postmessage or message ports
> should be able to understand what these objects are for.
> 

Yeah sure, I will update the documentation accordingly. Thanks for your feedback
:)

[haraken, 2015-06-24 04:25:27]

On 2015/06/24 04:01:01, vivekg_ wrote:
> On 2015/06/24 at 03:52:20, scheib wrote:
> > It helps somewhat? Expecting readers to find and read those patches isn't
> > reasonable, that's why we have documentation. I suggest the documentation
> > address:
> > + Short explanation of why postMessage needs special handling, (seems to be
> > that transferable objects need handling, and exceptions). So, something
> > like "postMessage results in calls being made out of a worker to another
> > worker or main document. Transferable objects must be extracted for this,
> > ..... e.g. arrays ..... e.g. passing message ports ... .
> > + Indicate what the IDL looks like (done), and what the corresponding C++
> > interface must be.
> >
>
https://sites.google.com/a/chromium.org/dev/blink/webidl/blink-idl-extended-a...
> > is an example of this.
> > + Explain what those parameters are, what should an implementation do with
> > context, message, ports, and exceptionState?
> > 
> > A Blink developer who's never worked with postmessage or message ports
> > should be able to understand what these objects are for.
> > 
> 
> Yeah sure, I will update the documentation accordingly. Thanks for your
feedback
> :)

Yes, thanks for your feedback!