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

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


Before looking into more details, maybe we want to confirm that the code works
as expected (i.e. want test cases).

>> Source/WebCore/ChangeLog:8
>> +	    No new tests. (OOPS!)
> 
> You should remove the 'No new tests' and either add and list tests, or
explain why no new tests were possible.  [changelog/nonewtests] [5]

Please add test cases in fast/events/constructors/. (Please look at other tests
in that directory.)

> Source/WebCore/ChangeLog:17
> +	   * Modules/intents/DeliveredIntent.h: Copied from
Source/WebCore/Modules/intents/Intent.h.
> +	   (WebCore):
> +	   (DeliveredIntentClient):
> +	   (WebCore::DeliveredIntentClient::~DeliveredIntentClient):
> +	   (WebCore::DeliveredIntentClient::postResult):
> +	   (WebCore::DeliveredIntentClient::postFailure):
> +	   (DeliveredIntent):
> +	   (WebCore::DeliveredIntent::~DeliveredIntent):

This ChangeLog seems wrong.

> Source/WebCore/bindings/v8/Dictionary.cpp:422
> +    v8::Local<v8::Value> v8Value;
> +    if (!getKey(key, v8Value))
> +	   return false;
> +
> +    if (v8Value->IsObject())
> +	   value = Dictionary(v8Value);

I am not sure if this implementation is correct. Could you add test cases as I
mentioned above?