|
|
Created:
5 years, 7 months ago by vivekg_samsung Modified:
5 years, 6 months ago Reviewers:
haraken, vivekg, jsbell, scheib CC:
arv+blink, blink-reviews, blink-reviews-bindings_chromium.org, blink-reviews-dom_chromium.org, Inactive, dglazkov+blink, eae+blinkwatch, falken, horo+watch_chromium.org, jsbell+serviceworker_chromium.org, kinuko+worker_chromium.org, kinuko+serviceworker, michaeln, nhiroki, rwlbuis, serviceworker-reviews, sof, tzik, vivekg_samsung Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
Description[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
Patch Set 1 #
Total comments: 1
Patch Set 2 : Bindings change alone #
Total comments: 2
Patch Set 3 : Typo correction: missing space in methods.cpp #
Created: 5 years, 7 months ago
Messages
Total messages: 19 (3 generated)
vivekg@chromium.org changed reviewers: + vivekg@chromium.org
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?
Also this patch will be broken down in smaller pieces once we agree on the approach.
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].
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.
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.
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).
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.
The CQ bit was checked by vivekg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1159653002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=195860
Message was sent while issue was closed.
scheib@chromium.org changed reviewers: + scheib@chromium.org
Message was sent while issue was closed.
Please document PostMessage on https://www.chromium.org/blink/webidl/blink-idl-extended-attributes
Message was sent while issue was closed.
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...
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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 :)
Message was sent while issue was closed.
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! |