Bug 80200

Summary: Add transfer map argument to Intent constructor
Product: WebKit Reporter: Greg Billock <gbillock>
Component: New BugsAssignee: Greg Billock <gbillock>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, dslomov, fishd, haraken, jamesr, japhet, ojan, simonjam, tkent, webkit.review.bot, wonsuk11.lee
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 81127, 83542    
Bug Blocks: 75123    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Greg Billock 2012-03-02 15:51:21 PST
Add transfer map argument to Intent constructor
Comment 1 Greg Billock 2012-03-02 15:53:23 PST
Created attachment 129973 [details]
Patch
Comment 2 WebKit Review Bot 2012-03-02 15:57:16 PST
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.
Comment 3 Adam Barth 2012-03-02 16:42:20 PST
You probably don't need custom bindings.
Comment 4 Kentaro Hara 2012-03-02 17:28:12 PST
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.
Comment 5 Dmitry Lomov 2012-03-02 18:43:23 PST
(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.
Comment 6 Kentaro Hara 2012-03-04 17:22:22 PST
(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
Comment 7 Dmitry Lomov 2012-03-04 21:23:11 PST
(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? :)
Comment 8 Kentaro Hara 2012-03-04 21:31:41 PST
(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.
Comment 9 Dmitry Lomov 2012-03-04 21:44:28 PST
(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.
Comment 10 Adam Barth 2012-03-04 22:48:06 PST
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.
Comment 11 Greg Billock 2012-03-05 09:36:42 PST
(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.
Comment 12 Greg Billock 2012-03-05 11:46:01 PST
Created attachment 130170 [details]
Patch
Comment 13 Greg Billock 2012-03-05 11:50:37 PST
(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.
Comment 14 WebKit Review Bot 2012-03-05 14:20:08 PST
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
Comment 15 Kentaro Hara 2012-03-05 16:40:01 PST
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.
Comment 16 Dmitry Lomov 2012-03-05 16:53:31 PST
(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.
Comment 17 Greg Billock 2012-03-12 13:22:03 PDT
Created attachment 131394 [details]
Patch
Comment 18 Adam Barth 2012-03-12 15:07:45 PDT
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.
Comment 19 Dmitry Lomov 2012-03-12 15:31:07 PDT
(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.
Comment 20 Dmitry Lomov 2012-03-12 15:32:49 PDT
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?
Comment 21 Adam Barth 2012-03-12 15:34:59 PDT
> Adam, API is expressible in WebIDL, it is similar to what happens in postMessage. 

Ok, great.
Comment 22 Greg Billock 2012-03-12 16:48:57 PDT
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.
Comment 23 Dmitry Lomov 2012-03-12 17:05:04 PDT
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
Comment 24 Adam Barth 2012-03-12 17:25:34 PDT
> 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.
Comment 25 Dmitry Lomov 2012-03-12 17:30:08 PDT
(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?
Comment 26 Adam Barth 2012-03-12 17:42:52 PDT
(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)
Comment 27 Dmitry Lomov 2012-03-12 17:49:08 PDT
(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?)
Comment 28 Greg Billock 2012-03-26 16:57:48 PDT
Created attachment 133932 [details]
Patch
Comment 29 Dmitry Lomov 2012-03-26 17:01:47 PDT
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
Comment 30 Greg Billock 2012-03-26 17:04:14 PDT
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?
Comment 31 Adam Barth 2012-03-26 17:15:09 PDT
> 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.
Comment 32 WebKit Review Bot 2012-03-26 19:59:06 PDT
Comment on attachment 133932 [details]
Patch

Attachment 133932 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12142260
Comment 33 Greg Billock 2012-03-27 10:59:44 PDT
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
Comment 34 Greg Billock 2012-03-27 11:07:06 PDT
(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
Comment 35 Greg Billock 2012-03-27 11:18:52 PDT
Created attachment 134102 [details]
Patch
Comment 36 Dmitry Lomov 2012-03-27 11:27:03 PDT
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.
Comment 37 Greg Billock 2012-03-27 11:41:32 PDT
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.
Comment 38 Dmitry Lomov 2012-03-27 11:55:09 PDT
(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.
Comment 39 Greg Billock 2012-03-27 12:12:11 PDT
(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.)
Comment 40 Dmitry Lomov 2012-03-27 13:15:14 PDT
(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?
Comment 41 Greg Billock 2012-03-27 14:11:59 PDT
Created attachment 134132 [details]
Patch
Comment 42 Greg Billock 2012-03-27 14:16:56 PDT
(In reply to comment #40)

Thanks! Sounds like I have the right image, then. (I uploaded a new patch.)
Comment 43 Dmitry Lomov 2012-03-27 14:44:32 PDT
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
Comment 44 Adam Barth 2012-03-27 14:46:10 PDT
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.
Comment 45 Greg Billock 2012-03-28 11:03:46 PDT
Created attachment 134338 [details]
Patch
Comment 46 Greg Billock 2012-03-28 11:07:23 PDT
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?)
Comment 47 Adam Barth 2012-03-28 11:08:13 PDT
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.
Comment 48 WebKit Review Bot 2012-03-28 11:08:47 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 49 WebKit Review Bot 2012-03-28 11:09:08 PDT
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.
Comment 50 Dmitry Lomov 2012-03-28 11:10:30 PDT
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)
Comment 51 Adam Barth 2012-03-28 11:10:40 PDT
> 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?
Comment 52 Dmitry Lomov 2012-03-28 11:11:52 PDT
(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)
Comment 53 Greg Billock 2012-03-28 11:14:50 PDT
Created attachment 134343 [details]
Patch
Comment 54 Greg Billock 2012-03-28 11:21:01 PDT
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.
Comment 55 Greg Billock 2012-03-28 11:25:45 PDT
(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.
Comment 56 Adam Barth 2012-03-28 13:26:14 PDT
@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?
Comment 57 Dmitry Lomov 2012-03-28 13:37:01 PDT
(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.
Comment 58 Greg Billock 2012-03-29 14:19:56 PDT
(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.
Comment 59 Dmitry Lomov 2012-03-30 10:17:34 PDT
(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.
Comment 60 Greg Billock 2012-04-02 11:39:11 PDT
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.
Comment 61 Adam Barth 2012-04-02 11:50:00 PDT
(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.
Comment 62 Greg Billock 2012-04-02 12:08:04 PDT
Created attachment 135151 [details]
Patch
Comment 63 Greg Billock 2012-04-02 12:12:10 PDT
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.
Comment 64 Greg Billock 2012-04-05 16:30:47 PDT
Created attachment 135932 [details]
Patch
Comment 65 Greg Billock 2012-04-05 16:31:19 PDT
(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
Comment 66 Dmitry Lomov 2012-04-05 16:36:25 PDT
Comment on attachment 135932 [details]
Patch

LGTM
Comment 67 Kentaro Hara 2012-04-05 23:20:19 PDT
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.
Comment 68 Greg Billock 2012-04-05 23:42:42 PDT
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.
Comment 69 Greg Billock 2012-04-06 09:09:51 PDT
Created attachment 136031 [details]
Patch
Comment 70 Kentaro Hara 2012-04-06 10:08:31 PDT
Comment on attachment 136031 [details]
Patch

r+ for the IDL and binding change. I am not familiar with the Intent part.
Comment 71 Adam Barth 2012-04-06 13:14:40 PDT
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?
Comment 72 Greg Billock 2012-04-06 13:23:19 PDT
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?
Comment 73 Dmitry Lomov 2012-04-06 13:28:26 PDT
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!
Comment 74 Greg Billock 2012-04-06 13:37:41 PDT
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?
Comment 75 Dmitry Lomov 2012-04-06 13:41:00 PDT
(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...)
Comment 76 Greg Billock 2012-04-06 14:50:35 PDT
Created attachment 136068 [details]
Patch
Comment 77 Greg Billock 2012-04-06 14:52:04 PDT
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...)
Comment 78 Dmitry Lomov 2012-04-06 14:55:30 PDT
Comment on attachment 136068 [details]
Patch

LGTM
Comment 79 Adam Barth 2012-04-06 15:04:55 PDT
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.
Comment 80 Greg Billock 2012-04-06 15:06:22 PDT
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. :-(
Comment 81 Adam Barth 2012-04-06 15:10:38 PDT
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.
Comment 82 Dmitry Lomov 2012-04-06 15:18:14 PDT
(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.
Comment 83 Dmitry Lomov 2012-04-06 15:26:27 PDT
(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.
Comment 84 WebKit Review Bot 2012-04-09 18:46:58 PDT
Comment on attachment 136068 [details]
Patch

Clearing flags on attachment: 136068

Committed r113656: <http://trac.webkit.org/changeset/113656>
Comment 85 WebKit Review Bot 2012-04-09 18:47:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 86 James Simonsen 2012-04-09 20:26:09 PDT
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
Comment 87 Greg Billock 2012-04-09 21:11:29 PDT
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
Comment 88 Greg Billock 2012-04-10 13:52:47 PDT
Created attachment 136533 [details]
Patch
Comment 89 Greg Billock 2012-04-10 13:54:28 PDT
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
Comment 90 WebKit Review Bot 2012-04-10 18:15:45 PDT
Comment on attachment 136533 [details]
Patch

Clearing flags on attachment: 136533

Committed r113799: <http://trac.webkit.org/changeset/113799>
Comment 91 WebKit Review Bot 2012-04-10 18:16:17 PDT
All reviewed patches have been landed.  Closing bug.