Print Preview: Fix failure to save with long page title

ui/shell_dialogs/select_file_dialog_unittest.cc

Index: ui/shell_dialogs/select_file_dialog_unittest.cc
diff --git a/ui/shell_dialogs/select_file_dialog_unittest.cc b/ui/shell_dialogs/select_file_dialog_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..045a114bae89e01f3103f6cd0e662d97950fdc8c
--- /dev/null
+++ b/ui/shell_dialogs/select_file_dialog_unittest.cc
@@ -0,0 +1,97 @@
+// Copyright (c) 2017 The Chromium Authors. All rights reserved.

[Lei Zhang, 2017/04/15 22:19:34]

No (c)

[rbpotter, 2017/04/17 17:48:49]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <stddef.h>
+
+#include "base/macros.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/shell_dialogs/select_file_dialog.h"
+
+namespace {
+#if defined(OS_WIN)

[Lei Zhang, 2017/04/15 22:19:34]

Can you get rid of the ifdefs with the help of FILE_PATH_LITERAL?

[rbpotter, 2017/04/17 17:48:49]

Done.

+const base::FilePath::StringType folder1 = L"folder1111\\";

[Lei Zhang, 2017/04/15 22:19:34]

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.

[rbpotter, 2017/04/17 17:48:49]

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.

+const base::FilePath::StringType folder2 = L"folder2222\\";
+const base::FilePath::StringType short_name = L"file1";
+const base::FilePath::StringType medium_name =
+    L"file12345678901234567890123456789012345678901234567890";
+const base::FilePath::StringType long_name =
+    L"abcdefghijklmnopqrstuvwxyz1234abcdefghijklmnopqrstuvwxyz1234abcdefghijklm"
+    L"nopqrstuvwxyz1234abcdefghijklmnopqrstuvwxyz1234abcdefghijklmnopqrstuvwxyz"
+    L"1234abcdefghijklmnopqrstuvwxyz1234abcdefghijklmnopqrstuvwxyz1234abcdefghi"
+    L"jklmnopqrstuvwxyz1234abcdefghijklmnopqrstuvwxyz1234";
+const base::FilePath::StringType html_ext = L".html";
+const base::FilePath::StringType pdf_ext = L".pdf";
+const base::FilePath::StringType long_ext =
+    long_name.substr(0, 210).insert(0, L".");

[Lei Zhang, 2017/04/15 22:19:34]

So just write this out.

[rbpotter, 2017/04/17 17:48:49]

Done.

+#else
+const base::FilePath::StringType folder1 = "folder1111/";
+const base::FilePath::StringType folder2 = "folder2222/";
+const base::FilePath::StringType short_name = "file1";
+const base::FilePath::StringType medium_name =
+    "file12345678901234567890123456789012345678901234567890";
+const base::FilePath::StringType long_name =
+    "abcdefghijklmnopqrstuvwxyz1234abcdefghijklmnopqrstuvwxyz1234abcdefghijklmn"
+    "opqrstuvwxyz1234abcdefghijklmnopqrstuvwxyz1234abcdefghijklmnopqrstuvwxyz12"
+    "34abcdefghijklmnopqrstuvwxyz1234abcdefghijklmnopqrstuvwxyz1234abcdefghijkl"
+    "mnopqrstuvwxyz1234abcdefghijklmnopqrstuvwxyz1234";
+const base::FilePath::StringType html_ext = ".html";
+const base::FilePath::StringType pdf_ext = ".pdf";
+const base::FilePath::StringType long_ext =
+    long_name.substr(0, 210).insert(0, ".");
+#endif
+}  // namespace
+
+TEST(ShellDialogs, ShortenFileNameIfNeeded) {
+  struct ShortenFileNameTestCase {
+    base::FilePath::StringType path;
+    base::FilePath::StringType expected;
+  } test_cases[] = {{folder1, folder1}, {folder1, folder1}, {folder1, folder1},

[Lei Zhang, 2017/04/15 22:19:34]

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.

[rbpotter, 2017/04/17 17:48:49]

Done.

+                    {folder1, folder1}, {folder1, folder1}, {folder1, folder1}};
+  // Short enough file names do not get truncated.
+  test_cases[0].path.append(folder2).append(short_name).append(html_ext);
+  test_cases[0].expected = test_cases[0].path;
+
+  // Long name should be truncated to 255 chars including extension.
+  test_cases[1].path.append(long_name).append(html_ext);
+  test_cases[1].expected.append(long_name.substr(0, 250)).append(html_ext);
+
+  // Long path but short file name is also not truncated (handled by system
+  // dialog).
+  for (int i = 0; i < 26; i++)
+    test_cases[2].path.append(folder2);
+  test_cases[2].path.append(short_name).append(pdf_ext);
+  test_cases[2].expected = test_cases[2].path;
+
+  // Long extension on a short path with total length < 255 is not truncated.
+  test_cases[3].path.append(folder2).append(short_name).append(long_ext);
+  test_cases[3].expected = test_cases[3].path;
+
+  // Long extension on a longer filename is truncated to length needed to get
+  // total length = 255
+  test_cases[4].path.append(folder2).append(medium_name).append(long_ext);
+  test_cases[4]
+      .expected.append(folder2)
+      .append(medium_name)
+      .append(long_ext.substr(0, 255 - 54));  // medium file name has length 54.
+
+  // Long extension on a very long filename leads to extension truncated to 13
+  // chars and long filename truncated to 255 - 13 chars.
+  test_cases[5].path.append(long_name).append(long_ext);
+  test_cases[5]
+      .expected.append(long_name.substr(0, 255 - 13))
+      .append(long_ext.substr(0, 13));
+
+  for (size_t i = 0; i < arraysize(test_cases); ++i) {
+    EXPECT_EQ(test_cases[i].expected,
+              ui::SelectFileDialog::GetShortenedFilePath(
+                  base::FilePath(test_cases[i].path))
+                  .value());
+    EXPECT_LE(ui::SelectFileDialog::GetShortenedFilePath(
+                  base::FilePath(test_cases[i].path))
+                  .BaseName()
+                  .value()
+                  .length(),
+              255u);
+  }
+}