Bug 94695

Created attachment 159912 [details] Patch

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 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 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 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.

Created attachment 161175 [details] Patch Updated patch to address comments

LGTM.

Comment on attachment 161175 [details] Patch r=me given caio's comments

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