Bug 80200 – Add transfer map argument to Intent constructor

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Greg Billock]

Add transfer map argument to Intent constructor

[Greg Billock]

Created attachment 129973 [details]
Patch

[WebKit Review Bot]

Attachment 129973 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:63:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:69:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:78:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:89:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:101:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 5 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

You probably don't need custom bindings.

[Kentaro Hara]

Comment on attachment 129973 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129973&action=review

> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:84
> +        MessagePortArray messagePortArray;
> +        ArrayBufferArray arrayBufferArray;
> +        if (!extractTransferables(args[3], messagePortArray, arrayBufferArray))
> +            return throwError("Transfer map must contain MessagePorts or ArrayBuffers");

You are using custom bindings just because you need to call extractTransferables(), right?

V8DOMWindowCustom.cpp, V8WorkerCustom.cpp, V8MessagePortCustom.cpp and V8DedicatedWorkerContextCustom.cpp are also using extractTransferables() in custom bindings. I think we can introduce a new IDL attribute [V8ExtractTransferables] and then remove those custom bindings + V8IntentCustom.cpp.

[Dmitry Lomov]

(In reply to comment #4)
> (From update of attachment 129973 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129973&action=review
> 
> > Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:84
> > +        MessagePortArray messagePortArray;
> > +        ArrayBufferArray arrayBufferArray;
> > +        if (!extractTransferables(args[3], messagePortArray, arrayBufferArray))
> > +            return throwError("Transfer map must contain MessagePorts or ArrayBuffers");
> 
> You are using custom bindings just because you need to call extractTransferables(), right?
> 
> V8DOMWindowCustom.cpp, V8WorkerCustom.cpp, V8MessagePortCustom.cpp and V8DedicatedWorkerContextCustom.cpp are also using extractTransferables() in custom bindings. I think we can introduce a new IDL attribute [V8ExtractTransferables] and then remove those custom bindings + V8IntentCustom.cpp.

This is a good idea in principle, however if you look closely you will see that all those places extract transferables in not quite the same way due to postMessage/webkitPostMessage dichotomy. I do hope that will be resolved soon (ance the spec emerges from the strawman), and then we can do what you say - feel free to open a bug to add V8ExtractTransfrables attribute and assign it to me.
In the meanwhile, let's do custom bindings here for a bit.

[Kentaro Hara]

(In reply to comment #5)
> This is a good idea in principle, however if you look closely you will see that all those places extract transferables in not quite the same way due to postMessage/webkitPostMessage dichotomy. I do hope that will be resolved soon (ance the spec emerges from the strawman), and then we can do what you say - feel free to open a bug to add V8ExtractTransfrables attribute and assign it to me.
> In the meanwhile, let's do custom bindings here for a bit.

Then, shall we introduce the [V8ExtractTransfrables] IDL attribute so that it meets the usage in Intent.idl? After that, we can fix other IDL files so that they can also use the [V8ExtractTransfrables] IDL attribute.

BTW, if you do want to write V8IntentCustom.cpp, please do the following (At present, V8IntentCustom.cpp in the patch is full of bugs):

(1) Remove [CustomConstructor] IDL attribute
(2) Build
(3) In a few minutes, src/out/Release/obj/gen/webcore/bindings/V8Intent.cpp will be generated automatically
(4) Copy the generated V8Intent.cpp to WebCore/custom/v8/custom/V8IntentCustom.cpp
(5) Modify V8IntentCustom.cpp as you like

[Dmitry Lomov]

(In reply to comment #6)
> (In reply to comment #5)
> > This is a good idea in principle, however if you look closely you will see that all those places extract transferables in not quite the same way due to postMessage/webkitPostMessage dichotomy. I do hope that will be resolved soon (ance the spec emerges from the strawman), and then we can do what you say - feel free to open a bug to add V8ExtractTransfrables attribute and assign it to me.
> > In the meanwhile, let's do custom bindings here for a bit.
> 
> Then, shall we introduce the [V8ExtractTransfrables] IDL attribute so that it meets the usage in Intent.idl? After that, we can fix other IDL files so that they can also use the [V8ExtractTransfrables] IDL attribute.

I am not sure what good purpose having a single attribute with a single use would serve? 
However if Greg feels comfortable doing that, I think it is ok. 

> 
> BTW, if you do want to write V8IntentCustom.cpp, please do the following (At present, V8IntentCustom.cpp in the patch is full of bugs):
> 
> (1) Remove [CustomConstructor] IDL attribute
> (2) Build
> (3) In a few minutes, src/out/Release/obj/gen/webcore/bindings/V8Intent.cpp will be generated automatically
> (4) Copy the generated V8Intent.cpp to WebCore/custom/v8/custom/V8IntentCustom.cpp
> (5) Modify V8IntentCustom.cpp as you like

This is a good recipe :) My understanding is that Greg put this patch up as a quick-and-dirty "proof-of-concept" - it shouldn't have been marked r? :)

[Kentaro Hara]

(In reply to comment #7)
> I am not sure what good purpose having a single attribute with a single use would serve? 
> However if Greg feels comfortable doing that, I think it is ok. 

But I guess we should eventually use [V8ExtractTransfrables] also in Worker.idl, DedicatedWorkerContext.idl, MessagePort.idl, etc and remove their custom bindings. It would make sense to introduce [V8ExtractTransfrables] to Intent.idl as a first use case. Anyway we do not want to increase the number of custom bindings since they are likely to be buggy.

[Dmitry Lomov]

(In reply to comment #8)
> (In reply to comment #7)
> > I am not sure what good purpose having a single attribute with a single use would serve? 
> > However if Greg feels comfortable doing that, I think it is ok. 
> 
> But I guess we should eventually use [V8ExtractTransfrables] also in Worker.idl, DedicatedWorkerContext.idl, MessagePort.idl, etc and remove their custom bindings. It would make sense to introduce [V8ExtractTransfrables] to Intent.idl as a first use case. Anyway we do not want to increase the number of custom bindings since they are likely to be buggy.

I do hope this will happen; note btw that ExtractTransferables is not limited to V8 - the same concept exists in JSC as well.

[Adam Barth]

It's better to improve the code generator than to write custom bindings.  Many attributes started off being used in only a small number of places.

[Greg Billock]

(In reply to comment #4)
> (From update of attachment 129973 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129973&action=review
> 
> > Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:84
> > +        MessagePortArray messagePortArray;
> > +        ArrayBufferArray arrayBufferArray;
> > +        if (!extractTransferables(args[3], messagePortArray, arrayBufferArray))
> > +            return throwError("Transfer map must contain MessagePorts or ArrayBuffers");
> 
> You are using custom bindings just because you need to call extractTransferables(), right?
> 
> V8DOMWindowCustom.cpp, V8WorkerCustom.cpp, V8MessagePortCustom.cpp and V8DedicatedWorkerContextCustom.cpp are also using extractTransferables() in custom bindings. I think we can introduce a new IDL attribute [V8ExtractTransferables] and then remove those custom bindings + V8IntentCustom.cpp.

That'd be awesome. I think the reason I need custom here (Dmitry can confirm or deny) is that the "Array" type doesn't translate correctly. It'd be much nicer to not have the custom bits. It'd be really keen to code-gen the transferable support right into the creation of SerializedScriptValue so when the non-custom method is called, that's already magically happened. :-) I think this is the main use case, and as you point out, there are several parallel implementations of this.

[Greg Billock]

Created attachment 130170 [details]
Patch

[Greg Billock]

(In reply to comment #6)
> (In reply to comment #5)
> > This is a good idea in principle, however if you look closely you will see that all those places extract transferables in not quite the same way due to postMessage/webkitPostMessage dichotomy. I do hope that will be resolved soon (ance the spec emerges from the strawman), and then we can do what you say - feel free to open a bug to add V8ExtractTransfrables attribute and assign it to me.
> > In the meanwhile, let's do custom bindings here for a bit.
> 
> Then, shall we introduce the [V8ExtractTransfrables] IDL attribute so that it meets the usage in Intent.idl? After that, we can fix other IDL files so that they can also use the [V8ExtractTransfrables] IDL attribute.
> 
> BTW, if you do want to write V8IntentCustom.cpp, please do the following (At present, V8IntentCustom.cpp in the patch is full of bugs):
> 
> (1) Remove [CustomConstructor] IDL attribute
> (2) Build
> (3) In a few minutes, src/out/Release/obj/gen/webcore/bindings/V8Intent.cpp will be generated automatically
> (4) Copy the generated V8Intent.cpp to WebCore/custom/v8/custom/V8IntentCustom.cpp
> (5) Modify V8IntentCustom.cpp as you like

I did this, which helped: realized the 0-arg call that'd been puzzling me was in wrap mode, and is now handled.

But really, I *don't* want this to be custom. What'd be great is to be able to say "in [Optional=TransferArrayArg] Array transferMap" and have that get automagically handled by the code gen in making the SerializedScriptValue.

Especially because now I'm getting errors from V8Utilities that apparently I need to massage this value somehow, as it isn't an object. :-( There's quite a bit of parallel code here and there about this, so it'd be great to have a re-usable way to expose this clone/transfer operation.

[WebKit Review Bot]

Comment on attachment 130170 [details]
Patch

Attachment 130170 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11807999

New failing tests:
webintents/web-intents-api.html

[Kentaro Hara]

Comment on attachment 130170 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130170&action=review

> Source/WebCore/Modules/intents/Intent.idl:30
> +      Constructor(in DOMString action, in DOMString type, in [Optional=DefaultIsNullString] SerializedScriptValue data, in [Optional] Array transferMap),

[Optional=DefaultIsNullString] can be specified on DOMString only (https://trac.webkit.org/wiki/WebKitIDL#Optional). Maybe it should be [Optional=DefaultIsUndefined].

> But really, I *don't* want this to be custom. What'd be great is to be able to say "in [Optional=TransferArrayArg] Array transferMap" and have that get automagically handled by the code gen in making the SerializedScriptValue.

Agreed. Shall we introduce it?

> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:60
> +    if (args.Length() > 4)
> +        return throwError("Too many arguments to intent constructor", V8Proxy::TypeError);

Is this behavior speced? Normally we do not throw an error for too much arguments.

> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:89
> +        return throwError("Could not serialize intent data");

Is this behavior speced? Normally we return v8::Undefined() in such cases.

[Dmitry Lomov]

(In reply to comment #15)
> (From update of attachment 130170 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130170&action=review
> 
> > Source/WebCore/Modules/intents/Intent.idl:30
> > +      Constructor(in DOMString action, in DOMString type, in [Optional=DefaultIsNullString] SerializedScriptValue data, in [Optional] Array transferMap),
> 
> [Optional=DefaultIsNullString] can be specified on DOMString only (https://trac.webkit.org/wiki/WebKitIDL#Optional). Maybe it should be [Optional=DefaultIsUndefined].
> 
> > But really, I *don't* want this to be custom. What'd be great is to be able to say "in [Optional=TransferArrayArg] Array transferMap" and have that get automagically handled by the code gen in making the SerializedScriptValue.
> 
> Agreed. Shall we introduce it?

FWIW, I am still split on this one. It feels too early to do this, and not very useful for this single case.
Note that that transfer array attribute (it is a _list_, not a map) should in reality specify a _pair_ of arguments (in this case 'transfer' is a transfer list for 'intentData' argument). So it should be something like:

   [Optional=TransferArray(intentData)] Array trasferList

I would prefer not polluting WebkitIDL further with single-use-case V8-specific attributes though (until they are non-single use-case and apply to JSC too).

> 
> > Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:60
> > +    if (args.Length() > 4)
> > +        return throwError("Too many arguments to intent constructor", V8Proxy::TypeError);
> 
> Is this behavior speced? Normally we do not throw an error for too much arguments.
> 
> > Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:89
> > +        return throwError("Could not serialize intent data");
> 
> Is this behavior speced? Normally we return v8::Undefined() in such cases.

[Greg Billock]

Created attachment 131394 [details]
Patch

[Adam Barth]

I feel like I'm missing something in this bug.  Maybe it would be better to talk by voice or chat?  If this API is expressible in WebIDL, then we should be able to express it in WebKitIDL by improving the code generator.  If it's not expressible in WebIDL, then we should probably choose a different design that is.

[Dmitry Lomov]

(In reply to comment #18)
> I feel like I'm missing something in this bug.  Maybe it would be better to talk by voice or chat?  If this API is expressible in WebIDL, then we should be able to express it in WebKitIDL by improving the code generator.  If it's not expressible in WebIDL, then we should probably choose a different design that is.

Adam, API is expressible in WebIDL, it is similar to what happens in postMessage. 
With luck, we will be able to codegen more of the bindings. I know that Greg is working on this for Intents, and I'll be taking a look at adapting that code gen to cover other use cases. The patch is a work-in-progress at this point.

[Dmitry Lomov]

Comment on attachment 131394 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131394&action=review

> Source/WebCore/Modules/intents/Intent.cpp:87
> +        m_data = SerializedScriptValue::createFromWire(data->toWireString());

Why do you need these conversions?

[Adam Barth]

> Adam, API is expressible in WebIDL, it is similar to what happens in postMessage. 

Ok, great.

[Greg Billock]

Comment on attachment 131394 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131394&action=review

>> Source/WebCore/Modules/intents/Intent.cpp:87
>> +        m_data = SerializedScriptValue::createFromWire(data->toWireString());
> 
> Why do you need these conversions?

I'm basically taking a copy, which will get passed out to the embedder.

[Dmitry Lomov]

Comment on attachment 131394 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131394&action=review

>>> Source/WebCore/Modules/intents/Intent.cpp:87
>>> +        m_data = SerializedScriptValue::createFromWire(data->toWireString());
>> 
>> Why do you need these conversions?
> 
> I'm basically taking a copy, which will get passed out to the embedder.

I think it is not needed - you can just keep a RefPtr to SSV. SSV is not mutable once created, so no danger

[Adam Barth]

> API is expressible in WebIDL

I talked this over with Greg, and he seems to think the coupling between these two parameters isn't expressible in WebIDL.  If there's a way to express it, that would be good to know so we can model the WebKitIDL syntax off the WebIDL syntax.

Greg suggested that we use the following syntax:

  [...] foo(in [TransferMap=transfers] any data, in [Optional] transfers)

which seems reasonable.

[Dmitry Lomov]

(In reply to comment #24)
> > API is expressible in WebIDL
> 
> I talked this over with Greg, and he seems to think the coupling between these two parameters isn't expressible in WebIDL.  If there's a way to express it, that would be good to know so we can model the WebKitIDL syntax off the WebIDL syntax.
> 
> Greg suggested that we use the following syntax:
> 
>   [...] foo(in [TransferMap=transfers] any data, in [Optional] transfers)
> 
> which seems reasonable.

1. s/TransferMap/TransferList/ (or Transfer).
2. No strong preference, but why the attribute on 'data', not on 'transfers'. Any precdents here?
3. Need to imply optionality. I.e. for intents, data is optional, and transfers is optional. Valid combinations are (...), (...,data), (...,data, transfer). Can we express that in WebIDL?

[Adam Barth]

(In reply to comment #25)
> (In reply to comment #24)
> > > API is expressible in WebIDL
> > 
> > I talked this over with Greg, and he seems to think the coupling between these two parameters isn't expressible in WebIDL.  If there's a way to express it, that would be good to know so we can model the WebKitIDL syntax off the WebIDL syntax.
> > 
> > Greg suggested that we use the following syntax:
> > 
> >   [...] foo(in [TransferMap=transfers] any data, in [Optional] transfers)
> > 
> > which seems reasonable.
> 
> 1. s/TransferMap/TransferList/ (or Transfer).
> 2. No strong preference, but why the attribute on 'data', not on 'transfers'. Any precdents here?

I suggested the reverse, but Greg seemed to prefer this way.  Either is probably fine.

> 3. Need to imply optionality. I.e. for intents, data is optional, and transfers is optional. Valid combinations are (...), (...,data), (...,data, transfer). Can we express that in WebIDL?

As far as I can tell, none of this is expressible in WebIDL (i.e., http://www.w3.org/TR/WebIDL/ ).  We're discussing WebKitIDL.  If you'd like both to be optional, you can write the following:

[...] foo(in [Optional, TransferList=transfers] any data, in [Optional] transfers)

[Dmitry Lomov]

(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > > API is expressible in WebIDL
> > > 
> > > I talked this over with Greg, and he seems to think the coupling between these two parameters isn't expressible in WebIDL.  If there's a way to express it, that would be good to know so we can model the WebKitIDL syntax off the WebIDL syntax.
> > > 
> > > Greg suggested that we use the following syntax:
> > > 
> > >   [...] foo(in [TransferMap=transfers] any data, in [Optional] transfers)
> > > 
> > > which seems reasonable.
> > 
> > 1. s/TransferMap/TransferList/ (or Transfer).
> > 2. No strong preference, but why the attribute on 'data', not on 'transfers'. Any precdents here?
> 
> I suggested the reverse, but Greg seemed to prefer this way.  Either is probably fine.

Ok.

> 
> > 3. Need to imply optionality. I.e. for intents, data is optional, and transfers is optional. Valid combinations are (...), (...,data), (...,data, transfer). Can we express that in WebIDL?
> 
> As far as I can tell, none of this is expressible in WebIDL (i.e., http://www.w3.org/TR/WebIDL/ ).  We're discussing WebKitIDL.  

Right, sorry that is what I meant.

> If you'd like both to be optional, you can write the following:
> 
> [...] foo(in [Optional, TransferList=transfers] any data, in [Optional] transfers)

TransferList should imply the correct "optionality" for transfers. (Maybe it happens automatically with Optional now though?)

[Greg Billock]

Created attachment 133932 [details]
Patch

[Dmitry Lomov]

Comment on attachment 133932 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133932&action=review

Comments below

> Source/WebCore/Modules/intents/Intent.cpp:51
> +    fprintf(stderr, "Data is %s\n", data->toWireString().utf8().data());

Stale debug print

> Source/WebCore/Modules/intents/Intent.cpp:67
> +    fprintf(stderr, "Data is %s\n", data->toWireString().utf8().data());

Stale debug print

> Source/WebCore/Modules/intents/Intent.cpp:91
> +        m_data = SerializedScriptValue::createFromWire(data->toWireString());

No need for toWire/fromWire conversion. Just assign m_data = data

[Greg Billock]

OK, back from the W3C meeting now. I've updated this CL to use the new codegen (and fixed the include stanza there). Compiles now! So that should be happy news for codegen.

I was going to make the updates on https://trac.webkit.org/wiki/WebKitIDL, but it looks like someone with more permissions needs to add the new "TransferList" IDL modifier. (Or get me set up with the right access.)

Filed https://bugs.webkit.org/show_bug.cgi?id=82264 for JSC support for this modifier. Shall I file another for refitting existing postMessage implementation to use it? I suppose we should wait for JSC support, right?

[Adam Barth]

> I was going to make the updates on https://trac.webkit.org/wiki/WebKitIDL, but it looks like someone with more permissions needs to add the new "TransferList" IDL modifier. (Or get me set up with the right access.)

If you email me the text, I'm happy to add it for you.

> Filed https://bugs.webkit.org/show_bug.cgi?id=82264 for JSC support for this modifier. Shall I file another for refitting existing postMessage implementation to use it?

Sure.

> I suppose we should wait for JSC support, right?

Yeah, you probably want to mark the JSC bug as blocking the postMessage bug.

[WebKit Review Bot]

Comment on attachment 133932 [details]
Patch

Attachment 133932 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12142260

[Greg Billock]

Comment on attachment 133932 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133932&action=review

>> Source/WebCore/Modules/intents/Intent.cpp:51
>> +    fprintf(stderr, "Data is %s\n", data->toWireString().utf8().data());
> 
> Stale debug print

Removed

>> Source/WebCore/Modules/intents/Intent.cpp:67
>> +    fprintf(stderr, "Data is %s\n", data->toWireString().utf8().data());
> 
> Stale debug print

Removed

>> Source/WebCore/Modules/intents/Intent.cpp:91
>> +        m_data = SerializedScriptValue::createFromWire(data->toWireString());
> 
> No need for toWire/fromWire conversion. Just assign m_data = data

Done

[Greg Billock]

(In reply to comment #31)
> > I was going to make the updates on https://trac.webkit.org/wiki/WebKitIDL, but it looks like someone with more permissions needs to add the new "TransferList" IDL modifier. (Or get me set up with the right access.)
> 
> If you email me the text, I'm happy to add it for you.

Will do off-thread.

> 
> > Filed https://bugs.webkit.org/show_bug.cgi?id=82264 for JSC support for this modifier. Shall I file another for refitting existing postMessage implementation to use it?
> 
> Sure.
> 
> > I suppose we should wait for JSC support, right?
> 
> Yeah, you probably want to mark the JSC bug as blocking the postMessage bug.

https://bugs.webkit.org/show_bug.cgi?id=82356

[Greg Billock]

Created attachment 134102 [details]
Patch

[Dmitry Lomov]

Comment on attachment 134102 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134102&action=review

What are your plans for testing this?

> Source/WebCore/Modules/intents/Intent.cpp:67
> +    OwnPtr<MessagePortArray> entangledPorts = MessagePort::entanglePorts(*context, channels.release());

This is wrong (sorry for missing this in my previous review).
You want to entanglePorts _in the context of the reciever_, not in the context of sender.

The right thing to do would be to keep the disentangled MessagePortChannelArray in the Intent class, and  entangle ports on the receiving side. 
See DOMWindow.cpp for an example.

[Greg Billock]

Comment on attachment 134102 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134102&action=review

>> Source/WebCore/Modules/intents/Intent.cpp:67
>> +    OwnPtr<MessagePortArray> entangledPorts = MessagePort::entanglePorts(*context, channels.release());
> 
> This is wrong (sorry for missing this in my previous review).
> You want to entanglePorts _in the context of the reciever_, not in the context of sender.
> 
> The right thing to do would be to keep the disentangled MessagePortChannelArray in the Intent class, and  entangle ports on the receiving side. 
> See DOMWindow.cpp for an example.

Yeah, this is confusing to me. In DOMWindow.cpp:795 the disentanglePorts call gets made as part of postMessage(). What I thought I understood off-line, and read in DOMWindow.cpp, was that this MessagePortChannelArray then gets passed to the PostMessageTimer, which upon execution proceeds to call entanglePorts (line 126) as it creates the event to be dispatched. Or am I misreading this, and this timer is actually in the receiving DOMWindow? I was assuming there was some safety reason for the first disentangle having to do with lazy initialization or passing ports through workers or some such why it was needed.

[Dmitry Lomov]

(In reply to comment #37)
> (From update of attachment 134102 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134102&action=review
> 
> >> Source/WebCore/Modules/intents/Intent.cpp:67
> >> +    OwnPtr<MessagePortArray> entangledPorts = MessagePort::entanglePorts(*context, channels.release());
> > 
> > This is wrong (sorry for missing this in my previous review).
> > You want to entanglePorts _in the context of the reciever_, not in the context of sender.
> > 
> > The right thing to do would be to keep the disentangled MessagePortChannelArray in the Intent class, and  entangle ports on the receiving side. 
> > See DOMWindow.cpp for an example.
> 
> Yeah, this is confusing to me. In DOMWindow.cpp:795 the disentanglePorts call gets made as part of postMessage(). What I thought I understood off-line, and read in DOMWindow.cpp, was that this MessagePortChannelArray then gets passed to the PostMessageTimer, which upon execution proceeds to call entanglePorts (line 126) as it creates the event to be dispatched. Or am I misreading this, and this timer is actually in the receiving DOMWindow? 

The call on line 126 is in PostMessageTimer::event, which happens when the message gets received. Note how the context is passed into PostMessageTimer::event.

[Greg Billock]

(In reply to comment #38)
> (In reply to comment #37)
> > (From update of attachment 134102 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=134102&action=review
> > 
> > >> Source/WebCore/Modules/intents/Intent.cpp:67
> > >> +    OwnPtr<MessagePortArray> entangledPorts = MessagePort::entanglePorts(*context, channels.release());
> > > 
> > > This is wrong (sorry for missing this in my previous review).
> > > You want to entanglePorts _in the context of the reciever_, not in the context of sender.
> > > 
> > > The right thing to do would be to keep the disentangled MessagePortChannelArray in the Intent class, and  entangle ports on the receiving side. 
> > > See DOMWindow.cpp for an example.
> > 
> > Yeah, this is confusing to me. In DOMWindow.cpp:795 the disentanglePorts call gets made as part of postMessage(). What I thought I understood off-line, and read in DOMWindow.cpp, was that this MessagePortChannelArray then gets passed to the PostMessageTimer, which upon execution proceeds to call entanglePorts (line 126) as it creates the event to be dispatched. Or am I misreading this, and this timer is actually in the receiving DOMWindow? 
> 
> The call on line 126 is in PostMessageTimer::event, which happens when the message gets received. Note how the context is passed into PostMessageTimer::event.

Got it. I was reading the code in postMessage like this:

PostMessageTimer* timer = new PostMessageTimer(this, message, sourceOrigin, source, channels.release(), target.get(), stackTrace.release());

Gets the disentangled ports

    timer->startOneShot(0);

Runs the timer, causes postMessageTimerFired in that same window, which (line 822) does:
   RefPtr<MessageEvent> event = timer->event(document());

which then calls entanglePorts, and then the event is dispatched to the destination at the bottom of postMessageTimerFired. But I take it that's not right and the timer must fire on the destination window? (Just want to make sure I have the right mental model.)

[Dmitry Lomov]

(In reply to comment #39)
> (In reply to comment #38)
> > (In reply to comment #37)
> > > (From update of attachment 134102 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=134102&action=review
> > > 
> > > >> Source/WebCore/Modules/intents/Intent.cpp:67
> > > >> +    OwnPtr<MessagePortArray> entangledPorts = MessagePort::entanglePorts(*context, channels.release());
> > > > 
> > > > This is wrong (sorry for missing this in my previous review).
> > > > You want to entanglePorts _in the context of the reciever_, not in the context of sender.
> > > > 
> > > > The right thing to do would be to keep the disentangled MessagePortChannelArray in the Intent class, and  entangle ports on the receiving side. 
> > > > See DOMWindow.cpp for an example.
> > > 
> > > Yeah, this is confusing to me. In DOMWindow.cpp:795 the disentanglePorts call gets made as part of postMessage(). What I thought I understood off-line, and read in DOMWindow.cpp, was that this MessagePortChannelArray then gets passed to the PostMessageTimer, which upon execution proceeds to call entanglePorts (line 126) as it creates the event to be dispatched. Or am I misreading this, and this timer is actually in the receiving DOMWindow? 
> > 
> > The call on line 126 is in PostMessageTimer::event, which happens when the message gets received. Note how the context is passed into PostMessageTimer::event.
> 
> Got it. I was reading the code in postMessage like this:
> 
> PostMessageTimer* timer = new PostMessageTimer(this, message, sourceOrigin, source, channels.release(), target.get(), stackTrace.release());
> 
> Gets the disentangled ports
> 
>     timer->startOneShot(0);
> 
> Runs the timer, causes postMessageTimerFired in that same window, which (line 822) does:
>    RefPtr<MessageEvent> event = timer->event(document());
> 
> which then calls entanglePorts, and then the event is dispatched to the destination at the bottom of postMessageTimerFired. But I take it that's not right and the timer must fire on the destination window? (Just want to make sure I have the right mental model.)

Ok let me try to make in clear: so you are in some context. say inside a my-awesome-window.html.
That page is shown in some window, call it awesomeWindow. The javascript executing in my-awesome-window.html gets a hold on *another* window, let us call it destinationWindow. When javascript executing in my-awesome-window.html calls:
destinationWindow.postMessage(...)
the context in which this invocation happens is the context associated with awesomeWindow, i.e. with a my-awesome-window.html.

Ok now the response to that message needs to happen in destinationWindow's context, right? In terms of DOMWindow.cpp code, destinationWindow is 'this'. Henceforth what happens is that DOMWindow::postMessage (executing in the context of awesomeWindow) starts a timer, and a timer fires immediately (postMessageTimerFired) and uses *destinationWindow* context, a.k.a. this->doucment() to entangle ports (line 822):
   RefPtr<MessageEvent> event = timer->event(document());

Does this make sense?

[Greg Billock]

Created attachment 134132 [details]
Patch

[Greg Billock]

(In reply to comment #40)

Thanks! Sounds like I have the right image, then. (I uploaded a new patch.)

[Dmitry Lomov]

Comment on attachment 134132 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134132&action=review

Looks good to me, with a nit (below).
What are the plans for testing this? Will you have a layout test with transfer?

> Source/WebCore/Modules/intents/Intent.idl:29
> +      Constructor(in DOMString action, in DOMString type, in [Optional=DefaultIsNullString, TransferList=transferMap] SerializedScriptValue data, in [Optional=DefaultIsUndefined] Array transferMap),

s/transferMap/transferList

[Adam Barth]

Comment on attachment 134132 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134132&action=review

This looks good, but please add a test that calls these new constructors.

> Source/WebCore/ChangeLog:7
> +        Add transfer map argument to Intent constructor
> +        https://bugs.webkit.org/show_bug.cgi?id=80200
> +
> +        Reviewed by NOBODY (OOPS!).
> +

Can you add more information to this ChangeLog?  There was a thread on webkit-dev recently requesting that folks put more information in their ChangeLogs.  For example, you might add a link to the line of the spec you're implementing and/or explain why adding transfer map argument to Intent constructor is useful.

> Source/WebCore/Modules/intents/Intent.cpp:66
> +    OwnPtr<MessagePortChannelArray> channels = MessagePort::disentanglePorts(&ports, ec);

I'm slightly unsure about this line, but Comment #36 seems to indiciate that this is correct.

[Greg Billock]

Created attachment 134338 [details]
Patch

[Greg Billock]

Now with test and chromium API accessor. I'm a bit unsure about that, BTW. The only way I could find to really return something meaningful ends up calling webChannelRelease. I'm not sure what the implications of that are. Does the caller then own the channel, and needs to maintain it? If I get the message port IDs out, can I then delete those references and the ports will still function? I'm not sure what the refcounting semantics are for these objects... Perhaps I need to hang on to a ref in the IntentRequest object as well, if Intent is going to need to release them?)

[Adam Barth]

Comment on attachment 134338 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134338&action=review

> Source/WebCore/ChangeLog:8
> +	http://dvcs.w3.org/hg/web-intents/raw-file/tip/spec/Overview.html
> +	This adds the ability to pass transferables (i.e. MessagePorts)
> +	through web intents, and puts the calling convention in line
> +	with the Web Messaging spec:
> +	http://dev.w3.org/html5/postmsg/

Looks like you've got some indentation problems here.

> Source/WebCore/Modules/intents/Intent.h:61
> +    // Make an accessor for PlatformMessagePortChannel instead?

Technically we prefer comments to be complete sentences.

> Source/WebKit/chromium/public/WebIntent.h:69
> +    // Caller takes ownership of the ports.
> +    WEBKIT_EXPORT WebMessagePortChannelArray* ports() const;

Should we pick a name here that makes it more clear when reading the caller that it needs to take ownership of the ports?  I'm not sure if there's already a convention for such names in the WebKit API.  As written, this seems likely to lead to memory leaks.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 134338 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/webintents/web-intents-invoke-..." exit_code: 1
Source/WebCore/ChangeLog:4:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 5 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dmitry Lomov]

Comment on attachment 134338 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134338&action=review

A few more comments.
Darin Fisher will need to look at changes in WebKit/chromium/public/

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:1325
> +        printf("Have %zu ports\n", ports->size());

Debug print.

> LayoutTests/webintents/web-intents-invoke-port.html:12
> +        navigator.startActivity(new Intent("action1", "mime/type1", channel.port1, [channel.port1]));

Tests for exceptional behavior (see LayoutTests/fast/dom/Window/window-postmessage-args.html for some inspiration)

[Adam Barth]

> I'm a bit unsure about that, BTW.

Maybe it would be easier to just expose the number of ports?  Are you planning to use this for anything beyond testing?

[Dmitry Lomov]

(In reply to comment #50)
> (From update of attachment 134338 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134338&action=review
> 
> A few more comments.
> Darin Fisher will need to look at changes in WebKit/chromium/public/

Oh ok it looks like it is not Darin only anymore :) (a welcome change!)

> 
> > Tools/DumpRenderTree/chromium/WebViewHost.cpp:1325
> > +        printf("Have %zu ports\n", ports->size());
> 
> Debug print.
> 
> > LayoutTests/webintents/web-intents-invoke-port.html:12
> > +        navigator.startActivity(new Intent("action1", "mime/type1", channel.port1, [channel.port1]));
> 
> Tests for exceptional behavior (see LayoutTests/fast/dom/Window/window-postmessage-args.html for some inspiration)

[Greg Billock]

Created attachment 134343 [details]
Patch

[Greg Billock]

Comment on attachment 134338 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134338&action=review

>>> Source/WebCore/ChangeLog:8
>>> +	http://dev.w3.org/html5/postmsg/
>> 
>> Looks like you've got some indentation problems here.
> 
> Line contains tab character.  [whitespace/tab] [5]

Yeah. Fixed up. :-/

>> Source/WebCore/Modules/intents/Intent.h:61
>> +    // Make an accessor for PlatformMessagePortChannel instead?
> 
> Technically we prefer comments to be complete sentences.

Yeah, this is a plea for help more than something intended to make it to checkin. :-) I'm not sure what to do about exposing these ports; I don't think I understand the ownership and implications of the port vs. channel vs. platform well enough to know what to do. In the Chromium API, I basically need to expose the platform channel in order to get the port IDs to pass through the IPC system. The only way I can see to do that involves webChannelRelease in the platform code. I don't have a clear sense as to what that really means, and what the implications are for which references I need to keep tucked away here in WebCore to make sure everything works.

[Greg Billock]

(In reply to comment #51)
> > I'm a bit unsure about that, BTW.
> 
> Maybe it would be easier to just expose the number of ports?  Are you planning to use this for anything beyond testing?

When this gets into the chromium-side code, I'll need to do the WebMessagePortChannelImpl thing and get the port ID out to send through the IPC  to the browser and then service context. I played about with exposing that API down here, but Dmitry talked me out of it. :-) I'm left not really knowing what's going on with the platform message channels, though, and if this is the right way to expose the API, whether that has implications for WebCore I need to account for.

[Adam Barth]

@dslomov: Any insights as to what we should do here?  Did we used to do something similar when workers were out of process?  If so, should we copy the pattern from that code?

[Dmitry Lomov]

(In reply to comment #56)
> @dslomov: Any insights as to what we should do here?  Did we used to do something similar when workers were out of process?  If so, should we copy the pattern from that code?

The model code to look at is chromium MessagePort implementation. I can have a look later today.

[Greg Billock]

(In reply to comment #50)
> (From update of attachment 134338 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134338&action=review
> 
> A few more comments.
> Darin Fisher will need to look at changes in WebKit/chromium/public/
> 
> > Tools/DumpRenderTree/chromium/WebViewHost.cpp:1325
> > +        printf("Have %zu ports\n", ports->size());
> 
> Debug print.
> 
> > LayoutTests/webintents/web-intents-invoke-port.html:12
> > +        navigator.startActivity(new Intent("action1", "mime/type1", channel.port1, [channel.port1]));
> 
> Tests for exceptional behavior (see LayoutTests/fast/dom/Window/window-postmessage-args.html for some inspiration)

The exceptional tests there (shouldFail) are for unsupported data types, it looks like to me. Did you have any particular port failure cases in mind? Or do you mean the neutered ArrayBuffer?

WRT model MessagePort code, I'm not sure I'm looking in the right place. All the chromium API does is access WebMessagPortChannel (which is purely an interface) and PlatformMessagePortChannel (which is what I'm trying to expose correctly in the CL).

On the chromium side, the code only uses the WebMessagePortChannelImpl class. It grabs the IDs out of there and sends the message on, but I'm not sure of the protocol on the receiving end of how to actually hook it up after that.

[Dmitry Lomov]

(In reply to comment #58)
> (In reply to comment #50)
> > (From update of attachment 134338 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=134338&action=review
> > 
> > A few more comments.
> > Darin Fisher will need to look at changes in WebKit/chromium/public/
> > 
> > > Tools/DumpRenderTree/chromium/WebViewHost.cpp:1325
> > > +        printf("Have %zu ports\n", ports->size());
> > 
> > Debug print.

Sorry for a bad comment here, I somehow missed that it is in DumpRenderTree
> > 
> > > LayoutTests/webintents/web-intents-invoke-port.html:12
> > > +        navigator.startActivity(new Intent("action1", "mime/type1", channel.port1, [channel.port1]));
> > 
> > Tests for exceptional behavior (see LayoutTests/fast/dom/Window/window-postmessage-args.html for some inspiration)
> 
> The exceptional tests there (shouldFail) are for unsupported data types, it looks like to me. Did you have any particular port failure cases in mind? Or do you mean the neutered ArrayBuffer?
>

Just unsupported data types, to make sure the exception is propagated correctly (I think it is, looking at the code, but it's nice to make sure)
 
> WRT model MessagePort code, I'm not sure I'm looking in the right place. All the chromium API does is access WebMessagPortChannel (which is purely an interface) and PlatformMessagePortChannel (which is what I'm trying to expose correctly in the CL).
> 
> On the chromium side, the code only uses the WebMessagePortChannelImpl class. It grabs the IDs out of there and sends the message on, but I'm not sure of the protocol on the receiving end of how to actually hook it up after that.

tl;dr: This is a reasonable interface for WebIntent, although I suggets renaming ports to 

Alright, let's focus on what you actually need to do :)
The key thing that Intent class provides (AFAIU) is a package of "stuff" that needs to be transferred to the intent handler, possibly over to the other side of IPC boundary. The similar code currently exists in PlatformMessagePortChannel::postMessageToRemote in Source/WebKit/chromium/src/PlatformMessagePortChannel.cpp. (MessagePortChannel::EventData is in fact a pretty similar package of SSV+MessagePortChannels). You will have to write similar code somewhere where intents are already being IPCd over. 

What happens in PlatformMessagePortChannel::postMessageToRemote is that WebCore data types are being transferred into chromium-port-specific datatypes, namely SSV goes to WebString (=wire string) and each MessagePortChannel becomes and WebMessagePortChannel. So the transformations is from SSV+MessagePortChannelArray to WebString+WebMessagePortChannelArray. The latter pair is the suitable message to send over the IPC. 

The API on WebIntent should facilitate doing the same. The user of WebIntent class should be able to obtain a suitable WebString+WebMessagePortChannelArray pair out of it to send down the wire. So the combination of data() + ports() methods on WebIntent looks right. Maybe rename 'ports' into 'messagePortChannels' though.

[Greg Billock]

Comment on attachment 134338 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134338&action=review

>> Source/WebKit/chromium/public/WebIntent.h:69
>> +    WEBKIT_EXPORT WebMessagePortChannelArray* ports() const;
> 
> Should we pick a name here that makes it more clear when reading the caller that it needs to take ownership of the ports?  I'm not sure if there's already a convention for such names in the WebKit API.  As written, this seems likely to lead to memory leaks.

Renamed to messagePortChannels, but yes, a name like "adoptMessagePortChannelArrays" is what we want. In PlatformMessagePortChannel it has a "release" suffix. Shall I use that?

>>> LayoutTests/webintents/web-intents-invoke-port.html:12
>>> +        navigator.startActivity(new Intent("action1", "mime/type1", channel.port1, [channel.port1]));
>> 
>> Tests for exceptional behavior (see LayoutTests/fast/dom/Window/window-postmessage-args.html for some inspiration)
> 
> Just unsupported data types, to make sure the exception is propagated correctly (I think it is, looking at the code, but it's nice to make sure)

OK, Added.

[Adam Barth]

(In reply to comment #60)
> (From update of attachment 134338 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134338&action=review
> 
> >> Source/WebKit/chromium/public/WebIntent.h:69
> >> +    WEBKIT_EXPORT WebMessagePortChannelArray* ports() const;
> > 
> > Should we pick a name here that makes it more clear when reading the caller that it needs to take ownership of the ports?  I'm not sure if there's already a convention for such names in the WebKit API.  As written, this seems likely to lead to memory leaks.
> 
> Renamed to messagePortChannels, but yes, a name like "adoptMessagePortChannelArrays" is what we want. In PlatformMessagePortChannel it has a "release" suffix. Shall I use that?

Release can be a good word to indicate transfer of ownership.

[Greg Billock]

Created attachment 135151 [details]
Patch

[Greg Billock]

OK, The impl in WebIntent is now very parallel to the PlatformMessagePortChannel code (and references it; perhaps that'll help any changes made there propagate).

I used Dmitry's suggested API method name in this patch, but I agree it might be error-prone. Shall I change it to *release?

(In reply to comment #59)
> (In reply to comment #58)
> > (In reply to comment #50)
> > > (From update of attachment 134338 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=134338&action=review
> > > 
> > > A few more comments.
> > > Darin Fisher will need to look at changes in WebKit/chromium/public/
> > > 
> > > > Tools/DumpRenderTree/chromium/WebViewHost.cpp:1325
> > > > +        printf("Have %zu ports\n", ports->size());
> > > 
> > > Debug print.
> 
> Sorry for a bad comment here, I somehow missed that it is in DumpRenderTree
> > > 
> > > > LayoutTests/webintents/web-intents-invoke-port.html:12
> > > > +        navigator.startActivity(new Intent("action1", "mime/type1", channel.port1, [channel.port1]));
> > > 
> > > Tests for exceptional behavior (see LayoutTests/fast/dom/Window/window-postmessage-args.html for some inspiration)
> > 
> > The exceptional tests there (shouldFail) are for unsupported data types, it looks like to me. Did you have any particular port failure cases in mind? Or do you mean the neutered ArrayBuffer?
> >
> 
> Just unsupported data types, to make sure the exception is propagated correctly (I think it is, looking at the code, but it's nice to make sure)
> 
> > WRT model MessagePort code, I'm not sure I'm looking in the right place. All the chromium API does is access WebMessagPortChannel (which is purely an interface) and PlatformMessagePortChannel (which is what I'm trying to expose correctly in the CL).
> > 
> > On the chromium side, the code only uses the WebMessagePortChannelImpl class. It grabs the IDs out of there and sends the message on, but I'm not sure of the protocol on the receiving end of how to actually hook it up after that.
> 
> tl;dr: This is a reasonable interface for WebIntent, although I suggets renaming ports to 
> 
> Alright, let's focus on what you actually need to do :)
> The key thing that Intent class provides (AFAIU) is a package of "stuff" that needs to be transferred to the intent handler, possibly over to the other side of IPC boundary. The similar code currently exists in PlatformMessagePortChannel::postMessageToRemote in Source/WebKit/chromium/src/PlatformMessagePortChannel.cpp. (MessagePortChannel::EventData is in fact a pretty similar package of SSV+MessagePortChannels). You will have to write similar code somewhere where intents are already being IPCd over. 
> 
> What happens in PlatformMessagePortChannel::postMessageToRemote is that WebCore data types are being transferred into chromium-port-specific datatypes, namely SSV goes to WebString (=wire string) and each MessagePortChannel becomes and WebMessagePortChannel. So the transformations is from SSV+MessagePortChannelArray to WebString+WebMessagePortChannelArray. The latter pair is the suitable message to send over the IPC. 
> 
> The API on WebIntent should facilitate doing the same. The user of WebIntent class should be able to obtain a suitable WebString+WebMessagePortChannelArray pair out of it to send down the wire. So the combination of data() + ports() methods on WebIntent looks right. Maybe rename 'ports' into 'messagePortChannels' though.

[Greg Billock]

Created attachment 135932 [details]
Patch

[Greg Billock]

(In reply to comment #61)
> (In reply to comment #60)
> > (From update of attachment 134338 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=134338&action=review
> > 
> > >> Source/WebKit/chromium/public/WebIntent.h:69
> > >> +    WEBKIT_EXPORT WebMessagePortChannelArray* ports() const;
> > > 
> > > Should we pick a name here that makes it more clear when reading the caller that it needs to take ownership of the ports?  I'm not sure if there's already a convention for such names in the WebKit API.  As written, this seems likely to lead to memory leaks.
> > 
> > Renamed to messagePortChannels, but yes, a name like "adoptMessagePortChannelArrays" is what we want. In PlatformMessagePortChannel it has a "release" suffix. Shall I use that?
> 
> Release can be a good word to indicate transfer of ownership.

Done

[Dmitry Lomov]

Comment on attachment 135932 [details]
Patch

LGTM

[Kentaro Hara]

Comment on attachment 135932 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135932&action=review

> Source/WebCore/Modules/intents/Intent.cpp:36
> +#include "ScriptExecutionContext.h"

Remove this

> Source/WebCore/Modules/intents/Intent.cpp:41
> +PassRefPtr<Intent> Intent::create(ScriptExecutionContext* context, const String& action, const String& type, PassRefPtr<SerializedScriptValue> data, ExceptionCode& ec)

Why do you need ScriptExecutionContext here? It seems it is not used.

> Source/WebCore/Modules/intents/Intent.cpp:55
> +PassRefPtr<Intent> Intent::create(ScriptExecutionContext* context, const String& action, const String& type, PassRefPtr<SerializedScriptValue> data, const MessagePortArray& ports, ExceptionCode& ec)

Ditto.

> Source/WebCore/Modules/intents/Intent.h:44
> +class ScriptExecutionContext;

Ditto.

> Source/WebCore/Modules/intents/Intent.h:52
> +    static PassRefPtr<Intent> create(ScriptExecutionContext*, const String& action, const String& type, PassRefPtr<SerializedScriptValue> data, ExceptionCode&);
> +    static PassRefPtr<Intent> create(ScriptExecutionContext*, const String& action, const String& type, PassRefPtr<SerializedScriptValue> data, const MessagePortArray& ports, ExceptionCode&);

Ditto.

> Source/WebCore/Modules/intents/Intent.idl:30
> +      CallWith=ScriptExecutionContext,

Remove this.

[Greg Billock]

Comment on attachment 135932 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135932&action=review

>> Source/WebCore/Modules/intents/Intent.cpp:41
>> +PassRefPtr<Intent> Intent::create(ScriptExecutionContext* context, const String& action, const String& type, PassRefPtr<SerializedScriptValue> data, ExceptionCode& ec)
> 
> Why do you need ScriptExecutionContext here? It seems it is not used.

You're totally right. This got fossilized pre-codegen and never removed. Thanks for the catch.

[Greg Billock]

Created attachment 136031 [details]
Patch

[Kentaro Hara]

Comment on attachment 136031 [details]
Patch

r+ for the IDL and binding change. I am not familiar with the Intent part.

[Adam Barth]

Comment on attachment 136031 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136031&action=review

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:1334
> +    delete ports;

This cleans up all the channels in the array?

[Greg Billock]

Comment on attachment 136031 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136031&action=review

>> Tools/DumpRenderTree/chromium/WebViewHost.cpp:1334
>> +    delete ports;
> 
> This cleans up all the channels in the array?

I believe so. WebVector::destroy destructs the contents, which ends up in this case calling through to content::~WebMessagePortChannelImpl. There's not really any other artifacts around to destroy, but if I'm missing something, please let me know. Dmitry?

[Dmitry Lomov]

Comment on attachment 136031 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136031&action=review

>>> Tools/DumpRenderTree/chromium/WebViewHost.cpp:1334
>>> +    delete ports;
>> 
>> This cleans up all the channels in the array?
> 
> I believe so. WebVector::destroy destructs the contents, which ends up in this case calling through to content::~WebMessagePortChannelImpl. There's not really any other artifacts around to destroy, but if I'm missing something, please let me know. Dmitry?

WebMessagePortChannelArray is 
   typedef WebVector<class WebMessagePortChannel*> WebMessagePortChannelArray;
i.e. an array of _pointers_ to WebMessagePortChannel. Destroying a vector of pointers does not delete them (does not call the destructors of pointees, only the destructors of pointers themsleves).
Good catch Adam!

[Greg Billock]

Comment on attachment 136031 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136031&action=review

>>>> Tools/DumpRenderTree/chromium/WebViewHost.cpp:1334
>>>> +    delete ports;
>>> 
>>> This cleans up all the channels in the array?
>> 
>> I believe so. WebVector::destroy destructs the contents, which ends up in this case calling through to content::~WebMessagePortChannelImpl. There's not really any other artifacts around to destroy, but if I'm missing something, please let me know. Dmitry?
> 
> WebMessagePortChannelArray is 
>    typedef WebVector<class WebMessagePortChannel*> WebMessagePortChannelArray;
> i.e. an array of _pointers_ to WebMessagePortChannel. Destroying a vector of pointers does not delete them (does not call the destructors of pointees, only the destructors of pointers themsleves).
> Good catch Adam!

Ah, ok. So this really needs to walk the vector and destroy each port?

[Dmitry Lomov]

(In reply to comment #74)
> (From update of attachment 136031 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136031&action=review
> 
> >>>> Tools/DumpRenderTree/chromium/WebViewHost.cpp:1334
> >>>> +    delete ports;
> >>> 
> >>> This cleans up all the channels in the array?
> >> 
> >> I believe so. WebVector::destroy destructs the contents, which ends up in this case calling through to content::~WebMessagePortChannelImpl. There's not really any other artifacts around to destroy, but if I'm missing something, please let me know. Dmitry?
> > 
> > WebMessagePortChannelArray is 
> >    typedef WebVector<class WebMessagePortChannel*> WebMessagePortChannelArray;
> > i.e. an array of _pointers_ to WebMessagePortChannel. Destroying a vector of pointers does not delete them (does not call the destructors of pointees, only the destructors of pointers themsleves).
> > Good catch Adam!
> 
> Ah, ok. So this really needs to walk the vector and destroy each port?

Yes :(
(I wish we used RefPtrs in chromium...)

[Greg Billock]

Created attachment 136068 [details]
Patch

[Greg Billock]

OK, I think this is fixed up. :-/

(In reply to comment #75)
> (In reply to comment #74)
> > (From update of attachment 136031 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=136031&action=review
> > 
> > >>>> Tools/DumpRenderTree/chromium/WebViewHost.cpp:1334
> > >>>> +    delete ports;
> > >>> 
> > >>> This cleans up all the channels in the array?
> > >> 
> > >> I believe so. WebVector::destroy destructs the contents, which ends up in this case calling through to content::~WebMessagePortChannelImpl. There's not really any other artifacts around to destroy, but if I'm missing something, please let me know. Dmitry?
> > > 
> > > WebMessagePortChannelArray is 
> > >    typedef WebVector<class WebMessagePortChannel*> WebMessagePortChannelArray;
> > > i.e. an array of _pointers_ to WebMessagePortChannel. Destroying a vector of pointers does not delete them (does not call the destructors of pointees, only the destructors of pointers themsleves).
> > > Good catch Adam!
> > 
> > Ah, ok. So this really needs to walk the vector and destroy each port?
> 
> Yes :(
> (I wish we used RefPtrs in chromium...)

[Dmitry Lomov]

Comment on attachment 136068 [details]
Patch

LGTM

[Adam Barth]

Comment on attachment 136068 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136068&action=review

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:1335
> +    WebMessagePortChannelArray* ports = request.intent().messagePortChannelsRelease();
>      m_currentRequest = request;
> +    if (ports) {
> +        printf("Have %zu ports\n", ports->size());
> +        for (size_t i = 0; i < ports->size(); ++i)
> +            (*ports)[i]->destroy();

I'm still slightly worried that other clients of this function will have memory leaks, but I'm not sure what we can do about it.

[Greg Billock]

Comment on attachment 136068 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136068&action=review

>> Tools/DumpRenderTree/chromium/WebViewHost.cpp:1335
>> +            (*ports)[i]->destroy();
> 
> I'm still slightly worried that other clients of this function will have memory leaks, but I'm not sure what we can do about it.

agree on both counts. :-(

[Adam Barth]

Comment on attachment 136068 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136068&action=review

>>> Tools/DumpRenderTree/chromium/WebViewHost.cpp:1335
>>> +            (*ports)[i]->destroy();
>> 
>> I'm still slightly worried that other clients of this function will have memory leaks, but I'm not sure what we can do about it.
> 
> agree on both counts. :-(

Is WebMessagePortChannelArray just a typedef of WebArray?  Can we add a destroy() function to WebMessagePortChannelArray?  Anyway, these are things to consider for future patches.

[Dmitry Lomov]

(In reply to comment #79)
> (From update of attachment 136068 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136068&action=review
> 
> > Tools/DumpRenderTree/chromium/WebViewHost.cpp:1335
> > +    WebMessagePortChannelArray* ports = request.intent().messagePortChannelsRelease();
> >      m_currentRequest = request;
> > +    if (ports) {
> > +        printf("Have %zu ports\n", ports->size());
> > +        for (size_t i = 0; i < ports->size(); ++i)
> > +            (*ports)[i]->destroy();
> 
> I'm still slightly worried that other clients of this function will have memory leaks, but I'm not sure what we can do about it.

It is a worry, but in a typical scenario WebMessagePortChannels contained in an array get entangled with their appropriate MessagePorts and then take care of themselves (delete themselves on destroy). I.e. in a typical scenario, the code that deletes only the array but does not delete the channels will be exactly what is called for.

[Dmitry Lomov]

(In reply to comment #81)
> (From update of attachment 136068 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136068&action=review
> 
> >>> Tools/DumpRenderTree/chromium/WebViewHost.cpp:1335
> >>> +            (*ports)[i]->destroy();
> >> 
> >> I'm still slightly worried that other clients of this function will have memory leaks, but I'm not sure what we can do about it.
> > 
> > agree on both counts. :-(
> 
> Is WebMessagePortChannelArray just a typedef of WebArray?  Can we add a destroy() function to WebMessagePortChannelArray?  Anyway, these are things to consider for future patches.

Yes it is a typedef for WebVector. 
I assume we might want to have something like deleteAll:

    void WebVector<T>::deleteAll() const
    {
        for (size_t i = 0; i < m_size; i++) {
            delete *m_ptr[i];
        }
    }
Only works if T is a pointer.

However it won't help here because WebMessagePortChannel is a special kind of object that deletes itself. Its destructor is protected. 

Is 'destroy' method a typical pattern in chromium? If so, we can have a 'destroyAll' method.

[WebKit Review Bot]

Comment on attachment 136068 [details]
Patch

Clearing flags on attachment: 136068

Committed r113656: <http://trac.webkit.org/changeset/113656>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Simonsen]

FYI, this broke Chromium-win. It's late, so I'm just rolling it out. Please fix it and re-land when you get a chance.

Looks like %zu isn't supported on Windows:

--- e:\b\build\slave\Webkit_Vista\build\layout-test-results\webintents/web-intents-invoke-port-expected.txt 
+++ e:\b\build\slave\Webkit_Vista\build\layout-test-results\webintents/web-intents-invoke-port-actual.txt 
@@ -1,5 +1,5 @@
 Received Web Intent: action=action1 type=mime/type1
-Have 1 ports
+Have zu ports
 PASS successfullyParsed is true
 
 TEST COMPLETE

[Greg Billock]

Rats. Sorry, James. Will fix.

(In reply to comment #86)
> FYI, this broke Chromium-win. It's late, so I'm just rolling it out. Please fix it and re-land when you get a chance.
> 
> Looks like %zu isn't supported on Windows:
> 
> --- e:\b\build\slave\Webkit_Vista\build\layout-test-results\webintents/web-intents-invoke-port-expected.txt 
> +++ e:\b\build\slave\Webkit_Vista\build\layout-test-results\webintents/web-intents-invoke-port-actual.txt 
> @@ -1,5 +1,5 @@
>  Received Web Intent: action=action1 type=mime/type1
> -Have 1 ports
> +Have zu ports
>  PASS successfullyParsed is true
> 
>  TEST COMPLETE

[Greg Billock]

Created attachment 136533 [details]
Patch

[Greg Billock]

Added an int typecast which should be more printf-cross-platform.

(In reply to comment #87)
> Rats. Sorry, James. Will fix.
> 
> (In reply to comment #86)
> > FYI, this broke Chromium-win. It's late, so I'm just rolling it out. Please fix it and re-land when you get a chance.
> > 
> > Looks like %zu isn't supported on Windows:
> > 
> > --- e:\b\build\slave\Webkit_Vista\build\layout-test-results\webintents/web-intents-invoke-port-expected.txt 
> > +++ e:\b\build\slave\Webkit_Vista\build\layout-test-results\webintents/web-intents-invoke-port-actual.txt 
> > @@ -1,5 +1,5 @@
> >  Received Web Intent: action=action1 type=mime/type1
> > -Have 1 ports
> > +Have zu ports
> >  PASS successfullyParsed is true
> > 
> >  TEST COMPLETE

[WebKit Review Bot]

Comment on attachment 136533 [details]
Patch

Clearing flags on attachment: 136533

Committed r113799: <http://trac.webkit.org/changeset/113799>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.