[Media Gallery] Add existing attached devices to the prefs in time for media galleries dialog. (issue 11304022)

[kmad...@chromium.org]

Reviewers: vandebo,

Description:
[Media Gallery] Add existing attached devices to the prefs in time for media
galleries dialog.

BUG=149154
TEST=none


Please review this at https://chromiumcodereview.appspot.com/11304022/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M chrome/browser/extensions/api/media_galleries/media_galleries_api.h
M chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
M chrome/browser/media_gallery/media_file_system_registry.h
M chrome/browser/media_gallery/media_file_system_registry.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
3afa7d604d2a712157add79d981c477175955287..4cdab51ef6b43d6f989561ab9e54b06c149c9b0c
100644
--- a/chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
+++ b/chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
@@ -67,6 +67,14 @@ bool ApiIsAccessible(std::string* error) {
MediaGalleriesGetMediaFileSystemsFunction::
~MediaGalleriesGetMediaFileSystemsFunction() {}

+bool MediaGalleriesGetMediaFileSystemsFunction::HasPermission() {
+ if (!ExtensionFunction::HasPermission())
+ return false;
+ MediaFileSystemRegistry::GetInstance()->AddAttachedMediaDeviceGalleries(
+ render_view_host());
+ return true;
+}
+
bool MediaGalleriesGetMediaFileSystemsFunction::RunImpl() {
if (!ApiIsAccessible(&error_))
return false;
Index: chrome/browser/extensions/api/media_galleries/media_galleries_api.h
diff --git
a/chrome/browser/extensions/api/media_galleries/media_galleries_api.h
b/chrome/browser/extensions/api/media_galleries/media_galleries_api.h
index
5ef77613a17f2299dcdb2a0a807aeef1b90c5c7c..04636c61b20baa2c0f438e0bc2821d20e08983e0
100644
--- a/chrome/browser/extensions/api/media_galleries/media_galleries_api.h
+++ b/chrome/browser/extensions/api/media_galleries/media_galleries_api.h
@@ -22,6 +22,9 @@ class MediaGalleriesGetMediaFileSystemsFunction

protected:
virtual ~MediaGalleriesGetMediaFileSystemsFunction();
+
+ // ExtensionFunction:: override.
+ virtual bool HasPermission() OVERRIDE;
virtual bool RunImpl() OVERRIDE;

private:
Index: chrome/browser/media_gallery/media_file_system_registry.cc
diff --git a/chrome/browser/media_gallery/media_file_system_registry.cc
b/chrome/browser/media_gallery/media_file_system_registry.cc
index
3cb34d76273748cede69d5423d9b0143edcf7ec6..5bc2480a00e0aeb72871d105c5922b2cc254b1d8
100644
--- a/chrome/browser/media_gallery/media_file_system_registry.cc
+++ b/chrome/browser/media_gallery/media_file_system_registry.cc
@@ -482,9 +482,6 @@ void
MediaFileSystemRegistry::GetMediaFileSystemsForExtension(
Profile::FromBrowserContext(rvh->GetProcess()->GetBrowserContext());
MediaGalleriesPreferences* preferences =
MediaGalleriesPreferencesFactory::GetForProfile(profile);
-
- if (!ContainsKey(extension_hosts_map_, profile))
- AddAttachedMediaDeviceGalleries(preferences);
MediaGalleryPrefIdSet galleries =
preferences->GalleriesForExtension(*extension);

@@ -578,6 +575,32 @@ void
MediaFileSystemRegistry::OnRemovableStorageDetached(
}
}

+void MediaFileSystemRegistry::AddAttachedMediaDeviceGalleries(
+ const content::RenderViewHost* rvh) {
+ Profile* profile =
+ Profile::FromBrowserContext(rvh->GetProcess()->GetBrowserContext());
+ if (ContainsKey(extension_hosts_map_, profile))
+ return; // Devices already enumerated.
+
+ // SystemMonitor may be NULL in unit tests.
+ SystemMonitor* system_monitor = SystemMonitor::Get();
+ if (!system_monitor)
+ return;
+
+ std::vector<SystemMonitor::RemovableStorageInfo> existing_devices =
+ system_monitor->GetAttachedRemovableStorage();
+ MediaGalleriesPreferences* preferences =
+ MediaGalleriesPreferencesFactory::GetForProfile(profile);
+ DCHECK(preferences);
+ for (size_t i = 0; i < existing_devices.size(); i++) {
+ if (!MediaStorageUtil::IsMediaDevice(existing_devices[i].device_id))
+ continue;
+ preferences->AddGallery(existing_devices[i].device_id,
+ existing_devices[i].name, FilePath(),
+ false /*not user added*/);
+ }
+}
+
/******************
* Private methods
******************/
@@ -678,24 +701,6 @@ void
MediaFileSystemRegistry::RemoveScopedMtpDeviceMapEntry(
}
#endif

-void MediaFileSystemRegistry::AddAttachedMediaDeviceGalleries(
- MediaGalleriesPreferences* preferences) {
- // SystemMonitor may be NULL in unit tests.
- SystemMonitor* system_monitor = SystemMonitor::Get();
- if (!system_monitor)
- return;
-
- std::vector<SystemMonitor::RemovableStorageInfo> existing_devices =
- system_monitor->GetAttachedRemovableStorage();
- for (size_t i = 0; i < existing_devices.size(); i++) {
- if (!MediaStorageUtil::IsMediaDevice(existing_devices[i].device_id))
- continue;
- preferences->AddGallery(existing_devices[i].device_id,
- existing_devices[i].name, FilePath(),
- false /*not user added*/);
- }
-}
-
void MediaFileSystemRegistry::OnExtensionGalleriesHostEmpty(
Profile* profile, const std::string& extension_id) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Index: chrome/browser/media_gallery/media_file_system_registry.h
diff --git a/chrome/browser/media_gallery/media_file_system_registry.h
b/chrome/browser/media_gallery/media_file_system_registry.h
index
45f3163d0b4e665e26968a21e35dfe9015718a92..8215b6166746b9b7b35ca825a6170b0983e5209f
100644
--- a/chrome/browser/media_gallery/media_file_system_registry.h
+++ b/chrome/browser/media_gallery/media_file_system_registry.h
@@ -96,6 +96,10 @@ class MediaFileSystemRegistry
const FilePath::StringType& location) OVERRIDE;
virtual void OnRemovableStorageDetached(const std::string& id) OVERRIDE;

+ // Register all the media devices found in system monitor as
auto-detected
+ // galleries for a given RVH profile preferences. Called on the UI
thread.
+ void AddAttachedMediaDeviceGalleries(const content::RenderViewHost* rvh);
+
private:
friend struct base::DefaultLazyInstanceTraits<MediaFileSystemRegistry>;
class MediaFileSystemContextImpl;
@@ -129,10 +133,6 @@ class MediaFileSystemRegistry
const FilePath::StringType& device_location);
#endif

- // Register all the media devices found in system monitor as
auto-detected
- // galleries for the passed |preferences|.
- void AddAttachedMediaDeviceGalleries(MediaGalleriesPreferences*
preferences);
-
void OnExtensionGalleriesHostEmpty(Profile* profile,
const std::string& extension_id);

[van...@chromium.org]

http://codereview.chromium.org/11304022/diff/2001/chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
File
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

http://codereview.chromium.org/11304022/diff/2001/chrome/browser/extensions/api/media_galleries/media_galleries_api.cc#newcode73
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:73:
MediaFileSystemRegistry::GetInstance()->AddAttachedMediaDeviceGalleries(
I think putting this here abuses it's use. Let just call it in the
relevant places in RunImpl()

http://codereview.chromium.org/11304022/diff/2001/chrome/browser/media_gallery/media_file_system_registry.cc
File chrome/browser/media_gallery/media_file_system_registry.cc (left):

http://codereview.chromium.org/11304022/diff/2001/chrome/browser/media_gallery/media_file_system_registry.cc#oldcode487
chrome/browser/media_gallery/media_file_system_registry.cc:487:
AddAttachedMediaDeviceGalleries(preferences);
Maybe we should still call the new function here?

http://codereview.chromium.org/11304022/

[kmad...@chromium.org]

Addressed review comments. PTAL.

Thanks.

http://codereview.chromium.org/11304022/diff/2001/chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
File
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

http://codereview.chromium.org/11304022/diff/2001/chrome/browser/extensions/api/media_galleries/media_galleries_api.cc#newcode73
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:73:
MediaFileSystemRegistry::GetInstance()->AddAttachedMediaDeviceGalleries(

On 2012/10/27 19:57:57, vandebo wrote:
> I think putting this here abuses it's use. Let just call it in the
relevant
> places in RunImpl()

Moved this function call to the top of RunImpl() function. I guess
irrespective of the interactive mode, we want to add the attached device
galleries to the preferences.

http://codereview.chromium.org/11304022/diff/2001/chrome/browser/media_gallery/media_file_system_registry.cc
File chrome/browser/media_gallery/media_file_system_registry.cc (left):

http://codereview.chromium.org/11304022/diff/2001/chrome/browser/media_gallery/media_file_system_registry.cc#oldcode487
chrome/browser/media_gallery/media_file_system_registry.cc:487:
AddAttachedMediaDeviceGalleries(preferences);

On 2012/10/27 19:57:57, vandebo wrote:
> Maybe we should still call the new function here?

I don't think this call is required here. This function is called from
MediaGalleriesGetMediaFileSystemsFunction. If we are going to call
AddAttachedMediaDeviceGalleries from
MediaGalleriesGetMediaFileSystemsFunction::RunImpl, this call is not
required here. Please correct me if I am wrong.

http://codereview.chromium.org/11304022/

[van...@chromium.org]

On 2012/10/28 01:13:43, kmadhusu wrote:
> Addressed review comments. PTAL.

> Thanks.


http://codereview.chromium.org/11304022/diff/2001/chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
> File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
> (right):


http://codereview.chromium.org/11304022/diff/2001/chrome/browser/extensions/api/media_galleries/media_galleries_api.cc#newcode73
> chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:73:
> MediaFileSystemRegistry::GetInstance()->AddAttachedMediaDeviceGalleries(
> On 2012/10/27 19:57:57, vandebo wrote:
> > I think putting this here abuses it's use. Let just call it in the
> relevant
> > places in RunImpl()

> Moved this function call to the top of RunImpl() function. I guess
irrespective
> of the interactive mode, we want to add the attached device galleries to
> the
> preferences.

Hmm, I'd like it to be more encapsulated. How about this: rename it to
EnsureAttachedMediaDevicesAdded or something along those lines and then
call it
both from MediaFileSystemRegistry::GetMediaFileSystemsForExtension and
MediaGalleriesGetMediaFileSystemsFunction::ShowDialog with a comment on the
later one saying that we need to make the attached devices are added as
galleries before invoking the dialog. If you like that plan, then LGTM.

http://codereview.chromium.org/11304022/diff/2001/chrome/browser/media_gallery/media_file_system_registry.cc
> File chrome/browser/media_gallery/media_file_system_registry.cc (left):


http://codereview.chromium.org/11304022/diff/2001/chrome/browser/media_gallery/media_file_system_registry.cc#oldcode487
> chrome/browser/media_gallery/media_file_system_registry.cc:487:
> AddAttachedMediaDeviceGalleries(preferences);
> On 2012/10/27 19:57:57, vandebo wrote:
> > Maybe we should still call the new function here?

> I don't think this call is required here. This function is called from
> MediaGalleriesGetMediaFileSystemsFunction. If we are going to call
> AddAttachedMediaDeviceGalleries from
> MediaGalleriesGetMediaFileSystemsFunction::RunImpl, this call is not
> required
> here. Please correct me if I am wrong.

If called from RunImpl, it's not needed here, no.

In a follow up CL, please see if you can add a API test (see
media_galleries_apitest.cc)

http://codereview.chromium.org/11304022/

[kmad...@chromium.org]

I am fine with renaming AddAttached...() function to
EnsureAttachedMediaDevicesAdded().

Your suggestion (moving this call to ::GetMediaFileSystemsForExtension and
::ShowDialog) will make the code fragile. AddAttachedMediaDeviceGalleries()
initializes the preferences with the attached media galleries. This function
should be called irrespective of the media galleries api extension call. In
future, if you add more switch cases, we should remember to call this
AddAttached...() function in all those extension api handlers. WDYT?