[webkit-reviews] review denied: [Bug 84220] Implement object-literal constructor for Intent object : [Attachment 138817] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Greg Billock
<gbillock at google.com>'s request for review:
Bug 84220: Implement object-literal constructor for Intent object
https://bugs.webkit.org/show_bug.cgi?id=84220

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

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=138817&action=review


Sorry for the iterative comments. Almost r+.

> Source/WebCore/ChangeLog:9
> +	   This will hopefully be temporary, as we'll convert to just using
this
> +	   constructor, which can then use codegen.

Please update the description. It is a good idea to add the link to the spec
that supports your change.
http://dvcs.w3.org/hg/web-intents/raw-file/tip/spec/Overview.html ?

> Source/WebCore/ChangeLog:11
> +	   Added layout tests for object constructor:
> +	   webintents/web-intent-obj-constructor.html

Our convention is to write

	Test: webintents/web-intent-obj-constructor.html

Please look at other ChangeLogs.

> Source/WebCore/Modules/intents/Intent.cpp:60
> +PassRefPtr<Intent> Intent::create(ScriptState* scriptState, const
Dictionary& options, ExceptionCode& ec)

How about removing scriptState from the method arguments, and call
ScriptState::current() just before dataValue.serialize()?

> Source/WebCore/Modules/intents/Intent.cpp:77
> +    options.get("service", service);
> +    if (!service.isEmpty()) {

if (options.get("service", service) && !service.isEmpty())

> Source/WebKit/chromium/src/WebIntent.cpp:139
> +    for (int i = 0; keyIter != m_private->extras().end().keys(); ++keyIter,
++i)

Nit: int => size_t?

> LayoutTests/webintents/web-intents-obj-constructor.html:43
> +	   shouldNotThrow("new
WebKitIntent({'action':'a','type':'b','service':''})");

>> The same comment for the following tests. (Though I know the number of tests
increases.)
>>
> Currently the spec doesn't have accessors for |service| and |extras|, so I've
left them as is.

It seems the spec has service and extras as a readonly attribute:
http://dvcs.w3.org/hg/web-intents/raw-file/tip/spec/Overview.html

Maybe you need to add service and extras to Intent.idl?

At least, given that you implemented accessors for service and extras in this
patch, we should test them. Otherwise, we might want to remove accessors for
service and extras from this patch for now.