Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5754002:
Moving away from shell api to support long path names on windows for filesystem. (Closed)
|
Created:
10 years ago by Kavita Kanetkar Modified:
8 years, 9 months ago Reviewers:
michaeln, Mark Mentovai, darin (slow to review), Erik does not do reviews, Paweł Hajdan Jr. CC:
chromium-reviews, ben+cc_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionContains 2 parts. Adds back \\?\ prefix to file names for extended path names.
And secondly fixes 2 methods in file_util_win.cc required by filesystem to support that pathname prefix.
BUG=63574
TEST=file_system_operation_unittest.cc
Patch Set 1 : '' #Patch Set 2 : '' #
Total comments: 23
Patch Set 3 : '' #
Total comments: 1
Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 20
Patch Set 6 : '' #
Total comments: 3
Patch Set 7 : '' #
Total comments: 37
Patch Set 8 : '' #
Total comments: 19
Patch Set 9 : '' #
Total comments: 18
Patch Set 10 : '' #
Total comments: 9
Patch Set 11 : '' #
Total comments: 10
Messages
Total messages: 21 (0 generated)
Can you please take an initial look? I have tested layout tests, unittests and all things I could think of. I haven't run any performance tests though. Thanks!
Not a full review, just some prelim comments questions about the Delete() function. http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc File base/file_util_win.cc (right): http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode139 base/file_util_win.cc:139: // Otherwise, it's a file. Try DeleteFile first because it should be faster. comment doesn't add any value and its somewhat stale... maybe just remove the comment http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode145 base/file_util_win.cc:145: // If it is, then remove it with RemoveDirectory. comment doesn't add any value, stating the obvious, maybe remove it... also the part about checking if it's a directory seems out of place since thats not done here http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode146 base/file_util_win.cc:146: return RemoveDirectory(path.value().c_str()) != 0; indent is off by one and {}'s are needed for this one line block http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode149 base/file_util_win.cc:149: // Matches posix version of iterating directories. How does this loop recurse into the child directories (does FileEnumerator drill down), or is it not supposed to do that? http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode151 base/file_util_win.cc:151: std::stack<FilePath::StringType> directories; Would it make sense to have this be a stack<FilePath> instead of strings? http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode163 base/file_util_win.cc:163: else { 'else {' should be up on the previous line http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode167 base/file_util_win.cc:167: DCHECK(::SetFileAttributes(current.value().c_str(), I don't think SetFileAttributes() call will execute in release builds since its behind a DCHECK. Its probably worth a comment about why the attributes are being examined and changed.
Thanks. Please take a look. Can either of you LGTM this eventually? http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc File base/file_util_win.cc (right): http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode139 base/file_util_win.cc:139: // Otherwise, it's a file. Try DeleteFile first because it should be faster. On 2010/12/10 20:08:44, michaeln wrote: > comment doesn't add any value and its somewhat stale... maybe just remove the > comment Done. http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode145 base/file_util_win.cc:145: // If it is, then remove it with RemoveDirectory. On 2010/12/10 20:08:44, michaeln wrote: > comment doesn't add any value, stating the obvious, maybe remove it... also the > part about checking if it's a directory seems out of place since thats not done > here Done. http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode146 base/file_util_win.cc:146: return RemoveDirectory(path.value().c_str()) != 0; On 2010/12/10 20:08:44, michaeln wrote: > indent is off by one and {}'s are needed for this one line block Done. http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode149 base/file_util_win.cc:149: // Matches posix version of iterating directories. FileEnumerator takes care of the full depth. I added 1 more subdir level in unittest. On 2010/12/10 20:08:44, michaeln wrote: > How does this loop recurse into the child directories (does FileEnumerator drill > down), or is it not supposed to do that? http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode151 base/file_util_win.cc:151: std::stack<FilePath::StringType> directories; I looked at posix. I liked stack<string> more than stack<FilePath> I can change it if you want. On 2010/12/10 20:08:44, michaeln wrote: > Would it make sense to have this be a stack<FilePath> instead of strings? http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode163 base/file_util_win.cc:163: else { On 2010/12/10 20:08:44, michaeln wrote: > 'else {' should be up on the previous line Done. http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode167 base/file_util_win.cc:167: DCHECK(::SetFileAttributes(current.value().c_str(), On 2010/12/10 20:08:44, michaeln wrote: > I don't think SetFileAttributes() call will execute in release builds since its > behind a DCHECK. > > Its probably worth a comment about why the attributes are being examined and > changed. Done.
http://codereview.chromium.org/5754002/diff/9/base/file_util_unittest.cc File base/file_util_unittest.cc (right): http://codereview.chromium.org/5754002/diff/9/base/file_util_unittest.cc#newc... base/file_util_unittest.cc:699: TEST_F(FileUtilTest, DISABLED_DeleteWildCard) { Maybe we should delete this test (and cc erikkay after initial review iteration). http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc File base/file_util_win.cc (right): http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode95 base/file_util_win.cc:95: wchar_t file_path_buf[MAX_PATH]; If we use any of these long-path unsafe methods we will get into trouble but fixing all of them would be tough... Can we put comments on long-path/extended-path safe methods (like Move, Copy and Delete) so that callers can at least make sure calling a method is safe or not? http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode168 base/file_util_win.cc:168: FILE_ATTRIBUTE_NORMAL)); Shouldn't we call this with (attribute &~ FILE_ATTRIBUTE_READONLY)? The file may have other attributes. http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode248 base/file_util_win.cc:248: if (!AbsolutePath(&real_to_path)) ...and here we're calling AbsolutePath. It is not long-path safe is it? http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode293 base/file_util_win.cc:293: FilePath::StringType suffix( nit: indent http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode305 base/file_util_win.cc:305: LOG(ERROR) << "CopyDirectory() couldn't create directory: " << nit: would better put this << in the next line, indented (here and below) http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode309 base/file_util_win.cc:309: } indent (entire this block) http://codereview.chromium.org/5754002/diff/36001/base/file_util_unittest.cc File base/file_util_unittest.cc (right): http://codereview.chromium.org/5754002/diff/36001/base/file_util_unittest.cc#... base/file_util_unittest.cc:695: Do we need these two empty lines?
Please take a look. http://codereview.chromium.org/5754002/diff/9/base/file_util_unittest.cc File base/file_util_unittest.cc (right): http://codereview.chromium.org/5754002/diff/9/base/file_util_unittest.cc#newc... base/file_util_unittest.cc:699: TEST_F(FileUtilTest, DISABLED_DeleteWildCard) { On 2010/12/10 23:03:43, kinuko wrote: > Maybe we should delete this test (and cc erikkay after initial review > iteration). Done. http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc File base/file_util_win.cc (right): http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode168 base/file_util_win.cc:168: FILE_ATTRIBUTE_NORMAL)); Actually since file was being deleted anyway, I didn't bother. But you're right. If Delete fails, it now retains other attributes except read-only. On 2010/12/10 23:03:43, kinuko wrote: > Shouldn't we call this with (attribute &~ FILE_ATTRIBUTE_READONLY)? > The file may have other attributes.
Please take a look. http://codereview.chromium.org/5754002/diff/9/base/file_util_unittest.cc File base/file_util_unittest.cc (right): http://codereview.chromium.org/5754002/diff/9/base/file_util_unittest.cc#newc... base/file_util_unittest.cc:699: TEST_F(FileUtilTest, DISABLED_DeleteWildCard) { On 2010/12/10 23:03:43, kinuko wrote: > Maybe we should delete this test (and cc erikkay after initial review > iteration). Done. http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc File base/file_util_win.cc (right): http://codereview.chromium.org/5754002/diff/9/base/file_util_win.cc#newcode168 base/file_util_win.cc:168: FILE_ATTRIBUTE_NORMAL)); Actually since file was being deleted anyway, I didn't bother. But you're right. If Delete fails, it now retains other attributes except read-only. On 2010/12/10 23:03:43, kinuko wrote: > Shouldn't we call this with (attribute &~ FILE_ATTRIBUTE_READONLY)? > The file may have other attributes.
Thanks, I put some more comments. http://codereview.chromium.org/5754002/diff/63002/base/file_util_win.cc File base/file_util_win.cc (right): http://codereview.chromium.org/5754002/diff/63002/base/file_util_win.cc#newco... base/file_util_win.cc:133: // greater than 260. Can we put this comment (maybe in a simpler form, like 'this method is extended-path safe on Windows') for all the methods that support extended paths? Also I think it'd be better to put the comment in the header file. http://codereview.chromium.org/5754002/diff/63002/base/file_util_win.cc#newco... base/file_util_win.cc:161: // This is needed because DeleteFile can fail for read-only files. nit: indentation (in this block) http://codereview.chromium.org/5754002/diff/63002/base/file_util_win.cc#newco... base/file_util_win.cc:163: if (attributes != INVALID_FILE_ATTRIBUTES && If it's INVALID_FILE_ATTRIBUTES the following DeleteFile will fail anyway? Should we exit early? http://codereview.chromium.org/5754002/diff/63002/base/file_util_win.cc#newco... base/file_util_win.cc:164: attributes &~ FILE_ATTRIBUTE_READONLY) um, you'd want to keep this '&' http://codereview.chromium.org/5754002/diff/63002/base/file_util_win.cc#newco... base/file_util_win.cc:165: ::SetFileAttributes(current.value().c_str(), FILE_ATTRIBUTE_NORMAL); and replace this FILE_ATTRIBUTE_NORMAL with (attributes &~ FILE_ATTRIBUTE_READONLY). http://codereview.chromium.org/5754002/diff/63002/base/file_util_win.cc#newco... base/file_util_win.cc:245: if (!real_to_path.StartsWithExtendedPathPrefix() && nit: indentation http://codereview.chromium.org/5754002/diff/63002/base/file_util_win.cc#newco... base/file_util_win.cc:285: DCHECK(recursive || DirectoryExists(from_path)); Now we may want some comment here (as we do in file_util_posix) explaining why we DCHECK here? http://codereview.chromium.org/5754002/diff/63002/base/file_util_win.cc#newco... base/file_util_win.cc:315: return success; I really wonder if we could just have this one in file_util.cc and deprecate file_util_posix's implementation. The whole logic is not very simple and having duplicated code here and there is very unfortunate. We can use FilePath::kSeparators for the path separator, and make StartsWithExtendedPathPrefix() just returns false for non-windows implementation. (And same for Delete. Though we need to hide or separate out attribute checks to make it work for all platforms.) http://codereview.chromium.org/5754002/diff/63002/webkit/fileapi/file_system_... File webkit/fileapi/file_system_operation_unittest.cc (right): http://codereview.chromium.org/5754002/diff/63002/webkit/fileapi/file_system_... webkit/fileapi/file_system_operation_unittest.cc:436: FilePath dir_path(FILE_PATH_LITERAL("\\\\?\\") + dir.path().value()); Can we use FilePath::kExtendedPathPrefix? http://codereview.chromium.org/5754002/diff/63002/webkit/fileapi/file_system_... webkit/fileapi/file_system_operation_unittest.cc:457: "0123456789012345678901234567890123456789"); It might be better to define FilePath::String long_path_char160("01234...") and use it here and above? http://codereview.chromium.org/5754002/diff/63002/webkit/fileapi/file_system_... File webkit/fileapi/file_system_path_manager.cc (right): http://codereview.chromium.org/5754002/diff/63002/webkit/fileapi/file_system_... webkit/fileapi/file_system_path_manager.cc:179: FilePath::StringType extended_length_str(L"\\\\?\\"); Can we use FilePath::kExtendedPathPrefix? Also might it better making sure if it doesn't start with the prefix before doing that? (Now we have the method for it)
Thanks Kinuko for the review, adding Erik to review too. http://codereview.chromium.org/5754002/diff/63002/base/file_util_win.cc File base/file_util_win.cc (right): http://codereview.chromium.org/5754002/diff/63002/base/file_util_win.cc#newco... base/file_util_win.cc:133: // greater than 260. On 2010/12/15 00:09:30, kinuko wrote: > Can we put this comment (maybe in a simpler form, like 'this method is > extended-path safe on Windows') for all the methods that support extended paths? > Also I think it'd be better to put the comment in the header file. Done. http://codereview.chromium.org/5754002/diff/63002/base/file_util_win.cc#newco... base/file_util_win.cc:161: // This is needed because DeleteFile can fail for read-only files. On 2010/12/15 00:09:30, kinuko wrote: > nit: indentation (in this block) Done. http://codereview.chromium.org/5754002/diff/63002/base/file_util_win.cc#newco... base/file_util_win.cc:163: if (attributes != INVALID_FILE_ATTRIBUTES && I thought about it but I feel it's ok to let Delete fail. On 2010/12/15 00:09:30, kinuko wrote: > If it's INVALID_FILE_ATTRIBUTES the following DeleteFile will fail anyway? > Should we exit early? http://codereview.chromium.org/5754002/diff/63002/base/file_util_win.cc#newco... base/file_util_win.cc:165: ::SetFileAttributes(current.value().c_str(), FILE_ATTRIBUTE_NORMAL); Oops yes. On 2010/12/15 00:09:30, kinuko wrote: > and replace this FILE_ATTRIBUTE_NORMAL with (attributes &~ > FILE_ATTRIBUTE_READONLY). http://codereview.chromium.org/5754002/diff/63002/base/file_util_win.cc#newco... base/file_util_win.cc:245: if (!real_to_path.StartsWithExtendedPathPrefix() && On 2010/12/15 00:09:30, kinuko wrote: > nit: indentation Done. http://codereview.chromium.org/5754002/diff/63002/base/file_util_win.cc#newco... base/file_util_win.cc:285: DCHECK(recursive || DirectoryExists(from_path)); On 2010/12/15 00:09:30, kinuko wrote: > Now we may want some comment here (as we do in file_util_posix) explaining why > we DCHECK here? Done. http://codereview.chromium.org/5754002/diff/63002/webkit/fileapi/file_system_... File webkit/fileapi/file_system_operation_unittest.cc (right): http://codereview.chromium.org/5754002/diff/63002/webkit/fileapi/file_system_... webkit/fileapi/file_system_operation_unittest.cc:436: FilePath dir_path(FILE_PATH_LITERAL("\\\\?\\") + dir.path().value()); On 2010/12/15 00:09:30, kinuko wrote: > Can we use FilePath::kExtendedPathPrefix? Done. http://codereview.chromium.org/5754002/diff/63002/webkit/fileapi/file_system_... webkit/fileapi/file_system_operation_unittest.cc:457: "0123456789012345678901234567890123456789"); On 2010/12/15 00:09:30, kinuko wrote: > It might be better to define FilePath::String long_path_char160("01234...") and > use it here and above? Done. http://codereview.chromium.org/5754002/diff/63002/webkit/fileapi/file_system_... File webkit/fileapi/file_system_path_manager.cc (right): http://codereview.chromium.org/5754002/diff/63002/webkit/fileapi/file_system_... webkit/fileapi/file_system_path_manager.cc:179: FilePath::StringType extended_length_str(L"\\\\?\\"); On 2010/12/15 00:09:30, kinuko wrote: > Can we use FilePath::kExtendedPathPrefix? > Also might it better making sure if it doesn't start with the prefix before > doing that? (Now we have the method for it) Done.
Thanks, a few more nits (in addition to the comment issue we talked offline). I still wonder if we can put the common implementation in file_util.cc. The code this patch is adding is mostly same with what we have in file_util_posix.cc. http://codereview.chromium.org/5754002/diff/73001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_operation_unittest.cc (right): http://codereview.chromium.org/5754002/diff/73001/webkit/fileapi/file_system_... webkit/fileapi/file_system_operation_unittest.cc:444: const base::StringPiece kLongPathChar160( const char kLongPathChar160[] would be enough? http://codereview.chromium.org/5754002/diff/73001/webkit/fileapi/file_system_... webkit/fileapi/file_system_operation_unittest.cc:447: "0123456789012345678901234567890123456789" ); nit: extra spaces at the end (between '"' and ')') http://codereview.chromium.org/5754002/diff/73001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_path_manager.cc (right): http://codereview.chromium.org/5754002/diff/73001/webkit/fileapi/file_system_... webkit/fileapi/file_system_path_manager.cc:183: } else { this else block looks not necessary.
Drive-by with some test comments. http://codereview.chromium.org/5754002/diff/84001/base/file_util_unittest.cc File base/file_util_unittest.cc (right): http://codereview.chromium.org/5754002/diff/84001/base/file_util_unittest.cc#... base/file_util_unittest.cc:741: file_util::CreateDirectory(sub_subdir_path1); nit: Should you check return value of this? http://codereview.chromium.org/5754002/diff/84001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_operation_unittest.cc (right): http://codereview.chromium.org/5754002/diff/84001/webkit/fileapi/file_system_... webkit/fileapi/file_system_operation_unittest.cc:464: // ScopedTempDir does not delete dir with long name since it does Oh, should we rather fix ScopedTempDir?
+mark who knows the FilePath issues much better than I I think the key issue here is looking at what you did in file_system_path_manager.cc. I don't want to see code like this sprinkled throughout the system. This seems like an important issue to fix generally across Chrome (I imagine it's the cause of any number of odd bugs out in the field). http://codereview.chromium.org/5754002/diff/84001/base/file_util_unittest.cc File base/file_util_unittest.cc (left): http://codereview.chromium.org/5754002/diff/84001/base/file_util_unittest.cc#... base/file_util_unittest.cc:699: TEST_F(FileUtilTest, DeleteWildCard) { Are you sure nobody's using this feature? Should we add a test that verifies that if you try to use a wildcard that it causes an error? http://codereview.chromium.org/5754002/diff/84001/base/file_util_unittest.cc File base/file_util_unittest.cc (right): http://codereview.chromium.org/5754002/diff/84001/base/file_util_unittest.cc#... base/file_util_unittest.cc:741: file_util::CreateDirectory(sub_subdir_path1); On 2010/12/16 08:58:40, Paweł Hajdan Jr. wrote: > nit: Should you check return value of this? doesn't the next assert effectively do this? http://codereview.chromium.org/5754002/diff/84001/base/file_util_win.cc File base/file_util_win.cc (right): http://codereview.chromium.org/5754002/diff/84001/base/file_util_win.cc#newco... base/file_util_win.cc:159: // This is needed because DeleteFile can fail for read-only files. doesn't this mean that we need the same logic above? should we just move it into DeleteFile? http://codereview.chromium.org/5754002/diff/84001/base/file_util_win.cc#newco... base/file_util_win.cc:162: attributes & FILE_ATTRIBUTE_READONLY) multiline conditional plus multiline body means you need {} http://codereview.chromium.org/5754002/diff/84001/base/file_util_win.cc#newco... base/file_util_win.cc:237: !AbsolutePath(&real_to_path)) This seems to imply that AbsolutePath returns the wrong value with StartsWithExtendedPathPrefix is true. Shouldn't we fix AbsolutePath? http://codereview.chromium.org/5754002/diff/84001/base/file_util_win.cc#newco... base/file_util_win.cc:249: if (real_to_path.value().size() >= real_from_path.value().size() && Is this just testing whether from is an ancestor of to? if so, I think we already have a method on FilePath to do this for you. http://codereview.chromium.org/5754002/diff/84001/base/file_util_win.cc#newco... base/file_util_win.cc:262: // |to_path| may not exist yet, start the loop with |to_path|. this comment doesn't seem right http://codereview.chromium.org/5754002/diff/84001/base/file_util_win.cc#newco... base/file_util_win.cc:267: success = false; Why does the code continue from here? The next lines are no-ops right? Just return false. http://codereview.chromium.org/5754002/diff/84001/base/file_util_win.cc#newco... base/file_util_win.cc:277: // This matches previous shell implementation that assumed that does the POSIX impl match this behavior as well? or is there some code that depends on this behavior that's windows-only? http://codereview.chromium.org/5754002/diff/84001/base/file_util_win.cc#newco... base/file_util_win.cc:286: // Strip the leading '\' (if any). nit: newline above // http://codereview.chromium.org/5754002/diff/84001/base/file_util_win.cc#newco... base/file_util_win.cc:294: if (::CreateDirectory(target_path.value().c_str(), NULL) == 0) { doesn't this mean that if CreateDirectory fails, the copy might believe that it succeeded when it didn't? http://codereview.chromium.org/5754002/diff/84001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_operation_unittest.cc (right): http://codereview.chromium.org/5754002/diff/84001/webkit/fileapi/file_system_... webkit/fileapi/file_system_operation_unittest.cc:464: // ScopedTempDir does not delete dir with long name since it does On 2010/12/16 08:58:40, Paweł Hajdan Jr. wrote: > Oh, should we rather fix ScopedTempDir? Agree. We use ScopedTempDir in a number of places, so we should make sure that actually works as well. http://codereview.chromium.org/5754002/diff/84001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_path_manager.cc (right): http://codereview.chromium.org/5754002/diff/84001/webkit/fileapi/file_system_... webkit/fileapi/file_system_path_manager.cc:179: if (!root_path.StartsWithExtendedPathPrefix()) { I really don't like this approach. Doesn't this mean we need to sprinkle this logic around everywhere in our code that manipulates FilePath objects? It seems like we should come up with a way to handle this in FilePath.Append where it magically prepends the prefix when it needs to.
http://codereview.chromium.org/5754002/diff/84001/base/file_path.h File base/file_path.h (right): http://codereview.chromium.org/5754002/diff/84001/base/file_path.h#newcode155 base/file_path.h:155: // Path length is limited to 260 on windows unless prefixed Windows, a proper name, requires a capital letter. Same on line 317. http://codereview.chromium.org/5754002/diff/84001/base/file_path.h#newcode317 base/file_path.h:317: // On windows, returns whether or not path starts with extended path prefix. path. Which path? The path? extended path prefix. Which extended path prefix? The extended path prefix? // On Windows, returns true if the path starts with the extended path prefix. http://codereview.chromium.org/5754002/diff/84001/base/file_path.h#newcode318 base/file_path.h:318: bool StartsWithExtendedPathPrefix() const; A new function requires a unit test, even if it’s a simple and relatively stupid test. http://codereview.chromium.org/5754002/diff/84001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_path_manager.cc (right): http://codereview.chromium.org/5754002/diff/84001/webkit/fileapi/file_system_... webkit/fileapi/file_system_path_manager.cc:179: if (!root_path.StartsWithExtendedPathPrefix()) { This is wrong. It should check whether root_path is already a UNC path. Whether it’s a UNC path in \\?\ or anywhere else is irrelevant. http://codereview.chromium.org/5754002/diff/84001/webkit/fileapi/file_system_... webkit/fileapi/file_system_path_manager.cc:182: return FilePath(extended_length_str).Append(kFileSystemDirectory); Nobody takes care to do this anywhere else in Chrome as far as I’m aware. I think we’re generally expecting that if people are working with tremendously-long paths, they’ll need to come at us in UNC format anyway. They’ll need to come at many other programs in that format as well. What’s special about this case?
Thanks for the review. Please take a look. http://codereview.chromium.org/5754002/diff/84001/base/file_path.h File base/file_path.h (right): http://codereview.chromium.org/5754002/diff/84001/base/file_path.h#newcode155 base/file_path.h:155: // Path length is limited to 260 on windows unless prefixed On 2010/12/16 19:48:19, Mark Mentovai wrote: > Windows, a proper name, requires a capital letter. Same on line 317. Done. http://codereview.chromium.org/5754002/diff/84001/base/file_path.h#newcode317 base/file_path.h:317: // On windows, returns whether or not path starts with extended path prefix. On 2010/12/16 19:48:19, Mark Mentovai wrote: > path. Which path? The path? > > extended path prefix. Which extended path prefix? The extended path prefix? > > // On Windows, returns true if the path starts with the extended path prefix. Done. http://codereview.chromium.org/5754002/diff/84001/base/file_path.h#newcode318 base/file_path.h:318: bool StartsWithExtendedPathPrefix() const; On 2010/12/16 19:48:19, Mark Mentovai wrote: > A new function requires a unit test, even if it’s a simple and relatively stupid > test. Done. http://codereview.chromium.org/5754002/diff/84001/base/file_util_unittest.cc File base/file_util_unittest.cc (left): http://codereview.chromium.org/5754002/diff/84001/base/file_util_unittest.cc#... base/file_util_unittest.cc:699: TEST_F(FileUtilTest, DeleteWildCard) { I think so. I will email chromium-dev for heads-up. On 2010/12/16 18:34:54, Erik Kay wrote: > Are you sure nobody's using this feature? Should we add a test that verifies > that if you try to use a wildcard that it causes an error? http://codereview.chromium.org/5754002/diff/84001/base/file_util_win.cc File base/file_util_win.cc (right): http://codereview.chromium.org/5754002/diff/84001/base/file_util_win.cc#newco... base/file_util_win.cc:159: // This is needed because DeleteFile can fail for read-only files. DeleteFile resolves to BOOL WINAPI DeleteFile( __in LPCTSTR lpFileName ); Hence I had to keep it out of DeleteFile. On 2010/12/16 18:34:54, Erik Kay wrote: > doesn't this mean that we need the same logic above? should we just move it > into DeleteFile? http://codereview.chromium.org/5754002/diff/84001/base/file_util_win.cc#newco... base/file_util_win.cc:162: attributes & FILE_ATTRIBUTE_READONLY) On 2010/12/16 18:34:54, Erik Kay wrote: > multiline conditional plus multiline body means you need {} Done. http://codereview.chromium.org/5754002/diff/84001/base/file_util_win.cc#newco... base/file_util_win.cc:237: !AbsolutePath(&real_to_path)) AbsolutePath currently creates: wchar_t file_path_buf[MAX_PATH] for it to pass to _wfullpath And for every call to create: wchar_t array of size 32K seemed like a bad idea. If there's any other way around it, I can try fixing AbsolutePath() On 2010/12/16 18:34:54, Erik Kay wrote: > This seems to imply that AbsolutePath returns the wrong value with > StartsWithExtendedPathPrefix is true. Shouldn't we fix AbsolutePath? http://codereview.chromium.org/5754002/diff/84001/base/file_util_win.cc#newco... base/file_util_win.cc:249: if (real_to_path.value().size() >= real_from_path.value().size() && I don't think it handles cases such as: to_path = C:\a\b\c\d\e from_path = C:\a\b Since it's not a strict parent of from_path. On 2010/12/16 18:34:54, Erik Kay wrote: > Is this just testing whether from is an ancestor of to? if so, I think we > already have a method on FilePath to do this for you. Done. http://codereview.chromium.org/5754002/diff/84001/base/file_util_win.cc#newco... base/file_util_win.cc:262: // |to_path| may not exist yet, start the loop with |to_path|. On 2010/12/16 18:34:54, Erik Kay wrote: > this comment doesn't seem right Done. http://codereview.chromium.org/5754002/diff/84001/base/file_util_win.cc#newco... base/file_util_win.cc:267: success = false; On 2010/12/16 18:34:54, Erik Kay wrote: > Why does the code continue from here? The next lines are no-ops right? Just > return false. Done. http://codereview.chromium.org/5754002/diff/84001/base/file_util_win.cc#newco... base/file_util_win.cc:277: // This matches previous shell implementation that assumed that Yes. Actually previous shell32 implementation matches this. And posix version was written to match it. On 2010/12/16 18:34:54, Erik Kay wrote: > does the POSIX impl match this behavior as well? or is there some code that > depends on this behavior that's windows-only? http://codereview.chromium.org/5754002/diff/84001/base/file_util_win.cc#newco... base/file_util_win.cc:286: // Strip the leading '\' (if any). On 2010/12/16 18:34:54, Erik Kay wrote: > nit: newline above // Done. http://codereview.chromium.org/5754002/diff/84001/base/file_util_win.cc#newco... base/file_util_win.cc:294: if (::CreateDirectory(target_path.value().c_str(), NULL) == 0) { No. This part of the code checks if target directory exists already and we're trying to Create it. If so, count it as a success case (Matches posix code errno != EEXIST) On 2010/12/16 18:34:54, Erik Kay wrote: > doesn't this mean that if CreateDirectory fails, the copy might believe that it > succeeded when it didn't? http://codereview.chromium.org/5754002/diff/84001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_path_manager.cc (right): http://codereview.chromium.org/5754002/diff/84001/webkit/fileapi/file_system_... webkit/fileapi/file_system_path_manager.cc:179: if (!root_path.StartsWithExtendedPathPrefix()) { Isn't it the case that even in UNC if path doesn't begin with \\?\ it will be restricted to 260? Maybe I don't understand your comment correctly. On 2010/12/16 19:48:19, Mark Mentovai wrote: > This is wrong. It should check whether root_path is already a UNC path. Whether > it’s a UNC path in \\?\ or anywhere else is irrelevant. http://codereview.chromium.org/5754002/diff/84001/webkit/fileapi/file_system_... webkit/fileapi/file_system_path_manager.cc:182: return FilePath(extended_length_str).Append(kFileSystemDirectory); Yes. Actually shell32 api that restricts path lengths to 260 is used in many places. The fix for allowing extended path lengths with filesystem was a priority because the profile directory and the filesystem root path was already too long. Hence user-created files/directories need extended path support. On 2010/12/16 19:48:19, Mark Mentovai wrote: > Nobody takes care to do this anywhere else in Chrome as far as I’m aware. I > think we’re generally expecting that if people are working with > tremendously-long paths, they’ll need to come at us in UNC format anyway. > They’ll need to come at many other programs in that format as well. > > What’s special about this case?
http://codereview.chromium.org/5754002/diff/84001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_path_manager.cc (right): http://codereview.chromium.org/5754002/diff/84001/webkit/fileapi/file_system_... webkit/fileapi/file_system_path_manager.cc:179: if (!root_path.StartsWithExtendedPathPrefix()) { On 2010/12/16 18:34:54, Erik Kay wrote: > I really don't like this approach. Doesn't this mean we need to sprinkle this > logic around everywhere in our code that manipulates FilePath objects? It seems > like we should come up with a way to handle this in FilePath.Append where it > magically prepends the prefix when it needs to. In gears this was handled by doing the \\?\'ification just prior to making win32 system calls instead of trying to make sure that paths were expressed in the long'ish way throughout. http://code.google.com/p/gears/source/browse/trunk/gears/base/common/file_win...
http://codereview.chromium.org/5754002/diff/105001/base/file_path.cc File base/file_path.cc (right): http://codereview.chromium.org/5754002/diff/105001/base/file_path.cc#newcode39 base/file_path.cc:39: "\\\\?\\"); This is wrapped poorly. Wrap after the =, so FILE_PATH_LITERAL("\\\\?\\"); is on a line by itself. http://codereview.chromium.org/5754002/diff/105001/webkit/fileapi/file_system... File webkit/fileapi/file_system_path_manager.cc (right): http://codereview.chromium.org/5754002/diff/105001/webkit/fileapi/file_system... webkit/fileapi/file_system_path_manager.cc:179: if (!root_path.StartsWithExtendedPathPrefix()) { I thought that UNC paths weren’t restricted to 260 characters, and that \\?\ was used to get paths into UNC-land to avoid the limit. I didn’t think there was anything special about \\?\ that isn’t also special about other UNC paths.
http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc File base/file_util_win.cc (right): http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... base/file_util_win.cc:159: // This is needed because DeleteFile can fail for read-only files. Should the read-only attribute testing/fixing also be done for the simple non-recursive delete a single file case (line 140)? http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... base/file_util_win.cc:238: !AbsolutePath(&real_to_path)) Maybe AbsolutePath should test for the extended path prefix instead of doing it at each callsite? http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... base/file_util_win.cc:253: return false; I think these up front tests could be simplified... if (PathExists(to_path)) real_to_path = to_path; else real_to_path = to_path.DirName(); if (!AbsolutePath(real_to_path) || !AbsolutePath(real_from_path)) return false; I'm not sure what the last condition is checking for, if its parent/child relationship i think there's a helper for that? http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... base/file_util_win.cc:257: static_cast<FileEnumerator::FILE_TYPE>(FileEnumerator::FILES); Are these static_casts needed? http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... base/file_util_win.cc:261: FileEnumerator traversal(from_path, recursive, traverse_type); It would be nice to instance this enumerator closer to the while loop down below. http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... base/file_util_win.cc:262: FilePath current = from_path; Why is 'real_from_path' not used here? http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... base/file_util_win.cc:263: if (!PathExists(from_path)) { Would be nice to move this 'are the inputs valid' type of test up closer to the other tests of that nature. http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... base/file_util_win.cc:287: if (!suffix.empty()) { Would 'relative_path' be a better name? Is this ever expected to be empty in this loop? http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... base/file_util_win.cc:293: if (DirectoryExists(current)) { Could this system call be avoided by using the file info for the current item of the FileEnumerator?
On 2010/12/17 02:32:03, michaeln wrote: > http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... > base/file_util_win.cc:253: return false; > I think these up front tests could be simplified... > > if (PathExists(to_path)) > real_to_path = to_path; > else > real_to_path = to_path.DirName(); > > if (!AbsolutePath(real_to_path) || !AbsolutePath(real_from_path)) > return false; > > I'm not sure what the last condition is checking for, if its parent/child > relationship i think there's a helper for that? > > http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... > base/file_util_win.cc:257: > static_cast<FileEnumerator::FILE_TYPE>(FileEnumerator::FILES); > Are these static_casts needed? > > http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... > base/file_util_win.cc:261: FileEnumerator traversal(from_path, recursive, > traverse_type); > It would be nice to instance this enumerator closer to the while loop down > below. > > http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... > base/file_util_win.cc:262: FilePath current = from_path; > Why is 'real_from_path' not used here? > > http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... > base/file_util_win.cc:263: if (!PathExists(from_path)) { > Would be nice to move this 'are the inputs valid' type of test up closer to the > other tests of that nature. > > http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... > base/file_util_win.cc:287: if (!suffix.empty()) { > Would 'relative_path' be a better name? Is this ever expected to be empty in > this loop? This CopyDirectory routine is not very easy to understand, and I agree with many of your comments here (e.g. the first AbsolutePath logic looks too verbose, the var name suffix is not very intuitive etc). But I would rather first want to see if we could factor out a common part from file_util_{posix,win}.cc and put it in file_util.cc... since most of the code seems to be taken from file_util_posix.cc. If we clean up the code, it'd be better done in a consistent way across platforms imho. That's why I keep asking this.
Thanks for patiently working on this - I made some more comments. I'd like erik/michael to take another look but to me it looks very close now. http://codereview.chromium.org/5754002/diff/117001/base/file_path.cc File base/file_path.cc (right): http://codereview.chromium.org/5754002/diff/117001/base/file_path.cc#newcode47 base/file_path.cc:47: const int kSharePrefixLength = 2; arraysize(kSharePrefix) would also work. http://codereview.chromium.org/5754002/diff/117001/base/file_path.cc#newcode1208 base/file_path.cc:1208: // TODO(kkanetkar): Taken from gears. Replace this when 'Replace this' may not correctly state what we want to do? We'll need this logic but want to call it differently (automatically like in Append). http://codereview.chromium.org/5754002/diff/117001/base/file_path.h File base/file_path.h (right): http://codereview.chromium.org/5754002/diff/117001/base/file_path.h#newcode322 base/file_path.h:322: StringType ToLongFileNameHack(const StringType& path) const; If it's not called anywhere outside the class, can we move it into anonymous namespace in .cc file? http://codereview.chromium.org/5754002/diff/117001/base/file_path.h#newcode326 base/file_path.h:326: FilePath GetLongPathHack() const; Can you put a comment that: - this also prepends the extended path prefix so that the path can become longer than MAX_PATH char - the path needs to exist to make this return a correct long path if it has 8.3 short names in it http://codereview.chromium.org/5754002/diff/117001/base/file_path_unittest.cc File base/file_path_unittest.cc (right): http://codereview.chromium.org/5754002/diff/117001/base/file_path_unittest.cc... base/file_path_unittest.cc:1113: long_path2_value.c_str(), WriteInto(&short_path, MAX_PATH), MAX_PATH); I think we can just use long_path2.value().c_str() here. http://codereview.chromium.org/5754002/diff/117001/base/file_path_unittest.cc... base/file_path_unittest.cc:1117: FilePath path8_3(short_path); Do we need path8_3? Using short_path looks fine to me. http://codereview.chromium.org/5754002/diff/117001/base/file_path_unittest.cc... base/file_path_unittest.cc:1123: // Should be a no-op if path starts with extended path prefix. nit: if path starts ... -> if path already starts http://codereview.chromium.org/5754002/diff/117001/base/file_path_unittest.cc... base/file_path_unittest.cc:1127: // Should be a no-op if path starts with UNC prefix. ditto. http://codereview.chromium.org/5754002/diff/117001/base/file_path_unittest.cc... base/file_path_unittest.cc:1135: expected_prefix.append(FPL("filer\\home\\me")); nit: s/filer/unc_path/ and s/expected_prefix/prefixed_unc_path/? http://codereview.chromium.org/5754002/diff/121002/base/file_util_win.cc File base/file_util_win.cc (right): http://codereview.chromium.org/5754002/diff/121002/base/file_util_win.cc#newc... base/file_util_win.cc:246: if (!AbsolutePath(&real_to_path)) Shouldn't we call the startWith checks before calling AbsolutePath (if we don't modify AbsolutePath)? Might have been better to keep the FilePath::StartsWith...() method as well. if (StartsWith(path_, kExtendedPathPrefix, false) || StartsWith(path_, kUNCExtendedPathPrefix, false)) http://codereview.chromium.org/5754002/diff/121002/base/file_util_win.cc#newc... base/file_util_win.cc:280: static_cast<FileEnumerator::FILE_TYPE>(FileEnumerator::FILES); (repeating michael's question) is this static_cast necessary? http://codereview.chromium.org/5754002/diff/121002/base/file_util_win.cc#newc... base/file_util_win.cc:289: FilePath::StringType relative_path_suffix( I think michael meant suffix -> relative_path rather than relative_path_suffix. Also can you update the comment to match the code? http://codereview.chromium.org/5754002/diff/121002/base/file_util_win.cc#newc... base/file_util_win.cc:301: if (traversal.IsDirectory(info)) { FileEnumerator::IsDirectory(info) would be better? IsDirectory is a static method. http://codereview.chromium.org/5754002/diff/121002/webkit/fileapi/file_system... File webkit/fileapi/file_system_operation_unittest.cc (right): http://codereview.chromium.org/5754002/diff/121002/webkit/fileapi/file_system... webkit/fileapi/file_system_operation_unittest.cc:435: FilePath dir_path(FilePath::kExtendedPathPrefix + dir.path().value()); Can we simply call GetLongPathHack() here?
MSDN doc describing \\?\ and \\?\UNC\: http://msdn.microsoft.com/en-us/library/aa365247(v=vs.85).aspx#maxpath
Thanks for the review. Please take a look. Mark, I have included code to deal with UNC and extended path prefixes. Now the caller can call methods on FilePath rather than prefixing paths themselves. Thanks, Kavita http://codereview.chromium.org/5754002/diff/84001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_path_manager.cc (right): http://codereview.chromium.org/5754002/diff/84001/webkit/fileapi/file_system_... webkit/fileapi/file_system_path_manager.cc:179: if (!root_path.StartsWithExtendedPathPrefix()) { Changed this to a central place in FilePath. And made FilePath a bit smarter to manipulate extended paths and 8.3 format ones. On 2010/12/16 18:34:54, Erik Kay wrote: > I really don't like this approach. Doesn't this mean we need to sprinkle this > logic around everywhere in our code that manipulates FilePath objects? It seems > like we should come up with a way to handle this in FilePath.Append where it > magically prepends the prefix when it needs to. http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc File base/file_util_win.cc (right): http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... base/file_util_win.cc:159: // This is needed because DeleteFile can fail for read-only files. On 2010/12/17 02:32:03, michaeln wrote: > Should the read-only attribute testing/fixing also be done for the simple > non-recursive delete a single file case (line 140)? Done. http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... base/file_util_win.cc:238: !AbsolutePath(&real_to_path)) As discussed, leaving this as-is. On 2010/12/17 02:32:03, michaeln wrote: > Maybe AbsolutePath should test for the extended path prefix instead of doing it > at each callsite? http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... base/file_util_win.cc:253: return false; The helper is only for strict parent/child. It won't work for a/b/c/d/ and a/b/ . On 2010/12/17 02:32:03, michaeln wrote: > I think these up front tests could be simplified... > > if (PathExists(to_path)) > real_to_path = to_path; > else > real_to_path = to_path.DirName(); > > if (!AbsolutePath(real_to_path) || !AbsolutePath(real_from_path)) > return false; > > I'm not sure what the last condition is checking for, if its parent/child > relationship i think there's a helper for that? http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... base/file_util_win.cc:257: static_cast<FileEnumerator::FILE_TYPE>(FileEnumerator::FILES); Yes On 2010/12/17 02:32:03, michaeln wrote: > Are these static_casts needed? http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... base/file_util_win.cc:261: FileEnumerator traversal(from_path, recursive, traverse_type); On 2010/12/17 02:32:03, michaeln wrote: > It would be nice to instance this enumerator closer to the while loop down > below. Done. http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... base/file_util_win.cc:263: if (!PathExists(from_path)) { On 2010/12/17 02:32:03, michaeln wrote: > Would be nice to move this 'are the inputs valid' type of test up closer to the > other tests of that nature. Done. http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... base/file_util_win.cc:287: if (!suffix.empty()) { On 2010/12/17 02:32:03, michaeln wrote: > Would 'relative_path' be a better name? Is this ever expected to be empty in > this loop? Done. http://codereview.chromium.org/5754002/diff/105001/base/file_util_win.cc#newc... base/file_util_win.cc:293: if (DirectoryExists(current)) { On 2010/12/17 02:32:03, michaeln wrote: > Could this system call be avoided by using the file info for the current item of > the FileEnumerator? Done. http://codereview.chromium.org/5754002/diff/117001/base/file_path.cc File base/file_path.cc (right): http://codereview.chromium.org/5754002/diff/117001/base/file_path.cc#newcode47 base/file_path.cc:47: const int kSharePrefixLength = 2; On 2010/12/22 23:20:49, kinuko wrote: > arraysize(kSharePrefix) would also work. Done. http://codereview.chromium.org/5754002/diff/117001/base/file_path.cc#newcode1208 base/file_path.cc:1208: // TODO(kkanetkar): Taken from gears. Replace this when On 2010/12/22 23:20:49, kinuko wrote: > 'Replace this' may not correctly state what we want to do? We'll need this > logic but want to call it differently (automatically like in Append). Done. http://codereview.chromium.org/5754002/diff/117001/base/file_path.h File base/file_path.h (right): http://codereview.chromium.org/5754002/diff/117001/base/file_path.h#newcode322 base/file_path.h:322: StringType ToLongFileNameHack(const StringType& path) const; On 2010/12/22 23:20:49, kinuko wrote: > If it's not called anywhere outside the class, can we move it into anonymous > namespace in .cc file? Done. http://codereview.chromium.org/5754002/diff/117001/base/file_path.h#newcode326 base/file_path.h:326: FilePath GetLongPathHack() const; On 2010/12/22 23:20:49, kinuko wrote: > Can you put a comment that: > - this also prepends the extended path prefix so that the path can become longer > than MAX_PATH char > - the path needs to exist to make this return a correct long path if it has 8.3 > short names in it Done. http://codereview.chromium.org/5754002/diff/117001/base/file_path_unittest.cc File base/file_path_unittest.cc (right): http://codereview.chromium.org/5754002/diff/117001/base/file_path_unittest.cc... base/file_path_unittest.cc:1113: long_path2_value.c_str(), WriteInto(&short_path, MAX_PATH), MAX_PATH); On 2010/12/22 23:20:49, kinuko wrote: > I think we can just use long_path2.value().c_str() here. Done. http://codereview.chromium.org/5754002/diff/117001/base/file_path_unittest.cc... base/file_path_unittest.cc:1117: FilePath path8_3(short_path); Sorry. That was before I changed the GetLongPathHack() in FilePath to return a FilePath than manipulate the path_ internally. Done. On 2010/12/22 23:20:49, kinuko wrote: > Do we need path8_3? Using short_path looks fine to me. http://codereview.chromium.org/5754002/diff/117001/base/file_path_unittest.cc... base/file_path_unittest.cc:1123: // Should be a no-op if path starts with extended path prefix. On 2010/12/22 23:20:49, kinuko wrote: > nit: if path starts ... -> if path already starts Done. http://codereview.chromium.org/5754002/diff/117001/base/file_path_unittest.cc... base/file_path_unittest.cc:1127: // Should be a no-op if path starts with UNC prefix. On 2010/12/22 23:20:49, kinuko wrote: > ditto. Done. http://codereview.chromium.org/5754002/diff/117001/base/file_path_unittest.cc... base/file_path_unittest.cc:1135: expected_prefix.append(FPL("filer\\home\\me")); On 2010/12/22 23:20:49, kinuko wrote: > nit: s/filer/unc_path/ and s/expected_prefix/prefixed_unc_path/? Done. http://codereview.chromium.org/5754002/diff/121002/base/file_util_win.cc File base/file_util_win.cc (right): http://codereview.chromium.org/5754002/diff/121002/base/file_util_win.cc#newc... base/file_util_win.cc:246: if (!AbsolutePath(&real_to_path)) On 2010/12/22 23:20:49, kinuko wrote: > Shouldn't we call the startWith checks before calling AbsolutePath (if we don't > modify AbsolutePath)? > > Might have been better to keep the FilePath::StartsWith...() method as well. > > if (StartsWith(path_, kExtendedPathPrefix, false) || > StartsWith(path_, kUNCExtendedPathPrefix, false)) Done. http://codereview.chromium.org/5754002/diff/121002/base/file_util_win.cc#newc... base/file_util_win.cc:289: FilePath::StringType relative_path_suffix( I liked suffix var name better. But changed since you both want it changed. On 2010/12/22 23:20:49, kinuko wrote: > I think michael meant suffix -> relative_path rather than relative_path_suffix. > Also can you update the comment to match the code? http://codereview.chromium.org/5754002/diff/121002/base/file_util_win.cc#newc... base/file_util_win.cc:301: if (traversal.IsDirectory(info)) { I needed to replace this with DirectoryExists() need to ask current whether it's a directory, current is assigned to traversal.Next() at the end. On 2010/12/22 23:20:49, kinuko wrote: > FileEnumerator::IsDirectory(info) would be better? > > IsDirectory is a static method. http://codereview.chromium.org/5754002/diff/121002/webkit/fileapi/file_system... File webkit/fileapi/file_system_operation_unittest.cc (right): http://codereview.chromium.org/5754002/diff/121002/webkit/fileapi/file_system... webkit/fileapi/file_system_operation_unittest.cc:435: FilePath dir_path(FilePath::kExtendedPathPrefix + dir.path().value()); On 2010/12/22 23:20:49, kinuko wrote: > Can we simply call GetLongPathHack() here? Done.
http://codereview.chromium.org/5754002/diff/129012/base/file_path.cc File base/file_path.cc (right): http://codereview.chromium.org/5754002/diff/129012/base/file_path.cc#newcode176 base/file_path.cc:176: DWORD length_to_try = static_cast<DWORD>(path.length() * 2); I don't understand this *2 logic. Why not just pass NULL and always retrieve the length first? http://codereview.chromium.org/5754002/diff/129012/base/file_path.cc#newcode182 base/file_path.cc:182: return path; I'm a bit nervous that this won't work in all cases. Here's the scenario: (1) get a FilePath object that's a short name near the 260 char limit (2) call Append to add a path component to it, pushing it past 260 (say you want to create a subdirectory) (3) now we call ToLongFileNameHack which returns the path that you passed in without the parent being converted to a proper long name (4) we prepend \\? to this filename since (5) the file operation fails since the path is bogus I don't have a great suggestion here. We may need to look at the callsites that could generate a problem like this and see if there are's anything obvious we could do. Maybe we need to figure out how to make sure we're never passing around short names to begin with. http://codereview.chromium.org/5754002/diff/129012/base/file_path.cc#newcode1240 base/file_path.cc:1240: if (StartsWith(path_, kExtendedPathPrefix, false) || why not call StartsWithExtendedPathOrUNCPrefix? http://codereview.chromium.org/5754002/diff/129012/base/file_path.h File base/file_path.h (right): http://codereview.chromium.org/5754002/diff/129012/base/file_path.h#newcode160 base/file_path.h:160: // '\\' Prefix for network shares. nit: newline above comments http://codereview.chromium.org/5754002/diff/129012/base/file_path.h#newcode325 base/file_path.h:325: // Note: The path needs to exist to make this return a correct long path This isn't acceptable in FilePath. One of FilePath's invariants is that it's supposed to never generate IO. It should be safe to call FilePath methods from any thread. If we need to do this 8.3 shortname lookup, then we need to add it to file_util instead. Maybe that's OK since the only time we should be doing this is inside of file_util already, right? http://codereview.chromium.org/5754002/diff/129012/base/file_path_unittest.cc File base/file_path_unittest.cc (right): http://codereview.chromium.org/5754002/diff/129012/base/file_path_unittest.cc... base/file_path_unittest.cc:1094: TEST_F(FilePathTest, TestGetLongPathHack) { we may need to test some really long paths as well (some near 32767) http://codereview.chromium.org/5754002/diff/129012/base/file_util.h File base/file_util.h (right): http://codereview.chromium.org/5754002/diff/129012/base/file_util.h#newcode103 base/file_util.h:103: // Safe to pass extended-path to this method on Windows. It seems better to mark all of the ones that are unsafe and add a TODO / bug # to all of those so that they can be fixed. http://codereview.chromium.org/5754002/diff/129012/base/file_util_win.cc File base/file_util_win.cc (right): http://codereview.chromium.org/5754002/diff/129012/base/file_util_win.cc#newc... base/file_util_win.cc:144: bool Delete(const FilePath& path, bool recursive) { it seems like this Delete change is worthwhile on its own independent of the extended length path change. you may want to just split it out into a separate CL so that it can be landed / managed independently. http://codereview.chromium.org/5754002/diff/129012/webkit/fileapi/file_system... File webkit/fileapi/file_system_operation_unittest.cc (right): http://codereview.chromium.org/5754002/diff/129012/webkit/fileapi/file_system... webkit/fileapi/file_system_operation_unittest.cc:431: TEST_F(FileSystemOperationTest, TestCreateVeryLongName) { again, we may want to test a much longer path here (closer to the new max) http://codereview.chromium.org/5754002/diff/129012/webkit/fileapi/file_system... webkit/fileapi/file_system_operation_unittest.cc:435: FilePath dir_path(dir.path().GetLongPathHack()); This wasn't really what I had in mind. We don't want these #ifdefs sprinkled throughout the code. Can't we do this directly in each of the various file_util:: methods? That way the only place this needs to exist is in file_util_win.cc. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
