Currently, allowedDirectory argument is just ignored. <rdar://problem/13574729>
Created attachment 199959 [details] proposed patch
Comment on attachment 199959 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=199959&action=review > Source/WebCore/platform/KURL.cpp:340 > +KURL KURL::fromFileSystemPath(const String& path) > +{ > + return KURL(ParsedURLString, "file://" + path); > +} This doesn’t seem right unless the path has special characters such as "#" already URL-encoded. Given the function name, I don’t see how that is clear.
Comment on attachment 199959 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=199959&action=review > Source/WebCore/platform/KURL.h:53 > +enum FileSystemPathTag { FileSystemPath }; Looks like this is unused; presumably used in an earlier version of the patch.
Comment on attachment 199959 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=199959&action=review >> Source/WebCore/platform/KURL.cpp:340 >> +} > > This doesn’t seem right unless the path has special characters such as "#" already URL-encoded. Given the function name, I don’t see how that is clear. I think a correct version needs to call encodeWithURLEscapeSequences. And I think we need a fast path in encodeWithURLEscapeSequences for when nothing needs encoding. And I think that KURL::setPath has an incorrect FIXME in it.
I'd like to hear what Sam or Anders think about the proposed C API before updating the patch. Perhaps it should take a WKURLRef, not a WKStringRef?
(In reply to comment #5) > I'd like to hear what Sam or Anders think about the proposed C API before updating the patch. Perhaps it should take a WKURLRef, not a WKStringRef? A URL makes more sense to me.
Created attachment 199984 [details] proposed patch > A URL makes more sense to me. OK
Comment on attachment 199984 [details] proposed patch 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.
Committed <http://trac.webkit.org/r149422>. > Strange for this to take URL strings instead of URLs. I was following the pattern of loadURL(const String&) here.