[Win, MediaGallery] Enumerate and handle mtp device attach/detach events.

chrome/browser/system_monitor/portable_device_watcher_win.cc

+// 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 "chrome/browser/system_monitor/portable_device_watcher_win.h"
+
+#include <dbt.h>
+#include <portabledevice.h>
+
+#include "base/file_path.h"
+#include "base/logging.h"
+#include "base/stl_util.h"
+#include "base/string_util.h"
+#include "base/threading/sequenced_worker_pool.h"
+#include "base/utf_string_conversions.h"
+#include "base/win/scoped_co_mem.h"
+#include "base/win/scoped_comptr.h"
+#include "chrome/browser/system_monitor/media_storage_util.h"
+#include "chrome/browser/system_monitor/removable_device_constants.h"
+#include "content/public/browser/browser_thread.h"
+
+namespace chrome {
+
+namespace {
+
+// Name of the client application that communicates with the mtp device.
+const char16 kClientName[] = L"Chromium";
+
+// Name of the sequenced task runner.
+const char kMediaTaskRunnerName[] = "media-task-runner";
+
+// Returns true if |data| represents a class of portable devices.
+bool IsPortableDeviceStructure(LPARAM data) {
+  DEV_BROADCAST_HDR* broadcast_hdr =
+      reinterpret_cast<DEV_BROADCAST_HDR*>(data);
+  if (!broadcast_hdr ||
+      (broadcast_hdr->dbch_devicetype != DBT_DEVTYP_DEVICEINTERFACE)) {
+    return false;
+  }
+
+  GUID guidDevInterface = GUID_NULL;
+  if (FAILED(CLSIDFromString(kWPDDevInterfaceGUID, &guidDevInterface)))
+    return false;
+  DEV_BROADCAST_DEVICEINTERFACE* dev_interface =
+      reinterpret_cast<DEV_BROADCAST_DEVICEINTERFACE*>(data);
+  return (IsEqualGUID(dev_interface->dbcc_classguid, guidDevInterface) != 0);
+}
+
+// Returns the portable device plug and play device ID string.
+string16 GetPnpDeviceId(LPARAM data) {
+  DEV_BROADCAST_DEVICEINTERFACE* dev_interface =
+      reinterpret_cast<DEV_BROADCAST_DEVICEINTERFACE*>(data);
+  if (!dev_interface)
+    return string16();
+  string16 device_id(dev_interface->dbcc_name);
+  DCHECK(IsStringASCII(device_id));
+  return StringToLowerASCII(device_id);
+}
+
+// Gets the friendly name of the device specified by the |pnp_device_id|. On
+// success, returns true and fills in |name|.
+bool GetFriendlyName(const string16& pnp_device_id,
+                     IPortableDeviceManager* device_manager,
+                     string16* name) {
+  DCHECK(device_manager);
+  DCHECK(name);
+  DWORD name_len = 0;
+  HRESULT hr = device_manager->GetDeviceFriendlyName(pnp_device_id.c_str(),
+                                                     NULL, &name_len);
+  if (FAILED(hr))
+    return false;
+
+  hr = device_manager->GetDeviceFriendlyName(
+      pnp_device_id.c_str(), WriteInto(name, name_len), &name_len);
+  return SUCCEEDED(hr) ? !name->empty() : false;

[Peter Kasting, 2012/10/26 23:46:23]

Nit: Simpler:

  return SUCCEEDED(hr) && !name->empty();

(several functions)

[kmadhusu, 2012/10/28 22:57:16]

Done.

+}
+
+// Gets the manufacturer name of the device specified by the |pnp_device_id|.
+// On success, returns true and fills in |name|.
+bool GetManufacturerName(const string16& pnp_device_id,
+                         IPortableDeviceManager* device_manager,
+                         string16* name) {
+  DCHECK(device_manager);
+  DCHECK(name);
+  DWORD name_len = 0;
+  HRESULT hr = device_manager->GetDeviceManufacturer(pnp_device_id.c_str(),
+                                                     NULL, &name_len);
+  if (FAILED(hr))
+    return false;
+
+  hr = device_manager->GetDeviceManufacturer(pnp_device_id.c_str(),
+                                             WriteInto(name, name_len),
+                                             &name_len);
+  return SUCCEEDED(hr) ? !name->empty() : false;
+}
+
+// Gets the description of the device specified by the |pnp_device_id|. On
+// success, returns true and fills in |description|.
+bool GetDeviceDescription(const string16& pnp_device_id,
+                          IPortableDeviceManager* device_manager,
+                          string16* description) {
+  DCHECK(device_manager);
+  DCHECK(description);
+  DWORD desc_len = 0;
+  HRESULT hr = device_manager->GetDeviceDescription(pnp_device_id.c_str(), NULL,
+                                                    &desc_len);
+  if (FAILED(hr))
+    return false;
+
+  hr = device_manager->GetDeviceDescription(pnp_device_id.c_str(),
+                                            WriteInto(description, desc_len),
+                                            &desc_len);
+  return SUCCEEDED(hr) ? !description->empty() : false;
+}
+
+// On success, returns true and updates |client_info| with a reference to an
+// IPortableDeviceValues interface that holds information about the
+// application that communicates with the device.
+bool GetClientInformation(
+    base::win::ScopedComPtr<IPortableDeviceValues>* client_info) {
+  HRESULT hr = client_info->CreateInstance(__uuidof(PortableDeviceValues),
+                                           NULL, CLSCTX_INPROC_SERVER);
+  if (FAILED(hr)) {
+    DPLOG(ERROR) << "Failed to create an instance of IPortableDeviceValues";
+    return false;
+  }
+
+  // Attempt to set client details.
+  (*client_info)->SetStringValue(WPD_CLIENT_NAME, kClientName);
+  (*client_info)->SetUnsignedIntegerValue(WPD_CLIENT_MAJOR_VERSION, 0);
+  (*client_info)->SetUnsignedIntegerValue(WPD_CLIENT_MINOR_VERSION, 0);
+  (*client_info)->SetUnsignedIntegerValue(WPD_CLIENT_REVISION, 0);
+  (*client_info)->SetUnsignedIntegerValue(
+      WPD_CLIENT_SECURITY_QUALITY_OF_SERVICE, SECURITY_IMPERSONATION);
+  (*client_info)->SetUnsignedIntegerValue(WPD_CLIENT_DESIRED_ACCESS,
+                                          GENERIC_READ);
+  return true;
+}
+
+// Opens the device for communication. |pnp_device_id| specifies the plug and
+// play device ID string. On success, returns true and updates |device| with a
+// reference to the portable device interface.
+bool SetUp(const string16& pnp_device_id,
+           base::win::ScopedComPtr<IPortableDevice>* device) {
+  base::win::ScopedComPtr<IPortableDeviceValues> client_info;
+  if (!GetClientInformation(&client_info))
+    return false;
+
+  HRESULT hr = device->CreateInstance(__uuidof(PortableDevice), NULL,
+                                      CLSCTX_INPROC_SERVER);
+  if (FAILED(hr)) {
+    DPLOG(ERROR) << "Failed to create an instance of IPortableDevice";
+    return false;
+  }
+
+  hr = (*device)->Open(pnp_device_id.c_str(), client_info.get());
+  if (SUCCEEDED(hr))
+    return true;
+
+  if (hr == E_ACCESSDENIED)
+    DPLOG(ERROR) << "Access denied to open the device";
+  return false;
+}
+
+// Returns the unique id property key of the object specified by the
+// |object_id|.
+REFPROPERTYKEY GetUniqueIdPropertyKey(const string16& object_id) {
+  return (object_id == WPD_DEVICE_OBJECT_ID) ?
+      WPD_DEVICE_SERIAL_NUMBER : WPD_OBJECT_PERSISTENT_UNIQUE_ID;
+}
+
+// On success, returns true and populates |properties_to_read| with the
+// property key of the object specified by the |object_id|.
+bool PopulatePropertyKeyCollection(
+    const string16& object_id,
+    base::win::ScopedComPtr<IPortableDeviceKeyCollection>* properties_to_read) {
+  HRESULT hr = properties_to_read->CreateInstance(
+      __uuidof(PortableDeviceKeyCollection), NULL, CLSCTX_INPROC_SERVER);
+  if (FAILED(hr)) {
+    DPLOG(ERROR) << "Failed to create IPortableDeviceKeyCollection instance";
+    return false;
+  }
+  REFPROPERTYKEY key = GetUniqueIdPropertyKey(object_id);
+  hr = (*properties_to_read)->Add(key);
+  return SUCCEEDED(hr);
+}
+
+// Wrapper function to get content property string value.
+bool GetStringPropertyValue(IPortableDeviceValues* properties_values,
+                            REFPROPERTYKEY key,
+                            string16* value) {
+  DCHECK(properties_values);
+  DCHECK(value);
+  base::win::ScopedCoMem<char16> buffer;
+  HRESULT hr = properties_values->GetStringValue(key, &buffer);
+  if (FAILED(hr))
+    return false;
+  *value = static_cast<const wchar_t*>(buffer);

[Peter Kasting, 2012/10/26 23:46:23]

Nit: wchar_t -> char16, since the original type and the receiving object both
use char16

[kmadhusu, 2012/10/28 22:57:16]

Done.

+  return true;
+}
+
+// Constructs a unique identifier for the object specified by the |object_id|.
+// On success, returns true and fills in |unique_id|.
+bool GetObjectUniqueId(IPortableDevice* device,
+                       const string16& object_id,
+                       string16* unique_id) {
+  DCHECK(device);
+  DCHECK(unique_id);
+  base::win::ScopedComPtr<IPortableDeviceContent> content;
+  HRESULT hr = device->Content(content.Receive());
+  if (FAILED(hr)) {
+    DPLOG(ERROR) << "Failed to get IPortableDeviceContent interface";
+    return false;
+  }
+
+  base::win::ScopedComPtr<IPortableDeviceProperties> properties;
+  hr = content->Properties(properties.Receive());
+  if (FAILED(hr)) {
+    DPLOG(ERROR) << "Failed to get IPortableDeviceProperties interface";
+    return false;
+  }
+
+  base::win::ScopedComPtr<IPortableDeviceKeyCollection> properties_to_read;
+  if (!PopulatePropertyKeyCollection(object_id, &properties_to_read))
+    return false;
+
+  base::win::ScopedComPtr<IPortableDeviceValues> properties_values;
+  if (FAILED(properties->GetValues(object_id.c_str(),
+                                   properties_to_read.get(),
+                                   properties_values.Receive()))) {
+    return false;
+  }
+
+  REFPROPERTYKEY key = GetUniqueIdPropertyKey(object_id);
+  return GetStringPropertyValue(properties_values.get(), key, unique_id);
+}
+
+// Constructs the device storage unique identifier using |device_serial_num| and
+// |storage_id|. On success, returns true and fills in |device_storage_id|.
+bool ConstructDeviceStorageUniqueId(const string16& device_serial_num,
+                              const string16& storage_id,
+                              std::string* device_storage_id) {
+  if (device_serial_num.empty() && storage_id.empty())
+    return false;
+
+  DCHECK(device_storage_id);
+  string16 unique_id(storage_id + L':' + device_serial_num);
+
+  *device_storage_id = MediaStorageUtil::MakeDeviceId(
+       MediaStorageUtil::MTP_OR_PTP, UTF16ToUTF8(unique_id));
+  return true;
+}
+
+// Gets a list of removable storage object identifiers present in |device|.
+// On success, returns true and fills in |storage_ids|.
+bool GetRemovableStorageObjectIds(IPortableDevice* device,
+                                  std::vector<string16>* storage_ids) {
+  DCHECK(device);
+  DCHECK(storage_ids);
+  base::win::ScopedComPtr<IPortableDeviceCapabilities> capabilities;
+  HRESULT hr = device->Capabilities(capabilities.Receive());
+  if (FAILED(hr)) {
+    DPLOG(ERROR) << "Failed to get IPortableDeviceCapabilities interface";
+    return false;
+  }
+
+  base::win::ScopedComPtr<IPortableDevicePropVariantCollection> storage_obj_ids;
+  hr = capabilities->GetFunctionalObjects(WPD_FUNCTIONAL_CATEGORY_STORAGE,
+                                          storage_obj_ids.Receive());
+  if (FAILED(hr)) {
+    DPLOG(ERROR) << "Failed to get IPortableDevicePropVariantCollection";
+    return false;
+  }
+
+  DWORD num_storage_obj_ids = 0;
+  hr = storage_obj_ids->GetCount(&num_storage_obj_ids);
+  if (FAILED(hr))
+    return false;
+
+  for (DWORD index = 0; index < num_storage_obj_ids; ++index) {
+    PROPVARIANT object_id = {0};
+    PropVariantInit(&object_id);
+    hr = storage_obj_ids->GetAt(index, &object_id);
+    if (SUCCEEDED(hr) && (object_id.pwszVal != NULL) &&
+        (object_id.vt == VT_LPWSTR)) {
+      storage_ids->push_back(object_id.pwszVal);
+    }
+    PropVariantClear(&object_id);
+  }
+  return true;
+}
+
+// Returns true if the portable device is mounted on a volume. |device_name|
+// specifies the name of the device.
+bool IsVolumeMountedPortableDevice(const string16& device_name) {
+  // If the device is a volume mounted device, |device_name| will be
+  // the volume name.
+  return (device_name.length() >= 2) && (device_name[1] == L':') &&
+      ((device_name[0] >= L'A') && (device_name[0] <= L'Z') ||

[Peter Kasting, 2012/10/26 23:46:23]

Nit: Should be indented 4.

Also, add parens: right now you have "a && b || (c && d)", presumably you want
"(a && b) || (c && d)"

[kmadhusu, 2012/10/28 22:57:16]

I thought we align the conditions. I am not sure about your comment. Line #299
already has 4 spaces.
Done.

[Peter Kasting, 2012/10/29 20:41:33]

Huh, somehow in the "diff against prior CL" view it appears to have a different
number of spaces.  Weird.  Ignore that then.

+      ((device_name[0] >= L'a') && (device_name[0] <= L'z')));
+}
+
+// Returns the name of the device specified by |pnp_device_id|.
+// Since this task may take a longer time to complete, this function is
+// accessed via the media task runner.
+string16 GetDeviceNameOnBlockingThread(
+    IPortableDeviceManager* portable_device_manager,
+    const string16& pnp_device_id) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+  DCHECK(portable_device_manager);
+  string16 name;
+  GetFriendlyName(pnp_device_id, portable_device_manager, &name) ||
+      GetDeviceDescription(pnp_device_id, portable_device_manager, &name) ||
+      GetManufacturerName(pnp_device_id, portable_device_manager, &name);
+  return name;
+}
+
+// Access the device and gets the device storage details. Since this task may
+// take a longer time to complete, this function is accessed via the media task
+// runner. On success, returns true and populates |storage_objects| with device

[Peter Kasting, 2012/10/26 23:46:23]

Nit: You added comments to a number of functions saying "this task may take a
long time to complete, so access via task runner" or similar.  That's not what I
meant to happen -- it's already obvious that's the case since you're directly
DCHECKing that you're being run in the blocking pool.

Instead, I was looking for comments on the particular Windows API calls that
take a long time.  For example:

// This function calls XYZApi(), which checks for attached file systems.  If any
of these are on network drives, Windows may take an arbitrarily long time to
respond.  Additionally, the call to ABCApi(), while not an arbitrary long call,
can take up to 250 ms to return.  We can't block the UI thread in either of
these cases, so we segregate this functionality and run it via the task runner.

[kmadhusu, 2012/10/28 22:57:16]

I misunderstood your earlier comment. 

How about this?
"Media devices may respond slowly to the windows portable device API request.
These requests may take an arbitrarily long time to complete. Therefore,
segregate this functionality and run it via sequenced task runner instead of
blocking the UI thread".

Once we finalize the text, I will update all the function comments.

[Peter Kasting, 2012/10/29 20:41:33]

I don't want a block of text that's pasted into several function comments. 
That's worse than no comment at all.

The idea is to be specific about precisely which call(s) can take a long time,
ideally with supporting documentation like MSDN articles that note the
blocking/responsiveness characteristics of certain APIs.  This way someone
refactoring in the future can understand how to change the code if they
move/add/delete API calls.

The sort of comment you're suggesting here is vague.  It basically just says
"things might be slow", which is already obvious from the way you wrote the
code.

[kmadhusu, 2012/10/30 01:29:24]

Removed the redundant (on why I am running these tasks on an blocking thread
instead of using UI thread) comment from all the function headers. While I was
testing with a very old Kodak camera, I noticed that it takes > 100ms to get the
device properties. Whereas, if I use a Nexus 7, it is pretty fast. So, the time
taken to complete these requests depends on the device capability. I didn't find
any MSDN article to backup my statement. That is the reason, I cannot provide
any numerical values regarding the delay.

I think anyone who is reading this code, can understand that we are
communicating with the device to get certain information. Therefore, it is
better to do these tasks on a blocking thread instead of an UI thread.

Added a file level comment regarding this.

+// storage details.
+bool GetDeviceStorageObjectsOnBlockingThread(
+    const string16& pnp_device_id,
+    PortableDeviceWatcherWin::StorageObjects* storage_objects) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+  DCHECK(storage_objects);
+  base::win::ScopedComPtr<IPortableDevice> device;
+  if (!SetUp(pnp_device_id, &device))
+    return false;
+
+  string16 device_serial_num;
+  if (!GetObjectUniqueId(device.get(), WPD_DEVICE_OBJECT_ID,
+                         &device_serial_num)) {
+    return false;
+  }
+
+  std::vector<string16> storage_obj_ids;
+  if (!GetRemovableStorageObjectIds(device.get(), &storage_obj_ids))
+    return false;
+
+  for (std::vector<string16>::const_iterator id_iter = storage_obj_ids.begin();

[Peter Kasting, 2012/10/26 23:46:23]

Nit: You are inconsistent across this file about how you name your iterators:
xyz_iter, iter, or it.  Be consistent.

The rest of Chrome code tends to use "i" or "it".  I personally prefer the
former.

[kmadhusu, 2012/10/28 22:57:16]

"i" or "it" does not provide any information about the type of the iterator. I
have to look at the declaration to find that information. I am going to follow
"xyz_iter" pattern in this file. 

+       id_iter != storage_obj_ids.end(); ++id_iter) {
+    string16 storage_persistent_id;
+    if (!GetObjectUniqueId(device.get(), *id_iter, &storage_persistent_id))
+      continue;
+
+    std::string device_storage_id;
+    if (ConstructDeviceStorageUniqueId(device_serial_num, storage_persistent_id,
+                                       &device_storage_id)) {
+      PortableDeviceWatcherWin::DeviceStorageObject storage_object;

[Peter Kasting, 2012/10/26 23:46:23]

Nit: Use aggregate initialization form or add a constructor for this struct.

[kmadhusu, 2012/10/28 22:57:16]

Added a constructor.

+      storage_object.object_temporary_id = *id_iter;
+      storage_object.object_persistent_id = device_storage_id;
+      storage_objects->push_back(storage_object);
+    }
+  }
+  return true;
+}
+
+// Access the device and gets the device details (name, storage info, etc.,).

[Peter Kasting, 2012/10/26 23:46:23]

Nit: "Access" -> "Accesses", "etc.," -> "etc."

[kmadhusu, 2012/10/28 22:57:16]

Done.

+// Since this task may take a longer time to complete, this function is
+// accessed via the media task runner. On success returns true and fills in
+// |device_details|. |pnp_device_id| specifies the plug and play device ID
+// string.
+bool GetDeviceInfoOnBlockingThread(
+    IPortableDeviceManager* portable_device_manager,
+    const string16& pnp_device_id,
+    PortableDeviceWatcherWin::DeviceDetails* device_details) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+  DCHECK(portable_device_manager);
+  DCHECK(device_details);
+  DCHECK(!pnp_device_id.empty());
+
+  device_details->name = GetDeviceNameOnBlockingThread(portable_device_manager,
+                                                       pnp_device_id);
+  if (IsVolumeMountedPortableDevice(device_details->name))
+    return false;
+
+  device_details->location = pnp_device_id;
+  PortableDeviceWatcherWin::StorageObjects storage_objects;
+  return GetDeviceStorageObjectsOnBlockingThread(
+      pnp_device_id, &device_details->storage_objects);
+}
+
+// Wrapper function to get an instance of portable device manager. This function
+// may take a long time to complete. Therefore, access this function via a media
+// task runner. On success, returns true and fills in |portable_device_mgr|.
+bool GetPortableDeviceManager(
+  base::win::ScopedComPtr<IPortableDeviceManager>* portable_device_mgr) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+  HRESULT hr = portable_device_mgr->CreateInstance(
+      __uuidof(PortableDeviceManager), NULL, CLSCTX_INPROC_SERVER);
+  if (SUCCEEDED(hr))
+    return true;
+
+  // Either there is no portable device support (it must be a XP with Windows

[Peter Kasting, 2012/10/26 23:46:23]

Nit: "it must be a XP with Windows Media Player Version < 10" -> "i.e. Windows
XP with old versions of Media Player"

[kmadhusu, 2012/10/28 22:57:16]

Done.

+  // Media Player Version < 10) or the thread does not have COM initialized.
+  DCHECK_NE(CO_E_NOTINITIALIZED, hr);
+  return false;
+}
+
+// Enumerates the attached portable devices. This function accesses the device
+// to get the device details. Since this task may take a longer time to
+// complete, this function is accessed via the media task runner. On success,
+// fills in |device_details_vector| with the attached portable device details.
+void EnumerateAttachedDevicesOnBlockingThread(
+    PortableDeviceWatcherWin::DeviceDetailsVector* device_details_vector) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+  DCHECK(device_details_vector);
+  base::win::ScopedComPtr<IPortableDeviceManager> portable_device_mgr;
+  if (!GetPortableDeviceManager(&portable_device_mgr))
+    return;
+
+  // Get the total number of devices found on the system.
+  DWORD pnp_device_count = 0;
+  HRESULT hr = portable_device_mgr->GetDevices(NULL, &pnp_device_count);
+  if (FAILED(hr))
+    return;
+
+  scoped_array<char16*> pnp_device_ids(new char16*[pnp_device_count]);
+  ZeroMemory(pnp_device_ids.get(), pnp_device_count);
+  hr = portable_device_mgr->GetDevices(pnp_device_ids.get(), &pnp_device_count);
+  if (FAILED(hr))
+    return;
+
+  for (DWORD index = 0; index < pnp_device_count; ++index) {
+    PortableDeviceWatcherWin::DeviceDetails device_details;
+    if (!GetDeviceInfoOnBlockingThread(portable_device_mgr,
+                                       pnp_device_ids[index],
+                                       &device_details)) {
+      continue;
+    }
+    device_details_vector->push_back(device_details);
+  }
+}
+
+// Handles the device attach event message on a media task runner. This
+// function communicates with the device to get the storage details. This task
+// may take a longer time. Therefore, access this function on a media task
+// runner. |pnp_device_id| specifies the attached plug and play device ID
+// string. On success, populates |device_details| with device information.
+void HandleDeviceAttachedEventOnBlockingThread(
+    const string16& pnp_device_id,
+    PortableDeviceWatcherWin::DeviceDetails* device_details) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+  DCHECK(device_details);
+  base::win::ScopedComPtr<IPortableDeviceManager> portable_device_mgr;
+  if (!GetPortableDeviceManager(&portable_device_mgr))
+    return;
+  // Sometimes, portable device manager doesn't have the new device details.
+  // Refresh the manager device list to update its details.
+  portable_device_mgr->RefreshDeviceList();
+  GetDeviceInfoOnBlockingThread(portable_device_mgr, pnp_device_id,
+                                device_details);
+}
+
+// Constructs and returns a storage path from storage unique identifier.
+string16 GetStoragePathFromStorageId(const std::string& storage_unique_id) {
+  // Construct a dummy device path using the storage name. This is only used
+  // for registering the device media file system.
+  DCHECK(!storage_unique_id.empty());
+  const std::string root_path("\\\\");

[Peter Kasting, 2012/10/26 23:46:23]

Nit: Frankly I think this should just be inlined into the next statement.

[kmadhusu, 2012/10/28 22:57:16]

Done.

+  return UTF8ToUTF16(root_path + storage_unique_id);
+}
+
+}  // namespace
+
+
+///////////////////////////////////////////////////////////////////////////////
+//             PortableDeviceWatcherWin implementation                       //
+///////////////////////////////////////////////////////////////////////////////

[Peter Kasting, 2012/10/26 23:46:23]

Nit: More common is the following:

...
}  // namespace


// PortableDeviceWatcherWin ---------------------------------------------------

PortableDeviceWatcherWin::PortableDeviceWatcherWin()
...

[kmadhusu, 2012/10/28 22:57:16]

Done.

+PortableDeviceWatcherWin::PortableDeviceWatcherWin()
+    : ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
+}
+
+PortableDeviceWatcherWin::~PortableDeviceWatcherWin() {
+}
+
+void PortableDeviceWatcherWin::Init() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  base::SequencedWorkerPool* pool = content::BrowserThread::GetBlockingPool();
+  base::SequencedWorkerPool::SequenceToken media_sequence_token =
+      pool->GetNamedSequenceToken(kMediaTaskRunnerName);
+  media_task_runner_ = pool->GetSequencedTaskRunnerWithShutdownBehavior(
+      media_sequence_token, base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
+  EnumerateAttachedDevices();
+}
+
+void PortableDeviceWatcherWin::OnWindowMessage(UINT event_type, LPARAM data) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  if (!IsPortableDeviceStructure(data))
+    return;
+
+  string16 device_id = GetPnpDeviceId(data);
+  if (event_type == DBT_DEVICEARRIVAL)
+    HandleDeviceAttachEvent(device_id);
+  else if (event_type == DBT_DEVICEREMOVECOMPLETE)
+    HandleDeviceDetachEvent(device_id);
+}
+
+void PortableDeviceWatcherWin::EnumerateAttachedDevices() {
+  DCHECK(media_task_runner_.get());
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  DeviceDetailsVector* device_details_vector = new DeviceDetailsVector;
+  media_task_runner_->PostTaskAndReply(
+      FROM_HERE,
+      base::Bind(&EnumerateAttachedDevicesOnBlockingThread,
+                 device_details_vector),
+      base::Bind(&PortableDeviceWatcherWin::OnDidEnumerateAttachedDevices,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 base::Owned(device_details_vector)));
+}
+
+void PortableDeviceWatcherWin::OnDidEnumerateAttachedDevices(
+    const DeviceDetailsVector* device_details_vector) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  DCHECK(device_details_vector);
+  for (DeviceDetailsVector::const_iterator it = device_details_vector->begin();
+       it != device_details_vector->end(); ++it) {

[Peter Kasting, 2012/10/26 23:46:23]

Nit: {} unnecessary

[kmadhusu, 2012/10/28 22:57:16]

If the condition spans over a line, I am adding {}. Just to be consistent, I am
leaving this as it is.

+    OnDidHandleDeviceAttachEvent(&(*it));
+  }
+}
+
+void PortableDeviceWatcherWin::HandleDeviceAttachEvent(
+    const string16& pnp_device_id) {
+  DCHECK(media_task_runner_.get());
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  DeviceDetails* device_details = new DeviceDetails;
+  media_task_runner_->PostTaskAndReply(
+      FROM_HERE,
+      base::Bind(&HandleDeviceAttachedEventOnBlockingThread, pnp_device_id,
+                 device_details),
+      base::Bind(&PortableDeviceWatcherWin::OnDidHandleDeviceAttachEvent,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 base::Owned(device_details)));
+}
+
+void PortableDeviceWatcherWin::OnDidHandleDeviceAttachEvent(
+    const DeviceDetails* device_details) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  DCHECK(device_details);
+

[Peter Kasting, 2012/10/26 23:46:23]

HandleDeviceAttachedEvent() can fail and leave |device_details| empty.  This
function needs to detect this and no-op, either by checking for an empty struct
here or by checking a return code from the other function (which might be
clearer).

[kmadhusu, 2012/10/28 22:57:16]

Modified code to check the return value of HandleDeviceAttachEvent().

+  const StorageObjects& storage_objects = device_details->storage_objects;
+  const string16& name = device_details->name;
+  const string16& location = device_details->location;

[Peter Kasting, 2012/10/26 23:46:23]

Nit: If any of these refs doesn't end up saving lines overall, I'd remove it and
just deref things directly where used

[kmadhusu, 2012/10/28 22:57:16]

I would like to leave it as it is.

+  DCHECK(!ContainsKey(device_map_, location));
+
+  base::SystemMonitor* system_monitor = base::SystemMonitor::Get();
+  DCHECK(system_monitor);
+  for (StorageObjects::const_iterator storage_iter = storage_objects.begin();
+       storage_iter != storage_objects.end(); ++storage_iter) {
+    const std::string& storage_id = storage_iter->object_persistent_id;
+    DCHECK(!ContainsKey(storage_map_, storage_id));
+
+    // Keep track of storage id and storage name to see how often we receive
+    // empty values.
+    MediaStorageUtil::RecordDeviceInfoHistogram(false, storage_id, name);
+    if (storage_id.empty() || name.empty())
+      return;
+
+    // Device can have several data partitions. Therefore, add the
+    // partition identifier to the storage name. E.g.: "Nexus 7 (s10001)"
+    string16 storage_name(name + L" (" + storage_iter->object_temporary_id +
+                          L')');

[Peter Kasting, 2012/10/26 23:46:23]

Nit: Indent 4

[kmadhusu, 2012/10/28 22:57:16]

Done.

+    storage_map_[storage_id] =
+        base::SystemMonitor::RemovableStorageInfo(storage_id, storage_name,
+            location);

[Peter Kasting, 2012/10/26 23:46:23]

Nit: Arg must be aligned with first arg above

[kmadhusu, 2012/10/28 22:57:16]

Done.

+    system_monitor->ProcessRemovableStorageAttached(
+        storage_id, storage_name, GetStoragePathFromStorageId(storage_id));
+  }
+  device_map_[location] = storage_objects;
+}
+
+void PortableDeviceWatcherWin::HandleDeviceDetachEvent(
+    const string16& pnp_device_id) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  MtpDeviceMap::iterator device_entry = device_map_.find(pnp_device_id);
+  if (device_entry == device_map_.end())
+    return;
+
+  base::SystemMonitor* system_monitor = base::SystemMonitor::Get();
+  DCHECK(system_monitor);
+
+  StorageObjects storage_objects = device_entry->second;

[Peter Kasting, 2012/10/26 23:46:23]

Nit: Does the loop below modify or invalidate what |device_entry| points to?  If
not, use a const reference or refer to device_entry->second explicitly below. 
If so, comment here what precisely makes a copy necessary.

[kmadhusu, 2012/10/28 22:57:16]

Loop does not modify |device_entry|->second value. Instead of copying, created a
reference variable to point to |device_entry|->second value.

+  for (StorageObjects::const_iterator storage_iter = storage_objects.begin();
+       storage_iter != storage_objects.end(); ++storage_iter) {
+    std::string storage_id = storage_iter->object_persistent_id;
+    MtpStorageMap::iterator storage_entry = storage_map_.find(storage_id);
+    DCHECK(storage_entry != storage_map_.end());
+    system_monitor->ProcessRemovableStorageDetached(
+        storage_entry->second.device_id);
+    storage_map_.erase(storage_entry);
+  }
+  device_map_.erase(device_entry);
+}
+
+}  // namespace chrome