Bug 95570 - [Qt] Port core QObject bridge to use JSC C API
Summary: [Qt] Port core QObject bridge to use JSC C API
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Hausmann
Depends on: 94702 95325
Blocks: 60842
  Show dependency treegraph
Reported: 2012-08-31 07:54 PDT by Simon Hausmann
Modified: 2014-01-13 21:45 PST (History)
2 users (show)

See Also:

Patch (75.65 KB, patch)
2012-08-31 08:08 PDT, Simon Hausmann
webkit-ews: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Hausmann 2012-08-31 07:54:02 PDT
[Qt] Port core QObject bridge to use JSC C API
Comment 1 Simon Hausmann 2012-08-31 08:08:32 PDT
Created attachment 161704 [details]
Comment 2 Simon Hausmann 2012-08-31 08:09:51 PDT
(The EWS won't like this patch as it depends on the previous patches :)
Comment 3 Early Warning System Bot 2012-08-31 09:12:23 PDT
Comment on attachment 161704 [details]

Attachment 161704 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13705013
Comment 4 Early Warning System Bot 2012-08-31 09:23:43 PDT
Comment on attachment 161704 [details]

Attachment 161704 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13713268
Comment 5 Caio Marcelo de Oliveira Filho 2012-09-04 16:57:43 PDT
Comment on attachment 161704 [details]

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);


> 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;


> 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)?
Comment 6 Anders Carlsson 2013-10-02 21:15:00 PDT
Comment on attachment 161704 [details]

Qt has been removed, clearing review flags.