Bug 94695 - [Qt] Port convertValueToQVariant to use the JSC C API
Summary: [Qt] Port convertValueToQVariant to use the JSC C API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Hausmann
URL:
Keywords:
Depends on:
Blocks: 60842
  Show dependency treegraph
 
Reported: 2012-08-22 05:38 PDT by Simon Hausmann
Modified: 2012-08-31 02:03 PDT (History)
1 user (show)

See Also:


Attachments
Patch (45.03 KB, patch)
2012-08-22 05:42 PDT, Simon Hausmann
no flags Details | Formatted Diff | Diff
Patch (45.00 KB, patch)
2012-08-29 04:01 PDT, Simon Hausmann
kenneth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Hausmann 2012-08-22 05:38:00 PDT
[Qt] Port convertValueToQVariant to use the JSC C API
Comment 1 Simon Hausmann 2012-08-22 05:42:44 PDT
Created attachment 159912 [details]
Patch
Comment 2 Caio Marcelo de Oliveira Filho 2012-08-22 11:37:34 PDT
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.
Comment 3 Simon Hausmann 2012-08-23 00:23:57 PDT
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" ?
Comment 4 Caio Marcelo de Oliveira Filho 2012-08-23 06:39:59 PDT
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.
Comment 5 Simon Hausmann 2012-08-29 03:49:21 PDT
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.
Comment 6 Simon Hausmann 2012-08-29 04:01:30 PDT
Created attachment 161175 [details]
Patch

Updated patch to address comments
Comment 7 Caio Marcelo de Oliveira Filho 2012-08-30 06:02:21 PDT
LGTM.
Comment 8 Kenneth Rohde Christiansen 2012-08-30 06:06:39 PDT
Comment on attachment 161175 [details]
Patch

r=me given caio's comments
Comment 9 Simon Hausmann 2012-08-31 02:03:01 PDT
Committed r127238: <http://trac.webkit.org/changeset/127238>