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

chrome/browser/system_monitor/portable_device_watcher_win.cc

Index: chrome/browser/system_monitor/portable_device_watcher_win.cc
diff --git a/chrome/browser/system_monitor/portable_device_watcher_win.cc b/chrome/browser/system_monitor/portable_device_watcher_win.cc
new file mode 100644
index 0000000000000000000000000000000000000000..96d6cc42feed346176bdccf7c86dd1281c114130
--- /dev/null
+++ b/chrome/browser/system_monitor/portable_device_watcher_win.cc
@@ -0,0 +1,640 @@
+// 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/stringprintf.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 "chrome/common/chrome_notification_types.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/notification_service.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();
+  DCHECK(IsStringASCII(dev_interface->dbcc_name));
+  return StringToLowerASCII(string16(dev_interface->dbcc_name));

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Explicit conversion to string16 unnecessary, I think

[kmadhusu, 2012/10/26 02:01:24]

If I don't have string16, compiler complaints that it cannot convert parameter 1
from void to const std::basic_string.

[Peter Kasting, 2012/10/26 02:14:49]

From void?  That doesn't make sense, you can't convert void to a string anyway
even with an explicit call.

[kmadhusu, 2012/10/26 22:23:26]

StringToLowerASCII template will not work with w_char* objects. It can only work
with string objects. So, I have to construct a string16 object to use
StringToLowerASCII. IsStringASCII function was implicitly converting the
device_id to std::wstring. Therefore, I constructed a local variable and used it
in both function calls.

+}
+
+// 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) {

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Add DCHECK(device_manager); to make it clear this function expects that. 
(several functions)

[kmadhusu, 2012/10/26 02:01:24]

Done.

+  DWORD name_len = 0;
+  HRESULT hr = device_manager->GetDeviceFriendlyName(pnp_device_id.c_str(),
+                                                     NULL, &name_len);
+  if (FAILED(hr))
+    return false;
+
+  string16 friendly_name;
+  hr = device_manager->GetDeviceFriendlyName(
+      pnp_device_id.c_str(), WriteInto(&friendly_name, name_len), &name_len);
+  if (FAILED(hr))
+    return false;
+
+  if (name)

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Is this ever NULL?  If not, DCHECK it atop the function, and use it
directly in place of |friendly_name|.  (similar comments apply in several
functions)

[kmadhusu, 2012/10/26 02:01:24]

As of now, this function is always called with a valid output param. I thought,
I can use this function just to query if a name is available or not. But I
guess, I don't need that function format now. As you suggested, added DCHECK and
used the name param directly.

+    *name = friendly_name;
+  return !friendly_name.empty();
+}
+
+// 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) {
+  DWORD name_len = 0;
+  HRESULT hr = device_manager->GetDeviceManufacturer(pnp_device_id.c_str(),
+                                                     NULL, &name_len);
+  if (FAILED(hr))
+    return false;
+
+  string16 manufacturer_name;
+  hr = device_manager->GetDeviceManufacturer(
+      pnp_device_id.c_str(), WriteInto(&manufacturer_name, name_len),
+      &name_len);
+  if (FAILED(hr))
+    return false;
+
+  if (name)
+    *name = manufacturer_name;
+  return !manufacturer_name.empty();
+}
+
+// Gets the description of the device specified by the |pnp_device_id|. On
+// success, returns true and fills in |name|.
+bool GetDeviceDescription(const string16& pnp_device_id,
+                          IPortableDeviceManager* device_manager,
+                          string16* name) {
+  DWORD desc_len = 0;
+  HRESULT hr = device_manager->GetDeviceDescription(pnp_device_id.c_str(), NULL,
+                                                    &desc_len);
+  if (FAILED(hr))
+    return false;
+
+  string16 description;
+  hr = device_manager->GetDeviceDescription(pnp_device_id.c_str(),
+                                            WriteInto(&description, desc_len),
+                                            &desc_len);
+  if (FAILED(hr))
+    return false;
+  if (name)
+    *name = description;
+  return !description.empty();
+}
+
+// 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);
+

[Peter Kasting, 2012/10/25 05:23:24]

Nit: I'd remove all these blank lines

[kmadhusu, 2012/10/26 02:01:24]

Done.

+  (*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 (FAILED(hr)) {

[Peter Kasting, 2012/10/25 05:23:24]

Nit: If you early-return for SUCCEEDED(hr) instead, you can omit the brackets.

[kmadhusu, 2012/10/26 02:01:24]

Done.

+    if (hr == E_ACCESSDENIED)
+      DPLOG(ERROR) << "Access denied to open the device";
+    return false;
+  }
+  return true;
+}
+
+// Returns true if the object specified by the |object_id| is the device root
+// object.
+bool IsDeviceObjectId(const string16& object_id) {
+  return (object_id == WPD_DEVICE_OBJECT_ID);

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Seems like this should just be inlined into the lone caller.

[kmadhusu, 2012/10/26 02:01:24]

Done.

+}
+
+// Returns the unique id property key of the object specified by the
+// |object_id|.
+REFPROPERTYKEY GetUniqueIdPropertyKey(const string16& object_id) {
+  return IsDeviceObjectId(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) {
+  base::win::ScopedCoMem<char16> buffer;
+  HRESULT hr = properties_values->GetStringValue(key, &buffer);
+  if (FAILED(hr))
+    return false;
+
+  if (value)
+    *value = string16(buffer);

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Explicit conversion to string16 may not be necessary (although you may have
to add a .get() call or something for that to compile)

[kmadhusu, 2012/10/26 02:01:24]

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) {
+  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 GetDeviceStorageUniqueId(const string16& device_serial_num,
+                              const string16& storage_id,
+                              std::string* device_storage_id) {
+  if (device_serial_num.empty() && storage_id.empty())
+    return false;
+
+  // |unique_id| format: StorageSerial:|storage_id|:|device_serial_num|
+  string16 unique_id = base::StringPrintf(L"%ls%ls%ls%ls",
+                                          chrome::kMtpDeviceStorageIdPrefix,
+                                          storage_id.c_str(),
+                                          chrome::kNonSpaceDelim,
+                                          device_serial_num.c_str());
+
+  // |device_storage_id| format: mtp:|unique_id|
+  *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) {
+  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/25 05:23:24]

Nit: Indent 4, not even

[kmadhusu, 2012/10/26 02:01:24]

Done.

+         ((device_name[0] >= L'a') && (device_name[0] <= L'z')));
+}
+
+// Returns the name of the device specified by |pnp_device_id|.
+// Accessed on the blocking thread.

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Comment as to why this (and the other functions that run via the task
runner) runs via the task runner, i.e. which call(s) may be slow and why.

You may also want to say "accessed via the task runner" in these function
comments instead of "blocking thread" since "blocking thread" is slightly
unclear.

(several functions)

[kmadhusu, 2012/10/26 02:01:24]

Done.

+string16 GetDeviceNameOnBlockingThread(
+    IPortableDeviceManager* portable_device_manager,
+    const string16& pnp_device_id) {
+  DCHECK(content::BrowserThread::
+      GetBlockingPool()->RunsTasksOnCurrentThread());

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Breaking after -> seems slightly better than ::  (several functions)

[kmadhusu, 2012/10/26 02:01:24]

Done.

+  string16 name;
+  if (!(GetFriendlyName(pnp_device_id, portable_device_manager, &name) ||

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Can this just be:

  GetFriendlyName(...) || GetDeviceDescription(...) || GetManufacturerName(...);
  return name;

Because if none of these set |name| on failure, we don't need to explicitly
return anything different...

[kmadhusu, 2012/10/26 02:01:24]

Done.

+        GetDeviceDescription(pnp_device_id, portable_device_manager,
+                             &name) ||
+        GetManufacturerName(pnp_device_id, portable_device_manager,
+                            &name))) {
+    return string16();
+  }
+  return name;
+}
+
+// Gets the device storage details on the blocking thread. On success, returns
+// true and populates |storage_info_list| with device storage details.
+bool GetDeviceStorageInfoListOnBlockingThread(
+    const string16& pnp_device_id,
+    std::vector<PortableDeviceWatcherWin::DeviceStorageInfo>*
+        storage_info_list) {
+  DCHECK(content::BrowserThread::
+      GetBlockingPool()->RunsTasksOnCurrentThread());
+  base::win::ScopedComPtr<IPortableDevice> device;
+  if (!Setup(pnp_device_id, &device))
+    return false;
+
+  std::vector<string16> storage_obj_ids;
+  if (!GetRemovableStorageObjectIds(device.get(), &storage_obj_ids))
+    return false;
+
+  // Get the device serial number (E.g.: 4889033500677371).
+  string16 device_serial_num;
+  if (!GetObjectUniqueId(device.get(), WPD_DEVICE_OBJECT_ID,
+                         &device_serial_num)) {
+    return false;
+  }
+
+  for (size_t index = 0; index < storage_obj_ids.size(); ++index) {

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Consider using a const_iterator instead

[kmadhusu, 2012/10/26 02:01:24]

Done.

+    // Get the storage object persistent id (E.g.: SID-{10001,D,31080448}).
+    string16 storage_unique_id;
+    if (!GetObjectUniqueId(device.get(), storage_obj_ids[index],
+                           &storage_unique_id)) {
+      continue;
+    }
+    std::string device_storage_id;
+    if (GetDeviceStorageUniqueId(device_serial_num, storage_unique_id,
+                                 &device_storage_id)) {
+      PortableDeviceWatcherWin::DeviceStorageInfo storage_info;
+      storage_info.storage_object_id = storage_obj_ids[index];
+      storage_info.unique_id = device_storage_id;
+      storage_info_list->push_back(storage_info);
+    }
+  }
+  return true;
+}
+
+// Gets the device details (name, number of storages, etc.,) on the blocking

[Peter Kasting, 2012/10/25 05:23:24]

Nit: storages

[kmadhusu, 2012/10/26 02:01:24]

Fixed.

+// thread. |pnp_device_id| specifies the plug and play device ID string.
+PortableDeviceWatcherWin::DeviceDetails GetDeviceInfoOnBlockingThread(
+    IPortableDeviceManager* portable_device_manager,
+    const string16& pnp_device_id) {
+  DCHECK(content::BrowserThread::
+      GetBlockingPool()->RunsTasksOnCurrentThread());
+  DCHECK(!pnp_device_id.empty());
+  PortableDeviceWatcherWin::DeviceDetails device_details;
+  string16 device_name = GetDeviceNameOnBlockingThread(portable_device_manager,
+                                                       pnp_device_id);
+  std::vector<PortableDeviceWatcherWin::DeviceStorageInfo> storage_info_list;
+  if (!GetDeviceStorageInfoListOnBlockingThread(pnp_device_id,
+                                                &storage_info_list)) {
+    return device_details;
+  }
+
+  if (IsVolumeMountedPortableDevice(device_name))
+    return device_details;
+
+  device_details.storage_info_list = storage_info_list;
+  device_details.name = device_name;
+  device_details.location = pnp_device_id;

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Seems like we could set these directly (instead of using temps) if the
intermediate return points returned a DeviceDetails().

Although maybe this should return a bool or something instead, see below.

[kmadhusu, 2012/10/26 02:01:24]

Done.

+  return device_details;
+}
+
+// Enumerates and returns a list of attached mtp device details on a
+// blocking thread.
+std::vector<PortableDeviceWatcherWin::DeviceDetails>
+EnumerateAttachedStoragesOnBlockingThread() {

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Indent 4.  Also: storages

[kmadhusu, 2012/10/26 02:01:24]

Done.

+  DCHECK(content::BrowserThread::
+      GetBlockingPool()->RunsTasksOnCurrentThread());
+  std::vector<PortableDeviceWatcherWin::DeviceDetails> device_info_list;
+  base::win::ScopedComPtr<IPortableDeviceManager> portable_device_mgr;
+  HRESULT hr = portable_device_mgr.CreateInstance(
+      __uuidof(PortableDeviceManager), NULL, CLSCTX_INPROC_SERVER);
+  if (FAILED(hr)) {
+    // Either there is no portable device support (it must be a XP with
+    // Windows Media Player Version < 10) or the thread does not have COM
+    // initialized.
+    DCHECK_NE(CO_E_NOTINITIALIZED, hr);
+    return device_info_list;
+  }
+
+  // Get the total number of devices found on the system.
+  DWORD pnp_device_count = 0;
+  hr = portable_device_mgr->GetDevices(NULL, &pnp_device_count);
+  if (FAILED(hr))
+    return device_info_list;
+
+  scoped_array<LPWSTR> pnp_device_ids(new LPWSTR[pnp_device_count]);
+  ZeroMemory(pnp_device_ids.get(), pnp_device_count);

[Peter Kasting, 2012/10/25 05:23:24]

Doesn't operator new zero this memory for you?

[kmadhusu, 2012/10/26 02:01:24]

No.

[Peter Kasting, 2012/10/26 02:14:49]

Oh hey, while I'm here, the style guide says to use the C++ types (in this case
wchar_t*) in place of Windows typedefed types like LPWSTR.

[kmadhusu, 2012/10/26 22:23:26]

Done. Used char16* instead of LPWSTR.

+  hr = portable_device_mgr->GetDevices(pnp_device_ids.get(), &pnp_device_count);
+  if (FAILED(hr))
+    return device_info_list;
+
+  for (DWORD index = 0; index < pnp_device_count; ++index) {
+    device_info_list.push_back(GetDeviceInfoOnBlockingThread(

[Peter Kasting, 2012/10/25 05:23:24]

If GetDeviceInfoOnBlockingThread fails, it returns an empty device details,
which we happily shove on this list.  Maybe we should be checking the return
code from that function.  If it took a DeviceDetails* and returned a bool, it
could freely write into the supplied DeviceDetails (avoiding temps, as I
suggested in that function) and return false on failure, at which point we could
discard the details here.

[kmadhusu, 2012/10/26 02:01:24]

Done.

+        portable_device_mgr, pnp_device_ids[index]));
+  }
+ return device_info_list;
+}
+
+// Handles the device attach event message on the blocking thread.
+// |pnp_device_id| specifies the attached plug and play device ID string.
+PortableDeviceWatcherWin::DeviceDetails
+HandleDeviceAttachedEventOnBlockingThread(const string16& pnp_device_id) {

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Indent 4

[kmadhusu, 2012/10/26 02:01:24]

Done.

+  DCHECK(content::BrowserThread::
+      GetBlockingPool()->RunsTasksOnCurrentThread());
+  PortableDeviceWatcherWin::DeviceDetails device_details;
+  base::win::ScopedComPtr<IPortableDeviceManager> portable_device_mgr;

[Peter Kasting, 2012/10/25 05:23:24]

Nit: This whole block that gets the portable device manager is duplicated from
above, maybe should be its own function.

[kmadhusu, 2012/10/26 02:01:24]

Done.

+  HRESULT hr = portable_device_mgr.CreateInstance(
+      __uuidof(PortableDeviceManager), NULL, CLSCTX_INPROC_SERVER);
+  if (FAILED(hr)) {
+    // Either there is no portable device support (it must be a XP with
+    // Windows Media Player Version < 10) or the thread does not have COM
+    // initialized.
+    DCHECK_NE(CO_E_NOTINITIALIZED, hr);
+    return device_details;
+  }
+  portable_device_mgr->RefreshDeviceList();

[Peter Kasting, 2012/10/25 05:23:24]

Do we need to do this call in the function above too?

[kmadhusu, 2012/10/26 02:01:24]

No. It is not required there. When a new device is attached, sometimes portable
device manager is not aware of the new device. Just to be on the safer side, I
added this call here.

[Peter Kasting, 2012/10/26 02:14:49]

Please comment about why this is here, then, especially if you can give specific
circumstances which cause this lack of awareness to happen.

[kmadhusu, 2012/10/26 22:23:26]

Done.

+  return GetDeviceInfoOnBlockingThread(portable_device_mgr, pnp_device_id);
+}
+
+// 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 device media file system.

[Peter Kasting, 2012/10/25 05:23:24]

Nit: device -> the device

[kmadhusu, 2012/10/26 02:01:24]

Done.

+  DCHECK(!storage_unique_id.empty());
+  std::string root_path("\\\\");
+  return UTF8ToUTF16(root_path + storage_unique_id);
+}
+
+}  // namespace
+
+PortableDeviceWatcherWin::PortableDeviceWatcherWin()

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Please add divider comments in this file to separate the file-scoped
functions from the PortableDeviceWatcherWin functions.  See omnibox code for
sample divider use.

[kmadhusu, 2012/10/26 02:01:24]

Done.

+    : ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
+  registrar_.Add(this, chrome::NOTIFICATION_APP_TERMINATING,
+                 content::NotificationService::AllSources());
+}
+
+PortableDeviceWatcherWin::~PortableDeviceWatcherWin() {
+}
+
+void PortableDeviceWatcherWin::Init() {
+  if (app_terminating_flag_.IsSet())
+    return;
+
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

[Peter Kasting, 2012/10/25 05:23:24]

Nit: This DCHECK goes atop this function.

[kmadhusu, 2012/10/26 02:01:24]

Done.

+  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);
+  DCHECK(media_task_runner_.get());

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Do we need this DCHECK?  Is there some reason a reader might think the call
could have failed?

[kmadhusu, 2012/10/26 02:01:24]

Removed. We can skip this check. If it failed for some reason, then we are in
much trouble.

+  EnumerateAttachedDevices();
+}
+
+bool PortableDeviceWatcherWin::GetDeviceInfo(const FilePath& device_path,
+                                             string16* location,
+                                             std::string* unique_id,
+                                             string16* name,
+                                             bool* removable) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  NOTIMPLEMENTED();

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Is this TODO?

[kmadhusu, 2012/10/26 02:01:24]

As of now, there is no UI to trigger this function. I just added it as a place
holder for future use. vandebo@ said, I can add this function when required.
Therefore, removed the function declaration and definition.

+  return false;
+}
+
+void PortableDeviceWatcherWin::OnWindowMessage(UINT event_type, LPARAM data) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  if (app_terminating_flag_.IsSet())
+    return;
+
+  switch (event_type) {

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Simpler:

  if (IsPortableDeviceStructure(data)) {
    string16 device_id(GetPnpDeviceId(data));
    if (event_type == DBT_DEVICEARRIVAL)
      HandleDeviceAttachEvent(device_id)
    else if (event_type == DBT_DEVICEMOVECOMPLETE)
      HandleDeviceDetachEvent(device_id);
  }

You can replace the final if with a DCHECK if these are the only two cases in
which this function is called.

[kmadhusu, 2012/10/26 02:01:24]

Added the following code:
  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);

We can receive other device changed events. So, I didn't add any DCHECK at the
end.

+    case DBT_DEVICEARRIVAL: {
+      if (IsPortableDeviceStructure(data))
+        HandleDeviceAttachEvent(GetPnpDeviceId(data));
+      break;
+    }
+    case DBT_DEVICEREMOVECOMPLETE: {
+      if (IsPortableDeviceStructure(data))
+        HandleDeviceDetachEvent(GetPnpDeviceId(data));
+      break;
+    }
+  }
+}
+
+void PortableDeviceWatcherWin::EnumerateAttachedDevices() {
+  DCHECK(media_task_runner_.get());
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  if (app_terminating_flag_.IsSet())
+    return;
+
+  base::PostTaskAndReplyWithResult(
+      media_task_runner_,
+      FROM_HERE,
+      base::Bind(&EnumerateAttachedStoragesOnBlockingThread),
+      base::Bind(&PortableDeviceWatcherWin::OnDidEnumerateAttachedDevices,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void PortableDeviceWatcherWin::HandleDeviceAttachEvent(
+    const string16& pnp_device_id) {
+  DCHECK(media_task_runner_.get());
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  if (app_terminating_flag_.IsSet())
+    return;
+
+  base::PostTaskAndReplyWithResult(
+      media_task_runner_,
+      FROM_HERE,
+      base::Bind(&HandleDeviceAttachedEventOnBlockingThread, pnp_device_id),
+      base::Bind(&PortableDeviceWatcherWin::OnHandleDeviceAttachEvent,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+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);
+
+  StorageInfoList storage_info_list = device_entry->second;
+  for (size_t index = 0; index < storage_info_list.size(); ++index) {

[Peter Kasting, 2012/10/25 05:23:24]

Nit: For looping over any typedefed type, use a [const_]iterator, which will
still work if you change the typedef to a non-random-access type.  (several
functions)

[kmadhusu, 2012/10/26 02:01:24]

Done.

+    std::string storage_id = storage_info_list[index].unique_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);
+}
+
+void PortableDeviceWatcherWin::OnDidEnumerateAttachedDevices(
+    std::vector<DeviceDetails> device_details_list) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  for (size_t index = 0; index < device_details_list.size(); ++index)
+    OnHandleDeviceAttachEvent(device_details_list[index]);
+}
+
+void PortableDeviceWatcherWin::OnHandleDeviceAttachEvent(

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Maybe this should be named OnDidHandleDeviceAttachEvent()

[kmadhusu, 2012/10/26 02:01:24]

Done.

+    DeviceDetails device_details) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  if (app_terminating_flag_.IsSet())
+    return;
+
+  const StorageInfoList& storage_info_list = device_details.storage_info_list;
+  const string16& name = device_details.name;
+  const string16& location = device_details.location;
+  DCHECK(!ContainsKey(device_map_, location));
+  base::SystemMonitor* system_monitor = base::SystemMonitor::Get();
+  DCHECK(system_monitor);
+  for (size_t index = 0; index < storage_info_list.size(); ++index) {
+    const std::string& storage_id = storage_info_list[index].unique_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 storage_info_list. Therefore, add the

[Peter Kasting, 2012/10/25 05:23:24]

Nit: This first sentence seems to be missing some words

[kmadhusu, 2012/10/26 02:01:24]

Fixed.

+    // partition details to the storage name. E.g.: "Nexus 7 (s10001)"
+    string16 storage_name = base::StringPrintf(
+        L"%ls (%ls)", name.c_str(),
+        storage_info_list[index].storage_object_id.c_str());
+
+    base::SystemMonitor::RemovableStorageInfo storage_info(storage_id,

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Or just inline this into the next statement

[kmadhusu, 2012/10/26 02:01:24]

Done

+                                                           storage_name,
+                                                           location);
+    storage_map_[storage_id] = storage_info;
+    system_monitor->ProcessRemovableStorageAttached(
+        storage_id, storage_name, GetStoragePathFromStorageId(storage_id));
+  }
+  device_map_[location] = storage_info_list;
+}
+
+void PortableDeviceWatcherWin::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  DCHECK_EQ(chrome::NOTIFICATION_APP_TERMINATING, type);
+  app_terminating_flag_.Set();
+}
+
+}  // namespace chrome