[webkit-reviews] review denied: [Bug 73051] [Web Intents] Flagged-off WebCore implementation of navigator.startActivity : [Attachment 116428] 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: [Web Intents] Flagged-off WebCore implementation of
navigator.startActivity
https://bugs.webkit.org/show_bug.cgi?id=73051

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

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


> Source/WebCore/WebCore.gypi:2972
> +	       'page/Intent.cpp',
> +	       'page/Intent.h',
> +	       'page/IntentsController.cpp',
> +	       'page/IntentsController.h',
> +	       'page/IntentResultCallback.h',

Can you make an intents directory in Modules?  "page" shouldn't really be a
dumping group for every random feature.

> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:111
> +    SerializedScriptValue* ssv = impl->data();

ssv => please use complete words in variable names.

> Source/WebCore/page/DOMWindow.idl:622
> +#if defined(ENABLE_WEB_INTENTS) && ENABLE_WEB_INTENTS
> +	   attribute IntentConstructor Intent; // Usable with the new operator
> +#endif

This is an ideal candidate for Supplemental (which doesn't yet exist).

> Source/WebCore/page/Intent.h:50
> +    static PassRefPtr<Intent> create(
> +	   const String& action,
> +	   const String& type) {

One line please.

> Source/WebCore/page/Intent.h:71
> +    Intent();
> +    Intent(String action, String type);
> +    Intent(String action, String type, PassRefPtr<SerializedScriptValue>
prpData);

Can these all be combined with default values?

> Source/WebCore/page/Navigator.idl:72
> +#if defined(ENABLE_WEB_INTENTS) && ENABLE_WEB_INTENTS
> +	   void startActivity(in Intent intent, in [Callback, Optional]
IntentResultCallback successCallback, in [Callback, Optional]
IntentResultCallback failureCallback)
> +	       raises(DOMException);
> +#endif

Another good candidate for Supplemental.

> Source/WebCore/page/Page.cpp:149
> +#if ENABLE(WEB_INTENTS)
> +    , m_intentsController(adoptPtr(new IntentsController()))
> +#endif

Why is this controller per-page?  Naively, I would have expected it to be
per-ScriptExecutionContext.