[webkit-reviews] review denied: [Bug 84220] Implement object-literal constructor for Intent object : [Attachment 137812] 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 137812: Patch
https://bugs.webkit.org/attachment.cgi?id=137812&action=review

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


Please add more test cases. As commented below, we need to confirm the behavior
when we pass various parameters to the Intent constructor. (Please look at
existing tests in fast/events/constructors/.)

> Source/WebCore/Modules/intents/Intent.cpp:67
> +    String action;
> +    bool actionExists = parameters.getWithUndefinedOrNullCheck("action",
action);
> +    String type;
> +    bool typeExists = parameters.getWithUndefinedOrNullCheck("type", type);
> +    String service;
> +    parameters.get("service", service);

Is this behavior speced? (i.e. what should happen when we pass null or
undefined?)
 
Anyway please add test cases for all possible parameters.

> Source/WebCore/Modules/intents/Intent.cpp:71
> +    if (!actionExists || action.isEmpty()) {
>	   ec = SYNTAX_ERR;
>	   return 0;

Ditto.

> Source/WebCore/Modules/intents/Intent.cpp:75
> +    if (!typeExists || type.isEmpty()) {
>	   ec = SYNTAX_ERR;
>	   return 0;

Ditto.

> Source/WebCore/Modules/intents/Intent.idl:31
> +	 Constructor(in Dictionary parameters),

Remove this. This IDL attribute makes no sense under [CustomConstructor].

> Source/WebCore/bindings/v8/Dictionary.cpp:101
> -    if (!options->Has(v8Key))
> +    if (!options->Has(v8Key->ToString()))

What is the rational for this change?

> Source/WebCore/bindings/v8/Dictionary.cpp:437
> +	 if (key.IsEmpty() || !key->Length() || !wrapper->Has(key))

Can't this be just 'if (!wrapper->Has(key))'?

> Source/WebCore/bindings/v8/custom/V8IntentConstructor.cpp:86
> +    if (ec)
> +	   return throwError(ec);

Nit: Move these two lines to just below 'RefPtr<Intent> impl = ...'.