Redesigned and refactored ScopedMTPDeviceMapEntry, MTPDeviceMapService & MTPDeviceDelegate classes.

chrome/browser/media_gallery/media_file_system_registry.cc

 // Copyright (c) 2012 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.
 
 // MediaFileSystemRegistry implementation.
 
 #include "chrome/browser/media_gallery/media_file_system_registry.h"
 
 #include <set>
 #include <vector>
@@ skipping 112 matching lines @@
                                          const FilePath& fs_path,
                                          const std::string& filesystem_id)
     : name(fs_name),
       path(fs_path),
       fsid(filesystem_id) {
 }
 
 MediaFileSystemInfo::MediaFileSystemInfo() {}
 
 #if defined(SUPPORT_MTP_DEVICE_FILESYSTEM)
-
 ScopedMTPDeviceMapEntry::ScopedMTPDeviceMapEntry(
     const FilePath::StringType& device_location,
     const base::Closure& no_references_callback)
     : device_location_(device_location),
-      delegate_(new MTPDeviceDelegateImpl(device_location)),
       no_references_callback_(no_references_callback) {
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       Bind(&MTPDeviceMapService::AddDelegate,
            base::Unretained(MTPDeviceMapService::GetInstance()),
-           device_location_, make_scoped_refptr(delegate_)));
+           device_location_,
+           new MTPDeviceDelegateImpl(device_location_)));
 }
 
 ScopedMTPDeviceMapEntry::~ScopedMTPDeviceMapEntry() {
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       Bind(&MTPDeviceMapService::RemoveDelegate,
            base::Unretained(MTPDeviceMapService::GetInstance()),
            device_location_));
   no_references_callback_.Run();
 }
-
 #endif  // defined(SUPPORT_MTP_DEVICE_FILESYSTEM)
 
 // The main owner of this class is
 // |MediaFileSystemRegistry::extension_hosts_map_|, but a callback may
 // temporarily hold a reference.
 class ExtensionGalleriesHost
     : public base::RefCountedThreadSafe<ExtensionGalleriesHost>,
       public content::NotificationObserver {
  public:
   // |no_references_callback| is called when the last RenderViewHost reference
@@ skipping 505 matching lines @@
   SystemMonitor* system_monitor = SystemMonitor::Get();
   if (system_monitor)
     system_monitor->AddDevicesChangedObserver(this);
 }
 
 MediaFileSystemRegistry::~MediaFileSystemRegistry() {
   // SystemMonitor may be NULL in unit tests.
   SystemMonitor* system_monitor = SystemMonitor::Get();
   if (system_monitor)
     system_monitor->RemoveDevicesChangedObserver(this);
+  DCHECK(mtp_device_delegate_map_.empty());
 }
 
 void MediaFileSystemRegistry::OnMediaGalleriesRememberedGalleriesChanged(
     PrefServiceBase* prefs) {
   // Find the Profile that contains the source PrefService.
   PrefChangeRegistrarMap::iterator pref_change_it =
       pref_change_registrar_map_.begin();
   for (; pref_change_it != pref_change_registrar_map_.end(); ++pref_change_it) {
     if (pref_change_it->first->GetPrefs() == prefs)
       break;
@@ skipping 23 matching lines @@
     gallery_host_it->second->RevokeOldGalleries(
         preferences->GalleriesForExtension(*extension));
   }
 }
 
 #if defined(SUPPORT_MTP_DEVICE_FILESYSTEM)
 ScopedMTPDeviceMapEntry*
 MediaFileSystemRegistry::GetOrCreateScopedMTPDeviceMapEntry(
     const FilePath::StringType& device_location) {
   MTPDeviceDelegateMap::iterator delegate_it =
-      mtp_delegate_map_.find(device_location);
-  if (delegate_it != mtp_delegate_map_.end() && delegate_it->second.get())
+      mtp_device_delegate_map_.find(device_location);
+  if (delegate_it != mtp_device_delegate_map_.end())
     return delegate_it->second;
   ScopedMTPDeviceMapEntry* mtp_device_host = new ScopedMTPDeviceMapEntry(
       device_location, base::Bind(

[Lei Zhang, 2012/11/22 02:25:25]

Why not just pass in |mtp_device_delegate_map_| instead of a closure? It makes
it much easier to see what's going on when reading the ScopedMTPDeviceMapEntry
code by itself. The added flexibility of having a closure isn't needed since we
always pass in the same closure.

[kmadhusu, 2012/11/22 02:41:10]

MediaFileSystemRegistry owns |mtp_device_delegate_map_|.
|mtp_device_delegate_map_| is an implementation detail of
MediaFileSystemRegistry and not ScopedMTPDeviceMapEntry. ScopedMTPDeviceMapEntry
should not manage the map which is owned by the consumer
(MediaFileSystemRegistry). Consumers of ScopedMTPDeviceMapEntry should be
notified about the destruction of the object to do the required clean up.

[Lei Zhang, 2012/11/22 03:09:42]

Sometimes it's ok for a class to give its private variables out. I think it's
less error prone for ScopedMTPDeviceMapEntry to manage its own map entry, but I
don't care either way.

           &MediaFileSystemRegistry::RemoveScopedMTPDeviceMapEntry,
           base::Unretained(this), device_location));
-  mtp_delegate_map_[device_location] = mtp_device_host->AsWeakPtr();
+  mtp_device_delegate_map_[device_location] = mtp_device_host;
   return mtp_device_host;
 }
 
 void MediaFileSystemRegistry::RemoveScopedMTPDeviceMapEntry(
     const FilePath::StringType& device_location) {
   MTPDeviceDelegateMap::iterator delegate_it =
-      mtp_delegate_map_.find(device_location);
-  DCHECK(delegate_it != mtp_delegate_map_.end());
-  mtp_delegate_map_.erase(delegate_it);
+      mtp_device_delegate_map_.find(device_location);
+  DCHECK(delegate_it != mtp_device_delegate_map_.end());
+  mtp_device_delegate_map_.erase(delegate_it);
 }
 #endif
 
 void MediaFileSystemRegistry::OnExtensionGalleriesHostEmpty(
     Profile* profile, const std::string& extension_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   ExtensionGalleriesHostMap::iterator extension_hosts =
       extension_hosts_map_.find(profile);
   DCHECK(extension_hosts != extension_hosts_map_.end());
   ExtensionHostMap::size_type erase_count =
       extension_hosts->second.erase(extension_id);
   DCHECK_EQ(1U, erase_count);
   if (extension_hosts->second.empty()) {
     extension_hosts_map_.erase(extension_hosts);
 
     PrefChangeRegistrarMap::iterator pref_it =
         pref_change_registrar_map_.find(profile);
     DCHECK(pref_it != pref_change_registrar_map_.end());
     delete pref_it->second;
     pref_change_registrar_map_.erase(pref_it);
   }
 }
 
 }  // namespace chrome