Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2804793002:
Print Preview: Fix failure to save with long page title (Closed)
DescriptionPrint Preview: Fix failure to save with long page title
BUG=707538
Review-Url: https://codereview.chromium.org/2804793002
Cr-Commit-Position: refs/heads/master@{#466053}
Committed: https://chromium.googlesource.com/chromium/src/+/e5be563564bd38f1f8355406a658ff93bc71027a
Patch Set 1 #
Total comments: 1
Patch Set 2 : Move length check to select file dialog #
Total comments: 8
Patch Set 3 : Remove path check #
Total comments: 4
Patch Set 4 : Fix extension issues #
Total comments: 6
Patch Set 5 : Move modifications to function #
Total comments: 11
Patch Set 6 : Address comments #
Total comments: 4
Patch Set 7 : Use beginning of extension #
Total comments: 4
Patch Set 8 : Use resize, simplify #
Total comments: 4
Patch Set 9 : Add test #
Total comments: 12
Patch Set 10 : Fix unit test #Patch Set 11 : Add integration test #
Total comments: 31
Patch Set 12 : Address comments and fix test dcheck #
Total comments: 16
Patch Set 13 : Address comments #
Total comments: 2
Patch Set 14 : Remove unused line #
Messages
Total messages: 90 (56 generated)
Description was changed from ========== Fix long title BUG=707538 ========== to ========== Print Preview: Fix failure to save with long page title BUG=707538 ==========
rbpotter@chromium.org changed reviewers: + thestig@chromium.org
This is probably ok, but I wonder if we should be doing this in SelectFileDialog::SelectFile() or at least DCHECKing there to make sure the paths are short enough. Other SelectFileDialog users can potentially hit the same issue. +ananta who touched a lot of the code in ui/shell_dialogs/select_file_dialog.*. https://codereview.chromium.org/2804793002/diff/1/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2804793002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1033: // Take at most 251 characters - need 4 for the extension and max length We may want to use PATH_MAX instead. On Windows there's MAX_PATH, but I don't remember if PATH_MAX is portable across all platforms.
ananta@chromium.org changed reviewers: + ananta@chromium.org
It would be great if we could confirm that the problem exists only on Windows? +1 to move the long path check to SelectFileDialog. If the bug is Windows only, we could potentially look at GetShortPathName as a workaround.
On 2017/04/05 21:23:15, ananta wrote: > It would be great if we could confirm that the problem exists only on Windows? > > +1 to move the long path check to SelectFileDialog. > > If the bug is Windows only, we could potentially look at GetShortPathName as a > workaround. Not only Windows. Comment 2 on the bug says this is an issue on Mac and Linux also. I did some investigation and it looks like on all 3 platforms the file name length must be 255 characters or less. Maximum path length is a related issue. Should probably check both but it is less clear how to handle a path that is too long; truncating works for file names but might have odd results with paths.
On 2017/04/05 21:32:06, rbpotter wrote: > On 2017/04/05 21:23:15, ananta wrote: > > It would be great if we could confirm that the problem exists only on Windows? > > > > +1 to move the long path check to SelectFileDialog. > > > > If the bug is Windows only, we could potentially look at GetShortPathName as a > > workaround. > > Not only Windows. Comment 2 on the bug says this is an issue on Mac and > Linux also. I did some investigation and it looks like on all 3 platforms > the file name length must be 255 characters or less. > > Maximum path length is a related issue. Should probably check both but it > is less clear how to handle a path that is too long; truncating works for > file names but might have odd results with paths. Moved things into the select file dialog and added a check for max path. Worth noting that from testing it seems like paths that are too long will just trigger a "Path too long, try another name" message when the user actually tries to save, but will still allow the dialog to appear. Names that are > 255 cause the problem reported where the dialog never shows up at all and things fail silently. So it may not matter if the path is too long and this cannot be fixed by truncating the filename.
https://codereview.chromium.org/2804793002/diff/20001/ui/shell_dialogs/select... File ui/shell_dialogs/select_file_dialog.cc (right): https://codereview.chromium.org/2804793002/diff/20001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:89: // Shorten filename if necessary. Do this right before the SelectFileImpl() call. Otherwise all this calculation may get thrown away if the code flow hits line 118. https://codereview.chromium.org/2804793002/diff/20001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:92: int max_name_length = 255; Can this be size_t to avoid all the casts below. https://codereview.chromium.org/2804793002/diff/20001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:94: default_path.value().length() > MAX_PATH - 1) { Does MAX_PATH work on POSIX? https://codereview.chromium.org/2804793002/diff/20001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:105: path = path.Append(filename); Isn't this the same as: path = default_path; ?
https://codereview.chromium.org/2804793002/diff/20001/ui/shell_dialogs/select... File ui/shell_dialogs/select_file_dialog.cc (right): https://codereview.chromium.org/2804793002/diff/20001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:89: // Shorten filename if necessary. On 2017/04/06 00:47:05, Lei Zhang wrote: > Do this right before the SelectFileImpl() call. Otherwise all this calculation > may get thrown away if the code flow hits line 118. Done. https://codereview.chromium.org/2804793002/diff/20001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:92: int max_name_length = 255; On 2017/04/06 00:47:05, Lei Zhang wrote: > Can this be size_t to avoid all the casts below. Done. https://codereview.chromium.org/2804793002/diff/20001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:94: default_path.value().length() > MAX_PATH - 1) { On 2017/04/06 00:47:05, Lei Zhang wrote: > Does MAX_PATH work on POSIX? Removed path length check so not an issue any more. The patch does work on Linux, but I can't reproduce the bug on Stable there either... https://codereview.chromium.org/2804793002/diff/20001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:105: path = path.Append(filename); On 2017/04/06 00:47:05, Lei Zhang wrote: > Isn't this the same as: path = default_path; ? Done.
https://codereview.chromium.org/2804793002/diff/40001/ui/shell_dialogs/select... File ui/shell_dialogs/select_file_dialog.cc (right): https://codereview.chromium.org/2804793002/diff/40001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:105: size_t max_name_length = 255; const size_t kMaxNameLength ? https://codereview.chromium.org/2804793002/diff/40001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:107: base::FilePath::StringType extension = filename.Extension(); What if the extension is 256 letters long?
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
https://codereview.chromium.org/2804793002/diff/40001/ui/shell_dialogs/select... File ui/shell_dialogs/select_file_dialog.cc (right): https://codereview.chromium.org/2804793002/diff/40001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:105: size_t max_name_length = 255; On 2017/04/12 19:10:25, Lei Zhang wrote: > const size_t kMaxNameLength ? Done. https://codereview.chromium.org/2804793002/diff/40001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:107: base::FilePath::StringType extension = filename.Extension(); On 2017/04/12 19:10:25, Lei Zhang wrote: > What if the extension is 256 letters long? Have addressed this in new patch.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2804793002/diff/60001/ui/shell_dialogs/select... File ui/shell_dialogs/select_file_dialog.cc (right): https://codereview.chromium.org/2804793002/diff/60001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:102: // Shorten filename if necessary. This is getting long-ish. Can we split it off into its own function? https://codereview.chromium.org/2804793002/diff/60001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:111: if (kMaxNameLength > filename.value().length()) { This reads more naturally if you flip the equation around. It's easier to think of "if the file name length is less than the maximum" than "if the maximum is greater than the file name length". https://codereview.chromium.org/2804793002/diff/60001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:119: extension = This is a bit weird. If the code does not go through this path, then |extension| has a leading dot. If we go through here, then it does not. Looking at FilePath::AddExtension(), it looks like AddExtension() can tolerate both cases, but it may be good to be consistent.
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2804793002/diff/60001/ui/shell_dialogs/select... File ui/shell_dialogs/select_file_dialog.cc (right): https://codereview.chromium.org/2804793002/diff/60001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:102: // Shorten filename if necessary. On 2017/04/13 01:34:51, Lei Zhang wrote: > This is getting long-ish. Can we split it off into its own function? Done. https://codereview.chromium.org/2804793002/diff/60001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:111: if (kMaxNameLength > filename.value().length()) { On 2017/04/13 01:34:51, Lei Zhang wrote: > This reads more naturally if you flip the equation around. It's easier to think > of "if the file name length is less than the maximum" than "if the maximum is > greater than the file name length". Done. https://codereview.chromium.org/2804793002/diff/60001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:119: extension = On 2017/04/13 01:34:50, Lei Zhang wrote: > This is a bit weird. If the code does not go through this path, then |extension| > has a leading dot. If we go through here, then it does not. Looking at > FilePath::AddExtension(), it looks like AddExtension() can tolerate both cases, > but it may be good to be consistent. Done.
https://codereview.chromium.org/2804793002/diff/80001/ui/shell_dialogs/select... File ui/shell_dialogs/select_file_dialog.cc (right): https://codereview.chromium.org/2804793002/diff/80001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:78: namespace { There's already one up on line 20. https://codereview.chromium.org/2804793002/diff/80001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:80: base::FilePath GetShortenedFilePath(const base::FilePath& path) { Can you add a comment to explain why we need this? https://codereview.chromium.org/2804793002/diff/80001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:84: if (filename.value().length() > kMaxNameLength) { Flip to <=, and then return early. https://codereview.chromium.org/2804793002/diff/80001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:89: if (filename.value().length() < kMaxNameLength) { I think you can omit this check. If the |filename| length is say, 300, then we'll end up with std::max(13, 255 - 300), which is still 13. https://codereview.chromium.org/2804793002/diff/80001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:97: new_extension[0] = base::FilePath::kExtensionSeparator; Isn't this an out of bounds access since |new_extension| starts out with length 0? Just do: base::FilePath::StringType new_extension = base::FilePath::kExtensionSeparator;
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
https://codereview.chromium.org/2804793002/diff/80001/ui/shell_dialogs/select... File ui/shell_dialogs/select_file_dialog.cc (right): https://codereview.chromium.org/2804793002/diff/80001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:78: namespace { On 2017/04/13 19:33:53, Lei Zhang wrote: > There's already one up on line 20. Done. https://codereview.chromium.org/2804793002/diff/80001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:80: base::FilePath GetShortenedFilePath(const base::FilePath& path) { On 2017/04/13 19:33:53, Lei Zhang wrote: > Can you add a comment to explain why we need this? Done. https://codereview.chromium.org/2804793002/diff/80001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:84: if (filename.value().length() > kMaxNameLength) { On 2017/04/13 19:33:53, Lei Zhang wrote: > Flip to <=, and then return early. Done. https://codereview.chromium.org/2804793002/diff/80001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:89: if (filename.value().length() < kMaxNameLength) { On 2017/04/13 19:33:53, Lei Zhang wrote: > I think you can omit this check. If the |filename| length is say, 300, then > we'll end up with std::max(13, 255 - 300), which is still 13. Both kMaxNameLength and filename.value().length() are size_ts as is max_extension_length, so I did this to avoid casting in the std::max call. What would be preferable between casting vs checking the values? https://codereview.chromium.org/2804793002/diff/80001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:97: new_extension[0] = base::FilePath::kExtensionSeparator; On 2017/04/13 19:33:53, Lei Zhang wrote: > Isn't this an out of bounds access since |new_extension| starts out with length > 0? Just do: > > base::FilePath::StringType new_extension = base::FilePath::kExtensionSeparator; That doesn't work since the extension separator is a CharType, but I changed this to use push_back instead, which should prevent the out of bounds access.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2804793002/diff/80001/ui/shell_dialogs/select... File ui/shell_dialogs/select_file_dialog.cc (right): https://codereview.chromium.org/2804793002/diff/80001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.cc:89: if (filename.value().length() < kMaxNameLength) { On 2017/04/13 20:53:56, rbpotter wrote: > On 2017/04/13 19:33:53, Lei Zhang wrote: > > I think you can omit this check. If the |filename| length is say, 300, then > > we'll end up with std::max(13, 255 - 300), which is still 13. > > Both kMaxNameLength and filename.value().length() are size_ts as is > max_extension_length, so I did this to avoid casting in the std::max > call. What would be preferable between casting vs checking the values? Try writing 13U instead? https://codereview.chromium.org/2804793002/diff/100001/ui/shell_dialogs/selec... File ui/shell_dialogs/select_file_dialog.cc (right): https://codereview.chromium.org/2804793002/diff/100001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog.cc:45: base::FilePath::StringType new_extension; Just an idea: would this be clearer if we instead first trimmed |extension| down via substr(). Then insert(0, 1, kExtensionSeparator) ? Thereby eliminating the need for |new_extension|. https://codereview.chromium.org/2804793002/diff/100001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog.cc:48: extension.substr(extension.length() - max_extension_length + 1, We should truncate the extension instead of taking the end portion.
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
https://codereview.chromium.org/2804793002/diff/100001/ui/shell_dialogs/selec... File ui/shell_dialogs/select_file_dialog.cc (right): https://codereview.chromium.org/2804793002/diff/100001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog.cc:45: base::FilePath::StringType new_extension; On 2017/04/13 21:13:44, Lei Zhang wrote: > Just an idea: would this be clearer if we instead first trimmed |extension| down > via substr(). Then insert(0, 1, kExtensionSeparator) ? Thereby eliminating the > need for |new_extension|. Don't need this if we truncate since FinalExtension() has the leading . https://codereview.chromium.org/2804793002/diff/100001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog.cc:48: extension.substr(extension.length() - max_extension_length + 1, On 2017/04/13 21:13:44, Lei Zhang wrote: > We should truncate the extension instead of taking the end portion. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2804793002/diff/120001/ui/shell_dialogs/selec... File ui/shell_dialogs/select_file_dialog.cc (right): https://codereview.chromium.org/2804793002/diff/120001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog.cc:42: if (extension.length() >= max_extension_length) { Just > now? https://codereview.chromium.org/2804793002/diff/120001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog.cc:45: extension = extension.substr(0, max_extension_length); Can we trim by using resize()? Ditto on line 48.
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
https://codereview.chromium.org/2804793002/diff/120001/ui/shell_dialogs/selec... File ui/shell_dialogs/select_file_dialog.cc (right): https://codereview.chromium.org/2804793002/diff/120001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog.cc:42: if (extension.length() >= max_extension_length) { On 2017/04/13 21:53:09, Lei Zhang wrote: > Just > now? Done. https://codereview.chromium.org/2804793002/diff/120001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog.cc:45: extension = extension.substr(0, max_extension_length); On 2017/04/13 21:53:09, Lei Zhang wrote: > Can we trim by using resize()? Ditto on line 48. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rbpotter@chromium.org changed reviewers: + thakis@chromium.org
On 2017/04/13 23:02:11, rbpotter wrote: > https://codereview.chromium.org/2804793002/diff/120001/ui/shell_dialogs/selec... > File ui/shell_dialogs/select_file_dialog.cc (right): > > https://codereview.chromium.org/2804793002/diff/120001/ui/shell_dialogs/selec... > ui/shell_dialogs/select_file_dialog.cc:42: if (extension.length() >= > max_extension_length) { > On 2017/04/13 21:53:09, Lei Zhang wrote: > > Just > now? > > Done. > > https://codereview.chromium.org/2804793002/diff/120001/ui/shell_dialogs/selec... > ui/shell_dialogs/select_file_dialog.cc:45: extension = extension.substr(0, > max_extension_length); > On 2017/04/13 21:53:09, Lei Zhang wrote: > > Can we trim by using resize()? Ditto on line 48. > > Done. +thakis@ from ui/OWNERS since we moved all changes into ui/shell_dialogs/select_file_dialog.cc
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Do we have some integration test for this? https://codereview.chromium.org/2804793002/diff/140001/ui/shell_dialogs/selec... File ui/shell_dialogs/select_file_dialog.cc (right): https://codereview.chromium.org/2804793002/diff/140001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog.cc:26: // is the limit on Windows and Linux, and on Windows the system file FWIW, on Windows, the limit for the full path (not just basename) is 260 characters unless you use advanced tactics (https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).as...). So this might not be enough on Windows? https://codereview.chromium.org/2804793002/diff/140001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog.cc:29: base::FilePath filename = path.BaseName(); Move this below the early return, you currently always make a copy even you won't need it 99.7% of the time.
Patchset #9 (id:160001) has been deleted
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #9 (id:180001) has been deleted
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #9 (id:200001) has been deleted
Added a unit test. https://codereview.chromium.org/2804793002/diff/140001/ui/shell_dialogs/selec... File ui/shell_dialogs/select_file_dialog.cc (right): https://codereview.chromium.org/2804793002/diff/140001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog.cc:26: // is the limit on Windows and Linux, and on Windows the system file On 2017/04/14 14:03:34, Nico wrote: > FWIW, on Windows, the limit for the full path (not just basename) is 260 > characters unless you use advanced tactics > (https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).as...). > So this might not be enough on Windows? It fails to save in the system dialog if the path length is > 260, but the dialog still opens - the user gets a "File path is too long" message from the Windows dialog and can fix the problem. If the file name itself is > 255 characters, the dialog does not open at all and we get a silent failure from print preview. The intent was to fix the silent failure and allow the error from the dialog to handle the long path case. We could change this to check the path as well, though. What do you think? Should we let the system dialog handle that issue or handle it here? https://codereview.chromium.org/2804793002/diff/140001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog.cc:29: base::FilePath filename = path.BaseName(); On 2017/04/14 14:03:34, Nico wrote: > Move this below the early return, you currently always make a copy even you > won't need it 99.7% of the time. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2804793002/diff/220001/ui/shell_dialogs/selec... File ui/shell_dialogs/select_file_dialog.cc (right): https://codereview.chromium.org/2804793002/diff/220001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog.cc:78: // Returns a file path with a base name at most 255 characters long. This Comment goes in the header now. https://codereview.chromium.org/2804793002/diff/220001/ui/shell_dialogs/selec... File ui/shell_dialogs/select_file_dialog_unittest.cc (right): https://codereview.chromium.org/2804793002/diff/220001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog_unittest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. No (c) https://codereview.chromium.org/2804793002/diff/220001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog_unittest.cc:12: #if defined(OS_WIN) Can you get rid of the ifdefs with the help of FILE_PATH_LITERAL? https://codereview.chromium.org/2804793002/diff/220001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog_unittest.cc:13: const base::FilePath::StringType folder1 = L"folder1111\\"; These should be const base::FilePath::CharType varname[], otherwise this adds static initializers. There are checks for those for the chrome binary, so if this was outside of a unit test, and in the chrome binary, it would cause the checks to fail. https://codereview.chromium.org/2804793002/diff/220001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog_unittest.cc:26: long_name.substr(0, 210).insert(0, L"."); So just write this out. https://codereview.chromium.org/2804793002/diff/220001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog_unittest.cc:49: } test_cases[] = {{folder1, folder1}, {folder1, folder1}, {folder1, folder1}, Don't compute the inputs / expected outputs in tests. Just write them out explicitly. It's much easier to understand what is being tested that way.
Patchset #10 (id:240001) has been deleted
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2804793002/diff/220001/ui/shell_dialogs/selec... File ui/shell_dialogs/select_file_dialog.cc (right): https://codereview.chromium.org/2804793002/diff/220001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog.cc:78: // Returns a file path with a base name at most 255 characters long. This On 2017/04/15 22:19:34, Lei Zhang wrote: > Comment goes in the header now. Done. https://codereview.chromium.org/2804793002/diff/220001/ui/shell_dialogs/selec... File ui/shell_dialogs/select_file_dialog_unittest.cc (right): https://codereview.chromium.org/2804793002/diff/220001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog_unittest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/04/15 22:19:34, Lei Zhang wrote: > No (c) Done. https://codereview.chromium.org/2804793002/diff/220001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog_unittest.cc:12: #if defined(OS_WIN) On 2017/04/15 22:19:34, Lei Zhang wrote: > Can you get rid of the ifdefs with the help of FILE_PATH_LITERAL? Done. https://codereview.chromium.org/2804793002/diff/220001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog_unittest.cc:13: const base::FilePath::StringType folder1 = L"folder1111\\"; On 2017/04/15 22:19:34, Lei Zhang wrote: > These should be const base::FilePath::CharType varname[], otherwise this adds > static initializers. There are checks for those for the chrome binary, so if > this was outside of a unit test, and in the chrome binary, it would cause the > checks to fail. I put these in the test directly using FILE_PATH_LITERAL, as occurs in file_path_unittest.cc. Let me know if I should define them in a different way. https://codereview.chromium.org/2804793002/diff/220001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog_unittest.cc:26: long_name.substr(0, 210).insert(0, L"."); On 2017/04/15 22:19:34, Lei Zhang wrote: > So just write this out. Done. https://codereview.chromium.org/2804793002/diff/220001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog_unittest.cc:49: } test_cases[] = {{folder1, folder1}, {folder1, folder1}, {folder1, folder1}, On 2017/04/15 22:19:34, Lei Zhang wrote: > Don't compute the inputs / expected outputs in tests. Just write them out > explicitly. It's much easier to understand what is being tested that way. Done.
As far as I can tell, this tests the new function (great!) but not the bug this is fixing, right?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/17 18:58:26, Nico wrote: > As far as I can tell, this tests the new function (great!) but not the bug this > is fixing, right? Added a new integration test. It is Windows only, but I was only able to reproduce the original bug on Windows.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.win%2... [ RUN ] PrintPreviewHandlerTest.TestSaveAsPdfLongFileName [4568:520:0419/123028.049:6939018:FATAL:run_loop.cc(49)] Check failed: thread_checker_.CalledOnValidThread(). :-/
https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.h (right): https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.h:117: // The underlying dialog object. Protecteed to expose to unit tests. typo https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.h:265: virtual void PostPrintToPdfTask(); Still need to be virtual? https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc (right): https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:7: #include <Windows.h> lowercase https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:32: bool init_called = false; Can we avoid these global variables by doing the following? - Make them FakePrintPreviewHandler members. - Bind() a pointer to the FakePrintPreviewHandler as the first argument to GetSaveFileNameImpl() - Then stick into the OPENFILENAME struct's lCustData field. - For WM_INITDIALOG, |wParam| is the same struct. https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:40: UINT uiMsg, message, wparam, lparam https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:76: run_loop->Quit(); As is, would this end up calling Quit() twice? https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:88: base::RunLoop new_run_loop; If the RunLoop becomes a member variable, can we get rid of this? https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:96: protected: private? https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:121: PrintPreviewHandlerTest() {} Use braces or default consistently between the ctor/dtor. https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:127: // Create a new tab Not sure if you need to do this. I think the browser starts with 1 tab already. https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:155: // Initialize |preview_handler_| Probably not needed. https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:156: preview_handler_.reset( Can we use foo = base::MakeUnique<Foo>() instead of foo.reset(new Foo()) ? https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:161: PrintPreviewUI* preview_ui_; Initialize this variable, or maybe just drop it and have CreateUiAndHandler() return it. It's not used for anything else. https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:171: ASSERT_FALSE(!init_called && preview_handler_->save_failed()); So this is the same as the following, right? ASSERT_TRUE(init_called || !preview_handler_->save_failed()); Should it actually check the following? EXPECT_TRUE(init_called); EXPECT_FALSE(preview_handler_->save_failed()); https://codereview.chromium.org/2804793002/diff/280001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2804793002/diff/280001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:4417: sources += [ "../browser/ui/webui/print_preview/print_preview_handler_unittest.cc" ] Just name it print_preview_handler_win_unittest.cc, then you can put it in the list above, just like ui/shell_dialogs/select_file_dialog_win_unittest.cc.
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.h (right): https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.h:117: // The underlying dialog object. Protecteed to expose to unit tests. On 2017/04/19 21:14:07, Lei Zhang wrote: > typo Done. https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.h:265: virtual void PostPrintToPdfTask(); On 2017/04/19 21:14:07, Lei Zhang wrote: > Still need to be virtual? Done. https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc (right): https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:7: #include <Windows.h> On 2017/04/19 21:14:07, Lei Zhang wrote: > lowercase Done. https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:32: bool init_called = false; On 2017/04/19 21:14:07, Lei Zhang wrote: > Can we avoid these global variables by doing the following? > > - Make them FakePrintPreviewHandler members. > - Bind() a pointer to the FakePrintPreviewHandler as the first argument to > GetSaveFileNameImpl() > - Then stick into the OPENFILENAME struct's lCustData field. > - For WM_INITDIALOG, |wParam| is the same struct. Done. https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:40: UINT uiMsg, On 2017/04/19 21:14:07, Lei Zhang wrote: > message, wparam, lparam Done. https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:76: run_loop->Quit(); On 2017/04/19 21:14:07, Lei Zhang wrote: > As is, would this end up calling Quit() twice? Only one of FileSelected or FileSelectionCanceled should ever be called, and these are now the only places that call Quit(). https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:88: base::RunLoop new_run_loop; On 2017/04/19 21:14:08, Lei Zhang wrote: > If the RunLoop becomes a member variable, can we get rid of this? Done. https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:96: protected: On 2017/04/19 21:14:08, Lei Zhang wrote: > private? Done. https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:121: PrintPreviewHandlerTest() {} On 2017/04/19 21:14:08, Lei Zhang wrote: > Use braces or default consistently between the ctor/dtor. Done. https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:127: // Create a new tab On 2017/04/19 21:14:08, Lei Zhang wrote: > Not sure if you need to do this. I think the browser starts with 1 tab already. Does not start with a tab - tried removing this and could not get the initiator web contents. https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:155: // Initialize |preview_handler_| On 2017/04/19 21:14:07, Lei Zhang wrote: > Probably not needed. Done. https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:156: preview_handler_.reset( On 2017/04/19 21:14:08, Lei Zhang wrote: > Can we use foo = base::MakeUnique<Foo>() instead of foo.reset(new Foo()) ? Done. https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:161: PrintPreviewUI* preview_ui_; On 2017/04/19 21:14:07, Lei Zhang wrote: > Initialize this variable, or maybe just drop it and have CreateUiAndHandler() > return it. It's not used for anything else. Done. https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:171: ASSERT_FALSE(!init_called && preview_handler_->save_failed()); On 2017/04/19 21:14:07, Lei Zhang wrote: > So this is the same as the following, right? > > ASSERT_TRUE(init_called || !preview_handler_->save_failed()); > > Should it actually check the following? > > EXPECT_TRUE(init_called); > EXPECT_FALSE(preview_handler_->save_failed()); Done. https://codereview.chromium.org/2804793002/diff/280001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2804793002/diff/280001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:4417: sources += [ "../browser/ui/webui/print_preview/print_preview_handler_unittest.cc" ] On 2017/04/19 21:14:08, Lei Zhang wrote: > Just name it print_preview_handler_win_unittest.cc, then you can put it in the > list above, just like ui/shell_dialogs/select_file_dialog_win_unittest.cc. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc (right): https://codereview.chromium.org/2804793002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc:127: // Create a new tab On 2017/04/19 23:30:13, rbpotter wrote: > On 2017/04/19 21:14:08, Lei Zhang wrote: > > Not sure if you need to do this. I think the browser starts with 1 tab > already. > > Does not start with a tab - tried removing this and could not get the > initiator web contents. OK, I remembered wrong then. https://codereview.chromium.org/2804793002/diff/300001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc (right): https://codereview.chromium.org/2804793002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc:10: #include "base/auto_reset.h" No longer needed. Also don't need timer.h, plugin_service.h, and site_instance.h. https://codereview.chromium.org/2804793002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc:47: save_failed_ = false; Maybe the body should be just NOTREACHED() ? https://codereview.chromium.org/2804793002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc:58: if (!init_called_) This should always eval to true, right? https://codereview.chromium.org/2804793002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc:62: bool save_failed() { return save_failed_; } const methods https://codereview.chromium.org/2804793002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc:83: NULL); nullptr https://codereview.chromium.org/2804793002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc:122: PrintPreviewHandlerTest() : preview_handler_(nullptr), preview_ui_(nullptr) {} Don't have to initialize |preview_handler_| since it's not a raw pointer. https://codereview.chromium.org/2804793002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc:133: void CreateUiAndHandler() { UI since we have PrintPreviewUI and not PrintPreviewUi? https://codereview.chromium.org/2804793002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc:167: ASSERT_TRUE(preview_handler_->init_called()); You can use EXPECT_TRUE, since it doesn't hurt if execution continues and does the next check.
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2804793002/diff/300001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc (right): https://codereview.chromium.org/2804793002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc:10: #include "base/auto_reset.h" On 2017/04/20 00:12:30, Lei Zhang wrote: > No longer needed. Also don't need timer.h, plugin_service.h, and > site_instance.h. Done. https://codereview.chromium.org/2804793002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc:47: save_failed_ = false; On 2017/04/20 00:12:30, Lei Zhang wrote: > Maybe the body should be just NOTREACHED() ? Done. https://codereview.chromium.org/2804793002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc:58: if (!init_called_) On 2017/04/20 00:12:30, Lei Zhang wrote: > This should always eval to true, right? It should. Was checking this just in case things were off with timing. Probably don't need to though. https://codereview.chromium.org/2804793002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc:62: bool save_failed() { return save_failed_; } On 2017/04/20 00:12:30, Lei Zhang wrote: > const methods Done. https://codereview.chromium.org/2804793002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc:83: NULL); On 2017/04/20 00:12:30, Lei Zhang wrote: > nullptr Done. https://codereview.chromium.org/2804793002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc:122: PrintPreviewHandlerTest() : preview_handler_(nullptr), preview_ui_(nullptr) {} On 2017/04/20 00:12:30, Lei Zhang wrote: > Don't have to initialize |preview_handler_| since it's not a raw pointer. Done. https://codereview.chromium.org/2804793002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc:133: void CreateUiAndHandler() { On 2017/04/20 00:12:30, Lei Zhang wrote: > UI since we have PrintPreviewUI and not PrintPreviewUi? Done. https://codereview.chromium.org/2804793002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc:167: ASSERT_TRUE(preview_handler_->init_called()); On 2017/04/20 00:12:30, Lei Zhang wrote: > You can use EXPECT_TRUE, since it doesn't hurt if execution continues and does > the next check. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
New changes lgtm. https://codereview.chromium.org/2804793002/diff/320001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc (right): https://codereview.chromium.org/2804793002/diff/320001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc:25: using web_modal::WebContentsModalDialogManager; Not needed.
lgtm!
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2804793002/diff/320001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc (right): https://codereview.chromium.org/2804793002/diff/320001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler_win_unittest.cc:25: using web_modal::WebContentsModalDialogManager; On 2017/04/20 04:49:21, Lei Zhang wrote: > Not needed. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rbpotter@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2804793002/#ps340001 (title: "Remove unused line")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1492709892691270,
"parent_rev": "ec5d03e1954f4bd4d5f29aeec3372ac270ece07d", "commit_rev":
"e5be563564bd38f1f8355406a658ff93bc71027a"}
Message was sent while issue was closed.
Description was changed from ========== Print Preview: Fix failure to save with long page title BUG=707538 ========== to ========== Print Preview: Fix failure to save with long page title BUG=707538 Review-Url: https://codereview.chromium.org/2804793002 Cr-Commit-Position: refs/heads/master@{#466053} Committed: https://chromium.googlesource.com/chromium/src/+/e5be563564bd38f1f8355406a658... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:340001) as https://chromium.googlesource.com/chromium/src/+/e5be563564bd38f1f8355406a658... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
