Update Windows System Monitor Removable Device Impl.

chrome/browser/system_monitor/removable_device_notifications_window_win.cc

Index: chrome/browser/system_monitor/removable_device_notifications_window_win.cc
diff --git a/chrome/browser/system_monitor/removable_device_notifications_window_win.cc b/chrome/browser/system_monitor/removable_device_notifications_window_win.cc
index 6cd090ee10bad44abc75bd13b7a9817118e7d6f1..4da4ae7bfb89a8ee70eed657262fe425a063b488 100644
--- a/chrome/browser/system_monitor/removable_device_notifications_window_win.cc
+++ b/chrome/browser/system_monitor/removable_device_notifications_window_win.cc
@@ -6,33 +6,107 @@
 
 #include <windows.h>
 #include <dbt.h>
-
-#include <string>
+#include <fileapi.h>
 
 #include "base/file_path.h"
+#include "base/metrics/histogram.h"
 #include "base/string_number_conversions.h"
 #include "base/system_monitor/system_monitor.h"
+#include "base/utf_string_conversions.h"
 #include "base/win/wrapped_window_proc.h"
 #include "chrome/browser/system_monitor/media_device_notifications_utils.h"
 #include "chrome/browser/system_monitor/media_storage_util.h"
 #include "content/public/browser/browser_thread.h"
 
+namespace chrome {

[rvargas (doing something else), 2012/09/15 02:33:55]

I don't see any reason for extending the chrome namespace to include the using
directives nor the anon namespace.

[vandebo (ex-Chrome), 2012/09/15 19:45:10]

Fixed.

+
 using base::SystemMonitor;
+using base::win::WrappedWindowProc;
 using content::BrowserThread;
 
 namespace {
 
-const wchar_t WindowClassName[] = L"Chrome_RemovableDeviceNotificationWindow";
+const DWORD kMaxPathBufLen = MAX_PATH + 1;
+
+const wchar_t kWindowClassName[] = L"Chrome_RemovableDeviceNotificationWindow";
+
+static RemovableDeviceNotificationsWindowWin*
+    g_removable_device_notifications_window_win = NULL;
+
+// The following msdn blog entry is helpful for understanding disk volumes
+// and how they are treated in Windows:
+// http://blogs.msdn.com/b/adioltean/archive/2005/04/16/408947.aspx
+bool GetDeviceInfo(const FilePath& device_path, std::wstring* device_location,
+                   std::string* unique_id, string16* name, bool* removable) {
+  wchar_t mount_point[kMaxPathBufLen];
+  if (!GetVolumePathName(device_path.value().c_str(), mount_point,
+                         kMaxPathBufLen)) {
+    return false;
+  }
+  if (device_location)
+    *device_location = std::wstring(mount_point);

[rvargas (doing something else), 2012/09/15 02:33:55]

string16

[vandebo (ex-Chrome), 2012/09/15 19:45:10]

Changed all wchar/wstring to char16/string16

+
+  if (unique_id) {
+    wchar_t guid[kMaxPathBufLen];
+    if (!GetVolumeNameForVolumeMountPoint(mount_point, guid, kMaxPathBufLen))
+      return false;
+    // In case it has two GUID's (see above mentioned blog), do it again.
+    if (!GetVolumeNameForVolumeMountPoint(guid, guid, kMaxPathBufLen))

[rvargas (doing something else), 2012/09/15 02:33:55]

This is not correct. What the blog says is to call the function on the "current"
drive root first and build the path again from there. Note that the guids are
actually consistent all the time, so most likely you don't have to care about
this issue at all. (but I don't know what's the end purpose of this code).

[vandebo (ex-Chrome), 2012/09/15 19:45:10]

I still don't completely follow the blog on this point.  If you mean that
C:\drive_d_mount will always give one GUID even if D:\ is mounted there and D:\
consistently gives a different GUID, then I think it does need to be resolved
here because of GetDeviceInfoForPath()

[rvargas (doing something else), 2012/09/17 06:10:11]

But you said below that you only care about one mount point, and that would be
two mount points for d.

OK, if you really have to get to the drive (something smells funny there), then
the issue is that in order to get to c:\drive_d_mount you first have to make
sure c:\ is really c:\... so you'd have to expand c: to a volume name and then
try volumex-y\mount_d_drive

[vandebo (ex-Chrome), 2012/09/17 18:51:59]

In terms of notifying SM about a device attached, we only care about one mount
point, yes.  But if our consumer asks for device info for a given path, we have
to consider any mount point relevant to that path.
 
I see.  Updated.

[rvargas (doing something else), 2012/09/17 22:26:06]

I'm sorry... after thinking this through I realized that what I said doesn't
really makes sense. What you had before is actually correct... not that it makes
sense either, but it works around a bug that was fixed with Vista SP1.

[vandebo (ex-Chrome), 2012/09/17 23:09:15]

Changed back.

+      return false;
+    WideToUTF8(guid, wcslen(guid), unique_id);
+  }
 
-LRESULT GetVolumeName(LPCWSTR drive,
-                      LPWSTR volume_name,
-                      unsigned int volume_name_len) {
-  return GetVolumeInformation(drive, volume_name, volume_name_len, NULL, NULL,
-      NULL, NULL, 0);
+  if (name) {
+    wchar_t volume_name[kMaxPathBufLen];
+    if (!GetVolumeInformation(mount_point, volume_name, kMaxPathBufLen, NULL,
+                              NULL, NULL, NULL, 0)) {
+      return false;
+    }
+    if (wcslen(volume_name) > 0) {
+      WideToUTF16(volume_name, wcslen(volume_name), name);

[rvargas (doing something else), 2012/09/15 02:33:55]

ah?

+    } else {
+      *name = device_path.LossyDisplayName();
+    }
+  }
+
+  if (removable) {
+    UINT type = GetDriveType(mount_point);
+    *removable = (type == DRIVE_REMOVABLE);
+  }
+
+  return true;
+}
+
+std::vector<FilePath> GetAttachedDevices() {
+  std::vector<FilePath> result;
+  wchar_t volume_name[kMaxPathBufLen];
+  HANDLE find_handle = FindFirstVolume(volume_name, kMaxPathBufLen);
+  if (find_handle == INVALID_HANDLE_VALUE)
+    return result;
+
+  while (true) {
+    wchar_t volume_path[kMaxPathBufLen];
+    DWORD return_count;
+    if (GetVolumePathNamesForVolumeName(volume_name, volume_path,
+                                        kMaxPathBufLen, &return_count)) {
+      if (GetDriveType(volume_path) == DRIVE_REMOVABLE)
+        result.push_back(FilePath(volume_path));

[rvargas (doing something else), 2012/09/15 02:33:55]

I assume that you only care about one mount point.

[vandebo (ex-Chrome), 2012/09/15 19:45:10]

Correct.  Would you like a comment?

[rvargas (doing something else), 2012/09/17 06:10:11]

No need, I was just double checking.

+    } else {
+      NOTREACHED();

[rvargas (doing something else), 2012/09/15 02:33:55]

OS calls may (and will) fail at any time

[vandebo (ex-Chrome), 2012/09/15 19:45:10]

I think this function will fail gracefully - just skip the volume where the call
failed.  I added the NOTREACHED in case it starts consistently failing, a
developer will notice.

[rvargas (doing something else), 2012/09/17 06:10:11]

Yeah, the function works fine. It's just that random DCHECKs are annoying when
running debug builds. If I hit this DCHECK and I'm not really working on this
code, I'd remove it, because there's no guarantee that the OS call is not going
to fail, and no side effect. If the failure was significant enough while
developing the code (now), the function should have failed.

If you want to help someone debugging a random failure, a DLOG is a better
option.

[vandebo (ex-Chrome), 2012/09/17 18:51:59]

Done.

+    }
+    if (!FindNextVolume(find_handle, volume_name, kMaxPathBufLen)) {
+      if (GetLastError() != ERROR_NO_MORE_FILES)
+        NOTREACHED();
+      break;
+    }
+  }
+
+  FindVolumeClose(find_handle);
+  return result;
 }
 
 // Returns 0 if the devicetype is not volume.
-DWORD GetVolumeBitMaskFromBroadcastHeader(DWORD data) {
+uint32 GetVolumeBitMaskFromBroadcastHeader(DWORD data) {
   PDEV_BROADCAST_HDR dev_broadcast_hdr =

[rvargas (doing something else), 2012/09/15 02:33:55]

DEV_BROADCAST_HDR* ... which points me to the fact that the argument is not a
DWORD, it is an LPARAM

[vandebo (ex-Chrome), 2012/09/15 19:45:10]

Fixed.

       reinterpret_cast<PDEV_BROADCAST_HDR>(data);
   if (dev_broadcast_hdr->dbch_devicetype == DBT_DEVTYP_VOLUME) {
@@ -43,130 +117,148 @@ DWORD GetVolumeBitMaskFromBroadcastHeader(DWORD data) {
   return 0;
 }
 
-}  // namespace
+FilePath DriveNumberToFilePath(int drive_number) {
+  FilePath::StringType path(L"_:\\");

[rvargas (doing something else), 2012/09/15 02:33:55]

nit: use string16 directly

[vandebo (ex-Chrome), 2012/09/15 19:45:10]

Done.

+  path[0] = L'A' + drive_number;
+  return FilePath(path);
+}
 
-namespace chrome {
+}  // namespace
 
 RemovableDeviceNotificationsWindowWin::RemovableDeviceNotificationsWindowWin()
-    : atom_(0),
+    : window_class_(0),
       instance_(NULL),
       window_(NULL),
-      volume_name_func_(&GetVolumeName) {
-  Init();
+      get_device_info_func_(&GetDeviceInfo) {
 }
 
-RemovableDeviceNotificationsWindowWin::RemovableDeviceNotificationsWindowWin(
-    VolumeNameFunc volume_name_func)
-    : atom_(0),
-      instance_(NULL),
-      window_(NULL),
-      volume_name_func_(volume_name_func) {
-  Init();
+// static
+RemovableDeviceNotificationsWindowWin*
+RemovableDeviceNotificationsWindowWin::GetInstance() {
+  DCHECK(g_removable_device_notifications_window_win);
+  return g_removable_device_notifications_window_win;
 }
 
 void RemovableDeviceNotificationsWindowWin::Init() {
-  WNDCLASSEX window_class;
-  base::win::InitializeWindowClass(
-      WindowClassName,
-      &base::win::WrappedWindowProc<
-          RemovableDeviceNotificationsWindowWin::WndProcThunk>,
-      0, 0, 0, NULL, NULL, NULL, NULL, NULL,
-      &window_class);
-  instance_ = window_class.hInstance;
-  atom_ = RegisterClassEx(&window_class);
-  DCHECK(atom_);
-
-  window_ = CreateWindow(MAKEINTATOM(atom_), 0, 0, 0, 0, 0, 0, 0, 0, instance_,
-                         0);
-  SetWindowLongPtr(window_, GWLP_USERDATA, reinterpret_cast<LONG_PTR>(this));
+  DoInit(&GetAttachedDevices);
 }
 
-RemovableDeviceNotificationsWindowWin::~RemovableDeviceNotificationsWindowWin(
-    ) {
-  if (window_)
-    DestroyWindow(window_);
+bool RemovableDeviceNotificationsWindowWin::GetDeviceInfoForPath(
+    const FilePath& path,
+    base::SystemMonitor::RemovableStorageInfo* device_info) {
+  std::wstring location;

[rvargas (doing something else), 2012/09/15 02:33:55]

string16

[vandebo (ex-Chrome), 2012/09/15 19:45:10]

Done.

+  std::string unique_id;
+  string16 name;
+  bool removable;
+  if (!get_device_info_func_(path, &location, &unique_id, &name, &removable))
+    return false;
+
+  // To compute the device id, the device type is needed.  For removable
+  // devices, that requires knowing if there's a DCIM directory, which would
+  // require bouncing over to the file thread.  Instead, just iterate the
+  // devices in SystemMonitor.
+  std::string device_id;
+  if (removable) {
+    std::vector<SystemMonitor::RemovableStorageInfo> attached_devices =
+        SystemMonitor::Get()->GetAttachedRemovableStorage();
+    bool found = false;
+    for (size_t i = 0; i < attached_devices.size(); i++) {
+      MediaStorageUtil::Type type;
+      std::string id;
+      MediaStorageUtil::CrackDeviceId(attached_devices[i].device_id, &type,
+                                      &id);
+      if (id == unique_id) {
+        found = true;
+        device_id = attached_devices[i].device_id;
+        break;
+      }
+    }
+    if (!found)
+      return false;
+  } else {
+    device_id = MediaStorageUtil::MakeDeviceId(
+        MediaStorageUtil::FIXED_MASS_STORAGE, unique_id);
+  }
 
-  if (atom_)
-    UnregisterClass(MAKEINTATOM(atom_), instance_);
+  if (device_info) {
+    device_info->device_id = device_id;
+    device_info->name = name;
+    device_info->location = location;
+  }
+  return true;
 }
 
-LRESULT RemovableDeviceNotificationsWindowWin::OnDeviceChange(UINT event_type,
-                                                              DWORD data) {
+void RemovableDeviceNotificationsWindowWin::Init(
+    GetDeviceInfoFunc get_device_info_func,
+    GetAttachedDevicesFunc get_attached_devices_func) {
+  get_device_info_func_ = get_device_info_func;
+  DoInit(get_attached_devices_func);
+}
+
+void RemovableDeviceNotificationsWindowWin::OnDeviceChange(UINT event_type,
+                                                           DWORD data) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   switch (event_type) {
     case DBT_DEVICEARRIVAL: {
       DWORD unitmask = GetVolumeBitMaskFromBroadcastHeader(data);
       for (int i = 0; unitmask; ++i, unitmask >>= 1) {
-        if (unitmask & 0x01) {
-          FilePath::StringType drive(L"_:\\");
-          drive[0] = L'A' + i;
-          WCHAR volume_name[MAX_PATH + 1];
-          if ((*volume_name_func_)(drive.c_str(), volume_name, MAX_PATH + 1)) {
-            // TODO(kmadhusu) We need to look up a real device id as well as
-            // having a fall back for volume name.
-            std::string device_id = MediaStorageUtil::MakeDeviceId(
-                MediaStorageUtil::REMOVABLE_MASS_STORAGE_WITH_DCIM,
-                base::IntToString(i));
-            BrowserThread::PostTask(
-                BrowserThread::FILE, FROM_HERE,
-                base::Bind(&RemovableDeviceNotificationsWindowWin::
-                    CheckDeviceTypeOnFileThread, this, device_id,
-                    FilePath::StringType(volume_name), FilePath(drive)));
-          }
-        }
+        if (!(unitmask & 0x01))
+          continue;
+        AddNewDevice(DriveNumberToFilePath(i));
       }
       break;
     }
     case DBT_DEVICEREMOVECOMPLETE: {
       DWORD unitmask = GetVolumeBitMaskFromBroadcastHeader(data);
       for (int i = 0; unitmask; ++i, unitmask >>= 1) {
-        if (unitmask & 0x01) {
-          std::string device_id = MediaStorageUtil::MakeDeviceId(
-              MediaStorageUtil::REMOVABLE_MASS_STORAGE_WITH_DCIM,
-              base::IntToString(i));
-          SystemMonitor::Get()->ProcessRemovableStorageDetached(device_id);
-        }
+        if (!(unitmask & 0x01))
+          continue;
+
+        FilePath device = DriveNumberToFilePath(i);
+        MountPointDeviceIdMap::const_iterator device_info =
+            device_ids_.find(device);
+        // If the devices isn't type removable (like a CD), it won't be there.
+        if (device_info == device_ids_.end())
+          continue;
+
+        SystemMonitor::Get()->ProcessRemovableStorageDetached(
+            device_info->second);
+        device_ids_.erase(device_info);
       }
       break;
     }
   }
-  return TRUE;
 }
 
-void RemovableDeviceNotificationsWindowWin::CheckDeviceTypeOnFileThread(
-    const std::string& id,
-    const FilePath::StringType& device_name,
-    const FilePath& path) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  if (!IsMediaDevice(path.value()))
-    return;
+RemovableDeviceNotificationsWindowWin::
+    ~RemovableDeviceNotificationsWindowWin() {
+  if (window_)
+    DestroyWindow(window_);
 
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::Bind(
-          &RemovableDeviceNotificationsWindowWin::
-              ProcessRemovableDeviceAttachedOnUIThread,
-          this, id, device_name, path));
-}
+  if (window_class_)
+    UnregisterClass(MAKEINTATOM(window_class_), instance_);
 
-void
-RemovableDeviceNotificationsWindowWin::ProcessRemovableDeviceAttachedOnUIThread(
-    const std::string& id,
-    const FilePath::StringType& device_name,
-    const FilePath& path) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_EQ(this, g_removable_device_notifications_window_win);
+  g_removable_device_notifications_window_win = NULL;
+}
 
-  SystemMonitor::Get()->ProcessRemovableStorageAttached(id,
-                                                        device_name,
-                                                        path.value());
+// static
+LRESULT CALLBACK RemovableDeviceNotificationsWindowWin::WndProcThunk(
+    HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam) {
+  RemovableDeviceNotificationsWindowWin* msg_wnd =
+      reinterpret_cast<RemovableDeviceNotificationsWindowWin*>(
+          GetWindowLongPtr(hwnd, GWLP_USERDATA));
+  if (msg_wnd)
+    return msg_wnd->WndProc(hwnd, message, wparam, lparam);
+  return ::DefWindowProc(hwnd, message, wparam, lparam);
 }
 
 LRESULT CALLBACK RemovableDeviceNotificationsWindowWin::WndProc(
     HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam) {
   switch (message) {
     case WM_DEVICECHANGE:
-      return OnDeviceChange(static_cast<UINT>(wparam),
-                            static_cast<DWORD>(lparam));
+      OnDeviceChange(static_cast<UINT>(wparam), static_cast<DWORD>(lparam));
+      return TRUE;
     default:
       break;
   }
@@ -174,18 +266,75 @@ LRESULT CALLBACK RemovableDeviceNotificationsWindowWin::WndProc(
   return ::DefWindowProc(hwnd, message, wparam, lparam);
 }
 
-// static
-LRESULT CALLBACK RemovableDeviceNotificationsWindowWin::WndProcThunk(
-    HWND hwnd,
-    UINT message,
-    WPARAM wparam,
-    LPARAM lparam) {
-  RemovableDeviceNotificationsWindowWin* msg_wnd =
-      reinterpret_cast<RemovableDeviceNotificationsWindowWin*>(
-          GetWindowLongPtr(hwnd, GWLP_USERDATA));
-  if (msg_wnd)
-    return msg_wnd->WndProc(hwnd, message, wparam, lparam);
-  return ::DefWindowProc(hwnd, message, wparam, lparam);
+void RemovableDeviceNotificationsWindowWin::DoInit(
+    GetAttachedDevicesFunc get_attached_devices_func) {
+  DCHECK(!g_removable_device_notifications_window_win);
+  g_removable_device_notifications_window_win = this;

[rvargas (doing something else), 2012/09/15 02:33:55]

Basically this is a home grown singleton (without the documentation saying that
it has to be). Use a singleton or a lazy instance.

[vandebo (ex-Chrome), 2012/09/15 19:45:10]

I looked at both before and again just now.  There is a subtle difference that I
think means I can't use either.  SystemMonitor is scoped object created in
content BrowserMainLoop.  The AtExitManager is in a scope above that.  So using
Singleton or LazyInstance with default traits would clean up this object too
late.

Not that it ameliorates the issue here, but it was recently decided (a couple
days ago) that SystemMonitor has outgrown its design and needs to be refactored.
 It's likely that each part of it will be split out into its own class.  So the
removable device notification which is updated in this CL will be managed by an
object in chrome/browser or will become a Singleton/LazyInstance.

[rvargas (doing something else), 2012/09/17 06:10:11]

We need at least some documentation stating that there can only be one instance
of this class.

[vandebo (ex-Chrome), 2012/09/17 18:51:59]

Done.

+
+  WNDCLASSEX window_class;

[cpu_(ooo_6.6-7.5), 2012/09/14 20:49:48]

this does not work on windows 8 metro mode.

I strongly suggest you consider a design that does not have poup native windows.
Does not work today, and will not work later when aura + ash runs on windows.

I hope you are not trying to land this in m23.

[vandebo (ex-Chrome), 2012/09/14 21:19:21]

In case you stopped reading at this point, we don't ever show this window, it's
a hidden window used just to receive the device arrival/disconnect messages. 
Will that not work in windows 8 metro mode?
It seems that it doesn't do anything bad because this code is already in the
codebase...

+  base::win::InitializeWindowClass(
+      kWindowClassName,
+      &WrappedWindowProc<RemovableDeviceNotificationsWindowWin::WndProcThunk>,
+      0, 0, 0, NULL, NULL, NULL, NULL, NULL,
+      &window_class);
+  instance_ = window_class.hInstance;
+  window_class_ = RegisterClassEx(&window_class);
+  DCHECK(window_class_);
+
+  window_ = CreateWindow(MAKEINTATOM(window_class_), 0, 0, 0, 0, 0, 0, 0, 0,
+                         instance_, 0);
+  SetWindowLongPtr(window_, GWLP_USERDATA, reinterpret_cast<LONG_PTR>(this));
+
+  std::vector<FilePath> removable_devices = get_attached_devices_func();
+  for (size_t i = 0; i < removable_devices.size(); i++)
+    AddNewDevice(removable_devices[i]);
+}
+
+void RemovableDeviceNotificationsWindowWin::AddNewDevice(
+    const FilePath& device_path) {
+  std::string unique_id;
+  string16 device_name;
+  bool removable;
+  if (!get_device_info_func_(device_path, NULL, &unique_id, &device_name,
+                             &removable)) {
+    return;
+  }
+
+  if (!removable)
+    return;
+
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, base::Bind(
+      &RemovableDeviceNotificationsWindowWin::CheckDeviceTypeOnFileThread,
+      this, unique_id, device_name, device_path));
+}
+
+void RemovableDeviceNotificationsWindowWin::CheckDeviceTypeOnFileThread(
+    const std::string& unique_id,
+    const FilePath::StringType& device_name,
+    const FilePath& device) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  MediaStorageUtil::Type type =
+      MediaStorageUtil::REMOVABLE_MASS_STORAGE_NO_DCIM;
+  if (IsMediaDevice(device.value()))
+    type = MediaStorageUtil::REMOVABLE_MASS_STORAGE_WITH_DCIM;
+  std::string device_id = MediaStorageUtil::MakeDeviceId(type, unique_id);
+
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(
+      &RemovableDeviceNotificationsWindowWin::ProcessDeviceAttachedOnUIThread,
+      this, device_id, device_name, device));
+}
+
+void RemovableDeviceNotificationsWindowWin::ProcessDeviceAttachedOnUIThread(
+    const std::string& device_id,
+    const FilePath::StringType& device_name,
+    const FilePath& device) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  device_ids_[device] = device_id;

[rvargas (doing something else), 2012/09/15 02:33:55]

out of curiosity, base' SM keeps a list of removable devices and we keep another
one here?

[vandebo (ex-Chrome), 2012/09/15 19:45:10]

Base SM has a vector of the attached devices.  We could fetch that (makes a
copy) and iterate it instead of keeping our own state here.  A future refactor
may alliterative this inefficiency.  We need to access it because the device id
is dependent on the content of the drive so we can't determine it once the
device has departed.

+  SystemMonitor::Get()->ProcessRemovableStorageAttached(device_id,
+                                                        device_name,
+                                                        device.value());
 }
 
 }  // namespace chrome