Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1382783002:
Store USB device permissions in website settings. (Closed)
|
Created:
5 years, 2 months ago by Reilly Grant (use Gerrit) Modified:
5 years, 1 month ago Reviewers:
Bernhard Bauer, Ken Rockot(use gerrit already), felt, mlamouri (slow - plz ping), Elliot Glaysher, raymes, erg CC:
chromium-reviews, Jeffrey Yasskin, kcarattini, markusheintz_, raymes+watch_chromium.org, scheib, sebastian1433_gmail.com Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStore USB device permissions in website settings.
This change adds a new class, ChooserContextBase, that stores a list of
chosen objects for chooser-based permissions in website settings. It is
similar to PermissionContextBase.
UsbPermissionContext extends this and adds the concept of an ephemeral
device permission (for devices that can't be recorded in persistent
storage because they are not uniquely identifiable enough).
WebUSB is the initial consumer of this new API. WebUSBPermissionProvider
has been updated to check with the UsbPermissionContext and once there
is a chooser UI to populate the list of allowed devices the
--enable-webusb-on-any-origin flag can be removed.
BUG=529950
Committed: https://crrev.com/551eaa4879fb89ca1798b1001e32608380e1405a
Cr-Commit-Position: refs/heads/master@{#359001}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Convert WebUSBPermissionStore to a PermissionContext. #
Total comments: 19
Patch Set 3 : Split permission contexts from chooser permission contexts. #
Total comments: 7
Patch Set 4 : Roll in updates from the UI patch. #Patch Set 5 : Address bauerb@'s comments. #
Total comments: 1
Patch Set 6 : Remove UsbPermissionContext and add ChooserPermissionContext helper functions. #
Total comments: 44
Patch Set 7 : More chooser, less permission. #
Total comments: 2
Patch Set 8 : Removed ChooserType for now. #
Total comments: 2
Patch Set 9 : Remove virtual functions for now. #Patch Set 10 : Fix Android build. #
Total comments: 3
Patch Set 11 : Address mlamouri@'s comments. #Dependent Patchsets: Messages
Total messages: 58 (12 generated)
reillyg@chromium.org changed reviewers: + bauerb@chromium.org, rockot@chromium.org
rockot@, please take a look at //chrome/browser/usb. I will probably have to modify this a bit to support revoking entries from the website settings UI but this is a good start. bauerb@, please take a look at //components/content_settings/core.
lgtm https://chromiumcodereview.appspot.com/1382783002/diff/1/chrome/browser/usb/w... File chrome/browser/usb/web_usb_permission_store.cc (right): https://chromiumcodereview.appspot.com/1382783002/diff/1/chrome/browser/usb/w... chrome/browser/usb/web_usb_permission_store.cc:103: explicit USBDeviceEntry(scoped_refptr<UsbDevice> device) { nit: const scoped_refptr<UsbDevice>& here and elsewhere https://chromiumcodereview.appspot.com/1382783002/diff/1/chrome/browser/usb/w... File chrome/browser/usb/web_usb_permission_store.h (right): https://chromiumcodereview.appspot.com/1382783002/diff/1/chrome/browser/usb/w... chrome/browser/usb/web_usb_permission_store.h:44: }; nit: DISALLOW_COPY_AND_ASSIGN
raymes@chromium.org changed reviewers: + raymes@chromium.org
https://chromiumcodereview.appspot.com/1382783002/diff/1/chrome/browser/usb/w... File chrome/browser/usb/web_usb_permission_store.h (right): https://chromiumcodereview.appspot.com/1382783002/diff/1/chrome/browser/usb/w... chrome/browser/usb/web_usb_permission_store.h:24: class WebUSBPermissionStore : public KeyedService, I'd feel more comfortable if we had a ChooserPermissionStore which was used for chooser-based APIs (or some plan for sharing code with bluetooth :) https://chromiumcodereview.appspot.com/1382783002/diff/1/components/content_s... File components/content_settings/core/browser/website_settings_registry.cc (right): https://chromiumcodereview.appspot.com/1382783002/diff/1/components/content_s... components/content_settings/core/browser/website_settings_registry.cc:92: WebsiteSettingsInfo::UNSYNCABLE, WebsiteSettingsInfo::LOSSY); You won't want this to be lossy https://chromiumcodereview.appspot.com/1382783002/diff/1/components/content_s... components/content_settings/core/browser/website_settings_registry.cc:92: WebsiteSettingsInfo::UNSYNCABLE, WebsiteSettingsInfo::LOSSY); I think you should also register a content setting (see content_settings_registry.cc) for USB. The content setting will be used to control globally allowing/blocking of the usb feature.
https://codereview.chromium.org/1382783002/diff/1/chrome/browser/usb/web_usb_... File chrome/browser/usb/web_usb_permission_store.cc (right): https://codereview.chromium.org/1382783002/diff/1/chrome/browser/usb/web_usb_... chrome/browser/usb/web_usb_permission_store.cc:37: static WebUSBPermissionStoreFactory* GetInstance() { You will need to add a (visible from outside) method that initializes this singleton, to set up the dependency graph correctly; see https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro.... https://codereview.chromium.org/1382783002/diff/1/chrome/browser/usb/web_usb_... chrome/browser/usb/web_usb_permission_store.cc:58: } DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1382783002/diff/1/chrome/browser/usb/web_usb_... chrome/browser/usb/web_usb_permission_store.cc:131: base::ListValue* device_list; Initialize to nullptr? Otherwise you might dereference invalid memory below if the device isn't found. https://codereview.chromium.org/1382783002/diff/1/chrome/browser/usb/web_usb_... chrome/browser/usb/web_usb_permission_store.cc:133: NOTREACHED(); Instead of `if (...) NOTREACHED()`, store a success value in a boolean and DCHECK it, like you do below. But really, you would crash below anyway if the device isn't found. https://codereview.chromium.org/1382783002/diff/1/chrome/browser/usb/web_usb_... chrome/browser/usb/web_usb_permission_store.cc:145: size_t list_index_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1382783002/diff/1/chrome/browser/usb/web_usb_... chrome/browser/usb/web_usb_permission_store.cc:158: if (!value) { Nit: Braces are optional for single-line bodies, and above you omit them. https://codereview.chromium.org/1382783002/diff/1/chrome/browser/usb/web_usb_... File chrome/browser/usb/web_usb_permission_store.h (right): https://codereview.chromium.org/1382783002/diff/1/chrome/browser/usb/web_usb_... chrome/browser/usb/web_usb_permission_store.h:16: #include "url/gurl.h" You could forward-declare GURL.
reillyg@chromium.org changed reviewers: + felt@chromium.org, pfeldman@chromium.org
Adding felt@ for //chrome/browser/permissions and pfeldman@ for //content/public/browser. Please take another look as I've essentially rewritten this patch from scratch.
A note for raymes@, the lines added in ContentSettingRegistry::Register are commented out because I'm not allowed to configure the same ContentSettingType as a content and website setting. What should I do there?
raymes@chromium.org changed reviewers: + mlamouri@chromium.org
Thanks for digging into this Reilly! I think this is a good starting point. https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/chooser_permission_context_base.cc (right): https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/chooser_permission_context_base.cc:25: const GURL& origin) { I think this is probably where we want to check the content setting, (e.g. by calling GetPermissionStatus on the base class) https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/chooser_permission_context_base.h (right): https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/chooser_permission_context_base.h:17: class ChooserPermissionContextBase : public PermissionContextBase { Mounir should have some thoughts about inheriting from PermissionContextBase so reach out to him for comments. In general I feel it's a reasonable direction to head in but some functions won't be meaningful (RequestPermission/ResetPermission/Cancel). Those things only really apply to non-chooser permissions. We would need to rework the code to make that clear. In support of this direction though, I'm starting to feel more strongly about allowing permissions which are only queryable (not requestable) in the permissions API. e.g. having a way to see if the usb/bluetooth permission is globally on/off for a site would be useful. Mounir may have a different opinion though :) https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/chooser_permission_context_base.h:23: scoped_ptr<base::ListValue> GetChosenObjects(const GURL& origin); How is an "Object" defined? Would GetPreviouslyChosenObjects be more descriptive here? https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/chooser_permission_context_base.h:24: void GrantObjectPermission(const GURL& origin, Perhaps we should pass the top level and requesting origins in to these functions https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/usb/usb_... File chrome/browser/usb/usb_permission_context.h (right): https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/usb/usb_... chrome/browser/usb/usb_permission_context.h:21: explicit UsbPermissionContext(Profile* profile); For thought: Would it be possible to implement this in a way where we don't need a separate subclass for each chooser permission? Could we always have a map of origin+guid to setting? What things might vary across different chooser permissions? https://codereview.chromium.org/1382783002/diff/20001/components/content_sett... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1382783002/diff/20001/components/content_sett... components/content_settings/core/browser/content_settings_registry.cc:159: // Register(CONTENT_SETTINGS_TYPE_USB, "usb", I'd suggest naming this one "USB" and the website setting "USB_CHOOSER_DATA" or something like that (establish a pattern for future devs and hopefully we can write code to automate some of this later :)
https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/usb/usb_... File chrome/browser/usb/usb_permission_context_factory.h (right): https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/usb/usb_... chrome/browser/usb/usb_permission_context_factory.h:18: static UsbPermissionContextFactory* GetInstance(); I think you still need to call this method from https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...
I'm not sure why you want to inherit from PermissionContextBase. This class is used to Request/Query/Clear. What would happen if these permissions are called? What is the benefit from using PermissionContextBase for UsbPermissionContext? https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:125: ContentSettingsType type() const; We want stuff in permissions/ to abstract ContentSettings, we shouldn't add this unless maybe if you return a PermissionType.
https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/chooser_permission_context_base.cc (right): https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/chooser_permission_context_base.cc:25: const GURL& origin) { On 2015/10/06 05:28:20, raymes wrote: > I think this is probably where we want to check the content setting, (e.g. by > calling GetPermissionStatus on the base class) If this class is created for CONTENT_SETTING_TYPE_USB_CHOOSER_DATA then I can call GetPermissionStatus on the PermissionContext for CONTENT_SETTING_TYPE_USB. Does that make sense? https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/chooser_permission_context_base.h (right): https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/chooser_permission_context_base.h:17: class ChooserPermissionContextBase : public PermissionContextBase { On 2015/10/06 05:28:20, raymes wrote: > Mounir should have some thoughts about inheriting from PermissionContextBase so > reach out to him for comments. In general I feel it's a reasonable direction to > head in but some functions won't be meaningful > (RequestPermission/ResetPermission/Cancel). Those things only really apply to > non-chooser permissions. We would need to rework the code to make that clear. > > In support of this direction though, I'm starting to feel more strongly about > allowing permissions which are only queryable (not requestable) in the > permissions API. e.g. having a way to see if the usb/bluetooth permission is > globally on/off for a site would be useful. Mounir may have a different opinion > though :) Alright. https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/chooser_permission_context_base.h:23: scoped_ptr<base::ListValue> GetChosenObjects(const GURL& origin); On 2015/10/06 05:28:20, raymes wrote: > How is an "Object" defined? An object is defined by the subclass. For the benefit of the site-settings UI we will need to do define a way to translate an object into a user visible item that can be individually revoked. Otherwise it's an opaque bag of properties that the particular ChooserPermissionContextBase implementation assigns meaning to. > Would GetPreviouslyChosenObjects be more descriptive here? Done. https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/chooser_permission_context_base.h:24: void GrantObjectPermission(const GURL& origin, On 2015/10/06 05:28:20, raymes wrote: > Perhaps we should pass the top level and requesting origins in to these > functions Done. https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:125: ContentSettingsType type() const; On 2015/10/06 11:03:17, Mounir Lamouri wrote: > We want stuff in permissions/ to abstract ContentSettings, we shouldn't add this > unless maybe if you return a PermissionType. That's why I made it a protected method. Otherwise I'll just have to capture the value as it passes through the ChooserPermissionContextBase constructor and then the object will be saving the same field twice. https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/usb/usb_... File chrome/browser/usb/usb_permission_context.h (right): https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/usb/usb_... chrome/browser/usb/usb_permission_context.h:21: explicit UsbPermissionContext(Profile* profile); On 2015/10/06 05:28:20, raymes wrote: > For thought: Would it be possible to implement this in a way where we don't need > a separate subclass for each chooser permission? Could we always have a map of > origin+guid to setting? > > What things might vary across different chooser permissions? Each chooser permission may define the properties that define an object differently. For example, a USB device is a vendor ID, product ID and serial number. A Bluetooth device is likely something similar but not exactly the same. The GUID used in UsbPermissionContext is a temporary identifier assigned by Chrome. It isn't the same if the device is reconnected. https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/usb/usb_... File chrome/browser/usb/usb_permission_context_factory.h (right): https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/usb/usb_... chrome/browser/usb/usb_permission_context_factory.h:18: static UsbPermissionContextFactory* GetInstance(); On 2015/10/06 10:22:44, Bernhard Bauer wrote: > I think you still need to call this method from > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro... Done. https://codereview.chromium.org/1382783002/diff/20001/components/content_sett... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1382783002/diff/20001/components/content_sett... components/content_settings/core/browser/content_settings_registry.cc:159: // Register(CONTENT_SETTINGS_TYPE_USB, "usb", On 2015/10/06 05:28:20, raymes wrote: > I'd suggest naming this one "USB" and the website setting "USB_CHOOSER_DATA" or > something like that (establish a pattern for future devs and hopefully we can > write code to automate some of this later :) Okay. I think this will also resolve mlamouri@'s concerns about extending PermissionContextBase. If I make ChooserPermissionContextBase the root of its own class hierarchy then I can require that each chooser permission also references a boolean content setting.
Alright, I now have a permission context for USB and a chooser permission context for USB. As we build up UI for chooser permissions there's more infrastructure to build for chooser permissions but I think this is a good start.
Cool - again the direction look quite good to me :) I'll refrain from reviewing this in more detail though in case Mounir has high level concerns. One downside is that this all requires a ton of boilerplate. I think we can reduce that with some refactoring but we don't have to hold this up on getting that perfect. https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/chooser_permission_context_base.h (right): https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/chooser_permission_context_base.h:23: scoped_ptr<base::ListValue> GetChosenObjects(const GURL& origin); Could we impose some structure on it? This might make doing generic things (like UI) easier down the track. e.g. could we refer to these as "devices" - GetChosenDevices (could we envision a time when this wouldn't be a device?). Could ensure the return value is a simple dictionary of string:string? Thoughts? https://codereview.chromium.org/1382783002/diff/40001/chrome/browser/usb/usb_... File chrome/browser/usb/usb_permission_context.h (right): https://codereview.chromium.org/1382783002/diff/40001/chrome/browser/usb/usb_... chrome/browser/usb/usb_permission_context.h:10: class UsbPermissionContext : public PermissionContextBase { Again, this would only be a permission that is queryable, not requestable, etc. From my perspective that is completely ok and a good thing. The durable storage permission has some similarities. We would want to make this clear in the code though (e.g. RequestPermission should hit a NOTREACHED() or something). Mounir: what are your thoughts?
ping mlamouri@ :) On Wed, 7 Oct 2015 at 15:48 <raymes@chromium.org> wrote: > Cool - again the direction look quite good to me :) I'll refrain from > reviewing > this in more detail though in case Mounir has high level concerns. > > One downside is that this all requires a ton of boilerplate. I think we can > reduce that with some refactoring but we don't have to hold this up on > getting > that perfect. > > > > https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... > File chrome/browser/permissions/chooser_permission_context_base.h > (right): > > > https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... > chrome/browser/permissions/chooser_permission_context_base.h:23 > <https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi...> > : > scoped_ptr<base::ListValue> GetChosenObjects(const GURL& origin); > Could we impose some structure on it? This might make doing generic > things (like UI) easier down the track. e.g. could we refer to these as > "devices" - GetChosenDevices (could we envision a time when this > wouldn't be a device?). Could ensure the return value is a simple > dictionary of string:string? Thoughts? > > > https://codereview.chromium.org/1382783002/diff/40001/chrome/browser/usb/usb_... > File chrome/browser/usb/usb_permission_context.h (right): > > > https://codereview.chromium.org/1382783002/diff/40001/chrome/browser/usb/usb_... > chrome/browser/usb/usb_permission_context.h:10 > <https://codereview.chromium.org/1382783002/diff/40001/chrome/browser/usb/usb_...>: > class > UsbPermissionContext : public PermissionContextBase { > Again, this would only be a permission that is queryable, not > requestable, etc. From my perspective that is completely ok and a good > thing. The durable storage permission has some similarities. We would > want to make this clear in the code though (e.g. RequestPermission > should hit a NOTREACHED() or something). > > Mounir: what are your thoughts? > > https://codereview.chromium.org/1382783002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
How do you expect PermissionType::USB to be modified? Is it only meant to be exposing the ContentSettings to the chooser?
https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/chooser_permission_context_base.h (right): https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/chooser_permission_context_base.h:23: scoped_ptr<base::ListValue> GetChosenObjects(const GURL& origin); On 2015/10/07 04:48:29, raymes wrote: > Could we impose some structure on it? This might make doing generic things (like > UI) easier down the track. e.g. could we refer to these as "devices" - > GetChosenDevices (could we envision a time when this wouldn't be a device?). Yes -- for example, a chooser-based geolocation prompt might allow the user to select a location granularity, not a particular device. > Could ensure the return value is a simple dictionary of string:string? Thoughts? https://codereview.chromium.org/1382783002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/chooser_permission_context_base.cc (right): https://codereview.chromium.org/1382783002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/chooser_permission_context_base.cc:17: const char* const kObjectListKey = "chosen-objects"; Use char[] instead of char* (which I think also lets you get rid of one const). Also, constants automatically get internal linkage, so the anonymous namespace isn't necessary. https://codereview.chromium.org/1382783002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/chooser_permission_context_base.h (right): https://codereview.chromium.org/1382783002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/chooser_permission_context_base.h:20: }; No semicolon. https://codereview.chromium.org/1382783002/diff/40001/chrome/browser/usb/usb_... File chrome/browser/usb/usb_chooser_permission_context.cc (right): https://codereview.chromium.org/1382783002/diff/40001/chrome/browser/usb/usb_... chrome/browser/usb/usb_chooser_permission_context.cc:108: if (entry) { Braces are optional for one-line bodies, and in other places in this file they're not used, so they shouldn't be here either.
On 2015/10/12 11:00:08, Mounir Lamouri wrote: > How do you expect PermissionType::USB to be modified? Is it only meant to be > exposing the ContentSettings to the chooser? juncai@ is working on a patch to add such a chooser. I am working on a followup patch to this one that exposes these objects in the website settings popup so they can be individually revoked. Mocks here: go/webusb-mocks
https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/chooser_permission_context_base.h (right): https://codereview.chromium.org/1382783002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/chooser_permission_context_base.h:23: scoped_ptr<base::ListValue> GetChosenObjects(const GURL& origin); On 2015/10/12 11:03:48, Bernhard Bauer wrote: > On 2015/10/07 04:48:29, raymes wrote: > > Could we impose some structure on it? This might make doing generic things > (like > > UI) easier down the track. e.g. could we refer to these as "devices" - > > GetChosenDevices (could we envision a time when this wouldn't be a device?). > > Yes -- for example, a chooser-based geolocation prompt might allow the user to > select a location granularity, not a particular device. > > > Could ensure the return value is a simple dictionary of string:string? > Thoughts? My followup UI patch adds a virtual GetStringToDisplayForObject() method on this class that will convert a base::Value into a string to display in the UI. It also adds a virtual IsValidObject() method that subclasses can use to ensure that the values read from WebsiteSettings are well formed. I think I'll merge that part of the patch back into this one so you can see more of where I'm going with this.
> My followup UI patch adds a virtual GetStringToDisplayForObject() method on this > class that will convert a base::Value into a string to display in the UI. It > also adds a virtual IsValidObject() method that subclasses can use to ensure > that the values read from WebsiteSettings are well formed. I think I'll merge > that part of the patch back into this one so you can see more of where I'm going > with this. That makes sense. I think this patch is large enough so I'd suggest not adding more stuff to it but it's good to know the direction. Also please consider how you can separate UI code from the model. I think it would be ideal not to pollute this class with UI related code :)
https://codereview.chromium.org/1382783002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/chooser_permission_context_base.cc (right): https://codereview.chromium.org/1382783002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/chooser_permission_context_base.cc:17: const char* const kObjectListKey = "chosen-objects"; On 2015/10/12 11:03:48, Bernhard Bauer wrote: > Use char[] instead of char* (which I think also lets you get rid of one const). > Also, constants automatically get internal linkage, so the anonymous namespace > isn't necessary. Done. https://codereview.chromium.org/1382783002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/chooser_permission_context_base.h (right): https://codereview.chromium.org/1382783002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/chooser_permission_context_base.h:20: }; On 2015/10/12 11:03:48, Bernhard Bauer wrote: > No semicolon. Done. https://codereview.chromium.org/1382783002/diff/40001/chrome/browser/usb/usb_... File chrome/browser/usb/usb_chooser_permission_context.cc (right): https://codereview.chromium.org/1382783002/diff/40001/chrome/browser/usb/usb_... chrome/browser/usb/usb_chooser_permission_context.cc:108: if (entry) { On 2015/10/12 11:03:48, Bernhard Bauer wrote: > Braces are optional for one-line bodies, and in other places in this file > they're not used, so they shouldn't be here either. Done.
content settings LGTM https://codereview.chromium.org/1382783002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/chooser_permission_context_base.cc (right): https://codereview.chromium.org/1382783002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/chooser_permission_context_base.cc:71: DCHECK(object && IsValidObject(*object)); Split this up into two different DCHECKs.
I still can't understand why you need CONTENT_SETTINGS_TYPE_USB. The requirements for CONTENT_SETTINGS_TYPE_USB_CHOOSER_DATA are clear given that you need these for the device list. But why having a more general permission? I couldn't find anything in the mocks. If you have a design doc explaining the chooser_permission_context_base design, could you send me the link?
On 2015/10/13 10:34:03, Mounir Lamouri wrote: > I still can't understand why you need CONTENT_SETTINGS_TYPE_USB. The > requirements for CONTENT_SETTINGS_TYPE_USB_CHOOSER_DATA are clear given that you > need these for the device list. But why having a more general permission? I > couldn't find anything in the mocks. > > If you have a design doc explaining the chooser_permission_context_base design, > could you send me the link? Back in comment #5 raymes@ asked me to create a ChooserPermissionStore that could be shared between USB and Bluetooth and use a (mostly hidden) separate content setting so that the feature could be turned off by policy.
On 2015/10/13 at 15:31:50, reillyg wrote: > On 2015/10/13 10:34:03, Mounir Lamouri wrote: > > I still can't understand why you need CONTENT_SETTINGS_TYPE_USB. The > > requirements for CONTENT_SETTINGS_TYPE_USB_CHOOSER_DATA are clear given that you > > need these for the device list. But why having a more general permission? I > > couldn't find anything in the mocks. > > > > If you have a design doc explaining the chooser_permission_context_base design, > > could you send me the link? > > Back in comment #5 raymes@ asked me to create a ChooserPermissionStore that could be shared between USB and Bluetooth and use a (mostly hidden) separate content setting so that the feature could be turned off by policy. Why that other content settings need to be linked to a PermissionContextBase?
On 2015/10/13 15:42:51, Mounir Lamouri wrote: > On 2015/10/13 at 15:31:50, reillyg wrote: > > On 2015/10/13 10:34:03, Mounir Lamouri wrote: > > > I still can't understand why you need CONTENT_SETTINGS_TYPE_USB. The > > > requirements for CONTENT_SETTINGS_TYPE_USB_CHOOSER_DATA are clear given that > you > > > need these for the device list. But why having a more general permission? I > > > couldn't find anything in the mocks. > > > > > > If you have a design doc explaining the chooser_permission_context_base > design, > > > could you send me the link? > > > > Back in comment #5 raymes@ asked me to create a ChooserPermissionStore that > could be shared between USB and Bluetooth and use a (mostly hidden) separate > content setting so that the feature could be turned off by policy. > > Why that other content settings need to be linked to a PermissionContextBase? I assumed that was just how we did things these days. I can read it directly out of content settings too. It would save a lot of boilerplate code setting up the PermissionContextBase.
On 2015/10/13 15:45:38, Reilly Grant wrote: > On 2015/10/13 15:42:51, Mounir Lamouri wrote: > > On 2015/10/13 at 15:31:50, reillyg wrote: > > > On 2015/10/13 10:34:03, Mounir Lamouri wrote: > > > > I still can't understand why you need CONTENT_SETTINGS_TYPE_USB. The > > > > requirements for CONTENT_SETTINGS_TYPE_USB_CHOOSER_DATA are clear given > that > > you > > > > need these for the device list. But why having a more general permission? > I > > > > couldn't find anything in the mocks. > > > > > > > > If you have a design doc explaining the chooser_permission_context_base > > design, > > > > could you send me the link? > > > > > > Back in comment #5 raymes@ asked me to create a ChooserPermissionStore that > > could be shared between USB and Bluetooth and use a (mostly hidden) separate > > content setting so that the feature could be turned off by policy. > > > > Why that other content settings need to be linked to a PermissionContextBase? > > I assumed that was just how we did things these days. I can read it directly out > of content settings too. It would save a lot of boilerplate code setting up the > PermissionContextBase. Done.
On 2015/10/13 15:45:38, Reilly Grant wrote: > On 2015/10/13 15:42:51, Mounir Lamouri wrote: > > On 2015/10/13 at 15:31:50, reillyg wrote: > > > On 2015/10/13 10:34:03, Mounir Lamouri wrote: > > > > I still can't understand why you need CONTENT_SETTINGS_TYPE_USB. The > > > > requirements for CONTENT_SETTINGS_TYPE_USB_CHOOSER_DATA are clear given > that > > you > > > > need these for the device list. But why having a more general permission? > I > > > > couldn't find anything in the mocks. > > > > > > > > If you have a design doc explaining the chooser_permission_context_base > > design, > > > > could you send me the link? > > > > > > Back in comment #5 raymes@ asked me to create a ChooserPermissionStore that > > could be shared between USB and Bluetooth and use a (mostly hidden) separate > > content setting so that the feature could be turned off by policy. > > > > Why that other content settings need to be linked to a PermissionContextBase? > > I assumed that was just how we did things these days. I can read it directly out > of content settings too. It would save a lot of boilerplate code setting up the > PermissionContextBase. Thanks for doing this, I was having the same though yesterday :)
I finally found time to do a detailed review. I'm really sorry for the delay. Overall I think it's good :) https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/chooser_permission_context.h (right): https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context.h:21: class ChooserPermissionContext { Can we avoid adding this entire class for the time being and just access the the UsbChooserPermissionContext directly? It looks like it's not used in this patch anyway atm. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/chooser_permission_context_base.cc (right): https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context_base.cc:36: content_setting != CONTENT_SETTING_SESSION_ONLY) nit: SESSION_ONLY isn't a possible value for this setting so you can omit it here https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context_base.cc:42: if (!setting->Remove(kObjectListKey, &objects)) Hmm maybe I'm misreading but it looks like this dictionary always has just 1 key which is a base::ListValue. Why not just return a base::ListValue directly from GetWebsiteSetting/SetWebsiteSetting? https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context_base.cc:53: if ((*it)->GetType() != base::Value::TYPE_DICTIONARY) Should there be a DCHECK here? https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context_base.cc:85: const base::DictionaryValue& object) { optional: Maybe just passing a pointer here for consistency? https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/chooser_permission_context_base.h (right): https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context_base.h:17: class ListValue; nit: unused https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context_base.h:25: class ChooserPermissionContextBase : public KeyedService { Since this is quite different from a PermissionContext, how about we just call it ChooserContextBase (and friends)? This will make it more clear that the two things are different and reduces the risk of them getting tangled up. This also aligns with the fact that the security team talk about choosers and permissions as two different things. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context_base.h:26: public: I think these should mostly be protected https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context_base.h:27: using ObjectList = ScopedVector<base::DictionaryValue>; nit: I'm not sure if this adds a lot (someone reading the api will have to look at the typedef). I'd suggest just using ScopedVector<base::DictionaryValue> in the header file (api) and use the typedef in the implementation if it makes implementation cleaner. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context_base.h:44: virtual std::string GetStringToDisplayForObject( Is this for UI? - I think it would be better to separate this from the model. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... File chrome/browser/usb/usb_chooser_permission_context.cc (right): https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_permission_context.cc:8: #include "chrome/browser/usb/usb_chooser_permission_context.h" nit: this should go first with a space separating it from the other includes https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_permission_context.cc:40: utf8_serial_number == serial_number) nit: {} for multiline if https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_permission_context.cc:117: return FindForDevice(device_list, device); optional: != nullptr? https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_permission_context.cc:123: object.HasKey(kProductIdKey) && object.HasKey(kSerialNumberKey); I think we should probably also check that the number of keys == 4? https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... File chrome/browser/usb/usb_chooser_permission_context.h (right): https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_permission_context.h:16: class UsbChooserPermissionContext : public ChooserPermissionContextBase, I'm not sure inheritance is actually the right choice here, composition would be better. It's not particularly useful for this thing to "be a" ChooserContextBase (because all of them will have different APIs) instead it just "uses" ChooserContextBase. In other words, this thing should contain a ChooserContextBase (which I guess would have a different name). You could pass IsValidObject into ChooserContextBase as a base::Callback or else have a delegate interface which you pass in. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_permission_context.h:24: const std::string& guid); Could you please document what the GUID is? https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... File chrome/browser/usb/usb_chooser_permission_context_factory.h (right): https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_permission_context_factory.h:15: class UsbChooserPermissionContextFactory : public PermissionContextFactoryBase { It doesn't seem great to get the two things tangled here by inheriting from PermissionContextFactoryBase. Could we instead just inherit from BrowserContextKeyedServiceFactory or if really necessary, create a new superclass. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... File chrome/browser/usb/usb_chooser_permission_context_unittest.cc (right): https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_permission_context_unittest.cc:68: scoped_refptr<UsbDevice> other_device = nit: this is unused Though possible add a case where there are multiple devices? https://codereview.chromium.org/1382783002/diff/100001/content/public/browser... File content/public/browser/permission_type.h (right): https://codereview.chromium.org/1382783002/diff/100001/content/public/browser... content/public/browser/permission_type.h:25: USB = 10, I'm not sure that we should add this for now. The chooser is a different type of thing to the other permissions and it's going to be used in different ways. I think we might be able to avoid adding this altogether for the moment.
https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/chooser_permission_context.h (right): https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context.h:21: class ChooserPermissionContext { On 2015/10/14 02:07:04, raymes wrote: > Can we avoid adding this entire class for the time being and just access the the > UsbChooserPermissionContext directly? It looks like it's not used in this patch > anyway atm. It's used in my next patch and it makes more sense to me to introduce it here with the rest of the ChooserPermissionContext infrastructure than later in a primarily UI-focused patch. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/chooser_permission_context_base.cc (right): https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context_base.cc:36: content_setting != CONTENT_SETTING_SESSION_ONLY) On 2015/10/14 02:07:04, raymes wrote: > nit: SESSION_ONLY isn't a possible value for this setting so you can omit it > here Done. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context_base.cc:42: if (!setting->Remove(kObjectListKey, &objects)) On 2015/10/14 02:07:04, raymes wrote: > Hmm maybe I'm misreading but it looks like this dictionary always has just 1 key > which is a base::ListValue. Why not just return a base::ListValue directly from > GetWebsiteSetting/SetWebsiteSetting? I need to get the dictionary so that I can replace the list within it and write the whole thing back to website settings. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context_base.cc:53: if ((*it)->GetType() != base::Value::TYPE_DICTIONARY) On 2015/10/14 02:07:04, raymes wrote: > Should there be a DCHECK here? I would rather not DCHECK on invalid data in user preferences. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/chooser_permission_context_base.h (right): https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context_base.h:17: class ListValue; On 2015/10/14 02:07:04, raymes wrote: > nit: unused Done. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context_base.h:25: class ChooserPermissionContextBase : public KeyedService { On 2015/10/14 02:07:04, raymes wrote: > Since this is quite different from a PermissionContext, how about we just call > it ChooserContextBase (and friends)? This will make it more clear that the two > things are different and reduces the risk of them getting tangled up. This also > aligns with the fact that the security team talk about choosers and permissions > as two different things. Done. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context_base.h:26: public: On 2015/10/14 02:07:04, raymes wrote: > I think these should mostly be protected UI code will be calling these. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context_base.h:27: using ObjectList = ScopedVector<base::DictionaryValue>; On 2015/10/14 02:07:04, raymes wrote: > nit: I'm not sure if this adds a lot (someone reading the api will have to look > at the typedef). I'd suggest just using ScopedVector<base::DictionaryValue> in > the header file (api) and use the typedef in the implementation if it makes > implementation cleaner. Done. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context_base.h:44: virtual std::string GetStringToDisplayForObject( On 2015/10/14 02:07:04, raymes wrote: > Is this for UI? - I think it would be better to separate this from the model. I would rather keep all the knowledge about what a "USB chooser object" is in one place than spread it out into the UI code as well. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... File chrome/browser/usb/usb_chooser_permission_context.cc (right): https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_permission_context.cc:8: #include "chrome/browser/usb/usb_chooser_permission_context.h" On 2015/10/14 02:07:04, raymes wrote: > nit: this should go first with a space separating it from the other includes Done. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_permission_context.cc:40: utf8_serial_number == serial_number) On 2015/10/14 02:07:04, raymes wrote: > nit: {} for multiline if Done. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_permission_context.cc:117: return FindForDevice(device_list, device); On 2015/10/14 02:07:04, raymes wrote: > optional: != nullptr? Done. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_permission_context.cc:123: object.HasKey(kProductIdKey) && object.HasKey(kSerialNumberKey); On 2015/10/14 02:07:04, raymes wrote: > I think we should probably also check that the number of keys == 4? Done. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... File chrome/browser/usb/usb_chooser_permission_context.h (right): https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_permission_context.h:16: class UsbChooserPermissionContext : public ChooserPermissionContextBase, On 2015/10/14 02:07:04, raymes wrote: > I'm not sure inheritance is actually the right choice here, composition would be > better. It's not particularly useful for this thing to "be a" ChooserContextBase > (because all of them will have different APIs) instead it just "uses" > ChooserContextBase. In other words, this thing should contain a > ChooserContextBase (which I guess would have a different name). You could pass > IsValidObject into ChooserContextBase as a base::Callback or else have a > delegate interface which you pass in. The idea of ChooserContextBase, like PermissionContextBase, is that the UI code should be able to operate on one without knowing the details of what particular kind of choosen it objects it stores. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_permission_context.h:24: const std::string& guid); On 2015/10/14 02:07:04, raymes wrote: > Could you please document what the GUID is? Done. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... File chrome/browser/usb/usb_chooser_permission_context_factory.h (right): https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_permission_context_factory.h:15: class UsbChooserPermissionContextFactory : public PermissionContextFactoryBase { On 2015/10/14 02:07:04, raymes wrote: > It doesn't seem great to get the two things tangled here by inheriting from > PermissionContextFactoryBase. Could we instead just inherit from > BrowserContextKeyedServiceFactory or if really necessary, create a new > superclass. Done. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... File chrome/browser/usb/usb_chooser_permission_context_unittest.cc (right): https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_permission_context_unittest.cc:68: scoped_refptr<UsbDevice> other_device = On 2015/10/14 02:07:04, raymes wrote: > nit: this is unused > Though possible add a case where there are multiple devices? Fixed. https://codereview.chromium.org/1382783002/diff/100001/content/public/browser... File content/public/browser/permission_type.h (right): https://codereview.chromium.org/1382783002/diff/100001/content/public/browser... content/public/browser/permission_type.h:25: USB = 10, On 2015/10/14 02:07:04, raymes wrote: > I'm not sure that we should add this for now. The chooser is a different type of > thing to the other permissions and it's going to be used in different ways. I > think we might be able to avoid adding this altogether for the moment. Done.
Thanks! https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/chooser_permission_context.h (right): https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context.h:21: class ChooserPermissionContext { I don't feel entirely convinced that this is needed though - but it's hard to see becaues nothing in this CL uses it :) Since this CL is large, could we carve this off (along with the enum) and add it in a separate CL? https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/chooser_permission_context_base.h (right): https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context_base.h:44: virtual std::string GetStringToDisplayForObject( I disagree - I left an extended comment in usb_chooser_context_base.cc https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... File chrome/browser/usb/usb_chooser_permission_context.h (right): https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_permission_context.h:16: class UsbChooserPermissionContext : public ChooserPermissionContextBase, I don't think it's much like PermissionContextBase. The analogy would be that we expose GrantObjectPermission/RevokeObjectPermission/etc in ChooserContext and each subclass may provide specific implementations. But we have something different here. Instead the subclass is providing a specific API GrantDevicePermission/RevokeDevicePermission/etc and those specific APIs use the superclass functions. Why not hoist out the logic for the specific API and make it a wrapper around ChooserContext? Inheritance doesn't buy much here. Also notice that PermissionContext isn't meant to contain any UI logic (unfortunately it does contain some to show a permission bubble but the desire is to separate this out). But the logic for displaying different strings for permissions and so on isn't in PermissionContext, but in other UI-specific classes (see e.g. PermissionBubbleRequestImpl). We wouldn't want to add a new virtual method to PermissionContext which was GetDisplayString. I would just leave GetStringToDisplayForObject out of this CL for now so we can land this more quickly and discuss that separately. https://codereview.chromium.org/1382783002/diff/120001/chrome/browser/profile... File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (left): https://codereview.chromium.org/1382783002/diff/120001/chrome/browser/profile... chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:212: NotificationPermissionContextFactory::GetInstance(); How come this stuff has changed?
https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... File chrome/browser/usb/usb_chooser_permission_context.h (right): https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_permission_context.h:16: class UsbChooserPermissionContext : public ChooserPermissionContextBase, On 2015/10/29 06:14:41, raymes wrote: > I don't think it's much like PermissionContextBase. The analogy would be that we > expose GrantObjectPermission/RevokeObjectPermission/etc in ChooserContext and > each subclass may provide specific implementations. But we have something > different here. Instead the subclass is providing a specific API > GrantDevicePermission/RevokeDevicePermission/etc and those specific APIs use the > superclass functions. > > Why not hoist out the logic for the specific API and make it a wrapper around > ChooserContext? Inheritance doesn't buy much here. With that said, if there were other (non-UI specifc) virtual methods that were going to be added, I could see this design being reasonable, and I don't want to hold this up too long :)
https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/chooser_permission_context.h (right): https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context.h:21: class ChooserPermissionContext { On 2015/10/29 06:14:40, raymes wrote: > I don't feel entirely convinced that this is needed though - but it's hard to > see becaues nothing in this CL uses it :) Since this CL is large, could we carve > this off (along with the enum) and add it in a separate CL? Done. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/chooser_permission_context_base.h (right): https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/chooser_permission_context_base.h:44: virtual std::string GetStringToDisplayForObject( On 2015/10/29 06:14:40, raymes wrote: > I disagree - I left an extended comment in usb_chooser_context_base.cc Done. https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... File chrome/browser/usb/usb_chooser_permission_context.h (right): https://codereview.chromium.org/1382783002/diff/100001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_permission_context.h:16: class UsbChooserPermissionContext : public ChooserPermissionContextBase, On 2015/10/29 06:31:26, raymes wrote: > On 2015/10/29 06:14:41, raymes wrote: > > I don't think it's much like PermissionContextBase. The analogy would be that > we > > expose GrantObjectPermission/RevokeObjectPermission/etc in ChooserContext and > > each subclass may provide specific implementations. But we have something > > different here. Instead the subclass is providing a specific API > > GrantDevicePermission/RevokeDevicePermission/etc and those specific APIs use > the > > superclass functions. > > > > Why not hoist out the logic for the specific API and make it a wrapper around > > ChooserContext? Inheritance doesn't buy much here. > > With that said, if there were other (non-UI specifc) virtual methods that were > going to be added, I could see this design being reasonable, and I don't want to > hold this up too long :) In working on the UI I've added more (non-UI) virtual methods. https://codereview.chromium.org/1382783002/diff/120001/chrome/browser/profile... File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (left): https://codereview.chromium.org/1382783002/diff/120001/chrome/browser/profile... chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:212: NotificationPermissionContextFactory::GetInstance(); On 2015/10/29 06:14:41, raymes wrote: > How come this stuff has changed? Because I like making unrelated changes in a single CL. :P
CC palmer@ because he cares about choosers.
lgtm with the following comment. If you would rather do further iterations in the current patch then let me know. Thanks! https://codereview.chromium.org/1382783002/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/chooser_context_base.h (right): https://codereview.chromium.org/1382783002/diff/140001/chrome/browser/permiss... chrome/browser/permissions/chooser_context_base.h:39: virtual ScopedVector<base::DictionaryValue> GetPreviouslyChosenObjects( This and the function below weren't virtual in the previous patchset :) I have some thoughts on this but I think it will be better for you to land this without further changes. Are you ok to make these non-virtual again and make those changes in a separate CL? If so then lgtm :)
reillyg@chromium.org changed reviewers: + mmenke@chromium.org - pfeldman@chromium.org
mlamouri@ and mmenke@ please take a look. https://codereview.chromium.org/1382783002/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/chooser_context_base.h (right): https://codereview.chromium.org/1382783002/diff/140001/chrome/browser/permiss... chrome/browser/permissions/chooser_context_base.h:39: virtual ScopedVector<base::DictionaryValue> GetPreviouslyChosenObjects( On 2015/11/02 03:00:13, raymes wrote: > This and the function below weren't virtual in the previous patchset :) I have > some thoughts on this but I think it will be better for you to land this without > further changes. Are you ok to make these non-virtual again and make those > changes in a separate CL? If so then lgtm :) Done.
First of all, I think the CL description is slightly out of date. You might want to update it. Regarding the content, I'm a bit worried that chooser_context_base.h might be added with no more than one usage. Do you know of any one who is going to use that after it has landed? My concern here is that I am unable to judge if the base is a correct base and matches needs and requirements of other choosers. Would that make sense to keep a usb chooser class and have a separate CL implementing a base class for at two classes (including USB).
On 2015/11/03 11:38:30, Mounir Lamouri wrote: > First of all, I think the CL description is slightly out of date. You might want > to update it. I will update it. > Regarding the content, I'm a bit worried that chooser_context_base.h might be > added with no more than one usage. Do you know of any one who is going to use > that after it has landed? My concern here is that I am unable to judge if the > base is a correct base and matches needs and requirements of other choosers. > Would that make sense to keep a usb chooser class and have a separate CL > implementing a base class for at two classes (including USB). All of the work I am doing on chooser permissions will be reused by WebBluetooth. If you look back at the original version of this patch raymes@ told me to add this generic layer. Please stop blocking my progress on this feature by asking me to rewrite this patch for a 4th time. I have been trying to get this landed for a month.
Description was changed from ========== Store USB device permissions in website settings. This change adds a new type of PermissionContextBase for chooser-based permissions that stores a list of chosen objects in website settings. UsbPermissionContext is then based on this and adds the concept of an ephemeral device permission (for devices that can't be recorded in persistent storage because they are not uniquely identifiable enough). WebUSBPermissionProvider is updated to check with the UsbPermissionContext and once there is a chooser UI to populate the list of allowed devices the --enable-webusb-on-any-origin flag can be removed. BUG=529950 ========== to ========== Store USB device permissions in website settings. This change adds a new class, ChooserContextBase, that stores a list of chosen objects for chooser-based permissions in website settings. It is similar to PermissionContextBase. UsbPermissionContext extends this and adds the concept of an ephemeral device permission (for devices that can't be recorded in persistent storage because they are not uniquely identifiable enough). WebUSB is the initial consumer of this new API. WebUSBPermissionProvider has been updated to check with the UsbPermissionContext and once there is a chooser UI to populate the list of allowed devices the --enable-webusb-on-any-origin flag can be removed. BUG=529950 ==========
I am giving lgtm here to unblock you but I would like to see: - a design doc regarding the chooser permission model - I think your model doesn't match what some people have in mind. A chooser, for some folks doesn't save any state (like file picker). - concrete plans for Bluetooth to use this model - Bluetooth doesn't seem to use a guard content setting for example. - unit tests for chooser context base. In general, when you send a CL that adds basic concepts (here, chooser in permission code), it's probably a good practice to share a design doc of some sort. I believe I asked for one in my very first review iteration. The best way to get unblocked quickly would have been to write it and share it with me and other stack holders in order to discuss the design outside of the CL -- codereview isn't the best medium for that kind of discussions. Furthermore, my advice if you get opposed feedback from reviewers is to send them an email and ask them to find an agreement between them. It could have solved some of your issues in a couple of days instead of taking weeks. This said, again, a design doc would have helped here. https://codereview.chromium.org/1382783002/diff/180001/chrome/browser/permiss... File chrome/browser/permissions/chooser_context_base.h (right): https://codereview.chromium.org/1382783002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/chooser_context_base.h:36: ScopedVector<base::DictionaryValue> GetPreviouslyChosenObjects( nit: GetGrantedObjects() https://codereview.chromium.org/1382783002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/chooser_context_base.h:65: const ContentSettingsType permission_type_; nit: could you rename this |guard_content_setting_type_| https://codereview.chromium.org/1382783002/diff/180001/chrome/browser/permiss... chrome/browser/permissions/chooser_context_base.h:66: const ContentSettingsType chooser_type_; nit: could you rename this |objects_content_setting_type_| or whatever name that will make it clear it's about saving individual object permissions.
The CQ bit was checked by reillyg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, bauerb@chromium.org, raymes@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1382783002/#ps200001 (title: "Address mlamouri@'s comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1382783002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1382783002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
reillyg@chromium.org changed reviewers: + erg@google.com - mmenke@chromium.org
Oops, erg@, can you look at chrome_browser_main_parts_profiles.cc?
erg@chromium.org changed reviewers: + erg@chromium.org
profiles lgtm
The CQ bit was checked by reillyg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1382783002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1382783002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/551eaa4879fb89ca1798b1001e32608380e1405a Cr-Commit-Position: refs/heads/master@{#359001} |
