Bug 62813

Summary: [filesystem] Remove old filesystem naming restrictions
Product: WebKit Reporter: Eric U. <ericu>
Component: DOMAssignee: Eric U. <ericu>
Status: RESOLVED FIXED    
Severity: Normal CC: kinuko, levin, michaeln, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 62811    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch none

Description Eric U. 2011-06-16 13:29:29 PDT 
The FileSystem spec has changed, and in particular has removed a lot of restrictions on how one can name files and directories [see http://dev.w3.org/2009/dap/file-system/file-dir-sys.html].  We should remove the restrictions from DOMFilePath::isValidPath.

Before this change goes in, however, Chromium will have to fix http://code.google.com/p/chromium/issues/detail?id=78860 [via https://bugs.webkit.org/show_bug.cgi?id=62811] or it will suddenly get much, much worse.
Comment 1 Eric U. 2011-12-21 11:16:53 PST 
Created attachment 120195 [details]
Patch
Comment 2 Eric U. 2011-12-21 11:18:11 PST 
The chromium bug isn't fixed yet, but it's close, so I think we can go ahead and fix this one now, given that the code I'm removing has threading issues.
Comment 3 WebKit Review Bot 2011-12-21 11:24:32 PST 
Attachment 120195 [details] did not pass style-queue:

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

Last 3072 characters of output:
ed.txt
	M	LayoutTests/http/tests/inspector/compiler-source-mapping.html
	D	LayoutTests/platform/chromium/inspector/extensions/extensions-api-expected.txt
	M	LayoutTests/ChangeLog
	M	Source/cmake/WebKitFS.cmake
	M	Source/WebKit/chromium/WebKit.gyp
	M	Source/WebKit/chromium/ChangeLog
	M	Source/WebKit/chromium/scripts/generate_devtools_extension_api.py
	M	Source/WebKit/chromium/scripts/concatenate_js_files.py
	A	Source/WebKit/chromium/src/js/DevToolsExtensionAPI.js
	M	Source/WebKit/chromium/src/js/DevTools.js
	M	Source/WebKit/chromium/WebKit.gypi
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/WebCore.gypi
	M	Source/WebCore/inspector/front-end/DebuggerPresentationModel.js
	M	Source/WebCore/inspector/front-end/ExtensionAPI.js
	M	Source/WebCore/inspector/front-end/CompilerSourceMapping.js
	M	Source/WebCore/inspector/front-end/utilities.js
	M	Source/WebCore/inspector/front-end/ExtensionServer.js
	M	Source/WebKit2/ChangeLog
	M	Source/WebKit2/UIProcess/API/mac/WKProcessGroup.mm
	M	Source/WebKit2/UIProcess/API/mac/WKConnection.mm
	M	Source/WebKit2/UIProcess/API/mac/WKBrowsingContextController.mm
	M	Source/WebKit2/UIProcess/API/qt/tests/html/scroll.html
	M	Source/WebKit2/UIProcess/API/qt/tests/qquickwebview/tst_qquickwebview.cpp
	A	Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_favIconLoad.qml
	A	Source/WebKit2/UIProcess/API/qt/tests/qmltests/common/favicon.html
	A	Source/WebKit2/UIProcess/API/qt/tests/qmltests/common/favicon.png
	A	Source/WebKit2/UIProcess/API/qt/tests/qmltests/common/favicon2.html
	A	Source/WebKit2/UIProcess/API/qt/tests/qmltests/common/small-favicon.png
	M	Source/WebKit2/UIProcess/API/qt/tests/qmltests/qmltests.pro
	M	Source/WebKit2/UIProcess/API/qt/qwebiconimageprovider.cpp
	M	Source/WebKit2/UIProcess/win/WebPopupMenuProxyWin.cpp
	M	Source/CMakeLists.txt
	M	CMakeLists.txt
W: -empty_dir: trunk/LayoutTests/platform/chromium/inspector/extensions/extensions-api-expected.txt
r103417 = 9ddda54f0d64b5c0fdf7c62c872f46a53facd64b (refs/remotes/origin/master)
	M	Tools/Scripts/build-webkit
	M	Tools/Scripts/webkitdirs.pm
	M	Tools/ChangeLog
r103418 = 3a244c58aa52b4caba3d417cb4c59c5eadab09b4 (refs/remotes/origin/master)
First, rewinding head to replay your work on top of it...
Applying: Cleanup up clients when deallocating WebKit2 API objects
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
CONFLICT (rename/delete): Rename LayoutTests/platform/chromium/inspector/extensions/extensions-api-expected.txt->LayoutTests/inspector/extensions/extensions-api-expected.txt in HEAD and deleted in Cleanup up clients when deallocating WebKit2 API objects
Failed to merge in the changes.
Patch failed at 0001 Cleanup up clients when deallocating WebKit2 API objects

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 4 Eric Seidel (no email) 2011-12-21 13:55:47 PST 
Comment on attachment 120195 [details]
Patch

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

Who would be famliar enough with the spec to review this?  (even a non-reviewer)

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

This statement is false. :)
Comment 5 Eric U. 2011-12-21 14:08:22 PST 
(In reply to comment #4)
> (From update of attachment 120195 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120195&action=review
> 
> Who would be famliar enough with the spec to review this?  (even a non-reviewer)

Kinuko [on the CC list] knows it well.  I believe David Levin's going to look at it, though, since this CL removes some non-thread-safe code that's annoying him.
 
> > Source/WebCore/ChangeLog:8
> > +        No new tests. (OOPS!)
> 
> This statement is false. :)

Well, modifications of existing tests, I suppose.  OOPS! indeed.  ;'>
Comment 6 David Levin 2011-12-21 16:00:15 PST 
Comment on attachment 120195 [details]
Patch

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

>>> Source/WebCore/ChangeLog:8
>>> +        No new tests. (OOPS!)
>> 
>> This statement is false. :)
> 
> Well, modifications of existing tests, I suppose.  OOPS! indeed.  ;'>

Just remove it and put in the correct statement.

> Source/WebCore/fileapi/DOMFilePath.cpp:122
> +    // Embedded NULs are not allowed.

NUL? Embedbed \0 are not allowed.

> Source/WebCore/fileapi/DOMFilePath.cpp:123
> +    if (path.find((UChar)0) != WTF::notFound)

Use c++ style casts. Or why not just do '\0'?
Comment 7 Eric U. 2011-12-21 16:23:37 PST 
Created attachment 120236 [details]
Patch
Comment 8 WebKit Review Bot 2011-12-21 16:25:29 PST 
Attachment 120236 [details] did not pass style-queue:

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

Updating OpenSource
From git://git.webkit.org/WebKit
   723f8ee..710a358  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 103470 = 723f8ee6c151d1039a19237711dc00f3284f27f8
r103471 = 710a358c829343885bc1490e41fde775bbcdfd98
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
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: 167519 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 9 David Levin 2011-12-21 16:34:14 PST 
*** Bug 74969 has been marked as a duplicate of this bug. ***
Comment 10 WebKit Review Bot 2011-12-22 03:03:46 PST 
Comment on attachment 120236 [details]
Patch

Clearing flags on attachment: 120236

Committed r103516: <http://trac.webkit.org/changeset/103516>
Comment 11 WebKit Review Bot 2011-12-22 03:03:52 PST 
All reviewed patches have been landed.  Closing bug.
Comment 12 Ryosuke Niwa 2011-12-22 12:12:47 PST 
fast/filesystem/op-restricted-chars.html is failing on Windows because you can't have : in filenames on Windows:
* Running: RestrictedChars
PASS Succeeded: "/".getFile("a<b")
PASS Succeeded: "/".getFile("a>b")
FAIL Got unexpected error 1 while "/".getFile("a:b")
FAIL Got error 1
PASS successfullyParsed is true

TEST COMPLETE

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=fast%2Ffilesystem%2Fop-restricted-chars.html
Comment 13 Ryosuke Niwa 2011-12-22 12:14:11 PST 
Filed https://bugs.webkit.org/show_bug.cgi?id=75110 to track the failure.