[Linux, Media Gallery] Fix MTPRecursiveDeviceObjectEnumerator to recursively enumerate all the medi…

[Linux, Media Gallery] Fix MTPRecursiveDeviceObjectEnumerator to recursively enumerate all the media file object entries.

BUG=none
TEST=none


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176822

[kmadhusu, 2013-01-11 04:51:39]

https://codereview.chromium.org/11855005/diff/1/chrome/browser/media_gallery/...
File
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc
(left):

https://codereview.chromium.org/11855005/diff/1/chrome/browser/media_gallery/...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc:59:
new MTPDeviceObjectEnumerator(worker->get_file_entries()));
This code was listing only the top 2 levels of the directory contents but not
the entire subtree.

[Lei Zhang, 2013-01-11 06:35:43]

If you just add a function to get the item id in MTPDeviceObjectEnumerator, then
you can drop |file_entries_| and |file_entry_iter_| from
RecursiveDeviceObjectEnumerator. Right now there's a lot of redundancy because
both classes have to keep track of the current file entry list.

https://codereview.chromium.org/11855005/diff/1/chrome/browser/media_gallery/...
File
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc
(right):

https://codereview.chromium.org/11855005/diff/1/chrome/browser/media_gallery/...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc:40:
MaybeUpdateCurrentFileEntryList();
side note: functions named "MaybeSomething" sounds like they are not
deterministic.

Q1: Is it going to do Something?
A: Maybe?

Q2: What conditions govern when it works?
A2: The name doesn't really say.

https://codereview.chromium.org/11855005/diff/1/chrome/browser/media_gallery/...
File chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.h
(right):

https://codereview.chromium.org/11855005/diff/1/chrome/browser/media_gallery/...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.h:65:
// Used to recursively enumerate the sub-directory objects.
Just say "Queue of untraversed directories."

[kmadhusu, 2013-01-11 22:34:22]

Removed |file_entry_iter_| and |file_entries_| from
MTPRecursiveDeviceObjectEnumerator and added HasMoreEntries() and GetEntryId()
functions in MTPDeviceObjectEnumerator. PTAL.

Thanks.

https://codereview.chromium.org/11855005/diff/1/chrome/browser/media_gallery/...
File
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc
(right):

https://codereview.chromium.org/11855005/diff/1/chrome/browser/media_gallery/...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc:40:
MaybeUpdateCurrentFileEntryList();
On 2013/01/11 06:35:44, Lei Zhang wrote:
> side note: functions named "MaybeSomething" sounds like they are not
> deterministic.
> 
> Q1: Is it going to do Something?
> A: Maybe?
> 
> Q2: What conditions govern when it works?
> A2: The name doesn't really say.

Renamed the function and updated the function definition.

https://codereview.chromium.org/11855005/diff/1/chrome/browser/media_gallery/...
File chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.h
(right):

https://codereview.chromium.org/11855005/diff/1/chrome/browser/media_gallery/...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.h:65:
// Used to recursively enumerate the sub-directory objects.
On 2013/01/11 06:35:44, Lei Zhang wrote:
> Just say "Queue of untraversed directories."

Done.

[Lei Zhang, 2013-01-11 23:25:24]

https://codereview.chromium.org/11855005/diff/10002/chrome/browser/media_gall...
File
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc
(right):

https://codereview.chromium.org/11855005/diff/10002/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc:25:
DCHECK(on_shutdown_event_);
Can you also add a DCHECK to make sure |entries| is non-empty? If it's empty,
it's been used wrong.

https://codereview.chromium.org/11855005/diff/10002/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc:39:
current_enumerator_.get())->HasMoreEntries())
If you call Next() after you reach the end, you are casting an
EmptyFileEnumerator to a MTPDeviceObjectEnumerator.

https://codereview.chromium.org/11855005/diff/10002/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc:80:
break;
you can just return here and get rid of |has_more entries|.

[kmadhusu, 2013-01-11 23:52:45]

https://codereview.chromium.org/11855005/diff/10002/chrome/browser/media_gall...
File
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc
(right):

https://codereview.chromium.org/11855005/diff/10002/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc:25:
DCHECK(on_shutdown_event_);
On 2013/01/11 23:25:24, Lei Zhang wrote:
> Can you also add a DCHECK to make sure |entries| is non-empty? If it's empty,
> it's been used wrong.

Done.

https://codereview.chromium.org/11855005/diff/10002/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc:39:
current_enumerator_.get())->HasMoreEntries())
On 2013/01/11 23:25:24, Lei Zhang wrote:
> If you call Next() after you reach the end, you are casting an
> EmptyFileEnumerator to a MTPDeviceObjectEnumerator.

If current_enumerator is set to EmptyFileEnumerator, we will return empty
FilePath @ line 50. Therefore, the use case which you mentioned should not ever
happen. If it happens this interface is not used correctly. To be safe, added a
check to make sure we are not dereferencing a null pointer.

https://codereview.chromium.org/11855005/diff/10002/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc:80:
break;
On 2013/01/11 23:25:24, Lei Zhang wrote:
> you can just return here and get rid of |has_more entries|.

Done.

[Lei Zhang, 2013-01-12 00:08:35]

https://codereview.chromium.org/11855005/diff/10002/chrome/browser/media_gall...
File
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc
(right):

https://codereview.chromium.org/11855005/diff/10002/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc:39:
current_enumerator_.get())->HasMoreEntries())
On 2013/01/11 23:52:45, kmadhusu wrote:
> On 2013/01/11 23:25:24, Lei Zhang wrote:
> > If you call Next() after you reach the end, you are casting an
> > EmptyFileEnumerator to a MTPDeviceObjectEnumerator.
> 
> If current_enumerator is set to EmptyFileEnumerator, we will return empty
> FilePath @ line 50. Therefore, the use case which you mentioned should not
ever
> happen. If it happens this interface is not used correctly. To be safe, added
a
> check to make sure we are not dereferencing a null pointer.

It should never happen, but it can happen. If it does, you can either fail
gracefully, or cause a scary undefined condition. Avoid the latter case.

https://codereview.chromium.org/11855005/diff/13002/chrome/browser/media_gall...
File
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc
(right):

https://codereview.chromium.org/11855005/diff/13002/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc:41:
if (!enumerator) {
I fail to see how this helps. How does |enumerator| ever become NULL?

[kmadhusu, 2013-01-12 01:03:53]

https://codereview.chromium.org/11855005/diff/13002/chrome/browser/media_gall...
File
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc
(right):

https://codereview.chromium.org/11855005/diff/13002/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc:41:
if (!enumerator) {
On 2013/01/12 00:08:36, Lei Zhang wrote:
> I fail to see how this helps. How does |enumerator| ever become NULL?

Removed this check and modified code to handle the failure gracefully using an
explicit empty enumerator.

[Lei Zhang, 2013-01-12 01:48:01]

https://codereview.chromium.org/11855005/diff/19001/chrome/browser/media_gall...
File chrome/browser/media_gallery/linux/mtp_device_object_enumerator.cc (right):

https://codereview.chromium.org/11855005/diff/19001/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_device_object_enumerator.cc:46: *entry_id
= (*file_entry_iter_).item_id();
Can't you write it as: file_entry_iter-_>item_id() ?

https://codereview.chromium.org/11855005/diff/19001/chrome/browser/media_gall...
File chrome/browser/media_gallery/linux/mtp_device_object_enumerator.h (right):

https://codereview.chromium.org/11855005/diff/19001/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_device_object_enumerator.h:35: bool
GetEntryId(uint32_t* entry_id);
const

https://codereview.chromium.org/11855005/diff/19001/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_device_object_enumerator.h:39: bool
HasMoreEntries();
const

https://codereview.chromium.org/11855005/diff/19001/chrome/browser/media_gall...
File
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc
(right):

https://codereview.chromium.org/11855005/diff/19001/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc:41:
if (!current_enumerator_.get())
You can just write if (!current_enumerator_) these days.

https://codereview.chromium.org/11855005/diff/19001/chrome/browser/media_gall...
File chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.h
(right):

https://codereview.chromium.org/11855005/diff/19001/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.h:48:
void UpdateEnumeratorToTraverseSubdirectory();
Comment.

https://codereview.chromium.org/11855005/diff/19001/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.h:48:
void UpdateEnumeratorToTraverseSubdirectory();
Alternatively, return a scoped_ptr<MTPDeviceObjectEnumerator>. Then it's obvious
what's happening to |current_enumerator_|.

[kmadhusu, 2013-01-14 18:47:32]

https://codereview.chromium.org/11855005/diff/19001/chrome/browser/media_gall...
File chrome/browser/media_gallery/linux/mtp_device_object_enumerator.cc (right):

https://codereview.chromium.org/11855005/diff/19001/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_device_object_enumerator.cc:46: *entry_id
= (*file_entry_iter_).item_id();
On 2013/01/12 01:48:01, Lei Zhang wrote:
> Can't you write it as: file_entry_iter-_>item_id() ?

Done.

https://codereview.chromium.org/11855005/diff/19001/chrome/browser/media_gall...
File chrome/browser/media_gallery/linux/mtp_device_object_enumerator.h (right):

https://codereview.chromium.org/11855005/diff/19001/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_device_object_enumerator.h:35: bool
GetEntryId(uint32_t* entry_id);
On 2013/01/12 01:48:01, Lei Zhang wrote:
> const

Done.

https://codereview.chromium.org/11855005/diff/19001/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_device_object_enumerator.h:39: bool
HasMoreEntries();
On 2013/01/12 01:48:01, Lei Zhang wrote:
> const

Done.

https://codereview.chromium.org/11855005/diff/19001/chrome/browser/media_gall...
File
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc
(right):

https://codereview.chromium.org/11855005/diff/19001/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc:41:
if (!current_enumerator_.get())
On 2013/01/12 01:48:01, Lei Zhang wrote:
> You can just write if (!current_enumerator_) these days.

Done.

https://codereview.chromium.org/11855005/diff/19001/chrome/browser/media_gall...
File chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.h
(right):

https://codereview.chromium.org/11855005/diff/19001/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.h:48:
void UpdateEnumeratorToTraverseSubdirectory();
On 2013/01/12 01:48:01, Lei Zhang wrote:
> Alternatively, return a scoped_ptr<MTPDeviceObjectEnumerator>. Then it's
obvious
> what's happening to |current_enumerator_|.

Done.

https://codereview.chromium.org/11855005/diff/19001/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.h:48:
void UpdateEnumeratorToTraverseSubdirectory();
On 2013/01/12 01:48:01, Lei Zhang wrote:
> Comment.

Done.

[Lei Zhang, 2013-01-14 20:04:41]

lgtm

https://codereview.chromium.org/11855005/diff/20005/chrome/browser/media_gall...
File
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc
(right):

https://codereview.chromium.org/11855005/diff/20005/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc:54:
return (current_enumerator_.get()) ?
you can drop the .get() here and below.

[kmadhusu, 2013-01-14 20:28:29]

https://codereview.chromium.org/11855005/diff/20005/chrome/browser/media_gall...
File
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc
(right):

https://codereview.chromium.org/11855005/diff/20005/chrome/browser/media_gall...
chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc:54:
return (current_enumerator_.get()) ?
On 2013/01/14 20:04:41, Lei Zhang wrote:
> you can drop the .get() here and below.

Done.

[commit-bot: I haz the power, 2013-01-14 20:41:28]

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

[commit-bot: I haz the power, 2013-01-15 07:17:53]

Change committed as 176822