[Webkit-unassigned] [Bug 73051] [Web Intents] Flagged-off WebCore implementation of navigator.startActivity

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=73051


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #116428|review?                     |review-
               Flag|                            |




--- Comment #5 from Adam Barth <abarth at webkit.org>  2011-11-23 18:19:27 PST ---
(From update of attachment 116428)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.