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

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