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();
   for (MediaGalleriesPrefInfoMap::const_iterator it = known_galleries.begin();
        it != known_galleries.end();
        ++it) {
     if (it->second.type == MediaGalleryPrefInfo::kScanResult &&
         !ContainsKey(permitted_galleries, it->first)) {
       gallery_count++;
       file_counts->audio_count += it->second.audio_count;
       file_counts->image_count += it->second.image_count;
       file_counts->video_count += it->second.video_count;
     }
   }
   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;
+}
+
 }  // namespace
 
 MediaScanManager::MediaScanManager()
     : scoped_extension_registry_observer_(this),
       weak_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 }
 
 MediaScanManager::~MediaScanManager() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ skipping 101 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,

[vandebo (ex-Chrome), 2014/06/02 16:29:25]

Recursively find... ?

[Kevin Bailey, 2014/06/02 21:03:43]

Done.

+  // 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;
+  for (MediaFolderFinder::MediaFolderFinderResults::const_iterator it =
+           found_folders.begin(); it != found_folders.end(); ++it) {
+    base::FilePath candidate = it->first;

[vandebo (ex-Chrome), 2014/06/02 16:29:25]

nit: it->first is already in, it's the parent that's the candidate.

Maybe s/candidate/media_dir/ s/parent_directory/candidate/

[Kevin Bailey, 2014/06/02 21:03:43]

Indeed, I reused the name. But since it follows |parent_directory| up the stack,
maybe |child_directory| is better?

+    base::FilePath parent_directory = candidate.DirName();
+
+    // Parent of root is root.
+    while (!parent_directory.empty() && candidate != 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 was insufficient, but is now sufficient, mark qualified and
+      // check parent.
+      if ((parent_it->second.seen_count - 1) * 100 /

[vandebo (ex-Chrome), 2014/06/02 16:29:25]

Instead of doing the extra math, just check that it wasn't already qualified;
better yet, do an early exit if it was.
if (parent_it->second.is_qualified)
  break;
...

[Kevin Bailey, 2014/06/02 21:03:43]

Done. For some reason, I thought it wouldn't work for a newly added entry.

[vandebo (ex-Chrome), 2014/06/03 16:27:21]

You didn't do the early exit, which makes things easier to scan.

[Kevin Bailey, 2014/06/03 18:01:20]

Done.

+          parent_it->second.entries_count < kContainerDirectoryMinimumPercent &&
+          parent_it->second.seen_count * 100 / parent_it->second.entries_count
+          >= kContainerDirectoryMinimumPercent) {
+        parent_it->second.is_qualified = true;
+      } else {
+        break;
+      }
+      candidate = parent_directory;
+      parent_directory = candidate.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)

[vandebo (ex-Chrome), 2014/06/02 16:29:25]

Are you sure this is sufficient? It seems that you'd need to check the qualified
flag all the way back down to the media directory.  (I think you can compute it
on the to the root).

i.e.

R/
R/X/P/A/media
R/X/P/B/media
R/X/P/C/media
R/X/M
R/X/N
R/Y/Q/D/media
R/Y/Q/E/media
R/Y/Q/F/media
R/Y/I
R/Y/J

P and Q are clearly containers, but I think this code will find R as a
container, when it's not.

[Kevin Bailey, 2014/06/02 21:03:43]

Neither X nor Y will be qualified, due to 80% rule, so we'll never even visit R.

If you thought the rule was 60%, I would argue that R would be a container under
those rules.

[vandebo (ex-Chrome), 2014/06/03 16:27:21]

Hmm, indeed.  SG then.

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