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

Tue Apr 24 23:21:35 PDT 2012

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 138629: Patch
https://bugs.webkit.org/attachment.cgi?id=138629&action=review

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

Almost looks OK except for test cases.

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

Nit: Shall we rename 'parameters' to 'options', since you are using 'options'
in the caller side?

> Source/WebCore/Modules/intents/Intent.cpp:137
> +const KURL& Intent::service() const
> +{
> +    return m_service;
> +}

Nit: you can put this code in the header.

> Source/WebCore/Modules/intents/Intent.cpp:152
> +const WTF::HashMap<String, String>& Intent::extras() const
> +{
> +    return m_extras;
> +}

Nit: you can put this code in the header.

> Source/WebCore/bindings/v8/Dictionary.cpp:432
> +    if (isUndefinedOrNull())
> +	   return false;
> +    if (!m_options->IsObject())

You can write:

    if (!isObject())
	return false;

> Source/WebCore/bindings/v8/custom/V8IntentConstructor.cpp:67
> +	   V8DOMWrapper::setDOMWrapper(args.Holder(), &info, impl.get());

Nit: args.Holder() => wrapper.

> Source/WebCore/bindings/v8/custom/V8IntentConstructor.cpp:69
> +	   return args.Holder();

Nit: args.Holder() => wrapper.

> Source/WebCore/bindings/v8/custom/V8IntentConstructor.cpp:93
> +    return args.Holder();

Nit: args.Holder() => wrapper.

> LayoutTests/webintents/web-intents-obj-constructor.html:2
> +  <head>

Nit: 4 space indent please. Or no indent is OK. WebKit does not use 2 space
indent.

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

Instead, we might write the test like this:

shouldBeEqualToString('(new WebKitIntent({"action":"a","type":"b"})).action',
'a');
shouldBeEqualToString('(new WebKitIntent({"action":"a","type":"b"})).type',
'b');

The same comment for the following tests. (Though I know the number of tests
increases.)