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..f1104af54020421fe80de48a304b369ae2665f6e 100644
--- a/chrome/browser/media_galleries/media_scan_manager.cc
+++ b/chrome/browser/media_galleries/media_scan_manager.cc
@@ -225,45 +225,64 @@ MediaFolderFinder::MediaFolderFinderResults FindContainerScanResults(
       abs_sensitive_locations.push_back(path);
   }
   // Count the number of scan results with the same parent directory.

[vandebo (ex-Chrome), 2014/05/14 22:33:08]

This comment seems wrong now

[Kevin Bailey, 2014/05/20 16:21:39]

Better?

+  MediaFolderFinder::MediaFolderFinderResults result;
   typedef std::map<base::FilePath, int /*count*/> ContainerCandidates;
   ContainerCandidates candidates;

[vandebo (ex-Chrome), 2014/05/14 22:33:08]

Having result, candidates, and interesting_candidates makes things hard to
follow, can you come up with some better names to explain the difference? I
wasn't able to work through this logic easily, so I stopped.

[Kevin Bailey, 2014/05/20 16:21:39]

I renamed to |candidates_to_check|, and commented each one.

+  std::set<base::FilePath> interesting_candidates;
+  // Add incoming folders to all the lists.
   for (MediaFolderFinder::MediaFolderFinderResults::const_iterator it =
            found_folders.begin(); it != found_folders.end(); ++it) {
-    base::FilePath parent_directory = it->first.DirName();
+    // All original folders are "permanent" candidates.
+    // If not subsumed by parent, they get reported.
+    // TODO: We could even keep the original audio_count, ...

[vandebo (ex-Chrome), 2014/05/14 22:33:08]

Container results should be reported with zero counts - It turns out that
totalling up counts is profile specific, so can't be done at this level.

[Kevin Bailey, 2014/05/20 16:21:39]

Done.

+    result[it->first] = MediaGalleryScanResult();
+    // We have to use this count in case this candidate is treated like a
+    // parent. Media files have to outnumber non-media entries.
+    candidates[it->first] = it->second.audio_count + it->second.image_count +

[vandebo (ex-Chrome), 2014/05/14 22:33:08]

Why do we need the counts of media files?

[Kevin Bailey, 2014/05/20 16:21:39]

Gone.

+        it->second.video_count;
+    // Add it to the list to be considered.
+    interesting_candidates.insert(it->first);
+  }
+  base::FilePath candidate;
+  while (!interesting_candidates.empty()) {
+    candidate = *interesting_candidates.begin();
+    // Remove in case it gets added back.
+    interesting_candidates.erase(interesting_candidates.begin());
+    // It *could* have been nuked.
+    ContainerCandidates::iterator it = candidates.find(candidate);
+    if (it == candidates.end())
+      continue;
+    base::FilePath parent_directory = candidate.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())
+    if (abs_parent_directory.empty()) {
+      result.erase(result.find(candidate));
+      candidates.erase(it);
       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;
+        break;
       }
     }
-    if (is_sensitive)
+    if (is_sensitive) {
+      result.erase(result.find(candidate));
+      candidates.erase(it);
       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)
+    ContainerCandidates::iterator it_parent = candidates.find(parent_directory);
+    if (it_parent == candidates.end()) {
+      candidates[parent_directory] = 1;
       continue;
-
-    base::FileEnumerator dir_counter(it->first, false /*recursive*/,
+    }
+    it_parent->second++;
+    // If a parent directory has more than one scan result, consider it.
+    base::FileEnumerator dir_counter(it_parent->first, false /*recursive*/,
                                      base::FileEnumerator::DIRECTORIES);
     base::FileEnumerator::FileInfo info;
     int count = 0;
@@ -273,8 +292,25 @@ MediaFolderFinder::MediaFolderFinderResults FindContainerScanResults(
       if (!base::IsLink(name))
         count++;
     }
-    if (it->second * 100 / count >= kContainerDirectoryMinimumPercent)
-      result[it->first] = MediaGalleryScanResult();
+    if (it_parent->second * 100 / count >= kContainerDirectoryMinimumPercent) {
+      // We're keeping this parent. Remove all children.

[vandebo (ex-Chrome), 2014/05/14 22:33:08]

Which children to remove is Profile specific, so you can't do it here.

[Kevin Bailey, 2014/05/20 16:21:39]

Gone, but we do remove children that we've added.

+      for (ContainerCandidates::iterator it_child = candidates.begin();
+           it_child != candidates.end(); ) {
+        if (it_child != it_parent &&
+            it_child->first.DirName() == it_parent->first) {
+          ContainerCandidates::iterator next(it_child);
+          ++next;
+          result.erase(result.find(it_child->first));
+          candidates.erase(it_child);
+          it_child = next;
+        } else {
+          ++it_child;
+        }
+      }
+      // It's a qualified candidate now.
+      result[it_parent->first] = MediaGalleryScanResult();
+      interesting_candidates.insert(it_parent->first);
+    }
   }
   return result;
 }
@@ -466,16 +502,11 @@ void MediaScanManager::OnScanCompleted(
                  found_folders,
                  folder_finder_->graylisted_folders()),
       base::Bind(&MediaScanManager::OnFoundContainerDirectories,
-                 weak_factory_.GetWeakPtr(),
-                 found_folders));
+                 weak_factory_.GetWeakPtr()));
 }
 
 void MediaScanManager::OnFoundContainerDirectories(
-    const MediaFolderFinder::MediaFolderFinderResults& found_folders,
     const MediaFolderFinder::MediaFolderFinderResults& container_folders) {
-  MediaFolderFinder::MediaFolderFinderResults folders;
-  folders.insert(found_folders.begin(), found_folders.end());
-  folders.insert(container_folders.begin(), container_folders.end());
 
   for (ScanObserverMap::iterator scans_for_profile = observers_.begin();
        scans_for_profile != observers_.end();
@@ -490,7 +521,7 @@ void MediaScanManager::OnFoundContainerDirectories(
     if (!extension_service)
       continue;
 
-    AddScanResultsForProfile(preferences, folders);
+    AddScanResultsForProfile(preferences, container_folders);

[vandebo (ex-Chrome), 2014/05/14 22:33:08]

We need to add all the scan results - this is the only place results are added -
not just container folders.  If this is all the results (including the
containers), then the variable should be renamed.

[Kevin Bailey, 2014/05/20 16:21:39]

Restored original code.

 
     ScanningExtensionIdSet* scanning_extensions =
         &scans_for_profile->second.scanning_extensions;