Bug 83634

Created attachment 136565 [details] Patch

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 136565 [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/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 136565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136565&action=review The main thing we need here is tests. Perhaps adding a function to Internals.idl that sends an intent to a given window? > Source/WebCore/ChangeLog:3 > + IDL and implementation for Web Intents delivery Can you add a link to the spec? >> Source/WebCore/ChangeLog:8 >> + No new tests. (OOPS!) > > You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Can we add some tests, even just basic ones that check that the property exists? > Source/WebCore/Modules/intents/DOMWindowIntents.cpp:71 > + return true; This always returns true? > Source/WebCore/Modules/intents/DOMWindowIntents.idl:36 > + // !!! does this mean JS won't be able to call intent.postResult? or > + // just that the setter is masked so they'll be able to do "var intent = x" > + // which I want? The latter. readonly just refers to the slot in the DOMWindow, not the object that's in the slot. > Source/WebCore/Modules/intents/DeliveredIntent.h:56 > + virtual MessagePortArray* ports() const; > + virtual String getExtra(const String& key); > + virtual void postResult(PassRefPtr<SerializedScriptValue> data); > + virtual void postFailure(PassRefPtr<SerializedScriptValue> data); Why do these need to be virtual? > Source/WebCore/bindings/v8/custom/V8DeliveredIntentCustom.cpp:39 > +v8::Handle<v8::Value> V8DeliveredIntent::portsAccessorGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info) Does this need to be custom? Do we need to teach the CodeGenerator something about translating MessagePortArrays into JS arrays? > Source/WebKit/chromium/public/WebFrame.h:-575 > - This change seems spurious. > Source/WebKit/chromium/src/WebFrameImpl.cpp:1850 > + WebSerializedScriptValue ssv = WebSerializedScriptValue::fromString(intent.data()); Generally, we prefer using complete words in variable names. Perhaps intentData ?

Comment on attachment 136565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136565&action=review >> Source/WebCore/ChangeLog:3 >> + IDL and implementation for Web Intents delivery > > Can you add a link to the spec? Why yes. :-) >>> Source/WebCore/ChangeLog:8 >>> + No new tests. (OOPS!) >> >> You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] > > Can we add some tests, even just basic ones that check that the property exists? Definitely. I uploaded this to get a sense of what else needed done. Do you like the API and approach; am I using the Supplement<> stuff right. That kind of thing. (More codegen changes? :-)) There's still critical bits missing, like wiring up the return pathway for postResult and postFailure. >> Source/WebCore/Modules/intents/DOMWindowIntents.cpp:71 >> + return true; > > This always returns true? I originally had these as void methods, but then I got concerned about the sequencing of integating this with chromium, and whether the chromium API change will get split off from the webcore impl. This design enables chromium code to bridge that period, after which we can ignore the return value and set it to void. Is that too conservative? >> Source/WebCore/Modules/intents/DOMWindowIntents.idl:36 >> + // which I want? > > The latter. readonly just refers to the slot in the DOMWindow, not the object that's in the slot. Cool. Thanks. >> Source/WebCore/Modules/intents/DeliveredIntent.h:56 >> + virtual void postFailure(PassRefPtr<SerializedScriptValue> data); > > Why do these need to be virtual? No reason. I was copying a usage pattern I saw elsewhere (IDB I think), and pondered the same question... >> Source/WebCore/bindings/v8/custom/V8DeliveredIntentCustom.cpp:39 >> +v8::Handle<v8::Value> V8DeliveredIntent::portsAccessorGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info) > > Does this need to be custom? Do we need to teach the CodeGenerator something about translating MessagePortArrays into JS arrays? Now why did I have a suspicion you'd say that. :-) All our ports accessors are custom, yes. (I basically copy-pasted this from one of the Worker ones, IIRC.) I haven't looked closely enough yet to know where the gap is here. >> Source/WebKit/chromium/public/WebFrame.h:-575 >> - > > This change seems spurious. yes. removed. >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1850 >> + WebSerializedScriptValue ssv = WebSerializedScriptValue::fromString(intent.data()); > > Generally, we prefer using complete words in variable names. Perhaps intentData ? Done.

Comment on attachment 136565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136565&action=review > Source/WebCore/Modules/intents/DOMWindowIntents.cpp:39 > + , m_window(window) Do you need m_window? You don't seem to use it. >>> Source/WebCore/Modules/intents/DOMWindowIntents.cpp:71 >>> + return true; >> >> This always returns true? > > I originally had these as void methods, but then I got concerned about the sequencing of integating this with chromium, and whether the chromium API change will get split off from the webcore impl. This design enables chromium code to bridge that period, after which we can ignore the return value and set it to void. Is that too conservative? I'm not sure I quite follow, but that seems like something we can deal with at the WebKit API layer. This function should probably just be void. >>> Source/WebCore/bindings/v8/custom/V8DeliveredIntentCustom.cpp:39 >>> +v8::Handle<v8::Value> V8DeliveredIntent::portsAccessorGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info) >> >> Does this need to be custom? Do we need to teach the CodeGenerator something about translating MessagePortArrays into JS arrays? > > Now why did I have a suspicion you'd say that. :-) All our ports accessors are custom, yes. (I basically copy-pasted this from one of the Worker ones, IIRC.) I haven't looked closely enough yet to know where the gap is here. Copy/pasting code is usually a strong indicator that something is wrong. :)

This generally looks good. There are some details to polish but nothing major.

Created attachment 136715 [details] Patch

Created attachment 136731 [details] Patch

Comment on attachment 136731 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136731&action=review > Source/WebCore/ChangeLog:9 > + No new tests. (OOPS!) We won't be able to land this patch with this line in the ChangeLog. We either need to add some tests or explain why this change isn't testable. > Source/WebCore/Modules/intents/DOMWindowIntents.cpp:70 > + m_intent = intent; > + m_intent->setFrame(frame()); What should we do if frame() is null? This happens after the DOMWindow is detached from the frame. It looks like we'll crash later on in postResult, so it's probably better to return early if frame() is null. > Source/WebCore/Modules/intents/DeliveredIntent.h:68 > + Frame* m_frame; What keeps this pointer from becoming stale? It just looks like you're holding a raw pointer to the Frame without any way of knowing when the frame gets destroyed. DeliveredIntent looks to be a DOM object that script can hold on to long after the frame has been destroyed. I suspect that you want to associate the DeliveredIntent with a ScriptExecutionObject by making DeliveredIntent a subclass of ContextDestructionObserver. You can then get to the frame via Document::frame(), which will be null after the frame is gone. > Source/WebCore/loader/FrameLoaderClient.h:334 > #if ENABLE(WEB_INTENTS) > virtual void dispatchIntent(PassRefPtr<IntentRequest>) = 0; > + virtual void deliveredIntentResult(int, PassRefPtr<SerializedScriptValue>) = 0; > + virtual void deliveredIntentFailure(int, PassRefPtr<SerializedScriptValue>) = 0; > #endif Is there a reason these functions are part of the FrameLoaderClient? Should they be part of an IntentClient instead? Maybe that doesn't matter. > Source/WebKit/chromium/public/WebFrameClient.h:386 > + // Send a success reply to a web intent delivered to this window. > + virtual void deliveredIntentResult(WebFrame*, int, const WebSerializedScriptValue&) { } This name seems a bit odd. Maybe didProduceIntentResult ? That's not a great name either.

Comment on attachment 136731 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136731&action=review >> Source/WebCore/ChangeLog:9 >> + No new tests. (OOPS!) > > We won't be able to land this patch with this line in the ChangeLog. We either need to add some tests or explain why this change isn't testable. Yes. I haven't removed it because I want to make sure I don't get CQed before adding the test. :-) >> Source/WebCore/Modules/intents/DOMWindowIntents.cpp:70 >> + m_intent->setFrame(frame()); > > What should we do if frame() is null? This happens after the DOMWindow is detached from the frame. It looks like we'll crash later on in postResult, so it's probably better to return early if frame() is null. This is in a delivery/teardown kind of race, right? That is, we really shouldn't attempt to deliver the intent if the frame is null. (Just to cross t's, the from() method above shouldn't need to protect from that race, correct? That is, the method cannot be called with DOMWindow either null or in a state which it shouldn't be supplemented?) >> Source/WebCore/Modules/intents/DeliveredIntent.h:68 >> + Frame* m_frame; > > What keeps this pointer from becoming stale? It just looks like you're holding a raw pointer to the Frame without any way of knowing when the frame gets destroyed. DeliveredIntent looks to be a DOM object that script can hold on to long after the frame has been destroyed. > > I suspect that you want to associate the DeliveredIntent with a ScriptExecutionObject by making DeliveredIntent a subclass of ContextDestructionObserver. You can then get to the frame via Document::frame(), which will be null after the frame is gone. My mental image is that the supplemental machinery tears down the DOMWindowIntents object before this object would become stale (the frame() comes from the DOMWindowProperty), and the DOMWindowIntents is what owns the DeliveredIntent. If I have that lifecycle image incorrect, I need some other machinery to make sure I don't violate. Does this supplemental lifecycle imply the kind of ScriptExecutionContext/Document lifecycle, or do they proceed independently enough that it's a problem? >> Source/WebCore/loader/FrameLoaderClient.h:334 >> #endif > > Is there a reason these functions are part of the FrameLoaderClient? Should they be part of an IntentClient instead? Maybe that doesn't matter. This object looks like it groups the various client notifications from the frame. I don't recall whether we had the discussion of a potential IntentClient when dispatchIntent went in or not. I've just laid this delivery stuff alongside the existing invocation intents code, but I don't think it'll keep growing -- there's only the two directions. >> Source/WebKit/chromium/public/WebFrameClient.h:386 >> + virtual void deliveredIntentResult(WebFrame*, int, const WebSerializedScriptValue&) { } > > This name seems a bit odd. Maybe didProduceIntentResult ? That's not a great name either. Yeah, I went through about three iterations. My theory was that the client needs to be told that that intent it delivered has now produced a result. How about "resultForDeliveredIntent"? I tried "postIntentResult" to be very parallel to the proposed JS language, as well.

(In reply to comment #11) > (From update of attachment 136731 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136731&action=review > > >> Source/WebCore/Modules/intents/DOMWindowIntents.cpp:70 > >> + m_intent->setFrame(frame()); > > > > What should we do if frame() is null? This happens after the DOMWindow is detached from the frame. It looks like we'll crash later on in postResult, so it's probably better to return early if frame() is null. > > This is in a delivery/teardown kind of race, right? That is, we really shouldn't attempt to deliver the intent if the frame is null. (Just to cross t's, the from() method above shouldn't need to protect from that race, correct? That is, the method cannot be called with DOMWindow either null or in a state which it shouldn't be supplemented?) Yeah, I don't think it can actually occur given that this function is only called in one place. Perhaps we should just ASSERT that it's non-null. > >> Source/WebCore/Modules/intents/DeliveredIntent.h:68 > >> + Frame* m_frame; > > > > What keeps this pointer from becoming stale? It just looks like you're holding a raw pointer to the Frame without any way of knowing when the frame gets destroyed. DeliveredIntent looks to be a DOM object that script can hold on to long after the frame has been destroyed. > > > > I suspect that you want to associate the DeliveredIntent with a ScriptExecutionObject by making DeliveredIntent a subclass of ContextDestructionObserver. You can then get to the frame via Document::frame(), which will be null after the frame is gone. > > My mental image is that the supplemental machinery tears down the DOMWindowIntents object before this object would become stale (the frame() comes from the DOMWindowProperty), That part is true. > and the DOMWindowIntents is what owns the DeliveredIntent. So, that's not quite right. DeliveredIntent is not owned by DOMWindowIntents. DeliveredIntent is RefCounted and can be held by JavaScript for an arbitrary lifetime. > If I have that lifecycle image incorrect, I need some other machinery to make sure I don't violate. Does this supplemental lifecycle imply the kind of ScriptExecutionContext/Document lifecycle, or do they proceed independently enough that it's a problem? I didn't understand your question. You need something to tell you when the Frame dies. As I wrote above, I'd recommend using a ScriptExecutionContext as your context object (rather than a Frame). Then you can use a ContextDestructionObserver base class, which will make sure to null out the pointer before it becomes stale. From the ScriptExecutionContext (which is always Document on the main thread), you can get back to the frame via a pointer that's also cleared automatically. > >> Source/WebCore/loader/FrameLoaderClient.h:334 > >> #endif > > > > Is there a reason these functions are part of the FrameLoaderClient? Should they be part of an IntentClient instead? Maybe that doesn't matter. > > This object looks like it groups the various client notifications from the frame. I don't recall whether we had the discussion of a potential IntentClient when dispatchIntent went in or not. I've just laid this delivery stuff alongside the existing invocation intents code, but I don't think it'll keep growing -- there's only the two directions. Yeah, FrameLoaderClient is a mess of random functions that folks have added over time. I'm slightly sad that we're adding to this mess, but if these are all the functions, then it's probably ok. > >> Source/WebKit/chromium/public/WebFrameClient.h:386 > >> + virtual void deliveredIntentResult(WebFrame*, int, const WebSerializedScriptValue&) { } > > > > This name seems a bit odd. Maybe didProduceIntentResult ? That's not a great name either. > > Yeah, I went through about three iterations. My theory was that the client needs to be told that that intent it delivered has now produced a result. How about "resultForDeliveredIntent"? I tried "postIntentResult" to be very parallel to the proposed JS language, as well. didPostResultForIntent? didPostFailureForIntent? It's a tricky function to name well.

Created attachment 136790 [details] Patch

Comment on attachment 136790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136790&action=review > Tools/DumpRenderTree/chromium/WebViewHost.cpp:1336 > + printf("Web intent success for id %d\n", id); I'd like to assume the payload is text and print it, but the serialized form will only deserialize to a v8 value. Is it OK to add v8 deps to this class for that?

Comment on attachment 136565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136565&action=review >> Source/WebCore/Modules/intents/DOMWindowIntents.cpp:39 >> + , m_window(window) > > Do you need m_window? You don't seem to use it. Yep. already gone >>>> Source/WebCore/Modules/intents/DOMWindowIntents.cpp:71 >>>> + return true; >>> >>> This always returns true? >> >> I originally had these as void methods, but then I got concerned about the sequencing of integating this with chromium, and whether the chromium API change will get split off from the webcore impl. This design enables chromium code to bridge that period, after which we can ignore the return value and set it to void. Is that too conservative? > > I'm not sure I quite follow, but that seems like something we can deal with at the WebKit API layer. This function should probably just be void. OK, will do. Shall I split this CL in half? I've added the reply pathways now. They aren't that complex, but there's a lot more files involved. >>>> Source/WebCore/bindings/v8/custom/V8DeliveredIntentCustom.cpp:39 >>>> +v8::Handle<v8::Value> V8DeliveredIntent::portsAccessorGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info) >>> >>> Does this need to be custom? Do we need to teach the CodeGenerator something about translating MessagePortArrays into JS arrays? >> >> Now why did I have a suspicion you'd say that. :-) All our ports accessors are custom, yes. (I basically copy-pasted this from one of the Worker ones, IIRC.) I haven't looked closely enough yet to know where the gap is here. > > Copy/pasting code is usually a strong indicator that something is wrong. :) That and editing AnythingCustom.cpp :-) Shall we deal with the codegen gap first on this one, or afterwards?

Comment on attachment 136790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136790&action=review > Source/WebCore/Modules/intents/DeliveredIntent.h:67 > + int m_intentIdentifier; I'm not sure whether we should have identifiers at this layer. Generally, we prefer for the WebKit-Chromium API to be object oriented. Any identifiers that you need for cross-process communication should be handled in the Chromium side. > Source/WebCore/Modules/intents/DeliveredIntent.h:68 > + Frame* m_frame; This pointer can still be stale and is a security vulnerability.

> That and editing AnythingCustom.cpp :-) Shall we deal with the codegen gap first on this one, or afterwards? I'd prefer to fix the code generator first, if that's possible. It's better to write the code the first way instead of incurring technical debt and then paying it off.

(In reply to comment #12) > (In reply to comment #11) > > (From update of attachment 136731 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=136731&action=review > > > > >> Source/WebCore/Modules/intents/DOMWindowIntents.cpp:70 > > >> + m_intent->setFrame(frame()); > > > > > > What should we do if frame() is null? This happens after the DOMWindow is detached from the frame. It looks like we'll crash later on in postResult, so it's probably better to return early if frame() is null. > > > > This is in a delivery/teardown kind of race, right? That is, we really shouldn't attempt to deliver the intent if the frame is null. (Just to cross t's, the from() method above shouldn't need to protect from that race, correct? That is, the method cannot be called with DOMWindow either null or in a state which it shouldn't be supplemented?) > > Yeah, I don't think it can actually occur given that this function is only called in one place. Perhaps we should just ASSERT that it's non-null. Done. > > > >> Source/WebCore/Modules/intents/DeliveredIntent.h:68 > > >> + Frame* m_frame; > > > > > > What keeps this pointer from becoming stale? It just looks like you're holding a raw pointer to the Frame without any way of knowing when the frame gets destroyed. DeliveredIntent looks to be a DOM object that script can hold on to long after the frame has been destroyed. > > > > > > I suspect that you want to associate the DeliveredIntent with a ScriptExecutionObject by making DeliveredIntent a subclass of ContextDestructionObserver. You can then get to the frame via Document::frame(), which will be null after the frame is gone. > > > > My mental image is that the supplemental machinery tears down the DOMWindowIntents object before this object would become stale (the frame() comes from the DOMWindowProperty), > > That part is true. > > > and the DOMWindowIntents is what owns the DeliveredIntent. > > So, that's not quite right. DeliveredIntent is not owned by DOMWindowIntents. DeliveredIntent is RefCounted and can be held by JavaScript for an arbitrary lifetime. > > > If I have that lifecycle image incorrect, I need some other machinery to make sure I don't violate. Does this supplemental lifecycle imply the kind of ScriptExecutionContext/Document lifecycle, or do they proceed independently enough that it's a problem? > > I didn't understand your question. You need something to tell you when the Frame dies. As I wrote above, I'd recommend using a ScriptExecutionContext as your context object (rather than a Frame). Then you can use a ContextDestructionObserver base class, which will make sure to null out the pointer before it becomes stale. From the ScriptExecutionContext (which is always Document on the main thread), you can get back to the frame via a pointer that's also cleared automatically. OK, and that would solve all issues of Frame* pointers, too. > > >> Source/WebCore/loader/FrameLoaderClient.h:334 > > >> #endif > > > > > > Is there a reason these functions are part of the FrameLoaderClient? Should they be part of an IntentClient instead? Maybe that doesn't matter. > > > > This object looks like it groups the various client notifications from the frame. I don't recall whether we had the discussion of a potential IntentClient when dispatchIntent went in or not. I've just laid this delivery stuff alongside the existing invocation intents code, but I don't think it'll keep growing -- there's only the two directions. > > Yeah, FrameLoaderClient is a mess of random functions that folks have added over time. I'm slightly sad that we're adding to this mess, but if these are all the functions, then it's probably ok. > > > >> Source/WebKit/chromium/public/WebFrameClient.h:386 > > >> + virtual void deliveredIntentResult(WebFrame*, int, const WebSerializedScriptValue&) { } > > > > > > This name seems a bit odd. Maybe didProduceIntentResult ? That's not a great name either. > > > > Yeah, I went through about three iterations. My theory was that the client needs to be told that that intent it delivered has now produced a result. How about "resultForDeliveredIntent"? I tried "postIntentResult" to be very parallel to the proposed JS language, as well. > > didPostResultForIntent? > didPostFailureForIntent? > > It's a tricky function to name well. I like those the best so far.

Comment on attachment 136790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136790&action=review >> Source/WebCore/Modules/intents/DeliveredIntent.h:67 >> + int m_intentIdentifier; > > I'm not sure whether we should have identifiers at this layer. Generally, we prefer for the WebKit-Chromium API to be object oriented. Any identifiers that you need for cross-process communication should be handled in the Chromium side. I'll move this up. >> Source/WebCore/Modules/intents/DeliveredIntent.h:68 >> + Frame* m_frame; > > This pointer can still be stale and is a security vulnerability. Removing as part of basing on SEC.

Created attachment 137380 [details] Patch

Attachment 137380 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/webintents/web-intents-deliver..." exit_code: 1 Source/WebCore/ChangeLog:9: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 137380 [details] Patch Attachment 137380 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12415368 New failing tests: webintents/web-intents-delivery.html

Created attachment 137399 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 137925 [details] Patch

Attachment 137925 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/webintents/web-intents-deliver..." exit_code: 1 Source/WebCore/ChangeLog:9: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.

My test is indicating that the [Replaceable] attribute can be set locally, but that then I can't override it in my code, even after it's been deleted in Javascript. Is that right? I think that's OK for my use case, I just need to structure my tests a little differently.

Comment on attachment 137925 [details] Patch Attachment 137925 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12460001 New failing tests: webintents/web-intents-delivery.html

Created attachment 137945 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 137925 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137925&action=review > Source/WebCore/Modules/intents/DOMWindowIntents.idl:35 > + readonly attribute [Replaceable] DeliveredIntent webkitIntent; Why do you want to put [Replaceable] here? We don't use [Replaceable] unless the spec requires it. (Please look at http://dev.w3.org/2006/webapi/WebIDL/#Replaceable)

Created attachment 138391 [details] Patch

Comment on attachment 138391 [details] Patch Attachment 138391 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12517188 New failing tests: webintents/web-intents-delivery-reuse.html

Created attachment 138514 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 138657 [details] Patch

Replaced [Replaceable] -- without that JS can't use the variable (that's what caused the delivery-reuse test to fail).

Comment on attachment 138657 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138657&action=review > Source/WebKit/chromium/public/WebFrame.h:582 > + virtual void deliverIntent(const WebDeliveredIntentClient&) = 0; can you explain how this is supposed to work across processes? i presume the embedder needs to provide the delivered intent and somehow proxy the postResult / postFailure events back to the sender of the intent. i'm not seeing the callback interface for postResult / postFailure that would enable the above. i was expecting WebDeliveredIntentClient to be an interface implemented by the embedder, but it seems to be a concrete class that works differently. can you give me an idea of how this is supposed to work?

Comment on attachment 138657 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138657&action=review >> Source/WebKit/chromium/public/WebFrame.h:582 >> + virtual void deliverIntent(const WebDeliveredIntentClient&) = 0; > > can you explain how this is supposed to work across processes? i presume the embedder > needs to provide the delivered intent and somehow proxy the postResult / postFailure > events back to the sender of the intent. > > i'm not seeing the callback interface for postResult / postFailure that would enable > the above. i was expecting WebDeliveredIntentClient to be an interface implemented > by the embedder, but it seems to be a concrete class that works differently. can > you give me an idea of how this is supposed to work? I haven't plumbed this in yet, as I wanted to get advice on it. What I'd like is for embedders to subclass WebDeliveredIntentClient and then handle postResult/postFailure directly. Yes, forwarding the results back to the caller will be the job of the embedder. If subclassing is not a good plan, and an interface class is more expected, it would be pretty easy to swivel in that direction. On the Chromium side, this will be coordinated by the WebIntentsHost. When that class gets the OnSetIntent message, it'll whip up one of these client objects, and then deliver it to the WebFrame on DidClearWindowObject. Internally, it would then forward results back through IPC. So from the embedder side, it's a little easier to subclass as there's less API surface to support, but not dramatically.

Created attachment 139120 [details] Patch

Any more thoughts about the WebCore side of this change, Kentaro? It looks to me like the [Replaceable] is necessary to allow JS the use of the variable name if webcore isn't using it.

(In reply to comment #38) > Any more thoughts about the WebCore side of this change, Kentaro? It looks to me like the [Replaceable] is necessary to allow JS the use of the variable name if webcore isn't using it. I am a bit confused. Didn't we reach a conclusion that we do not need [Replaceable] here? What is the rational for using [Replaceable]? Other webkitXXX attributes in DOMWindow.idl do not have [Replaceable].

(In reply to comment #39) > (In reply to comment #38) > > Any more thoughts about the WebCore side of this change, Kentaro? It looks to me like the [Replaceable] is necessary to allow JS the use of the variable name if webcore isn't using it. > > I am a bit confused. Didn't we reach a conclusion that we do not need [Replaceable] here? What is the rational for using [Replaceable]? Other webkitXXX attributes in DOMWindow.idl do not have [Replaceable]. We did, but it was the wrong conclusion -- the attribute can't be used by JS without [Replaceable]. Or, more accurately, using [Replaceable] is the only way I know to make the attribute reusable by JS. There's a test included which fails without the modifier. For the name "webkitIntent" it is not as important. For "intent" (unprefixed) we want to have this attribute not stomp on existing JS usage which may use that name but not be intent-enabled. Given that, I'd still like to maintain the corresponding re-use semantics.

(In reply to comment #40) > We did, but it was the wrong conclusion -- the attribute can't be used by JS without [Replaceable]. Or, more accurately, using [Replaceable] is the only way I know to make the attribute reusable by JS. There's a test included which fails without the modifier. For the name "webkitIntent" it is not as important. For "intent" (unprefixed) we want to have this attribute not stomp on existing JS usage which may use that name but not be intent-enabled. Given that, I'd still like to maintain the corresponding re-use semantics. Thanks, I understand why you want to add [Replaceable]. But my question is what is the rational for making webkitIntent behave differently from other attributes in DOMWindow.idl? Is it speced?

(In reply to comment #41) > (In reply to comment #40) > > We did, but it was the wrong conclusion -- the attribute can't be used by JS without [Replaceable]. Or, more accurately, using [Replaceable] is the only way I know to make the attribute reusable by JS. There's a test included which fails without the modifier. For the name "webkitIntent" it is not as important. For "intent" (unprefixed) we want to have this attribute not stomp on existing JS usage which may use that name but not be intent-enabled. Given that, I'd still like to maintain the corresponding re-use semantics. > > Thanks, I understand why you want to add [Replaceable]. But my question is what is the rational for making webkitIntent behave differently from other attributes in DOMWindow.idl? Is it speced? Yes, the spec says it shouldn't be populated unless the page declares that it uses web intents.

(In reply to comment #36) > (From update of attachment 138657 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=138657&action=review > > >> Source/WebKit/chromium/public/WebFrame.h:582 > >> + virtual void deliverIntent(const WebDeliveredIntentClient&) = 0; > > > > can you explain how this is supposed to work across processes? i presume the embedder > > needs to provide the delivered intent and somehow proxy the postResult / postFailure > > events back to the sender of the intent. > > > > i'm not seeing the callback interface for postResult / postFailure that would enable > > the above. i was expecting WebDeliveredIntentClient to be an interface implemented > > by the embedder, but it seems to be a concrete class that works differently. can > > you give me an idea of how this is supposed to work? > > I haven't plumbed this in yet, as I wanted to get advice on it. What I'd like is for embedders to subclass WebDeliveredIntentClient and then handle postResult/postFailure directly. Yes, forwarding the results back to the caller will be the job of the embedder. > > If subclassing is not a good plan, and an interface class is more expected, it would be pretty easy to swivel in that direction. Yes, don't have the embedder subclass. Instead, they should implement an interface. > On the Chromium side, this will be coordinated by the WebIntentsHost. When that class gets the OnSetIntent message, it'll whip up one of these client objects, and then deliver it to the WebFrame on DidClearWindowObject. Internally, it would then forward results back through IPC. So from the embedder side, it's a little easier to subclass as there's less API surface to support, but not dramatically.

Created attachment 140119 [details] Patch

(In reply to comment #43) > (In reply to comment #36) > > (From update of attachment 138657 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=138657&action=review > > > > >> Source/WebKit/chromium/public/WebFrame.h:582 > > >> + virtual void deliverIntent(const WebDeliveredIntentClient&) = 0; > > > > > > can you explain how this is supposed to work across processes? i presume the embedder > > > needs to provide the delivered intent and somehow proxy the postResult / postFailure > > > events back to the sender of the intent. > > > > > > i'm not seeing the callback interface for postResult / postFailure that would enable > > > the above. i was expecting WebDeliveredIntentClient to be an interface implemented > > > by the embedder, but it seems to be a concrete class that works differently. can > > > you give me an idea of how this is supposed to work? > > > > I haven't plumbed this in yet, as I wanted to get advice on it. What I'd like is for embedders to subclass WebDeliveredIntentClient and then handle postResult/postFailure directly. Yes, forwarding the results back to the caller will be the job of the embedder. > > > > If subclassing is not a good plan, and an interface class is more expected, it would be pretty easy to swivel in that direction. > > Yes, don't have the embedder subclass. Instead, they should implement an interface. OK, I've change the signature now to reflect this. I left the forwarder object in the WebDeliveredIntentClient, so it isn't a pure interface. Would you prefer that be pulled out so WDIC can be simpler pure-virtual? Easy to do if that's the way these things always are. The other main issue (I think) is [Replaceable]/[Deletable]. The main thing I want to have happen is for the name ("webkitIntent" now, but just "intent" unprefixed) to be available to JS on pages that aren't getting intents delivered to them. There's a test for this, which passes now (with [Replaceable]), and does not pass (JS can't read values) with no decoration on that attribute. So what's the right solution? Adam and I thought [Replaceable] was the way to go. Do I need Deletable as well? Instead of? (The IDL docs are a bit hazy on this, saying mainly that "Deletable" means it can be deleted. :-)) > > > > On the Chromium side, this will be coordinated by the WebIntentsHost. When that class gets the OnSetIntent message, it'll whip up one of these client objects, and then deliver it to the WebFrame on DidClearWindowObject. Internally, it would then forward results back through IPC. So from the embedder side, it's a little easier to subclass as there's less API surface to support, but not dramatically.

Created attachment 140574 [details] Patch

Comment on attachment 140574 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140574&action=review > Source/WebCore/Modules/intents/DeliveredIntent.h:51 > +class DeliveredIntentClient : public RefCounted<DeliveredIntentClient> { Generally clients aren't RefCounted. Their lifetime is managed by the WebKit layer. > Source/WebKit/chromium/public/WebDeliveredIntentClient.h:51 > +#include "WebMessagePortChannel.h" > +#include "platform/WebCommon.h" > +#include "platform/WebPrivatePtr.h" > +#include "platform/WebString.h" > +#include "platform/WebVector.h" > + > +namespace WebCore { > +class DeliveredIntent; > +class DeliveredIntentClient; > +} > + > +#if WEBKIT_IMPLEMENTATION > +namespace WTF { template <typename T> class PassRefPtr; } > +#endif > + > +namespace WebKit { > + > +class DeliveredIntentClientImpl; Many of these declarations appear unused. Please only declare things that you need. > Source/WebKit/chromium/public/WebDeliveredIntentClient.h:69 > +#if WEBKIT_IMPLEMENTATION > +#if ENABLE(WEB_INTENTS) > + static void SetClient(WebCore::DeliveredIntent*, const WebDeliveredIntentClient&); > +#endif > +#endif This doesn't seem to fix here. Wouldn't te client be supplied at the same time that the DeliveredIntent is supplied? > Source/WebKit/chromium/src/WebDeliveredIntentClient.cpp:44 > + DeliveredIntentClientImpl(const WebDeliveredIntentClient* owner) : m_owner(owner) { } owner ? The WebDeliveredIntentClient object doesn't own this object, does it? We should probably use a different name. Also, please mark one argument constructors "explicit". > Source/WebKit/chromium/src/WebDeliveredIntentClient.cpp:70 > +void WebDeliveredIntentClient::SetClient(WebCore::DeliveredIntent* intent, const WebDeliveredIntentClient& intentClient) > +{ > + RefPtr<DeliveredIntentClientImpl> client(adoptRef(new DeliveredIntentClientImpl(&intentClient))); > + intent->setClient(client.release()); > +} DeliveredIntentClientImpl shouldn't be in WebDeliveredIntentClient.cpp. There shouldn't be a WebDeliveredIntentClient.cpp at all because WebDeliveredIntentClient is a pure virtual interface. Instead, DeliveredIntentClientImpl should be in a DeliveredIntentClientImpl.h and DeliveredIntentClientImpl.cpp file. > Source/WebKit/chromium/src/WebFrameImpl.cpp:1891 > +void WebFrameImpl::deliverIntent(const WebIntent&intent, const WebDeliveredIntentClient& intentClient) const WebIntent&intent -> const WebIntent& intent > Source/WebKit/chromium/src/WebFrameImpl.cpp:1899 > + HashMap<String, String> extras; > + WebVector<WebString> names = intent.extrasNames(); > + for (size_t i = 0; i < names.size(); ++i) > + extras.set(names[i], intent.extrasValue(names[i])); It seems like there should be a more elegant want to take or copy the extra data out of an intent rather than this bare handed lifting. > Source/WebKit/chromium/src/WebFrameImpl.cpp:1905 > + RefPtr<DeliveredIntent> deliveredIntent = > + DeliveredIntent::create(m_frame, intent.action(), intent.type(), intentData, messagePortArray.release().get(), extras); It seems like we should supply the intentClient to the DeliveredIntent here rather than calling through the strange WebDeliveredIntentClient::SetClient static. messagePortArray.release().get() => This makes no sense. Calling release will create a PassOwnPtr object, which will delete the pointer when the temporary is destroyed. Calling get() gives you a raw pointer to something that's about to be deleted.

Comment on attachment 140574 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140574&action=review >> Source/WebCore/Modules/intents/DeliveredIntent.h:51 >> +class DeliveredIntentClient : public RefCounted<DeliveredIntentClient> { > > Generally clients aren't RefCounted. Their lifetime is managed by the WebKit layer. Removed. >> Source/WebKit/chromium/public/WebDeliveredIntentClient.h:51 >> +class DeliveredIntentClientImpl; > > Many of these declarations appear unused. Please only declare things that you need. Thanks. I think I got them all. (Leftovers I hadn't cleared.) >> Source/WebKit/chromium/public/WebDeliveredIntentClient.h:69 >> +#endif > > This doesn't seem to fix here. Wouldn't te client be supplied at the same time that the DeliveredIntent is supplied? Agreed. That's better in the create factory. Moved. >> Source/WebKit/chromium/src/WebDeliveredIntentClient.cpp:44 >> + DeliveredIntentClientImpl(const WebDeliveredIntentClient* owner) : m_owner(owner) { } > > owner ? The WebDeliveredIntentClient object doesn't own this object, does it? We should probably use a different name. Also, please mark one argument constructors "explicit". Done, and moved internally to WebFrameImpl. >> Source/WebKit/chromium/src/WebDeliveredIntentClient.cpp:70 >> +} > > DeliveredIntentClientImpl shouldn't be in WebDeliveredIntentClient.cpp. There shouldn't be a WebDeliveredIntentClient.cpp at all because WebDeliveredIntentClient is a pure virtual interface. Instead, DeliveredIntentClientImpl should be in a DeliveredIntentClientImpl.h and DeliveredIntentClientImpl.cpp file. Done and moved to WebFrameImpl internally. >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1891 >> +void WebFrameImpl::deliverIntent(const WebIntent&intent, const WebDeliveredIntentClient& intentClient) > > const WebIntent&intent -> const WebIntent& intent done >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1899 >> + extras.set(names[i], intent.extrasValue(names[i])); > > It seems like there should be a more elegant want to take or copy the extra data out of an intent rather than this bare handed lifting. We don't have a "WebHashtable" object. :-( (Maybe we should...) >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1905 >> + DeliveredIntent::create(m_frame, intent.action(), intent.type(), intentData, messagePortArray.release().get(), extras); > > It seems like we should supply the intentClient to the DeliveredIntent here rather than calling through the strange WebDeliveredIntentClient::SetClient static. > > messagePortArray.release().get() => This makes no sense. Calling release will create a PassOwnPtr object, which will delete the pointer when the temporary is destroyed. Calling get() gives you a raw pointer to something that's about to be deleted. Changed this to better mimic what MessageEvent is doing -- using PassOwnPtrs.

Created attachment 140635 [details] Patch

Comment on attachment 140635 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140635&action=review > Source/WebCore/Modules/intents/DOMWindowIntents.h:47 > + virtual void deliver(PassRefPtr<DeliveredIntent>); Why is this virtual? Do we subclass this object? > Source/WebCore/Modules/intents/DeliveredIntent.h:58 > + virtual void postResult(PassRefPtr<SerializedScriptValue> data) { } > + virtual void postFailure(PassRefPtr<SerializedScriptValue> data) { } Should these be pure virtual? Who would want to subclass this object and not implement these methods? > Source/WebKit/chromium/src/WebFrameImpl.cpp:510 > +class DeliveredIntentClientImpl : public WebCore::DeliveredIntentClient { Please put this class in its own file rather than larding up WebFrameImpl.cpp with extra code. WebFrameImpl.cpp is already way, way too long. We don't want it to become the all-sing, all-dance file of WebKit. > Source/WebKit/chromium/src/WebFrameImpl.cpp:528 > + const WebDeliveredIntentClient* m_client; Why const? That doesn't make any sense. The ownership model of this pointer is more complicated than needed. The way you've got things set up, this pointer can often point off into garbage memory (because the lifetime of the object it points to is controlled by the embedder, and supposedly linked to the WebFrame). The consumer of this class, however, shields us from using unallocated memory by checking it's frame() pointer for null. That seems overly fragile. Somehow, we need a more direct link between the lifetime of the WebDeliveredIntentClient object and this raw pointer. One approach is to have WebKit notify the embedder when the WebDeliveredIntentClient is no longer needed. Another approach is to have the embedder notify WebKit when WebDeliveredIntentClient is no longer available. A third option is to use a single WebDeliveredIntentClient object for each WebFrame and couple the lifetimes that way. When calling back through WebDeliveredIntentClient, WebKit would supply a context object, such as a WebDeliveredIntent, to indiciate which intent was calling back through the interface. This third approach is what we use most often in the WebKit API and is the least susceptible to leaks or use-after-free. > Source/WebKit/chromium/src/WebFrameImpl.cpp:1922 > + OwnPtr<MessagePortArray> messagePortArray; What's the point of declaring this local variable? It only ever contains a null pointer. You might as well just call DeliveredIntent::create with nullptr instead. > Source/WebKit/chromium/src/WebFrameImpl.cpp:1924 > + OwnPtr<DeliveredIntentClientImpl> client(adoptPtr(new DeliveredIntentClientImpl(&intentClient))); Why are we taking the address of intentClient? It seems better to just pass the object to this function by pointer rather than by const reference. > Source/WebKit/chromium/src/WebFrameImpl.cpp:1931 > + DOMWindow* window = m_frame->domWindow(); > + DOMWindowIntents::from(window)->deliver(deliveredIntent.release()); You can combine these two lines. > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2217 > + m_shell->webView()->mainFrame()->deliverIntent(intent, *m_intentClient); Notice the balancing * here that makes it seem like we should pass this argument by pointer rather than by const reference.

Comment on attachment 140635 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140635&action=review >> Source/WebCore/Modules/intents/DOMWindowIntents.h:47 >> + virtual void deliver(PassRefPtr<DeliveredIntent>); > > Why is this virtual? Do we subclass this object? No. Usual culprit: me trying to follow usages but either getting puzzled by them or picking the wrong ones. :-) >> Source/WebCore/Modules/intents/DeliveredIntent.h:58 >> + virtual void postFailure(PassRefPtr<SerializedScriptValue> data) { } > > Should these be pure virtual? Who would want to subclass this object and not implement these methods? Sounds good. >> Source/WebKit/chromium/src/WebFrameImpl.cpp:510 >> +class DeliveredIntentClientImpl : public WebCore::DeliveredIntentClient { > > Please put this class in its own file rather than larding up WebFrameImpl.cpp with extra code. WebFrameImpl.cpp is already way, way too long. We don't want it to become the all-sing, all-dance file of WebKit. :-) Done. I was lured by the siren calls of the nearby impl classes. :-) >> Source/WebKit/chromium/src/WebFrameImpl.cpp:528 >> + const WebDeliveredIntentClient* m_client; > > Why const? That doesn't make any sense. > > The ownership model of this pointer is more complicated than needed. The way you've got things set up, this pointer can often point off into garbage memory (because the lifetime of the object it points to is controlled by the embedder, and supposedly linked to the WebFrame). The consumer of this class, however, shields us from using unallocated memory by checking it's frame() pointer for null. That seems overly fragile. > > Somehow, we need a more direct link between the lifetime of the WebDeliveredIntentClient object and this raw pointer. One approach is to have WebKit notify the embedder when the WebDeliveredIntentClient is no longer needed. Another approach is to have the embedder notify WebKit when WebDeliveredIntentClient is no longer available. A third option is to use a single WebDeliveredIntentClient object for each WebFrame and couple the lifetimes that way. When calling back through WebDeliveredIntentClient, WebKit would supply a context object, such as a WebDeliveredIntent, to indiciate which intent was calling back through the interface. This third approach is what we use most often in the WebKit API and is the least susceptible to leaks or use-after-free. I think a separate client object is more ergonomic than routing the calls through FrameLoaderClient. (That's how it was in the first version.) The slip surface is definitely this trampoline object, which has split loyalties between needing to be a WebCore::DeliveredIntentClient, but forward to the chromium API object. It's easy enough to add a destroy() method to the chromium API to propagate up the notification that the WebDeliveredIntentClient can be killed. That fits nicely with how I've got the ownerships set up currently. The const here is to match const-ness on the WebFrame API, not the WebCore API. That's the exposed surface, which I was trying to mimic the webkit API with, but I see it is more idiomatic to pass WebDeliveredIntentClient*. >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1922 >> + OwnPtr<MessagePortArray> messagePortArray; > > What's the point of declaring this local variable? It only ever contains a null pointer. You might as well just call DeliveredIntent::create with nullptr instead. This is a TODO, but I'll call with 0 at this point. >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1924 >> + OwnPtr<DeliveredIntentClientImpl> client(adoptPtr(new DeliveredIntentClientImpl(&intentClient))); > > Why are we taking the address of intentClient? It seems better to just pass the object to this function by pointer rather than by const reference. Changed deliverIntent() signature. >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1931 >> + DOMWindowIntents::from(window)->deliver(deliveredIntent.release()); > > You can combine these two lines. Done >> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2217 >> + m_shell->webView()->mainFrame()->deliverIntent(intent, *m_intentClient); > > Notice the balancing * here that makes it seem like we should pass this argument by pointer rather than by const reference. Done.

Created attachment 140658 [details] Patch

Comment on attachment 140658 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140658&action=review I think we're getting close here. Hopefully we'll be ready after one more iteration. > Source/WebCore/Modules/intents/DeliveredIntent.h:77 > + virtual void frameDestroyed(); Please add the OVERRIDE keyword to this function to ensure that it continue to override the FrameDestructionObserver method. > Source/WebCore/Modules/intents/DeliveredIntent.h:84 > + OwnPtr<DeliveredIntentClient> m_client; Why does DeliveredIntentClient have a destroy() method? Why not just clear this pointer and use the destructor as the destroy signal? > Source/WebKit/chromium/public/WebDeliveredIntentClient.h:46 > + virtual ~WebDeliveredIntentClient() { }; This ; looks suprious. > Source/WebKit/chromium/src/DeliveredIntentClientImpl.cpp:39 > +void DeliveredIntentClientImpl::postResult(const PassRefPtr<WebCore::SerializedScriptValue> data) The const isn't needed here. It's not really meaningful anyway. > Source/WebKit/chromium/src/DeliveredIntentClientImpl.cpp:44 > +void DeliveredIntentClientImpl::postFailure(const PassRefPtr<WebCore::SerializedScriptValue> data) The const isn't needed here. It's not really meaningful anyway. > Source/WebKit/chromium/src/DeliveredIntentClientImpl.h:61 > + virtual void postResult(const PassRefPtr<WebCore::SerializedScriptValue> data); > + virtual void postFailure(const PassRefPtr<WebCore::SerializedScriptValue> data); These signatures don't match the ones in WebCore::DeliveredIntentClient. Given that the methods in WebCore::DeliveredIntentClient are pure virtual, the compiler must be matching them up (assuming that your patch compiles), but it would be better for the signatures to be identical. > Source/WebKit/chromium/src/WebFrameImpl.cpp:1899 > + HashMap<String, String> extras; > + WebVector<WebString> names = intent.extrasNames(); > + for (size_t i = 0; i < names.size(); ++i) > + extras.set(names[i], intent.extrasValue(names[i])); WebIntent is just a wrapper around WebCore::Intent. Can we add a #if WEBKIT_IMPLEMENTATION method to get the underlying WebCore:Intent? Then we could access the extras hashmap directly without needing this bare-handed lifting to marshall this data across the API to ourselves. > Source/WebKit/chromium/src/WebFrameImpl.cpp:1905 > + RefPtr<DeliveredIntent> deliveredIntent = > + DeliveredIntent::create(m_frame, client.release(), intent.action(), intent.type(), intentData, ports.release(), extras); This can just be one line. > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:287 > + delete m_intentClient; Can we use a smart pointer rather than calling delete manually.

Created attachment 140744 [details] Patch

Comment on attachment 140658 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140658&action=review >> Source/WebCore/Modules/intents/DeliveredIntent.h:77 >> + virtual void frameDestroyed(); > > Please add the OVERRIDE keyword to this function to ensure that it continue to override the FrameDestructionObserver method. Done >> Source/WebCore/Modules/intents/DeliveredIntent.h:84 >> + OwnPtr<DeliveredIntentClient> m_client; > > Why does DeliveredIntentClient have a destroy() method? Why not just clear this pointer and use the destructor as the destroy signal? Changed. >> Source/WebKit/chromium/public/WebDeliveredIntentClient.h:46 >> + virtual ~WebDeliveredIntentClient() { }; > > This ; looks suprious. Oops! >> Source/WebKit/chromium/src/DeliveredIntentClientImpl.cpp:39 >> +void DeliveredIntentClientImpl::postResult(const PassRefPtr<WebCore::SerializedScriptValue> data) > > The const isn't needed here. It's not really meaningful anyway. Done >> Source/WebKit/chromium/src/DeliveredIntentClientImpl.cpp:44 >> +void DeliveredIntentClientImpl::postFailure(const PassRefPtr<WebCore::SerializedScriptValue> data) > > The const isn't needed here. It's not really meaningful anyway. Done >> Source/WebKit/chromium/src/DeliveredIntentClientImpl.h:61 >> + virtual void postFailure(const PassRefPtr<WebCore::SerializedScriptValue> data); > > These signatures don't match the ones in WebCore::DeliveredIntentClient. Given that the methods in WebCore::DeliveredIntentClient are pure virtual, the compiler must be matching them up (assuming that your patch compiles), but it would be better for the signatures to be identical. Done. >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1899 >> + extras.set(names[i], intent.extrasValue(names[i])); > > WebIntent is just a wrapper around WebCore::Intent. Can we add a #if WEBKIT_IMPLEMENTATION method to get the underlying WebCore:Intent? Then we could access the extras hashmap directly without needing this bare-handed lifting to marshall this data across the API to ourselves. Can't get an include for hashmap in there, but I exposed the WebCore::Intent object, and got it off that API. >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1905 >> + DeliveredIntent::create(m_frame, client.release(), intent.action(), intent.type(), intentData, ports.release(), extras); > > This can just be one line. Done. >> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:287 >> + delete m_intentClient; > > Can we use a smart pointer rather than calling delete manually. Done.

Comment on attachment 140744 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140744&action=review > Source/WebKit/chromium/src/DeliveredIntentClientImpl.h:41 > +namespace WTF { template <typename T> class PassRefPtr; } > + > +namespace WebCore { > +class SerializedScriptValue; > +} This isn't an API header. You can just include the actual headers if you like. > Source/WebKit/chromium/src/WebFrameImpl.cpp:1897 > + HashMap<String, String> extras = webcoreIntent->extras(); You're creating an extra copy of the HashMap here. If you just passed webcoreIntent->extras() to DeliveredIntent::create directly, you could avoid this copy.

Comment on attachment 140744 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140744&action=review >> Source/WebKit/chromium/src/DeliveredIntentClientImpl.h:41 >> +} > > This isn't an API header. You can just include the actual headers if you like. Moved here to the header. >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1897 >> + HashMap<String, String> extras = webcoreIntent->extras(); > > You're creating an extra copy of the HashMap here. If you just passed webcoreIntent->extras() to DeliveredIntent::create directly, you could avoid this copy. Done.

Created attachment 141761 [details] Patch

Comment on attachment 141761 [details] Patch Attachment 141761 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12679609 New failing tests: webintents/web-intents-delivery-reuse.html

Created attachment 141769 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 141761 [details] Patch This looks fine, but it seems you have a problem with a failing test.

Created attachment 141828 [details] Patch

Oops. Fixed. I think in the end my checkout must have been out of sync. (In reply to comment #61) > (From update of attachment 141761 [details]) > This looks fine, but it seems you have a problem with a failing test.

Comment on attachment 141828 [details] Patch Attachment 141828 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12704302 New failing tests: webintents/web-intents-delivery-reuse.html

Created attachment 141852 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 141828 [details] Patch The box is still red.

Created attachment 142113 [details] Patch

Comment on attachment 142113 [details] Patch Attachment 142113 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12708437 New failing tests: webintents/web-intents-delivery-reuse.html

Created attachment 142160 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 142303 [details] Patch

Now cured (after a couple bad theories). (In reply to comment #66) > (From update of attachment 141828 [details]) > The box is still red.

Comment on attachment 142303 [details] Patch yay green box

Comment on attachment 142303 [details] Patch Clearing flags on attachment: 142303 Committed r117384: <http://trac.webkit.org/changeset/117384>

All reviewed patches have been landed. Closing bug.