Bug 61343

Summary: [Qt] JSC bridge: implement __qt_sender__ without using Scope Chain
Product: WebKit Reporter: Caio Marcelo de Oliveira Filho <cmarcelo>
Component: WebKit QtAssignee: Caio Marcelo de Oliveira Filho <cmarcelo>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, kling, noam, oliver
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
kling: review-
Patch v2, updates after kling comments
none
Patch v3, remove RootObject comment, add bug URL to API comment
none
Patch none

Description Caio Marcelo de Oliveira Filho 2011-05-23 22:30:14 PDT
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.
Comment 1 Caio Marcelo de Oliveira Filho 2011-05-23 23:00:05 PDT
Created attachment 94569 [details]
Patch
Comment 2 Caio Marcelo de Oliveira Filho 2011-05-23 23:07:41 PDT
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.
Comment 3 Noam Rosenthal 2011-05-24 06:56:06 PDT
You should probably mention in the changelog that this is already tested in tst_qwebframe.
Otherwise it looks perfect to me.
Comment 4 Andreas Kling 2011-05-24 08:50:00 PDT
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.
Comment 5 Caio Marcelo de Oliveira Filho 2011-05-24 09:27:48 PDT
Created attachment 94626 [details]
Patch v2, updates after kling comments
Comment 6 Caio Marcelo de Oliveira Filho 2011-05-24 09:30:23 PDT
(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 7 Oliver Hunt 2011-05-24 09:40:34 PDT
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.
Comment 8 Caio Marcelo de Oliveira Filho 2011-05-24 11:05:14 PDT
(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.
Comment 9 Caio Marcelo de Oliveira Filho 2011-05-24 12:09:16 PDT
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
Comment 10 Caio Marcelo de Oliveira Filho 2011-05-24 12:10:15 PDT
Created attachment 94660 [details]
Patch v3, remove RootObject comment, add bug URL to API comment
Comment 11 Jędrzej Nowacki 2011-05-25 07:43:28 PDT
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.
Comment 12 Caio Marcelo de Oliveira Filho 2011-05-25 10:29:49 PDT
Created attachment 94805 [details]
Patch
Comment 13 Caio Marcelo de Oliveira Filho 2011-05-25 10:31:04 PDT
(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 14 Andreas Kling 2011-05-25 10:34:58 PDT
Comment on attachment 94805 [details]
Patch

Shazam!
Comment 15 WebKit Commit Bot 2011-05-25 12:28:33 PDT
Comment on attachment 94805 [details]
Patch

Clearing flags on attachment: 94805

Committed r87315: <http://trac.webkit.org/changeset/87315>
Comment 16 WebKit Commit Bot 2011-05-25 12:28:39 PDT
All reviewed patches have been landed.  Closing bug.