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..29ee5b94eb6351eef5e1b733173ce1e69ccbdd51 100644
--- a/chrome/browser/media_galleries/media_scan_manager.cc
+++ b/chrome/browser/media_galleries/media_scan_manager.cc
@@ -210,10 +210,18 @@ void AddScanResultsForProfile(
                              unique_found_folders.size() + to_update.size());
 }
 
+struct ContainerCount {
+  int seen_count, entries_count;
+  bool has_parent;
+
+  ContainerCount()
+    :seen_count(0), entries_count(-1), has_parent(false) {}

[vandebo (ex-Chrome), 2014/05/21 15:55:09]

nit: previous line, and ": "

[Kevin Bailey, 2014/05/21 17:29:36]

Done.

+};
+
 // 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.
+// container directories.
 MediaFolderFinder::MediaFolderFinderResults FindContainerScanResults(
     const MediaFolderFinder::MediaFolderFinderResults& found_folders,
     const std::vector<base::FilePath>& sensitive_locations) {
@@ -224,57 +232,96 @@ MediaFolderFinder::MediaFolderFinderResults FindContainerScanResults(
     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;
+  // 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;
-  for (MediaFolderFinder::MediaFolderFinderResults::const_iterator it =
-           found_folders.begin(); it != found_folders.end(); ++it) {
-    base::FilePath parent_directory = it->first.DirName();
+  // |candidates_to_check| are members of |candidates| that have crossed
+  // threshold to be returned, and whose parents should likewise be checked.

[vandebo (ex-Chrome), 2014/05/21 15:55:09]

nit: "...and whose parents still need to be checked."

[Kevin Bailey, 2014/05/21 17:29:36]

I was really trying to say that we would recur on the parents, and their
relevant parents, but this is fine.

+  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;
     base::FilePath abs_parent_directory =
         base::MakeAbsoluteFilePath(parent_directory);
-    if (abs_parent_directory.empty())
+    if (abs_parent_directory.empty()) {
+      candidates.erase(candidates.find(candidate));

[vandebo (ex-Chrome), 2014/05/21 15:55:09]

Not needed - you only add to candidates after this check, so a sensitive folder
will never get added.

[Kevin Bailey, 2014/05/21 17:29:36]

I think this one was left-over but the next one is necessary.

       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) {
+      candidates.erase(candidates.find(candidate));

[vandebo (ex-Chrome), 2014/05/21 15:55:09]

ditto

[Kevin Bailey, 2014/05/21 17:29:36]

I don't think so. Consider:

A/
  sensitive/
  B/
    C/media
    D/media
  E/
    F/media
    G/media

So we add B because it has media subdirs, then we add E likewise. Now, because A
has 2 entries (pretend it reached 80%), we consider it and discover that it is
sensitive. We should therefore remove it.

[vandebo (ex-Chrome), 2014/05/21 19:21:00]

Please walk be through it if I'm missing something... on patch set 9, line 283
is where we add a directory, this check is on line 272, so if a directory is
sensitive, it'll never get added.

[Kevin Bailey, 2014/05/21 20:17:38]

Recall that a directory itself need not be sensitive but if one of it's children
are, then it gets excluded: line 269. In my example, A has a sensitive child.

[vandebo (ex-Chrome), 2014/05/22 17:35:51]

Won't that check on 269 prevent A from ever being added though?  At B, we'll
consider A, A is a parent of sensitive, is_sensitive gets set.  Same at E.

[Kevin Bailey, 2014/05/27 14:50:13]

Done.

       continue;
-
-    ContainerCandidates::iterator existing = candidates.find(parent_directory);
-    if (existing == candidates.end()) {
-      candidates[parent_directory] = 1;
-    } else {
-      existing->second++;
+    }
+    // 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()) {
+      candidates[parent_directory].seen_count = 1;
+      continue;
+    }
+    parent_it->second.seen_count++;

[vandebo (ex-Chrome), 2014/05/21 15:55:09]

If you like, use ContainerCount* parent_counts = &parent_it->second in place of
parent_it->second

[Kevin Bailey, 2014/05/21 17:29:36]

Done.

+    // If a parent directory has more than one scan result, consider it.
+    // If we haven't scanned it yet, do so.
+    if (parent_it->second.entries_count == -1) {
+      base::FileEnumerator dir_counter(parent_it->first, false /*recursive*/,

[vandebo (ex-Chrome), 2014/05/21 15:55:09]

parent_it->first => parent_directory

[Kevin Bailey, 2014/05/21 17:29:36]

Done.

+                                       base::FileEnumerator::DIRECTORIES);
+      base::FileEnumerator::FileInfo info;
+      int count = 0;

[vandebo (ex-Chrome), 2014/05/21 15:55:09]

nit: you could use parent_it->second.entries_count directly.

[Kevin Bailey, 2014/05/21 17:29:36]

I generally prefer to write in an exception safe style, even when not needed, in
order to better remember during those times when it is, in fact, necessary.
Simplicity is a noble goal too, though. :)

+      for (base::FilePath name = dir_counter.Next();
+           !name.empty();
+           name = dir_counter.Next()) {
+        if (!base::IsLink(name))
+          count++;
+      }
+      parent_it->second.entries_count = count;
+    }
+    if (parent_it->second.seen_count * 100 / parent_it->second.entries_count
+        >= kContainerDirectoryMinimumPercent) {
+      // We're keeping this parent. Mark immediate children for exclusion.
+      for (ContainerCandidates::iterator child_it = candidates.begin();
+           child_it != candidates.end(); ++child_it) {
+        if (child_it != parent_it &&
+            child_it->first.DirName() == parent_it->first) {

[vandebo (ex-Chrome), 2014/05/21 15:55:09]

parent_it->first => parent_directory

[Kevin Bailey, 2014/05/21 17:29:36]

Done.

+          child_it->second.has_parent = true;
+        }
+      }
+      // It's a qualified candidate now.
+      candidates_to_check.insert(parent_it->first);

[vandebo (ex-Chrome), 2014/05/21 15:55:09]

parent_it->first => parent_directory

[Kevin Bailey, 2014/05/21 17:29:36]

Done.

     }
   }
-
-  // If a parent directory has more than one scan result, consider it.
   MediaFolderFinder::MediaFolderFinderResults result;
+  // Copy and return worthy results.
   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)
+       it != candidates.end(); ++it) {
+    if (it->second.seen_count > 1 && it->second.seen_count * 100 /

[vandebo (ex-Chrome), 2014/05/21 15:55:09]

Instead of duplicating the criteria here would it be better to add another
boolean to ContainerCount ?

[Kevin Bailey, 2014/05/21 17:29:36]

Sure, swapping it for |has_parent| is free.

+        it->second.entries_count >= kContainerDirectoryMinimumPercent &&
+        !it->second.has_parent) {
       result[it->first] = MediaGalleryScanResult();
+    }
   }
   return result;
 }