[Webkit-unassigned] [Bug 116408] [Qt] Support Object construction for window properties added using QtWebKitBridge

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=116408





--- Comment #3 from Vivek Galatage <vivekg at webkit.org>  2013-05-25 04:38:10 PST ---
(From update of attachment 202883)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.