Have FindContainerScanResults() find broader media container directories

chrome/browser/media_galleries/media_scan_manager.cc

Index: chrome/browser/media_galleries/media_scan_manager.cc
diff --git a/chrome/browser/media_galleries/media_scan_manager.cc b/chrome/browser/media_galleries/media_scan_manager.cc
index b631a682e3656e8b639d105151d1246641d74da5..8d3e19e6d1160ff6c7bb729399dcb851540e7d64 100644
--- a/chrome/browser/media_galleries/media_scan_manager.cc
+++ b/chrome/browser/media_galleries/media_scan_manager.cc
@@ -210,75 +210,6 @@ void AddScanResultsForProfile(
                              unique_found_folders.size() + to_update.size());
 }
 
-// A single directory may contain many folders with media in them, without
-// containing any media itself. In fact, the primary purpose of that directory
-// may be to contain media directories. This function tries to find those
-// immediate container directories.
-MediaFolderFinder::MediaFolderFinderResults FindContainerScanResults(
-    const MediaFolderFinder::MediaFolderFinderResults& found_folders,
-    const std::vector<base::FilePath>& sensitive_locations) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
-  std::vector<base::FilePath> abs_sensitive_locations;
-  for (size_t i = 0; i < sensitive_locations.size(); ++i) {
-    base::FilePath path = base::MakeAbsoluteFilePath(sensitive_locations[i]);
-    if (!path.empty())
-      abs_sensitive_locations.push_back(path);
-  }
-  // Count the number of scan results with the same parent directory.
-  typedef std::map<base::FilePath, int /*count*/> ContainerCandidates;
-  ContainerCandidates candidates;
-  for (MediaFolderFinder::MediaFolderFinderResults::const_iterator it =
-           found_folders.begin(); it != found_folders.end(); ++it) {
-    base::FilePath parent_directory = it->first.DirName();
-
-    // Skip sensitive folders and their ancestors.
-    bool is_sensitive = false;
-    base::FilePath abs_parent_directory =
-        base::MakeAbsoluteFilePath(parent_directory);
-    if (abs_parent_directory.empty())
-      continue;
-    for (size_t i = 0; i < abs_sensitive_locations.size(); ++i) {
-      if (abs_parent_directory == abs_sensitive_locations[i] ||
-          abs_parent_directory.IsParent(abs_sensitive_locations[i])) {
-        is_sensitive = true;
-        continue;
-      }
-    }
-    if (is_sensitive)
-      continue;
-
-    ContainerCandidates::iterator existing = candidates.find(parent_directory);
-    if (existing == candidates.end()) {
-      candidates[parent_directory] = 1;
-    } else {
-      existing->second++;
-    }
-  }
-
-  // If a parent directory has more than one scan result, consider it.
-  MediaFolderFinder::MediaFolderFinderResults result;
-  for (ContainerCandidates::const_iterator it = candidates.begin();
-       it != candidates.end();
-       ++it) {
-    if (it->second <= 1)
-      continue;
-
-    base::FileEnumerator dir_counter(it->first, false /*recursive*/,
-                                     base::FileEnumerator::DIRECTORIES);
-    base::FileEnumerator::FileInfo info;
-    int count = 0;
-    for (base::FilePath name = dir_counter.Next();
-         !name.empty();
-         name = dir_counter.Next()) {
-      if (!base::IsLink(name))
-        count++;
-    }
-    if (it->second * 100 / count >= kContainerDirectoryMinimumPercent)
-      result[it->first] = MediaGalleryScanResult();
-  }
-  return result;
-}
-
 int CountScanResultsForExtension(MediaGalleriesPreferences* preferences,
                                  const extensions::Extension* extension,
                                  MediaGalleryScanResult* file_counts) {
@@ -302,6 +233,29 @@ int CountScanResultsForExtension(MediaGalleriesPreferences* preferences,
   return gallery_count;
 }
 
+int CountDirectoryEntries(const base::FilePath& path) {
+  base::FileEnumerator dir_counter(path, false /*recursive*/,
+                                   base::FileEnumerator::DIRECTORIES);
+  int count = 0;
+  base::FileEnumerator::FileInfo info;
+  for (base::FilePath name = dir_counter.Next();
+       !name.empty();
+       name = dir_counter.Next()) {
+    if (!base::IsLink(name))
+      ++count;
+  }
+  return count;
+}
+
+struct ContainerCount {
+  int seen_count, entries_count;
+  bool is_qualified;
+
+  ContainerCount() : seen_count(0), entries_count(-1), is_qualified(false) {}
+};
+
+typedef std::map<base::FilePath, ContainerCount> ContainerCandidates;
+
 }  // namespace
 
 MediaScanManager::MediaScanManager()
@@ -423,6 +377,89 @@ void MediaScanManager::SetMediaFolderFinderFactory(
   testing_folder_finder_factory_ = factory;
 }
 
+// A single directory may contain many folders with media in them, without
+// containing any media itself. In fact, the primary purpose of that directory
+// may be to contain media directories. This function tries to find those
+// container directories.
+MediaFolderFinder::MediaFolderFinderResults
+MediaScanManager::FindContainerScanResults(
+    const MediaFolderFinder::MediaFolderFinderResults& found_folders,
+    const std::vector<base::FilePath>& sensitive_locations) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
+  std::vector<base::FilePath> abs_sensitive_locations;
+  for (size_t i = 0; i < sensitive_locations.size(); ++i) {
+    base::FilePath path = base::MakeAbsoluteFilePath(sensitive_locations[i]);
+    if (!path.empty())
+      abs_sensitive_locations.push_back(path);
+  }
+  // Recursively find parent directories with majority of media directories,
+  // or container directories.
+  // |candidates| keeps track of directories which might have enough
+  // such directories to have us return them.
+  typedef std::map<base::FilePath, ContainerCount> ContainerCandidates;
+  ContainerCandidates candidates;
+  for (MediaFolderFinder::MediaFolderFinderResults::const_iterator it =
+           found_folders.begin(); it != found_folders.end(); ++it) {
+    base::FilePath child_directory = it->first;
+    base::FilePath parent_directory = child_directory.DirName();
+
+    // Parent of root is root.
+    while (!parent_directory.empty() && child_directory != parent_directory) {
+      // Skip sensitive folders and their ancestors.
+      base::FilePath abs_parent_directory =
+          base::MakeAbsoluteFilePath(parent_directory);
+      if (abs_parent_directory.empty())
+        break;
+      bool is_sensitive = false;
+      for (size_t i = 0; i < abs_sensitive_locations.size(); ++i) {
+        if (abs_parent_directory == abs_sensitive_locations[i] ||
+            abs_parent_directory.IsParent(abs_sensitive_locations[i])) {
+          is_sensitive = true;
+          break;
+        }
+      }
+      if (is_sensitive)
+        break;
+
+      // Don't bother with ones we already have.
+      if (found_folders.find(parent_directory) != found_folders.end())
+        continue;
+
+      ContainerCandidates::iterator parent_it =
+          candidates.find(parent_directory);
+      if (parent_it == candidates.end()) {
+        ContainerCount count;
+        count.seen_count = 1;
+        count.entries_count = CountDirectoryEntries(parent_directory);
+        parent_it = candidates.insert(std::make_pair(parent_directory,
+                                                     count)).first;
+      } else {
+        ++candidates[parent_directory].seen_count;
+      }
+      // If already sufficient, bail.
+      if (parent_it->second.is_qualified)
+        break;
+      // If now sufficient, mark qualified and check parent.
+      if (parent_it->second.seen_count * 100 / parent_it->second.entries_count
+          >= kContainerDirectoryMinimumPercent) {
+        parent_it->second.is_qualified = true;
+      } else {
+        break;

[vandebo (ex-Chrome), 2014/06/03 18:08:31]

nit: This else break construct is kind of hard to read / ugly. I was hoping
you'd take care of it with the early exit. Can you make the early exit:

if (parent_it->second.is_qualified || parent_it->second.seen_count * 100 /
parent_it->second.entries_count < kContainerDirectoryMinimumPercent) {
  break;
}

Then line 445 joins up with line 449,450.

[Kevin Bailey, 2014/06/03 18:31:25]

Couldn't make it *exactly* the same due to line length. Hope it's ok.

[vandebo (ex-Chrome), 2014/06/03 18:42:31]

Assumed... as is, it violated style guide. I'd suggest:
      if (parent_it->second.is_qualified || 
          parent_it->second.seen_count * 100 / parent_it->second.entries_count <
             kContainerDirectoryMinimumPercent) {

You can also see what git cl format suggests.

+      }
+      child_directory = parent_directory;
+      parent_directory = child_directory.DirName();
+    }
+  }
+  MediaFolderFinder::MediaFolderFinderResults result;
+  // Copy and return worthy results.
+  for (ContainerCandidates::const_iterator it = candidates.begin();
+       it != candidates.end(); ++it) {
+    if (it->second.is_qualified && it->second.seen_count >= 2)
+      result[it->first] = MediaGalleryScanResult();
+  }
+  return result;
+}
+
 MediaScanManager::ScanObservers::ScanObservers() : observer(NULL) {}
 MediaScanManager::ScanObservers::~ScanObservers() {}