Bug 45150 – [Qt] V8 port for Qt platform: Qt bridge port

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Vlad]

V8 qt meta system port. Reimplemented qt bridge runtime, instance and pixmapruntime components.

[Vlad]

Created attachment 66446 [details]
QT meta system port for v8

[anton muhin]

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 ([e-mail]) 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?

[Andreas Kling]

Comment on attachment 66446 [details]
QT meta system port for v8

Please address Anton's comments.

[Vlad]

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.

[WebKit Review Bot]

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.

[Simon Hausmann]

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.