Bug 83561 – [V8] Calendar Picker: Add a helper function to expose PagePopupClient::setValueAndClosePopup() to JavaScript

• Component: New Bugs
• Status: RESOLVED FIXED

[Kent Tamura]

Add a helper function to expose PagePopupClient::setValueAndClosePopup() to JavaScript

[Kent Tamura]

Created attachment 136425 [details]
Patch

[Kentaro Hara]

Comment on attachment 136425 [details]
Patch

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()

[Kent Tamura]

Comment on attachment 136425 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136425&action=review

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

Yes, it is.
Both of toNative(args.Holder()) and toNative(args.This()) didn't work.  They crashed in V8DOMWindow::toNative().
I don't understand what's 'Holder', but probably the function registration to context->Global() is not compatible with V8DOMWindow::(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)).

This is ok.  All of the code calling setValueAndClosePopup() is controllable.  The code will be provided by WebCore.  e.g. WebCore/Resource/calendarPicker.js.

[Kentaro Hara]

(In reply to comment #3)
> >> Source/WebCore/bindings/v8/ScriptController.cpp:249
> >> +    DOMWindow* imp = V8DOMWindow::toNative(args.Data()->ToObject());
> > 
> > Is this correct? Maybe toNative(args.Holder())?
> 
> Yes, it is.
> Both of toNative(args.Holder()) and toNative(args.This()) didn't work.  They crashed in V8DOMWindow::toNative().
> I don't understand what's 'Holder', but probably the function registration to context->Global() is not compatible with V8DOMWindow::(args.Holder()).

args.Holder() points to the object on which the method is defined; i.e. in case of div.appendChild(), args.Holder() points to the prototype object of Node, where appendChild() is defined. So in this case, as you mentioned, args.Holder() is not correct here. But I don't understand what args.Data() represents. abarth@ might know it well...

> >> 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)).
> 
> This is ok.  All of the code calling setValueAndClosePopup() is controllable.  The code will be provided by WebCore.  e.g. WebCore/Resource/calendarPicker.js.

Makes sense.

[Kent Tamura]

(In reply to comment #4)
> (In reply to comment #3)
> > >> Source/WebCore/bindings/v8/ScriptController.cpp:249
> > >> +    DOMWindow* imp = V8DOMWindow::toNative(args.Data()->ToObject());
> > > 
> > > Is this correct? Maybe toNative(args.Holder())?
> > 
> > Yes, it is.
> > Both of toNative(args.Holder()) and toNative(args.This()) didn't work.  They crashed in V8DOMWindow::toNative().
> > I don't understand what's 'Holder', but probably the function registration to context->Global() is not compatible with V8DOMWindow::(args.Holder()).
> 
> args.Holder() points to the object on which the method is defined; i.e. in case of div.appendChild(), args.Holder() points to the prototype object of Node, where appendChild() is defined. So in this case, as you mentioned, args.Holder() is not correct here. But I don't understand what args.Data() represents. abarth@ might know it well...

args.Data() is the second argument of FunctionTemplate::New().

> v8::Local<v8::FunctionTemplate> templ = v8::FunctionTemplate::New(setValueAndClosePopupCallback, V8DOMWindow::wrap(frame->existingDOMWindow()));

Probably I should register the function to V8DOMWindow, not context->Global().

[Kent Tamura]

(In reply to comment #5)
> > v8::Local<v8::FunctionTemplate> templ = v8::FunctionTemplate::New(setValueAndClosePopupCallback, V8DOMWindow::wrap(frame->existingDOMWindow()));
> 
> Probably I should register the function to V8DOMWindow, not context->Global().

I tried:
A) V8DOMWindow::wrap(window)->Set(v8::String::New("setValueAndClosePopup"), ...) and imp = V8DOMWindow::toNative(args.Holder())
B) V8DOMWindow::wrap(window)->Set(v8::String::New("setValueAndClosePopup"), ...) and imp = V8DOMWindow::toNative(args.This())
C) V8DOMWindow::wrap(window)->GetPrototype()->ToObject()->Set(v8::String::New("setValueAndClosePopup"), ...) and imp = V8DOMWindow::toNative(args.Holder())
D) V8DOMWindow::wrap(window)->GetPrototype()->ToObject()->Set(v8::String::New("setValueAndClosePopup"), ...) and imp = V8DOMWindow::toNative(args.This())

All of them called setValueAndClosePopupCallback(), but crashed.

[Kent Tamura]

Created attachment 136586 [details]
Patch 2

Fix typos in comments

[Adam Barth]

Comment on attachment 136586 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=136586&action=review

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

What if this stuff throws an exception?  This feels like a strange place for custom bindings.  Is there really no way for the code generator to do this work for us?

> Source/WebCore/page/DOMWindowPagePopupClient.cpp:45
> +const AtomicString& DOMWindowPagePopupClient::name()

I think we usually call this supplementName()

> Source/WebCore/page/DOMWindowPagePopupClient.h:42
> +class DOMWindowPagePopupClient : public Supplement<DOMWindow> {

This name seems to imply that this class is a client, but it's not really.  Maybe DOMWindowPagePopup is a better name?

> Source/WebCore/page/DOMWindowPagePopupClient.h:44
> +    static void setValueAndClosePopup(DOMWindow*, int intValue, const String& stringValue);

intValue and stringValue are kind of meaningless variable names.  What do these values represent?

> Source/WebCore/page/DOMWindowPagePopupClient.h:48
> +    DOMWindowPagePopupClient(PagePopupClient*);

One-argument constructors should have the explicit keyword.

[Adam Barth]

Comment on attachment 136586 [details]
Patch 2

Ok.  I've warmed up to this patch.  If you're willing to fix the nits above and the exception handling, then I think this is fine.

[Kent Tamura]

Created attachment 136645 [details]
Patch for landing

[Kent Tamura]

Thank you for the review.

(In reply to comment #8)
> > +    DOMWindowPagePopupClient::setValueAndClosePopup(imp, toInt32(args[0]), toWebCoreString(args, 1));
> 
> What if this stuff throws an exception?  This feels like a strange place for custom bindings.  Is there really no way for the code generator to do this work for us?

I have added exception handlings for arguments.
I have no good idea to use the code generator. If we do it, we need to check a per-page or per-window setting in V8DOMWindow::ConfigureV8DOMWindowTemplate().  It looks very difficult.

> > Source/WebCore/page/DOMWindowPagePopupClient.cpp:45
> > +const AtomicString& DOMWindowPagePopupClient::name()
> I think we usually call this supplementName()

Renamed.

> > Source/WebCore/page/DOMWindowPagePopupClient.h:42
> > +class DOMWindowPagePopupClient : public Supplement<DOMWindow> {
> 
> This name seems to imply that this class is a client, but it's not really.  Maybe DOMWindowPagePopup is a better name?

Renamed.

> > Source/WebCore/page/DOMWindowPagePopupClient.h:44
> > +    static void setValueAndClosePopup(DOMWindow*, int intValue, const String& stringValue);
> 
> intValue and stringValue are kind of meaningless variable names.  What do these values represent?

We can't know their meanings because they are defined by a PagePopupClient implementation.  For the calendar picker, the integer value means -1:cancel and 0:set the string value.  Other PagePopupClient implementations will have different meanings.

> > Source/WebCore/page/DOMWindowPagePopupClient.h:48
> > +    DOMWindowPagePopupClient(PagePopupClient*);
> 
> One-argument constructors should have the explicit keyword.

Added.

[WebKit Review Bot]

Comment on attachment 136645 [details]
Patch for landing

Clearing flags on attachment: 136645

Committed r113846: <http://trac.webkit.org/changeset/113846>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.