[Qt] Port convertQVariantToValue to use the JSC C API

• Component: New Bugs
• Status: RESOLVED FIXED

[Simon Hausmann]

[Qt] Port convertQVariantToValue to use the JSC C API

[Simon Hausmann]

Created attachment 159682 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 159682 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159682&action=review

> Source/WebCore/bridge/qt/qt_runtime.cpp:838
> +        // ### FIXME: Need private JSC C API.

If possible, create a bug for this and reference it here.

> Source/WebCore/bridge/qt/qt_runtime.cpp:884
> +            if (exception && *exception)
> +                break;
> +            if (propertyValue)
> +                JSObjectSetProperty(context, ret, propertyName, propertyValue, kJSPropertyAttributeNone, exception);
> +            if (exception && *exception)
> +                break;

Do we want to stop when there's an error converting something, or just skip it to be consistent with the other "container" types?

> Source/WebCore/bridge/qt/qt_runtime.cpp:894
> +        // ### Could use special array class that lazily converts.

Use "TODO" :-)

[Kenneth Rohde Christiansen]

Comment on attachment 159682 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159682&action=review

> Source/WebCore/ChangeLog:19
> +        * bridge/qt/qt_instance.cpp:
> +        (Bindings):
> +        (JSC::Bindings::QtField::valueFromInstance):
> +        * bridge/qt/qt_runtime.cpp:
> +        (JSC::Bindings::convertQVariantToValue):
> +        (JSC::Bindings::QtRuntimeMethod::call):
> +        (JSC::Bindings::QtConnectionObject::execute):
> +        (JSC::Bindings::::valueAt):
> +        * bridge/qt/qt_runtime.h:
> +        (Bindings):

a bit more info could be added. Like now you actually throw errors

>> Source/WebCore/bridge/qt/qt_runtime.cpp:894
>> +        // ### Could use special array class that lazily converts.
> 
> Use "TODO" :-)

I think we are actually supposed to use FIXME: still a fixme or todo is better with a bug link :-)

> Source/WebCore/bridge/qt/qt_runtime.cpp:928
>      } else if (type == (QMetaType::Type)qMetaTypeId<QList<int> >()) {

why no space before qMeta here you have that higher up in the code

[Simon Hausmann]

Comment on attachment 159682 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159682&action=review

>> Source/WebCore/ChangeLog:19
>> +        (Bindings):
> 
> a bit more info could be added. Like now you actually throw errors

The error handling is actually just an artifact of the JSC C API, behaviourally nothing changed (because in the internal API the exceptions are not handled using a separate variable but they "remain" in ExecState). But I'll write a few more words :)

>> Source/WebCore/bridge/qt/qt_runtime.cpp:838
>> +        // ### FIXME: Need private JSC C API.
> 
> If possible, create a bug for this and reference it here.

On second thought, I think I should remove the comment. The DOM binding for this type doesn't belong into the (pure) JSC C API and the use of internal API is okay here. When moving this code out WebCore at some point I'll see if we can handle the bytearray conversion using the custom type code. Along with the pixmap runtime there'll always be a little bit of code left that uses the JSC internals, but we can keep that code minimal and separate from the rest.

>> Source/WebCore/bridge/qt/qt_runtime.cpp:884
>> +                break;
> 
> Do we want to stop when there's an error converting something, or just skip it to be consistent with the other "container" types?

Hm, right. Ok, I'll change it back for consistency and pass null in fact. (we don't want to "pollute" the exception parameter of the overall function then)

>>> Source/WebCore/bridge/qt/qt_runtime.cpp:894
>>> +        // ### Could use special array class that lazily converts.
>> 
>> Use "TODO" :-)
> 
> I think we are actually supposed to use FIXME: still a fixme or todo is better with a bug link :-)

Alright, I'll file a bug and use TODO.

>> Source/WebCore/bridge/qt/qt_runtime.cpp:928
>>      } else if (type == (QMetaType::Type)qMetaTypeId<QList<int> >()) {
> 
> why no space before qMeta here you have that higher up in the code

Hm, good point, that's inconsistent, although I think higher up in the code there should not be a space. I'll make it more consistent :)

[Simon Hausmann]

Created attachment 159902 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 159902 [details]
Patch

I still think the changelog could use a few words

[Simon Hausmann]

(In reply to comment #6)
> (From update of attachment 159902 [details])
> I still think the changelog could use a few words

Oops, I forgot that in the uploaded patch. Will elaborate when landing. Sorry :)

[Simon Hausmann]

Committed r126290: <http://trac.webkit.org/changeset/126290>