[webkit-reviews] review denied: [Bug 83634] IDL and implementation for Web Intents delivery : [Attachment 136731] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Greg Billock <gbillock at google.com>'s
request for review:
Bug 83634: IDL and implementation for Web Intents delivery
https://bugs.webkit.org/show_bug.cgi?id=83634

Attachment 136731: Patch
https://bugs.webkit.org/attachment.cgi?id=136731&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.