[webkit-reviews] review denied: [Bug 76014] Web Intents chromium API modifications to track IntentRequest invocation method : [Attachment 122827] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Greg Billock
<gbillock at google.com>'s request for review:
Bug 76014: Web Intents chromium API modifications to track IntentRequest
invocation method
https://bugs.webkit.org/show_bug.cgi?id=76014

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=122827&action=review


> Source/WebKit/chromium/public/WebFrameClient.h:385
> +    // the WebFrame starting the activity. The callee uses this object to

This comment is incorrect now, right?  "Replies to this request should be sent
to __the request object__."

> Source/WebKit/chromium/public/WebFrameClient.h:387
> +    // soon as possible when it is no longer needed.)

This parenthetical comment is perhaps a bit confusing since there is no way
to release a WebIntentRequest.

> Source/WebKit/chromium/public/WebIntent.h:47
> +    WebIntent();

constructors should be implemented inline.

> Source/WebKit/chromium/public/WebIntent.h:48
> +    WebIntent(const WebIntent&);

this should just be an inline call to assign.

> Source/WebKit/chromium/public/WebIntentRequest.h:50
> +    WebIntentRequest();

implement constructors inline

> Source/WebKit/chromium/public/WebIntentRequest.h:70
> +#if ENABLE(WEB_INTENTS)

since you've unconditionally declared WebCore::IntentRequest, it seems like
this
function does not need to be protected by ENABLE(WEB_INTENTS).	in generaly,
it'd
be nice to avoid using ENABLE macros in public header files, even though this
one
is protected by WEBKIT_IMPLEMENTATION.

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1637
> +    WebIntentRequest request(intentRequest);

nit: you should be able to delete this line and just pass intentRequest
directly to dispatchIntent.  it should be implicitly converted to
WebIntentRequest.

> Source/WebKit/chromium/src/WebIntent.cpp:66
> +#endif

nit: use #else here and elsewhere instead of relying on first return statement
wins.

> Source/WebKit/chromium/src/WebIntentRequest.cpp:109
> +    RefPtr<WebCore::SerializedScriptValue> serializedValue = 

nit: we'd normally write this like so:

 
m_private->postFailure(PassRefPtr<WebCore::SerializedScriptValue>(data).get());