Bug 75049

Summary: [filesystem/Chromium] filesystem URLs are not fully supported
Product: WebKit Reporter: Eric U. <ericu>
Component: DOMAssignee: Eric U. <ericu>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, brettw, dglazkov, fishd, jamesr, kinuko, michaeln, serya, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Small plumbing changes; the real work's done in googleURL and the Chromium codebase.
none
Patch
none
Patch
none
Patch none

Description Eric U. 2011-12-21 15:33:44 PST
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.
Comment 1 Eric U. 2011-12-21 15:57:35 PST
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.
Comment 2 WebKit Review Bot 2011-12-21 15:59:34 PST
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.
Comment 3 WebKit Review Bot 2011-12-21 16:19:36 PST
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
Comment 4 Adam Barth 2011-12-21 23:01:22 PST
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 { }
Comment 5 Eric U. 2012-01-24 14:43:24 PST
Created attachment 123813 [details]
Patch
Comment 6 Eric U. 2012-01-24 14:44:34 PST
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.
Comment 7 Kinuko Yasuda 2012-01-24 15:09:40 PST
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.
Comment 8 Eric U. 2012-01-24 15:28:31 PST
(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.
Comment 9 Kinuko Yasuda 2012-01-24 15:45:36 PST
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.
Comment 10 Early Warning System Bot 2012-01-24 16:16:48 PST
Comment on attachment 123813 [details]
Patch

Attachment 123813 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11342115
Comment 11 Gyuyoung Kim 2012-01-24 16:17:24 PST
Comment on attachment 123813 [details]
Patch

Attachment 123813 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11344106
Comment 12 Eric U. 2012-01-24 17:24:39 PST
Created attachment 123853 [details]
Patch
Comment 13 Eric U. 2012-01-24 17:25:08 PST
Comment on attachment 123853 [details]
Patch

Added missing header include.
Comment 14 WebKit Review Bot 2012-01-24 18:20:16 PST
Comment on attachment 123853 [details]
Patch

Clearing flags on attachment: 123853

Committed r105843: <http://trac.webkit.org/changeset/105843>
Comment 15 WebKit Review Bot 2012-01-24 18:20:22 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 James Robinson 2012-01-24 21:19:33 PST
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.
Comment 17 Eric U. 2012-01-25 13:28:07 PST
Created attachment 123998 [details]
Patch
Comment 18 Eric U. 2012-01-25 13:28:39 PST
Comment on attachment 123998 [details]
Patch

Fixed a test expectation I missed last time.
Comment 19 WebKit Review Bot 2012-01-25 14:43:36 PST
Comment on attachment 123998 [details]
Patch

Clearing flags on attachment: 123998

Committed r105930: <http://trac.webkit.org/changeset/105930>
Comment 20 WebKit Review Bot 2012-01-25 14:43:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Mark Rowe (bdash) 2012-01-25 17:05:30 PST
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.
Comment 22 Mark Rowe (bdash) 2012-01-25 17:25:33 PST
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.
Comment 23 Eric U. 2012-02-02 11:17:17 PST
This patch was removed due to a Chromium test failure.
Comment 24 Eric U. 2012-02-02 11:30:28 PST
Scratch that--apparently I misunderstood.  The patch is still in there.  Sorry for the noise.
Comment 25 Sergey Ryazanov 2012-04-20 05:02:19 PDT
*** Bug 75701 has been marked as a duplicate of this bug. ***