Bug 78106 – [chromium] Store URL string for File.path() to handle files on remote filesystems

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Kinuko Yasuda]

[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.

[Kinuko Yasuda]

Created attachment 126070 [details]
Patch

[WebKit Review Bot]

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.

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Philippe Normand]

Comment on attachment 126070 [details]
Patch

Attachment 126070 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11449415

[Kinuko Yasuda]

Created attachment 126073 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Darin Fisher (:fishd, Google)]

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?

[Kinuko Yasuda]

Created attachment 126220 [details]
Patch

[Kinuko Yasuda]

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.

[WebKit Review Bot]

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

[Kinuko Yasuda]

Created attachment 126508 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Kinuko Yasuda]

Created attachment 126676 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Kinuko Yasuda]

Created attachment 126709 [details]
Patch

Fixing tests

[Kinuko Yasuda]

Ok, now the cr-linux tests look good (finally).  Darin, could you take another look at the patch?  Thanks!

[Darin Fisher (:fishd, Google)]

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?

[Kinuko Yasuda]

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.

[Kinuko Yasuda]

Created attachment 126953 [details]
Patch

[WebKit Review Bot]

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.

[Kinuko Yasuda]

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?

[Kinuko Yasuda]

Created attachment 126956 [details]
Patch

[WebKit Review Bot]

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.

[Kinuko Yasuda]

Update for style-check is somehow failing but all other tests look good.

[Darin Fisher (:fishd, Google)]

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?

[Kinuko Yasuda]

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?