[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





--- Comment #3 from Dominic Cooney <dominicc at chromium.org>  2011-11-23 18:04:40 PST ---
(From update of attachment 116428)
View in context: https://bugs.webkit.org/attachment.cgi?id=116428&action=review

I am super excited about Web Intents being available in WebKit! I have made some suggestions inline.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Probably worth either:

- adding tests
- commenting why no new tests are required

> Source/WebCore/ChangeLog:9
> +

It might be useful to add a link to the draft spec of what you’re implementing here.

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

FYI This has to be a custom method for now, but haraken is working on making this unnecessary using Constructor(…) metadata. I think the only thing blocking you is that Constructor attribute does not support overloads yet.

> Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp:55
> +        fprintf(stderr, "Creating empty intent\n");

Probably want to remove this ad-hoc logging?

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

Code generation will *almost* do this for you. I believe that the only difference is that it would give null for empty script values, whereas you give undefined. How does specing returning undefined line up with other DOM attributes that are SerializedScriptValue? If others return null, you should just use generation. If others return undefined, you might consider teaching the code generators about SerializedScriptValue?.

> Source/WebCore/loader/FrameLoaderClient.h:83
>      class IntSize;

Isn’t S < e?

> Source/WebCore/loader/FrameLoaderClient.h:327
> +        virtual void dispatchIntent(const Intent& intent, int id) { }

The "intent" parameter name does not add anything that isn’t in the type, so omit the name. id is OK, natch.

> Source/WebCore/page/Intent.cpp:38
> +Intent::Intent()

Why not simply have one constructor and put the default values in ::create overloads?

> Source/WebCore/page/Intent.cpp:52
> +    setAction(action);

It might be more typical in WebKit to use : m_action(action), … style initializers… why call the setters?

> Source/WebCore/page/Intent.cpp:82
> +void Intent::setData(PassRefPtr<SerializedScriptValue> prpData)

I believe the WebKit style guide on naming parameters prpFoo is that prpFoo is an acceptable prefix when the use of the parameter is non-trivial and to that end it is being transferred to a RefPtr at the start of the function. That isn’t what is happening here.

> Source/WebCore/page/Intent.cpp:87
> +      m_data = SerializedScriptValue::nullValue();

This needs to be indented two more spaces.

> Source/WebCore/page/Intent.idl:33
> +        readonly attribute DOMString action;

Why have all of the setters in C++ then?

> Source/WebCore/page/IntentResultCallback.idl:32
> +        boolean handleEvent(in SerializedScriptValue result);

This looks to be indented too far… I think IDL just indents four.

> Source/WebCore/page/IntentsController.h:44
> +    int getIntentId(PassRefPtr<IntentResultCallback> successCallback, PassRefPtr<IntentResultCallback> errorCallback);

It seems odd to me that a method named *get*-something to not be idempotent. Why not call it

assignIntentId

or something to hint at its side-effecting nature?

> Source/WebCore/page/Navigator.cpp:331
> +        ec = VALIDATION_ERR;

FYI DOM4 seems to deprecate this: <http://www.w3.org/TR/domcore/#dom-domexception-validation_err>

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