Expose gallery watching functionality to the public mediaGalleries API.

chrome/browser/extensions/api/media_galleries/media_galleries_api.cc

Index: chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
diff --git a/chrome/browser/extensions/api/media_galleries/media_galleries_api.cc b/chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
index 7682d792149044501111fca72caee21ae201c473..d7145d8960a98655e7233c3e1372a5aede43e7cf 100644
--- a/chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
+++ b/chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
@@ -68,6 +68,12 @@ namespace MediaGalleries = api::media_galleries;
 namespace DropPermissionForMediaFileSystem =
     MediaGalleries::DropPermissionForMediaFileSystem;
 namespace GetMediaFileSystems = MediaGalleries::GetMediaFileSystems;
+namespace AddGalleryWatch =
+    extensions::api::media_galleries::AddGalleryWatch;

[tommycli, 2014/08/19 16:36:02]

make consistent with above declarations... i.e. MediaGalleries::AddGalleryWatch.
And below 2 also.

[Oren Blasberg, 2014/08/20 22:59:36]

Done.

+namespace RemoveGalleryWatch =
+    extensions::api::media_galleries::RemoveGalleryWatch;
+namespace GetAllGalleryWatch =
+    extensions::api::media_galleries::GetAllGalleryWatch;
 
 namespace {
 
@@ -121,6 +127,28 @@ bool Setup(Profile* profile, std::string* error, base::Closure callback) {
   return true;
 }
 
+// Gets the |gallery_file_path| and |gallery_pref_id| of the gallery specified

[tommycli, 2014/08/19 16:36:02]

Make briefer. Probably don't need both first sentence and second. Should be
"Returns true and sets..." if you choose to use the second sentence.

Probably update the comment in the private api too.

[Oren Blasberg, 2014/08/20 22:59:36]

Done.

+// by the |gallery_id|. Returns true and set |gallery_file_path| and
+// |gallery_pref_id| if the |gallery_id| is valid and returns false otherwise.
+bool GetGalleryFilePathAndId(const std::string& gallery_id,
+                             Profile* profile,
+                             const Extension* extension,
+                             base::FilePath* gallery_file_path,
+                             MediaGalleryPrefId* gallery_pref_id) {
+  MediaGalleryPrefId pref_id;
+  if (!base::StringToUint64(gallery_id, &pref_id))
+    return false;
+  MediaGalleriesPreferences* preferences =
+      g_browser_process->media_file_system_registry()->GetPreferences(profile);
+  base::FilePath file_path(
+      preferences->LookUpGalleryPathForExtension(pref_id, extension, false));
+  if (file_path.empty())
+    return false;
+  *gallery_pref_id = pref_id;
+  *gallery_file_path = file_path;
+  return true;
+}
+
 WebContents* GetWebContents(content::RenderViewHost* rvh,
                             Profile* profile,
                             const std::string& app_id) {
@@ -1034,4 +1062,198 @@ void MediaGalleriesGetMetadataFunction::ConstructNextBlob(
   SendResponse(true);
 }
 
+///////////////////////////////////////////////////////////////////////////////

[tommycli, 2014/08/19 16:36:02]

We don't use this block to divide between classes in this file. But I do think
it's a good idea. maybe add them for the other classes?

[Oren Blasberg, 2014/08/20 22:59:36]

Done.

+//              MediaGalleriesAddGalleryWatchFunction                        //
+///////////////////////////////////////////////////////////////////////////////
+MediaGalleriesAddGalleryWatchFunction::

[tommycli, 2014/08/19 16:36:02]

I assume you ran git cl format on everything right? If you did, I will say
nothing about formatting whatsoever...

[Oren Blasberg, 2014/08/20 22:59:36]

Didn't at first, did in patch 4.

+~MediaGalleriesAddGalleryWatchFunction() {
+}
+
+bool MediaGalleriesAddGalleryWatchFunction::RunAsync() {
+  DCHECK(GetProfile());
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  if (!render_view_host() || !render_view_host()->GetProcess())
+    return false;
+
+  scoped_ptr<AddGalleryWatch::Params> params(
+      AddGalleryWatch::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+
+  MediaGalleriesPreferences* preferences =
+      g_browser_process->media_file_system_registry()->GetPreferences(
+          GetProfile());
+  preferences->EnsureInitialized(base::Bind(
+      &MediaGalleriesAddGalleryWatchFunction::OnPreferencesInit,
+      this,
+      params->gallery_id));
+
+  return true;
+}
+
+void MediaGalleriesAddGalleryWatchFunction::OnPreferencesInit(
+    const std::string& pref_id) {
+
+  base::FilePath gallery_file_path;
+  MediaGalleryPrefId gallery_pref_id = 0;
+  if (!GetGalleryFilePathAndId(pref_id,
+                               GetProfile(),
+                               extension(),
+                               &gallery_file_path,
+                               &gallery_pref_id)) {
+    error_ = kInvalidGalleryId;
+    const std::string error_str(kInvalidGalleryId);
+    HandleResponse(gallery_pref_id, error_str);

[tommycli, 2014/08/19 16:36:02]

If you have an error here, you still check for an event listener in the
HandleResponse section... you should probably just return an error
immediately... 

These two errors are handled inconsistently. In one, you set a result, and in
the other, you don't.

[Oren Blasberg, 2014/08/20 22:59:36]

Done.

+    return;
+  }
+
+  gallery_watch_manager()->AddWatch(
+      GetProfile(),
+      extension(),
+      gallery_pref_id,
+      base::Bind(&MediaGalleriesAddGalleryWatchFunction::HandleResponse,
+                 this,
+                 gallery_pref_id));
+
+}
+
+void MediaGalleriesAddGalleryWatchFunction::HandleResponse(
+    MediaGalleryPrefId gallery_id,
+    const std::string& error) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  // If an app added a file watch without any event listeners on the
+  // onGalleryChanged event, that's an error.
+  MediaGalleriesEventRouter* api = MediaGalleriesEventRouter::Get(GetProfile());
+  if (!api->ExtensionHasGalleryChangeListener(extension()->id())) {
+    api::media_galleries::AddGalleryWatchResult result;

[tommycli, 2014/08/19 16:36:02]

Does the callback still get called when there's an error?

[Oren Blasberg, 2014/08/20 22:59:36]

It does, as evidenced by the setupWatchOnUnlistenedValidGalleries test.
Also I tried it. It does, with success false, and the error occurs too.

[tommycli, 2014/08/20 23:21:12]

Awesome. Thanks for testing that out.

+    result.gallery_id = base::Uint64ToString(gallery_id);
+    result.success = false;
+    SetResult(result.ToValue().release());
+    error_ = kMissingEventListener;
+    SendResponse(false);
+    return;
+  }
+
+  api::media_galleries::AddGalleryWatchResult result;
+  result.gallery_id = base::Uint64ToString(gallery_id);
+  result.success = error.empty();
+  SetResult(result.ToValue().release());
+  if (error.empty()) {
+    base::DictionaryValue* dv = new base::DictionaryValue();
+    results_->GetDictionary(0, &dv);

[tommycli, 2014/08/19 16:36:02]

This looks strange. What's the purpose of this?

[Oren Blasberg, 2014/08/20 22:59:36]

Whoops. It was part of debugging code I forgot to remove.

+    SendResponse(true);
+  } else {
+    error_ = error.c_str();
+    SendResponse(false);
+  }
+}
+
+///////////////////////////////////////////////////////////////////////////////
+//              MediaGalleriesRemoveGalleryWatchFunction                     //
+///////////////////////////////////////////////////////////////////////////////
+
+MediaGalleriesRemoveGalleryWatchFunction::
+~MediaGalleriesRemoveGalleryWatchFunction() {
+}
+
+bool MediaGalleriesRemoveGalleryWatchFunction::RunAsync() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  if (!render_view_host() || !render_view_host()->GetProcess())
+    return false;
+
+  scoped_ptr<RemoveGalleryWatch::Params> params(
+      RemoveGalleryWatch::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+
+  MediaGalleriesPreferences* preferences =
+      g_browser_process->media_file_system_registry()->GetPreferences(
+          GetProfile());
+  preferences->EnsureInitialized(base::Bind(
+      &MediaGalleriesRemoveGalleryWatchFunction::OnPreferencesInit,
+      this,
+      params->gallery_id));
+  return true;
+}
+
+void MediaGalleriesRemoveGalleryWatchFunction::OnPreferencesInit(
+    const std::string& pref_id) {
+  base::FilePath gallery_file_path;
+  MediaGalleryPrefId gallery_pref_id = 0;
+  if (!GetGalleryFilePathAndId(pref_id,
+                               GetProfile(),
+                               extension(),
+                               &gallery_file_path,
+                               &gallery_pref_id)) {
+    error_ = kInvalidGalleryId;
+    SendResponse(false);
+    return;
+  }
+
+  gallery_watch_manager()->RemoveWatch(
+      GetProfile(), extension_id(), gallery_pref_id);
+  SendResponse(true);
+}
+
+///////////////////////////////////////////////////////////////////////////////
+//              MediaGalleriesGetAllGalleryWatchFunction                     //
+///////////////////////////////////////////////////////////////////////////////
+
+MediaGalleriesGetAllGalleryWatchFunction::
+~MediaGalleriesGetAllGalleryWatchFunction() {
+}
+
+bool MediaGalleriesGetAllGalleryWatchFunction::RunAsync() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  if (!render_view_host() || !render_view_host()->GetProcess())
+    return false;
+
+  MediaGalleriesPreferences* preferences =
+      g_browser_process->media_file_system_registry()->GetPreferences(
+          GetProfile());
+  preferences->EnsureInitialized(base::Bind(
+      &MediaGalleriesGetAllGalleryWatchFunction::OnPreferencesInit,
+      this));
+  return true;
+}
+
+void MediaGalleriesGetAllGalleryWatchFunction::OnPreferencesInit() {
+  std::vector<std::string> result;
+  MediaGalleryPrefIdSet gallery_ids =
+      gallery_watch_manager()->GetWatchSet(GetProfile(), extension_id());
+  for (MediaGalleryPrefIdSet::const_iterator iter = gallery_ids.begin();
+       iter != gallery_ids.end(); ++iter) {
+    result.push_back(base::Uint64ToString(*iter));
+  }
+  results_ = GetAllGalleryWatch::Results::Create(result);

[tommycli, 2014/08/19 16:36:02]

I wonder if results_ = or SetResult is the correct thing here. They have
different meaning.

It's worth testing this.

[Oren Blasberg, 2014/08/20 22:59:36]

It looks like SetResult is used when you have just one Value:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

It's a convenience for making it into a ListValue.

In this case, we already have a list so it shouldn't be needed and setting
results_ directly is the most direct.

[tommycli, 2014/08/20 23:21:12]

Oh okay, great. thanks for verifyincg

+  SendResponse(true);
+}
+
+///////////////////////////////////////////////////////////////////////////////
+//              MediaGalleriesRemoveAllGalleryWatchFunction                  //
+///////////////////////////////////////////////////////////////////////////////
+
+MediaGalleriesRemoveAllGalleryWatchFunction::
+~MediaGalleriesRemoveAllGalleryWatchFunction() {
+}
+
+bool MediaGalleriesRemoveAllGalleryWatchFunction::RunAsync() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  if (!render_view_host() || !render_view_host()->GetProcess())
+    return false;
+
+  MediaGalleriesPreferences* preferences =
+      g_browser_process->media_file_system_registry()->GetPreferences(
+          GetProfile());
+  preferences->EnsureInitialized(base::Bind(
+      &MediaGalleriesRemoveAllGalleryWatchFunction::OnPreferencesInit,
+      this));
+  return true;
+}
+
+void MediaGalleriesRemoveAllGalleryWatchFunction::OnPreferencesInit() {
+  gallery_watch_manager()->RemoveAllWatches(
+      GetProfile(), extension_id());
+  SendResponse(true);
+}
+
 }  // namespace extensions