Bug 108651

Summary: [WK2][Qt] Replace WebPagePageGroup usage for user scripts with WKPageGroupRef
Product: WebKit Reporter: Simon Hausmann <hausmann>
Component: New BugsAssignee: Simon Hausmann <hausmann>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, cmarcelo, menard, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 108471    
Attachments:
Description Flags
Patch sam: review+

Simon Hausmann
Reported 2013-02-01 09:30:20 PST
[WK2][Qt] Replace WebPagePageGroup usage for user scripts with WKPageGroupRef
Attachments
Patch (5.57 KB, patch)
2013-02-01 09:31 PST, Simon Hausmann 		sam: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Simon Hausmann
Comment 1 2013-02-01 09:31:13 PST
Created attachment 186062 [details] Patch
Kenneth Rohde Christiansen
Comment 2 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?
Simon Hausmann
Comment 3 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()))
Sam Weinig
Comment 4 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.
Simon Hausmann
Comment 5 2013-02-04 04:23:38 PST
Committed r141756: <http://trac.webkit.org/changeset/141756>
Note You need to log in before you can comment on or make changes to this bug.