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

[kmad...@chromium.org]

(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_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: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_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/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?

https://codereview.chromium.org/11297002/

[rsl...@chromium.org]

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());

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

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: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=Nonmember,_Static_Member,_and_Global_Functions#Nonmember,_Static_Member,_and_Global_Functions

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

https://codereview.chromium.org/11297002/

[kmad...@chromium.org]

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:
> 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=Nonmember%2C_Static_Member%2C_and_Global_Functions#Nonmember%2C_Static_Member%2C_and_Global_Functions

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

https://codereview.chromium.org/11297002/

[kmad...@chromium.org]

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
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_Comments#Class_Comments

> 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_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#newcode31
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_gallery/win/mtp_device_operations_util.cc#newcode93
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_gallery/win/mtp_device_operations_util.cc#newcode165
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_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);

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmember%252C_Static_Member%252C_and_Global_Functions#Nonmember%252C_Static_Member%252C_and_Global_Functions

> >
> > "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_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/118026/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc#newcode70
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_gallery/win/mtp_device_delegate_impl_win.cc#newcode87
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.

https://codereview.chromium.org/11297002/

[pkas...@chromium.org]

I'm going to be gone for a week+ starting tonight, so at least for now I'm
going
to leave final review to Ryan, and if things aren't closed when I get back
I'll
do more review.

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 03:19:40, kmadhusu wrote:
> Fixed. Created free functions in the chrome namespace.

If you're going to use namespaces, consider putting these in their own
namespace rather than the chrome namespace (or perhaps in their own
namespace inside the chrome namespace) since they're all related to each
other. But brettw understands how we're trying to use namespaces better
than I do so you may want to ask him.

https://codereview.chromium.org/11297002/

[kmad...@chromium.org]

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:
> On 2013/01/08 03:19:40, kmadhusu wrote:
> > Fixed. Created free functions in the chrome namespace.

> If you're going to use namespaces, consider putting these in their own
namespace
> rather than the chrome namespace (or perhaps in their own namespace
inside the
> chrome namespace) since they're all related to each other. But brettw
> understands how we're trying to use namespaces better than I do so you
may want
> to ask him.

brettw@ is OOO today. I have sent an email to him regarding this.

https://codereview.chromium.org/11297002/

[rsl...@chromium.org]

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_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 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_Comments#Class_Comments
> >
> > 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_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/118026/chrome/browser/media_gallery/win/mtp_device_object_entry.h#newcode18
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_gallery/win/mtp_device_object_enumerator.h
File chrome/browser/media_gallery/win/mtp_device_object_enumerator.h
(right):

https://codereview.chromium.org/11297002/diff/118026/chrome/browser/media_gallery/win/mtp_device_object_enumerator.h#newcode20
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.

https://codereview.chromium.org/11297002/

[kmad...@chromium.org]

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 is instantiated per MTP device partition.

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_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/118026/chrome/browser/media_gallery/win/mtp_device_object_entry.h#newcode18
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_gallery/win/mtp_device_object_enumerator.h
File chrome/browser/media_gallery/win/mtp_device_object_enumerator.h
(right):

https://codereview.chromium.org/11297002/diff/118026/chrome/browser/media_gallery/win/mtp_device_object_enumerator.h#newcode20
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.

https://codereview.chromium.org/11297002/

[kmad...@chromium.org]

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:

> On 2013/01/08 04:34:01, Peter Kasting wrote:
> > On 2013/01/08 03:19:40, kmadhusu wrote:

> > > Fixed. Created free functions in the chrome namespace.
> >
> > If you're going to use namespaces, consider putting these in their
own
> namespace
> > rather than the chrome namespace (or perhaps in their own namespace
inside the
> > chrome namespace) since they're all related to each other. But
brettw
> > understands how we're trying to use namespaces better than I do so
you may
> want
> > to ask him.

> brettw@ is OOO today. I have sent an email to him regarding this.

rsleevi@: brettw@ replied that he is a bit overloaded. He asked us to do
what is most clear for the project. I am fine with putting these
functions in their own namespace (within the chrome namespace).

namespace chrome {
namespace MTP_device_operations_utils {
OpenDevice(...);
...
} // namespace MTP_device_operations_utils
} // namespace chrome

WDYT?

https://codereview.chromium.org/11297002/

[rsl...@chromium.org]

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

While I have a few code suggestions, I trust you'll make an appropriate
decision
after considering them, and they are so minor that there's no need for
re-review.

This LGTM for the most part, but I am *very concerned* about blocking an
entire
thread to copy a file off an MTP device. This is something that should
absolutely be moved to an IMoniker/async style system ASAP.

It's unclear to me whether this CL enables the API for actual client
applications, or just includes it as part of the compilation unit but
behind an
appropriate feature flag. If it is getting enabled, the file copy issue
(indicated by FUTURE/DESIGN) would be enough for me to NACK this change
until it
was addressed asynchronously.


https://codereview.chromium.org/11297002/diff/125057/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/125057/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc#newcode35
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:35: //
MTP device delegate on the blocking pool thread.
nit: s/blocking pool thread/media task runner/

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc#newcode69
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:69: //
delegate on the blocking pool thread.
nit: s/blocking pool thread/media task runner/

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc#newcode76
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:76:
DCHECK(media_task_runner &&
media_task_runner->RunsTasksOnCurrentThread());
nit: Split this into two DCHECKs, based on the rest of the file/CL using
per-condition DCHECKs.

Same on line 86 as well.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc#newcode171
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc:171: //
Users will use HTML5 FileSystem Entry getMetadata() interface to get
actual
nit: s/get actual/get the actual/

https://codereview.chromium.org/11297002/diff/125057/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/125057/chrome/browser/media_gallery/win/mtp_device_object_entry.h#newcode10
chrome/browser/media_gallery/win/mtp_device_object_entry.h:10: #include
"base/string16.h"
IWYU: You should add
#include "base/basictypes.h"

(for int64)

https://codereview.chromium.org/11297002/diff/125057/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/125057/chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc#newcode31
chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc:31:
++object_entry_iter_;
*(object_entry_iter++) ?

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

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gallery/win/mtp_device_object_enumerator.h#newcode23
chrome/browser/media_gallery/win/mtp_device_object_enumerator.h:23: //
pool thread.
s/MTPDeviceObjectEnumerator is created, destroyed, and operated on the
blocking pool thread./MTPDeviceObjectEnumerator may only be used on a
single thread./

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
(right):

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode46
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:46:
GENERIC_READ);
BUG? You aren't checking the HRESULT of any of these methods.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode80
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:80: //
success, returns true and the stream contents are appended to snapshot
file.
comment: What is a "snapshot file" ?

Is this an concept of the FileAPI leaking in (with its snapshot state)?
Why not just say "file" ?

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode93
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:93:
optimal_transfer_size, &read);
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?

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_gallery/win/mtp_device_operations_util.cc#newcode99
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:99: if
((hr != S_OK) && (hr != S_FALSE))
minor nit: the inner parenthesis seem superfluous:

if (hr != S_OK && hr != S_FALSE)

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode139
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:139:
return SUCCEEDED(hr) ? static_cast<const char16*>(buffer) : string16();
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.

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode173
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:173: bool
GetObjectSize(IPortableDeviceValues* properties_values, int64* size) {
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)

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode330
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:330:
DPLOG(ERROR) << "Access denied to open the device";
1) Why only DPLOG for E_ACCESSDENIED
2) DPLOG uses GetLastError - but I thought the COM programming model was
such that the error is instead passed through |hr|?

https://codereview.chromium.org/11297002/diff/125057/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/125057/chrome/browser/media_gallery/win/mtp_device_operations_util.h#newcode24
chrome/browser/media_gallery/win/mtp_device_operations_util.h:24:
namespace media_transfer_protocol { }?
namespace mtp { }? (not a particular fan of this)
namespace media_transfer { }?

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gallery/win/mtp_device_operations_util.h#newcode32
chrome/browser/media_gallery/win/mtp_device_operations_util.h:32: //
|device|. On success, fills in |file_entry_info|. On failure, returns
the
nit: What does it return on success?

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gallery/win/mtp_device_operations_util.h#newcode41
chrome/browser/media_gallery/win/mtp_device_operations_util.h:41: //
|object_entries|. On failure, returns false and |object_entries| are not
grammar: s/are not set/is not set/

While "entries" is plural, it's the name of a singular variable.

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

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc#newcode43
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc:43:
MTPDeviceObjectEntry next_object_entry = *object_entry_iter_;
nit: *(object_entry_iter_++)

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc#newcode54
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc:54:
new MTPDeviceObjectEnumerator(object_entries));
Why do you not go further recursively?

https://codereview.chromium.org/11297002/diff/125057/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/125057/chrome/browser/system_monitor/portable_device_watcher_win.cc#newcode533
chrome/browser/system_monitor/portable_device_watcher_win.cc:533:
DCHECK(device_iter != device_map_.end());
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;

https://codereview.chromium.org/11297002/

[kmad...@chromium.org]

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_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/125057/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.cc#newcode35
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_gallery/win/mtp_device_delegate_impl_win.cc#newcode69
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_gallery/win/mtp_device_delegate_impl_win.cc#newcode76
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_gallery/win/mtp_device_delegate_impl_win.cc#newcode171
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_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/125057/chrome/browser/media_gallery/win/mtp_device_object_entry.h#newcode10
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_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/125057/chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc#newcode31
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_gallery/win/mtp_device_object_enumerator.h
File chrome/browser/media_gallery/win/mtp_device_object_enumerator.h
(right):

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gallery/win/mtp_device_object_enumerator.h#newcode23
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_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/125057/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode46
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_gallery/win/mtp_device_operations_util.cc#newcode80
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_gallery/win/mtp_device_operations_util.cc#newcode93
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_gallery/win/mtp_device_operations_util.cc#newcode99
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_gallery/win/mtp_device_operations_util.cc#newcode139
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_gallery/win/mtp_device_operations_util.cc#newcode173
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_gallery/win/mtp_device_operations_util.cc#newcode330
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_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/125057/chrome/browser/media_gallery/win/mtp_device_operations_util.h#newcode24
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_gallery/win/mtp_device_operations_util.h#newcode32
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_gallery/win/mtp_device_operations_util.h#newcode41
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_gallery/win/recursive_mtp_device_object_enumerator.cc
File
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc
(right):

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc#newcode43
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_gallery/win/recursive_mtp_device_object_enumerator.cc#newcode54
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_monitor/portable_device_watcher_win.cc
File chrome/browser/system_monitor/portable_device_watcher_win.cc
(right):

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/system_monitor/portable_device_watcher_win.cc#newcode533
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.

https://codereview.chromium.org/11297002/

[rsl...@chromium.org]

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_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/125057/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode93
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_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/125057/chrome/browser/media_gallery/win/mtp_device_operations_util.h#newcode24
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.

https://codereview.chromium.org/11297002/

[kmad...@chromium.org]

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
(right):

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode93
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:93:
optimal_transfer_size, &read);

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

https://codereview.chromium.org/11297002/diff/125057/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/125057/chrome/browser/media_gallery/win/mtp_device_operations_util.h#newcode24
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_gallery/win/recursive_mtp_device_object_enumerator.cc
File
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc
(right):

https://codereview.chromium.org/11297002/diff/125057/chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc#newcode54
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.

https://codereview.chromium.org/11297002/

[rsl...@chromium.org]

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, you return true, but fail
to update |*size|.

https://codereview.chromium.org/11297002/

[kmad...@chromium.org]

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:
> BUG? This means if |actual_size| > kint64max, you return true, but
fail to
> update |*size|.

Fixed.

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

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/media_file_system_registry_unittest.cc#newcode789
chrome/browser/media_gallery/media_file_system_registry_unittest.cc:789:
#if defined(SUPPORT_MTP_DEVICE_FILESYSTEM) && !defined(OS_WIN)
On Windows, this test depends on RemovableDeviceNotificationsWindowWin
to get the attached MTP device storage details. I need to refactor
removable_device_notifications_unittest.cc and move the required mock
classes to a common place. I will do this in a follow up CL and enable
this test.

https://codereview.chromium.org/11297002/

[rsl...@chromium.org]

lgtm



https://codereview.chromium.org/11297002/

[the...@chromium.org]

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
This comment is not helpful. It just says foo contains info about foo.
It might be more helpful to know what Windows data structure this is
capturing information from.

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_object_entry.h#newcode26
chrome/browser/media_gallery/win/mtp_device_object_entry.h:26: // The
object identifier, e.g. "o299".
Where does this ID come from?

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_object_entry.h#newcode29
chrome/browser/media_gallery/win/mtp_device_object_entry.h:29: //
Friendly name of the object, e.g. "IMG_9911.jpeg".
Is this always just the file name or the full path?

https://codereview.chromium.org/11297002/diff/178004/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/178004/chrome/browser/system_monitor/portable_device_watcher_win.cc#newcode541
chrome/browser/system_monitor/portable_device_watcher_win.cc:541: break;
You can just return true here and return false at line 544.

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc
File
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc
(right):

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc#newcode665
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:665:
TEST_F(RemovableDeviceNotificationsWindowWinTest, GetMTPStorageInfo) {
nit: isn't this really testing GetMTPStorageInfoFromDeviceId ?

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/system_monitor/removable_storage_notifications.h
File chrome/browser/system_monitor/removable_storage_notifications.h
(right):

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/system_monitor/removable_storage_notifications.h#newcode37
chrome/browser/system_monitor/removable_storage_notifications.h:37: //
interface details and |storage_object_id| with storage object temporary
Both here and in portable_device_watcher_win.h, you use the term
"storage object temporary identifier". At some level, you should define
what a "storage object temporary identifier" is, because it's not a well
known term.

https://codereview.chromium.org/11297002/diff/178004/webkit/fileapi/media/mtp_device_file_system_config.h
File webkit/fileapi/media/mtp_device_file_system_config.h (right):

https://codereview.chromium.org/11297002/diff/178004/webkit/fileapi/media/mtp_device_file_system_config.h#newcode11
webkit/fileapi/media/mtp_device_file_system_config.h:11: // OS_CHROMEOS
implies OS_LINUX.
This comment has the defined values flipped around.

https://codereview.chromium.org/11297002/

[the...@chromium.org]

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 error = GetFileInfo(device_file_path,
file_info);
You should do this before writing the file to disk. Otherwise you can
fail, but wrote a file to disk.

https://codereview.chromium.org/11297002/diff/178004/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/178004/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h#newcode46
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:46: //
Defer the device initializations until the first file operation request.
nit: Defers

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h#newcode81
chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:81: //
The Pnp Device Id, used to open the device for communication,
nit: plug & play abbreviates to PnP.

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

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_object_enumerator.h#newcode43
chrome/browser/media_gallery/win/mtp_device_object_enumerator.h:43:
MTPDeviceObjectEntry current_object_;
const ref

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#newcode66
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:66: bool
GetDeviceObjectEnumerator(IPortableDevice* device,
This can also just return an IEnumPortableDeviceObjectIDs* instead of
using a bool + out param.

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode90
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:90:
DCHECK_GT(optimal_transfer_size, 0u);
I don't see any guarantee from IPortableDeviceResources::GetStream that
this will be true.

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode92
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:92: DWORD
read = 0;
Doesn't IStream::Read return a ULONG? Is a ULONG always equal in size to
a DWORD?

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode106
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:106:
buffer.erase(read);
Can't you avoid manipulating the buffer, and instead just write
std::min(read, buffer.length()) to file?

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 > 0) && SUCCEEDED(hr));
Why do you need the SUCCEEDED() part? You've already checked the value
of |hr| at line 103. SUCCEEDED(hr) is always true here.

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode126
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:126:
return ((content_type == WPD_CONTENT_TYPE_AUDIO_ALBUM) ||
I'm not sure albums (aka playlists??) are directories. Have you tested
this with an audio or video album?

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode283
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:283: bool
get_all_entries,
You can just indicate this with a non empty |object_name| instead.

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode296
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:296:
DWORD num_objects_to_request = 10;
const?

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode297
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:297: for
(HRESULT hr = S_OK; hr == S_OK;) {
Isn't this just do { } while (hr == S_OK) ?

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode378
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:378:
DCHECK(!local_path.value().empty());
!local_path.empty() ?

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode407
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:407:
DCHECK_EQ(1U, object_entries.size());
FWIW, this DCHECK can fail. In MTP, it is perfectly legal to have object
A with id 1 and object B with id 2, and both objects can have the same
parent id and can have the same friendly name. Though this case is
probably rare. I don't think the Linux MTP code handles this situation
gracefully either. You can put a TODO in here with my name and
http://crbug.com/169930.

https://codereview.chromium.org/11297002/diff/178004/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/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.h#newcode31
chrome/browser/media_gallery/win/mtp_device_operations_util.h:31:
base::win::ScopedComPtr<IPortableDevice>* device);
You can return a NULL ScopedComPtr right? If so, there's no need to have
|device| as an out param. Just return NULL to indicate failure.

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.h#newcode53
chrome/browser/media_gallery/win/mtp_device_operations_util.h:53: bool
WriteFileObjectData(IPortableDevice* device,
Can you rename this to WriteFileObjectContentToPath ?
"WriteFileObjectData" sounds like it is trying to save data into a file
object.

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.h#newcode57
chrome/browser/media_gallery/win/mtp_device_operations_util.h:57: //
Returns the identifier of the object specified by the |object_name|.
Is |object_name| the friendly name?

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

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc#newcode39
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc:39:
MTPDeviceObjectEntry current_object_entry = *(object_entry_iter_++);
const ref

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

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h#newcode8
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h:8:
#include <portabledeviceapi.h>
nit: empty line between C and C++ headers. Here and elsewhere.

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h#newcode26
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h:26:
// object entries set. RecursiveMTPDeviceObjectEnumerator communicates
with the
Doesn't it go through MTPDeviceObjectEnumerator?

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h#newcode28
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h:28:
// RecursiveMTPDeviceObjectEnumerator supports media file system
operations.
What does this mean exactly? Are you trying to say it implements
AbstractFileEnumerator?

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h#newcode30
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h:30:
// device is already opened for communication.
Are you trying to say the |device| passed into the ctor should already
be initialized and ready for communication? Shouldn't this statement be
in the ctor comment?

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h#newcode52
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h:52:
// List of current directory object entries.
Can you add a TODO to remove |curr_object_entries_| and
|object_entry_iter_| ?

https://codereview.chromium.org/11297002/diff/178004/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/178004/chrome/browser/system_monitor/portable_device_watcher_win.cc#newcode531
chrome/browser/system_monitor/portable_device_watcher_win.cc:531:
*device_location = storage_map_iter->second.location;
If you delay assigning to |device_location| until line 540, then you can
avoid returning any info in the out params if you return false.

https://codereview.chromium.org/11297002/

[kmad...@chromium.org]

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() || !LazyInit())

On 2013/01/14 23:25:30, Lei Zhang wrote:
> nit: root.empty()

Done.

https://chromiumcodereview.appspot.com/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 error = GetFileInfo(device_file_path,
file_info);

On 2013/01/14 23:25:30, Lei Zhang wrote:
> You should do this before writing the file to disk. Otherwise you can
fail, but
> wrote a file to disk.

Done.

https://chromiumcodereview.appspot.com/11297002/diff/178004/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://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h#newcode46

chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:46: //
Defer the device initializations until the first file operation request.

On 2013/01/14 23:25:30, Lei Zhang wrote:
> nit: Defers

Done.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h#newcode81

chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h:81: //
The Pnp Device Id, used to open the device for communication,

On 2013/01/14 23:25:30, Lei Zhang wrote:
> nit: plug & play abbreviates to PnP.

Done.

https://chromiumcodereview.appspot.com/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://chromiumcodereview.appspot.com/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

On 2013/01/14 20:49:02, Lei Zhang wrote:
> This comment is not helpful. It just says foo contains info about foo.
It might
> be more helpful to know what Windows data structure this is capturing
> information from.

Rephrased the comment to include details about the interface that is
used to retrieve the object property values. MSDN documentation only
provides details about the interface that is used to retrieve the values
and do not specify any data structure name.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_object_entry.h#newcode26

chrome/browser/media_gallery/win/mtp_device_object_entry.h:26: // The
object identifier, e.g. "o299".

On 2013/01/14 20:49:02, Lei Zhang wrote:
> Where does this ID come from?

ID is obtained from IEnumPortableDeviceObjectIDs interface.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_object_entry.h#newcode29

chrome/browser/media_gallery/win/mtp_device_object_entry.h:29: //
Friendly name of the object, e.g. "IMG_9911.jpeg".

On 2013/01/14 20:49:02, Lei Zhang wrote:
> Is this always just the file name or the full path?

This field always store the file name.

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

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_object_enumerator.h#newcode43
chrome/browser/media_gallery/win/mtp_device_object_enumerator.h:43:
MTPDeviceObjectEntry current_object_;
On 2013/01/14 23:25:30, Lei Zhang wrote:
> const ref

This field changes in .cc line 30. Changed to a pointer field.

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#newcode66

chrome/browser/media_gallery/win/mtp_device_operations_util.cc:66: bool
GetDeviceObjectEnumerator(IPortableDevice* device,

On 2013/01/14 23:25:30, Lei Zhang wrote:
> This can also just return an IEnumPortableDeviceObjectIDs* instead of
using a
> bool + out param.

Done and also fixed GetDeviceContent.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode90
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:90:
DCHECK_GT(optimal_transfer_size, 0u);

On 2013/01/14 23:25:30, Lei Zhang wrote:
> I don't see any guarantee from IPortableDeviceResources::GetStream
that this
> will be true.

If the optimal_transfer_size is equal to 0, this function will now
return false.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode92

chrome/browser/media_gallery/win/mtp_device_operations_util.cc:92: DWORD
read = 0;

On 2013/01/14 23:25:30, Lei Zhang wrote:
> Doesn't IStream::Read return a ULONG? Is a ULONG always equal in size
to a
> DWORD?

As per
http://msdn.microsoft.com/en-us/library/windows/desktop/aa383751(v=vs.85).aspx,
both DWORD and ULONG are the same.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode106
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:106:
buffer.erase(read);

On 2013/01/14 23:25:30, Lei Zhang wrote:
> Can't you avoid manipulating the buffer, and instead just write
std::min(read,
> buffer.length()) to file?

Done.

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

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode126

chrome/browser/media_gallery/win/mtp_device_operations_util.cc:126:
return ((content_type == WPD_CONTENT_TYPE_AUDIO_ALBUM) ||

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(v=vs.85).aspx,
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.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode283
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:283: bool
get_all_entries,

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.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode296

chrome/browser/media_gallery/win/mtp_device_operations_util.cc:296:
DWORD num_objects_to_request = 10;

On 2013/01/14 23:25:30, Lei Zhang wrote:
> const?

Done.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode297

chrome/browser/media_gallery/win/mtp_device_operations_util.cc:297: for
(HRESULT hr = S_OK; hr == S_OK;) {

On 2013/01/14 23:25:30, Lei Zhang wrote:
> Isn't this just do { } while (hr == S_OK)

Yes. Leaving it as it is.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode378
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:378:
DCHECK(!local_path.value().empty());
On 2013/01/14 23:25:30, Lei Zhang wrote:
> !local_path.empty() ?

Done.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode407
chrome/browser/media_gallery/win/mtp_device_operations_util.cc:407:
DCHECK_EQ(1U, object_entries.size());

On 2013/01/14 23:25:30, Lei Zhang wrote:
> FWIW, this DCHECK can fail. In MTP, it is perfectly legal to have
object A with
> id 1 and object B with id 2, and both objects can have the same parent
id and
> can have the same friendly name. Though this case is probably rare. I
don't
> think the Linux MTP code handles this situation gracefully either. You
can put a
> TODO in here with my name and http://crbug.com/169930.

Done.

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

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.h#newcode31

chrome/browser/media_gallery/win/mtp_device_operations_util.h:31:
base::win::ScopedComPtr<IPortableDevice>* device);

On 2013/01/14 23:25:30, Lei Zhang wrote:
> You can return a NULL ScopedComPtr right? If so, there's no need to
have
> |device| as an out param. Just return NULL to indicate failure.

Done.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.h#newcode53

chrome/browser/media_gallery/win/mtp_device_operations_util.h:53: bool
WriteFileObjectData(IPortableDevice* device,

On 2013/01/14 23:25:30, Lei Zhang wrote:
> Can you rename this to WriteFileObjectContentToPath ?
"WriteFileObjectData"
> sounds like it is trying to save data into a file object.

Done.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.h#newcode57

chrome/browser/media_gallery/win/mtp_device_operations_util.h:57: //
Returns the identifier of the object specified by the |object_name|.

On 2013/01/14 23:25:30, Lei Zhang wrote:
> Is |object_name| the friendly name?

Yes.

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

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc#newcode39

chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.cc:39:
MTPDeviceObjectEntry current_object_entry = *(object_entry_iter_++);

On 2013/01/14 23:25:30, Lei Zhang wrote:
> const ref

Done.

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

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h#newcode8
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h:8:
#include <portabledeviceapi.h>

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.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h#newcode26

chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h:26:
// object entries set. RecursiveMTPDeviceObjectEnumerator communicates
with the

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.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h#newcode28

chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h:28:
// RecursiveMTPDeviceObjectEnumerator supports media file system
operations.

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.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h#newcode30

chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h:30:
// device is already opened for communication.

On 2013/01/14 23:25:30, Lei Zhang wrote:
> Are you trying to say the |device| passed into the ctor should already
be
> initialized and ready for communication? Shouldn't this statement be
in the ctor
> comment?

Done.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h#newcode52

chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h:52:
// List of current directory object entries.

On 2013/01/14 23:25:30, Lei Zhang wrote:
> Can you add a TODO to remove |curr_object_entries_| and
|object_entry_iter_| ?

Done.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/system_monitor/portable_device_watcher_win.cc
File chrome/browser/system_monitor/portable_device_watcher_win.cc
(right):

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/system_monitor/portable_device_watcher_win.cc#newcode531

chrome/browser/system_monitor/portable_device_watcher_win.cc:531:
*device_location = storage_map_iter->second.location;

On 2013/01/14 23:25:30, Lei Zhang wrote:
> If you delay assigning to |device_location| until line 540, then you
can avoid
> returning any info in the out params if you return false.

Done.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/system_monitor/portable_device_watcher_win.cc#newcode541
chrome/browser/system_monitor/portable_device_watcher_win.cc:541: break;

On 2013/01/14 20:49:02, Lei Zhang wrote:
> You can just return true here and return false at line 544.

Done.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc
File
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc
(right):

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc#newcode665
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:665:
TEST_F(RemovableDeviceNotificationsWindowWinTest, GetMTPStorageInfo) {

On 2013/01/14 20:49:02, Lei Zhang wrote:
> nit: isn't this really testing GetMTPStorageInfoFromDeviceId ?

Done.

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/system_monitor/removable_storage_notifications.h
File chrome/browser/system_monitor/removable_storage_notifications.h
(right):

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/system_monitor/removable_storage_notifications.h#newcode37

chrome/browser/system_monitor/removable_storage_notifications.h:37: //
interface details and |storage_object_id| with storage object temporary

On 2013/01/14 20:49:02, Lei Zhang wrote:
> Both here and in portable_device_watcher_win.h, you use the term
"storage object
> temporary identifier". At some level, you should define what a
"storage object
> temporary identifier" is, because it's not a well known term.

Done.

https://chromiumcodereview.appspot.com/11297002/diff/178004/webkit/fileapi/media/mtp_device_file_system_config.h
File webkit/fileapi/media/mtp_device_file_system_config.h (right):

https://chromiumcodereview.appspot.com/11297002/diff/178004/webkit/fileapi/media/mtp_device_file_system_config.h#newcode11

webkit/fileapi/media/mtp_device_file_system_config.h:11: // OS_CHROMEOS
implies OS_LINUX.

On 2013/01/14 20:49:02, Lei Zhang wrote:
> This comment has the defined values flipped around.

Fixed.

https://chromiumcodereview.appspot.com/11297002/

[the...@chromium.org]

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 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/media_gallery/win/mtp_device_operations_util.cc#newcode126
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.aspx,

> 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/media_gallery/win/mtp_device_operations_util.cc#newcode283
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/media_gallery/win/recursive_mtp_device_object_enumerator.h
File
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h
(right):

https://chromiumcodereview.appspot.com/11297002/diff/178004/chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h#newcode8
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/media_gallery/win/recursive_mtp_device_object_enumerator.h#newcode26
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/media_gallery/win/recursive_mtp_device_object_enumerator.h#newcode28
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" ?

https://chromiumcodereview.appspot.com/11297002/

[the...@chromium.org]

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
chrome/browser/media_gallery/win/mtp_device_object_enumerator.cc:27: if
(object_entry_iter_ == object_entries_.end())
If we reached the end, |current_object_| still points to the last object
and will return its information.

https://chromiumcodereview.appspot.com/11297002/

[kmad...@chromium.org]

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 > 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.aspx
> and

http://msdn.microsoft.com/en-us/library/windows/desktop/ff485842%28v=vs.85%29.aspx

Removed SUCCEEDED(hr).

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/mtp_device_operations_util.cc#newcode126

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%2529.aspx,

> > 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_gallery/win/mtp_device_operations_util.cc#newcode283
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_gallery/win/recursive_mtp_device_object_enumerator.h
File
chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h
(right):

https://codereview.chromium.org/11297002/diff/178004/chrome/browser/media_gallery/win/recursive_mtp_device_object_enumerator.h#newcode8

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_gallery/win/recursive_mtp_device_object_enumerator.h#newcode26

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_gallery/win/recursive_mtp_device_object_enumerator.h#newcode28

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_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/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()) {

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_gallery/win/mtp_device_object_enumerator.cc#newcode27

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.

https://codereview.chromium.org/11297002/

[the...@chromium.org]

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 to mention WPD_CONTENT_TYPE_FUNCTIONAL_OBJECT are
root storage objects?

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));
You don't need extra parenthesis around each comparision, and you can
line up the second line to the first. e.g.

return (foo == bar ||
foo == qux ||
foo == baz);

https://codereview.chromium.org/11297002/

[the...@chromium.org]

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

https://codereview.chromium.org/11297002/

[kmad...@chromium.org]

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:
> still need to initialize |current_object_|.

Done

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));

On 2013/01/15 23:53:37, Lei Zhang wrote:
> Can you add a comment to mention WPD_CONTENT_TYPE_FUNCTIONAL_OBJECT
are root
> storage objects?

Done.

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));

On 2013/01/15 23:53:37, Lei Zhang wrote:
> You don't need extra parenthesis around each comparision, and you can
line up
> the second line to the first. e.g.

> return (foo == bar ||
> foo == qux ||
> foo == baz);

Done.

https://codereview.chromium.org/11297002/

[kmad...@chromium.org]

kinuko@: Please review webkit/* changes.

Thanks.

https://codereview.chromium.org/11297002/

[the...@chromium.org]

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.

https://codereview.chromium.org/11297002/

[the...@chromium.org]

Also, new files are copyright 2013.

https://codereview.chromium.org/11297002/

[kmad...@chromium.org]

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, Lei Zhang wrote:
> BTW, |current_object_| cannot be NULL.

Fixed.

https://chromiumcodereview.appspot.com/11297002/

[kin...@chromium.org]

webkit/ changes lgtm

https://codereview.chromium.org/11297002/

[commi...@chromium.org]

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


https://chromiumcodereview.appspot.com/11297002/

[commi...@chromium.org]

Presubmit check for 11297002-235004 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
webkit

Presubmit checks took 3.1s to calculate.



https://chromiumcodereview.appspot.com/11297002/

[kmad...@chromium.org]

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

Thanks.

https://chromiumcodereview.appspot.com/11297002/