Bug 75049 – [filesystem/Chromium] filesystem URLs are not fully supported

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Eric U.]

The current implementation of filesystem URLs does the bare minimum to load simple URLs.
URLs for filesystem objects with interesting characters in their names [e.g. % and non-ASCII] either don't work or look terrible.
Queries and references aren't implemented at all.
Escaping is missing.

As soon as https://bugs.webkit.org/show_bug.cgi?id=62813 lands, this will be a much bigger problem, as users will be free to make whatever interesting filenames they want, but it's already a problem.

This change will depend on changes to the googleurl library [up for review at http://codereview.appspot.com/4961060/], and won't actually fix Chromium until the corresponding Chromium-side changes are made [in progress at http://codereview.chromium.org/7811006, not quite in review yet].

This CL won't build without the googleURL changes, but once they're in, should be harmless [not change any behavior] until the Chromium code hits with the #define that turns on the new googleurl behavior.

[Eric U.]

Created attachment 120229 [details]
Small plumbing changes; the real work's done in googleURL and the Chromium codebase.

This won't build until the googleurl changes land.

[WebKit Review Bot]

Attachment 120229 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
	A	LayoutTests/fast/html/font-weight-bold-for-b-and-strong-expected.png
	A	LayoutTests/fast/html/font-weight-bold-for-b-and-strong-expected.txt
	A	LayoutTests/fast/html/font-weight-bold-for-b-and-strong.html
	M	LayoutTests/ChangeLog
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/css/html.css
r103468 = 97efa88a66ff7f597257cf86d0f8315cd016e1e3 (refs/remotes/origin/master)
First, rewinding head to replay your work on top of it...
Applying: Inform the scrolling coordinator when scrollbar layers come and go
Using index info to reconstruct a base tree...
<stdin>:474806: trailing whitespace.
        [Chromium] DatabaseTrackerChromium: iterating DatabaseSet races with Database disposal on worker thread 
<stdin>:474827: trailing whitespace.
        Nothing to test, just removing redundant code. Correct behavior tested by 
<stdin>:475346: trailing whitespace.
    
warning: 3 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 167254 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/chromium/test_expectations.txt
CONFLICT (content): Merge conflict in LayoutTests/platform/chromium/test_expectations.txt
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebCore/page/ScrollingCoordinator.h
CONFLICT (content): Merge conflict in Source/WebCore/page/ScrollingCoordinator.h
Auto-merging Source/WebCore/page/mac/ScrollingCoordinatorMac.mm
CONFLICT (content): Merge conflict in Source/WebCore/page/mac/ScrollingCoordinatorMac.mm
Auto-merging Source/WebCore/platform/ScrollView.cpp
Auto-merging Source/WebCore/rendering/RenderLayerCompositor.cpp
CONFLICT (content): Merge conflict in Source/WebCore/rendering/RenderLayerCompositor.cpp
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Auto-merging Tools/Scripts/build-webkit
Auto-merging Tools/Scripts/webkitdirs.pm
CONFLICT (content): Merge conflict in Tools/Scripts/webkitdirs.pm
Failed to merge in the changes.
Patch failed at 0001 Inform the scrolling coordinator when scrollbar layers come and go

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


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 120229 [details]
Small plumbing changes; the real work's done in googleURL and the Chromium codebase.

Attachment 120229 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10997176

[Adam Barth]

Comment on attachment 120229 [details]
Small plumbing changes; the real work's done in googleURL and the Chromium codebase.

View in context: https://bugs.webkit.org/attachment.cgi?id=120229&action=review

> Source/WebCore/page/SecurityOrigin.cpp:85
> +#if ENABLE(FILE_SYSTEM)

This shouldn't be guarded with ENABLE(FILE_SYSTEM).  It has nothing to do with FILE_SYSTEM.

> Source/WebCore/platform/KURL.h:226
> +#if ENABLE(FILE_SYSTEM)

This shouldn't be guarded with ENABLE(FILE_SYSTEM).  It has nothing to do with FILE_SYSTEM.

> Source/WebCore/platform/KURLGoogle.cpp:292
> +void KURLGooglePrivate::initInnerURL() {

{ should be on the next line.

> Source/WebCore/platform/KURLGoogle.cpp:337
> +    if (m_innerURL) {
> +      dest->m_innerURL = adoptPtr(new KURL(m_innerURL->copy()));
> +    }
> +    else
> +      dest->m_innerURL.clear();

Bad indent and we can drop the { }

[Eric U.]

Created attachment 123813 [details]
Patch

[Eric U.]

Comment on attachment 123813 [details]
Patch

Rolled in code review feedback.  Also, the googleurl changes are now in, so the bots should be happy with the change now.

[Kinuko Yasuda]

Comment on attachment 123813 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123813&action=review

> Source/WebCore/fileapi/EntryBase.cpp:78
> +    result.append(encodeWithURLEscapeSequences(m_fullPath));

Based on what I experienced on my reverted change this will break the chromeos tests.  (The tests expect toURL() return URL with unescaped directory separators)

This will escape all the directory separators as well-- I want to make sure this is what we want.

[Eric U.]

(In reply to comment #7)
> (From update of attachment 123813 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123813&action=review
> 
> > Source/WebCore/fileapi/EntryBase.cpp:78
> > +    result.append(encodeWithURLEscapeSequences(m_fullPath));
> 
> Based on what I experienced on my reverted change this will break the chromeos tests.  (The tests expect toURL() return URL with unescaped directory separators)
> 
> This will escape all the directory separators as well-- I want to make sure this is what we want.

That's taken care of in WebCore::encodeWithURLEscapeSequences.  After everything's escaped, I unescape just the directory separators.

[Kinuko Yasuda]

Comment on attachment 123813 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123813&action=review

>>> Source/WebCore/fileapi/EntryBase.cpp:78
>>> +    result.append(encodeWithURLEscapeSequences(m_fullPath));
>> 
>> Based on what I experienced on my reverted change this will break the chromeos tests.  (The tests expect toURL() return URL with unescaped directory separators)
>> 
>> This will escape all the directory separators as well-- I want to make sure this is what we want.
> 
> That's taken care of in WebCore::encodeWithURLEscapeSequences.  After everything's escaped, I unescape just the directory separators.

Oh I see, now I found the corresponding change in the function.  Cool so I no longer need to work around the escaping issue when I try relanding my change.

[Early Warning System Bot]

Comment on attachment 123813 [details]
Patch

Attachment 123813 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11342115

[Gyuyoung Kim]

Comment on attachment 123813 [details]
Patch

Attachment 123813 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11344106

[Eric U.]

Created attachment 123853 [details]
Patch

[Eric U.]

Comment on attachment 123853 [details]
Patch

Added missing header include.

[WebKit Review Bot]

Comment on attachment 123853 [details]
Patch

Clearing flags on attachment: 123853

Committed r105843: <http://trac.webkit.org/changeset/105843>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

This patch appears to have broken KURLTest.Encode:

http://build.webkit.org/builders/Chromium%20Mac%20Release%20%28Tests%29/builds/12331/steps/webkit-unit-tests/logs/stdio

[ RUN      ] KURLTest.Encode
/Users/chrome-bot/Desktop/WebKit-BuildSlave/chromium-mac-release-tests/build/Source/WebKit/chromium/tests/KURLTest.cpp:336: Failure
Value of: output
  Actual: 4-byte object <60-8A 36-00>
Expected: expectedOutput
Which is: 4-byte object <50-64 32-00>

Was this change in behavior intentional? If so, please update the test.

[Eric U.]

Created attachment 123998 [details]
Patch

[Eric U.]

Comment on attachment 123998 [details]
Patch

Fixed a test expectation I missed last time.

[WebKit Review Bot]

Comment on attachment 123998 [details]
Patch

Clearing flags on attachment: 123998

Committed r105930: <http://trac.webkit.org/changeset/105930>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Rowe (bdash)]

As with the first time you landed this patch, r105930 broke the Mac build.  The build has been broken red for over two hours at this point because of this.  Please fix it, and pay attention to the build bots in the future.

[Mark Rowe (bdash)]

Since people really love to be able to compile WebKit, I fixed this in r105950 by reapplying the exact same build fix that was made the first time this was landed.

[Eric U.]

This patch was removed due to a Chromium test failure.

[Eric U.]

Scratch that--apparently I misunderstood.  The patch is still in there.  Sorry for the noise.

[Sergey Ryazanov]

*** Bug 75701 has been marked as a duplicate of this bug. ***