Have FindContainerScanResults() find broader media container directories

chrome/browser/media_galleries/media_scan_manager.cc

 // Copyright 2014 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_galleries/media_scan_manager.h"
 
 #include "base/file_util.h"
 #include "base/files/file_enumerator.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
@@ skipping 192 matching lines @@
                             MediaGalleryPrefInfo::kScanResult,
                             gallery.volume_label, gallery.vendor_name,
                             gallery.model_name, gallery.total_size_in_bytes,
                             gallery.last_attach_time, file_counts.audio_count,
                             file_counts.image_count, file_counts.video_count);
   }
   UMA_HISTOGRAM_COUNTS_10000("MediaGalleries.ScanGalleriesPopulated",
                              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();
+struct ContainerCount {
+  int seen_count, entries_count;
+  bool is_qualified;
 
-    // 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;
-}
+  ContainerCount() : seen_count(0), entries_count(-1), is_qualified(false) {}
+};
 
 int CountScanResultsForExtension(MediaGalleriesPreferences* preferences,
                                  const extensions::Extension* extension,
                                  MediaGalleryScanResult* file_counts) {
   int gallery_count = 0;
 
   MediaGalleryPrefIdSet permitted_galleries =
       preferences->GalleriesForExtension(*extension);
   const MediaGalleriesPrefInfoMap& known_galleries =
       preferences->known_galleries();
@@ skipping 125 matching lines @@
                              base::Time::Now() - scan_start_time_);
     scan_start_time_ = base::Time();
   }
 }
 
 void MediaScanManager::SetMediaFolderFinderFactory(
     const MediaFolderFinderFactory& factory) {
   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);
+  }
+  // Find parent directories with majority of media directories,
+  // or container directories.
+  // |candidates| keeps track of directories which might have enough
+  // media directories to have us return them.
+  typedef std::map<base::FilePath, ContainerCount> ContainerCandidates;
+  ContainerCandidates candidates;
+  // |candidates_to_check| are members of |candidates| that have crossed
+  // threshold to be returned, and whose parents still need to be checked.
+  std::set<base::FilePath> candidates_to_check;
+  MediaFolderFinder::MediaFolderFinderResults::const_iterator folder_it =
+      found_folders.begin();
+  while (folder_it != found_folders.end() || !candidates_to_check.empty()) {
+    base::FilePath candidate;
+    // Go through incoming |found_folders| first, then discovered candidates.
+    if (folder_it != found_folders.end()) {
+      candidate = folder_it->first;
+      ++folder_it;
+    } else {
+      candidate = *candidates_to_check.begin();
+      // Remove in case it gets added back.
+      candidates_to_check.erase(candidates_to_check.begin());
+    }
+    base::FilePath parent_directory = candidate.DirName();
+
+    // Skip sensitive folders and their ancestors.
+    bool is_sensitive = false;

[vandebo (ex-Chrome), 2014/05/27 19:07:16]

nit: move this to line 409

[Kevin Bailey, 2014/05/27 21:04:36]

Done.

+    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;
+        break;
+      }
+    }
+    if (is_sensitive)
+      continue;
+    // Don't bother with ones we already have.

[vandebo (ex-Chrome), 2014/05/27 19:07:16]

nit: add a blank line above comment.

[Kevin Bailey, 2014/05/27 21:04:36]

Done.

+    if (found_folders.find(parent_directory) != found_folders.end())
+      continue;
+    ContainerCandidates::iterator parent_it = candidates.find(parent_directory);

[vandebo (ex-Chrome), 2014/05/27 19:07:16]

nit: add a blank line, otherwise at first glance this appears to be part of the
previous block which ignores dirs.

[Kevin Bailey, 2014/05/27 21:04:36]

Done.

+    if (parent_it == candidates.end()) {
+      candidates[parent_directory].seen_count = 1;
+      continue;
+    }
+    ContainerCount* parent_counts = &parent_it->second;
+    ++parent_counts->seen_count;
+    // If a parent directory has more than one scan result, consider it.

[vandebo (ex-Chrome), 2014/05/27 19:07:16]

nit: this comment should probably go up on line 421?

[Kevin Bailey, 2014/05/27 21:04:36]

I was attempting to say, "Since at this point it has more than 1 occurrence,
let's consider it." (The code above it doesn't represent this, so I put the
comment below it.) Let me know if the new comment is any better. Happy to change
it.

+    // If we haven't scanned it yet, do so.

[vandebo (ex-Chrome), 2014/05/27 19:07:16]

nit: scanned is a fairly generic term.. => If needed, count the number of
directory entries.

[Kevin Bailey, 2014/05/27 21:04:36]

Done with respect to the previous comment.

+    if (parent_counts->entries_count == -1) {
+      parent_counts->entries_count = 0;

[vandebo (ex-Chrome), 2014/05/27 19:07:16]

nit: Consider putting this into a helper function in order to shorten up this
function.

[Kevin Bailey, 2014/05/27 21:04:36]

Done. Didn't spot anything in base/files already doing it.

+      base::FileEnumerator dir_counter(parent_directory, false /*recursive*/,
+                                       base::FileEnumerator::DIRECTORIES);
+      base::FileEnumerator::FileInfo info;
+      for (base::FilePath name = dir_counter.Next();
+           !name.empty();
+           name = dir_counter.Next()) {
+        if (!base::IsLink(name))
+          ++parent_counts->entries_count;
+      }
+    }
+    if (parent_it->second.seen_count * 100 / parent_it->second.entries_count
+        >= kContainerDirectoryMinimumPercent) {
+      // It's a qualified candidate now.
+      parent_it->second.is_qualified = true;
+      candidates_to_check.insert(parent_directory);
+    }
+  }
+  MediaFolderFinder::MediaFolderFinderResults result;
+  // Copy and return worthy results.
+  for (ContainerCandidates::const_iterator it = candidates.begin();
+       it != candidates.end(); ++it) {
+    if (it->second.is_qualified)
+      result[it->first] = MediaGalleryScanResult();
+  }
+  return result;
+}
+
 MediaScanManager::ScanObservers::ScanObservers() : observer(NULL) {}
 MediaScanManager::ScanObservers::~ScanObservers() {}
 
 void MediaScanManager::OnExtensionUnloaded(
     content::BrowserContext* browser_context,
     const extensions::Extension* extension,
     extensions::UnloadedExtensionInfo::Reason reason) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   CancelScan(Profile::FromBrowserContext(browser_context), extension);
 }
@@ skipping 72 matching lines @@
                                                            gallery_count,
                                                            file_counts);
       }
     }
     scanning_extensions->clear();
     preferences->SetLastScanCompletionTime(base::Time::Now());
   }
   scoped_extension_registry_observer_.RemoveAll();
   folder_finder_.reset();
 }