[webkit-reviews] review granted: [Bug 112891] [Qt] Remove Qt specific WorkQueueItem definitions. : [Attachment 195174] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Zeno Albisser
<zeno at webkit.org>'s request for review:
Bug 112891: [Qt] Remove Qt specific WorkQueueItem definitions.
https://bugs.webkit.org/show_bug.cgi?id=112891

Attachment 195174: Patch
https://bugs.webkit.org/attachment.cgi?id=195174&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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.