Bug 95570

Created attachment 161704 [details] Patch

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

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

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

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

Comment on attachment 161704 [details] Patch Qt has been removed, clearing review flags.