[webkit-reviews] review denied: [Bug 73051] Implement navigator.startActivity for WebIntents : [Attachment 121006] 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 73051: Implement navigator.startActivity for WebIntents
https://bugs.webkit.org/show_bug.cgi?id=73051

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=121006&action=review


This looks great.  We just need to remove the custom bindings code.

> Source/WebCore/ChangeLog:3
> +	   [Web Intents] Flagged-off WebCore implementation of
navigator.startActivity

It would be nice to update this text to match the updated name of the bug.

> Source/WebKit/chromium/ChangeLog:5153
> +	   * public/WebIntentsClient.h: Added.
> +	   (WebKit::WebIntentsClient::~WebIntentsClient):
> +	   * public/WebIntentsController.h: Added.
> +	   * public/WebViewClient.h:
> +	   (WebKit::WebViewClient::webIntentsClient):
> +	   * src/WebIntentsController.cpp: Added.
> +	   (WebKit::WebIntentsController::webIntentReply):

These changes look spurious

> Source/WebCore/Modules/intents/Intent.cpp:61
> +void Intent::setData(PassRefPtr<SerializedScriptValue> data)

Should this just be a SerializedScriptValue* ?	This function doesn't take
ownership of the data.

> Source/WebCore/Modules/intents/Intent.cpp:63
> +    if (data.get())

No need for ".get()".  PassRefPtr has a bool operator.

> Source/WebCore/Modules/intents/Intent.idl:31
> +	 CustomConstructFunction,
> +	 V8CustomConstructor,

Why does this need a custom constructor?

> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:45
> +v8::Handle<v8::Value> V8Intent::constructorCallback(const v8::Arguments&
args)

This function is full of bugs.	We should just auto-generate it.

> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:94
> +v8::Handle<v8::Value> V8Intent::dataAccessorGetter(v8::Local<v8::String>
name, const v8::AccessorInfo& info)

This should be auto-generated as well.