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 195 matching lines @@
                             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.

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

nit: remove "immediate"

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

Done.

 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.

[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()) {
+    }
+    ContainerCandidates::iterator it_parent = candidates.find(parent_directory);
+    if (it_parent == candidates.end()) {
       candidates[parent_directory] = 1;
-    } else {
-      existing->second++;
+      continue;
     }
-  }
-
-  // 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*/,
+    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;
     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();
+    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;
 }
 
 int CountScanResultsForExtension(MediaGalleriesPreferences* preferences,
                                  const extensions::Extension* extension,
                                  MediaGalleryScanResult* file_counts) {
   int gallery_count = 0;
 
   MediaGalleryPrefIdSet permitted_galleries =
@@ skipping 171 matching lines @@
   UMA_HISTOGRAM_LONG_TIMES("MediaGalleries.ScanFinishedTime",
                            base::Time::Now() - scan_start_time_);
   scan_start_time_ = base::Time();
 
   content::BrowserThread::PostTaskAndReplyWithResult(
       content::BrowserThread::FILE, FROM_HERE,
       base::Bind(FindContainerScanResults,
                  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();
        ++scans_for_profile) {
     if (scans_for_profile->second.scanning_extensions.empty())
       continue;
     Profile* profile = scans_for_profile->first;
     MediaGalleriesPreferences* preferences =
         MediaGalleriesPreferencesFactory::GetForProfile(profile);
     ExtensionService* extension_service =
         extensions::ExtensionSystem::Get(profile)->extension_service();
     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;
     for (ScanningExtensionIdSet::const_iterator extension_id_it =
              scanning_extensions->begin();
          extension_id_it != scanning_extensions->end();
          ++extension_id_it) {
       const extensions::Extension* extension =
           extension_service->GetExtensionById(*extension_id_it, false);
       if (extension) {
         MediaGalleryScanResult file_counts;
         int gallery_count = CountScanResultsForExtension(preferences, extension,
                                                          &file_counts);
         scans_for_profile->second.observer->OnScanFinished(*extension_id_it,
                                                            gallery_count,
                                                            file_counts);
       }
     }
     scanning_extensions->clear();
     preferences->SetLastScanCompletionTime(base::Time::Now());
   }
   scoped_extension_registry_observer_.RemoveAll();
   folder_finder_.reset();
 }