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

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

BUG=149154
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164856

[vandebo (ex-Chrome), 2012-10-27 19:57:57]

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

http://codereview.chromium.org/11304022/diff/2001/chrome/browser/extensions/a...
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_galler...
File chrome/browser/media_gallery/media_file_system_registry.cc (left):

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

[kmadhusu, 2012-10-28 01:13:43]

Addressed review comments. PTAL.

Thanks.

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

http://codereview.chromium.org/11304022/diff/2001/chrome/browser/extensions/a...
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_galler...
File chrome/browser/media_gallery/media_file_system_registry.cc (left):

http://codereview.chromium.org/11304022/diff/2001/chrome/browser/media_galler...
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.

[vandebo (ex-Chrome), 2012-10-28 02:49:27]

On 2012/10/28 01:13:43, kmadhusu wrote:
> Addressed review comments. PTAL.
> 
> Thanks.
> 
>
http://codereview.chromium.org/11304022/diff/2001/chrome/browser/extensions/a...
> File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
> (right):
> 
>
http://codereview.chromium.org/11304022/diff/2001/chrome/browser/extensions/a...
> 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_galler...
> File chrome/browser/media_gallery/media_file_system_registry.cc (left):
> 
>
http://codereview.chromium.org/11304022/diff/2001/chrome/browser/media_galler...
> 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)

[kmadhusu, 2012-10-28 23:03:59]

On 2012/10/28 02:49:27, vandebo wrote:
> On 2012/10/28 01:13:43, kmadhusu wrote:
> > Addressed review comments. PTAL.
> > 
> > Thanks.
> > 
> >
>
http://codereview.chromium.org/11304022/diff/2001/chrome/browser/extensions/a...
> > File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
> > (right):
> > 
> >
>
http://codereview.chromium.org/11304022/diff/2001/chrome/browser/extensions/a...
> > 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.

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?

> 
> >
>
http://codereview.chromium.org/11304022/diff/2001/chrome/browser/media_galler...
> > File chrome/browser/media_gallery/media_file_system_registry.cc (left):
> > 
> >
>
http://codereview.chromium.org/11304022/diff/2001/chrome/browser/media_galler...
> > 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)

[kmadhusu, 2012-10-29 21:06:02]

Modified code to get MediaGalleriesPreferences from MediaFileSystemRegistry.

PTAL.

Thanks.

[vandebo (ex-Chrome), 2012-10-29 21:15:17]

LGTM with comments.

http://codereview.chromium.org/11304022/diff/13002/chrome/browser/media_galle...
File chrome/browser/media_gallery/media_file_system_registry.h (right):

http://codereview.chromium.org/11304022/diff/13002/chrome/browser/media_galle...
chrome/browser/media_gallery/media_file_system_registry.h:94: // |profile|.
Registers all the media devices found in system monitor as
nit: |profile|.  This method should be used instead of calling
MediaGalleriesPreferences directly because this method also ensures that
currently attached removable devices are added to the preferences.

http://codereview.chromium.org/11304022/diff/13002/chrome/browser/media_galle...
chrome/browser/media_gallery/media_file_system_registry.h:96:
MediaGalleriesPreferences* GetProfilePreferences(Profile* profile);
GetPreferences

[kmadhusu, 2012-10-29 21:23:35]

Addressed review comments.

Thanks.

http://codereview.chromium.org/11304022/diff/13002/chrome/browser/media_galle...
File chrome/browser/media_gallery/media_file_system_registry.h (right):

http://codereview.chromium.org/11304022/diff/13002/chrome/browser/media_galle...
chrome/browser/media_gallery/media_file_system_registry.h:94: // |profile|.
Registers all the media devices found in system monitor as
On 2012/10/29 21:15:17, vandebo wrote:
> nit: |profile|.  This method should be used instead of calling
> MediaGalleriesPreferences directly because this method also ensures that
> currently attached removable devices are added to the preferences.

Done.

http://codereview.chromium.org/11304022/diff/13002/chrome/browser/media_galle...
chrome/browser/media_gallery/media_file_system_registry.h:96:
MediaGalleriesPreferences* GetProfilePreferences(Profile* profile);
On 2012/10/29 21:15:17, vandebo wrote:
> GetPreferences

Done.

[commit-bot: I haz the power, 2012-10-29 21:59:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11304022/15004

[commit-bot: I haz the power, 2012-10-29 21:59:49]

Presubmit check for 11304022-15004 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/webui/options

[kmadhusu, 2012-10-29 22:06:55]

csilv@: Can you do the OWNER's check for ui/webui/options file change?

Thanks.

[csilv, 2012-10-29 22:30:34]

ui/webui/options LGTM

[commit-bot: I haz the power, 2012-10-29 22:33:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11304022/15004

[commit-bot: I haz the power, 2012-10-30 03:40:16]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-10-30 04:47:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11304022/15004

[commit-bot: I haz the power, 2012-10-30 05:11:04]

Change committed as 164856