Bug 112891 - [Qt] Remove Qt specific WorkQueueItem definitions.
Summary: [Qt] Remove Qt specific WorkQueueItem definitions.
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: Zeno Albisser
URL:
Keywords:
Depends on:
Blocks: 109677
  Show dependency treegraph
 
Reported: 2013-03-21 02:53 PDT by Zeno Albisser
Modified: 2013-03-27 04:15 PDT (History)
3 users (show)

See Also:


Attachments
Patch (14.96 KB, patch)
2013-03-21 02:59 PDT, Zeno Albisser
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-08 (608.46 KB, application/zip)
2013-03-21 08:09 PDT, WebKit Review Bot
no flags Details
Patch (15.22 KB, patch)
2013-03-22 14:24 PDT, Zeno Albisser
no flags Details | Formatted Diff | Diff
Patch (15.09 KB, patch)
2013-03-26 10:43 PDT, Zeno Albisser
no flags Details | Formatted Diff | Diff
Patch (15.23 KB, patch)
2013-03-26 15:26 PDT, Zeno Albisser
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zeno Albisser 2013-03-21 02:53:54 PDT
[Qt] Remove Qt specific WorkQueueItem definitions.
Comment 1 Zeno Albisser 2013-03-21 02:59:27 PDT
Created attachment 194210 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2013-03-21 03:03:01 PDT
Comment on attachment 194210 [details]
Patch

Very nice!
Comment 3 Zeno Albisser 2013-03-21 03:24:36 PDT
(In reply to comment #2)
> Very nice!

I hope the bots will share your opinion! ;)
Comment 4 WebKit Review Bot 2013-03-21 08:09:07 PDT
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
Comment 5 WebKit Review Bot 2013-03-21 08:09:11 PDT
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 6 Benjamin Poulain 2013-03-21 14:09:13 PDT
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?
Comment 7 Zeno Albisser 2013-03-22 03:24:45 PDT
(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?
Comment 8 Zeno Albisser 2013-03-22 14:24:08 PDT
Created attachment 194631 [details]
Patch
Comment 9 Jocelyn Turcotte 2013-03-26 04:14:23 PDT
(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 10 Jocelyn Turcotte 2013-03-26 04:33:17 PDT
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 11 Zeno Albisser 2013-03-26 08:40:53 PDT
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.
Comment 12 Zeno Albisser 2013-03-26 10:43:55 PDT
Created attachment 195115 [details]
Patch
Comment 13 Zeno Albisser 2013-03-26 15:26:16 PDT
Created attachment 195174 [details]
Patch
Comment 14 Benjamin Poulain 2013-03-26 18:19:59 PDT
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 15 Zeno Albisser 2013-03-27 02:49:54 PDT
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.
Comment 16 Zeno Albisser 2013-03-27 04:15:22 PDT
Committed r146976: <http://trac.webkit.org/changeset/146976>