As a follow-up to bug 114950, I think we can remove DOMFileSystemBlackBerry.cpp completely if we do the necessary changes to DOMFileSystemBase.cpp Here is the situation: - supportsToURL() and isValidType() can be directly removed from the BlackBerry file since the cross-platform implementation is identical. - crackFileSystemURL() adds code to deal with the cases where url.innerURL() is NULL. That's the case for non-GoogleURL, so I think we can just move this part to the cross-platform code. Actually, since there are no GoogleURLs anymore we can also remove KURL::innerURL() and all code that expects it to be non-NULL (I just checked, there's only a couple of cases). - createFileSystemURL() has a slightly different implementation. I belive the cross-platform code is wrong, though: KURL url = m_filesystemRootURL; // Remove the extra leading slash. url.setPath(url.path() + encodeWithURLEscapeSequences(fullPath.substring(1))); The problem here is that KURL::setPath() adds a leading slash if it's not present, which converts this root URL filesystem:file:///temporary/ into filesystem:/file:///temporary/ Additionally, the cross-platform code strips the leading slash from fullPath. This doesn't work in BlackBerry because m_filesystemRootURL doesn't contain a trailing slash, but I think we can fix that easily.
Created attachment 201699 [details] Patch Here goes my first proposal.
Comment on attachment 201699 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201699&action=review > Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:87 > + KURL originURL(ParsedURLString, url.path()); Is it really OK to parse just the path as a URL and claim it is a “parsed URL string”? What are you trying to accomplish here? How will originURL.path() be different from url.path()? > Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:90 > + if (path.isEmpty() || path[0] != '/') > + return false; No need for the path.isEmpty() check here. path[0] will return 0 if the path is empty. > Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:103 > + if (path.isEmpty() || path[0] != '/') > + return false; Ditto.
(In reply to comment #2) > > + KURL originURL(ParsedURLString, url.path()); > Is it really OK to parse just the path as a URL and claim it is a > “parsed URL string”? What are you trying to accomplish here? How > will originURL.path() be different from url.path()? The URL is something like this: filesystem:file:///temporary/foo In this case originURL would be file:///temporary/foo and originURL.path() would be /temporary/foo > > Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:90 > > + if (path.isEmpty() || path[0] != '/') > > + return false; > No need for the path.isEmpty() check here. path[0] will return 0 if > the path is empty. You're right, I'll correct this.
Comment on attachment 201699 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201699&action=review >> Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:87 >> + KURL originURL(ParsedURLString, url.path()); > > Is it really OK to parse just the path as a URL and claim it is a “parsed URL string”? What are you trying to accomplish here? How will originURL.path() be different from url.path()? The filesystem url is something like "filesystem:http://example.com/temporary/myfile.png", which is basically a wrapper. So url.path() is "http://example.com/temporary/myfile.png" and originalURL.path() is "/temporary/myfile.png"
Comment on attachment 201699 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201699&action=review It is awesome you are merging the two code. The KURL thingy is pretty bad though. >>> Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:87 >>> + KURL originURL(ParsedURLString, url.path()); >> >> Is it really OK to parse just the path as a URL and claim it is a “parsed URL string”? What are you trying to accomplish here? How will originURL.path() be different from url.path()? > > The filesystem url is something like "filesystem:http://example.com/temporary/myfile.png", which is basically a wrapper. So url.path() is "http://example.com/temporary/myfile.png" and originalURL.path() is "/temporary/myfile.png" Darin is absolutely right, this is completely wrong. ParsedURLString should _ONLY_ be used if the input string was already parsed and canonicalized by KURL. Code like this is a great way to get security problems. > Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:100 > + path = path.substring(1); > + > + if (path.startsWith(temporaryPathPrefix)) { > + type = FileSystemTypeTemporary; > + path = path.substring(temporaryPathPrefixLength); > + } else if (path.startsWith(persistentPathPrefix)) { > + type = FileSystemTypePersistent; > + path = path.substring(persistentPathPrefixLength); > + } else > + return false; This is very inefficient code, I don't know if you care about performance here?
(In reply to comment #5) Hey, sorry for the late reply. > > The filesystem url is something like "filesystem:http://example.com/temporary/myfile.png", which is basically a wrapper. So url.path() is "http://example.com/temporary/myfile.png" and originalURL.path() is "/temporary/myfile.png" > ParsedURLString should _ONLY_ be used if the input string was already parsed and canonicalized by KURL. Code like this is a great way to get security problems. Sure I can change that, I wonder if we should double check SecurityOrigin::extractInnerURL() as well which seems to be a similar case? > > Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:100 > > + path = path.substring(1); > > + > > + if (path.startsWith(temporaryPathPrefix)) { > > + type = FileSystemTypeTemporary; > > + path = path.substring(temporaryPathPrefixLength); > > + } else if (path.startsWith(persistentPathPrefix)) { > > + type = FileSystemTypePersistent; > > + path = path.substring(persistentPathPrefixLength); > > + } else > > + return false; > > This is very inefficient code, I don't know if you care about performance here? That I can also rework a bit.
Created attachment 202793 [details] Patch New patch, hope this one is a bit better.
Comment on attachment 202793 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=202793&action=review This is OK, but not ideal. A few comments below. > Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:90 > + if (path.isEmpty() || path[0] != '/') > + return false; The path.isEmpty() check here is redundant. Please remove it. I think I made this same comment on an earlier version of this patch. > Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:92 > + if (path.find(temporaryPathPrefix, 1) == 1) { find(x, 1) == 1 is an inefficient way to check if a prefix is present. That’s because it will keep searching for the prefix in the whole string to return an exact value of where we found it then afterward just check if it’s 1. It’s just not a good way to write the code. The good way to write it would be with hasPrefix, which we could easily do if temporaryPathPrefix had a "/" in it. > Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:95 > + } else if (path.find(persistentPathPrefix, 1) == 1) { Same comment as above. > Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:102 > + if (path.isEmpty() || path[0] != '/') > + return false; The path.isEmpty() check here is redundant. Please remove it. I think I made this same comment on an earlier version of this patch. > Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:104 > + filePath.swap(path); A little strange to do a swap here instead of an assignment. I suppose it’s more efficient.
Created attachment 203068 [details] Patch (In reply to comment #8) > The path.isEmpty() check here is redundant. Please remove it. I > think I made this same comment on an earlier version of this patch. You're right, sorry! I've just uploaded a new patch addressing all the other issues that you mentioned. temporaryPathPrefix and friends are public constants exported from the header file, so we cannot really change them. For this new patch I used String::substringSharingImpl(), which is more efficient and allows us to use startsWith() to check the prefix.
Created attachment 203069 [details] Patch (wrong file, sorry)
Comment on attachment 203069 [details] Patch Attachment 203069 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/747147 New failing tests: fast/dom/location-new-window-no-crash.html
Created attachment 203189 [details] Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Ping
Comment on attachment 203069 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203069&action=review > Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:108 > + } So how exactly will this change behavior for non BB ports? And which ports would that be? > Source/WebCore/platform/blackberry/LocalFileSystemBlackBerry.cpp:88 > + builder.append('/'); Is this change related to what the patch intends?
(In reply to comment #14) [...] I think your questions are already answered in the bug description :) I don't know if Benjamin or Darin haver further comments about this?
I guess this is a WONTFIX now (bug 122137)