Fix the open file dialog on Windows

ui/base/win/open_file_name_win.cc

 // Copyright (c) 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/base/win/open_file_name_win.h"
 
 #include "base/files/file_path.h"
 #include "base/strings/string_util.h"
 #include "base/win/windows_version.h"
 
@@ skipping 120 matching lines @@
 
 void OpenFileName::MaybeInstallWindowPositionHookForSaveAsOnXP() {
   if (base::win::GetVersion() >= base::win::VERSION_VISTA)
     return;
 
   openfilename_.Flags |= OFN_ENABLEHOOK;
   DCHECK(!openfilename_.lpfnHook);
   openfilename_.lpfnHook = &SaveAsDialogHook;
 }
 
 base::FilePath OpenFileName::GetSingleResult() {

[sky, 2014/09/29 23:53:03]

I would rather we always do the right thing here, and not assume GetSingleResult
is only called if allow multi-select was false.

[luken, 2014/09/30 17:25:07]

Agreed, your idea is better. I've refactored to just extract the values
correctly in GetResult().

-  base::FilePath directory;
-  std::vector<base::FilePath> filenames;
-  GetResult(&directory, &filenames);
-  if (filenames.size() != 1)
-    return base::FilePath();
-  return directory.Append(filenames[0]);
+  // The return value of |openfilename_.lpstrFile| is dependent on the
+  // value of the Multi-Select flag within |openfilename_|. If the flag is
+  // not set the return value will be a single null-terminated wide string.
+  // If it is set it will be more than one null-terminated wide string, itself
+  // terminated by an empty null-terminated wide string. We assert that
+  // GetSingleResult() is assuming there is only a single result to return.
+  DCHECK_EQ(0U, openfilename_.Flags & OFN_ALLOWMULTISELECT);
+  return base::FilePath(openfilename_.lpstrFile);
 }
 
 void OpenFileName::GetResult(base::FilePath* directory,
                              std::vector<base::FilePath>* filenames) {
   DCHECK(filenames->empty());
+  // The return value of |openfilename_.lpstrFile| is dependent on the

[sky, 2014/09/29 23:53:03]

nit: don't duplicate almost the whole comment. Do something like, see comment in
GetSingleResult for details.

[luken, 2014/09/30 17:25:07]

Done.

+  // value of the Multi-Select flag within |openfilename_|. If the flag is
+  // not set the return value will be a single null-terminated wide string.
+  // If it is set it will be more than one null-terminated wide string, itself
+  // terminated by an empty null-terminated wide string. We assert that
+  // GetResult() is assuming there is more than one result to return.
+  DCHECK_NE(0U, openfilename_.Flags & OFN_ALLOWMULTISELECT);
   const wchar_t* selection = openfilename_.lpstrFile;
   while (*selection) {  // Empty string indicates end of list.
     filenames->push_back(base::FilePath(selection));
     // Skip over filename and null-terminator.
     selection += filenames->back().value().length() + 1;
   }
   if (filenames->size() == 1) {
     // When there is one file, it contains the path and filename.
     *directory = (*filenames)[0].DirName();
     (*filenames)[0] = (*filenames)[0].BaseName();
@@ skipping 53 matching lines @@
         MakeTuple(base::string16(display_string, display_string_end),
                   base::string16(pattern, pattern_end)));
     display_string = pattern_end + 1;
   }
 
   return filters;
 }
 
 }  // namespace win
 }  // namespace ui