[webkit-reviews] review denied: [Bug 83561] [V8] Calendar Picker: Add a helper function to expose PagePopupClient::setValueAndClosePopup() to JavaScript : [Attachment 136425] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Kent Tamura
<tkent at chromium.org>'s request for review:
Bug 83561: [V8] Calendar Picker: Add a helper function to expose
PagePopupClient::setValueAndClosePopup() to JavaScript
https://bugs.webkit.org/show_bug.cgi?id=83561

Attachment 136425: Patch
https://bugs.webkit.org/attachment.cgi?id=136425&action=review

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


I am not familiar with the DOMWindowPagePopup part, and just commented on the
V8 bindings part.

> Source/WebCore/bindings/v8/ScriptController.cpp:249
> +    DOMWindow* imp = V8DOMWindow::toNative(args.Data()->ToObject());

Is this correct? Maybe toNative(args.Holder())?

> Source/WebCore/bindings/v8/ScriptController.cpp:250
> +    DOMWindowPagePopupClient::setValueAndClosePopup(imp, toInt32(args[0]),
toWebCoreString(args, 1));

Just a confirmation: If args[1] is undefined, toWebCoreString(args, 1) will
convert it to a WebCore string "undefined". Is it expected? If you want to
convert it to an empty WebCore string, you can write
toWebCoreString(MAYBE_MISSING_PARAMETER(args, 1, DefaultIsNullString)).

> Source/WebCore/bindings/v8/ScriptController.cpp:251
> +    // setValeuAndClosePopup() deletes the window. Do not access it.

Nit: setVal*ue*ClosePopup()

> Source/WebCore/page/DOMWindowPagePopupClient.cpp:56
> +    // setValeuAndClosePopup() deletes the window and this object. Do not
access them.

Nit: setVal*ue*AndClosePopup()