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)

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

Modified code to get MediaGalleriesPreferences from MediaFileSystemRegistry. PTAL. Thanks.