Media Galleries: Only try to initialize removable media galleries once per profile.

Media Galleries: Only try to initialize removable media galleries once per profile.

BUG=174836
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182141

[Lei Zhang, 2013-02-08 04:02:22]

The bug in question here is a case where |extension_hosts_map_| remains empty.
Thus GetPreferences() gets called over and over again.

[kmadhusu, 2013-02-08 19:21:11]

lgtm

[vandebo (ex-Chrome), 2013-02-08 19:40:27]

Actually I think the bug is one level down.  In
MediaGalleriesPreferences::AddGallery, we should only update the gallery type
from black listed to auto detected if the user added flag is true.

[Lei Zhang, 2013-02-08 20:01:21]

On 2013/02/08 19:40:27, vandebo wrote:
> Actually I think the bug is one level down.  In
> MediaGalleriesPreferences::AddGallery, we should only update the gallery type
> from black listed to auto detected if the user added flag is true.

That's a bug too. But it seems weird for GetPreferences() to repeatedly call
AddGallery() if the user has not called getMediaFileSystems() for a profile.

BTW, the # of profiles for a given user is typically 1 or 2. Extra memory usage
is not a concern here.

[vandebo (ex-Chrome), 2013-02-08 20:39:11]

On 2013/02/08 20:01:21, Lei Zhang wrote:
> On 2013/02/08 19:40:27, vandebo wrote:
> > Actually I think the bug is one level down.  In
> > MediaGalleriesPreferences::AddGallery, we should only update the gallery
type
> > from black listed to auto detected if the user added flag is true.
> 
> That's a bug too. But it seems weird for GetPreferences() to repeatedly call
> AddGallery() if the user has not called getMediaFileSystems() for a profile.
> 
> BTW, the # of profiles for a given user is typically 1 or 2. Extra memory
usage
> is not a concern here.

That's not my concern.  There's currently an invariant that when there are no
outstanding file systems, extension_hosts_map_ is empty.  There was previously a
bug where that invariant was broken and it broke some functionality.  So
changing that invariant will require a careful audit of the code.

This method isn't called a lot - just when the dialog controller is created and
when getting the file systems for an extension.

[Lei Zhang, 2013-02-08 22:00:32]

On 2013/02/08 20:39:11, vandebo wrote:
> That's not my concern.  There's currently an invariant that when there are no
> outstanding file systems, extension_hosts_map_ is empty.  There was previously
a
> bug where that invariant was broken and it broke some functionality.  So
> changing that invariant will require a careful audit of the code.

Do you remember what broke last time?

I looked through the |extension_hosts_map_| usage and it seems this is safe, but
you may want to double check.

Another place that's currently broken is OnRemovableStorageAttached(). If you
never call getMediaFileSystems() and go to the settings dialog, attach a
removable media drive and it won't show up because the profile is not in
|extension_hosts_map_|.

> This method isn't called a lot - just when the dialog controller is created
and
> when getting the file systems for an extension.

There's a few more places in the file change watcher that calls this, but still
not that many.

[vandebo (ex-Chrome), 2013-02-08 22:29:25]

On 2013/02/08 22:00:32, Lei Zhang wrote:
> On 2013/02/08 20:39:11, vandebo wrote:
> > That's not my concern.  There's currently an invariant that when there are
no
> > outstanding file systems, extension_hosts_map_ is empty.  There was
previously
> a
> > bug where that invariant was broken and it broke some functionality.  So
> > changing that invariant will require a careful audit of the code.
> 
> Do you remember what broke last time?

It may have just been that GetExtensionGalleriesHostCountForTests() didn't
return what I expected sometimes.

> I looked through the |extension_hosts_map_| usage and it seems this is safe,
but
> you may want to double check.
> 
> Another place that's currently broken is OnRemovableStorageAttached(). If you
> never call getMediaFileSystems() and go to the settings dialog, attach a
> removable media drive and it won't show up because the profile is not in
> |extension_hosts_map_|.
> 
> > This method isn't called a lot - just when the dialog controller is created
> and
> > when getting the file systems for an extension.
> 
> There's a few more places in the file change watcher that calls this, but
still
> not that many.

Can we fix the low level bug first?  There may be other ways to trigger it aside
from the path exercised here.

[Lei Zhang, 2013-02-12 17:38:17]

Rebased in patch set 2.

[vandebo (ex-Chrome), 2013-02-12 22:44:30]

Please update the issue description; if you think it needs to be updated.

https://chromiumcodereview.appspot.com/12212083/diff/8005/chrome/browser/medi...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

https://chromiumcodereview.appspot.com/12212083/diff/8005/chrome/browser/medi...
chrome/browser/media_gallery/media_file_system_registry.cc:548: // Create an
empty entry so the initialization code below only gets called
I still don't see the value of this.  When we restart chrome, the code below
here will get run again anyway...

[Lei Zhang, 2013-02-12 23:01:53]

https://chromiumcodereview.appspot.com/12212083/diff/8005/chrome/browser/medi...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

https://chromiumcodereview.appspot.com/12212083/diff/8005/chrome/browser/medi...
chrome/browser/media_gallery/media_file_system_registry.cc:548: // Create an
empty entry so the initialization code below only gets called
On 2013/02/12 22:44:30, vandebo wrote:
> I still don't see the value of this.  When we restart chrome, the code below
> here will get run again anyway...

While Chrome is running, every time GetPreferences() gets called, we can return
early on line 546 and avoid doing the work below. This CL is just a small
time/space tradeoff at this point.

[vandebo (ex-Chrome), 2013-02-12 23:11:37]

LGTM

https://chromiumcodereview.appspot.com/12212083/diff/8005/chrome/browser/medi...
File chrome/browser/media_gallery/media_file_system_registry.cc (left):

https://chromiumcodereview.appspot.com/12212083/diff/8005/chrome/browser/medi...
chrome/browser/media_gallery/media_file_system_registry.cc:807:
extension_hosts_map_.erase(extension_hosts);
nit: add a comment about not erasing extension_hosts

[Lei Zhang, 2013-02-12 23:30:59]

https://chromiumcodereview.appspot.com/12212083/diff/8005/chrome/browser/medi...
File chrome/browser/media_gallery/media_file_system_registry.cc (left):

https://chromiumcodereview.appspot.com/12212083/diff/8005/chrome/browser/medi...
chrome/browser/media_gallery/media_file_system_registry.cc:807:
extension_hosts_map_.erase(extension_hosts);
On 2013/02/12 23:11:37, vandebo wrote:
> nit: add a comment about not erasing extension_hosts

Done.

[commit-bot: I haz the power, 2013-02-13 02:55:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/12212083/6005

[commit-bot: I haz the power, 2013-02-13 06:27:45]

Change committed as 182141