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

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