[webkit-reviews] review denied: [Bug 83634] IDL and implementation for Web Intents delivery : [Attachment 140635] 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 140635: Patch
https://bugs.webkit.org/attachment.cgi?id=140635&action=review

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