Bug 112891 – [Qt] Remove Qt specific WorkQueueItem definitions.

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

[Zeno Albisser]

[Qt] Remove Qt specific WorkQueueItem definitions.

[Zeno Albisser]

Created attachment 194210 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 194210 [details]
Patch

Very nice!

[Zeno Albisser]

(In reply to comment #2)
> Very nice!

I hope the bots will share your opinion! ;)

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Benjamin Poulain]

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?

[Zeno Albisser]

(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?

[Zeno Albisser]

Created attachment 194631 [details]
Patch

[Jocelyn Turcotte]

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

[Jocelyn Turcotte]

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?

[Zeno Albisser]

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.

[Zeno Albisser]

Created attachment 195115 [details]
Patch

[Zeno Albisser]

Created attachment 195174 [details]
Patch

[Benjamin Poulain]

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.

[Zeno Albisser]

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.

[Zeno Albisser]

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