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

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


> Source/WebCore/Modules/intents/Intent.cpp:63
> +    if (!parameters.getWithUndefinedOrNullCheck("action", action) ||
action.isEmpty()) {

> > Why do you use getWithUndefinedOrNullCheck() for action and type, but use
get() for service? Is it speced?
> 
> It's speced that action/type must be present. (Service is optional.)

- 'must be present' and 'undefined or null' are different things. Please refer
to http://www.w3.org/TR/WebIDL/#es-DOMString

{type: undefined} => present. type is treated as a string "undefined".
{type: null} => present. type is treated as a string "null".
{type: ""} => present. type is treated as "".
{} => not present. type is treated as a null string.

Unless the spec explicitly says that undefined or null should be treated as a
null string, we should not use parameters.getUndefinedOrNullCheck().

- I think you do not need to check action.isEmpty() here. Isn't it the work of
Intent::create()?

- As I commented repeatedly, please add test cases for these. Missing type,
type = undefined, type = null, type = "". Same for action, service, extras etc.
Please look at other constructor test cases in fast/events/constructors/*. As
we have discussed, we often mis-implement the behavior for these corner cases.

> Source/WebCore/Modules/intents/Intent.cpp:69
> +    if (!parameters.getWithUndefinedOrNullCheck("type", type) ||
type.isEmpty()) {

Ditto.

> Source/WebCore/Modules/intents/Intent.cpp:77
> +    if (parameters.get("service", service) && (service.isNull() ||
service.isEmpty())) {
> +	   ec = SYNTAX_ERR;
> +	   return 0;

> If service is present, it's speced to be a URL, so a null or empty value is a
caller error. Is there a way to be more explicit with the message?

The check should be service.isEmpty(). String::isNull() returns true if the
string is null (i.e. not initialized). String::isEmpty() returns true if the
string is null or an empty string. Thus if isEmpty() is true, then isNull() is
true.

But I think that the check should be done in Intent::create().

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

Please add a test case for this.

> Source/WebCore/bindings/v8/custom/V8IntentConstructor.cpp:66
> +	   return toV8(impl.release(), args.Holder());

I would guess that V8Proxy::toV8() no longer exists (since I removed it:-)
Please use setJSWrapperForDOMObject(), just like you are doing below.

> Source/WebCore/bindings/v8/custom/V8IntentConstructor.cpp:81
> +	   return v8::Undefined();

Shouldn't we return some exception? (I am not sure though.)