Add transfer map argument to Intent constructor
Created attachment 129973 [details] Patch
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.
You probably don't need custom bindings.
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.
(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.
(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
(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? :)
(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.
(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.
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.
(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.
Created attachment 130170 [details] Patch
(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 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 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.
(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.
Created attachment 131394 [details] Patch
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.
(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 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, API is expressible in WebIDL, it is similar to what happens in postMessage. Ok, great.
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 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
> 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.
(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?
(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)
(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?)
Created attachment 133932 [details] Patch
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
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?
> 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 on attachment 133932 [details] Patch Attachment 133932 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12142260
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
(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
Created attachment 134102 [details] Patch
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 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.
(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.
(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.)
(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?
Created attachment 134132 [details] Patch
(In reply to comment #40) Thanks! Sounds like I have the right image, then. (I uploaded a new patch.)
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 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.
Created attachment 134338 [details] Patch
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 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.
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.
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 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)
> 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?
(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)
Created attachment 134343 [details] Patch
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.
(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.
@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?
(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.
(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.
(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 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.
(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.
Created attachment 135151 [details] Patch
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.
Created attachment 135932 [details] Patch
(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 on attachment 135932 [details] Patch LGTM
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 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.
Created attachment 136031 [details] Patch
Comment on attachment 136031 [details] Patch r+ for the IDL and binding change. I am not familiar with the Intent part.
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 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 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 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?
(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...)
Created attachment 136068 [details] Patch
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 on attachment 136068 [details] Patch LGTM
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 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 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.
(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.
(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 on attachment 136068 [details] Patch Clearing flags on attachment: 136068 Committed r113656: <http://trac.webkit.org/changeset/113656>
All reviewed patches have been landed. Closing bug.
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
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
Created attachment 136533 [details] Patch
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 on attachment 136533 [details] Patch Clearing flags on attachment: 136533 Committed r113799: <http://trac.webkit.org/changeset/113799>