[webkit-reviews] review denied: [Bug 76014] Web Intents chromium API modifications to track IntentRequest invocation method : [Attachment 122099] 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 122099: Patch
https://bugs.webkit.org/attachment.cgi?id=122099&action=review

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


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

remember, if you do not implement a method inline or behind a
WEBKIT_IMPLEMENTATION guard, then it needs to be decorated with WEBKIT_EXPORT. 
else the DLL build will fail to link.

>>> Source/WebKit/chromium/src/WebIntentRequest.cpp:59
>>> +	 if (m_intentRequest.get() && m_intentRequest->intent())
>> 
>> is m_intentRequest->intent() ever null?  It seems meaningless to have an
intent request without an intent.
> 
> We discussed before that we may want to clear it out, since it might be
heavy. That kind of makes this API state-dependent, which is awkward, but it
may be preferable to carrying a large Intent object payload for a long time.

Sorry to create more work for you, but we really try to make the WebKit API
just be a thin wrapper for WebCore.  You might as well expose WebIntent since
it can easily wrap WebCore::Intent as that is also ref-counted.  This way as
these objects grow, you don't have to have the WebKit API concepts differ from
the WebCore concepts.  Past experience suggests that keeping these aligned
helps.

> Source/WebKit/chromium/src/WebIntentRequest.cpp:86
> +void WebIntentRequest::postResult(const WebKit::WebString& data)

nit: postResult and postFailure should take WebSerializedScriptValue.  this
will help when you want to post more than just a string, which i'm sure we will
want to do.  it also will reduce the code for the WebKit layer, which is always
a good thing.

> Source/WebKit/chromium/src/WebIntentRequest.cpp:99
> +    if (m_intentRequest.get())

it does not look possible to create a WebIntentRequest that has a null
m_intentRequest.  why are you null checking?  just ASSERT or let it crash.