Add a helper function to expose PagePopupClient::setValueAndClosePopup() to JavaScript
Created attachment 136425 [details] Patch
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()
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.
(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.
(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().
(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.
Created attachment 136586 [details] Patch 2 Fix typos in comments
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.
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.
Created attachment 136645 [details] Patch for landing
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.
Comment on attachment 136645 [details] Patch for landing Clearing flags on attachment: 136645 Committed r113846: <http://trac.webkit.org/changeset/113846>
All reviewed patches have been landed. Closing bug.