[webkit-reviews] review granted: [Bug 115321] Implement file path restrictions in WebKit Objective C API : [Attachment 199984] proposed patch

[bugzilla-daemon at webkit.org]

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.