Bug 116408 – [Qt] Support Object construction for window properties added using QtWebKitBridge

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Arunprasad Rajkumar]

Native QObject can be added as a window property using QtWebKit Bridge(QWebFrame::addToJavaScriptWindowObject).

eg) 
// native
m_page->mainFrame()->addToJavaScriptWindowObject("Foo", foo); // where test is QObject*

// javascript
Foo.doOpeartion1(),..

Currently Object construction on qt's window property is not supported directly as like below,

eg) 
//javascript
var obj = new Foo();

Though it might done using script injection, but it needs considerable effort when there were more of objects. This could be simplified by implementing object construction using Instance::invokeConstruct(...).

Then idea is properties which needs construction will be added to window using addToJavaScriptWindowObject, but instead of direct QObject*, FactoryQObject* is passed like below,

eg) 
// native
m_page->mainFrame()->addToJavaScriptWindowObject("Foo", objectFactory); // where objectFactory is QObject*


FactoryQObject must have the method named "construct" with signature "QObject* construct(const QString& name, const QVariantList& args)". Whenever "new Foo()" is evaluated in JavaScript FactoryQObject::construct will be called to create a new instance for it. 

The argument "name" in "construct" is to distinguish between objects,

eg)
//native
m_page->mainFrame()->addToJavaScriptWindowObject("Foo", testFactory); // where testFactory is QObject*
m_page->mainFrame()->addToJavaScriptWindowObject("Bar", testFactory); // where testFactory is QObject*

When "new Foo()" called "name" is "Foo" & for "new Bar()" name is "Bar".

I'm uploading the patch for this bug soon.

Few months back this has been discussed in qtwebkit mailing list, https://lists.webkit.org/pipermail/webkit-qt/2013-January/003395.html.

[Arunprasad Rajkumar]

Created attachment 202229 [details]
Patch

[Arunprasad Rajkumar]

Created attachment 202883 [details]
Patch

[Vivek Galatage]

Comment on attachment 202883 [details]
Patch

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

> Source/WebCore/bridge/qt/qt_instance.cpp:48
> +static int getConstructMethodIndex(QObject* obj)

How about making this inline? Also can we move this method in QtInstance it self and name it 

class QtInstance : public Instance {
    ...

    QObject* getObject() const { return m_object.data(); }
    QObject* hashKey() const { return m_hashkey; }
    inline int constructMethodIndex() { 
         QObject* object = getObject();
         if (!object) 
             return -1;

         ...     
    }
}

> Source/WebCore/bridge/qt/qt_instance.cpp:256
> +    if (index >= 0) {

You are asserting it above. So I guess this is not required may be!

> Source/WebCore/bridge/qt/qt_instance.cpp:257
> +        QMetaMethod m = obj->metaObject()->method(index);

How about naming m -> constructMethod to be more verbose?

> Source/WebCore/bridge/qt/qt_instance.cpp:265
> +            void * qargs[3];

Extra white space between void & *

> Source/WebCore/bridge/qt/qt_instance.cpp:267
> +            QObject* qobject = 0;

Can you name qobject to some more verbose name may be customQObject etc?

> Source/WebCore/bridge/qt/qt_instance.cpp:272
> +            for (unsigned i = 0; i < args.size(); i++)

++i is preferred in WebKit.

> Source/WebCore/bridge/qt/qt_instance.h:85
> +

I am not sure but rest of the WebKit (including in Source/WebKit/qt) has the macro OVERRIDE as the suffix to the virtual methods which is missing here.

> Source/WebKit/qt/tests/qobjectbridge/tst_qobjectbridge.cpp:619
> +    TestConstructObject(int param1, const QString& param2)

Use descriptive names instead of param1 and param2 (if possible here)

> Source/WebKit/qt/tests/qobjectbridge/tst_qobjectbridge.cpp:783
> +    "var obj = new TestConstructObject%1(%1,'TestObject%1'); "

Missing tests for when var obj = new TestConstructObject() is invoked when the said custom object is not available. Please add the negative cases if appropriate in the context.

[Vivek Galatage]

Comment on attachment 202883 [details]
Patch

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

> Source/WebCore/bridge/qt/qt_instance.cpp:255
> +    ASSERT(index >= 0);

Why are you ASSERTing here?? As there is a case that may not support the object and hence it could be undefined..

[Arunprasad Rajkumar]

Created attachment 202888 [details]
Patch

[Simon Hausmann]

We just had a discussion about this.

You could support constructor syntax in your application on the JavaScript side if you wrap the factories in JavaScript itself. We assume that this is something you would like to clean up or simplify by moving everything to the C++ side and eliminating any JavaScript side API wrappers.

More generally speaking the question arises: How do you implement complete JavaScript APIs (that allow for the use of new expressions, function calls where you can access the this object properly, modify/setup the prototype chain, etc.) entirely in C++ without JavaScript wrappers?

I think the _clean_ solution to that requires a full C/C++ API that is powerful enough to allow for all these things. We could advocate the development of a Qt and C++ based API for that (similar to Qt's QtScript API), but I think a simpler solution that requires less effort and is faster (performance wise) would be to give access to the JavaScriptCore C API.

If you have the time and motivation to do the work, I would propose the following approach:

(1) Add API to QWebFrame that returns a JSContextRef for the frame's window object. (this may require forward-declaring JSContextRef)

(2) Modify the build system to give access to the JavaScriptCore C API when using QT += webkit-private in .pro files.


The former should be a relatively simple patch and the latter (which can be a separate patch for sure) may require a little bit more effort, but would complete the solution.

What do you think about that?

[Arunprasad Rajkumar]

(In reply to comment #6)
> We just had a discussion about this.
Thank you :)

> 
> You could support constructor syntax in your application on the JavaScript side if you wrap the factories in JavaScript itself. We assume that this is something you would like to clean up or simplify by moving everything to the C++ side and eliminating any JavaScript side API wrappers.

Yes, exactly :) The current patch allows the new expression on the QtBridge object.

> 
> More generally speaking the question arises: How do you implement complete JavaScript APIs 
(that allow for the use of new expressions, 

The current patch supports the use of new expression,

>function calls where you can access the this object properly,

Sorry, I didn't get you :( Is it similar like bridgeObj.myFunction=function(){ this.x ,... }..?

 modify/setup the prototype chain, etc.) entirely in C++ without JavaScript wrappers?

Are you meaning How to set up prototypical inheritance? bridgeObj.prototype?

> 
> I think the _clean_ solution to that requires a full C/C++ API that is powerful enough to allow for all these things. We could advocate the development of a Qt and C++ based API for that (similar to Qt's QtScript API), but I think a simpler solution that requires less effort and is faster (performance wise) would be to give access to the JavaScriptCore C API.
> 
Looks great, but if JSC C API is directly exposed, the getting/setting properties, method invocation should be handled explicitly. We may loose the power of meta object system in Qt.

> If you have the time and motivation to do the work, I would propose the following approach:

I'm always to ready to take :D

> 
> (1) Add API to QWebFrame that returns a JSContextRef for the frame's window object. (this may require forward-declaring JSContextRef)

> 
> (2) Modify the build system to give access to the JavaScriptCore C API when using QT += webkit-private in .pro files.

I understood your point.

> 
> 
> The former should be a relatively simple patch and the latter (which can be a separate patch for sure) may require a little bit more effort, but would complete the solution.
> 
> What do you think about that?
My worry is user has to take care of implementing JSObjectGetPropertyCallback, JSObjectSetPropertyCallback, JSObjectCallAsFunctionCallback, JSObjectCallAsConstructorCallback,.. But incase of QtBridge, it is taken care by Qt's Object Introspection.

[Arunprasad Rajkumar]

(In reply to comment #6)
> 
> (1) Add API to QWebFrame that returns a JSContextRef for the frame's window object. (this may require forward-declaring JSContextRef)
> 
> (2) Modify the build system to give access to the JavaScriptCore C API when using QT += webkit-private in .pro files.
> 
> 
> What do you think about that?

The approach looks fine for me, if u all guys agreed then, I can start working on this.

[Jocelyn Turcotte]

(In reply to comment #8)
> > What do you think about that?
> 
> The approach looks fine for me, if u all guys agreed then, I can start working on this.

I think this would be a good direction as well. You guys want the power, but other users might prefer reliability. By allowing the use the C API directly, privately without binary compatibility requirements, I think that this would allow to keep the reliability part.

[Arunprasad Rajkumar]

(In reply to comment #9)
> (In reply to comment #8)
> > > What do you think about that?
> > 
> > The approach looks fine for me, if u all guys agreed then, I can start working on this.
> 
> I think this would be a good direction as well. You guys want the power, but other users might prefer reliability. By allowing the use the C API directly, privately without binary compatibility requirements, I think that this would allow to keep the reliability part.

Thanks for your confirmation. I will start working this and submit incremental patches as suggested by Simon. I will open a master bug for this implementation. 

This bug should be closed right?

[Anders Carlsson]

Comment on attachment 202888 [details]
Patch

Qt has been removed, clearing review flags.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.