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

kmadhusu

pkasting@:Please review windows specific code in mtp_device_delegate_impl_win.cc thestig@: Please review media gallery specific code. Thanks.

8 years, 2 months ago (2012-10-24 22:35:18 UTC) #1

Peter Kasting

There are a ton of classes in the .cc file. At the very least, organize ...

8 years, 2 months ago (2012-10-24 22:51:41 UTC) #2

There are a ton of classes in the .cc file.

At the very least, organize them so all file-scoped functions are in a group and
each class declaration and definition are together, then put dividers between
them (see e.g. the omnibox code for examples), and add much better commenting. 
Example of a bad comment:

// Represents Mtp device object entry.
class MtpObjectEntry

This tells me nothing the class name doesn't tell me.

Your .h file should explain what its class is for to a reader who doesn't even
know what "MTP" stands for.  See the Google style guide for instructions on
file-level and class-level comments.  Say what the class does, how it's used,
etc.  The classes in the .cc file should have just as comprehensive of comments.
 Consider breaking each out into its own file if this increases clarity or
allows you to write better or more granular unittests.

http://codereview.chromium.org/11297002/diff/21001/chrome/browser/media_galle...
File chrome/browser/media_gallery/mtp_device_delegate_impl_win.h (right):

http://codereview.chromium.org/11297002/diff/21001/chrome/browser/media_galle...
chrome/browser/media_gallery/mtp_device_delegate_impl_win.h:70: // Stores the
plug and play device ID string. This is used to open the device
Nit: Member variables should not say "stores a" or "stores the".  (several
places)

http://codereview.chromium.org/11297002/diff/21001/chrome/browser/media_galle...
chrome/browser/media_gallery/mtp_device_delegate_impl_win.h:72: //
(E.g.:\\?\usb#vid_04a9&pid_3073#12#{6ac27878-a6fa-4155-ba85-f98f491d4f33})
Nit: For clarity, remove the parens, add quotes, add whitespace.  Ends up like:

  // The PnP device ID, used to open the device for communication, e.g.
  // "\\?\usb#vid_04a9&pid_3073#12#{6ac27878-a6fa-4155-ba85-f98f491d4f33}".

(Same applies below)

http://codereview.chromium.org/11297002/diff/21001/chrome/browser/media_galle...
chrome/browser/media_gallery/mtp_device_delegate_impl_win.h:77: const string16
registered_dev_path_;
Nit: Do not abbreviate "device" to "dev"

http://codereview.chromium.org/11297002/diff/21001/chrome/browser/media_galle...
chrome/browser/media_gallery/mtp_device_delegate_impl_win.h:87: // portable
device.
Nit: This comment mostly restates the code.  If this is the device the other
comments refer to, this could say something like "The device itself."

kmadhusu

pkasting@: Moved the worker classes from MtpDeviceDelegateImplWin to separate files. Please review windows specific code ...

8 years, 1 month ago (2012-10-25 19:12:48 UTC) #3

pkasting@: Moved the worker classes from MtpDeviceDelegateImplWin to separate
files. Please review windows specific code in
get_mtp_device_object_info_worker_win.cc, mtp_device_delegate_impl_win.cc,
mtp_device_directory_reader_win.cc, mtp_device_object_entry_win.cc,
open_mtp_device_worker_win.cc, read_mtp_device_file_worker_win.cc.

If it helps, I can divide this CL into small CLs for easier review. PTAL.

Thanks.

http://codereview.chromium.org/11297002/diff/21001/chrome/browser/media_galle...
File chrome/browser/media_gallery/mtp_device_delegate_impl_win.h (right):

http://codereview.chromium.org/11297002/diff/21001/chrome/browser/media_galle...
chrome/browser/media_gallery/mtp_device_delegate_impl_win.h:70: // Stores the
plug and play device ID string. This is used to open the device
On 2012/10/24 22:51:41, Peter Kasting wrote:
> Nit: Member variables should not say "stores a" or "stores the".  (several
> places)

Done.

http://codereview.chromium.org/11297002/diff/21001/chrome/browser/media_galle...
chrome/browser/media_gallery/mtp_device_delegate_impl_win.h:72: //
(E.g.:\\?\usb#vid_04a9&pid_3073#12#{6ac27878-a6fa-4155-ba85-f98f491d4f33})
On 2012/10/24 22:51:41, Peter Kasting wrote:
> Nit: For clarity, remove the parens, add quotes, add whitespace.  Ends up
like:
> 
>   // The PnP device ID, used to open the device for communication, e.g.
>   // "\\?\usb#vid_04a9&pid_3073#12#{6ac27878-a6fa-4155-ba85-f98f491d4f33}".
> 
> (Same applies below)

Done.

http://codereview.chromium.org/11297002/diff/21001/chrome/browser/media_galle...
chrome/browser/media_gallery/mtp_device_delegate_impl_win.h:77: const string16
registered_dev_path_;
On 2012/10/24 22:51:41, Peter Kasting wrote:
> Nit: Do not abbreviate "device" to "dev"

Done.

http://codereview.chromium.org/11297002/diff/21001/chrome/browser/media_galle...
chrome/browser/media_gallery/mtp_device_delegate_impl_win.h:87: // portable
device.
On 2012/10/24 22:51:41, Peter Kasting wrote:
> Nit: This comment mostly restates the code.  If this is the device the other
> comments refer to, this could say something like "The device itself."

Done.

Peter Kasting

If it's OK with you I'd like to wait to review this until http://codereview.chromium.org/11088012/ lands ...

8 years, 1 month ago (2012-10-26 22:37:38 UTC) #4

kmadhusu

On 2012/10/26 22:37:38, Peter Kasting wrote: > If it's OK with you I'd like to ...

8 years, 1 month ago (2012-10-26 22:39:08 UTC) #5

kmadhusu

CL is ready for review. (1) Created a MTPDeviceOperationsUtil file to handle all mtp device ...

8 years, 1 month ago (2012-11-01 01:17:24 UTC) #6

Lei Zhang

http://codereview.chromium.org/11297002/diff/30001/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right): http://codereview.chromium.org/11297002/diff/30001/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc#newcode37 chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:37: string16 GetFileObjectIdFromPath(IPortableDevice* device, In this case, GetFileObjectIdFromPath() would have ...

8 years, 1 month ago (2012-11-01 02:34:57 UTC) #7

kmadhusu

http://codereview.chromium.org/11297002/diff/30001/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h (right): http://codereview.chromium.org/11297002/diff/30001/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h#newcode101 chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:101: bool app_terminating_; Just realized that I should have used ...

8 years, 1 month ago (2012-11-01 02:38:00 UTC) #8

Peter Kasting

http://codereview.chromium.org/11297002/diff/30001/chrome/browser/system_monitor/removable_device_constants.cc File chrome/browser/system_monitor/removable_device_constants.cc (right): http://codereview.chromium.org/11297002/diff/30001/chrome/browser/system_monitor/removable_device_constants.cc#newcode19 chrome/browser/system_monitor/removable_device_constants.cc:19: const char16 kbmpFormat[] = L".bmp"; On 2012/11/01 02:34:57, Lei ...

8 years, 1 month ago (2012-11-01 04:17:36 UTC) #9

kmadhusu

thestig, pkasting: Addressed review comments. PTAL. Thanks. http://codereview.chromium.org/11297002/diff/30001/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right): http://codereview.chromium.org/11297002/diff/30001/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc#newcode37 chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:37: string16 GetFileObjectIdFromPath(IPortableDevice* ...

8 years, 1 month ago (2012-11-01 17:58:59 UTC) #10

Peter Kasting

It would be nice to use "MTP" consistently in place of "Mtp", you mostly do ...

8 years, 1 month ago (2012-11-01 21:38:37 UTC) #11

kmadhusu

pkasting: Addressed review comments. Will submit a clean up CL to rename mtp => MTP ...

8 years, 1 month ago (2012-11-02 03:27:16 UTC) #12

pkasting: Addressed review comments. Will submit a clean up CL to rename mtp =>
MTP in other files.

PTAL.

Thanks.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right):

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:174:
registered_device_path_, L"");
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: L"" -> std::string()

Changed L"" -> string16(). Did you mean string16()?

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:179:
DCHECK_GE(path_components.size(), static_cast<size_t>(1));
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: How about just DCHECK(!path_components.empty())?

I would like to keep this as it is.

 For e.g., if the |file_path| is "\\MTP:StorageSerial:SID-{1001,,192}:125\DCIM"
and |registered_device_path_| is "\\MTP:StorageSerial:SID-{1001,,192}:125", then
the |actual_file_path| is "\DCIM". When I split the string with "\" as
delimiter, we will get 2 components. Therefore, path_component will never be
empty. DCHECK(!path_components.empty()) will always be true. path_components[0]
will always have an empty string. That's why I have "begin() + 1" in loop
variable initialization statement.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h (right):

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:39: // This
class is ref-counted, because MtpDeviceDelegate is ref-counted.
On 2012/11/01 21:38:37, Peter Kasting wrote:
> And does MtpDeviceDelegate really need to be ref-counted?

Yes. This class is instantiated per MTP device storage. Extensions shares this
object to communicate with the device.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:51: virtual
fileapi::FileSystemFileUtil::AbstractFileEnumerator*
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: It would be nice for this function to return a scoped_ptr to indicate
> that's it's passing ownership of a heap-allocated object to the caller.

Will fix it in a separate CL. (This function signature matches
FileSystemFileUtil::CreateFileEnumerator function. If I am going to change this
function, its better to change all the instances of CreateFileEnumerator
function).

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:57: virtual
base::SequencedTaskRunner* media_task_runner() OVERRIDE;
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: virtual functions should never be named unix_hacker()-stlye (or inlined)

Will fix in a separate CL. I have to change several places in webkit/fileapi
code where this function is used.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:78: // e.g. If
the |file_path| is "\\MTP:StorageSerial:SID-{1001,,192}:125\DCIM"
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: If -> if

Done.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:103: // Used to
listen for NOTIFICATION_APP_TERMINATING message.
On 2012/11/01 21:38:37, Peter Kasting wrote:
> As in your previous change, you should not listen for APP_TERMINATING. 
Instead
> the class should simply be deleted at that point.  This will auto-cancel the
> tasks.

As I said earlier, this is a shared object. I cannot delete it directly.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
File chrome/browser/media_gallery/win/mtp_device_object_entry.cc (right):

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_object_entry.cc:17:
last_modified_time_(base::Time()) {
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: Explicit init of this member not necessary

Done.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
File chrome/browser/media_gallery/win/mtp_device_object_entry.h (right):

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_object_entry.h:9: // EXAMPLE:
On 2012/11/01 21:38:37, Peter Kasting wrote:
> This class seems simple enough that it doesn't seem like a full pseudo-class
> comment is necessary.  Plus this refers to MediaDeviceOperationsUtil which
isn't
> otherwise referenced here.
> 
> I'd probably just trim all this part out.

Done.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_object_entry.h:42: class
MTPDeviceObjectEntry {
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: Why is this not just a struct?  You only have accessors and a
constructor,
> so there's no need for a class.  This would further simplify things by
removing
> the accessors.
> 
> Or are you trying to ensure no one can modify one of these once it's created?

Yes. I don't want to overwrite any member variables after the creation of the
object.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_object_entry.h:44:
MTPDeviceObjectEntry();
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Do you really need this?  If this is necessary just for STL, comment to that
> effect.

Necessary for STL. Added a comment.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_object_entry.h:67: // True, if the
current object is a directory/folder/album content type.
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: True, -> True

Done.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
File chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc (right):

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc:28: return
FilePath();
On 2012/11/01 21:38:37, Peter Kasting wrote:
> I'm not familiar with the file enumerator APIs... once we return this, are we
> supposed to also clear current_object_, so the other accessors return default
> values?

It is not required. If "Next()" returns an empty path, we will not call other
accessors.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right):

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:24: const char16
kClientName[] = L"Chromium";
On 2012/11/01 21:38:37, Peter Kasting wrote:
> 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").

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

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:31:
DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
On 2012/11/01 21:38:37, Peter Kasting wrote:
> 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?

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.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:102: // Returns
true, if the object is a directory/folder/album otherwise returns
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: First sentence can be just "Returns whether the object is a
> directory/folder/album."

Done.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:129: return 
L".bmp";
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: Extra space (several places)

oops. Fixed.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:163: return
false;
On 2012/11/01 21:38:37, Peter Kasting wrote:
> 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.

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

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:190: if
(SUCCEEDED(hr) && last_modified_date.vt == VT_DATE) {
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: SUCCEEDED(hr) is guaranteed true here

Removed SUCCEEDED(hr).

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:198: return true;
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Doesn't seem like this should return true in cases where last_modified_time
was
> not set.

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.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:262: return
false;
On 2012/11/01 21:38:37, Peter Kasting wrote:
> More cases where you return false after writing to your outparams, in
violation
> of your documented contract

Fixed comment.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:275: // Creates a
MTP device object entry for the given |device| and |object_id|.
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: a -> an

Done.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:364: while (hr ==
S_OK) {
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: Or
> 
>   for (HRESULT hr = S_OK; hr == S_OK; ) {

Done.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:365: DWORD 
num_objects_fetched = 0;
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: extra space

Done.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:369: for (DWORD
index = 0; index < num_objects_fetched; index++) {
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: Prefer preincrement whenever possible

Done.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:410: const
string16& object_name) {
On 2012/11/01 21:38:37, Peter Kasting wrote:
> 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.

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

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
File chrome/browser/media_gallery/win/mtp_device_operations_util.h (right):

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_operations_util.h:68:
DISALLOW_COPY_AND_ASSIGN(MTPDeviceOperationsUtil);
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: This is probably unneeded since you can't instantiate in the first place.

Done.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
File chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc
(right):

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc:40:
return FilePath();
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Same question as before about whether we're supposed to make the other
accessors
> return default values once we hit this

This function is mainly called from FileUtilHelper class. This is a sample code
from  FileUtilHelper::ReadDirectory.

  while (!(current = file_enum->Next()).empty()) {
    base::FileUtilProxy::Entry entry;
    entry.is_directory = file_enum->IsDirectory();
    entry.name = VirtualPath::BaseName(current).value();
    entry.size = file_enum->Size();
    entry.last_modified_time = file_enum->LastModifiedTime();
    entries->push_back(entry);
  }

You can see that, we query the file properties only when Next() returns an
non-empty file path. So, we don't have to set the default values on other
accessor methods.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc:50:
if (!success || object_entries.empty()) {
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: Or just:
> 
>   if (!MTPDeviceOperationsUtil::GetDirectoryEntries(
>           device_.get(), next_object_entry.object_id(), &object_entries) ||
>       object_entries.empty()) {

I would like to use the "bool ret = GetFunc()" format.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/system_moni...
File chrome/browser/system_monitor/portable_device_watcher_win.cc (right):

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/system_moni...
chrome/browser/system_monitor/portable_device_watcher_win.cc:518:
MTPStorageMap::const_iterator storage_map_iter = storage_map_.find(
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: Break after '=' instead

Done.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/system_moni...
File chrome/browser/system_monitor/removable_device_notifications_window_win.cc
(right):

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:100:
storage_device_id, device_location, storage_object_id);
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: Or just:
> 
>   return (type == MediaStorageUtil::MTP_OR_PTP) &&
>       portable_device_watcher_->GetMTPStorageInfoFromDeviceId(
>           storage_device_id, device_location, storage_object_id);

Done.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/system_moni...
File
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc
(right):

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:665:

On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: Maybe should delete this blank line

Done.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:678:
DoMTPDeviceTest(kMTPDeviceWithValidInfo, false);
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: Probably should add a blank line above this

Done.

http://codereview.chromium.org/11297002/diff/35004/webkit/fileapi/media/mtp_d...
File webkit/fileapi/media/mtp_device_file_system_config.h (right):

http://codereview.chromium.org/11297002/diff/35004/webkit/fileapi/media/mtp_d...
webkit/fileapi/media/mtp_device_file_system_config.h:10: #if defined(OS_WIN) ||
defined(OS_LINUX)  // Implies defined(OS_CHROMEOS)
On 2012/11/01 21:38:37, Peter Kasting wrote:
> Nit: How about:
> 
> // Support MTP devices for Windows, Linux, and ChromeOS.  Note that
OS_CHROMEOS
> // implies OS_LINUX.
> #if defined(OS_WIN) || defined(OS_LINUX)

Done.

Peter Kasting

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right): http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc#newcode179 chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:179: DCHECK_GE(path_components.size(), static_cast<size_t>(1)); On 2012/11/02 03:27:16, kmadhusu wrote: > On ...

8 years, 1 month ago (2012-11-02 04:08:11 UTC) #13

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right):

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:179:
DCHECK_GE(path_components.size(), static_cast<size_t>(1));
On 2012/11/02 03:27:16, kmadhusu wrote:
> On 2012/11/01 21:38:37, Peter Kasting wrote:
> > Nit: How about just DCHECK(!path_components.empty())?
> 
> I would like to keep this as it is.
> 
>  For e.g., if the |file_path| is
"\\MTP:StorageSerial:SID-{1001,,192}:125\DCIM"
> and |registered_device_path_| is "\\MTP:StorageSerial:SID-{1001,,192}:125",
then
> the |actual_file_path| is "\DCIM". When I split the string with "\" as
> delimiter, we will get 2 components. Therefore, path_component will never be
> empty. DCHECK(!path_components.empty()) will always be true.
path_components[0]
> will always have an empty string. That's why I have "begin() + 1" in loop
> variable initialization statement.

Your existing check is "path_components.size() >= 1" which is equivalent to
"path_components.size() > 0" which is equivalent to "!path_components.empty()". 
Did you think you were checking "path_components.size() > 1" instead?

I would certainly hope "DCHECK(!path_components.empty()) will always be true",
after all, the point of a DCHECK is to assert that a particular condition is
always true.  And having it not empty is what makes the "+ 1" in your loop safe.
 Maybe I'm confused as to what you're telling me.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h (right):

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:39: // This
class is ref-counted, because MtpDeviceDelegate is ref-counted.
On 2012/11/02 03:27:16, kmadhusu wrote:
> On 2012/11/01 21:38:37, Peter Kasting wrote:
> > And does MtpDeviceDelegate really need to be ref-counted?
> 
> Yes. This class is instantiated per MTP device storage. Extensions shares this
> object to communicate with the device.

Please point me to somewhere that details the ownership model precisely.  The
fact that a class has multiple consumers does not mean it needs to be
refcounted.  There is almost always another way, which is why we ban refcounting
except with special permission.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:103: // Used to
listen for NOTIFICATION_APP_TERMINATING message.
On 2012/11/02 03:27:16, kmadhusu wrote:
> On 2012/11/01 21:38:37, Peter Kasting wrote:
> > As in your previous change, you should not listen for APP_TERMINATING. 
> Instead
> > the class should simply be deleted at that point.  This will auto-cancel the
> > tasks.
> 
> As I said earlier, this is a shared object. I cannot delete it directly. 

Again, I'm not convinced, especially after we moved away from APP_TERMINATING in
your other change.

Whoever owns this object should delete it during shutdown.  There should only be
one owner.

Even if for some reason you _had_ to refcount this, it should still simply
auto-concel on deletion, which would happen when the last ref was dropped.

Much like with refcounting itself, listening for app termination is pretty much
always wrong.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
File chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc (right):

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc:28: return
FilePath();
On 2012/11/02 03:27:16, kmadhusu wrote:
> On 2012/11/01 21:38:37, Peter Kasting wrote:
> > I'm not familiar with the file enumerator APIs... once we return this, are
we
> > supposed to also clear current_object_, so the other accessors return
default
> > values?
> 
> It is not required. If "Next()" returns an empty path, we will not call other
> accessors.

For both cases like this in this change, make sure the base class documents this
clearly in its API comments.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right):

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:163: return
false;
On 2012/11/02 03:27:16, kmadhusu wrote:
> On 2012/11/01 21:38:37, Peter Kasting wrote:
> > 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.
> 
> I would like to return bool. Fixed the comment to say that "On failure,
returns
> false".

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.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:198: return true;
On 2012/11/02 03:27:16, kmadhusu wrote:
> On 2012/11/01 21:38:37, Peter Kasting wrote:
> > Doesn't seem like this should return true in cases where last_modified_time
> was
> > not set.
> 
> 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.

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.

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
File chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc
(right):

http://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_galle...
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc:50:
if (!success || object_entries.empty()) {
On 2012/11/02 03:27:16, kmadhusu wrote:
> On 2012/11/01 21:38:37, Peter Kasting wrote:
> > Nit: Or just:
> > 
> >   if (!MTPDeviceOperationsUtil::GetDirectoryEntries(
> >           device_.get(), next_object_entry.object_id(), &object_entries) ||
> >       object_entries.empty()) {
> 
> I would like to use the "bool ret = GetFunc()" format.

Again, why?  I may agree with you if you tell me your rationale, but in the
absence of one, longer code + more temps == worse.

kmadhusu

pkasting@, thestig@: Sorry for the delay. I was fixing MTPDeviceDelegate to support weak pointers instead ...

8 years ago (2012-11-29 23:58:14 UTC) #14

pkasting@, thestig@: Sorry for the delay. I was fixing MTPDeviceDelegate to
support weak pointers instead of being ref-counted. In the new patch,
MTPDeviceDelegateImplWin supports weak pointers and is no longer ref-counted. I
have addressed your review comments. PTAL.

Thanks.

https://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right):

https://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:179:
DCHECK_GE(path_components.size(), static_cast<size_t>(1));
On 2012/11/02 04:08:11, Peter Kasting wrote:
> On 2012/11/02 03:27:16, kmadhusu wrote:
> > On 2012/11/01 21:38:37, Peter Kasting wrote:
> > > Nit: How about just DCHECK(!path_components.empty())?
> > 
> > I would like to keep this as it is.
> > 
> >  For e.g., if the |file_path| is
> "\\MTP:StorageSerial:SID-{1001,,192}:125\DCIM"
> > and |registered_device_path_| is "\\MTP:StorageSerial:SID-{1001,,192}:125",
> then
> > the |actual_file_path| is "\DCIM". When I split the string with "\" as
> > delimiter, we will get 2 components. Therefore, path_component will never be
> > empty. DCHECK(!path_components.empty()) will always be true.
> path_components[0]
> > will always have an empty string. That's why I have "begin() + 1" in loop
> > variable initialization statement.
> 
> Your existing check is "path_components.size() >= 1" which is equivalent to
> "path_components.size() > 0" which is equivalent to
"!path_components.empty()". 
> Did you think you were checking "path_components.size() > 1" instead?
> 
> I would certainly hope "DCHECK(!path_components.empty()) will always be true",
> after all, the point of a DCHECK is to assert that a particular condition is
> always true.  And having it not empty is what makes the "+ 1" in your loop
safe.
>  Maybe I'm confused as to what you're telling me.

Thanks for the explanation. You are right. DCHECK(!path_components.empty()) is
what I want here. Fixed.

https://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h (right):

https://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:39: // This
class is ref-counted, because MtpDeviceDelegate is ref-counted.
On 2012/11/02 04:08:11, Peter Kasting wrote:
> On 2012/11/02 03:27:16, kmadhusu wrote:
> > On 2012/11/01 21:38:37, Peter Kasting wrote:
> > > And does MtpDeviceDelegate really need to be ref-counted?
> > 
> > Yes. This class is instantiated per MTP device storage. Extensions shares
this
> > object to communicate with the device.
> 
> Please point me to somewhere that details the ownership model precisely.  The
> fact that a class has multiple consumers does not mean it needs to be
> refcounted.  There is almost always another way, which is why we ban
refcounting
> except with special permission.

MTPDeviceDelegateImplWin is no longer ref-counted. Modified MTPDeviceDelegate to
support weak pointers. Therefore, MTPDeviceDelegateImplWin supports weak
pointers.

Ownership Model:
(1) MediaFileSystemRegistry manages ScopedMTPDeviceMapEntry object.
(2) ScopedMTPDeviceMapEntry manages the lifetime of MTPDeviceDelegateImpl object
via MTPDeviceMapService class.
(3) ScopedMTPDeviceMapEntry adds an entry in MTPDeviceMapService when the MTP
device is registered as a media gallery file system by an extension.
(4) ScopedMTPDeviceMapEntry removes an entry from MTPDeviceMapService, when the
browser is in shutdown mode or when the last extension using the device delegate
is destroyed or when the device is detached from the system or when the user
revoke the device gallery permissions.
(5) When an entry is removed from MTPDeviceMapService, the ownership of the
|MTPDeviceDelegateImpl| is handed off to the |MTPDeviceDelegateImpl| object
which takes care of deleting itself on the right thread.

https://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:51: virtual
fileapi::FileSystemFileUtil::AbstractFileEnumerator*
On 2012/11/02 03:27:16, kmadhusu wrote:
> On 2012/11/01 21:38:37, Peter Kasting wrote:
> > Nit: It would be nice for this function to return a scoped_ptr to indicate
> > that's it's passing ownership of a heap-allocated object to the caller.
> 
> Will fix it in a separate CL. (This function signature matches
> FileSystemFileUtil::CreateFileEnumerator function. If I am going to change
this
> function, its better to change all the instances of CreateFileEnumerator
> function).

Fixed.

https://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:57: virtual
base::SequencedTaskRunner* media_task_runner() OVERRIDE;
On 2012/11/02 03:27:16, kmadhusu wrote:
> On 2012/11/01 21:38:37, Peter Kasting wrote:
> > Nit: virtual functions should never be named unix_hacker()-stlye (or
inlined)
> 
> Will fix in a separate CL. I have to change several places in webkit/fileapi
> code where this function is used.

Fixed.

https://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:103: // Used to
listen for NOTIFICATION_APP_TERMINATING message.
On 2012/11/02 04:08:11, Peter Kasting wrote:
> On 2012/11/02 03:27:16, kmadhusu wrote:
> > On 2012/11/01 21:38:37, Peter Kasting wrote:
> > > As in your previous change, you should not listen for APP_TERMINATING. 
> > Instead
> > > the class should simply be deleted at that point.  This will auto-cancel
the
> > > tasks.
> > 
> > As I said earlier, this is a shared object. I cannot delete it directly. 
> 
> Again, I'm not convinced, especially after we moved away from APP_TERMINATING
in
> your other change.
> 
> Whoever owns this object should delete it during shutdown.  There should only
be
> one owner.
> 
> Even if for some reason you _had_ to refcount this, it should still simply
> auto-concel on deletion, which would happen when the last ref was dropped.
> 
> Much like with refcounting itself, listening for app termination is pretty
much
> always wrong.

Fixed. Code no longer listens to app termination message.

https://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc (right):

https://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc:28: return
FilePath();
On 2012/11/02 04:08:11, Peter Kasting wrote:
> On 2012/11/02 03:27:16, kmadhusu wrote:
> > On 2012/11/01 21:38:37, Peter Kasting wrote:
> > > I'm not familiar with the file enumerator APIs... once we return this, are
> we
> > > supposed to also clear current_object_, so the other accessors return
> default
> > > values?
> > 
> > It is not required. If "Next()" returns an empty path, we will not call
other
> > accessors.
> 
> For both cases like this in this change, make sure the base class documents
this
> clearly in its API comments.

I will submit a separate CL to address this.

https://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right):

https://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:163: return
false;
On 2012/11/02 04:08:11, Peter Kasting wrote:
> On 2012/11/02 03:27:16, kmadhusu wrote:
> > On 2012/11/01 21:38:37, Peter Kasting wrote:
> > > 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.
> > 
> > I would like to return bool. Fixed the comment to say that "On failure,
> returns
> > false".
> 
> 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.

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

https://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:198: return true;
On 2012/11/02 04:08:11, Peter Kasting wrote:
> On 2012/11/02 03:27:16, kmadhusu wrote:
> > On 2012/11/01 21:38:37, Peter Kasting wrote:
> > > Doesn't seem like this should return true in cases where
last_modified_time
> > was
> > > not set.
> > 
> > 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.
> 
> 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.

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".

https://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc
(right):

https://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc:50:
if (!success || object_entries.empty()) {
On 2012/11/02 04:08:11, Peter Kasting wrote:
> On 2012/11/02 03:27:16, kmadhusu wrote:
> > On 2012/11/01 21:38:37, Peter Kasting wrote:
> > > Nit: Or just:
> > > 
> > >   if (!MTPDeviceOperationsUtil::GetDirectoryEntries(
> > >           device_.get(), next_object_entry.object_id(), &object_entries)
||
> > >       object_entries.empty()) {
> > 
> > I would like to use the "bool ret = GetFunc()" format.
> 
> Again, why?  I may agree with you if you tell me your rationale, but in the
> absence of one, longer code + more temps == worse.

There is no strong reason. It was easier for me to read. I am fine either way.
Therefore, inlined the function call in the if condition.

Peter Kasting

I don't have time to review further by now, but the comments below should keep ...

8 years ago (2012-12-03 20:13:00 UTC) #15

I don't have time to review further by now, but the comments below should keep
you busy for some time...

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right):

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:10: 
Nit: No blank line here

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:48:
UTF16ToUTF8(storage_device_id), pnp_device_id, storage_object_id);
Nit: Or just

  return RemovableDeviceNotificationsWindowWin::GetInstance()->
      GetMTPStorageInfoFromDeviceId(UTF16ToUTF8(storage_device_id),
                                    pnp_device_id, storage_object_id);

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:84: if
(root.value().empty() || !LazyInit()) {
Nit: Because the code to create your scoped_ptrs is so verbose, I suggest using
a temp:

{
  scoped_ptr<fileapi::FileSystemFileUtil::AbstractFileEnumerator>
      file_enumerator(new fileapi::FileSystemFileUtil::EmptyFileEnumerator());
  if (root.value().empty() || !LazyInit())
    return file_enumerator.Pass();
  ...
  if (recursive) {
    file_enumerator.reset(
        new RecursiveMTPDeviceObjectEnumerator(device_.get(), entries));
  } else {
    file_enumerator.reset(new MTPDeviceObjectEnumerator(entries));
  }
  return file_enumerator.Pass();
}

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:128: // Modify
the last modified time to null. This prevents the time stamp
Nit: Modify -> Set

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:129: //
verification in LocalFileStreamReader.
Nit: Can you be more detailed in describing what verification this prevents and
why?  I'm not familiar with this check.

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:185:
DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
Nit: Why not use |media_task_runner_| here like you do elsewhere?

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h (right):

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:8: // manages
the lifetime of this object via MTPDeviceMapService class.
Nit: I'm not sure I understand this sentence... maybe the last three words
should be removed, or the two classnames be swapped?

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:9: // This
object is constructed on UI thread. All the device operations are
Nit: on UI -> on the UI

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:12: // This
class is instantitated per MTP device storage.
Nit: instantitated -> instantiated

What does "per MTP device storage" mean?  You haven't used the word "storage"
before in this comment so it doesn't mean much to me.

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:56: virtual
base::WeakPtr<fileapi::MTPDeviceDelegate> GetAsWeakPtrOnIOThread()
You're vending a weak pointer on a different thread (IO) than the object is
created on (UI) or destroyed on (task runner).  Don't do this (see comments in
.cc file for more detail).  Weak pointers should only be created and
dereferenced on one thread, and it should be the thread the object is
created/destroyed on.  Also see comment on DeleteDelegateOnTaskRunner().

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:66: //
communication, else false.
Nit: "Returns whether the device is ready for communication." or "Returns
whether initialization succeeded."

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:72: // this
function returns the object id of "DCIM" folder.
Nit: of -> of the

Is the object ID "DCIM"?  Or is it something else?

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:74: // Returns
an empty string, if the device is detached while the request is in
Nit: Remove comma

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:78: // Deletes
the delegate on the task runner thread. Called by
!! Objects should be created and destroyed on the same thread, especially when
they support weak pointers.  So if you create this on the UI thread it should be
destroyed on the UI thread.

Here's the way you might implement this.  Make object creation happen on the UI
thread, declare all public APIs to be callable on the UI thread, and have a weak
pointer factory that vends weak pointers on the UI thread.  (We recommend using
weak pointer factories instead of SupportsWeakPtr.)

Have the object that creates this grab a weak pointer and pass it to the object
that needs to call on the IO thread.  That object can't deref the weak pointer,
but it can do something like:

  ui_loop->PostTaskAndReplyWithResult(
      FROM_HERE,
      base::Bind(&MTPDeviceDelegateImplWin::Func, delegate_weak_ptr, args),
      base::Bind(&MyClass::MyFunc, GetWeakPtr(), args));

This will call MTPDeviceDelegateImplWin::Func() on the UI thread if the delegate
still exists, then call back to MyClass::MyFunc() on the IO thread if the
calling object still exists.

Then internally, when you need to implement something as a call to the blocking
pool, you do something similar:

  media_task_runner_->PostTaskAndReplyWithResult(
      FROM_HERE,
      base::Bind(&MTPDeviceDelegateImplWin::StaticFunc, args),
      base::Bind(&MTPDeviceDelegateImplWin::ReplyFunc, GetWeakPtr(), args));

StaticFunc() should be static because it's not safe to touch any members unless
we guarantee that the object will outlive any blocking-pool-posted tasks, which
we don't.  Instead you pass in and receive data as arguments/return values.

You can change PostTaskAndReplyWithResult() to PostTaskAndReply() or just
PostTask() and optionally pass in closures to be called back at completion time
if you need to chain these sorts of things together (e.g. implement a call from
the IO thread object as a call-and-reply to the blocking pool).

In this world, you can still have something like LazyInit(), but you might do
something like this (exact syntax may not be right):

  std::stack<std::pair<Closure, Closure>> pending_tasks_;

  void MyFuncThatNeedsLazyInit() {
    EnsureInitAndThenRunTask(base::Bind(...), base::Bind(...));
  }
  void EnsureInitAndThenRunTask(const Closure& call_closure,
                                const Closure& reply_closure) {
    if (initialized_) {
      media_task_runner_->PostTaskAndReply(
          FROM_HERE, call_closure, reply_closure);
    }
    pending_tasks_.push(make_pair(call_closure, reply_closure));
    if (!pending_init_) {
      pending_init_ = true;
      media_task_runner_->PostTaskAndReplyWithResult(
          FROM_HERE, &LazyInit,
          base::Bind(&OnInitCompleted, GetWeakPtr()));
    }
  }
  void OnInitCompleted(bool succeeded) {
    pending_init_ = false;
    initialized_ = succeeded;
    if (!initialized_) {
      pending_tasks_.clear();
    } else {
      // run all pending tasks
    }
  }

This allows you to (a) ensure that only one call to LazyInit is active at any
time, (b) make LazyInit static, (c) keep the threading/memory usage safe, and
(d) still call back to all desired tasks once init succeeds.

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:81: void
DeleteDelegateOnTaskRunner();
Nit: I suggest adding "Thread" to the end of this function name

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:110: bool
cancel_pending_tasks_;
This seems like a job for a CancellationFlag.

kmadhusu

pkasting@: Thanks for your comments. I just want to clarify my assumptions before I start ...

8 years ago (2012-12-04 01:57:58 UTC) #16

Peter Kasting

On 2012/12/04 01:57:58, kmadhusu wrote: > Sorry, I don't see anything unsafe with this model. ...

8 years ago (2012-12-04 03:26:52 UTC) #17

kmadhusu

pkasting@: thestig@ and I discussed about the backend changes. DeviceMediaFileUtil access MTPDeviceDelegateImplWin object to complete ...

8 years ago (2012-12-15 02:34:56 UTC) #18

pkasting@: thestig@ and I discussed about the backend changes.
DeviceMediaFileUtil access MTPDeviceDelegateImplWin object to complete the file
system operation on the blocking thread. Therefore, we made changes to construct
and destruct the object on the same blocking thread.

In the latest patch, MTPDeviceDelegateImplWin no longer support weak pointers.
It lives mostly on the blocking thread.

Addressed other review comments. PTAL.

Thanks.

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right):

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:10: 
On 2012/12/03 20:13:00, Peter Kasting wrote:
> Nit: No blank line here

Done.

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:48:
UTF16ToUTF8(storage_device_id), pnp_device_id, storage_object_id);
On 2012/12/03 20:13:00, Peter Kasting wrote:
> Nit: Or just
> 
>   return RemovableDeviceNotificationsWindowWin::GetInstance()->
>       GetMTPStorageInfoFromDeviceId(UTF16ToUTF8(storage_device_id),
>                                     pnp_device_id, storage_object_id);

I need to do a static_cast before calling GetMTPStorageInfoFromDeviceId().
Therefore, I am leaving it as it is.

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:84: if
(root.value().empty() || !LazyInit()) {
On 2012/12/03 20:13:00, Peter Kasting wrote:
> Nit: Because the code to create your scoped_ptrs is so verbose, I suggest
using
> a temp:
> 
> {
>   scoped_ptr<fileapi::FileSystemFileUtil::AbstractFileEnumerator>
>       file_enumerator(new fileapi::FileSystemFileUtil::EmptyFileEnumerator());
>   if (root.value().empty() || !LazyInit())
>     return file_enumerator.Pass();
>   ...
>   if (recursive) {
>     file_enumerator.reset(
>         new RecursiveMTPDeviceObjectEnumerator(device_.get(), entries));
>   } else {
>     file_enumerator.reset(new MTPDeviceObjectEnumerator(entries));
>   }
>   return file_enumerator.Pass();
> }

Done.

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:128: // Modify
the last modified time to null. This prevents the time stamp
On 2012/12/03 20:13:00, Peter Kasting wrote:
> Nit: Modify -> Set

Done.

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:129: //
verification in LocalFileStreamReader.
On 2012/12/03 20:13:00, Peter Kasting wrote:
> Nit: Can you be more detailed in describing what verification this prevents
and
> why?  I'm not familiar with this check.

Done.

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:185:
DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
On 2012/12/03 20:13:00, Peter Kasting wrote:
> Nit: Why not use |media_task_runner_| here like you do elsewhere?

Done.

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h (right):

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:8: // manages
the lifetime of this object via MTPDeviceMapService class.
On 2012/12/03 20:13:00, Peter Kasting wrote:
> Nit: I'm not sure I understand this sentence... maybe the last three words
> should be removed, or the two classnames be swapped?

This sentence is an implementation detail. This sentence is not required in the
file header comment. Removed the sentence from here.

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:9: // This
object is constructed on UI thread. All the device operations are
On 2012/12/03 20:13:00, Peter Kasting wrote:
> Nit: on UI -> on the UI

This sentence is no longer used.

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:12: // This
class is instantitated per MTP device storage.
On 2012/12/03 20:13:00, Peter Kasting wrote:
> Nit: instantitated -> instantiated
> 
> What does "per MTP device storage" mean?  You haven't used the word "storage"
> before in this comment so it doesn't mean much to me.

Fixed. 
instantitated -> instantiated
storage -> partition

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:56: virtual
base::WeakPtr<fileapi::MTPDeviceDelegate> GetAsWeakPtrOnIOThread()
On 2012/12/03 20:13:00, Peter Kasting wrote:
> You're vending a weak pointer on a different thread (IO) than the object is
> created on (UI) or destroyed on (task runner).  Don't do this (see comments in
> .cc file for more detail).  Weak pointers should only be created and
> dereferenced on one thread, and it should be the thread the object is
> created/destroyed on.  Also see comment on DeleteDelegateOnTaskRunner().

This class no longer supports weak pointers. This class mostly lives on media
task runner thread except  for CancelPendingTasksAndDeleteDelegate() which
happens on the UI thread.

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:66: //
communication, else false.
On 2012/12/03 20:13:00, Peter Kasting wrote:
> Nit: "Returns whether the device is ready for communication." or "Returns
> whether initialization succeeded."

Done.

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:72: // this
function returns the object id of "DCIM" folder.
On 2012/12/03 20:13:00, Peter Kasting wrote:
> Nit: of -> of the
> 
> Is the object ID "DCIM"?  Or is it something else?

DCIM is the folder object name. Based on the stated example, this function
returns the object id of the "DCIM" folder object.

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:74: // Returns
an empty string, if the device is detached while the request is in
On 2012/12/03 20:13:00, Peter Kasting wrote:
> Nit: Remove comma

Done.

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:78: // Deletes
the delegate on the task runner thread. Called by
On 2012/12/03 20:13:00, Peter Kasting wrote:
> !! Objects should be created and destroyed on the same thread, especially when
> they support weak pointers.  So if you create this on the UI thread it should
be
> destroyed on the UI thread.
> 
> Here's the way you might implement this.  Make object creation happen on the
UI
> thread, declare all public APIs to be callable on the UI thread, and have a
weak
> pointer factory that vends weak pointers on the UI thread.  (We recommend
using
> weak pointer factories instead of SupportsWeakPtr.)
> 
> Have the object that creates this grab a weak pointer and pass it to the
object
> that needs to call on the IO thread.  That object can't deref the weak
pointer,
> but it can do something like:
> 
>   ui_loop->PostTaskAndReplyWithResult(
>       FROM_HERE,
>       base::Bind(&MTPDeviceDelegateImplWin::Func, delegate_weak_ptr, args),
>       base::Bind(&MyClass::MyFunc, GetWeakPtr(), args));
> 
> This will call MTPDeviceDelegateImplWin::Func() on the UI thread if the
delegate
> still exists, then call back to MyClass::MyFunc() on the IO thread if the
> calling object still exists.
> 
> Then internally, when you need to implement something as a call to the
blocking
> pool, you do something similar:
> 
>   media_task_runner_->PostTaskAndReplyWithResult(
>       FROM_HERE,
>       base::Bind(&MTPDeviceDelegateImplWin::StaticFunc, args),
>       base::Bind(&MTPDeviceDelegateImplWin::ReplyFunc, GetWeakPtr(), args));
> 
> StaticFunc() should be static because it's not safe to touch any members
unless
> we guarantee that the object will outlive any blocking-pool-posted tasks,
which
> we don't.  Instead you pass in and receive data as arguments/return values.
> 
> You can change PostTaskAndReplyWithResult() to PostTaskAndReply() or just
> PostTask() and optionally pass in closures to be called back at completion
time
> if you need to chain these sorts of things together (e.g. implement a call
from
> the IO thread object as a call-and-reply to the blocking pool).
> 
> In this world, you can still have something like LazyInit(), but you might do
> something like this (exact syntax may not be right):
> 
>   std::stack<std::pair<Closure, Closure>> pending_tasks_;
> 
>   void MyFuncThatNeedsLazyInit() {
>     EnsureInitAndThenRunTask(base::Bind(...), base::Bind(...));
>   }
>   void EnsureInitAndThenRunTask(const Closure& call_closure,
>                                 const Closure& reply_closure) {
>     if (initialized_) {
>       media_task_runner_->PostTaskAndReply(
>           FROM_HERE, call_closure, reply_closure);
>     }
>     pending_tasks_.push(make_pair(call_closure, reply_closure));
>     if (!pending_init_) {
>       pending_init_ = true;
>       media_task_runner_->PostTaskAndReplyWithResult(
>           FROM_HERE, &LazyInit,
>           base::Bind(&OnInitCompleted, GetWeakPtr()));
>     }
>   }
>   void OnInitCompleted(bool succeeded) {
>     pending_init_ = false;
>     initialized_ = succeeded;
>     if (!initialized_) {
>       pending_tasks_.clear();
>     } else {
>       // run all pending tasks
>     }
>   }
> 
> This allows you to (a) ensure that only one call to LazyInit is active at any
> time, (b) make LazyInit static, (c) keep the threading/memory usage safe, and
> (d) still call back to all desired tasks once init succeeds.

Removed DeleteDelegateOnTaskRunner() function.

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:81: void
DeleteDelegateOnTaskRunner();
On 2012/12/03 20:13:00, Peter Kasting wrote:
> Nit: I suggest adding "Thread" to the end of this function name

This function is no longer used.

https://codereview.chromium.org/11297002/diff/53003/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:110: bool
cancel_pending_tasks_;
On 2012/12/03 20:13:00, Peter Kasting wrote:
> This seems like a job for a CancellationFlag.

CancellationFlag can only be set on the thread it is instantiated. In our case,
CancellationFlag is created on sequenced task runner thread and has to be set on
UI thread. Therefore, I couldn't use it as such.

Peter Kasting

On 2012/12/18 21:46:38, kmadhusu wrote: > ping. I'm not going to get to this until ...

8 years ago (2012-12-18 21:48:36 UTC) #20

kmadhusu

On 2012/12/18 21:48:36, Peter Kasting wrote: > On 2012/12/18 21:46:38, kmadhusu wrote: > > ping. ...

8 years ago (2012-12-18 21:49:25 UTC) #21

Peter Kasting

This is looking much better! https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right): https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc#newcode152 chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:152: // safe member variables ...

8 years ago (2012-12-19 20:17:58 UTC) #22

kmadhusu

pkasting@: Addressed review comments. PTAL. Thanks. https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right): https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc#newcode152 chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:152: // safe member ...

8 years ago (2012-12-20 20:37:29 UTC) #24

pkasting@: Addressed review comments. PTAL.

Thanks.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right):

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:152: // safe
member variables in this function.
On 2012/12/19 20:17:58, Peter Kasting wrote:
> Nit: Given your DCHECK above, this is probably not necessary to spell out...
you
> can keep it if you think it's critical, though.

Removed.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:163: // Device
delegate will be destructed soon.
On 2012/12/19 20:17:58, Peter Kasting wrote:
> Nit: Perhaps this should instead be:
> 
> Both OpenDevice() (which we call below) and any operations our callers do may
> take some time.  Abort them immediately if we're in the process of shutting
> down.

Done.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h (right):

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:14: #include
<portabledeviceapi.h>
On 2012/12/19 20:17:58, Peter Kasting wrote:
> Nit: In general can we avoid such #includes by forward-declaring things like
> IPortableDevice?  (Several files)

How to forward declare this interface? There is no "portabledeviceidl.h" file. I
tried to do the forward declaration using "interface IPortableDevice". But I get
the following compile error:

d:\kmadhusu\home\kmadhusu\chromium\src\base\win\scoped_comptr.h(19) : error
C2787: 'IPortableDevice' : no GUID has been associated with this object
d:\kmadhusu\home\kmadhusu\chromium\src\chrome\browser\media_gallery\win\mtp_device_delegate_impl_win.cc(183)
: error C2664: 'chrome::MTPDeviceOperatio
nsUtil::OpenDevice' : cannot convert parameter 2 from
'base::win::ScopedComPtr<Interface> *' to 'base::win::ScopedComPtr<Interface> *'
        with
        [
            Interface=IPortableDevice
        ]
        Types pointed to are unrelated; conversion requires reinterpret_cast,
C-style cast or function-style cast
ninja: build stopped: subcommand failed.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:66: virtual
base::SequencedTaskRunner* GetMediaTaskRunner() OVERRIDE;
On 2012/12/19 20:17:58, Peter Kasting wrote:
> It looks like in codesearch this isn't called anywhere, and it's not called in
> this patch either.  Can we nuke this function entirely (including from the
> parent class)?

Thanks for pointing this out. Removed this function from the base class and from
here.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:96:
scoped_refptr<base::SequencedTaskRunner> media_task_runner_;
On 2012/12/19 20:17:58, Peter Kasting wrote:
> How come we actually need this anyway?  It seems like
> CancelPendingTasksAndDeleteDelegate() could just use GetBlockingPool()
directly,
> as could any other places that reference this.

This comment is misleading. This reference pointer is mainly used to
(1) make sure the file system operations are performed on the blocking pool
thread.
(2) destruct the MTPDeviceDelegateImplWin object on the blocking pool thread.

If I don't have a reference to task_runner here, I need to do the following:

bool IsMediaTaskRunnerThread() {
  base::SequencedWorkerPool* pool = content::BrowserThread::GetBlockingPool();
  base::SequencedWorkerPool::SequenceToken media_sequence_token =
      pool->GetNamedSequenceToken(fileapi::kMediaTaskRunnerName);
  return pool->IsRunningSequenceOnCurrentThread(media_sequence_token);
}

scoped_refptr<base::SequencedTaskRunner> GetSequencedTaskRunner() {
  base::SequencedWorkerPool* pool = content::BrowserThread::GetBlockingPool();
  base::SequencedWorkerPool::SequenceToken media_sequence_token =
      pool->GetNamedSequenceToken(fileapi::kMediaTaskRunnerName);
  return pool->GetSequencedTaskRunner(media_sequence_token);
}

void MTPDeviceDelegateImplWin::CancelPendingTasksAndDeleteDelegate() {
  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
  {
    base::AutoLock locked(cancel_tasks_lock_);
    cancel_pending_tasks_ = true;
  }
  GetSequencedTaskRunner()->DeleteSoon(FROM_HERE, this);
}

bool MTPDeviceDelegateImplWin::LazyInit() {
  DCHECK(IsMediaTaskRunnerThread());
  ...
}

We are already creating the sequenced task runner to construct the
MTPDeviceDelegateImplWin object. So, it's better to have a reference here and
use it when required.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:101: // The lock
to protect |cancel_pending_tasks_|. |cancel_pendings_tasks_| is
On 2012/12/19 20:17:58, Peter Kasting wrote:
> Nit: Extra 's'

Fixed.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_object_entry.h (right):

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_object_entry.h:20: typedef
std::vector<MTPDeviceObjectEntry> MTPDeviceObjectEntries;
On 2012/12/19 20:17:58, Peter Kasting wrote:
> Nit: You can skip the class forward declaration by declaring this typedef
below
> the cl;ass.  (That ordering is fine since the typedef itself is not being
> declared inside a class.)

Done.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_object_entry.h:23: class
MTPDeviceObjectEntry {
On 2012/12/19 20:17:58, Peter Kasting wrote:
> Since you have no other methods than a constructor/destructor and accessors,
> this class can simply be a struct with public data members.  Then you can get
> rid of the accessors.
> 
> You can probably nuke the DCHECKs about the blocking pool too, since it's
clear
> that a bare struct can't be safely used across multiple threads, and there's
> nothing really wrong with callers (e.g. test code) using the struct on a
> different thread, as long as they're consistently threadsafe about it.

Done.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_object_enumerator.h (right):

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_object_enumerator.h:9: // thread.
On 2012/12/19 20:17:58, Peter Kasting wrote:
> Nit: thread -> pool thread

Done.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right):

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:40: // Attempt to
set client details.
On 2012/12/19 20:17:58, Peter Kasting wrote:
> Nit: Comment doesn't really add anything

Removed.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:66: //
|enum_object_ids| is not set.
On 2012/12/19 20:17:58, Peter Kasting wrote:
> Nit: Last sentence isn't really necessary; it's implied, and your functions
> above don't have such a sentence in their comments.  (Multiple places)

Removed the last sentence. (Multiple places).

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:109: } while
((read > 0) && SUCCEEDED(hr));
On 2012/12/19 20:17:58, Peter Kasting wrote:
> Hmm, won't this mean that if Read() returns S_FALSE or E_PENDING we'll return
> true without necessarily appending the whole contents?

As per
http://msdn.microsoft.com/en-us/library/windows/desktop/aa380011(v=vs.85).aspx, 
Read() returns S_FALSE when the actual number of bytes read from the stream
object is less than the number of bytes requested (aka optimal_transfer_size).
This indicates the end of the stream has been reached. Therefore, it is fine to
return true when Read() returns S_FALSE.

When Read() returns E_PENDING, I should return false.

Fixed.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:113: 
On 2012/12/19 20:17:58, Peter Kasting wrote:
> Nit: Extra newline

Removed.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:140: return
SUCCEEDED(hr) ? static_cast<const char16*>(buffer) : string16();
On 2012/12/19 20:17:58, Peter Kasting wrote:
> Will this compile?  I'd think this would barf due to different types between
the
> two arms.

It compiles fine for me.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:162:
SystemTimeToFileTime(&system_time, &file_time))
On 2012/12/19 20:17:58, Peter Kasting wrote:
> It looks like if these function calls fail, we'll return true but
> |last_modified_time| won't be set.

Fixed.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:291:
scoped_array<char16*> object_ids(new char16*[num_objects_to_request]);
On 2012/12/19 20:17:58, Peter Kasting wrote:
> Does new char16*[] zero the pointers?

Since scoped_array is deprecated, modified code to use scoped_ptr<Foo[]>.
scoped_ptr<Foo[]> does not zero the pointers. Added code to zero the memory.

>  I can't remember.  If it doesn't, and
> Next() doesn't overwrite them all, we could have memory corruption when they
are
> deleted.  (Though you don't do this -- see below.)

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:293:
&num_objects_fetched);
On 2012/12/19 20:17:58, Peter Kasting wrote:
> MSDN for Next() says the strings should be freed with CoTaskMemFree().  You're
> not actually deleting them at all -- scoped_array deletes the array, but not
> what the pointers in it point to.
> 
> I don't remember if it's safe to use our standard delete function in place of
> CoTaskMemFree().  cpu might know.

You are right. I should call CoTaskMemFree() to free the actual data. Fixed.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:408:
DCHECK_EQ(object_entries.size(), static_cast<size_t>(1));
On 2012/12/19 20:17:58, Peter Kasting wrote:
> Nit: DCHECK_EQ(1U, object_entries.size());

Done.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:412:
MTPDeviceOperationsUtil::MTPDeviceOperationsUtil() {
On 2012/12/19 20:17:58, Peter Kasting wrote:
> No need to actually define this.  (This becomes clearer when you change to the
> macro in the header file.)

Done.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_operations_util.h (right):

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_operations_util.h:65:
MTPDeviceOperationsUtil();
On 2012/12/19 20:17:58, Peter Kasting wrote:
> Nit: Replace comment and declaration with
> DISALLOW_IMPLICIT_CONSTRUCTORS(MTPDeviceOperationsUtil);.

Done.

Peter Kasting

I haven't looked at your new patch; I'll do that tomorrow. https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right): ...

8 years ago (2012-12-20 21:24:20 UTC) #25

I haven't looked at your new patch; I'll do that tomorrow.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right):

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:38:
DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
Based on discussion with rsleevi, I suggest two DCHECK changes in this file. 
Both are to make this class agnostic about the actual thread it runs on; given
that the caller supplies a sequenced task runner, there's no reason that runner
couldn't be made to point at a dedicated thread instead of something in the
blocking pool.

So the first change is here.  Let's just remove this DCHECK.  This function is
local to this file so it's reasonable to assume the caller is using it
correctly.  If you were paranoid you could pass in a sequenced task runner and
check that it's running on the current thread (see below), but that seems
unnecessary.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:57:
DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
The second change is here.  Rather than check that the blocking pool runs tasks
on the current thread, just check that |media_task_runner| runs tasks on the
current thread.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h (right):

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:14: #include
<portabledeviceapi.h>
On 2012/12/20 20:37:29, kmadhusu wrote:
> On 2012/12/19 20:17:58, Peter Kasting wrote:
> > Nit: In general can we avoid such #includes by forward-declaring things like
> > IPortableDevice?  (Several files)
> 
> How to forward declare this interface? There is no "portabledeviceidl.h" file.

I was thinking we could just use "class IPortableDevice".  If the compiler
doesn't like that, just ignore this comment.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:96:
scoped_refptr<base::SequencedTaskRunner> media_task_runner_;
On 2012/12/20 20:37:29, kmadhusu wrote:
> We are already creating the sequenced task runner to construct the
> MTPDeviceDelegateImplWin object. So, it's better to have a reference here and
> use it when required.

I talked to rsleevi a bunch about this so I could fully understand all the
tradeoffs in different solutions.  I'm fine with what you're doing here.  Maybe
this comment should simply be something like:

"The task runner where most of the class lives and runs (save for
CancelPendingTasksAndDeleteDelegate())."

kmadhusu

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right): https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc#newcode38 chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:38: DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); On 2012/12/20 21:24:20, Peter Kasting wrote: > Based ...

8 years ago (2012-12-21 01:45:23 UTC) #26

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right):

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:38:
DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
On 2012/12/20 21:24:20, Peter Kasting wrote:
> Based on discussion with rsleevi, I suggest two DCHECK changes in this file. 
> Both are to make this class agnostic about the actual thread it runs on; given
> that the caller supplies a sequenced task runner, there's no reason that
runner
> couldn't be made to point at a dedicated thread instead of something in the
> blocking pool.
> 
> So the first change is here.  Let's just remove this DCHECK.  This function is
> local to this file so it's reasonable to assume the caller is using it
> correctly.  If you were paranoid you could pass in a sequenced task runner and
> check that it's running on the current thread (see below), but that seems
> unnecessary.

Done. Removed the DCHECK.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:57:
DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
On 2012/12/20 21:24:20, Peter Kasting wrote:
> The second change is here.  Rather than check that the blocking pool runs
tasks
> on the current thread, just check that |media_task_runner| runs tasks on the
> current thread.

Done.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h (right):

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:14: #include
<portabledeviceapi.h>
On 2012/12/20 21:24:20, Peter Kasting wrote:
> On 2012/12/20 20:37:29, kmadhusu wrote:
> > On 2012/12/19 20:17:58, Peter Kasting wrote:
> > > Nit: In general can we avoid such #includes by forward-declaring things
like
> > > IPortableDevice?  (Several files)
> > 
> > How to forward declare this interface? There is no "portabledeviceidl.h"
file.
> 
> I was thinking we could just use "class IPortableDevice".  If the compiler
> doesn't like that, just ignore this comment.

Compiler is not happy when I specify "class IPortableDevice". Ignoring this
review comment.

https://codereview.chromium.org/11297002/diff/65004/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:96:
scoped_refptr<base::SequencedTaskRunner> media_task_runner_;
On 2012/12/20 21:24:20, Peter Kasting wrote:
> On 2012/12/20 20:37:29, kmadhusu wrote:
> > We are already creating the sequenced task runner to construct the
> > MTPDeviceDelegateImplWin object. So, it's better to have a reference here
and
> > use it when required.
> 
> I talked to rsleevi a bunch about this so I could fully understand all the
> tradeoffs in different solutions.  I'm fine with what you're doing here. 
Maybe
> this comment should simply be something like:
> 
> "The task runner where most of the class lives and runs (save for
> CancelPendingTasksAndDeleteDelegate())."

Fixed the comment.

Peter Kasting

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right): https://codereview.chromium.org/11297002/diff/86007/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc#newcode68 chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:68: DCHECK(media_task_runner_.get()); Nit: This DCHECK should probably move to CreateMTPDeviceDelegate() ...

8 years ago (2012-12-21 18:13:30 UTC) #27

cpu_(ooo_6.6-7.5)

I am ok with calling CoTaskMemFree directly. Ping me if you need any more item ...

7 years, 12 months ago (2012-12-28 00:01:07 UTC) #28

kmadhusu

pkasting@: Happy New Year. Addressed review comments. Added a worker class (MTPGetStorageInfoWorker) to get the ...

7 years, 11 months ago (2013-01-02 19:48:31 UTC) #29

pkasting@: Happy New Year. Addressed review comments. Added a worker class
(MTPGetStorageInfoWorker) to get the device storage details on the UI thread.
PTAL.

Thanks.

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right):

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:68:
DCHECK(media_task_runner_.get());
On 2012/12/21 18:13:30, Peter Kasting wrote:
> Nit: This DCHECK should probably move to CreateMTPDeviceDelegate() (or you
could
> put both checks in both places)

Added a DCHECK in CreateMTPDeviceDelegate.

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_object_entry.cc (right):

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_object_entry.cc:13: size(0) {
On 2012/12/21 18:13:30, Peter Kasting wrote:
> Nit: You can put both these on the line with the function name if they both
fit.

Done.

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_object_entry.h (right):

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_object_entry.h:8: // thread.
On 2012/12/21 18:13:30, Peter Kasting wrote:
> Nit: I'd leave out the last sentence here; nothing in this struct guarantees
(or
> indeed cares) about what thread it's on.

Done.

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right):

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:96: if ((hr !=
S_OK) && (hr != S_FALSE))
On 2012/12/21 18:13:30, Peter Kasting wrote:
> Nit: I suggest explaining S_FALSE (the comment you wrote me about it would be
a
> pretty good explanation).

Done.

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:154: if
(last_modified_date.vt == VT_DATE) {
On 2012/12/21 18:13:30, Peter Kasting wrote:
> Nit: Use |last_modified_time_set| here for brevity/efficiency

Done.

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:164:
*last_modified_time = base::Time();
On 2012/12/21 18:13:30, Peter Kasting wrote:
> Doesn't explicitly clearing this violate the function contract?  The comments
> say we set this variable if everything succeeded.  On failure shouldn't we
just
> leave it alone?

As per your earlier
comment(https://codereview.chromium.org/11297002/diff/35004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode198),
you asked me to reset the outparam explicitly.

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:166: return
last_modified_time_set;
On 2012/12/21 18:13:30, Peter Kasting wrote:
> You don't have to do this in this CL, but you could make this simpler and
safer
> by using a scoping object for your PROPVARIANT.  Note that
> core_audio_util_win.cc locally defines a ScopedPropVariant, and the new
> scoped_ptr<> capabilities also give you the ability to simply use a
scoped_ptr<>
> with a custom deleter.  It would be nice to have a followup cleanup patch that
> removes all the PropVariantClear() calls in the codebase (looks like 9
> currently) and replaces them with this.

I will submit a cleanup patch later.

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:290:
ZeroMemory(object_ids.get(), num_objects_to_request);
On 2012/12/21 18:13:30, Peter Kasting wrote:
> Don't do this.  new[] will default-initialize non-POD types for you.
> 
> (You can also force default-initialization of POD types by appending a () to
the
> [], but it's not necessary in your case.)

Removed ZeroMemory(...).

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:292:
static_cast<char16**>(&object_ids[0]),
On 2012/12/21 18:13:30, Peter Kasting wrote:
> This makes me leery.  It actually works based on how ScopedCoMem is defined
> today, but if someone were to change that class definition, this could break.
> 
> I think ScopedCoMem is a good choice for individual strings, but in a case
like
> this with an array, I'd actually just go ahead and call CoTaskMemFree()
> directly.

Fixed. Used scoped_ptr<char16*[]> and CoTaskMemFree() directly.

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/system_mon...
File chrome/browser/system_monitor/portable_device_watcher_win.cc (right):

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/system_mon...
chrome/browser/system_monitor/portable_device_watcher_win.cc:523:
DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
On 2012/12/21 18:13:30, Peter Kasting wrote:
> This is scary.  What if the UI thread deletes the object while you're doing
> this?  I don't see how you can guarantee this is safe.

There is a chance that the UI thread may delete this object while the task
runner is running this code.

A better solution for this problem is to call this function on the UI thread.
Therefore, I created a worker class (MTPGetStorageInfoWorker) to get the device
storage details on the UI thread.

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/system_mon...
File chrome/browser/system_monitor/removable_device_notifications_window_win.cc
(right):

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:141:
DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
On 2012/12/21 18:13:30, Peter Kasting wrote:
> Doesn't the rest of this class run on the UI thread?  How is this safe?

I thought about this. There is a small chance of destructing
|portable_device_watcher_| on the UI thread while it is being accessed here on
the blocking thread. In order to avoid that I need to introduce another lock in
this class.

A better solution is to call this function on the UI thread. Made changes to
block the MTPDeviceDelegateImplWin task runner until we get the MTP storage
details from the UI thread.

Peter Kasting

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/media_gallery/win/mtp_device_operations_util.cc File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right): https://codereview.chromium.org/11297002/diff/86007/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode164 chrome/browser/media_gallery/win/mtp_device_operations_util.cc:164: *last_modified_time = base::Time(); On 2013/01/02 19:48:31, kmadhusu wrote: > ...

7 years, 11 months ago (2013-01-02 21:41:09 UTC) #30

kmadhusu

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/media_gallery/win/mtp_device_operations_util.cc File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right): https://codereview.chromium.org/11297002/diff/86007/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode164 chrome/browser/media_gallery/win/mtp_device_operations_util.cc:164: *last_modified_time = base::Time(); On 2013/01/02 21:41:09, Peter Kasting wrote: ...

7 years, 11 months ago (2013-01-02 23:49:48 UTC) #31

Peter Kasting

Your new helper class potentially deadlocks. You post a task on the UI thread and ...

7 years, 11 months ago (2013-01-04 21:13:38 UTC) #32

Your new helper class potentially deadlocks.

You post a task on the UI thread and then block.  The UI thread task may not
ever run, because the UI thread may begin to shut down before this task runs. 
In that case the UI thread will try to tear down the blocking pool, which will
block until your thread completes.  Boom, you're hosed.

I talked this over with rsleevi and we agreed on what the only possible fixes
are:

(1) (Not recommended) You can make this more of a PostTaskAndReply()-type thing,
where we don't block.  That means LazyInit() will then also have to be broken
into two phases (the call and the callback).  That in turn makes everyone who
calls LazyInit() two-phase, and so forth virally.  Ultimately this would mean
that a huge portion of this change would need to get rewritten.  Pretty nasty.

(2) CreateMTPDeviceDelegate() (or some similar location) can do the
PostTaskAndReply() to the UI thread directly to get the necessary info, then
pass it in to the MTPDeviceDelegateImplWin.  This merely requires that callers
don't expect their callback to run synchronously.  I think that's fine, since it
looks like the existing codebase calls this via PostTask().  Of course, the UI
thread also needs to have the necessary info available at the time this call
runs.  But presumably that's also already safe, since it doesn't seem like the
UI thread today could know that it's safe to wait longer before your worker
object were to come calling.

I see you've already checked in code like the stuff in this change for Linux, so
you'll need to fix the existing code too.  That also suggests to me that you're
having other reviewers look at code who don't sufficiently understand threading.
 I suggest you get some experienced and competent (regarding threading) folks
like rsleevi involved in your future design and reviews.  The fact that you've
come back to me several times with different designs that are all unsafe
suggests that the assistance would be valuable.

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/system_mon...
File chrome/browser/system_monitor/portable_device_watcher_win.cc (right):

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/system_mon...
chrome/browser/system_monitor/portable_device_watcher_win.cc:523:
DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
On 2013/01/02 19:48:31, kmadhusu wrote:
> A better solution for this problem is to call this function on the UI thread.

It might be nice to have DCHECKs that you're on the UI thread in all the
functions that are supposed to run on the UI thread, like you already have in
some of them.

https://codereview.chromium.org/11297002/diff/91027/chrome/browser/media_gall...
File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right):

https://codereview.chromium.org/11297002/diff/91027/chrome/browser/media_gall...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:307: for (DWORD
index = 0; index < num_objects_fetched; ++index) {
Nit: {} unnecessary

kmadhusu

https://codereview.chromium.org/11297002/diff/86007/chrome/browser/system_monitor/portable_device_watcher_win.cc File chrome/browser/system_monitor/portable_device_watcher_win.cc (right): https://codereview.chromium.org/11297002/diff/86007/chrome/browser/system_monitor/portable_device_watcher_win.cc#newcode523 chrome/browser/system_monitor/portable_device_watcher_win.cc:523: DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); On 2013/01/04 21:13:38, Peter Kasting wrote: > On ...

7 years, 11 months ago (2013-01-04 22:14:32 UTC) #33

kmadhusu

https://codereview.chromium.org/11297002/diff/128001/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right): https://codereview.chromium.org/11297002/diff/128001/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc#newcode131 chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:131: task_completed_event_.Signal(); On 2013/01/04 22:14:33, kmadhusu wrote: > Due to ...

7 years, 11 months ago (2013-01-04 22:16:42 UTC) #34

Peter Kasting

That sounds safer, but I still don't think this is a good solution. It's rather ...

7 years, 11 months ago (2013-01-05 00:00:34 UTC) #35

Ryan Sleevi

So I definitely have to agree with Peter, that the threading and ownership issues in ...

7 years, 11 months ago (2013-01-05 03:00:57 UTC) #36

So I definitely have to agree with Peter, that the threading and ownership
issues in this CL seem to represent several anti-patterns in Chrome that tend to
make code more error prone and more difficult to maintain, long term.

Broadly speaking, the overall design goal should be to
1) Have classes with clear thread affinities
2) Have classes with clear lifetimes - eg: ALWAYS living and dying on specific
threads
3) Have classes with clear ownerships - it should be possible to describe as a
tree the ownership lifetimes, and with dependencies always flowing down.

This CL makes heavy use of RefCounting to bounce the same object across multiple
threads, along with generally unsafe patterns such as WaitableEvents that block
on the UI(!!!) thread. That's a real anti-pattern.

Given that it seems both the Windows and the Linux implementations have to deal
with devices that only be accessible synchronously and may block indefinitely,
it seems like your primary design should be to ensure that

1) Your main "public" interfaces are declared asynchronously (eg: expecting
callbacks)
2) Your implementations behave synchronously

You can then vary the how-and-where of your implementations execution. It may be
on the content worker pool, or it may be that the devices you're interacting
with are so slow that putting it on the browser blocking pool would cause even
worse performance than starting 1 or more dedicated threads to deal with MTP
devices. I'm not sure, but the constant reliance on the content WorkerPool is a
real design concern, because it means this dependency is spread throughout your
code, rather than cleanly isolated.

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h (right):

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:9: // This class
is instantiated per MTP device partition.
STYLE: You should be describing these comments as Class comments, as per

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_...

This applies to every new header in this CL.

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right):

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:31:
DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
DESIGN: Is it really a design constraint that you run these on the blocking
pool?

It seems throughout this CL, that you're using the "Blocking Pool" as a
substitute for base::ThreadRestrictions::AssertIOAllowed(), with
AssertIOAllowed() being much more preferable than binding to any particular
pool.

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:93: std::string
buffer;
perf: Constantly re-allocating a new buffer inside the loop. You should declare
|buffer| outside.

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:109: return
false;
style: should be using braces here

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:165: else
style: should be using braces here

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:221: return
false;
style: should be using braces here

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:261: return
false;
style: Should be using braces here

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:410:
object_entries.empty())
style: You should be using braces here

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:412:
DCHECK_EQ(1U, object_entries.size());
Should be a CHECK_EQ - you have no guarantee the next line will crash (it's
undefined behaviour)

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_operations_util.h (right):

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.h:61: const
string16& object_name);
DESIGN: Given that you take an |IPortableDevice*| for all methods but
OpenDevice, why is this not a class with member functions, rather than the
static functions? Seems very C-API reminiscent, and it's not immediately clear
why.

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_get_storage_info_worker.cc (right):

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_get_storage_info_worker.cc:82:
worker->media_task_runner()->DeleteSoon(FROM_HERE, worker);
LEAK: If DeleteSoon fails to post a task, you'll end up leaking an object.

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_get_storage_info_worker.h (right):

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_get_storage_info_worker.h:31:
MTPGetStorageInfoWorkerDeleter> {
Objects that live on multiple threads should be strongly discouraged as a design
principle. It appears you made this RefCounted solely for that reason, which is
one of the Chromium anti-patterns.

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_get_storage_info_worker.h:41: void Run();
DESIGN: This is a broken design here. You should not arbitrarily block a thread
on the UI thread. That's not what the "blocking" pool is for.

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_get_storage_info_worker.h:79:
MTPDeviceDelegateImplWin* device_delegate_;
DESIGN: This sort of design coupling between classes is a signal of poor
ownership semantics that will be an ongoing maintenance burden.

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_get_storage_info_worker.h:102:
base::WaitableEvent* on_task_completed_event_;
I'm very concerned that this sort of technical debt is only going to be added,
and that there's no incentives to pay it down once this code lands.

I think this should be prioritized over the Windows implementation.

Peter Kasting

I think most of the major problems can be fixed by nuking the MTPGetStorageInfoWorker object ...

7 years, 11 months ago (2013-01-05 03:17:19 UTC) #37

I think most of the major problems can be fixed by nuking the
MTPGetStorageInfoWorker object entirely as I proposed earlier.

Of Ryan's larger points, this leaves the question of whether the remaining
design satisfies his suggestions about async public interfaces and sync
implementations.  I think the answer to that depends on where the "public" API
is declared to be.

I do agree that it would be nice to make as much code as possible agnostic to
the existence of the WorkerPool per se.  We've already moved somewhat in that
direction in the previous round of changes, but can probably do more.  I didn't
know about AssertIOAllowed(), that seems helpful.  There are also cases
currently where the code is basically trying to assert "this function runs on
the same thread as that function" where it might not need to if we got stricter
about enforcing that classes are accessed on exactly one thread -- though I
think that if we kill the worker object, the main remaining instance of this is
the MTPDeviceDelegate CancelPendingTasksAndDeleteDelegate() function, which,
while I still feel a little icky about, we've previously reviewed and OK'd.

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right):

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:109: return
false;
On 2013/01/05 03:00:58, Ryan Sleevi wrote:
> style: should be using braces here

Braces are optional if the body is one line.  It does not matter whether or not
the conditional is multiline.

Personally I encourage no braces in these scenarios, but more than anything, I
encourage consistency within a file and with the surrounding code.

(This reply applies anywhere you've made this comment)

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:412:
DCHECK_EQ(1U, object_entries.size());
On 2013/01/05 03:00:58, Ryan Sleevi wrote:
> Should be a CHECK_EQ - you have no guarantee the next line will crash (it's
> undefined behaviour)

That's fine.  We don't need to force a crash when a precondition is unsatisfied.
 It's completely OK to have undefined behavior happen (and in fact we do it in
many places throughout Chrome).

CHECK should only be used when violating the assertion would lead directly to an
exploitable security hole, or when we're trying to track down why some crash is
happening.  Otherwise, DCHECK is preferable.

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_get_storage_info_worker.cc (right):

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_get_storage_info_worker.cc:82:
worker->media_task_runner()->DeleteSoon(FROM_HERE, worker);
On 2013/01/05 03:00:58, Ryan Sleevi wrote:
> LEAK: If DeleteSoon fails to post a task, you'll end up leaking an object.

But if it fails to post a task, how do we know?  And when would that happen
other than during shutdown, in which case the leak is probably fine?

Ryan Sleevi

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gallery/win/mtp_device_operations_util.cc File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right): https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode412 chrome/browser/media_gallery/win/mtp_device_operations_util.cc:412: DCHECK_EQ(1U, object_entries.size()); On 2013/01/05 03:17:19, Peter Kasting wrote: > ...

7 years, 11 months ago (2013-01-05 03:44:37 UTC) #38

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right):

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:412:
DCHECK_EQ(1U, object_entries.size());
On 2013/01/05 03:17:19, Peter Kasting wrote:
> On 2013/01/05 03:00:58, Ryan Sleevi wrote:
> > Should be a CHECK_EQ - you have no guarantee the next line will crash (it's
> > undefined behaviour)
> 
> That's fine.  We don't need to force a crash when a precondition is
unsatisfied.
>  It's completely OK to have undefined behavior happen (and in fact we do it in
> many places throughout Chrome).
> 
> CHECK should only be used when violating the assertion would lead directly to
an
> exploitable security hole, or when we're trying to track down why some crash
is
> happening.  Otherwise, DCHECK is preferable.

You absolutely should force a crash when you're about to do something with
potentially uninitialized memory. Either that, or handle it - preferably handle
it if it's handleable.

Use CHECK here, or return string16() and drop the DCHECK.

The use of an uninitialized memory like this is usually a key step in an attack,
by carefully aligning things so that the uninitialized memory can be used to
determine the address of a vtable or undermine ASLR, and from there build a ROP
bundle. This is a common issue in WebKit code with heap spraying and U-A-Fs.

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_get_storage_info_worker.cc (right):

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_get_storage_info_worker.cc:82:
worker->media_task_runner()->DeleteSoon(FROM_HERE, worker);
On 2013/01/05 03:17:19, Peter Kasting wrote:
> On 2013/01/05 03:00:58, Ryan Sleevi wrote:
> > LEAK: If DeleteSoon fails to post a task, you'll end up leaking an object.
> 
> But if it fails to post a task, how do we know?  And when would that happen
> other than during shutdown, in which case the leak is probably fine?

blergh, DeleteSoon seems to be swallowing the result of
DeleteViaSequencedTaskRunner(...) [which is bool].

My concern here is that the lifetime guarantees afforded this class are pretty
weak. In particular, |media_task_runner()| has no established lifetime
guarantees, other than "less than the UI thread". As a result, if there's a task
posted to the UI thread, but this object (and it's associated STR) are torn
down, then it will leak. There's no API contract anywhere that requires this be
at shutdown.

It would be different if this object lived on the UI thread - since you know the
UI thread shutting down indicates shutdown - but otherwise the coupling here of
the order of shutdown and the lifetime of |task_runner| is pretty obscure and
brittle.

kmadhusu

(I have not modified any code, just responding to some of your comments). pkasting@: The ...

7 years, 11 months ago (2013-01-07 19:17:26 UTC) #39

(I have not modified any code, just responding to some of your comments).

pkasting@: The proposed solution is completely doable. It will nuke
MTPGetStorageInfoWorker class and the associated WaitableEvent.

Regarding MTPDeviceDelegateImplLinux, I totally agree that the WaitableEvent
solution is not the best solution. But it is a temporary solution until we
modify the public interfaces as asynchronous (crbug.com/154835). FileSystem team
didn't get a chance to work on that issue during the last quarter. But it will
be fixed in Q1 2013. I thought I will remove all the WaitableEvents from
MTPDeviceDelegateImplLinux after fixing that issue. Now that the issue is the
root cause of all these problems, I will make sure it gets fixed asap (or we
will find an alternative way immediately to remove all the WaitableEvents from
MTPDeviceDelegateImplLinux).

Thanks.

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right):

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:412:
DCHECK_EQ(1U, object_entries.size());
On 2013/01/05 03:44:37, Ryan Sleevi wrote:
> On 2013/01/05 03:17:19, Peter Kasting wrote:
> > On 2013/01/05 03:00:58, Ryan Sleevi wrote:
> > > Should be a CHECK_EQ - you have no guarantee the next line will crash
(it's
> > > undefined behaviour)
> > 
> > That's fine.  We don't need to force a crash when a precondition is
> unsatisfied.
> >  It's completely OK to have undefined behavior happen (and in fact we do it
in
> > many places throughout Chrome).
> > 
> > CHECK should only be used when violating the assertion would lead directly
to
> an
> > exploitable security hole, or when we're trying to track down why some crash
> is
> > happening.  Otherwise, DCHECK is preferable.
> 
> You absolutely should force a crash when you're about to do something with
> potentially uninitialized memory. Either that, or handle it - preferably
handle
> it if it's handleable.
> 
> Use CHECK here, or return string16() and drop the DCHECK.
> 
> The use of an uninitialized memory like this is usually a key step in an
attack,
> by carefully aligning things so that the uninitialized memory can be used to
> determine the address of a vtable or undermine ASLR, and from there build a
ROP
> bundle. This is a common issue in WebKit code with heap spraying and U-A-Fs.

rsleevi@: At line 410, I already handled the object_entries.empty() case and
returned string16(). |object_entries| is completely valid at line 412. I am just
making sure that the total number of entries is 1. If the number of entries is
more than one, we are just going to return the zeroth entry value. Based on my
testing, this DCHECK is always true. So, we don't need a CHECK_EQ here.

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_operations_util.h (right):

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.h:61: const
string16& object_name);
On 2013/01/05 03:00:58, Ryan Sleevi wrote:
> DESIGN: Given that you take an |IPortableDevice*| for all methods but
> OpenDevice, why is this not a class with member functions, rather than the
> static functions? Seems very C-API reminiscent, and it's not immediately clear
> why.

I thought an util class with a bunch of static methods is a common pattern. All
these util functions maintains no state and is reentrant. Will it be clear, if I
have a set of free functions in a namespace rather than static methods on a
class? What major benefit do I get by storing |IPortableDevice*| as a member
variable other than one less input param to the util functions?

Ryan Sleevi

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gallery/win/mtp_device_operations_util.cc File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right): https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode412 chrome/browser/media_gallery/win/mtp_device_operations_util.cc:412: DCHECK_EQ(1U, object_entries.size()); On 2013/01/07 19:17:26, kmadhusu wrote: > On ...

7 years, 11 months ago (2013-01-07 19:29:29 UTC) #40

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right):

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:412:
DCHECK_EQ(1U, object_entries.size());
On 2013/01/07 19:17:26, kmadhusu wrote:
> On 2013/01/05 03:44:37, Ryan Sleevi wrote:
> > On 2013/01/05 03:17:19, Peter Kasting wrote:
> > > On 2013/01/05 03:00:58, Ryan Sleevi wrote:
> > > > Should be a CHECK_EQ - you have no guarantee the next line will crash
> (it's
> > > > undefined behaviour)
> > > 
> > > That's fine.  We don't need to force a crash when a precondition is
> > unsatisfied.
> > >  It's completely OK to have undefined behavior happen (and in fact we do
it
> in
> > > many places throughout Chrome).
> > > 
> > > CHECK should only be used when violating the assertion would lead directly
> to
> > an
> > > exploitable security hole, or when we're trying to track down why some
crash
> > is
> > > happening.  Otherwise, DCHECK is preferable.
> > 
> > You absolutely should force a crash when you're about to do something with
> > potentially uninitialized memory. Either that, or handle it - preferably
> handle
> > it if it's handleable.
> > 
> > Use CHECK here, or return string16() and drop the DCHECK.
> > 
> > The use of an uninitialized memory like this is usually a key step in an
> attack,
> > by carefully aligning things so that the uninitialized memory can be used to
> > determine the address of a vtable or undermine ASLR, and from there build a
> ROP
> > bundle. This is a common issue in WebKit code with heap spraying and U-A-Fs.
> 
> rsleevi@: At line 410, I already handled the object_entries.empty() case and
> returned string16(). |object_entries| is completely valid at line 412. I am
just
> making sure that the total number of entries is 1. If the number of entries is
> more than one, we are just going to return the zeroth entry value. Based on my
> testing, this DCHECK is always true. So, we don't need a CHECK_EQ here.

ACK - you're right. DCHECK is totally fine here then.

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_operations_util.h (right):

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.h:61: const
string16& object_name);
On 2013/01/07 19:17:26, kmadhusu wrote:
> On 2013/01/05 03:00:58, Ryan Sleevi wrote:
> > DESIGN: Given that you take an |IPortableDevice*| for all methods but
> > OpenDevice, why is this not a class with member functions, rather than the
> > static functions? Seems very C-API reminiscent, and it's not immediately
clear
> > why.
> 
> I thought an util class with a bunch of static methods is a common pattern.
All
> these util functions maintains no state and is reentrant. Will it be clear, if
I
> have a set of free functions in a namespace rather than static methods on a
> class? What major benefit do I get by storing |IPortableDevice*| as a member
> variable other than one less input param to the util functions? 

Classes that are static-method-holders are discouraged in the style guide:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmem...

"Rather than creating classes only to group static member functions which do not
share static data, use namespaces instead."

kmadhusu

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gallery/win/mtp_device_operations_util.h File chrome/browser/media_gallery/win/mtp_device_operations_util.h (right): https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gallery/win/mtp_device_operations_util.h#newcode61 chrome/browser/media_gallery/win/mtp_device_operations_util.h:61: const string16& object_name); On 2013/01/07 19:29:29, Ryan Sleevi wrote: ...

7 years, 11 months ago (2013-01-07 19:36:42 UTC) #41

kmadhusu

pkasting@, rsleevi@: Addressed review comments. Removed MTPGetStorageInfoWorker class. PTAL. Thanks. https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h (right): https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h#newcode9 ...

7 years, 11 months ago (2013-01-08 03:19:40 UTC) #42

pkasting@, rsleevi@: Addressed review comments. Removed MTPGetStorageInfoWorker
class. PTAL.

Thanks.

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h (right):

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:9: // This class
is instantiated per MTP device partition.
On 2013/01/05 03:00:58, Ryan Sleevi wrote:
> STYLE: You should be describing these comments as Class comments, as per
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_...
> 
> This applies to every new header in this CL.

rsleevi@: Moved some class detail comments from the file header to the class
header section. Do you want me to add some code samples on how these classes are
instantiated? (It seems pretty straightforward to me).

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right):

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:31:
DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
On 2013/01/05 03:00:58, Ryan Sleevi wrote:
> DESIGN: Is it really a design constraint that you run these on the blocking
> pool?
> 
> It seems throughout this CL, that you're using the "Blocking Pool" as a
> substitute for base::ThreadRestrictions::AssertIOAllowed(), with
> AssertIOAllowed() being much more preferable than binding to any particular
> pool.

Replaced some of the instances of
"DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());"
with "base::ThreadRestrictions::AssertIOAllowed()".

I still have some DCHECKs on other files to make sure the function is called on
the right thread.

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:93: std::string
buffer;
On 2013/01/05 03:00:58, Ryan Sleevi wrote:
> perf: Constantly re-allocating a new buffer inside the loop. You should
declare
> |buffer| outside.

Done.

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:165: else
On 2013/01/05 03:00:58, Ryan Sleevi wrote:
> style: should be using braces here

Ignoring this comment since the braces are optional.

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_operations_util.h (right):

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.h:61: const
string16& object_name);
On 2013/01/07 19:36:43, kmadhusu wrote:
> On 2013/01/07 19:29:29, Ryan Sleevi wrote:
> > On 2013/01/07 19:17:26, kmadhusu wrote:
> > > On 2013/01/05 03:00:58, Ryan Sleevi wrote:
> > > > DESIGN: Given that you take an |IPortableDevice*| for all methods but
> > > > OpenDevice, why is this not a class with member functions, rather than
the
> > > > static functions? Seems very C-API reminiscent, and it's not immediately
> > clear
> > > > why.
> > > 
> > > I thought an util class with a bunch of static methods is a common
pattern.
> > All
> > > these util functions maintains no state and is reentrant. Will it be
clear,
> if
> > I
> > > have a set of free functions in a namespace rather than static methods on
a
> > > class? What major benefit do I get by storing |IPortableDevice*| as a
member
> > > variable other than one less input param to the util functions? 
> > 
> > Classes that are static-method-holders are discouraged in the style guide:
> > 
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmem...
> > 
> > "Rather than creating classes only to group static member functions which do
> not
> > share static data, use namespaces instead."
> 
> Fair enough. Will fix it in the next patch.

Fixed. Created free functions in the chrome namespace.

https://codereview.chromium.org/11297002/diff/118026/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right):

https://codereview.chromium.org/11297002/diff/118026/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:70: void
OnGotStorageInfoCreateDelegate(
In order to access the private constructor of MTPDeviceDelegateImplWin, I need
to make this function as a MTPDeviceDelegateImplWin friend. Therefore, defined
this function in the chrome namespace instead of anonymous  namespace.

https://codereview.chromium.org/11297002/diff/118026/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:87:
content::BrowserThread::PostTask(
pkasting@: I tried to use PostTaskAndReplyWithResult() using the following code
snippet:
 string16* pnp_device_id = new string16;
 string16* storage_object_id = new string16;
 content::BrowserThread::PostTaskAndReplyWithResult<bool>(
      content::BrowserThread::UI, FROM_HERE,
      base::Bind(&GetStorageInfoFromStoragePath,
                 device_location,
                 pnp_device_id,
                 storage_object_id),
      base::Bind(&OnGotStorageInfoCreateDelegate,
                 device_location,
                 media_task_runner,
                 callback,
                 base::Owned(pnp_device_id),
                 base::Owned(storage_object_id)));

Unfortunately, "MessageLoopProxy::current()" is NULL for media_task_runner.
Therefore, I am unable to use PostTaskAndReply or PostTaskAndReplyWithResult
directly. As a workaround, I did the following:

(1) CreateMTPDeviceDelegate will post a task on the UI thread to get the storage
details.
(2) Once got the storage details, post a task on the media task runner to create
the device delegate.

Peter Kasting

I'm going to be gone for a week+ starting tonight, so at least for now ...

7 years, 11 months ago (2013-01-08 04:34:01 UTC) #43

kmadhusu

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gallery/win/mtp_device_operations_util.h File chrome/browser/media_gallery/win/mtp_device_operations_util.h (right): https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gallery/win/mtp_device_operations_util.h#newcode61 chrome/browser/media_gallery/win/mtp_device_operations_util.h:61: const string16& object_name); On 2013/01/08 04:34:01, Peter Kasting wrote: ...

7 years, 11 months ago (2013-01-08 20:55:18 UTC) #44

Ryan Sleevi

A quick review looking over the diffs from patchset 14 -> 15. Since pkasting kicked ...

7 years, 11 months ago (2013-01-09 00:20:36 UTC) #45

A quick review looking over the diffs from patchset 14 -> 15. Since pkasting
kicked the final review bucket over to me, I'll take another pass on the
full/final CL later today, just to make sure. I did want to get these comments
in while Peter was still available though.

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h (right):

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:9: // This class
is instantiated per MTP device partition.
On 2013/01/08 03:19:40, kmadhusu wrote:
> On 2013/01/05 03:00:58, Ryan Sleevi wrote:
> > STYLE: You should be describing these comments as Class comments, as per
> > 
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_...
> > 
> > This applies to every new header in this CL.
> 
> rsleevi@: Moved some class detail comments from the file header to the class
> header section. Do you want me to add some code samples on how these classes
are
> instantiated? (It seems pretty straightforward to me).

My main concern was comments like "This class" reasonably belong located with
the class itself.

You've currently separated out descriptions of this class between the file level
comments and the class level comments, but both are needed to understand how the
class works - so they arguably belong there.

My particular point was the (old) line 9, which you removed, but I think it's
generally inconsistent with expectations and style to have both file-level and
class-level comments both describing a class and how it is used.

https://codereview.chromium.org/11297002/diff/118026/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_object_entry.h (right):

https://codereview.chromium.org/11297002/diff/118026/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_object_entry.h:18: // See comment at
top of file for the complete description.
While I realize the style guide permits this, you can see in Chromium code no
such occurrences of this pattern. Instead, Chromium code tends to keep the
description of the class with the class itself.

I realize this point may be a bit controversial, and Peter's much more a
style-lawyer than I am, but this has been my first time in Chromium code seeing
this approach, and it was enough to cause me to miss some of the implementation
details and guarantees that you originally had.

https://codereview.chromium.org/11297002/diff/118026/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_object_enumerator.h (right):

https://codereview.chromium.org/11297002/diff/118026/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_object_enumerator.h:20: // is mainly
used to enumerate top-level files of a media device file system.
"mainly used"? What other uses does it have? How do those look?

While I'm unfamiliar with the MTP, this code leaves me wondering.

I also tend to make a distinction between how a class should be used / can be
used and how a class is being used. Describing how a class "is being used" is
usually a recipe for comment obsolescence, since it's separated from the code
that's using it. In the long run, it creates more questions than answers.

Describing how a class can or should be used, however, provides much more useful
information to the reader attempting to understand this class.

kmadhusu

rsleevi@: Addressed review comments. PTAL. Thanks. https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h (right): https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h#newcode9 chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:9: // This class ...

7 years, 11 months ago (2013-01-09 18:11:14 UTC) #46

rsleevi@: Addressed review comments. PTAL.

Thanks.

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h (right):

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:9: // This class
is instantiated per MTP device partition.
On 2013/01/09 00:20:36, Ryan Sleevi wrote:
> On 2013/01/08 03:19:40, kmadhusu wrote:
> > On 2013/01/05 03:00:58, Ryan Sleevi wrote:
> > > STYLE: You should be describing these comments as Class comments, as per
> > > 
> > >
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_...
> > > 
> > > This applies to every new header in this CL.
> > 
> > rsleevi@: Moved some class detail comments from the file header to the class
> > header section. Do you want me to add some code samples on how these classes
> are
> > instantiated? (It seems pretty straightforward to me).
> 
> My main concern was comments like "This class" reasonably belong located with
> the class itself.
> 
> You've currently separated out descriptions of this class between the file
level
> comments and the class level comments, but both are needed to understand how
the
> class works - so they arguably belong there.
> 
> My particular point was the (old) line 9, which you removed, but I think it's
> generally inconsistent with expectations and style to have both file-level and
> class-level comments both describing a class and how it is used.

As we discussed, moved these class details to the class comments section and
also rephrased the comemnt.

https://codereview.chromium.org/11297002/diff/118026/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_object_entry.h (right):

https://codereview.chromium.org/11297002/diff/118026/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_object_entry.h:18: // See comment at
top of file for the complete description.
On 2013/01/09 00:20:36, Ryan Sleevi wrote:
> While I realize the style guide permits this, you can see in Chromium code no
> such occurrences of this pattern. Instead, Chromium code tends to keep the
> description of the class with the class itself.
> 
> I realize this point may be a bit controversial, and Peter's much more a
> style-lawyer than I am, but this has been my first time in Chromium code
seeing
> this approach, and it was enough to cause me to miss some of the
implementation
> details and guarantees that you originally had.

Fixed.

https://codereview.chromium.org/11297002/diff/118026/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_object_enumerator.h (right):

https://codereview.chromium.org/11297002/diff/118026/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_object_enumerator.h:20: // is mainly
used to enumerate top-level files of a media device file system.
On 2013/01/09 00:20:36, Ryan Sleevi wrote:
> "mainly used"? What other uses does it have? How do those look?
> 
> While I'm unfamiliar with the MTP, this code leaves me wondering.
> 
> I also tend to make a distinction between how a class should be used / can be
> used and how a class is being used. Describing how a class "is being used" is
> usually a recipe for comment obsolescence, since it's separated from the code
> that's using it. In the long run, it creates more questions than answers.
> 
> Describing how a class can or should be used, however, provides much more
useful
> information to the reader attempting to understand this class.

Rephrased the comment.

kmadhusu

https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gallery/win/mtp_device_operations_util.h File chrome/browser/media_gallery/win/mtp_device_operations_util.h (right): https://codereview.chromium.org/11297002/diff/123003/chrome/browser/media_gallery/win/mtp_device_operations_util.h#newcode61 chrome/browser/media_gallery/win/mtp_device_operations_util.h:61: const string16& object_name); On 2013/01/08 20:55:18, kmadhusu wrote: > ...

7 years, 11 months ago (2013-01-09 19:25:46 UTC) #47

Ryan Sleevi

Mostly, these are slight comment nits and a few IWYU nits. While I have a ...

7 years, 11 months ago (2013-01-09 20:22:25 UTC) #48

kmadhusu

rsleevi: Addressed most of your comments except one in recursive_mtp_device_object_enumerator.cc. We didn't plan to run ...

7 years, 11 months ago (2013-01-10 04:59:19 UTC) #49

rsleevi: Addressed most of your comments except one in
recursive_mtp_device_object_enumerator.cc.

We didn't plan to run this code behind a flag. After committing this CL, users
of mediaGalleries API will have MTP device media support on Windows. But,
considering the concerns, I think its better if we run this code behind the flag
until we fix the async issues. If you are fine with this, I will add a new flag
in the next patch. 

Thanks.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right):

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:35: // MTP
device delegate on the blocking pool thread.
On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> nit: s/blocking pool thread/media task runner/

Done.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:69: // delegate
on the blocking pool thread.
On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> nit: s/blocking pool thread/media task runner/

Done.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:76:
DCHECK(media_task_runner && media_task_runner->RunsTasksOnCurrentThread());
On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> nit: Split this into two DCHECKs, based on the rest of the file/CL using
> per-condition DCHECKs.
> 
> Same on line 86 as well.

Done.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:171: // Users
will use HTML5 FileSystem Entry getMetadata() interface to get actual
On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> nit: s/get actual/get the actual/

Done.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_object_entry.h (right):

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_object_entry.h:10: #include
"base/string16.h"
On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> IWYU: You should add
> #include "base/basictypes.h"
> 
> (for int64)

Done.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc (right):

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc:31:
++object_entry_iter_;
On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> *(object_entry_iter++) ?

Done.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_object_enumerator.h (right):

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_object_enumerator.h:23: // pool
thread.
On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> s/MTPDeviceObjectEnumerator is created, destroyed, and operated on the
blocking
> pool thread./MTPDeviceObjectEnumerator may only be used on a single thread./

Done.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right):

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:46:
GENERIC_READ);
On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> BUG? You aren't checking the HRESULT of any of these methods.

We attempt to set the client information. It is okay if they fail.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:80: // success,
returns true and the stream contents are appended to snapshot file.
On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> comment: What is a "snapshot file" ?
> 
> Is this an concept of the FileAPI leaking in (with its snapshot state)? Why
not
> just say "file" ?

Yes, the term "snapshot file" is a FileAPI concept. Snapshot file is just a
local file. Replaced "snapshot file" with "file".

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:93:
optimal_transfer_size, &read);
On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> FUTURE/DESIGN: It seems like this interface can be written to use asynchronous
> monikers in the future. Do you have a bug to address this?
> 
crbug.com/110119. FileSystem/File API doesn't support streamed reading from a
media file. This is a temporary solution to provide media file read support.

Added a TODO to deprecate this function after fixing crbug.com/110119.

> It seems VERY BAD to potentially be reading a 1GB video off MTP storage (for
> example)

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:99: if ((hr !=
S_OK) && (hr != S_FALSE))
On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> minor nit: the inner parenthesis seem superfluous:
> 
> if (hr != S_OK && hr != S_FALSE)

pkasting@ suggested parens around binary subexpressions.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:139: return
SUCCEEDED(hr) ? static_cast<const char16*>(buffer) : string16();
On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> DESIGN: This caught me a bit by surprise, since |buffer| is ScopedCoMem and I
> had to mentally double check the C++ spec to make sure that the conversion to
> string16 would happen before the destruction of |buffer|.
> 
> Further, the way it's written prevents most forms of RVO.
> 
> string16 result;
> if (SUCCEEDED(hr))
>   result.assign(buffer);
> return result;
> 
> Avoids the static cast, avoids the "Is it safe?", and gets RVO for free.

Done.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:173: bool
GetObjectSize(IPortableDeviceValues* properties_values, int64* size) {
On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> DESIGN: The impedence mismatch between an int64 and a uint64 makes me very
> uncomfortable here, especially when we're in the world of "Windows APIs and
> third-party code that breaks all expectations".
> 
> In an abundance of caution, if you're going to use int64* to represent the
size
> (rather than uint64), I would prefer you have a local variable "ULONGLONG
> actual_size" and make sure that actual_size <= kint64max before doing *size =
> static_cast<int64>(actual_size)

Done.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:330: DPLOG(ERROR)
<< "Access denied to open the device";
On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> 1) Why only DPLOG for E_ACCESSDENIED

The |client_info| used by Open() call requests for a GENERIC_READ access. This
access should not be denied. When the request is denied, I thought I should log
this detail for debugging purposes.

> 2) DPLOG uses GetLastError - but I thought the COM programming model was such
> that the error is instead passed through |hr|?

I didn't find any documentation in msdn website regarding this. Based on my
testing, I was able to get both GetLastError() and HResult details after each
call.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_operations_util.h (right):

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.h:24: 
On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> namespace media_transfer_protocol { }?
> namespace mtp { }? (not a particular fan of this)
> namespace media_transfer { }?

"media_transfer_protocol" is too generic. I prefer
"media_transfer_protocol_device_operations_utils". But it is too long.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.h:32: // |device|.
On success, fills in |file_entry_info|. On failure, returns the
On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> nit: What does it return on success?

No error (base::PLATFORM_FILE_OK). Fixed the comment.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.h:41: //
|object_entries|. On failure, returns false and |object_entries| are not
On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> grammar: s/are not set/is not set/
> 
> While "entries" is plural, it's the name of a singular variable.

Done.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc
(right):

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc:43:
MTPDeviceObjectEntry next_object_entry = *object_entry_iter_;
On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> nit: *(object_entry_iter_++)

Done.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc:54:
new MTPDeviceObjectEnumerator(object_entries));
On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> Why do you not go further recursively?

Good catch. I had a recursive enumerator in my test app so I didn't encounter
this bug. I have a fix locally but I need to test more. I will do the testing
tomorrow morning and will upload the fix in the next patch.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/system_mo...
File chrome/browser/system_monitor/portable_device_watcher_win.cc (right):

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/system_mo...
chrome/browser/system_monitor/portable_device_watcher_win.cc:533:
DCHECK(device_iter != device_map_.end());
On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> This DCHECK leaves me a lil squicky, since AIUI, this API will ultimately be
> glued up to client code and, given the threading, I find it very hard to
reason
> that this will always be the case (especially if the user removes devices etc)
> 
> Should this just be a proper check
> if (device_iter == device_map_.end())
>   return false;

When the device is removed, line 528 will be true and we will do an early
return. To be safe, modified the DCHECK to a proper check.

Ryan Sleevi

If this code, once landed, would "be live", then I would prefer you either fix ...

7 years, 11 months ago (2013-01-10 05:07:18 UTC) #50

If this code, once landed, would "be live", then I would prefer you either fix
the async ness (which will be a non-trivial CL, I fear), or land this behind a
flag for now while it's under development.

Because this functionality will ultimately get exposed to web content (even with
perhaps a little user input), it is much more important that we get it right and
safe, rather than get it in.

Once there's a flag that will protect us from monopolizing threads for large
reads, then we can land this.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right):

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:93:
optimal_transfer_size, &read);
On 2013/01/10 04:59:19, kmadhusu wrote:
> On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> > FUTURE/DESIGN: It seems like this interface can be written to use
asynchronous
> > monikers in the future. Do you have a bug to address this?
> > 
> crbug.com/110119. FileSystem/File API doesn't support streamed reading from a
> media file. This is a temporary solution to provide media file read support.
> 
> Added a TODO to deprecate this function after fixing crbug.com/110119.
> 
> > It seems VERY BAD to potentially be reading a 1GB video off MTP storage (for
> > example)
> 

That's an orthogonal issue. I understand that FileAPI doesn't support streaming
files, and so you have to copy the snapshot locally, but you should copy the
snapshot locally using asynchronous methods.

What I'm concerned about is a thread being monopolized for a large file on a
slow device. "Ideally" we should run it with an asynchronous moniker so that we
can abort the read, and so that other tasks can run while the OS is dispatching
the reads to the underlying MTP device.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_operations_util.h (right):

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.h:24: 
On 2013/01/10 04:59:19, kmadhusu wrote:
> On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> > namespace media_transfer_protocol { }?
> > namespace mtp { }? (not a particular fan of this)
> > namespace media_transfer { }?
> 
> "media_transfer_protocol" is too generic. I prefer
> "media_transfer_protocol_device_operations_utils". But it is too long.

media_transfer_protocol is not too generic for a namespace. Indeed, for
namespaces, you want generic names.

Perhaps I don't understand the concern here.

kmadhusu

rsleevi@: Added a command line flag and addressed review comments. PTAL. Thanks. https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gallery/win/mtp_device_operations_util.cc File chrome/browser/media_gallery/win/mtp_device_operations_util.cc ...

7 years, 11 months ago (2013-01-10 23:37:56 UTC) #51

rsleevi@: Added a command line flag and addressed review comments. PTAL.

Thanks.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right):

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:93:
optimal_transfer_size, &read);
On 2013/01/10 05:07:18, Ryan Sleevi wrote:
> On 2013/01/10 04:59:19, kmadhusu wrote:
> > On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> > > FUTURE/DESIGN: It seems like this interface can be written to use
> asynchronous
> > > monikers in the future. Do you have a bug to address this?
> > > 
> > crbug.com/110119. FileSystem/File API doesn't support streamed reading from
a
> > media file. This is a temporary solution to provide media file read support.
> > 
> > Added a TODO to deprecate this function after fixing crbug.com/110119.
> > 
> > > It seems VERY BAD to potentially be reading a 1GB video off MTP storage
(for
> > > example)
> > 
> 
> That's an orthogonal issue. I understand that FileAPI doesn't support
streaming
> files, and so you have to copy the snapshot locally, but you should copy the
> snapshot locally using asynchronous methods.
> 
> What I'm concerned about is a thread being monopolized for a large file on a
> slow device. "Ideally" we should run it with an asynchronous moniker so that
we
> can abort the read, and so that other tasks can run while the OS is
dispatching
> the reads to the underlying MTP device.

Until we fix the async issue, this CL will run behind a flag.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_operations_util.h (right):

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.h:24: 
On 2013/01/10 05:07:18, Ryan Sleevi wrote:
> On 2013/01/10 04:59:19, kmadhusu wrote:
> > On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> > > namespace media_transfer_protocol { }?
> > > namespace mtp { }? (not a particular fan of this)
> > > namespace media_transfer { }?
> > 
> > "media_transfer_protocol" is too generic. I prefer
> > "media_transfer_protocol_device_operations_utils". But it is too long.
> 
> media_transfer_protocol is not too generic for a namespace. Indeed, for
> namespaces, you want generic names.
> 
> Perhaps I don't understand the concern here.

Used media_transfer_protocol instead of
media_transfer_protocol_device_operations_utils.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc
(right):

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gal...
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc:54:
new MTPDeviceObjectEnumerator(object_entries));
On 2013/01/09 20:22:25, Ryan Sleevi wrote:
> Why do you not go further recursively?

Fixed.

Ryan Sleevi

https://codereview.chromium.org/11297002/diff/165001/chrome/browser/media_gallery/win/mtp_device_operations_util.cc File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right): https://codereview.chromium.org/11297002/diff/165001/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode188 chrome/browser/media_gallery/win/mtp_device_operations_util.cc:188: return SUCCEEDED(hr); BUG? This means if |actual_size| > kint64max, ...

7 years, 11 months ago (2013-01-11 17:40:46 UTC) #52

kmadhusu

https://codereview.chromium.org/11297002/diff/165001/chrome/browser/media_gallery/win/mtp_device_operations_util.cc File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right): https://codereview.chromium.org/11297002/diff/165001/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode188 chrome/browser/media_gallery/win/mtp_device_operations_util.cc:188: return SUCCEEDED(hr); On 2013/01/11 17:40:46, Ryan Sleevi wrote: > ...

7 years, 11 months ago (2013-01-11 19:01:59 UTC) #53

Lei Zhang

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_object_entry.h File chrome/browser/media_gallery/win/mtp_device_object_entry.h (right): https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_object_entry.h#newcode16 chrome/browser/media_gallery/win/mtp_device_object_entry.h:16: // MTPDeviceObjectEntry contains information about the media transfer protocol ...

7 years, 11 months ago (2013-01-14 20:49:02 UTC) #55

Lei Zhang

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right): https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc#newcode135 chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:135: if (root.value().empty() || !LazyInit()) nit: root.empty() https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc#newcode170 chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:170: base::PlatformFileError ...

7 years, 11 months ago (2013-01-14 23:25:30 UTC) #56

kmadhusu

thestig@: Addressed review comments. PTAL. Thanks. https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc File chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc (right): https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc#newcode135 chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:135: if (root.value().empty() || ...

7 years, 11 months ago (2013-01-15 19:08:17 UTC) #57

Lei Zhang

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right): https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode113 chrome/browser/media_gallery/win/mtp_device_operations_util.cc:113: } while ((read > 0) && SUCCEEDED(hr)); On 2013/01/15 ...

7 years, 11 months ago (2013-01-15 21:00:12 UTC) #58

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/me...
File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right):

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/me...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:113: } while
((read > 0) && SUCCEEDED(hr));
On 2013/01/15 19:08:17, kmadhusu wrote:
> On 2013/01/14 23:25:30, Lei Zhang wrote:
> > Why do you need the SUCCEEDED() part? You've already checked the value of
|hr|
> > at line 103. SUCCEEDED(hr) is always true here.
> 
> If |hr| is S_FALSE, SUCCEEDED(hr) will be false. Please refer to comment at
line
> 98 for more details.

No, SUCCEEDED(S_FALSE) returns true. S_ means success. See
http://msdn.microsoft.com/en-us/library/windows/desktop/ms687197(v=vs.85).aspx
and
http://msdn.microsoft.com/en-us/library/windows/desktop/ff485842(v=vs.85).aspx

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/me...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:126: return
((content_type == WPD_CONTENT_TYPE_AUDIO_ALBUM) ||
On 2013/01/15 19:08:17, kmadhusu wrote:
> On 2013/01/14 23:25:30, Lei Zhang wrote:
> > I'm not sure albums (aka playlists??) are directories. Have you tested this
> with
> > an audio or video album? 
> 
> As per
>
http://msdn.microsoft.com/en-us/library/windows/desktop/dd389020%28v=vs.85%29...,
> albums contains actual objects. Albums and playlists are equivalent but albums
> stores the actual objects instead of a virtual object or reference to the some
> object metadata. If the object is a playlist, it will describe its content
type
> as WPD_CONTENT_TYPE_PLAYLIST. I tested with an IMAGE_ALBUM object.
> 

Based on my understanding of MTP, I don't believe albums or playlists are
physical directories, but rather auxiliary metadata structures to help organize
objects into lists. Can you show me how you tested so we can examine the MTP
device with the album and figure out what's going on?

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/me...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:283: bool
get_all_entries,
On 2013/01/15 19:08:17, kmadhusu wrote:
> On 2013/01/14 23:25:30, Lei Zhang wrote:
> > You can just indicate this with a non empty |object_name| instead.
> 
> This is more clearer than the suggested solution.

Not particularly. It's not hard to understand "empty string = don't care about
the name". Having two parameters mean one has to understand the interaction
between those two parameters. With my suggestion, the documentation is simpler
too:

"To request a specific object entry, put the object name in |object_name|. Leave
|object_name| blank to request all entries."

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/me...
File chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h
(right):

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/me...
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h:8:
#include <portabledeviceapi.h>
On 2013/01/15 19:08:17, kmadhusu wrote:
> On 2013/01/14 23:25:30, Lei Zhang wrote:
> > nit: empty line between C and C++ headers. Here and elsewhere.
> 
> I had an empty line before. Reviewers asked me to remove the extra line.

I ran into the same issue on my readability review. In the end, we got one of
the C++ style guide authors to clarify this:

"... (thus there’s a mix of styles in the codebase), and the text of the guide
does not require or forbid any particular convention for blank line, then the
“when in Rome” principle should apply. In Chrome code, the only style we have
places a blank line between the C and C++ system header sections."

That said, I've noticed even base/*.h is not consistent on this. I would also
encourage consistency with surrounding code, but there aren't any other files
that includes C headers in c/b/media_galleries. The closest relative is
c/b/system_monitor, which has the blank line.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/me...
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h:26: //
object entries set. RecursiveMTPDeviceObjectEnumerator communicates with the
On 2013/01/15 19:08:17, kmadhusu wrote:
> On 2013/01/14 23:25:30, Lei Zhang wrote:
> > Doesn't it go through MTPDeviceObjectEnumerator?
> 
> RecursiveMTPDeviceObjectEnumerator communicates with the device to get the
> subdirectory object entries. RecursiveMTPDeviceObjectEnumerator uses
> MTPDeviceObjectEnumerator to iterate the current directory objects.

That should have gone in the comments and not in the reply here.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/me...
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h:28: //
RecursiveMTPDeviceObjectEnumerator supports media file system operations.
On 2013/01/15 19:08:17, kmadhusu wrote:
> On 2013/01/14 23:25:30, Lei Zhang wrote:
> > What does this mean exactly? Are you trying to say it implements
> > AbstractFileEnumerator?
> 
> RecursiveMTPDeviceObjectEnumerator is used to complete the DOMFileSystem
> operation, e.g DirectoryEntry.removeRecursively interface. In our case, it is
a
> media gallery file system operation.

Ok, but I don't see how this comment is useful. Aren't all the classes in this
directory used to "support media file system operations" ?

Lei Zhang

https://chromiumcodereview.appspot.com/11297002/diff/213001/chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc File chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc (right): https://chromiumcodereview.appspot.com/11297002/diff/213001/chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc#newcode17 chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc:17: object_entry_iter_(object_entries_.begin()) { You should initialize |current_object_| to NULL. https://chromiumcodereview.appspot.com/11297002/diff/213001/chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc#newcode27 ...

7 years, 11 months ago (2013-01-15 22:35:50 UTC) #59

kmadhusu

thestig@: Addressed review comments. PTAL. Thanks. https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right): https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode113 chrome/browser/media_gallery/win/mtp_device_operations_util.cc:113: } while ((read ...

7 years, 11 months ago (2013-01-15 23:41:58 UTC) #60

thestig@: Addressed review comments. PTAL.

Thanks.

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right):

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:113: } while
((read > 0) && SUCCEEDED(hr));
On 2013/01/15 21:00:13, Lei Zhang wrote:
> On 2013/01/15 19:08:17, kmadhusu wrote:
> > On 2013/01/14 23:25:30, Lei Zhang wrote:
> > > Why do you need the SUCCEEDED() part? You've already checked the value of
> |hr|
> > > at line 103. SUCCEEDED(hr) is always true here.
> > 
> > If |hr| is S_FALSE, SUCCEEDED(hr) will be false. Please refer to comment at
> line
> > 98 for more details.
> 
> No, SUCCEEDED(S_FALSE) returns true. S_ means success. See
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms687197%28v=vs.85%29...
> and
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ff485842%28v=vs.85%29...

Removed SUCCEEDED(hr).

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:126: return
((content_type == WPD_CONTENT_TYPE_AUDIO_ALBUM) ||
On 2013/01/15 21:00:13, Lei Zhang wrote:
> On 2013/01/15 19:08:17, kmadhusu wrote:
> > On 2013/01/14 23:25:30, Lei Zhang wrote:
> > > I'm not sure albums (aka playlists??) are directories. Have you tested
this
> > with
> > > an audio or video album? 
> > 
> > As per
> >
>
http://msdn.microsoft.com/en-us/library/windows/desktop/dd389020%2528v=vs.85%...,
> > albums contains actual objects. Albums and playlists are equivalent but
albums
> > stores the actual objects instead of a virtual object or reference to the
some
> > object metadata. If the object is a playlist, it will describe its content
> type
> > as WPD_CONTENT_TYPE_PLAYLIST. I tested with an IMAGE_ALBUM object.
> > 
> 
> Based on my understanding of MTP, I don't believe albums or playlists are
> physical directories, but rather auxiliary metadata structures to help
organize
> objects into lists. Can you show me how you tested so we can examine the MTP
> device with the album and figure out what's going on?

As discussed, removed the album types and added a TODO to investigate this in
detail.

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:283: bool
get_all_entries,
On 2013/01/15 21:00:13, Lei Zhang wrote:
> On 2013/01/15 19:08:17, kmadhusu wrote:
> > On 2013/01/14 23:25:30, Lei Zhang wrote:
> > > You can just indicate this with a non empty |object_name| instead.
> > 
> > This is more clearer than the suggested solution.
> 
> Not particularly. It's not hard to understand "empty string = don't care about
> the name". Having two parameters mean one has to understand the interaction
> between those two parameters. With my suggestion, the documentation is simpler
> too:
> 
> "To request a specific object entry, put the object name in |object_name|.
Leave
> |object_name| blank to request all entries."

Done.

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h
(right):

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gal...
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h:8:
#include <portabledeviceapi.h>
On 2013/01/15 21:00:13, Lei Zhang wrote:
> On 2013/01/15 19:08:17, kmadhusu wrote:
> > On 2013/01/14 23:25:30, Lei Zhang wrote:
> > > nit: empty line between C and C++ headers. Here and elsewhere.
> > 
> > I had an empty line before. Reviewers asked me to remove the extra line.
> 
> I ran into the same issue on my readability review. In the end, we got one of
> the C++ style guide authors to clarify this:
> 
> "... (thus there’s a mix of styles in the codebase), and the text of the guide
> does not require or forbid any particular convention for blank line, then the
> “when in Rome” principle should apply. In Chrome code, the only style we have
> places a blank line between the C and C++ system header sections."
> 
> That said, I've noticed even base/*.h is not consistent on this. I would also
> encourage consistency with surrounding code, but there aren't any other files
> that includes C headers in c/b/media_galleries. The closest relative is
> c/b/system_monitor, which has the blank line.

Done.

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gal...
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h:26: //
object entries set. RecursiveMTPDeviceObjectEnumerator communicates with the
On 2013/01/15 21:00:13, Lei Zhang wrote:
> On 2013/01/15 19:08:17, kmadhusu wrote:
> > On 2013/01/14 23:25:30, Lei Zhang wrote:
> > > Doesn't it go through MTPDeviceObjectEnumerator?
> > 
> > RecursiveMTPDeviceObjectEnumerator communicates with the device to get the
> > subdirectory object entries. RecursiveMTPDeviceObjectEnumerator uses
> > MTPDeviceObjectEnumerator to iterate the current directory objects.
> 
> That should have gone in the comments and not in the reply here.

Done.

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gal...
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h:28: //
RecursiveMTPDeviceObjectEnumerator supports media file system operations.
On 2013/01/15 21:00:13, Lei Zhang wrote:
> On 2013/01/15 19:08:17, kmadhusu wrote:
> > On 2013/01/14 23:25:30, Lei Zhang wrote:
> > > What does this mean exactly? Are you trying to say it implements
> > > AbstractFileEnumerator?
> > 
> > RecursiveMTPDeviceObjectEnumerator is used to complete the DOMFileSystem
> > operation, e.g DirectoryEntry.removeRecursively interface. In our case, it
is
> a
> > media gallery file system operation.
> 
> Ok, but I don't see how this comment is useful. Aren't all the classes in this
> directory used to "support media file system operations" ?

Removed.

https://codereview.chromium.org/11297002/diff/213001/chrome/browser/media_gal...
File chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc (right):

https://codereview.chromium.org/11297002/diff/213001/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc:17:
object_entry_iter_(object_entries_.begin()) {
On 2013/01/15 22:35:50, Lei Zhang wrote:
> You should initialize |current_object_| to NULL.

Done.

https://codereview.chromium.org/11297002/diff/213001/chrome/browser/media_gal...
chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc:27: if
(object_entry_iter_ == object_entries_.end())
On 2013/01/15 22:35:50, Lei Zhang wrote:
> If we reached the end, |current_object_| still points to the last object and
> will return its information.

Fixed.

Lei Zhang

lgtm https://codereview.chromium.org/11297002/diff/222001/chrome/browser/media_gallery/win/mtp_device_operations_util.cc File chrome/browser/media_gallery/win/mtp_device_operations_util.cc (right): https://codereview.chromium.org/11297002/diff/222001/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode135 chrome/browser/media_gallery/win/mtp_device_operations_util.cc:135: (content_type == WPD_CONTENT_TYPE_FUNCTIONAL_OBJECT)); Can you add a comment ...

7 years, 11 months ago (2013-01-15 23:53:37 UTC) #61

Lei Zhang

https://codereview.chromium.org/11297002/diff/222001/chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc File chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc (right): https://codereview.chromium.org/11297002/diff/222001/chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc#newcode17 chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc:17: object_entry_iter_(object_entries_.begin()) { still need to initialize |current_object_|.

7 years, 11 months ago (2013-01-15 23:54:48 UTC) #62

kmadhusu

https://codereview.chromium.org/11297002/diff/222001/chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc File chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc (right): https://codereview.chromium.org/11297002/diff/222001/chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc#newcode17 chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc:17: object_entry_iter_(object_entries_.begin()) { On 2013/01/15 23:54:48, Lei Zhang wrote: > ...

7 years, 11 months ago (2013-01-16 00:14:24 UTC) #63

Lei Zhang

https://codereview.chromium.org/11297002/diff/234004/chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc File chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc (right): https://codereview.chromium.org/11297002/diff/234004/chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc#newcode34 chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc:34: if (current_object_) BTW, |current_object_| cannot be NULL.

7 years, 11 months ago (2013-01-16 00:34:46 UTC) #65

kmadhusu

thestig: Updated copyright details. https://chromiumcodereview.appspot.com/11297002/diff/234004/chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc File chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc (right): https://chromiumcodereview.appspot.com/11297002/diff/234004/chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc#newcode34 chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc:34: if (current_object_) On 2013/01/16 00:34:46, ...

7 years, 11 months ago (2013-01-16 01:36:59 UTC) #67

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11297002/235004

7 years, 11 months ago (2013-01-16 03:32:00 UTC) #69

commit-bot: I haz the power

Presubmit check for 11297002-235004 failed and returned exit status 1. Running presubmit commit checks ...

7 years, 11 months ago (2013-01-16 03:32:12 UTC) #70

kmadhusu

brettw@: Can you do the OWNER's check for webkit/* changes? Thanks.

7 years, 11 months ago (2013-01-16 03:34:51 UTC) #71

kmadhusu

On 2013/01/17 01:00:17, tony wrote: > src/webkit LGTM. Why did the win trybot fail to ...

7 years, 11 months ago (2013-01-17 01:03:08 UTC) #73

Ryan Sleevi

On 2013/01/17 01:00:17, tony wrote: > src/webkit LGTM. Why did the win trybot fail to ...

7 years, 11 months ago (2013-01-17 01:03:30 UTC) #74

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11297002/235004

7 years, 11 months ago (2013-01-17 01:07:01 UTC) #75

commit-bot: I haz the power

Failed to apply patch for chrome/browser/system_monitor/removable_storage_notifications.h: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...

7 years, 11 months ago (2013-01-17 02:46:35 UTC) #76

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11297002/229008

7 years, 11 months ago (2013-01-17 07:30:45 UTC) #77

commit-bot: I haz the power

Retried try job too often on win_rel for step(s) base_unittests, browser_tests, cacheinvalidation_unittests, check_deps, chrome_frame_net_tests, chrome_frame_unittests, ...

7 years, 11 months ago (2013-01-17 09:19:11 UTC) #78

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11297002/244008

7 years, 11 months ago (2013-01-17 18:35:10 UTC) #79

commit-bot: I haz the power

Retried try job too often on mac_rel for step(s) browser_tests

7 years, 11 months ago (2013-01-17 20:22:25 UTC) #80

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11297002/244008

7 years, 11 months ago (2013-01-17 20:26:43 UTC) #81

Message was sent while issue was closed.

Change committed as 177517

Issue 11297002: [Media Gallery] Added code to support mtp device media file system on Windows. (Closed)

Description

Patch Set 1 : diff against codereview.chromium.org/11088012 #

Patch Set 2 : : Moved worker classes from MtpDeviceDelegateImpl to separate files #

Patch Set 3 : Addressed review comments #

Patch Set 4 : Addressed review comments #

Patch Set 5 : Addressed review comments #

Patch Set 6 : Addressed review comments #

Patch Set 7 : Rebase #

Patch Set 8 : Rebase + Addressed review comments #

Patch Set 9 : Add "private:" in mtp_device_operations_util.h #

Patch Set 10 : Removed DCHECK, added lock in PortableDeviceWatcherWin and fixed tests. #

Patch Set 11 : Addressed review comments #

Patch Set 12 : Rebase + Addressed review comment #

Patch Set 13 : Rebase + Fixed merge conflict + Addressed review comments #

Patch Set 14 : Modified LazyInit() to handle GetDeviceStorageInfo failure #

Patch Set 15 : Addressed review comments (Removed MTPGetStorageInfoWorker) #

Patch Set 16 : Fixed class comments and removed file comments #

Patch Set 17 : Addressed review comments #

Patch Set 18 : Added a chrome_switch and fixed RecursiveMTPDeviceObjectEnumerator. #

Patch Set 19 : Rebased + Improved RecursiveMTPDeviceObjectEnumerator code. #

Patch Set 20 : Addressed review comment and disabled MediaFileSystemRegistryTest.GalleryNameMTP #

Patch Set 21 : Addressed review comments #

Patch Set 22 : Rebase + IWYU #

Patch Set 23 : Addressed review comments #

Patch Set 24 : Addressed comments #

Patch Set 25 : Updated copyright details #

Patch Set 26 : Rebase #

Patch Set 27 : Rebase + Fixed win compile error by implementing GetMTPStorageInfoFromDeviceId in TestStorageNotifi… #

Messages