Have FindContainerScanResults() find broader media container directories

chrome/browser/media_galleries/media_scan_manager_unittest.cc

Index: chrome/browser/media_galleries/media_scan_manager_unittest.cc
diff --git a/chrome/browser/media_galleries/media_scan_manager_unittest.cc b/chrome/browser/media_galleries/media_scan_manager_unittest.cc
index 5ab0c4ef241fc0186eedb29279d0bcbf7753b638..ac16c888e3ff43b48beaa2598b4ef8f71cd22c3c 100644
--- a/chrome/browser/media_galleries/media_scan_manager_unittest.cc
+++ b/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,123 @@ TEST_F(MediaScanManagerTest, SingleResult) {
   EXPECT_EQ(galleries_before + 1, gallery_count());
 }
 
+TEST_F(MediaScanManagerTest, MergeRedundant) {

[vandebo (ex-Chrome), 2014/05/28 21:01:06]

nit: for all of these tests, a comment laying out the structure and anticipated
result would make things easier to peruse. 

[Kevin Bailey, 2014/05/29 00:36:41]

Done. Should look better.

+  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);
+  expected_folders.push_back(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());
+  }
+}
+
+TEST_F(MediaScanManagerTest, MergeRedundant2) {
+  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);
+  expected_folders.push_back(path);
+  MakeTestFolder("A/B1/B2", &path);
+  found_folders[path] = file_counts;
+  MakeTestFolder("A/C1", &path);
+  expected_folders.push_back(path);
+  MakeTestFolder("A/C1/C2", &path);
+  found_folders[path] = file_counts;
+  // Make "home dir" not dense enough.
+  MakeTestFolder("D", &path);

[vandebo (ex-Chrome), 2014/05/28 21:01:06]

So I can see both sides of the story on this one, what's your opinion?

If we have the example A/B/C/D/E/files - where each level has just the one
subdirectory, then it's probably some kind of organization scheme that either is
brand new (only one thing in it), or a failed scheme (everything ends up in E),
or a bunch of extra directories (an unzipped set of files?).  It seems that the
first option is least likely.  I wonder if intermediate directories should be
allowed to have just one child, but the top level ones should be required to
have more?

i.e. A/B/C/files, A/D/E/files should find A, but F/G/H/files should only have H
(which would have been passed in).

[Kevin Bailey, 2014/05/29 00:36:41]

I could change the behavior of the final loop - copying into |result| - such
that something had to be |qualified| AND have at least 2 entries. This would
cause us to investigate "Artist" directories but not include them, but would
include the parent "Media" directory.

Media/ *
  Artist1/
    Album1/files
  Artist2/
    Album1/files

I looked back through the emails and it wasn't clear how important it was to
include intermediates.

[vandebo (ex-Chrome), 2014/05/30 20:30:24]

In general, I think intermediates should be passed up to the caller for
consideration. I don't think it's required though, so for this particular case,
it seems reasonable to do that.

[Kevin Bailey, 2014/06/01 21:12:40]

I *could* add another loop to *include* parents with sole children who have an
"interesting" ancestor. So Home/Media/Artist would be included, but Home/
wouldn't. A small recursive function would be easiest.

+  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, 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;