Bug 94695

Summary: [Qt] Port convertValueToQVariant to use the JSC C API
Product: WebKit Reporter: Simon Hausmann <hausmann>
Component: New BugsAssignee: Simon Hausmann <hausmann>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 60842    
Attachments:
Description Flags
Patch
none
Patch kenneth: review+

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>