Bug 61343

Created attachment 94569 [details] Patch

First tentative to make __qt_sender__ implementation less dependent on JSC internals. This implementation is less powerful than injecting the scope, but seems to me that the biggest use case is covered. In the long term, I think we should deprecate '__qt_sender__'. Noam, I preferred to make this a separate change instead of adding more to bridge-in-jsc. If we end up landing this, it is easy to rebase the branch later.

You should probably mention in the changelog that this is already tested in tst_qwebframe. Otherwise it looks perfect to me.

Comment on attachment 94569 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94569&action=review > Source/WebCore/bridge/qt/qt_instance.h:79 > + void pop() { Q_ASSERT(!m_stack.isEmpty()); m_stack.pop(); } Curious to have a stack with a pop() method that doesn't return the popped value. No real objection though. > Source/WebKit/qt/Api/qwebframe.cpp:496 > + // ### Is this the correct RootObject? This sounds like something that should be answered before landing. > Source/WebKit/qt/Api/qwebframe.cpp:516 > + // ### Can we do this with public JSC API? Ditto. > Source/WebKit/qt/Api/qwebframe_p.h:111 > + void didClearedWindowObject(); Should be didClearWindowObject(). > Source/WebKit/qt/Api/qwebframe_p.h:134 > + void addQtSenderToGlobalObject(); Missing #if USE(JSC) guard.

Created attachment 94626 [details] Patch v2, updates after kling comments

(In reply to comment #4) > (From update of attachment 94569 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94569&action=review > > > Source/WebCore/bridge/qt/qt_instance.h:79 > > + void pop() { Q_ASSERT(!m_stack.isEmpty()); m_stack.pop(); } > > Curious to have a stack with a pop() method that doesn't return the popped value. No real objection though. Since we don't use the value, I think it's fine keep it as is. If someone needs the return value from pop() it's easy to update it. > > Source/WebKit/qt/Api/qwebframe.cpp:496 > > + // ### Is this the correct RootObject? > > This sounds like something that should be answered before landing. Agreed. I'll look into that. Noam, Oliver, is getting the RootObject from the dynamicGlobalObject a sane move here? > > Source/WebKit/qt/Api/qwebframe.cpp:516 > > + // ### Can we do this with public JSC API? > > Ditto. Fixed. Commented that JSC API still can't do this as an explanation why I needed to use internals. > > Source/WebKit/qt/Api/qwebframe_p.h:111 > > + void didClearedWindowObject(); > > Should be didClearWindowObject(). Fixed. > > Source/WebKit/qt/Api/qwebframe_p.h:134 > > + void addQtSenderToGlobalObject(); > > Missing #if USE(JSC) guard. Fixed.

Comment on attachment 94569 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94569&action=review > Source/WebKit/qt/Api/qwebframe.cpp:497 > + RefPtr<JSC::Bindings::RootObject> rootObject = JSC::Bindings::findRootObject(exec->dynamicGlobalObject()); i recommend asking anders or weinig as i don't know how the binding objects work > Source/WebKit/qt/Api/qwebframe.cpp:518 > + window->defineGetter(exec, propertyName.get()->identifier(&exec->globalData()), ::toJS(function), > + JSC::ReadOnly | JSC::DontEnum | JSC::DontDelete); file a bug on the inability to use JSC API to do this.

(In reply to comment #7) > (From update of attachment 94569 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94569&action=review > > > Source/WebKit/qt/Api/qwebframe.cpp:497 > > + RefPtr<JSC::Bindings::RootObject> rootObject = JSC::Bindings::findRootObject(exec->dynamicGlobalObject()); > > i recommend asking anders or weinig as i don't know how the binding objects work Thanks. > > Source/WebKit/qt/Api/qwebframe.cpp:518 > > + window->defineGetter(exec, propertyName.get()->identifier(&exec->globalData()), ::toJS(function), > > + JSC::ReadOnly | JSC::DontEnum | JSC::DontDelete); > > file a bug on the inability to use JSC API to do this. Created bug 61374 for that.

For the record, from IRC: <cmarcelo> andersca: hello. can you help me understand RootObject (JSC::Bindings)? I'm working on bug https://bugs.webkit.org/show_bug.cgi?id=61343 and am unsure whether I'm doing the right thing there when getting the RootObject. <andersca> cmarcelo: it looks OK to me

Created attachment 94660 [details] Patch v3, remove RootObject comment, add bug URL to API comment

Comment on attachment 94660 [details] Patch v3, remove RootObject comment, add bug URL to API comment View in context: https://bugs.webkit.org/attachment.cgi?id=94660&action=review Looks Ok > Source/WebCore/bridge/qt/qt_instance.cpp:329 > +QtInstance::QtSenderStack* QtInstance::qtSenderStack() > +{ > + static QtSenderStack senderStack; > + return &senderStack; > +} > + There are two genaral issues here (feel free to ignore them): 1. It breaks reentrancy, the object can't be shared between threads. Problably you can ignore that if you are sure that there is no multiple thread access (from log message). 2. In Qt we do not like static global, non POD variables. They introduce performance problems (compiler need to initialize most statics at the library load time) and in multithread environment initialization order is not really specified. I would suggest to use Q_GLOBAL_STATIC macro.

Created attachment 94805 [details] Patch

(In reply to comment #11) > 1. It breaks reentrancy, the object can't be shared between threads. Problably you can ignore that if you are sure that there is no multiple thread access (from log message). Yes. There'll be no multiple thread access in this case. > 2. In Qt we do not like static global, non POD variables. They introduce performance problems (compiler need to initialize most statics at the library load time) and in multithread environment initialization order is not really specified. I would suggest to use Q_GLOBAL_STATIC macro. Fixed.

Comment on attachment 94805 [details] Patch Shazam!

Comment on attachment 94805 [details] Patch Clearing flags on attachment: 94805 Committed r87315: <http://trac.webkit.org/changeset/87315>

All reviewed patches have been landed. Closing bug.