V8 qt meta system port. Reimplemented qt bridge runtime, instance and pixmapruntime components.
Created attachment 66446 [details] QT meta system port for v8
Comment on attachment 66446 [details] QT meta system port for v8 Just a couple of notes: 1) I am not a WebKit reviewer yet, but probably would be soon; 2) for me the patch is just too big, it might be easier to digest and understand if it's split into smaller parts; 3) feel free to cc me (antonm@chromium.org) on v8 related stuff---I did enough work on Chromium bindings, esp. in things closer to v8 proper. View in context: https://bugs.webkit.org/attachment.cgi?id=66446&action=prettypatch > WebCore/bridge/qt/v8/qt_instancev8.cpp:2 > + * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies) Maybe 2010? > WebCore/bridge/qt/v8/qt_instancev8.cpp:56 > + v8ClassTempl = v8::Persistent<v8::FunctionTemplate>::New(v8::FunctionTemplate::New()); 1) why not declare and initialize in one go? 2) v8ClassTempl is (apparently) a local handle, in that case using persistent handles would have performance/memory usage drawbacks and no real benefits (here and in all the cases below) > WebCore/bridge/qt/v8/qt_instancev8.cpp:66 > + // But which way is faster? internal fields are ways faster. Note as well, there are SetPointerIntoInternalField/GetPointerFromInternalField which should be preferred over explicit wrapping (at least if you care about performance :) > WebCore/bridge/qt/v8/qt_instancev8.cpp:72 > + m_v8Obj->SetHiddenValue(v8::String::New("__qt_hidden_marker__"), v8::External::Wrap(this)); I am not quite sure how does it work---I am not sure it'll retain this. And SetHiddenValue may affect internal fields if memory serves > WebCore/bridge/qt/v8/qt_instancev8.cpp:147 > + if (d->m_allowPrivate) up to you, but I would split into two parts: attributes calculation and actual m_v8Obj->Set > WebCore/bridge/qt/v8/qt_instancev8.cpp:156 > +QtInstance::~QtInstance() you don't make m_v8Obj weak and do not dispose it, that looks like a leak unless there is some other code which does that > WebCore/bridge/qt/v8/qt_instancev8.cpp:204 > + return v8::Boolean::New(false); there is v8::False() which is better imho > WebCore/bridge/qt/v8/qt_instancev8.cpp:209 > + QtInstance* inst = static_cast<QtInstance*>(v8::External::Unwrap(info.Holder()->GetInternalField(0))); ditto for GetPointerFromIntenalField > WebCore/bridge/qt/v8/qt_instancev8.cpp:221 > + if (!inst->getObject() /*&& !inst->m_methods.contains(ba) */) { commented out code > WebCore/bridge/qt/v8/qt_instancev8.cpp:303 > + m_v8Obj->Set(v8::String::New(name), value); maybe you should use this method in ctor above > WebCore/bridge/qt/v8/qt_instancev8.cpp:344 > + for (i = 0; i < meta->propertyCount(); i++) { I'd personally would use separate i's for both loops > WebCore/bridge/qt/v8/qt_instancev8.cpp:412 > + return v8::Boolean::New(true); ditto for v8::True() > WebCore/bridge/qt/v8/qt_instancev8.cpp:488 > + fieldGetter, fieldPropertyHandlerSetter, 0, 0, 0, v8::String::New("QObject PropertyHandler")); you could probably omit datum if you don't use it > WebCore/bridge/qt/v8/qt_pixmapruntimev8.cpp:2 > + * Copyright (C) 2009 Nokia Corporation and/or its subsidiary(-ies) 2010?
Comment on attachment 66446 [details] QT meta system port for v8 Please address Anton's comments.
Created attachment 67972 [details] Review changes. NOT a patch for commit yet. It is not a patch yet until datemath issue is sorted out. https://bugs.webkit.org/show_bug.cgi?id=45142. Anton, could you please take a look at the changes. Especially part of handling weak pointer to the injected QtObject.
Attachment 67972 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.
I think with the current changes in the trunk we can close this as wontfix. We can re-open this bug if the need for non-QtScript based QObject bindings arises again, but for the moment I don't see the need for them. After all it is our long-term vision to remove the custom bridging/bindings code in the Qt port of WebKit.