[Media Gallery] Added code to support mtp device media file system on Windows.

chrome/browser/media_gallery/win/mtp_device_operations_util.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.
+//
+// MTPDeviceOperationsUtil implementation.
+
+#include "chrome/browser/media_gallery/win/mtp_device_operations_util.h"
+
+#include <portabledevice.h>
+
+#include "base/logging.h"
+#include "base/string_util.h"
+#include "base/threading/sequenced_worker_pool.h"
+#include "base/time.h"
+#include "base/win/scoped_co_mem.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";

[Peter Kasting, 2012/11/01 21:38:37]

This is a bit unusual.  I assume it doesn't matter what name you supply, but
normally we don't refer to strings like "Chromium" ever, and instead use
kBrowserProcessExecutableName or something (which, on Windows, will always be
"chrome.exe").

[kmadhusu, 2012/11/02 03:27:16]

Fixed. I didn't know about this constant. It doesn't matter what name we set in
the client information.

+
+// 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) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());

[Peter Kasting, 2012/11/01 21:38:37]

Seems like at least some of these utilities won't be slow and thus don't really
need to DCHECK that they're on the blocking thread (even if they practically
will always be used there).

I'm not 100% sure about this one; a number of the others below e.g.
IsDirectory() or GetObjectExtension() don't seem like they'd be slow either?

[kmadhusu, 2012/11/02 03:27:16]

Most of these functions just reads the properties values. Added DCHECKs to
emphasis that they should be called by a function that executes on the blocking
thread. As you mentioned, these functions will be only be called by the member
functions of MTPDeviceOperationsUtil class. So, I am removing the DCHECKs from
the functions that reads the property key values.

+  DCHECK(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;
+}
+
+// Gets the content interface of the portable |device|. On success, returns
+// true and fills in |content| with the given portable |device| content.
+bool GetDeviceContent(IPortableDevice* device,
+                      IPortableDeviceContent** content) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+  DCHECK(device);
+  DCHECK(content);
+  return SUCCEEDED(device->Content(content));
+}
+
+// Gets the portable |device| enumerator interface to enumerate the objects.
+// |parent_id| specifies the parent object identifier. On success, returns
+// true and fills in |enum_object_ids|. On failure, returns false and
+// |enum_object_ids| is not set.
+bool GetDeviceObjectEnumerator(IPortableDevice* device,
+                               const string16& parent_id,
+                               IEnumPortableDeviceObjectIDs** enum_object_ids) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+  DCHECK(device);
+  DCHECK(!parent_id.empty());
+  DCHECK(enum_object_ids);
+  base::win::ScopedComPtr<IPortableDeviceContent> content;
+  if (!GetDeviceContent(device, content.Receive()))
+    return false;
+
+  HRESULT hr = content->EnumObjects(0, parent_id.c_str(), NULL,
+                                    enum_object_ids);
+  return SUCCEEDED(hr);
+}
+
+// Reads data from |stream| into a |data| string.
+void ReadStream(IStream* stream, size_t size, std::string* data) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+  DCHECK(stream);
+  DCHECK_GT(size, 0u);
+  DCHECK(data);
+  DWORD read = 0;
+  HRESULT hr = S_OK;
+  do {
+    std::string buffer;
+    hr = stream->Read(WriteInto(&buffer, size + 1), size, &read);
+    DCHECK(hr == S_OK || hr == S_FALSE || hr == E_PENDING);
+    if (read) {
+      buffer.erase(read);
+      DCHECK_EQ(read, buffer.length());
+      *data += buffer;
+    }
+  } while ((read > 0) && SUCCEEDED(hr));
+}
+
+// Returns true, if the object is a directory/folder/album otherwise returns

[Peter Kasting, 2012/11/01 21:38:37]

Nit: First sentence can be just "Returns whether the object is a
directory/folder/album."

[kmadhusu, 2012/11/02 03:27:16]

Done.

+// false. |properties_values| contains the object property key values.
+bool IsDirectory(IPortableDeviceValues* properties_values) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+  DCHECK(properties_values);
+  GUID content_type;
+  HRESULT hr = properties_values->GetGuidValue(WPD_OBJECT_CONTENT_TYPE,
+                                               &content_type);
+  DCHECK(SUCCEEDED(hr));
+  return ((content_type == WPD_CONTENT_TYPE_AUDIO_ALBUM) ||
+      (content_type == WPD_CONTENT_TYPE_FOLDER) ||
+      (content_type == WPD_CONTENT_TYPE_FUNCTIONAL_OBJECT) ||
+      (content_type == WPD_CONTENT_TYPE_IMAGE_ALBUM) ||
+      (content_type == WPD_CONTENT_TYPE_MIXED_CONTENT_ALBUM) ||
+      (content_type == WPD_CONTENT_TYPE_VIDEO_ALBUM));
+}
+
+// Returns the object extension type based on the object format specified
+// by the |properties_values|. If the object is an unsupport format, this
+// function returns an empty string.
+string16 GetObjectExtension(IPortableDeviceValues* properties_values) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+  DCHECK(properties_values);
+  GUID format;
+  HRESULT hr = properties_values->GetGuidValue(WPD_OBJECT_FORMAT, &format);
+  DCHECK(SUCCEEDED(hr));
+  if (IsEqualGUID(WPD_OBJECT_FORMAT_BMP, format))
+    return  L".bmp";

[Peter Kasting, 2012/11/01 21:38:37]

Nit: Extra space (several places)

[kmadhusu, 2012/11/02 03:27:16]

oops. Fixed.

+  if (IsEqualGUID(WPD_OBJECT_FORMAT_EXIF, format))
+    return  L".jpg";
+  if (IsEqualGUID(WPD_OBJECT_FORMAT_GIF, format))
+    return  L".gif";
+  if (IsEqualGUID(WPD_OBJECT_FORMAT_JFIF, format))
+    return  L".jfif";
+  if (IsEqualGUID(WPD_OBJECT_FORMAT_JP2, format))
+    return  L".jxr";
+  if (IsEqualGUID(WPD_OBJECT_FORMAT_JPEGXR, format))
+    return  L".jpx";
+  if (IsEqualGUID(WPD_OBJECT_FORMAT_JPX, format))
+    return  L".jp2";
+  if (IsEqualGUID(WPD_OBJECT_FORMAT_PICT, format))
+    return  L".pic";
+  if (IsEqualGUID(WPD_OBJECT_FORMAT_PNG, format))
+    return  L".png";
+  return string16();
+}
+
+// Gets the friendly name of the object from the property key values specified
+// by the |properties_values|. On success, returns true and fills in |name|.
+// On failure, returns false and |name| is not set.
+bool GetObjectName(IPortableDeviceValues* properties_values,
+                   bool is_directory,
+                   string16* name) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+  DCHECK(properties_values);
+  DCHECK(name);
+  base::win::ScopedCoMem<char16> buffer;
+  HRESULT hr = properties_values->GetStringValue(WPD_OBJECT_NAME, &buffer);
+  DCHECK(SUCCEEDED(hr));
+  *name = static_cast<const char16*>(buffer);
+  if (name->empty())
+    return false;

[Peter Kasting, 2012/11/01 21:38:37]

Here you wrote |name| and then returned false, which violates the function
contract.

Either fix or else don't guarantee that name will be unchanged on failure.

Even better might be to just return name directly and have callers check whether
it's empty, since that's what you seem to actually be doing in this function: we
return true iff name is non-empty.

[kmadhusu, 2012/11/02 03:27:16]

I would like to return bool. Fixed the comment to say that "On failure, returns
false".

[Peter Kasting, 2012/11/02 04:08:11]

What does returning a bool buy you over just returning the string, other than
more complexity?

And don't say "consistency" because there is no value in one anonymous namespace
function being consistent with another; the only purpose of these is to be
expedient.

[kmadhusu, 2012/11/29 23:58:14]

Your argument convinced me. Returning the name instead of a bool.

+  if (is_directory)
+    return true;
+
+  // Some MTP device objects does not have extension as part of their name.
+  // Check to see if an extension exists in the given object name.
+  if (name->find_first_of(L".") == string16::npos)
+    *name += GetObjectExtension(properties_values);
+  return true;
+}
+
+// Gets the last modified time of the object from the property key values
+// specified by the |properties_values|. On success, returns true and fills in
+// |last_modified_time|. On failure, returns false and |last_modified_time| is
+// not set.
+bool GetLastModifiedTime(IPortableDeviceValues* properties_values,
+                         base::Time* last_modified_time) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+  DCHECK(properties_values);
+  DCHECK(last_modified_time);
+  PROPVARIANT last_modified_date = {0};
+  PropVariantInit(&last_modified_date);
+  HRESULT hr = properties_values->GetValue(WPD_OBJECT_DATE_MODIFIED,
+                                           &last_modified_date);
+  if (FAILED(hr))
+    return false;
+
+  if (SUCCEEDED(hr) && last_modified_date.vt == VT_DATE) {

[Peter Kasting, 2012/11/01 21:38:37]

Nit: SUCCEEDED(hr) is guaranteed true here

[kmadhusu, 2012/11/02 03:27:16]

Removed SUCCEEDED(hr).

+    SYSTEMTIME system_time;
+    FILETIME file_time;
+    if (VariantTimeToSystemTime(last_modified_date.date, &system_time) &&
+        SystemTimeToFileTime(&system_time, &file_time))
+      *last_modified_time = base::Time::FromFileTime(file_time);
+  }
+  PropVariantClear(&last_modified_date);
+  return true;

[Peter Kasting, 2012/11/01 21:38:37]

Doesn't seem like this should return true in cases where last_modified_time was
not set.

[kmadhusu, 2012/11/02 03:27:16]

Some MTP devices does not set the modification time. In those cases, I don't
want to return false. Last modified time is just a file metadata. We should not
block the user from accessing the media files just because the last modified
time is unavailable.

[Peter Kasting, 2012/11/02 04:08:11]

The point is that last_modified_time may be entirely uninitialized.  I've never
seen a setter that returns true and doesn't write the variable it's supposed to
be setting.  If you want to indicate "we successfully calculated that there is
no last modified time" then reset that outparam explicitly.

[kmadhusu, 2012/11/29 23:58:14]

I have a test device (Canon powershot A70 camera) that does not save/set last
modified time details for any media files. When I tried to get
WPD_OBJECT_DATE_MODIFIED of those media files, "properties_values->GetValue"
call succeeds but last_modified_date.vt is set to VT_ERROR.

As you suggested in your earlier comment, I reset the outparam explicitly if
"last_modified_date.vt != VT_DATE".

+}
+
+// Gets the size of the file object in bytes from the property key values
+// specified by the |properties_values|. On success, returns true and fills
+// in |size|. On failure, returns false and |size| is not set.
+bool GetObjectSize(IPortableDeviceValues* properties_values, int64* size) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+  DCHECK(properties_values);
+  DCHECK(size);
+  HRESULT hr = properties_values->GetUnsignedLargeIntegerValue(
+      WPD_OBJECT_SIZE, reinterpret_cast<ULONGLONG*>(size));
+  return SUCCEEDED(hr);
+}
+
+// Gets the details of the object specified by the |object_id| given the media
+// transfer protocol |device|. On success, returns true and fills in |name|,
+// |is_directory|, |size| and |last_modified_time|. On failure, returns false
+// and all the output params are not set.
+bool GetObjectDetails(IPortableDevice* device,
+                      const string16 object_id,
+                      string16* name,
+                      bool* is_directory,
+                      int64* size,
+                      base::Time* last_modified_time) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+  DCHECK(device);
+  DCHECK(!object_id.empty());
+  DCHECK(name);
+  DCHECK(is_directory);
+  DCHECK(size);
+  DCHECK(last_modified_time);
+  base::win::ScopedComPtr<IPortableDeviceContent> content;
+  if (!GetDeviceContent(device, content.Receive()))
+    return false;
+
+  base::win::ScopedComPtr<IPortableDeviceProperties> properties;
+  HRESULT hr = content->Properties(properties.Receive());
+  if (FAILED(hr))
+    return false;
+
+  base::win::ScopedComPtr<IPortableDeviceKeyCollection> properties_to_read;
+  hr = properties_to_read.CreateInstance(__uuidof(PortableDeviceKeyCollection),
+                                         NULL,
+                                         CLSCTX_INPROC_SERVER);
+  if (FAILED(hr))
+    return false;
+
+  if (FAILED(properties_to_read->Add(WPD_OBJECT_CONTENT_TYPE)) ||
+      FAILED(properties_to_read->Add(WPD_OBJECT_FORMAT)) ||
+      FAILED(properties_to_read->Add(WPD_OBJECT_NAME)) ||
+      FAILED(properties_to_read->Add(WPD_OBJECT_DATE_MODIFIED)) ||
+      FAILED(properties_to_read->Add(WPD_OBJECT_SIZE)))
+    return false;
+
+  base::win::ScopedComPtr<IPortableDeviceValues> properties_values;
+  hr = properties->GetValues(object_id.c_str(),
+                             properties_to_read.get(),
+                             properties_values.Receive());
+  if (FAILED(hr))
+    return false;
+
+  *is_directory = IsDirectory(properties_values.get());
+  if (!GetObjectName(properties_values.get(), *is_directory, name))
+    return false;

[Peter Kasting, 2012/11/01 21:38:37]

More cases where you return false after writing to your outparams, in violation
of your documented contract

[kmadhusu, 2012/11/02 03:27:16]

Fixed comment.

+
+  if (*is_directory) {
+    // Directory entry does not have size and last modified date property key
+    // values.
+    *size = 0;
+    *last_modified_time = base::Time();
+    return true;
+  }
+  return (GetObjectSize(properties_values.get(), size) &&
+      GetLastModifiedTime(properties_values.get(), last_modified_time));
+}
+
+// Creates a MTP device object entry for the given |device| and |object_id|.

[Peter Kasting, 2012/11/01 21:38:37]

Nit: a -> an

[kmadhusu, 2012/11/02 03:27:16]

Done.

+// On success, returns true and fills in |entry|. On failure, returns false
+// and |entry| is not set.
+bool GetMTPDeviceObjectEntry(IPortableDevice* device,
+                             const string16& object_id,
+                             MTPDeviceObjectEntry* entry) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+  DCHECK(device);
+  DCHECK(!object_id.empty());
+  DCHECK(entry);
+  string16 name;
+  bool is_directory;
+  int64 size;
+  base::Time last_modified_time;
+  if (!GetObjectDetails(device, object_id, &name, &is_directory, &size,
+                        &last_modified_time))
+    return false;
+  *entry = MTPDeviceObjectEntry(object_id, name, is_directory, size,
+                                last_modified_time);
+  return true;
+}
+
+}  // namespace
+
+
+// MTPDeviceOperationsUtil ----------------------------------------------------
+
+// static
+bool MTPDeviceOperationsUtil::OpenDevice(
+    const string16& pnp_device_id,
+    base::win::ScopedComPtr<IPortableDevice>* device) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+  DCHECK(!pnp_device_id.empty());
+  DCHECK(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))
+    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;
+}
+
+// static
+base::PlatformFileError MTPDeviceOperationsUtil::GetFileEntryInfo(
+    IPortableDevice* device,
+    const string16& object_id,
+    base::PlatformFileInfo* file_entry_info) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+  DCHECK(device);
+  DCHECK(!object_id.empty());
+  DCHECK(file_entry_info);
+  MTPDeviceObjectEntry entry;
+  if (!GetMTPDeviceObjectEntry(device, object_id, &entry))
+    return base::PLATFORM_FILE_ERROR_NOT_FOUND;
+
+  file_entry_info->size = entry.size();
+  file_entry_info->is_directory = entry.is_directory();
+  file_entry_info->is_symbolic_link = false;
+  file_entry_info->last_modified = entry.last_modified_time();
+  file_entry_info->last_accessed = entry.last_modified_time();
+  file_entry_info->creation_time = base::Time();
+  return base::PLATFORM_FILE_OK;
+}
+
+// static
+bool MTPDeviceOperationsUtil::GetDirectoryEntries(
+    IPortableDevice* device,
+    const string16& directory_object_id,
+    MTPDeviceObjectEntries* object_entries) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+  DCHECK(device);
+  DCHECK(!directory_object_id.empty());
+  DCHECK(object_entries);
+  base::win::ScopedComPtr<IEnumPortableDeviceObjectIDs> enum_object_ids;
+  if (!GetDeviceObjectEnumerator(device, directory_object_id,
+                                 enum_object_ids.Receive()))
+    return false;
+
+  // Loop calling Next() while S_OK is being returned.
+  DWORD num_objects_to_request = 10;
+  HRESULT hr = S_OK;
+  while (hr == S_OK) {

[Peter Kasting, 2012/11/01 21:38:37]

Nit: Or

  for (HRESULT hr = S_OK; hr == S_OK; ) {

[kmadhusu, 2012/11/02 03:27:16]

Done.

+    DWORD  num_objects_fetched = 0;

[Peter Kasting, 2012/11/01 21:38:37]

Nit: extra space

[kmadhusu, 2012/11/02 03:27:16]

Done.

+    scoped_array<char16*> object_ids(new char16*[num_objects_to_request]);
+    hr = enum_object_ids->Next(num_objects_to_request, object_ids.get(),
+                               &num_objects_fetched);
+    for (DWORD index = 0; index < num_objects_fetched; index++) {

[Peter Kasting, 2012/11/01 21:38:37]

Nit: Prefer preincrement whenever possible

[kmadhusu, 2012/11/02 03:27:16]

Done.

+      MTPDeviceObjectEntry entry;
+      if (GetMTPDeviceObjectEntry(device, object_ids[index], &entry))
+        object_entries->push_back(entry);
+    }
+  }
+  return true;
+}
+
+// static
+bool MTPDeviceOperationsUtil::GetFileObjectData(IPortableDevice* device,
+                                                const string16& file_object_id,
+                                                std::string* file_data) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+  DCHECK(device);
+  DCHECK(!file_object_id.empty());
+  DCHECK(file_data);
+  base::win::ScopedComPtr<IPortableDeviceContent> content;
+  if (!GetDeviceContent(device, content.Receive()))
+    return false;
+
+  base::win::ScopedComPtr<IPortableDeviceResources> resources;
+  HRESULT hr = content->Transfer(resources.Receive());
+  if (FAILED(hr))
+    return false;
+
+  base::win::ScopedComPtr<IStream> file_stream;
+  DWORD optimal_transfer_size = 0;
+  hr = resources->GetStream(file_object_id.c_str(), WPD_RESOURCE_DEFAULT,
+                            STGM_READ, &optimal_transfer_size,
+                            file_stream.Receive());
+  if (FAILED(hr))
+    return false;
+  ReadStream(file_stream.get(), optimal_transfer_size, file_data);
+  return true;
+}
+
+// static
+string16 MTPDeviceOperationsUtil::GetObjectIdFromName(
+    IPortableDevice* device,
+    const string16& parent_id,
+    const string16& object_name) {

[Peter Kasting, 2012/11/01 21:38:37]

This function seems very similar to GetDirectoryEntries(), can we partly or
fully combine these somehow?  Seems like only the core action in the loop (and
thus the return type) differ.

[kmadhusu, 2012/11/02 03:27:16]

Done. Created a helper function "GetMTPDeviceObjectEntries" with a boolean input
param |get_all_entries|.

+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+  DCHECK(device);
+  DCHECK(!parent_id.empty());
+  DCHECK(!object_name.empty());
+  base::win::ScopedComPtr<IEnumPortableDeviceObjectIDs> enum_object_ids;
+  if (!GetDeviceObjectEnumerator(device, parent_id, enum_object_ids.Receive()))
+    return string16();
+
+  DWORD num_objects_to_request = 10;
+  HRESULT hr = S_OK;
+  while (hr == S_OK) {
+    DWORD  num_objects_fetched = 0;
+    scoped_array<char16*> object_ids(new char16*[num_objects_to_request]);
+    hr = enum_object_ids->Next(num_objects_to_request, object_ids.get(),
+                               &num_objects_fetched);
+    for (DWORD index = 0; index < num_objects_fetched; index++) {
+      MTPDeviceObjectEntry entry;
+      if (!GetMTPDeviceObjectEntry(device, object_ids[index], &entry))
+        return string16();
+      if (entry.file_name() == object_name)
+        return entry.object_id();
+    }
+  }
+  return string16();
+}
+
+MTPDeviceOperationsUtil::MTPDeviceOperationsUtil() {
+}
+
+}  // namespace chrome