Connect MediaFileSystemRegistry with MediaGalleriesPreferences

Connect MediaFileSystemRegistry with MediaGalleriesPreferences

Refactor MediaFileSystemRegistry to make state easier to manage and connect the extension preferences to the registry.

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

[Lei Zhang, 2012-08-24 04:39:04]

https://chromiumcodereview.appspot.com/10871049/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://chromiumcodereview.appspot.com/10871049/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:73:
media_fs_registry->GetMediaFileSystemsForExtension(rph, *GetExtension());
Don't you need to pass in a rvh instead of a rph?

I'm trying to implement "chrome.mediaGalleries.registerForMediaFileSystemChange"
and I realized I need a place to store the registered callbacks. Maybe
MediaFileSystemRegistry is the place to do it?

https://chromiumcodereview.appspot.com/10871049/diff/1/chrome/browser/media_g...
File chrome/browser/media_gallery/media_file_system_registry.h (right):

https://chromiumcodereview.appspot.com/10871049/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_file_system_registry.h:19: #include
"content/public/browser/notification_observer.h"
You can remove these guys if you are removing NotificationObserver.

[vandebo (ex-Chrome), 2012-09-04 21:02:20]

As we discussed, tests will come later in the review process or in a follow up
CL.

https://chromiumcodereview.appspot.com/10871049/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://chromiumcodereview.appspot.com/10871049/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:73:
media_fs_registry->GetMediaFileSystemsForExtension(rph, *GetExtension());
On 2012/08/24 04:39:04, Lei Zhang wrote:
> Don't you need to pass in a rvh instead of a rph?

Done.

> I'm trying to implement
"chrome.mediaGalleries.registerForMediaFileSystemChange"
> and I realized I need a place to store the registered callbacks. Maybe
> MediaFileSystemRegistry is the place to do it?

Sounds pretty reasonable.

https://chromiumcodereview.appspot.com/10871049/diff/1/chrome/browser/media_g...
File chrome/browser/media_gallery/media_file_system_registry.h (right):

https://chromiumcodereview.appspot.com/10871049/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_file_system_registry.h:19: #include
"content/public/browser/notification_observer.h"
On 2012/08/24 04:39:04, Lei Zhang wrote:
> You can remove these guys if you are removing NotificationObserver.

Done.

[Lei Zhang, 2012-09-04 23:38:51]

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/int...
File chrome/browser/intents/device_attached_intent_source.cc (right):

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/int...
chrome/browser/intents/device_attached_intent_source.cc:107:
DCHECK(MediaStorageUtil::IsRemovableDevice(id));
I think you need to remove this?

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:53: struct
InvalidatedGallery {
Move this to inside MediaFileSystemRegistry::OnRemovableStorageDetached() since
it's only used there?

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:58: typedef
std::list<InvalidatedGallery> InvalidatedGalleries;
nit: #include <list>

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:94: // For each
galelry in the list of permitted |galleries|, looks up or creates
nit: typo

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:155: if (gallery ==
pref_id_map_.end())
Can this ever happen?

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:164: // Indicate that
the passed render process will reference the file system
render process -> rvh

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:224: friend class
base::RefCounted<ExtensionGalleriesHost>;
nit: blan line after this

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:235: std::set<const
WebContents*> dead_rvhs = rph_info->second;
|dead_rvhs| is weirdly named, since it's a set of WebContents.

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:243:
DCHECK(rph_refs_.find(rph) == rph_refs_.end());
You can use ContainsKey() here.

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:315: if
(galleries.empty() && !extension_host)
Can you comment what this case means?
"Never had access to / tried to access any galleries and still has no access to
any galleries".

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:318: if
(!extension_host) {
ditto, please comment.
"Has access to galleries, but has never accessed before."

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:352: for
(ExtensionHostMap::const_iterator extension_host_it =
Do you think it'll be cleaner to put the second and third for-loop into a helper
function?

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:398: Profile*
profile, const std::string& extension_id) {
Also DCHECK this is on the UI thread since we access |extension_hosts_map_|
here.

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
File chrome/browser/media_gallery/media_file_system_registry.h (right):

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.h:22: class FilePath;
nit: you don't need this since you're adding file_path.h.

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.h:68: // Map a profile
and extension to the ExtensionGallerisHost.
nit: typo

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.h:69: typedef
std::map<Profile*, ExtensionHostMap>
You can probably use const Profile* here and on line 76?

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.h:70:
ExtensionGalleriesHostMap;
nit: fits on previous line.

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
File chrome/browser/media_gallery/media_storage_util.cc (right):

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_storage_util.cc:178: return
FilePath(FindRemovableStorageLocationById(device_id));
I feel FindRemovableStorageLocationById() should just return a FilePath instead.

[vandebo (ex-Chrome), 2012-09-05 00:58:12]

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/int...
File chrome/browser/intents/device_attached_intent_source.cc (right):

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/int...
chrome/browser/intents/device_attached_intent_source.cc:107:
DCHECK(MediaStorageUtil::IsRemovableDevice(id));
On 2012/09/04 23:38:51, Lei Zhang wrote:
> I think you need to remove this?

Why? Device attach intent should only trigger for removable devices.

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:53: struct
InvalidatedGallery {
On 2012/09/04 23:38:51, Lei Zhang wrote:
> Move this to inside MediaFileSystemRegistry::OnRemovableStorageDetached()
since
> it's only used there?

I did try that originally, but the compiler complains: "template argument ...
uses local type"
Googling says '§14.3.1/2 from the 2003 C++ Standard: A local type, a type with
no linkage, an unnamed type or a type compounded from any of these types shall
not be used as a template-argument for a template type-parameter.'

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:58: typedef
std::list<InvalidatedGallery> InvalidatedGalleries;
On 2012/09/04 23:38:51, Lei Zhang wrote:
> nit: #include <list>

Done.

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:94: // For each
galelry in the list of permitted |galleries|, looks up or creates
On 2012/09/04 23:38:51, Lei Zhang wrote:
> nit: typo

Done.

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:155: if (gallery ==
pref_id_map_.end())
On 2012/09/04 23:38:51, Lei Zhang wrote:
> Can this ever happen?

Yes.  If an extension doesn't have access to a removable device (i.e. not all
galleries) and it gets unplugged,
MediaFileSystemRegistry::OnRemovableStorageDetached will call this.

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:164: // Indicate that
the passed render process will reference the file system
On 2012/09/04 23:38:51, Lei Zhang wrote:
> render process -> rvh

Done.

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:224: friend class
base::RefCounted<ExtensionGalleriesHost>;
On 2012/09/04 23:38:51, Lei Zhang wrote:
> nit: blan line after this

Done.

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:235: std::set<const
WebContents*> dead_rvhs = rph_info->second;
On 2012/09/04 23:38:51, Lei Zhang wrote:
> |dead_rvhs| is weirdly named, since it's a set of WebContents.

Done.

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:243:
DCHECK(rph_refs_.find(rph) == rph_refs_.end());
On 2012/09/04 23:38:51, Lei Zhang wrote:
> You can use ContainsKey() here.

Done.

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:315: if
(galleries.empty() && !extension_host)
On 2012/09/04 23:38:51, Lei Zhang wrote:
> Can you comment what this case means?
> "Never had access to / tried to access any galleries and still has no access
to
> any galleries".

Done.

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:318: if
(!extension_host) {
On 2012/09/04 23:38:51, Lei Zhang wrote:
> ditto, please comment.
> "Has access to galleries, but has never accessed before."

I'm not sure this chunk of code needs a comment.... If we don't have an
extension galleries host, we make one.  Any thoughts on a comment that doesn't
just repeat what the code says?

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:352: for
(ExtensionHostMap::const_iterator extension_host_it =
On 2012/09/04 23:38:51, Lei Zhang wrote:
> Do you think it'll be cleaner to put the second and third for-loop into a
helper
> function?

I realized that this code was inefficient in that it stores the exploded state
(profiles x extension_hosts x pref_ids).  I changed it to use less space, which
moves some of the loops around.  I also made the iterators names more readable. 
Let me know what you think about abstracting any of it to improve readability.

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:398: Profile*
profile, const std::string& extension_id) {
On 2012/09/04 23:38:51, Lei Zhang wrote:
> Also DCHECK this is on the UI thread since we access |extension_hosts_map_|
> here.

Done.

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
File chrome/browser/media_gallery/media_file_system_registry.h (right):

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.h:22: class FilePath;
On 2012/09/04 23:38:51, Lei Zhang wrote:
> nit: you don't need this since you're adding file_path.h.

Done.

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.h:68: // Map a profile
and extension to the ExtensionGallerisHost.
On 2012/09/04 23:38:51, Lei Zhang wrote:
> nit: typo

Done.

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.h:69: typedef
std::map<Profile*, ExtensionHostMap>
On 2012/09/04 23:38:51, Lei Zhang wrote:
> You can probably use const Profile* here and on line 76?

I had that originally, but removed it because
MediaGalleriesPreferencesFactory::GetForProfile calls
ProfileKeyedServiceFactory:GetServiceForProfile and they both take a non-const
Profile.

https://chromiumcodereview.appspot.com/10871049/diff/12003/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.h:70:
ExtensionGalleriesHostMap;
On 2012/09/04 23:38:51, Lei Zhang wrote:
> nit: fits on previous line.

Done.

[Lei Zhang, 2012-09-05 06:29:22]

https://chromiumcodereview.appspot.com/10871049/diff/14010/chrome/browser/med...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

https://chromiumcodereview.appspot.com/10871049/diff/14010/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:60: //typedef
std::list<InvalidatedGalleryInfo> InvalidatedGalleriesInfo;
Remove debug code?

https://chromiumcodereview.appspot.com/10871049/diff/14010/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:126:
pref_id_map_[*id] = new_entry;
You can safely insert() here since we know there's no entry with key = *id.

https://chromiumcodereview.appspot.com/10871049/diff/14010/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:132: if
(galleries.size() != pref_id_map_.size()) {
The size of |galleries| is never greater than |pref_id_map_| size, right? Can
you mention this and use < here instead?

https://chromiumcodereview.appspot.com/10871049/diff/14010/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:257:
rph_refs_[rph].erase(contents);
If you just get the iterator once, you can use it three times here.

https://chromiumcodereview.appspot.com/10871049/diff/14010/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:329: extension_host =
extension_hosts_map_[profile][extension->id()].get();
you can write this as:
extension_host = new ExtensionGalleriesHost(...);
extension_hosts_map_[profile][extension->id()] = extension_host;

https://chromiumcodereview.appspot.com/10871049/diff/14010/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:348:
std::list<InvalidatedGalleriesInfo> invalid_galleries_info;
Does this really need to be a list? You're probably better off with a vector
since it'll use less memory and you won't have to use an iterator at line 372.

https://chromiumcodereview.appspot.com/10871049/diff/14010/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:371: //
TODO(kmadhusu, vandebo): Clean up this code. http://crbug.com/140340.
Does this still apply?

https://chromiumcodereview.appspot.com/10871049/diff/14010/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:421:
extension_hosts_map_.erase(profile);
You can pass |extension_hosts| here to use the more efficient version of
map::erase().

[vandebo (ex-Chrome), 2012-09-05 18:25:48]

https://chromiumcodereview.appspot.com/10871049/diff/14010/chrome/browser/med...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

https://chromiumcodereview.appspot.com/10871049/diff/14010/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:60: //typedef
std::list<InvalidatedGalleryInfo> InvalidatedGalleriesInfo;
On 2012/09/05 06:29:22, Lei Zhang wrote:
> Remove debug code?

Done.

https://chromiumcodereview.appspot.com/10871049/diff/14010/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:126:
pref_id_map_[*id] = new_entry;
On 2012/09/05 06:29:22, Lei Zhang wrote:
> You can safely insert() here since we know there's no entry with key = *id.

You'd like me to use insert() here?  The syntax is ugly and we don't have
map_util.h yet...

https://chromiumcodereview.appspot.com/10871049/diff/14010/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:132: if
(galleries.size() != pref_id_map_.size()) {
On 2012/09/05 06:29:22, Lei Zhang wrote:
> The size of |galleries| is never greater than |pref_id_map_| size, right? Can
> you mention this and use < here instead?

I think that's true, but this shortcut only works when we make file systems for
every gallery.  So if galleries where less than pref_id_map_.size(), then
something would be wrong with the assumption of the shortcut.  I'd rather leave
it as a != comparison.

https://chromiumcodereview.appspot.com/10871049/diff/14010/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:257:
rph_refs_[rph].erase(contents);
On 2012/09/05 06:29:22, Lei Zhang wrote:
> If you just get the iterator once, you can use it three times here.

Done.

https://chromiumcodereview.appspot.com/10871049/diff/14010/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:329: extension_host =
extension_hosts_map_[profile][extension->id()].get();
On 2012/09/05 06:29:22, Lei Zhang wrote:
> you can write this as:
> extension_host = new ExtensionGalleriesHost(...);
> extension_hosts_map_[profile][extension->id()] = extension_host;

Done.

https://chromiumcodereview.appspot.com/10871049/diff/14010/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:348:
std::list<InvalidatedGalleriesInfo> invalid_galleries_info;
On 2012/09/05 06:29:22, Lei Zhang wrote:
> Does this really need to be a list? You're probably better off with a vector
> since it'll use less memory and you won't have to use an iterator at line 372.

Done.

https://chromiumcodereview.appspot.com/10871049/diff/14010/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:371: //
TODO(kmadhusu, vandebo): Clean up this code. http://crbug.com/140340.
On 2012/09/05 06:29:22, Lei Zhang wrote:
> Does this still apply?

Not sure, but moved it to RevokeGalleryByPrefId.

https://chromiumcodereview.appspot.com/10871049/diff/14010/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:421:
extension_hosts_map_.erase(profile);
On 2012/09/05 06:29:22, Lei Zhang wrote:
> You can pass |extension_hosts| here to use the more efficient version of
> map::erase().

Done.

[Lei Zhang, 2012-09-05 18:31:01]

lgtm

https://chromiumcodereview.appspot.com/10871049/diff/14010/chrome/browser/med...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

https://chromiumcodereview.appspot.com/10871049/diff/14010/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:126:
pref_id_map_[*id] = new_entry;
On 2012/09/05 18:25:48, vandebo wrote:
> On 2012/09/05 06:29:22, Lei Zhang wrote:
> > You can safely insert() here since we know there's no entry with key = *id.
> 
> You'd like me to use insert() here?  The syntax is ugly and we don't have
> map_util.h yet...

Meh, ok. I don't know what the STL designers were thinking when they wrote
insert().

[commit-bot: I haz the power, 2012-09-07 21:43:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/10871049/8025

[commit-bot: I haz the power, 2012-09-07 23:38:07]

Change committed as 155534