[Webkit-unassigned] [Bug 45150] [Qt] V8 port for QT platform: QT meta system port.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=45150





--- Comment #2 from anton muhin <antonm at chromium.org>  2010-09-06 07:48:47 PST ---
(From update of attachment 66446)
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 at 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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.