Have FindContainerScanResults() attempt to find media "container" directories...

Have FindContainerScanResults() attempt to find media "container" directories as parents of found media folders with sufficient density (which gives us confidence that it is a media container.) This version looks beyond the immediate parents, further up the directory tree.


BUG=

Review URL: https://codereview.chromium.org/285433004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@275264 0039d316-1c4b-4281-b951-d872f2087c98

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,28 @@ 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 +376,90 @@ 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 previously sufficient, or not sufficient, bail.
+      if (parent_it->second.is_qualified ||
+          parent_it->second.seen_count * 100 / parent_it->second.entries_count <
+              kContainerDirectoryMinimumPercent) {
+        break;
+      }
+      // Otherwise, mark qualified and check parent.
+      parent_it->second.is_qualified = true;
+      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() {}
 

chrome/browser/media_galleries/media_scan_manager.h

@@ -53,6 +53,7 @@ class MediaScanManager : public extensions::ExtensionRegistryObserver {
 
  protected:
   friend class MediaGalleriesPlatformAppBrowserTest;
+  friend class MediaScanManagerTest;
 
   typedef base::Callback<MediaFolderFinder*(
       const MediaFolderFinder::MediaFolderFinderResultsCallback&)>
@@ -60,6 +61,11 @@ class MediaScanManager : public extensions::ExtensionRegistryObserver {
 
   void SetMediaFolderFinderFactory(const MediaFolderFinderFactory& factory);
 
+  // Here so that friend MediaScanManagerTest can access it.
+  static MediaFolderFinder::MediaFolderFinderResults FindContainerScanResults(
+      const MediaFolderFinder::MediaFolderFinderResults& found_folders,
+      const std::vector<base::FilePath>& sensitive_locations);
+
  private:
   struct ScanObservers {
     ScanObservers();

chrome/browser/media_galleries/media_scan_manager_unittest.cc

@@ -230,7 +230,7 @@ class MediaScanManagerTest : public MediaScanManagerObserver,
     EXPECT_EQ(video_count, pref_info->second.video_count);
   }
 
-  // MediaScanMangerObserver implementation.
+  // MediaScanManagerObserver implementation.
   virtual void OnScanFinished(
       const std::string& extension_id,
       int gallery_count,
@@ -242,6 +242,15 @@ class MediaScanManagerTest : public MediaScanManagerObserver,
     EXPECT_EQ(expected_file_counts_.video_count, file_counts.video_count);
   }
 
+ protected:
+  // So derived tests can access ...::FindContainerScanResults().
+  MediaFolderFinder::MediaFolderFinderResults FindContainerScanResults(
+      const MediaFolderFinder::MediaFolderFinderResults& found_folders,
+      const std::vector<base::FilePath>& sensitive_locations) {
+    return MediaScanManager::FindContainerScanResults(found_folders,
+                                                      sensitive_locations);
+  }
+
  private:
   void OnFindFoldersStarted(
       MediaFolderFinder::MediaFolderFinderResultsCallback callback) {
@@ -306,6 +315,205 @@ TEST_F(MediaScanManagerTest, SingleResult) {
   EXPECT_EQ(galleries_before + 1, gallery_count());
 }
 
+// Generally test that it includes directories with sufficient density
+// and excludes others.
+//
+// A/ - NOT included
+// A/B/ - NOT included
+// A/B/C/files
+// A/D/ - NOT included
+// A/D/E/files
+// A/D/F/files
+// A/D/G/files
+// A/D/H/
+// A/H/ - included in results
+// A/H/I/files
+// A/H/J/files
+TEST_F(MediaScanManagerTest, MergeRedundant) {
+  base::FilePath path;
+  MediaFolderFinder::MediaFolderFinderResults found_folders;
+  std::vector<base::FilePath> sensitive_locations;
+  std::vector<base::FilePath> expected_folders;
+  MediaGalleryScanResult file_counts;
+  file_counts.audio_count = 1;
+  file_counts.image_count = 2;
+  file_counts.video_count = 3;
+  MakeTestFolder("A", &path);
+  MakeTestFolder("A/B", &path);
+  MakeTestFolder("A/B/C", &path);
+  found_folders[path] = file_counts;
+  // Not dense enough.
+  MakeTestFolder("A/D", &path);
+  MakeTestFolder("A/D/E", &path);
+  found_folders[path] = file_counts;
+  MakeTestFolder("A/D/F", &path);
+  found_folders[path] = file_counts;
+  MakeTestFolder("A/D/G", &path);
+  found_folders[path] = file_counts;
+  MakeTestFolder("A/D/H", &path);
+  // Dense enough to be reported.
+  MakeTestFolder("A/H", &path);
+  expected_folders.push_back(path);
+  MakeTestFolder("A/H/I", &path);
+  found_folders[path] = file_counts;
+  MakeTestFolder("A/H/J", &path);
+  found_folders[path] = file_counts;
+  MediaFolderFinder::MediaFolderFinderResults results =
+      FindContainerScanResults(found_folders, sensitive_locations);
+  EXPECT_EQ(expected_folders.size(), results.size());
+  for (std::vector<base::FilePath>::const_iterator it =
+           expected_folders.begin();
+       it != expected_folders.end();
+       ++it) {
+    EXPECT_TRUE(results.find(*it) != results.end());
+  }
+}
+
+// Make sure intermediates are not included.
+//
+// A/ - included in results
+// A/B1/ - NOT included
+// A/B1/B2/files
+// A/C1/ - NOT included
+// A/C1/C2/files
+TEST_F(MediaScanManagerTest, MergeRedundantNoIntermediates) {
+  base::FilePath path;
+  MediaFolderFinder::MediaFolderFinderResults found_folders;
+  std::vector<base::FilePath> sensitive_locations;
+  std::vector<base::FilePath> expected_folders;
+  MediaGalleryScanResult file_counts;
+  file_counts.audio_count = 1;
+  file_counts.image_count = 2;
+  file_counts.video_count = 3;
+  MakeTestFolder("A", &path);
+  expected_folders.push_back(path);
+  MakeTestFolder("A/B1", &path);
+  MakeTestFolder("A/B1/B2", &path);
+  found_folders[path] = file_counts;
+  MakeTestFolder("A/C1", &path);
+  MakeTestFolder("A/C1/C2", &path);
+  found_folders[path] = file_counts;
+  // Make "home dir" not dense enough.
+  MakeTestFolder("D", &path);
+  MediaFolderFinder::MediaFolderFinderResults results =
+      FindContainerScanResults(found_folders, sensitive_locations);
+  EXPECT_EQ(expected_folders.size(), results.size());
+  for (std::vector<base::FilePath>::const_iterator it =
+           expected_folders.begin();
+       it != expected_folders.end();
+       ++it) {
+    EXPECT_TRUE(results.find(*it) != results.end());
+  }
+}
+
+// Make sure "A/" only gets a count of 1, from "A/D/",
+// not 2 from "A/D/H/" and "A/D/I/".
+//
+// A/ - NOT included
+// A/D/ - included in results
+// A/D/E/files
+// A/D/F/files
+// A/D/G/files
+// A/D/H/files
+// A/D/I/ - NOT included
+// A/D/I/J/files
+TEST_F(MediaScanManagerTest, MergeRedundantVerifyNoOvercount) {
+  base::FilePath path;
+  MediaFolderFinder::MediaFolderFinderResults found_folders;
+  std::vector<base::FilePath> sensitive_locations;
+  std::vector<base::FilePath> expected_folders;
+  MediaGalleryScanResult file_counts;
+  file_counts.audio_count = 1;
+  file_counts.image_count = 2;
+  file_counts.video_count = 3;
+  MakeTestFolder("A", &path);
+  MakeTestFolder("A/D", &path);
+  expected_folders.push_back(path);
+  MakeTestFolder("A/D/E", &path);
+  found_folders[path] = file_counts;
+  MakeTestFolder("A/D/F", &path);
+  found_folders[path] = file_counts;
+  MakeTestFolder("A/D/G", &path);
+  found_folders[path] = file_counts;
+  MakeTestFolder("A/D/H", &path);
+  found_folders[path] = file_counts;
+  MakeTestFolder("A/D/I", &path);
+  MakeTestFolder("A/D/I/J", &path);
+  found_folders[path] = file_counts;
+  MediaFolderFinder::MediaFolderFinderResults results =
+      FindContainerScanResults(found_folders, sensitive_locations);
+  EXPECT_EQ(expected_folders.size(), results.size());
+  for (std::vector<base::FilePath>::const_iterator it =
+           expected_folders.begin();
+       it != expected_folders.end();
+       ++it) {
+    EXPECT_TRUE(results.find(*it) != results.end());
+  }
+}
+
+// Make sure that sensistive directories are pruned.
+//
+// A/ - NOT included
+// A/B/ - sensitive
+// A/C/ - included in results
+// A/C/G/files
+// A/C/H/files
+// A/D/ - included in results
+// A/D/I/files
+// A/D/J/files
+// A/E/ - included in results
+// A/E/K/files
+// A/E/L/files
+// A/F/ - included in results
+// A/F/M/files
+// A/F/N/files
+TEST_F(MediaScanManagerTest, MergeRedundantWithSensitive) {
+  base::FilePath path;
+  MediaFolderFinder::MediaFolderFinderResults found_folders;
+  std::vector<base::FilePath> sensitive_locations;
+  std::vector<base::FilePath> expected_folders;
+  MediaGalleryScanResult file_counts;
+  file_counts.audio_count = 1;
+  file_counts.image_count = 2;
+  file_counts.video_count = 3;
+  MakeTestFolder("A", &path);
+  MakeTestFolder("A/B", &path);
+  sensitive_locations.push_back(path);
+  MakeTestFolder("A/C", &path);
+  expected_folders.push_back(path);
+  MakeTestFolder("A/C/G", &path);
+  found_folders[path] = file_counts;
+  MakeTestFolder("A/C/H", &path);
+  found_folders[path] = file_counts;
+  MakeTestFolder("A/D", &path);
+  expected_folders.push_back(path);
+  MakeTestFolder("A/D/I", &path);
+  found_folders[path] = file_counts;
+  MakeTestFolder("A/D/J", &path);
+  found_folders[path] = file_counts;
+  MakeTestFolder("A/E", &path);
+  expected_folders.push_back(path);
+  MakeTestFolder("A/E/K", &path);
+  found_folders[path] = file_counts;
+  MakeTestFolder("A/E/L", &path);
+  found_folders[path] = file_counts;
+  MakeTestFolder("A/F", &path);
+  expected_folders.push_back(path);
+  MakeTestFolder("A/F/M", &path);
+  found_folders[path] = file_counts;
+  MakeTestFolder("A/F/N", &path);
+  found_folders[path] = file_counts;
+  MediaFolderFinder::MediaFolderFinderResults results =
+      FindContainerScanResults(found_folders, sensitive_locations);
+  EXPECT_EQ(expected_folders.size(), results.size());
+  for (std::vector<base::FilePath>::const_iterator it =
+           expected_folders.begin();
+       it != expected_folders.end();
+       ++it) {
+    EXPECT_TRUE(results.find(*it) != results.end());
+  }
+}
+
 TEST_F(MediaScanManagerTest, Containers) {
   MediaGalleryScanResult file_counts;
   file_counts.audio_count = 1;
@@ -422,6 +630,8 @@ TEST_F(MediaScanManagerTest, UpdateExistingScanResults) {
   MakeTestFolder("gscan/dir1", &path);
   found_folders[path] = file_counts;
 
+  MakeTestFolder("junk", &path);
+
   SetFindFoldersResults(true, found_folders);
   file_counts.video_count = 7;
   SetExpectedScanResults(1 /*gallery_count*/, file_counts);
@@ -462,6 +672,8 @@ TEST_F(MediaScanManagerTest, UpdateExistingCounts) {
   MakeTestFolder("scan", &path);
   found_folders[path] = file_counts;
 
+  MakeTestFolder("junk", &path);
+
   file_counts.audio_count = 5;
   MakeTestFolder("user/dir2", &path);
   found_folders[path] = file_counts;