[webkit-reviews] review granted: [Bug 115321] Implement file path restrictions in WebKit Objective C API : [Attachment 199984] proposed patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Apr 29 08:41:44 PDT 2013
Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 115321: Implement file path restrictions in WebKit Objective C API
https://bugs.webkit.org/show_bug.cgi?id=115321
Attachment 199984: proposed patch
https://bugs.webkit.org/attachment.cgi?id=199984&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=199984&action=review
> Source/WebKit2/UIProcess/WebPageProxy.cpp:751
> + resourceDirectoryURL = KURL(ParsedURLString, "file:///");
> +
> + String resourceDirectoryPath = resourceDirectoryURL.fileSystemPath();
Seems a little strange to construct a URL with "file:///" in it just to extract
the "/" out of it afterward. Could define resourceDirectoryPath earlier and set
it to "/" when resourceDirectoryURLString in a more direct way.
I think it’s also good to use ASCIILiteral in cases like this.
> Source/WebKit2/UIProcess/WebPageProxy.h:291
> + void loadFile(const String& fileURL, const String&
resourceDirectoryURL);
Strange for this to take URL strings instead of URLs.
> Source/WebKit2/UIProcess/API/C/WKPage.cpp:97
> + toImpl(pageRef)->loadFile(toWTFString(fileURL),
toWTFString(resourceDirectoryURL));
Why not convert WKURLRef to KURL instead of going through String?
> Source/WebKit2/UIProcess/API/C/WKPage.h:365
> +WK_EXPORT void WKPageLoadFile(WKPageRef page, WKURLRef fileURL, WKURLRef
resourceDirectoryURL);
I agree that these argument types make sense given Cocoa’s use of CFURL and
NSURL for file paths.
> Source/WebKit2/UIProcess/API/mac/WKBrowsingContextController.mm:128
> WKRetainPtr<WKURLRef> wkURL =
adoptWK(WKURLCreateWithCFURL((CFURLRef)URL));
> - WKPageLoadURL(self._pageRef, wkURL.get());
> + WKRetainPtr<WKURLRef> wkAllowedDirectory =
adoptWK(WKURLCreateWithCFURL((CFURLRef)allowedDirectory));
> +
> + WKPageLoadFile(self._pageRef, wkURL.get(), wkAllowedDirectory.get());
It seems a little unwieldy to have these local variables. WIth a helper
function to convert NSURL to WKRetainPtr<WKURLRef> we could make this a lot
simpler.
More information about the webkit-reviews
mailing list