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

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


First round of comments.

> Source/WebCore/Modules/intents/Intent.cpp:63
> +    bool actionExists = parameters.getWithUndefinedOrNullCheck("action",
action);

Why do you use getWithUndefinedOrNullCheck() for action and type, but use get()
for service? Is it speced?

> Source/WebCore/Modules/intents/Intent.cpp:68
> +    if (parameters.get("service", service)
> +	   && (service.isNull() || service.isEmpty())) {

This condition ('service.isNull() || service.isEmpty()') sounds strange. Is it
speced? We need to take case how Empty, Null and Undefined should be treated.
Also please add test cases for Empty, Null and Undefined.

Nit: No line break please.

> Source/WebCore/Modules/intents/Intent.cpp:73
> +    if (!actionExists || action.isEmpty()) {

Nit: actionExists would not be necessary. 'if (!paramters.get("action", action)
|| action.isEmpty)'.

> Source/WebCore/Modules/intents/Intent.cpp:77
> +    if (!typeExists || type.isEmpty()) {

Ditto.

> Source/WebCore/Modules/intents/Intent.cpp:85
> +    if (portsExists)

Ditto.

> Source/WebCore/Modules/intents/Intent.cpp:91
> +    if (dataExists) {

Ditto.

> Source/WebCore/Modules/intents/Intent.cpp:95
> +	       ec = DATA_CLONE_ERR;

Please add a test case for this exception.

> Source/WebCore/Modules/intents/Intent.cpp:103
> +    if (extrasExists)

Ditto. extrasExists would not be necessary.

> Source/WebCore/Modules/intents/Intent.h:72
> +

Nit: No line break please.

> Source/WebCore/Modules/intents/Intent.h:75
> +

Ditto.