[chromium] Store URL string for File.path() to handle files on remote filesystems Chromium port stores local file path string to File.path() (as most other ports do) but this limits the usage of File object only to local files. As we're planning to support remote files on remote filesystems in File/FileSystem API we should use URL string (i.e. file: or filesystem: URLs) instead of local file paths.
Created attachment 126070 [details] Patch
Attachment 126070 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/chromium/FileSystemChromium.cpp:41: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebKit/chromium/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] Source/WebKit/chromium/ChangeLog:13: Line contains tab character. [whitespace/tab] [5] Source/WebKit/chromium/ChangeLog:27: Line contains tab character. [whitespace/tab] [5] Source/WebKit/chromium/ChangeLog:29: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:7: Line contains tab character. [whitespace/tab] [5] Source/WebKit/chromium/src/WebDragData.cpp:137: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebKit/chromium/src/WebFileChooserCompletionImpl.cpp:32: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 9 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment on attachment 126070 [details] Patch Attachment 126070 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11449415
Created attachment 126073 [details] Patch
Comment on attachment 126073 [details] Patch Attachment 126073 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11460430 New failing tests: http/tests/local/formdata/send-form-data-constructed-from-form.html http/tests/local/fileapi/send-dragged-file.html fast/events/drag-to-navigate.html http/tests/local/formdata/send-form-data-with-sliced-file.html http/tests/local/formdata/send-form-data.html http/tests/local/formdata/upload-events.html http/tests/local/formdata/form-data-with-unknown-file-extension.html
Comment on attachment 126073 [details] Patch Attachment 126073 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11449461 New failing tests: http/tests/local/formdata/send-form-data-constructed-from-form.html http/tests/local/fileapi/send-dragged-file.html fast/events/drag-to-navigate.html http/tests/local/formdata/send-form-data-with-sliced-file.html http/tests/local/formdata/send-form-data.html http/tests/local/formdata/upload-events.html http/tests/local/formdata/form-data-with-unknown-file-extension.html
Comment on attachment 126073 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126073&action=review > Source/WebCore/platform/chromium/FileSystemChromium.cpp.orig:42 > +<<<<<<< HEAD oops > Source/WebKit/chromium/public/platform/WebHTTPBody.h:103 > + WEBKIT_EXPORT void appendURLRange(const WebURL&, long long start, long long length, double modificationTime); Perhaps you should document that only blob:, file: and filesystem: URLs are supported? (Please also make the code assert this if it does not already.) > Source/WebKit/chromium/src/WebHTTPBody.cpp:160 > + m_private->appendFileRange(String::fromUTF8(url.spec().data()), start, length, modificationTime); does this one support blob: URLs?
Created attachment 126220 [details] Patch
Comment on attachment 126073 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126073&action=review >> Source/WebCore/platform/chromium/FileSystemChromium.cpp.orig:42 >> +<<<<<<< HEAD > > oops Deleted. Apparently airplane wasn't the best place to make a patch... >> Source/WebKit/chromium/public/platform/WebHTTPBody.h:103 >> + WEBKIT_EXPORT void appendURLRange(const WebURL&, long long start, long long length, double modificationTime); > > Perhaps you should document that only blob:, file: and filesystem: URLs are supported? (Please also make the code assert this if it does not already.) Done. >> Source/WebKit/chromium/src/WebHTTPBody.cpp:160 >> + m_private->appendFileRange(String::fromUTF8(url.spec().data()), start, length, modificationTime); > > does this one support blob: URLs? No. Added assertion.
Comment on attachment 126220 [details] Patch Attachment 126220 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11487090 New failing tests: http/tests/local/formdata/send-form-data-constructed-from-form.html http/tests/local/fileapi/send-dragged-file.html fast/events/drag-to-navigate.html http/tests/local/formdata/send-form-data-with-sliced-file.html http/tests/local/formdata/send-form-data.html http/tests/local/formdata/upload-events.html http/tests/local/formdata/form-data-with-unknown-file-extension.html
Created attachment 126508 [details] Patch
Comment on attachment 126508 [details] Patch Attachment 126508 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11487724 New failing tests: http/tests/local/formdata/send-form-data-constructed-from-form.html http/tests/local/fileapi/send-dragged-file.html fast/events/drag-to-navigate.html http/tests/local/formdata/send-form-data-with-sliced-file.html http/tests/local/formdata/send-form-data.html http/tests/local/formdata/upload-events.html http/tests/local/formdata/form-data-with-unknown-file-extension.html
Comment on attachment 126508 [details] Patch Attachment 126508 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11485673 New failing tests: http/tests/local/formdata/send-form-data-constructed-from-form.html http/tests/local/fileapi/send-dragged-file.html fast/events/drag-to-navigate.html http/tests/local/formdata/send-form-data-with-sliced-file.html http/tests/local/formdata/send-form-data.html http/tests/local/formdata/upload-events.html http/tests/local/formdata/form-data-with-unknown-file-extension.html
Created attachment 126676 [details] Patch
Comment on attachment 126676 [details] Patch Attachment 126676 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11509410 New failing tests: fast/events/drag-to-navigate.html
Comment on attachment 126676 [details] Patch Attachment 126676 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11507503 New failing tests: fast/events/drag-to-navigate.html
Created attachment 126709 [details] Patch Fixing tests
Ok, now the cr-linux tests look good (finally). Darin, could you take another look at the patch? Thanks!
Comment on attachment 126709 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126709&action=review > Source/WebCore/platform/KURL.cpp:1978 > +String filePathToFileURL(const String& path) It is kind of unfortunate that we need to fork this code into WebKit. You could also just add these methods to WebKitPlatformSupport, so that you can back them up with net::FileURLToFilePath/FilePathToFileURL. That way, if someone improves those methods, we won't end up with fragmented behavior. > Source/WebCore/platform/KURL.h:301 > +String filePathToFileURL(const String&); only the chromium port needs this, right? > Source/WebKit/chromium/src/ChromeClientImpl.cpp:719 > + params.initialValue = fileURLToFilePath(params.selectedFiles[0]); it seems like the FileChooser interface should also change to work with file: URLs instead of raw file paths. can you add a FIXME for that?
Comment on attachment 126709 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126709&action=review >> Source/WebCore/platform/KURL.h:301 >> +String filePathToFileURL(const String&); > > only the chromium port needs this, right? The issue is we're calling it in DOMFileSystem code which is outside the chromium port.. (otherwise calling PlatformSupport's one would have been definitely better) well let me take another look to see if we can intercept the callback chain while it's in chromium ports.
Created attachment 126953 [details] Patch
Attachment 126953 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets Using index info to reconstruct a base tree... <stdin>:1578: trailing whitespace. <stdin>:1647: trailing whitespace. <stdin>:1657: trailing whitespace. <stdin>:1672: trailing whitespace. return 0; <stdin>:1674: trailing whitespace. warning: squelched 7 whitespace errors warning: 12 lines add whitespace errors. Falling back to patching base and 3-way merge... warning: too many files (created: 168768 deleted: 3), skipping inexact rename detection Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Auto-merging Tools/ChangeLog CONFLICT (content): Merge conflict in Tools/ChangeLog Failed to merge in the changes. Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 126709 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126709&action=review >> Source/WebCore/platform/KURL.cpp:1978 >> +String filePathToFileURL(const String& path) > > It is kind of unfortunate that we need to fork this code into WebKit. > You could also just add these methods to WebKitPlatformSupport, so > that you can back them up with net::FileURLToFilePath/FilePathToFileURL. > That way, if someone improves those methods, we won't end up with > fragmented behavior. Removed this code. >>> Source/WebCore/platform/KURL.h:301 >>> +String filePathToFileURL(const String&); >> >> only the chromium port needs this, right? > > The issue is we're calling it in DOMFileSystem code which is outside the chromium port.. (otherwise calling PlatformSupport's one would have been definitely better) well let me take another look to see if we can intercept the callback chain while it's in chromium ports. Done. The new patch still has code in WebKitPlatformSupport::fileURLToPath() but I will remove them as soon as the chrome-side change is landed. >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:719 >> + params.initialValue = fileURLToFilePath(params.selectedFiles[0]); > > it seems like the FileChooser interface should also change to work with file: URLs instead of raw file paths. can you add a FIXME for that? WebFileChooserCompletionImpl already has the change, and the corresponding chrome patch has the same change in FileChooserCompletion code for ppapi... Would that be enough?
Created attachment 126956 [details] Patch
Attachment 126956 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets Using index info to reconstruct a base tree... <stdin>:1578: trailing whitespace. <stdin>:1647: trailing whitespace. <stdin>:1657: trailing whitespace. <stdin>:1672: trailing whitespace. return 0; <stdin>:1674: trailing whitespace. warning: squelched 7 whitespace errors warning: 12 lines add whitespace errors. Falling back to patching base and 3-way merge... warning: too many files (created: 168768 deleted: 3), skipping inexact rename detection Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging Source/WebCore/ChangeLog Auto-merging Tools/ChangeLog CONFLICT (content): Merge conflict in Tools/ChangeLog Failed to merge in the changes. Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.
Update for style-check is somehow failing but all other tests look good.
Comment on attachment 126956 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126956&action=review > Source/WebCore/platform/chromium/FileSystemChromium.cpp:41 > +static String filePathOrFileURLToPath(const String& pathOrUrl) nit: pathOrUrl -> pathOrURL nit: perhaps you could just name this function toFilePath?
Thanks Darin. As Brett raised a concern about using URL for local file path I'll likely drop this patch but will be making some changes in WebCore. (In reply to comment #28) > (From update of attachment 126956 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126956&action=review > > > Source/WebCore/platform/chromium/FileSystemChromium.cpp:41 > > +static String filePathOrFileURLToPath(const String& pathOrUrl) > > nit: pathOrUrl -> pathOrURL > > nit: perhaps you could just name this function toFilePath?