On windows filepaths need \\?\ prefix to allow extended file path names. Fix ...

On windows filepaths need \\?\ prefix to allow extended file path names. Fix to bug 63574.

BUG=63574
TEST=file_system_operation_unittest.cc and manually tested on FAT32.



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=67385

[Kavita Kanetkar, 2010-11-25 03:33:06]

Please take a look.

[kinuko (google), 2010-11-25 03:36:47]

Only a few nits.

http://codereview.chromium.org/5259003/diff/1/chrome/browser/renderer_host/br...
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/5259003/diff/1/chrome/browser/renderer_host/br...
chrome/browser/renderer_host/browser_render_process_host.cc:245:
profile->GetPath()),
nit: indent

http://codereview.chromium.org/5259003/diff/1/webkit/fileapi/file_system_path...
File webkit/fileapi/file_system_path_manager.cc (right):

http://codereview.chromium.org/5259003/diff/1/webkit/fileapi/file_system_path...
webkit/fileapi/file_system_path_manager.cc:192: base_path_(profile_path),
Please remove this line and use profile_path below, or replace this line with:
base_path_(GetFileSystemCommonRootDirectory(profile_path)

[Kavita Kanetkar, 2010-11-25 03:42:03]

Thanks for the quick review!

Please take a look.

http://codereview.chromium.org/5259003/diff/1/chrome/browser/renderer_host/br...
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/5259003/diff/1/chrome/browser/renderer_host/br...
chrome/browser/renderer_host/browser_render_process_host.cc:245:
profile->GetPath()),
On 2010/11/25 03:36:47, kinuko (google) wrote:
> nit: indent

Done.

http://codereview.chromium.org/5259003/diff/1/webkit/fileapi/file_system_path...
File webkit/fileapi/file_system_path_manager.cc (right):

http://codereview.chromium.org/5259003/diff/1/webkit/fileapi/file_system_path...
webkit/fileapi/file_system_path_manager.cc:192: base_path_(profile_path),
On 2010/11/25 03:36:47, kinuko (google) wrote:
> Please remove this line and use profile_path below, or replace this line with:
> base_path_(GetFileSystemCommonRootDirectory(profile_path)

Done.

[kinuko (google), 2010-11-25 04:04:11]

lgtm

[ericu, 2010-11-25 04:18:35]

http://codereview.chromium.org/5259003/diff/9001/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_operation_unittest.cc (right):

http://codereview.chromium.org/5259003/diff/9001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation_unittest.cc:470: FilePath
dir_path(FILE_PATH_LITERAL("\\\\?\\") + dir.path().value());
I don't see anything forcing this test only to run on Windows.  Is there
anything?  If not, we'll break on Mac+Linux.

We need to test creating really long paths on each platform according to its
particular syntax [nothing special for non-Windows, the new stuff for Windows].

http://codereview.chromium.org/5259003/diff/9001/webkit/fileapi/file_system_p...
File webkit/fileapi/file_system_path_manager.h (right):

http://codereview.chromium.org/5259003/diff/9001/webkit/fileapi/file_system_p...
webkit/fileapi/file_system_path_manager.h:87: FilePath base_path_;
Why remove the const?

[Kavita Kanetkar, 2010-11-25 04:33:39]

Please take a look.

[ericu, 2010-11-25 04:37:27]

LGTM; if you can think of a better comment, feel free to update it before
committing.

http://codereview.chromium.org/5259003/diff/19001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation_unittest.cc (right):

http://codereview.chromium.org/5259003/diff/19001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation_unittest.cc:478: // windows primarily.
I feel like what we should really be doing is running the same test on all
platforms, and getting the same result.  At some point in the code, there should
be an API that just does the right thing for us without us having to know what
platform it is.  We should run tests at that level.

There's nothing wrong with this code, but the comment feels wrong to me.