Bug 108651 - [WK2][Qt] Replace WebPagePageGroup usage for user scripts with WKPageGroupRef
Summary: [WK2][Qt] Replace WebPagePageGroup usage for user scripts with WKPageGroupRef
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: Simon Hausmann
URL:
Keywords:
Depends on:
Blocks: 108471
  Show dependency tree / graph
 
Reported: 2013-02-01 09:30 PST by Simon Hausmann
Modified: 2013-02-04 04:23 PST (History)
4 users (show)

See Also:

Attachments
Patch (5.57 KB, patch)
2013-02-01 09:31 PST, Simon Hausmann 		sam: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Hausmann 2013-02-01 09:30:20 PST 
[WK2][Qt] Replace WebPagePageGroup usage for user scripts with WKPageGroupRef
Comment 1 Simon Hausmann 2013-02-01 09:31:13 PST 
Created attachment 186062 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2013-02-01 09:33:18 PST 
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?
Comment 3 Simon Hausmann 2013-02-01 12:48:05 PST 
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()))
Comment 4 Sam Weinig 2013-02-01 14:10:44 PST 
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.
Comment 5 Simon Hausmann 2013-02-04 04:23:38 PST 
Committed r141756: <http://trac.webkit.org/changeset/141756>