iTunes file util and data provider for media galleries

chrome/browser/media_galleries/imported_media_gallery_registry.cc

Index: chrome/browser/media_galleries/imported_media_gallery_registry.cc
diff --git a/chrome/browser/media_galleries/imported_media_gallery_registry.cc b/chrome/browser/media_galleries/imported_media_gallery_registry.cc
index 0a17c82cf4293d7abe4b9116df31e6ffdc6c03b8..8d50e701687037d58a772115a1970baf1eac9a2c 100644
--- a/chrome/browser/media_galleries/imported_media_gallery_registry.cc
+++ b/chrome/browser/media_galleries/imported_media_gallery_registry.cc
@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/threading/sequenced_worker_pool.h"
+#include "chrome/browser/media_galleries/fileapi/itunes_data_provider.h"
 #include "chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "content/public/browser/browser_thread.h"
@@ -46,7 +47,6 @@ ImportedMediaGalleryRegistry* ImportedMediaGalleryRegistry::GetInstance() {
   return g_imported_media_gallery_registry.Pointer();
 }
 
-// static
 std::string ImportedMediaGalleryRegistry::RegisterPicasaFilesystemOnUIThread(
     const base::FilePath& database_path) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
@@ -60,28 +60,78 @@ std::string ImportedMediaGalleryRegistry::RegisterPicasaFilesystemOnUIThread(
   if (fsid.empty())
     return "";
 
-  MediaTaskRunner()->PostTask(
-      FROM_HERE,
-      Bind(&ImportedMediaGalleryRegistry::RegisterPicasaFileSystem,
-           base::Unretained(GetInstance()), database_path));
+  picasa_fsids_.insert(fsid);
+
+  if (picasa_fsids_.size() == 1) {
+    MediaTaskRunner()->PostTask(
+        FROM_HERE,
+        Bind(&ImportedMediaGalleryRegistry::RegisterPicasaFileSystem,
+             base::Unretained(GetInstance()), database_path));

[Lei Zhang, 2013/06/06 03:48:47]

Since this is no longer static, use this instead of GetInstance() ?

[vandebo (ex-Chrome), 2013/06/06 19:49:18]

Done.

+#ifndef NDEBUG
+    picasa_database_path_ = database_path;
+  } else {
+    DCHECK_EQ(picasa_database_path_, database_path);

[Lei Zhang, 2013/06/06 03:48:47]

If these were FilePaths, you need to compare their .value()s instead. Sigh.

[vandebo (ex-Chrome), 2013/06/06 19:49:18]

Done.

+#endif
+  }
 
   return fsid;
 }
 
-// static
-bool ImportedMediaGalleryRegistry::RevokePicasaFilesystemOnUIThread(
+std::string ImportedMediaGalleryRegistry::RegisterITunesFilesystemOnUIThread(
+    const base::FilePath& library_xml_path) {

[Lei Zhang, 2013/06/06 03:48:47]

DCHECK the path is not empty?

[vandebo (ex-Chrome), 2013/06/06 19:49:18]

Done.

+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  std::string fsid =
+      fileapi::IsolatedContext::GetInstance()->RegisterFileSystemForVirtualPath(
+           fileapi::kFileSystemTypeItunes,
+           extension_misc::kMediaFileSystemPathPart,
+           base::FilePath());
+
+  if (fsid.empty())
+    return "";

[Lei Zhang, 2013/06/06 03:48:47]

FWIW, this can't actually be reached. If we reach this line, we'll just fail a
CHECK() in the current caller.

We should return std::string() BTW.

[vandebo (ex-Chrome), 2013/06/06 19:49:18]

It can fail in IsolatedContext, so I fixed the callers.
Done.

[Lei Zhang, 2013/06/07 03:29:07]

With the current implementation and the parameters we pass in, it can't fail.

I think there are multiple layers where we all have DCHECKS that the fsid is
non-empty, now that you removed the one CHECK.

[vandebo (ex-Chrome), 2013/06/07 03:46:52]

Are you saying that we should add a CHECK in IsolatedContext and not have the
failure case?  That seems fine to me.

+
+  itunes_fsids_.insert(fsid);
+
+  if (itunes_fsids_.size() == 1) {
+    MediaTaskRunner()->PostTask(
+        FROM_HERE,
+        Bind(&ImportedMediaGalleryRegistry::RegisterITunesFileSystem,
+             base::Unretained(GetInstance()), library_xml_path));
+#ifndef NDEBUG
+    itunes_xml_library_path_ = library_xml_path;
+  } else {
+    DCHECK_EQ(itumes_xml_library_path_, library_xml_path);

[Lei Zhang, 2013/06/06 03:48:47]

typo: itumes

[vandebo (ex-Chrome), 2013/06/06 19:49:18]

Done.

+#endif
+  }
+
+  return fsid;
+}
+
+bool ImportedMediaGalleryRegistry::RevokeImportedFilesystemOnUIThread(
     const std::string& fsid) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
   if (!fileapi::IsolatedContext::GetInstance()->RevokeFileSystem(fsid))
     return false;
 
-  MediaTaskRunner()->PostTask(
-      FROM_HERE,
-      Bind(&ImportedMediaGalleryRegistry::RevokePicasaFileSystem,
-           base::Unretained(GetInstance())));
+  if (picasa_fsids_.erase(fsid) && !picasa_fsids_.size()) {

[Lei Zhang, 2013/06/06 03:48:47]

!foo_set.size() -> foo_set.empty()

[Lei Zhang, 2013/06/06 03:48:47]

If |picasa_fsids_| has 2 elements, and you erase 1, you shouldn't need to go on
and check |itunes_fsids_| below.

[vandebo (ex-Chrome), 2013/06/06 19:49:18]

Done.

[vandebo (ex-Chrome), 2013/06/06 19:49:18]

Done.

+    MediaTaskRunner()->PostTask(
+        FROM_HERE,
+        Bind(&ImportedMediaGalleryRegistry::RevokePicasaFileSystem,
+             base::Unretained(GetInstance())));
+    return true;
+  }
+
+  if (itunes_fsids_.erase(fsid) && !itunes_fsids_.size()) {
+    MediaTaskRunner()->PostTask(
+        FROM_HERE,
+        Bind(&ImportedMediaGalleryRegistry::RevokeITunesFileSystem,
+             base::Unretained(GetInstance())));
+    return true;
+  }
 
-  return true;
+  NOTREACHED();
+  return false;
 }
 
 // static
@@ -92,31 +142,45 @@ ImportedMediaGalleryRegistry::picasa_data_provider() {
   return GetInstance()->picasa_data_provider_.get();
 }
 
-ImportedMediaGalleryRegistry::ImportedMediaGalleryRegistry()
-    : picasa_filesystems_count_(0) {
+// static
+itunes::ITunesDataProvider*
+ImportedMediaGalleryRegistry::itunes_data_provider() {

[Lei Zhang, 2013/06/06 03:48:47]

nit: These are not simple getters, and should not be named as such.

[vandebo (ex-Chrome), 2013/06/06 19:49:18]

Done.

+  DCHECK(CurrentlyOnMediaTaskRunnerThread());
+  DCHECK(GetInstance()->itunes_data_provider_);
+  return GetInstance()->itunes_data_provider_.get();
 }
 
+ImportedMediaGalleryRegistry::ImportedMediaGalleryRegistry() {}
+
 ImportedMediaGalleryRegistry::~ImportedMediaGalleryRegistry() {
-  DCHECK_EQ(0, picasa_filesystems_count_);
+  DCHECK_EQ(0U, picasa_fsids_.size());
+  DCHECK_EQ(0U, itunes_fsids_.size());
 }
 
 void ImportedMediaGalleryRegistry::RegisterPicasaFileSystem(
     const base::FilePath& database_path) {
   DCHECK(CurrentlyOnMediaTaskRunnerThread());
-
-  if (++picasa_filesystems_count_ == 1) {
-    DCHECK(!picasa_data_provider_);
-    picasa_data_provider_.reset(new picasa::PicasaDataProvider(database_path));
-  }
+  DCHECK(!picasa_data_provider_);
+  picasa_data_provider_.reset(new picasa::PicasaDataProvider(database_path));
 }
 
 void ImportedMediaGalleryRegistry::RevokePicasaFileSystem() {
   DCHECK(CurrentlyOnMediaTaskRunnerThread());
+  DCHECK(picasa_data_provider_);
+  picasa_data_provider_.reset();
+}
 
-  if (--picasa_filesystems_count_ == 0) {
-    DCHECK(picasa_data_provider_);
-    picasa_data_provider_.reset(NULL);
-  }
+void ImportedMediaGalleryRegistry::RegisterITunesFileSystem(
+    const base::FilePath& xml_library_path) {
+  DCHECK(CurrentlyOnMediaTaskRunnerThread());
+  DCHECK(!itunes_data_provider_);
+  itunes_data_provider_.reset(new itunes::ITunesDataProvider(xml_library_path));
+}
+
+void ImportedMediaGalleryRegistry::RevokeITunesFileSystem() {
+  DCHECK(CurrentlyOnMediaTaskRunnerThread());
+  DCHECK(itunes_data_provider_);
+  itunes_data_provider_.reset();
 }
 
 }  // namespace chrome