Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12212010:
Truncate the download file name if it exceeds the filesystem limit. (Closed)
|
Created:
7 years, 10 months ago by kinaba Modified:
7 years, 9 months ago CC:
chromium-reviews, benjhayden+dwatch_chromium.org, erikwright+watch_chromium.org, rdsmith+dwatch_chromium.org, sail+watch_chromium.org, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionTruncate the download file name if it exceeds the filesystem limit.
BUG=162734
TEST=Manually tested (Rename a file on Google Drive to >300 characters, and download).
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=183727
Patch Set 1 : #
Total comments: 6
Patch Set 2 : Partially addressed review comments (#3). #
Total comments: 8
Patch Set 3 : Rebase + review fix (#5,#6). #Patch Set 4 : Add Windows (UTF-16) support. #
Total comments: 6
Patch Set 5 : Move to file_util, care about MAX_PATH, uniquification #
Total comments: 3
Patch Set 6 : GetVolumePathName(). #
Total comments: 6
Patch Set 7 : Review fix (#14), rebase. #
Total comments: 10
Patch Set 8 : Add test. #
Total comments: 2
Messages
Total messages: 25 (0 generated)
+asanka, +rdsmith, could you take a look from download people's perspective? (after I could be sure that I'm not going completely wrong direction, I'd ask someone else about base/ addition.) This is for crbug/162734, the case when suggested file name is too long for the filesystem, which especially causes problem in ChromeOS having shorter limit (143) but it occurs elsewhere, too (typical limit on Win/Linux/Mac is 255).
(Oops, I'll take care of the android failure later...)
I think this basic approach makes sense. There are some nits I'd like to pick at (as noted), but I think it makes sense to do that after you get first round feedback from the base/ folks. Note: Asanka's OOO until this coming Monday, and wow will he have a lot of email to deal with when he comes back. Having said that, he's very much the right downloads person to review this CL, so hopefully we can avoid committing until he had a chance to look at it. It's not that far away. https://codereview.chromium.org/12212010/diff/5002/chrome/browser/download/do... File chrome/browser/download/download_path_reservation_tracker.cc (right): https://codereview.chromium.org/12212010/diff/5002/chrome/browser/download/do... chrome/browser/download/download_path_reservation_tracker.cc:106: // the comment for FilePath::AsUTF8Unsafe), hence no safe way to truncate. I'm inclined to think that we'll end up with a better user experience if we power through and do it anyway, but that might be worth checking with people wiser in the ways of character encodings than I. https://codereview.chromium.org/12212010/diff/5002/chrome/browser/download/do... chrome/browser/download/download_path_reservation_tracker.cc:115: // - Uniquifies |suggested_path| if |should_uniquify_path| is true. Should mention truncation in this comment as well. https://codereview.chromium.org/12212010/diff/5002/chrome/browser/download/do... chrome/browser/download/download_path_reservation_tracker.cc:171: const int kExtraChars = sizeof(" (100).crdownload") - 1; I'd rather find some way to export the amount of space we need to reserve for the intermediate name from the system that's creating the intermediate name (chrome_download_manager_delegate.h). But that's a nit that we can resolve in later reviews. https://codereview.chromium.org/12212010/diff/5002/chrome/browser/download/do... chrome/browser/download/download_path_reservation_tracker.cc:175: if (limit > 0) { I'd be inclined to reserve a few more characters here, though I'll admit I'm not sure how many; five? It should be pulled out into a separate constant, as I could imagine people might want to play with the value for maintainability.
+mark for the changes in base/. Could you take a look? Primary use of the new function is for this download code, but I'm planning to use it in other place; implementation of ChromeOS file dialog. (I rather blindly calling up you from the recent reviewers for the files; if you know more appropriate person, please forward. Thanks!) > Having said that, he's very much the right downloads person to review this CL, so hopefully we can avoid committing until he had a chance to look at it. Makes sense. For now, addressed the easier part of the comments. https://codereview.chromium.org/12212010/diff/5002/chrome/browser/download/do... File chrome/browser/download/download_path_reservation_tracker.cc (right): https://codereview.chromium.org/12212010/diff/5002/chrome/browser/download/do... chrome/browser/download/download_path_reservation_tracker.cc:115: // - Uniquifies |suggested_path| if |should_uniquify_path| is true. On 2013/02/07 19:12:03, rdsmith wrote: > Should mention truncation in this comment as well. Done. https://codereview.chromium.org/12212010/diff/5002/chrome/browser/download/do... chrome/browser/download/download_path_reservation_tracker.cc:175: if (limit > 0) { On 2013/02/07 19:12:03, rdsmith wrote: > I'd be inclined to reserve a few more characters here, though I'll admit I'm not > sure how many; five? It should be pulled out into a separate constant, as I > could imagine people might want to play with the value for maintainability. Pulled out the constant (tentatively 5). IMHO we can be more aggressive like, say, 100, since all modern filesystem has max_length>=255 (ecryptfs's 143 is notably low) but not quite sure either...
https://codereview.chromium.org/12212010/diff/5016/base/sys_info_posix.cc File base/sys_info_posix.cc (right): https://codereview.chromium.org/12212010/diff/5016/base/sys_info_posix.cc#new... base/sys_info_posix.cc:56: if (!path.IsAbsolute()) I don’t think this is relevant to the API. Maximum path component length of a relative path can still be meaningful. https://codereview.chromium.org/12212010/diff/5016/base/sys_info_posix.cc#new... base/sys_info_posix.cc:60: if (statvfs(path.value().c_str(), &stats) != 0) { On Mac OS X, you need to use pathconf(…, _PC_NAME_MAX) instead of statvfs if you’re interested in f_namemax. statvfs returns a constant value for NAME_MAX without considering the actual filesystem. On Linux, pathconf(…, _PC_NAME_MAX) calls statfs under the hood, so pathconf should be safe to use everywhere. https://codereview.chromium.org/12212010/diff/5016/base/sys_info_posix.cc#new... base/sys_info_posix.cc:64: // Android uses statfs instead of statvfs. The field name differs. If you’re sticking with statfs for any code… Hide this detail by way of a #define up above where statvfs is defined as statfs for Android? #define f_namemax f_namelen
https://codereview.chromium.org/12212010/diff/5016/base/sys_info_win.cc File base/sys_info_win.cc (right): https://codereview.chromium.org/12212010/diff/5016/base/sys_info_win.cc#newco... base/sys_info_win.cc:71: if (!path.IsAbsolute()) Same.
Thanks for the comments! https://codereview.chromium.org/12212010/diff/5016/base/sys_info_posix.cc File base/sys_info_posix.cc (right): https://codereview.chromium.org/12212010/diff/5016/base/sys_info_posix.cc#new... base/sys_info_posix.cc:56: if (!path.IsAbsolute()) On 2013/02/08 18:14:02, Mark Mentovai wrote: > I don’t think this is relevant to the API. Maximum path component length of a > relative path can still be meaningful. Done. https://codereview.chromium.org/12212010/diff/5016/base/sys_info_posix.cc#new... base/sys_info_posix.cc:60: if (statvfs(path.value().c_str(), &stats) != 0) { On 2013/02/08 18:14:02, Mark Mentovai wrote: > On Mac OS X, you need to use pathconf(…, _PC_NAME_MAX) instead of statvfs if > you’re interested in f_namemax. statvfs returns a constant value for NAME_MAX > without considering the actual filesystem. > > On Linux, pathconf(…, _PC_NAME_MAX) calls statfs under the hood, so pathconf > should be safe to use everywhere. Thanks for catching it! That greatly simplifies the code. Done. https://codereview.chromium.org/12212010/diff/5016/base/sys_info_posix.cc#new... base/sys_info_posix.cc:64: // Android uses statfs instead of statvfs. The field name differs. On 2013/02/08 18:14:02, Mark Mentovai wrote: > If you’re sticking with statfs for any code… > > Hide this detail by way of a #define up above where statvfs is defined as statfs > for Android? > > #define f_namemax f_namelen Done (using pathconf). https://codereview.chromium.org/12212010/diff/5016/base/sys_info_win.cc File base/sys_info_win.cc (right): https://codereview.chromium.org/12212010/diff/5016/base/sys_info_win.cc#newco... base/sys_info_win.cc:71: if (!path.IsAbsolute()) On 2013/02/08 18:14:42, Mark Mentovai wrote: > Same. Done.
LGTM
https://codereview.chromium.org/12212010/diff/3005/base/sys_info.h File base/sys_info.h (right): https://codereview.chromium.org/12212010/diff/3005/base/sys_info.h#newcode40 base/sys_info.h:40: static int GetMaximumPathComponentLength(const FilePath& path); Why in sys_info.h? Have you considered putting this in file_util.h? https://codereview.chromium.org/12212010/diff/3005/chrome/browser/download/do... File chrome/browser/download/download_path_reservation_tracker.cc (right): https://codereview.chromium.org/12212010/diff/3005/chrome/browser/download/do... chrome/browser/download/download_path_reservation_tracker.cc:177: int max_length = base::SysInfo::GetMaximumPathComponentLength(dir); We should also limit the length of the full path to be shorter than MAX_PATH on Windows. Some of the Windows APIs have this limitation. https://codereview.chromium.org/12212010/diff/3005/chrome/browser/download/do... chrome/browser/download/download_path_reservation_tracker.cc:184: const int kExtraChars = sizeof(" (100).crdownload") - 1; Feel free to push back on this one: The intermediate path suffixes are implementation details. Ideally we shouldn't truncate the download filename to accomodate them. I.e. the user should be able to set a 255 character download filename if the file system supports it. This might be even more of an issue when the limit is 143. One approach we were discussing was to use random intermediate filenames if the suffixes would cause the intermediate filename to exceed file system limitations. This could conceivably be done by returning the path component limit back to ChromeDownloadManagerDelegate where the limit would be used to determine whether to use a .crdownload suffix or a random filename as the intermediate.
Revised the patch. Mark, could you take another look? I felt better to move the function from SysInfo to file_util per asanka@'s suggestion, and also added some check not to overrun MAX_PATH as a whole path string. https://codereview.chromium.org/12212010/diff/3005/base/sys_info.h File base/sys_info.h (right): https://codereview.chromium.org/12212010/diff/3005/base/sys_info.h#newcode40 base/sys_info.h:40: static int GetMaximumPathComponentLength(const FilePath& path); On 2013/02/12 20:20:12, asanka wrote: > Why in sys_info.h? Have you considered putting this in file_util.h? Fm, sounds better. Done. https://codereview.chromium.org/12212010/diff/3005/chrome/browser/download/do... File chrome/browser/download/download_path_reservation_tracker.cc (right): https://codereview.chromium.org/12212010/diff/3005/chrome/browser/download/do... chrome/browser/download/download_path_reservation_tracker.cc:177: int max_length = base::SysInfo::GetMaximumPathComponentLength(dir); On 2013/02/12 20:20:12, asanka wrote: > We should also limit the length of the full path to be shorter than MAX_PATH on > Windows. Some of the Windows APIs have this limitation. Done (inside the query function). https://codereview.chromium.org/12212010/diff/3005/chrome/browser/download/do... chrome/browser/download/download_path_reservation_tracker.cc:184: const int kExtraChars = sizeof(" (100).crdownload") - 1; On 2013/02/12 20:20:12, asanka wrote: > Feel free to push back on this one: > > The intermediate path suffixes are implementation details. Ideally we shouldn't > truncate the download filename to accomodate them. I.e. the user should be able > to set a 255 character download filename if the file system supports it. This > might be even more of an issue when the limit is 143. > > One approach we were discussing was to use random intermediate filenames if the > suffixes would cause the intermediate filename to exceed file system > limitations. This could conceivably be done by returning the path component > limit back to ChromeDownloadManagerDelegate where the limit would be used to > determine whether to use a .crdownload suffix or a random filename as the > intermediate. Sounds reasonable. Let me handle it in a separate CL to keep the current one simpler. In the latest patchset, I've at least removed the " (100)" reservation part so that by making uniquification respect the name length limit.
https://codereview.chromium.org/12212010/diff/24002/base/file_util_win.cc File base/file_util_win.cc (right): https://codereview.chromium.org/12212010/diff/24002/base/file_util_win.cc#new... base/file_util_win.cc:963: PathStripToRootW(&root[0]); Looking at http://msdn.microsoft.com/en-us/library/windows/desktop/bb773757(v=vs.85).aspx , it recommends PathCchStripToRoot because that function also has a size argument. Looking at http://msdn.microsoft.com/en-us/library/windows/desktop/hh707096(v=vs.85).aspx , I see that PathCchStripToRoot also works on UNC paths, and it says that PathStripToRoot does not. Do we care about UNC paths? https://codereview.chromium.org/12212010/diff/24002/base/file_util_win.cc#new... base/file_util_win.cc:964: PathAddBackslashW(&root[0]); Same with PathCchAddBackslash. See http://msdn.microsoft.com/en-us/library/windows/desktop/bb773561(v=vs.85).aspx and http://msdn.microsoft.com/en-us/library/windows/desktop/hh707078(v=vs.85).aspx .
https://codereview.chromium.org/12212010/diff/24002/base/file_util_win.cc File base/file_util_win.cc (right): https://codereview.chromium.org/12212010/diff/24002/base/file_util_win.cc#new... base/file_util_win.cc:963: PathStripToRootW(&root[0]); On 2013/02/14 13:40:58, Mark Mentovai wrote: > Looking at > http://msdn.microsoft.com/en-us/library/windows/desktop/bb773757%28v=vs.85%29... , > it recommends PathCchStripToRoot because that function also has a size argument. Unfortunately PathCchStripToRoot() appears to be only available on Windows 8 and Windows Server 2012 and later. Good point about the string size. The documentation for PathStripToRoot() states that the buffer is expected to be of MAX_PATH length in characters and be NUL terminated. I think we should make sure that the buffer length requirements of PathStripToRoot() be respected. I.e. make sure that the length of |path| doesn't exceed MAX_PATH and size of |root| is MAX_PATH even if |path| is shorter than MAX_PATH. Also normalize path separators before calling this function (FilePath::NormalizePathSeparators()). PathStripToRoot() doesn't seem to deal with forward slashes even though some other Windows APIs do. > Looking at > http://msdn.microsoft.com/en-us/library/windows/desktop/hh707096%28v=vs.85%29... , > I see that PathCchStripToRoot also works on UNC paths, and it says that > PathStripToRoot does not. > > Do we care about UNC paths? We do care about UNC paths. :-( Have you looked at using GetVolumePathName() instead of Path{CCh,}StripToRoot()?
Thanks for pointing out the essentials! Changed the code to use GetVolumePathName as suggested. Some notes: - PathStripToRoot: As far as I tried it correctly handled UNC paths like "\\?\C:\..." or "\\host\root\...", but it didn't recognize "\\?\UNC\host\root\...". - PathCChStripToRoot: Might be good but only available in >=Win8. - GetVolumePathName: Handles all the cases above in >=WinXP with taking buffer size argument and returning with trailing backslash. It is also "correct" for the cases like "D:\" is mounted on "C:\foo\bar" and asked about "C:\foo\bar\buz".
https://codereview.chromium.org/12212010/diff/6051/base/file_util_win.cc File base/file_util_win.cc (right): https://codereview.chromium.org/12212010/diff/6051/base/file_util_win.cc#newc... base/file_util_win.cc:958: MAX_PATH)) { arraysize(volume_path) https://codereview.chromium.org/12212010/diff/6051/base/file_util_win.cc#newc... base/file_util_win.cc:970: int whole_path_limit = std::max(0, MAX_PATH - 1 - static_cast<int>(prefix)); So one +1 is for the separator and one -1 is to keep it < MAX_PATH? It’s confusing to do this in two places and on two lines without a comment. https://codereview.chromium.org/12212010/diff/6051/base/file_util_win.cc#newc... base/file_util_win.cc:970: int whole_path_limit = std::max(0, MAX_PATH - 1 - static_cast<int>(prefix)); You might not be able to get rid of all of the static_casts, but maybe you can get away with calling prefix an “int” to get rid of the cast on this line.
Updated. https://codereview.chromium.org/12212010/diff/6051/base/file_util_win.cc File base/file_util_win.cc (right): https://codereview.chromium.org/12212010/diff/6051/base/file_util_win.cc#newc... base/file_util_win.cc:958: MAX_PATH)) { On 2013/02/15 13:49:42, Mark Mentovai wrote: > arraysize(volume_path) Done. https://codereview.chromium.org/12212010/diff/6051/base/file_util_win.cc#newc... base/file_util_win.cc:970: int whole_path_limit = std::max(0, MAX_PATH - 1 - static_cast<int>(prefix)); On 2013/02/15 13:49:42, Mark Mentovai wrote: > So one +1 is for the separator and one -1 is to keep it < MAX_PATH? It’s > confusing to do this in two places and on two lines without a comment. Added some comments for clarification. https://codereview.chromium.org/12212010/diff/6051/base/file_util_win.cc#newc... base/file_util_win.cc:970: int whole_path_limit = std::max(0, MAX_PATH - 1 - static_cast<int>(prefix)); On 2013/02/15 13:49:42, Mark Mentovai wrote: > You might not be able to get rid of all of the static_casts, but maybe you can > get away with calling prefix an “int” to get rid of the cast on this line. The expression assigned to |prefix| has type size_t so calling it will just move the static_cast to there. As far as I could come up, the current form is the simplest (but not quire sure).
LGTM in base.
Thank you for the review, Mark. asanka, could you please take one more round of look? On 2013/02/18 23:49:03, Mark Mentovai wrote: > LGTM in base.
Apologies for the delay. I have a couple of nits for the current patchset. Could you add some test coverage for the new code in download_path_reservation_tracker_unittest.cc? Catch me on IM if you need to work out how to test this. For this fix to be complete, you'll also need to address file_util::GetUniquePathNumber(). It's used in content/download/download_file_impl.cc among other places. It can cause a failure due to filename length restrictions if there's a stray (possibly left over) file that conflicts with the intermediate download file and the download filename is already dangerously close to the limit. None of the callers of GetUniquePathNumber() use the second (|suffix|) parameter, and all the callers use the returned path number to construct a suffix and call FilePath::InsertBeforeExtensionASCII(). So you could convert GetUniquePathNumber() to, say, bool GetUniquePath(base::FilePath* path) which alters |path|. This will simplify the call sites and also allow you to impose length restrictions on |path| during uniquification. This change can (and probably should) happen in a separate CL. https://codereview.chromium.org/12212010/diff/34014/chrome/browser/download/d... File chrome/browser/download/download_path_reservation_tracker.cc (right): https://codereview.chromium.org/12212010/diff/34014/chrome/browser/download/d... chrome/browser/download/download_path_reservation_tracker.cc:105: // Truncates path->BaseName() to satisfy the condition .value().size() <= limit. Nit: path->BaseName().value().size() <= limit. |limit| only applies to the BaseName(). https://codereview.chromium.org/12212010/diff/34014/chrome/browser/download/d... chrome/browser/download/download_path_reservation_tracker.cc:106: // - It keeps the extension as is. Only truncates the body part. Nit: "base filename" would, I think, be clearer than "body part." https://codereview.chromium.org/12212010/diff/34014/chrome/browser/download/d... chrome/browser/download/download_path_reservation_tracker.cc:133: 0, limit > 0 && CBU16_IS_TRAIL(name[limit]) ? limit - 1 : limit); Nit: limit > 0 is always true at this point. https://codereview.chromium.org/12212010/diff/34014/chrome/browser/download/d... chrome/browser/download/download_path_reservation_tracker.cc:213: std::string suffix(StringPrintf(" (%d)", uniquifier)); Nit: Use base::StringPrintf() and include "base/stringprintf.h" https://codereview.chromium.org/12212010/diff/34014/chrome/browser/download/d... chrome/browser/download/download_path_reservation_tracker.cc:217: break; Nit: We could get here if |max_length| is -1 and there is a path conflict. If that case was handled explicitly then we wouldn't be doing arithmetic on a signal value and the code would be easier to follow.
Added some test coverage. I'd like to tackle the file_util::GetUniquePathNumber() part separately (thanks for letting me know it; I didn't realize that.) https://codereview.chromium.org/12212010/diff/34014/chrome/browser/download/d... File chrome/browser/download/download_path_reservation_tracker.cc (right): https://codereview.chromium.org/12212010/diff/34014/chrome/browser/download/d... chrome/browser/download/download_path_reservation_tracker.cc:105: // Truncates path->BaseName() to satisfy the condition .value().size() <= limit. On 2013/02/19 17:59:15, asanka wrote: > Nit: path->BaseName().value().size() <= limit. > > |limit| only applies to the BaseName(). Done. https://codereview.chromium.org/12212010/diff/34014/chrome/browser/download/d... chrome/browser/download/download_path_reservation_tracker.cc:106: // - It keeps the extension as is. Only truncates the body part. On 2013/02/19 17:59:15, asanka wrote: > Nit: "base filename" would, I think, be clearer than "body part." Done. https://codereview.chromium.org/12212010/diff/34014/chrome/browser/download/d... chrome/browser/download/download_path_reservation_tracker.cc:133: 0, limit > 0 && CBU16_IS_TRAIL(name[limit]) ? limit - 1 : limit); On 2013/02/19 17:59:15, asanka wrote: > Nit: limit > 0 is always true at this point. Done. https://codereview.chromium.org/12212010/diff/34014/chrome/browser/download/d... chrome/browser/download/download_path_reservation_tracker.cc:213: std::string suffix(StringPrintf(" (%d)", uniquifier)); On 2013/02/19 17:59:15, asanka wrote: > Nit: Use base::StringPrintf() and include "base/stringprintf.h" Done. https://codereview.chromium.org/12212010/diff/34014/chrome/browser/download/d... chrome/browser/download/download_path_reservation_tracker.cc:217: break; On 2013/02/19 17:59:15, asanka wrote: > Nit: We could get here if |max_length| is -1 and there is a path conflict. If > that case was handled explicitly then we wouldn't be doing arithmetic on a > signal value and the code would be easier to follow. Good catch. Thanks. Explicitly dispatched the |max_length| = -1 case.
Sounds good. Thank you! chrome/browser/download LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kinaba@chromium.org/12212010/42001
Message was sent while issue was closed.
Change committed as 183727
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/12212010/diff/42001/chrome/browser/dow... File chrome/browser/download/download_path_reservation_tracker_unittest.cc (right): https://chromiumcodereview.appspot.com/12212010/diff/42001/chrome/browser/dow... chrome/browser/download/download_path_reservation_tracker_unittest.cc:456: #if defined(OS_WIN) || defined(OS_MAC) || defined(OS_CHROMEOS) It's OS_MACOSX ( https://codereview.chromium.org/12829005/ )
Message was sent while issue was closed.
On 2013/03/13 23:25:00, Nico wrote: > https://chromiumcodereview.appspot.com/12212010/diff/42001/chrome/browser/dow... > File chrome/browser/download/download_path_reservation_tracker_unittest.cc > (right): > > https://chromiumcodereview.appspot.com/12212010/diff/42001/chrome/browser/dow... > chrome/browser/download/download_path_reservation_tracker_unittest.cc:456: #if > defined(OS_WIN) || defined(OS_MAC) || defined(OS_CHROMEOS) > It's OS_MACOSX ( https://codereview.chromium.org/12829005/ ) oops! thanks for pointing it out.
Message was sent while issue was closed.
https://codereview.chromium.org/12212010/diff/42001/chrome/browser/download/d... File chrome/browser/download/download_path_reservation_tracker.cc (right): https://codereview.chromium.org/12212010/diff/42001/chrome/browser/download/d... chrome/browser/download/download_path_reservation_tracker.cc:34: static const size_t kTruncatedNameLengthLowerbound = 5; nit: you are already in unnamed namespace, static is redundant here. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
