Bug 112891

Created attachment 194210 [details] Patch

Comment on attachment 194210 [details] Patch Very nice!

(In reply to comment #2) > Very nice! I hope the bots will share your opinion! ;)

Comment on attachment 194210 [details] Patch Attachment 194210 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17225464 New failing tests: fast/preloader/document-write-noscript.html

Created attachment 194261 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

Comment on attachment 194210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194210&action=review Looks like your lean the JSStringRef? > Source/JavaScriptCore/API/JSStringRefQt.cpp:42 > +JSStringRef JSStringCreateWithQString(const QString& str) Should this return a JSRetainPtr to avoid any mistake?

(In reply to comment #6) > (From update of attachment 194210 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=194210&action=review > > Looks like your lean the JSStringRef? > > Source/JavaScriptCore/API/JSStringRefQt.cpp:42 > > +JSStringRef JSStringCreateWithQString(const QString& str) > > Should this return a JSRetainPtr to avoid any mistake? Actually after looking into this, i believe my patch is correct. JSStringCreateWithQString returns a StringRef, which by itself is not wrong. I only use it with LoadHTMLStringItem/LoadAlternateHTMLStringItem/LoadingScriptItem/LoadItem. All of them take a JSStringRef in the constructor and create and hold a JSRetainPtr<JSStringRef> for it internally. However it might be more safe to use a JSRetainPtr to avoid leakage in future. But it requires converting from raw JSStringRef to JSRetainPtr<JSStringRef> forwards and backwards. What do you think?

Created attachment 194631 [details] Patch

(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 194210 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=194210&action=review > > > > Looks like your lean the JSStringRef? > > > Source/JavaScriptCore/API/JSStringRefQt.cpp:42 > > > +JSStringRef JSStringCreateWithQString(const QString& str) > > > > Should this return a JSRetainPtr to avoid any mistake? > > Actually after looking into this, i believe my patch is correct. > JSStringCreateWithQString returns a StringRef, which by itself is not wrong. > I only use it with LoadHTMLStringItem/LoadAlternateHTMLStringItem/LoadingScriptItem/LoadItem. > All of them take a JSStringRef in the constructor and create and hold a JSRetainPtr<JSStringRef> for it internally. > However it might be more safe to use a JSRetainPtr to avoid leakage in future. But it requires converting from raw JSStringRef to JSRetainPtr<JSStringRef> forwards and backwards. What do you think? JSStringCreateWithCFString does return a JSStringRef as well, the same model as with WKStringCreateWith*. I think that it's the caller that is expected to adopt the 1-ref'd string into a JSRetainPtr, which was missing from your patch.

Comment on attachment 194631 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194631&action=review > Source/JavaScriptCore/API/JSStringRefQt.cpp:44 > + return JSRetainPtr<JSStringRef>(Adopt, JSStringCreateWithUTF8CString(str.toUtf8().constData())); This will convert the wchar QString into a UTF8 QByteArray and then convert it back to a wchar WTF::String. Would it be possible to use OpaqueJSString::create(String&) to take advantage of the existing WTF::String::String(const QString&) code? Kind of the invert of JSStringCopyQString. > Tools/DumpRenderTree/qt/TestRunnerQt.cpp:35 > +#include "OpaqueJSString.h" Would it be possible to avoid this? Replacing JSStringRef->qString() with JSStringCopyQString() maybe?

Comment on attachment 194631 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194631&action=review >> Source/JavaScriptCore/API/JSStringRefQt.cpp:44 >> + return JSRetainPtr<JSStringRef>(Adopt, JSStringCreateWithUTF8CString(str.toUtf8().constData())); > > This will convert the wchar QString into a UTF8 QByteArray and then convert it back to a wchar WTF::String. > Would it be possible to use OpaqueJSString::create(String&) to take advantage of the existing WTF::String::String(const QString&) code? > Kind of the invert of JSStringCopyQString. I think i can make use of OpaqueJSString::create indeed. I'll try it out and update the patch. >> Tools/DumpRenderTree/qt/TestRunnerQt.cpp:35 >> +#include "OpaqueJSString.h" > > Would it be possible to avoid this? Replacing JSStringRef->qString() with JSStringCopyQString() maybe? Yes, that should work.

Created attachment 195115 [details] Patch

Created attachment 195174 [details] Patch

Comment on attachment 195174 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195174&action=review I did not check every use of the WorkQueue, I trust you on this. > Source/JavaScriptCore/API/JSStringRefQt.cpp:42 > +JSRetainPtr<JSStringRef> JSStringCreateWithQString(const QString& str) str->string or qString > Source/JavaScriptCore/API/JSStringRefQt.cpp:46 > + if (jsString) Do you really need that branch? I'd think return JSRetainPtr<JSStringRef>(Adopt, jsString.release().leakRef()); would be fine in any case. > Tools/DumpRenderTree/qt/WorkQueueItemQt.h:39 > + LoadAlternateHTMLStringItem(const JSStringRef content, const JSStringRef baseURL, const JSStringRef failingURL) You could take const JSRetainPtr<JSStringRef>& as arguments to avoid using .get() at the call site. On a recent clang, it may even get rid of the ref-deref.

Comment on attachment 195174 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195174&action=review >> Source/JavaScriptCore/API/JSStringRefQt.cpp:42 >> +JSRetainPtr<JSStringRef> JSStringCreateWithQString(const QString& str) > > str->string or qString I'll update that before landing. >> Source/JavaScriptCore/API/JSStringRefQt.cpp:46 >> + if (jsString) > > Do you really need that branch? > > I'd think > return JSRetainPtr<JSStringRef>(Adopt, jsString.release().leakRef()); > would be fine in any case. The branch is necessary. Since the QString internal data ptr can be 0x0 in case of an empty string, WTFString would not actually adopt any string but miss it's m_impl ptr. In this case OpaqueJSString::create(..) returns 0. And calling release() on that causes a segfault. >> Tools/DumpRenderTree/qt/WorkQueueItemQt.h:39 >> + LoadAlternateHTMLStringItem(const JSStringRef content, const JSStringRef baseURL, const JSStringRef failingURL) > > You could take const JSRetainPtr<JSStringRef>& as arguments to avoid using .get() at the call site. > > On a recent clang, it may even get rid of the ref-deref. Not perfectly in line with the other WorkQueueItems, but it certainly looks nicer. I'll try it out and in case it works nicely update it before landing.

Committed r146976: <http://trac.webkit.org/changeset/146976>