Bug 94695 – [Qt] Port convertValueToQVariant to use the JSC C API

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

[Simon Hausmann]

[Qt] Port convertValueToQVariant to use the JSC C API

[Simon Hausmann]

Created attachment 159912 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 159912 [details]
Patch

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

> Source/WebCore/bridge/qt/qt_runtime.cpp:224
> +        int objdist = 0;

Since you are touching this code, could you improve this name?

> Source/WebCore/bridge/qt/qt_runtime.cpp:244
> +        JSStringRef lengthStr = JSStringCreateWithUTF8CString("length");

"static JSStringRef". And get rid of the release below.

> Source/WebCore/bridge/qt/qt_runtime.cpp:452
> +                JSStringRef str = JSValueToStringCopy(context, value, exception);
> +                QChar ch;
> +                if (str && JSStringGetLength(str) > 0)
> +                    ch = *reinterpret_cast<const QChar*>(JSStringGetCharactersPtr(str));
> +                ret = QVariant(ch);
> +                if (str)
> +                    JSStringRelease(str);

Use JSRetainPtr and get rid of the "if (str) JSStringRelease(str);".

> Source/WebCore/bridge/qt/qt_runtime.cpp:466
> +            JSStringRef str = JSValueToStringCopy(context, value, exception);

JSRetainPtr here too, since we are creating a block in the case should be OK.

> Source/WebKit/qt/Api/qwebelement.cpp:763
> +    return JSC::Bindings::convertValueToQVariant(toRef(state), toRef(state, evaluationResult), QMetaType::Void, &distance, /*exception*/0);

Use "JSValueRef* ignoredException = 0;" then pass it on the function. We use else-where to avoid the inline comment.

[Simon Hausmann]

Comment on attachment 159912 [details]
Patch

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

>> Source/WebKit/qt/Api/qwebelement.cpp:763
>> +    return JSC::Bindings::convertValueToQVariant(toRef(state), toRef(state, evaluationResult), QMetaType::Void, &distance, /*exception*/0);
> 
> Use "JSValueRef* ignoredException = 0;" then pass it on the function. We use else-where to avoid the inline comment.

What's the advantage of an extra variable on the stack and passing a non-null pointer over the inline comment "ignored exception" ?

[Caio Marcelo de Oliveira Filho]

Comment on attachment 159912 [details]
Patch

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

>>> Source/WebKit/qt/Api/qwebelement.cpp:763
>>> +    return JSC::Bindings::convertValueToQVariant(toRef(state), toRef(state, evaluationResult), QMetaType::Void, &distance, /*exception*/0);
>> 
>> Use "JSValueRef* ignoredException = 0;" then pass it on the function. We use else-where to avoid the inline comment.
> 
> What's the advantage of an extra variable on the stack and passing a non-null pointer over the inline comment "ignored exception" ?

It is a null pointer. Effectively it will be the same than passing 0.

It's a tradeoff between one more pointer in the stack and a variable name versus having the inline comment. I tend to choose the first option in those cases, but YMMV.

[Simon Hausmann]

Comment on attachment 159912 [details]
Patch

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

>> Source/WebCore/bridge/qt/qt_runtime.cpp:224
>> +        int objdist = 0;
> 
> Since you are touching this code, could you improve this name?

Going for propertyConversionDistance :)

>> Source/WebCore/bridge/qt/qt_runtime.cpp:244
>> +        JSStringRef lengthStr = JSStringCreateWithUTF8CString("length");
> 
> "static JSStringRef". And get rid of the release below.

Done.

>> Source/WebCore/bridge/qt/qt_runtime.cpp:452
>> +                    JSStringRelease(str);
> 
> Use JSRetainPtr and get rid of the "if (str) JSStringRelease(str);".

Done.

>> Source/WebCore/bridge/qt/qt_runtime.cpp:466
>> +            JSStringRef str = JSValueToStringCopy(context, value, exception);
> 
> JSRetainPtr here too, since we are creating a block in the case should be OK.

Done.

>>>> Source/WebKit/qt/Api/qwebelement.cpp:763
>>>> +    return JSC::Bindings::convertValueToQVariant(toRef(state), toRef(state, evaluationResult), QMetaType::Void, &distance, /*exception*/0);
>>> 
>>> Use "JSValueRef* ignoredException = 0;" then pass it on the function. We use else-where to avoid the inline comment.
>> 
>> What's the advantage of an extra variable on the stack and passing a non-null pointer over the inline comment "ignored exception" ?
> 
> It is a null pointer. Effectively it will be the same than passing 0.
> 
> It's a tradeoff between one more pointer in the stack and a variable name versus having the inline comment. I tend to choose the first option in those cases, but YMMV.

Ah right, it's actually a null pointer. Okay, I don't care too much about it and am fine with using your pattern.

[Simon Hausmann]

Created attachment 161175 [details]
Patch

Updated patch to address comments

[Caio Marcelo de Oliveira Filho]

LGTM.

[Kenneth Rohde Christiansen]

Comment on attachment 161175 [details]
Patch

r=me given caio's comments

[Simon Hausmann]

Committed r127238: <http://trac.webkit.org/changeset/127238>