Match whole path components in DevicePathToDriveLetterPath(). Add tests.

base/file_util_win.cc

-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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 "base/file_util.h"
 
 #include <windows.h>
 #include <propvarutil.h>
 #include <psapi.h>
 #include <shellapi.h>
 #include <shlobj.h>
 #include <time.h>
 
 #include <limits>
 #include <string>
 
 #include "base/file_path.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "base/win/pe_image.h"
 #include "base/win/scoped_comptr.h"
 #include "base/win/scoped_handle.h"
+#include "base/win/win_api_interface.h"
 #include "base/win/win_util.h"
 #include "base/win/windows_version.h"
 
 namespace file_util {
 
 namespace {
 
 const DWORD kFileShareAll =
     FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE;
 
+}  // namespace
+
 // Helper for NormalizeFilePath(), defined below.
-bool DevicePathToDriveLetterPath(const FilePath& device_path,
-                                 FilePath* drive_letter_path) {
+bool DevicePathToDriveLetterPath(base::win::WinApiInterface& win_api,

[rvargas (doing something else), 2012/01/11 22:26:26]

I don't think that adding an argument with fn pointers to every function we want
to test is something that scales.

I'm also wondering if it's not better to test the caller of this code, given
that so far this is just a "private helper" that is used from a single location.

[Sam Kerner (Chrome), 2012/01/11 23:26:27]

If I tested the caller by adding a parameter, users of base would need to get
the new parameter.  I wanted to avoid changing the exported interface.  I
realize that I ended up exporting DevicePathToDriveLetterPath().  My intention
is to only expose it for tests.

[rvargas (doing something else), 2012/01/11 23:53:33]

And that's one of the arguments against testing passing an extra argument to
every fn we want to test.

+                                 const FilePath& full_nt_device_path,
+                                 FilePath* out_drive_letter_path) {
   base::ThreadRestrictions::AssertIOAllowed();
 
   // Get the mapping of drive letters to device paths.
   const int kDriveMappingSize = 1024;
   wchar_t drive_mapping[kDriveMappingSize] = {'\0'};
-  if (!::GetLogicalDriveStrings(kDriveMappingSize - 1, drive_mapping)) {
+  if (!win_api.GetLogicalDriveStrings(kDriveMappingSize - 1, drive_mapping)) {
     DLOG(ERROR) << "Failed to get drive mapping.";
     return false;
   }
 
   // The drive mapping is a sequence of null terminated strings.
   // The last string is empty.
   wchar_t* drive_map_ptr = drive_mapping;
-  wchar_t device_name[MAX_PATH];
+  wchar_t device_path_as_string[MAX_PATH];
   wchar_t drive[] = L" :";
 
   // For each string in the drive mapping, get the junction that links
   // to it.  If that junction is a prefix of |device_path|, then we
   // know that |drive| is the real path prefix.
   while (*drive_map_ptr) {
     drive[0] = drive_map_ptr[0];  // Copy the drive letter.
 
-    if (QueryDosDevice(drive, device_name, MAX_PATH) &&
-        StartsWith(device_path.value(), device_name, true)) {
-      *drive_letter_path = FilePath(drive +
-          device_path.value().substr(wcslen(device_name)));
-      return true;
+    if (win_api.QueryDosDevice(drive, device_path_as_string, MAX_PATH)) {
+      FilePath device_path_to_drive(device_path_as_string);

[rvargas (doing something else), 2012/01/11 22:26:26]

This name is confusing. How about just device_path ?

+      if (device_path_to_drive == full_nt_device_path ||
+          device_path_to_drive.IsParent(full_nt_device_path)) {
+        *out_drive_letter_path = FilePath(drive +
+            full_nt_device_path.value().substr(wcslen(device_path_as_string)));
+        return true;
+      }
     }
     // Move to the next drive letter string, which starts one
     // increment after the '\0' that terminates the current string.
     while (*drive_map_ptr++);
   }
 
   // No drive matched.  The path does not start with a device junction
   // that is mounted as a drive letter.  This means there is no drive
   // letter path to the volume that holds |device_path|, so fail.
   return false;
 }
 
-}  // namespace
-
 bool AbsolutePath(FilePath* path) {
   base::ThreadRestrictions::AssertIOAllowed();
   wchar_t file_path_buf[MAX_PATH];
   if (!_wfullpath(file_path_buf, path->value().c_str(), MAX_PATH))
     return false;
   *path = FilePath(file_path_buf);
   return true;
 }
 
 int CountFilesCreatedAfter(const FilePath& path,
@@ skipping 945 matching lines @@
 
 bool NormalizeFilePath(const FilePath& path, FilePath* real_path) {
   base::ThreadRestrictions::AssertIOAllowed();
   FilePath mapped_file;
   if (!NormalizeToNativeFilePath(path, &mapped_file))
     return false;
   // NormalizeToNativeFilePath() will return a path that starts with
   // "\Device\Harddisk...".  Helper DevicePathToDriveLetterPath()
   // will find a drive letter which maps to the path's device, so
   // that we return a path starting with a drive letter.
-  return DevicePathToDriveLetterPath(mapped_file, real_path);
+
+  return DevicePathToDriveLetterPath(
+      *base::win::WinApiInterface::GetInstance(), mapped_file, real_path);
 }
 
 bool NormalizeToNativeFilePath(const FilePath& path, FilePath* nt_path) {
   base::ThreadRestrictions::AssertIOAllowed();
   // In Vista, GetFinalPathNameByHandle() would give us the real path
   // from a file handle.  If we ever deprecate XP, consider changing the
   // code below to a call to GetFinalPathNameByHandle().  The method this
   // function uses is explained in the following msdn article:
   // http://msdn.microsoft.com/en-us/library/aa366789(VS.85).aspx
   base::win::ScopedHandle file_handle(
@@ skipping 104 matching lines @@
       uint8 unused = *(touch + offset);
       offset += step_size;
     }
     FreeLibrary(dll_module);
   }
 
   return true;
 }
 
 }  // namespace file_util