| Summary: | [WK2][Qt] Replace WebPagePageGroup usage for user scripts with WKPageGroupRef | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Hausmann <hausmann> | ||||
| Component: | New Bugs | Assignee: | 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
Simon Hausmann
2013-02-01 09:30:20 PST
Created attachment 186062 [details]
Patch
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 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 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. Committed r141756: <http://trac.webkit.org/changeset/141756> |