Print Preview: Fix failure to save with long page title

ui/shell_dialogs/select_file_dialog.cc

Index: ui/shell_dialogs/select_file_dialog.cc
diff --git a/ui/shell_dialogs/select_file_dialog.cc b/ui/shell_dialogs/select_file_dialog.cc
index 1aa7a0e26164079e0563943180353d03cdb6c861..74ede08f3ef64c925a66c25444a15dc8e541f44a 100644
--- a/ui/shell_dialogs/select_file_dialog.cc
+++ b/ui/shell_dialogs/select_file_dialog.cc
@@ -5,6 +5,7 @@
 #include "ui/shell_dialogs/select_file_dialog.h"
 
 #include <stddef.h>
+#include <algorithm>
 
 #include "base/bind.h"
 #include "base/location.h"
@@ -74,6 +75,39 @@ scoped_refptr<SelectFileDialog> SelectFileDialog::Create(
   return CreateSelectFileDialog(listener, policy);
 }
 
+namespace {

[Lei Zhang, 2017/04/13 19:33:53]

There's already one up on line 20.

[rbpotter, 2017/04/13 20:53:56]

Done.

+
+base::FilePath GetShortenedFilePath(const base::FilePath& path) {

[Lei Zhang, 2017/04/13 19:33:53]

Can you add a comment to explain why we need this?

[rbpotter, 2017/04/13 20:53:56]

Done.

+  base::FilePath filename = path.BaseName();
+  base::FilePath short_path = path;
+  const size_t kMaxNameLength = 255;
+  if (filename.value().length() > kMaxNameLength) {

[Lei Zhang, 2017/04/13 19:33:53]

Flip to <=, and then return early.

[rbpotter, 2017/04/13 20:53:56]

Done.

+    base::FilePath::StringType extension = filename.FinalExtension();
+    filename = filename.RemoveFinalExtension();
+    // 1 for . plus 12 for longest known extension.
+    size_t max_extension_length = 13;
+    if (filename.value().length() < kMaxNameLength) {

[Lei Zhang, 2017/04/13 19:33:53]

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.

[rbpotter, 2017/04/13 20:53:56]

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?

[Lei Zhang, 2017/04/13 21:13:44]

Try writing 13U instead?

+      max_extension_length = std::max(
+          max_extension_length, kMaxNameLength - filename.value().length());
+    }
+    if (extension.length() >= max_extension_length) {
+      // Take the last max_extension_length - 1 characters (save 1 character
+      // for the '.').
+      base::FilePath::StringType new_extension;
+      new_extension[0] = base::FilePath::kExtensionSeparator;

[Lei Zhang, 2017/04/13 19:33:53]

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;

[rbpotter, 2017/04/13 20:53:56]

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.

+      extension = new_extension.append(
+          extension.substr(extension.length() - max_extension_length + 1,
+                           max_extension_length - 1));
+    }
+    short_path = path.DirName().Append(
+        filename.value().substr(0, kMaxNameLength - extension.length()));
+    short_path = short_path.AddExtension(extension);
+  }
+  return short_path;
+}
+
+}  // namespace
+
 void SelectFileDialog::SelectFile(
     Type type,
     const base::string16& title,
@@ -98,8 +132,10 @@ void SelectFileDialog::SelectFile(
     return;
   }
 
+  base::FilePath path = GetShortenedFilePath(default_path);
+
   // Call the platform specific implementation of the file selection dialog.
-  SelectFileImpl(type, title, default_path, file_types, file_type_index,
+  SelectFileImpl(type, title, path, file_types, file_type_index,
                  default_extension, owning_window, params);
 }