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

base/file_util_win.cc

Index: base/file_util_win.cc
diff --git a/base/file_util_win.cc b/base/file_util_win.cc
index 31d2f05d8dca7d06e29d1604472e5ed8507c1748..d4ae1c6b64ae33592e0b4911c6dcb06d86b16b28 100644
--- a/base/file_util_win.cc
+++ b/base/file_util_win.cc
@@ -1,4 +1,4 @@
-// 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.
 
@@ -25,6 +25,7 @@
 #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"
 
@@ -35,15 +36,18 @@ 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;
   }
@@ -51,7 +55,7 @@ bool DevicePathToDriveLetterPath(const FilePath& device_path,
   // 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
@@ -60,11 +64,14 @@ bool DevicePathToDriveLetterPath(const FilePath& device_path,
   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.
@@ -77,8 +84,6 @@ bool DevicePathToDriveLetterPath(const FilePath& device_path,
   return false;
 }
 
-}  // namespace
-
 bool AbsolutePath(FilePath* path) {
   base::ThreadRestrictions::AssertIOAllowed();
   wchar_t file_path_buf[MAX_PATH];
@@ -1044,7 +1049,9 @@ bool NormalizeFilePath(const FilePath& path, FilePath* real_path) {
   // "\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) {