[Qt] JSC bridge: implement __qt_sender__ without using Scope Chain

[Caio Marcelo de Oliveira Filho]

When JS functions are called as result of signal emission, one can evaluate __qt_sender__ to obtain the object that emitted the signal. This is similar as QObject::sender() function.

To implement that the current bridge code manipulates the Scope Chain to add a new node with '__qt_sender__' inside. Oliver pointed out that there are problems with this implementation, and it breaks internal assumptions about scope chain that JSC can use for optimizations.

[Caio Marcelo de Oliveira Filho]

Created attachment 94569 [details]
Patch

[Caio Marcelo de Oliveira Filho]

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.

[Noam Rosenthal]

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

[Andreas Kling]

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.

[Caio Marcelo de Oliveira Filho]

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

[Caio Marcelo de Oliveira Filho]

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

[Oliver Hunt]

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.

[Caio Marcelo de Oliveira Filho]

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

[Caio Marcelo de Oliveira Filho]

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

[Caio Marcelo de Oliveira Filho]

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

[Jędrzej Nowacki]

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.

[Caio Marcelo de Oliveira Filho]

Created attachment 94805 [details]
Patch

[Caio Marcelo de Oliveira Filho]

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

[Andreas Kling]

Comment on attachment 94805 [details]
Patch

Shazam!

[WebKit Commit Bot]

Comment on attachment 94805 [details]
Patch

Clearing flags on attachment: 94805

Committed r87315: <http://trac.webkit.org/changeset/87315>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.