Bug 108651 – [WK2][Qt] Replace WebPagePageGroup usage for user scripts with WKPageGroupRef

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

[Simon Hausmann]

[WK2][Qt] Replace WebPagePageGroup usage for user scripts with WKPageGroupRef

[Simon Hausmann]

Created attachment 186062 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 186062 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186062&action=review

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:740
> +static WKStringRef readUserScript(const QUrl& url)

shouldn't you add Create/Copy or so in the method name to make it obvious that you need to adopt the result

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:779
> +        WKRetainPtr<WKStringRef> contents = adoptWK(readUserScript(url));
> +        if (WKStringIsEmpty(contents.get()))

don't you need to check whehter contents is null or not?

[Simon Hausmann]

Comment on attachment 186062 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186062&action=review

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:740
>> +static WKStringRef readUserScript(const QUrl& url)
> 
> shouldn't you add Create/Copy or so in the method name to make it obvious that you need to adopt the result

Not sure how important that is here. createUserScriptString?

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:779
>> +        if (WKStringIsEmpty(contents.get()))
> 
> don't you need to check whehter contents is null or not?

You're right, at least WKStringIsEmpty seems to crash with a null pointer, so I'm going to write it as

if (!contents || WKStringIsEmpty(contents.get()))

[Sam Weinig]

Comment on attachment 186062 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186062&action=review

Other than the existing comments, r=me.

> Source/WebKit2/ChangeLog:3
> +        [WK2][Qt] Replace WebPagePageGroup usage for user scripts with WKPageGroupRef

I think you meant WebPageGroup.

[Simon Hausmann]

Committed r141756: <http://trac.webkit.org/changeset/141756>