[Qt] Port core QObject bridge to use JSC C API

• Component: New Bugs
• Status: RESOLVED INVALID

[Simon Hausmann]

[Qt] Port core QObject bridge to use JSC C API

[Simon Hausmann]

Created attachment 161704 [details]
Patch

[Simon Hausmann]

(The EWS won't like this patch as it depends on the previous patches :)

[Early Warning System Bot]

Comment on attachment 161704 [details]
Patch

Attachment 161704 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13705013

[Early Warning System Bot]

Comment on attachment 161704 [details]
Patch

Attachment 161704 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13713268

[Caio Marcelo de Oliveira Filho]

Comment on attachment 161704 [details]
Patch

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

Couldn't look at everything today, will continue tomorrow.

> Source/WebCore/bridge/qt/qt_class.cpp:75
> +    JSWeakObjectMapSet(m_impl->m_context, m_impl->m_map, key, object);

What if m_impl was created in one context group, wrap a certain QObject, then later in another context group we ask for wrapped version again. It'll give us one that is tied to the first context group. I don't know whether this case could happen, but the fact that set() is parametrized by context (group) and get() is not seems confusing at first to me.

You can think of this at QtClass level too: QtClasses are ultimately tied with a certain context group.

Question: do you know if there are cases in the our API tests in which we have two different context groups being used?

> Source/WebCore/bridge/qt/qt_class.cpp:95
> +    QtSafeObject(QObject* object, bool own)

Minor idea: use "type tag" to identify the ownership cases more explicitly when creating QtSafeObject.

> Source/WebCore/bridge/qt/qt_class.cpp:115
> +    if (!object)
> +        return 0;
> +    if (!JSValueIsObjectOfClass(context, object, qtObjectBaseClass()))

"if (!object || !JSValue...)".

> Source/WebCore/bridge/qt/qt_class.cpp:208
> +        JSStringRef stringRef = JSStringCreateWithCharacters(reinterpret_cast<const JSChar*>(string.data()), string.length() + 1);

JSRetainPtr.

> Source/WebCore/bridge/qt/qt_class.cpp:224
> +QString QtClass::toString(QObject* obj)

Would a "QVariant callToStringMetaMethod()" helper function make things clearer? Return QVariant() if call fails,   This would get the Qt meta object method out of the way, and you might even get by without the goto...

> Source/WebCore/bridge/qt/qt_class.cpp:245
> +    if (m.access() == QMetaMethod::Private
> +        || m.methodType() == QMetaMethod::Signal
> +        || m.parameterCount())

Should we check if returnType() is not void?

Minor: m.parameterCount > 0 also reads better here.

> Source/WebCore/bridge/qt/qt_class.cpp:252
> +    retsig = m.typeName();
> +    if (retsig && *retsig) {
> +        ret = QVariant(QMetaType::type(retsig), (void*)0);
> +        qargs[0] = ret.data();
> +    }

I think we can use returnType() here instead of retsig.

> Source/WebCore/bridge/qt/qt_class.cpp:329
> +    static HashMap<const QMetaObject*, QtClass*> classes;

DEFINE_STATIC_LOCAL?

> Source/WebCore/bridge/qt/qt_class.h:34
> +class WeakMapImpl : public RefCounted<WeakMapImpl> {

I think would be good mention in the ChangeLog that QtClasses will end up sharing the same WeakMapImpl.

> Source/WebKit/qt/tests/qobjectbridge/tst_qobjectbridge.cpp:624
> +// The JavaScriptCore GC marks the C stack. To try to ensure that there is
> +// no JSObject* left in stack memory by the compiler, we call this function
> +// to zap some bytes of memory before calling collectGarbage().

Just curious: can't JSC GC stop at the SP (stack pointer)?

[Anders Carlsson]

Comment on attachment 161704 [details]
Patch

Qt has been removed, clearing review flags.