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

chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/media_gallery/linux/mtp_recursive_device_object_enumerator.h"
 
 #include "base/sequenced_task_runner.h"
 #include "base/synchronization/waitable_event.h"
 #include "chrome/browser/media_gallery/linux/mtp_device_object_enumerator.h"
 #include "chrome/browser/media_gallery/linux/mtp_read_directory_worker.h"
 
 namespace chrome {
 
 MTPRecursiveDeviceObjectEnumerator::MTPRecursiveDeviceObjectEnumerator(
     const std::string& handle,
     base::SequencedTaskRunner* task_runner,
     const std::vector<MtpFileEntry>& entries,
     base::WaitableEvent* task_completed_event,
     base::WaitableEvent* shutdown_event)
     : device_handle_(handle),
       media_task_runner_(task_runner),
-      file_entries_(entries),
-      file_entry_iter_(file_entries_.begin()),
       on_task_completed_event_(task_completed_event),
       on_shutdown_event_(shutdown_event) {
   DCHECK(on_task_completed_event_);
   DCHECK(on_shutdown_event_);

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

Can you also add a DCHECK to make sure |entries| is non-empty? If it's empty,
it's been used wrong.

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

Done.

   current_enumerator_.reset(new MTPDeviceObjectEnumerator(entries));
 }
 
 MTPRecursiveDeviceObjectEnumerator::~MTPRecursiveDeviceObjectEnumerator() {
 }
 
 FilePath MTPRecursiveDeviceObjectEnumerator::Next() {
   if (on_shutdown_event_->IsSignaled()) {
     // Process is in shut down mode.
     return FilePath();
   }
 
-  FilePath path = current_enumerator_->Next();
-  if (!path.empty())
-    return path;
+  if (!static_cast<MTPDeviceObjectEnumerator*>(
+          current_enumerator_.get())->HasMoreEntries())

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

If you call Next() after you reach the end, you are casting an
EmptyFileEnumerator to a MTPDeviceObjectEnumerator.

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

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.

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

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.

+    UpdateEnumeratorToTraverseSubdirectory();
 
-  // We reached the end.
-  if (file_entry_iter_ == file_entries_.end())
-    return FilePath();
-
-  // Enumerate subdirectories of the next media file entry.
-  MtpFileEntry next_file_entry = *file_entry_iter_;
-  ++file_entry_iter_;
-
-  // Create a MTPReadDirectoryWorker object to enumerate sub directories.
-  scoped_refptr<MTPReadDirectoryWorker> worker(new MTPReadDirectoryWorker(
-      device_handle_, next_file_entry.item_id(), media_task_runner_,
-      on_task_completed_event_, on_shutdown_event_));
-  worker->Run();
-  if (!worker->get_file_entries().empty()) {
-    current_enumerator_.reset(
-        new MTPDeviceObjectEnumerator(worker->get_file_entries()));
-  } else {
-    current_enumerator_.reset(
-        new fileapi::FileSystemFileUtil::EmptyFileEnumerator());
+  if (IsDirectory()) {
+    // If the current entry is a directory, add it to
+    // |untraversed_directory_entry_ids_| to scan after scanning this directory.
+    DirectoryEntryId dir_entry_id;
+    if (static_cast<MTPDeviceObjectEnumerator*>(
+            current_enumerator_.get())->GetEntryId(&dir_entry_id))
+      untraversed_directory_entry_ids_.push(dir_entry_id);
   }
   return current_enumerator_->Next();
 }
 
 int64 MTPRecursiveDeviceObjectEnumerator::Size() {
   return current_enumerator_->Size();
 }
 
 bool MTPRecursiveDeviceObjectEnumerator::IsDirectory() {
   return current_enumerator_->IsDirectory();
 }
 
 base::Time MTPRecursiveDeviceObjectEnumerator::LastModifiedTime() {
   return current_enumerator_->LastModifiedTime();
 }
 
+void MTPRecursiveDeviceObjectEnumerator::
+UpdateEnumeratorToTraverseSubdirectory() {
+  bool has_more_entries = false;
+  while (!untraversed_directory_entry_ids_.empty()) {
+    // Create a MTPReadDirectoryWorker object to enumerate sub directories.
+    DirectoryEntryId dir_entry_id = untraversed_directory_entry_ids_.front();
+    untraversed_directory_entry_ids_.pop();
+    scoped_refptr<MTPReadDirectoryWorker> worker(new MTPReadDirectoryWorker(
+        device_handle_, dir_entry_id, media_task_runner_,
+        on_task_completed_event_, on_shutdown_event_));
+    worker->Run();
+    if (!worker->get_file_entries().empty()) {
+      current_enumerator_.reset(
+          new MTPDeviceObjectEnumerator(worker->get_file_entries()));
+      has_more_entries = true;
+      break;

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

you can just return here and get rid of |has_more entries|.

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

Done.

+    }
+  }
+
+  if (!has_more_entries) {
+    // Reached the end. Traversed all the sub directories.
+    current_enumerator_.reset(
+        new fileapi::FileSystemFileUtil::EmptyFileEnumerator());
+  }
+}
+
 }  // namespace chrome