Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12091054:
Media Galleries: Always call MediaFileSystemRegistry::GetMediaFileSystemsForExtension() before show… (Closed)
|
Created:
7 years, 10 months ago by Lei Zhang Modified:
7 years, 10 months ago Reviewers:
vandebo (ex-Chrome) CC:
chromium-reviews, Aaron Boodman, chromium-apps-reviews_chromium.org Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionMedia Galleries: Always call MediaFileSystemRegistry::GetMediaFileSystemsForExtension() before showing a dialog.
BUG=158849
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179815
Patch Set 1 #
Total comments: 7
Patch Set 2 : #
Total comments: 3
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : #
Messages
Total messages: 11 (0 generated)
The first time one opens a media gallery permission dialog with interactivity = yes, the MediaFileSystemRegistry does not have an entry in |extension_hosts_map_| for the extension. Whereas for all other interactivity settings, there is. This makes the state of |extension_hosts_map_| consistent.
On 2013/01/30 00:23:26, Lei Zhang wrote: > The first time one opens a media gallery permission dialog with interactivity = > yes, the MediaFileSystemRegistry does not have an entry in > |extension_hosts_map_| for the extension. Whereas for all other interactivity > settings, there is. This makes the state of |extension_hosts_map_| consistent. Evan was aware of this difference and made sure to handle it in the dialog controller. Is this solving a problem/bug? It's not free because it requires figuring out the galleries an extra time.
On 2013/01/30 02:32:32, vandebo wrote: > On 2013/01/30 00:23:26, Lei Zhang wrote: > > The first time one opens a media gallery permission dialog with interactivity > = > > yes, the MediaFileSystemRegistry does not have an entry in > > |extension_hosts_map_| for the extension. Whereas for all other interactivity > > settings, there is. This makes the state of |extension_hosts_map_| consistent. > > Evan was aware of this difference and made sure to handle it in the dialog > controller. Is this solving a problem/bug? It's not free because it requires > figuring out the galleries an extra time. I'm in favor of having the registry in a consistent state rather than compensating for it in the dialog controller. The extra cost in computing power is negligible. What I'm trying to do in an upcoming CL is to keep the prefs and dialogs in sync. Things are going to get more complicated and having this inconsistent state would make it so much harder. The change involves having the dialog controller listen for pref changes. That way, both the registry and the dialog controller can get new auto detected devices without getting into a race over who got the notification first. In order for that to work, MediaFileSystemRegistry::OnRemovableStorageAttached() has to know about the extension in |extension_hosts_map_| to notify their prefs of the new gallery.
On 2013/01/30 02:54:08, Lei Zhang wrote: > On 2013/01/30 02:32:32, vandebo wrote: > > On 2013/01/30 00:23:26, Lei Zhang wrote: > > > The first time one opens a media gallery permission dialog with > interactivity > > = > > > yes, the MediaFileSystemRegistry does not have an entry in > > > |extension_hosts_map_| for the extension. Whereas for all other > interactivity > > > settings, there is. This makes the state of |extension_hosts_map_| > consistent. > > > > Evan was aware of this difference and made sure to handle it in the dialog > > controller. Is this solving a problem/bug? It's not free because it requires > > figuring out the galleries an extra time. > > I'm in favor of having the registry in a consistent state rather than > compensating for it in the dialog controller. The extra cost in computing power > is negligible. To be clear, the registry is in a consistent state already. > What I'm trying to do in an upcoming CL is to keep the prefs and dialogs in > sync. Things are going to get more complicated and having this inconsistent > state would make it so much harder. > > The change involves having the dialog controller listen for pref changes. That > way, both the registry and the dialog controller can get new auto detected > devices without getting into a race over who got the notification first. In > order for that to work, MediaFileSystemRegistry::OnRemovableStorageAttached() > has to know about the extension in |extension_hosts_map_| to notify their prefs > of the new gallery. Will this still work correctly for extensions that currently don't have permission for any extensions? That's another case where we ensure that |extension_hosts_map_| is empty. https://codereview.chromium.org/12091054/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/12091054/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:96: registry->GetMediaFileSystemsForExtension( Since this isn't strictly required, add a comment about why we do it anyway. https://codereview.chromium.org/12091054/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:102: &MediaGalleriesGetMediaFileSystemsFunction::ShowDialogIfNoGalleries, Can you rearrange this code to eliminate the code duplication? https://codereview.chromium.org/12091054/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:122: ShowDialog(); Or maybe the comment should go here. https://codereview.chromium.org/12091054/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/media_galleries/media_galleries_api.h (right): https://codereview.chromium.org/12091054/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/media_galleries/media_galleries_api.h:30: void AlwaysShowDialog( nit: ShowDialog(
We'll have to modify the registry for the case of extensions with no access to any file systems. That can happen in a later CL. https://codereview.chromium.org/12091054/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/12091054/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:96: registry->GetMediaFileSystemsForExtension( On 2013/01/30 17:47:30, vandebo wrote: > Since this isn't strictly required, add a comment about why we do it anyway. Done. https://codereview.chromium.org/12091054/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:102: &MediaGalleriesGetMediaFileSystemsFunction::ShowDialogIfNoGalleries, On 2013/01/30 17:47:30, vandebo wrote: > Can you rearrange this code to eliminate the code duplication? Done. https://codereview.chromium.org/12091054/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/media_galleries/media_galleries_api.h (right): https://codereview.chromium.org/12091054/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/media_galleries/media_galleries_api.h:30: void AlwaysShowDialog( On 2013/01/30 17:47:30, vandebo wrote: > nit: ShowDialog( Name's already taken on line 46.
LGTM https://codereview.chromium.org/12091054/diff/6001/chrome/browser/extensions/... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/12091054/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:92: // On the first call to this API, the MediaFileSystemRegistry would not This comment is pretty opaque to the actual reason... How about something like The DialogController uses preferences changes to update itself if the gallery list changes when it is open. To ensure that MediaFileSystemRegistry updates the preferences, we need to call GetMediaFileSystemsForExtension so that it knows that something is interested in updates for |GetExtension()|. https://codereview.chromium.org/12091054/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:118: void MediaGalleriesGetMediaFileSystemsFunction::AlwaysShowDialog( Hmm, I wonder if it would be worthwhile to add a bind helper that would eat an argument, so you could just use that at the call site...
https://codereview.chromium.org/12091054/diff/6001/chrome/browser/extensions/... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/12091054/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:92: // On the first call to this API, the MediaFileSystemRegistry would not On 2013/01/30 21:24:25, vandebo wrote: > This comment is pretty opaque to the actual reason... How about something like > > The DialogController uses preferences changes to update itself if the gallery > list changes when it is open. To ensure that MediaFileSystemRegistry updates > the preferences, we need to call GetMediaFileSystemsForExtension so that it > knows that something is interested in updates for |GetExtension()|. Done. I made it opaque because the statements in the suggested comments are not yet true. But I suppose they will be soon.
LGTM https://codereview.chromium.org/12091054/diff/9001/chrome/browser/extensions/... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/12091054/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:92: // To keep the media galleries permissions dialogs in sync with pref nit nit: the cause and effect seem backward in this first sentence... an attempt to tighten it up... The MediaFileSystemRegistry only updates preferences for extensions that it knows are in use. Since this may be the first call to chrome.getMediaFileSystems for this extension, GetMediaFileSystemsForExtension() is called here solely so that MediaFileSystemRegistry will send preference changes.
https://codereview.chromium.org/12091054/diff/9001/chrome/browser/extensions/... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/12091054/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:92: // To keep the media galleries permissions dialogs in sync with pref On 2013/01/30 23:23:12, vandebo wrote: > nit nit: the cause and effect seem backward in this first sentence... an attempt > to tighten it up... > > The MediaFileSystemRegistry only updates preferences for extensions that it > knows are in use. Since this may be the first call to chrome.getMediaFileSystems > for this extension, GetMediaFileSystemsForExtension() is called here solely so > that MediaFileSystemRegistry will send preference changes. Sounds good.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/12091054/16001
Message was sent while issue was closed.
Change committed as 179815 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
