Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1388343003:
Adds a field trial to control Web API kill switches. (Closed)
|
Created:
5 years, 2 months ago by kcarattini Modified:
5 years, 1 month ago CC:
chrome-apps-syd-reviews_chromium.org, chromium-reviews, mlamouri+watch-permissions_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds a field trial to control Web API kill switches.
Added for permissions that subclass PermissionContextBase.
BUG=539736
Committed: https://crrev.com/2ee48ad5fc593755cdaa2a99b81579092c729476
Cr-Commit-Position: refs/heads/master@{#356183}
Patch Set 1 #Patch Set 2 : Add MediaStream #
Total comments: 6
Patch Set 3 : Review Comments #
Total comments: 6
Patch Set 4 : Add test #
Total comments: 7
Patch Set 5 : Fix comment #
Total comments: 6
Patch Set 6 : Review Comments #Patch Set 7 : Change 'enabled' to 'blocked' #
Total comments: 20
Patch Set 8 : Review Comments #
Total comments: 21
Patch Set 9 : Review Comments #Patch Set 10 : Rebase #
Messages
Total messages: 30 (4 generated)
kcarattini@chromium.org changed reviewers: + mlamouri@chromium.org, nparker@chromium.org, raymes@chromium.org
https://codereview.chromium.org/1388343003/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/1388343003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:23: Put in an anonymous namespace, or make static https://codereview.chromium.org/1388343003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:52: variations::GetVariationParamValue(kApiFieldStudy, Meta: We should document how to use this kill switch. I'm imagining an emergency a year from now and people not knowing that this thing exists or how to use it. https://codereview.chromium.org/1388343003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:52: variations::GetVariationParamValue(kApiFieldStudy, Since you use this twice, I'd make a helper func (in the anon-namespace) like IsApiKillSwitchOn(permission_type) or some such.
https://codereview.chromium.org/1388343003/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/1388343003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:23: On 2015/10/09 00:38:37, nparker wrote: > Put in an anonymous namespace, or make static Done. https://codereview.chromium.org/1388343003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:52: variations::GetVariationParamValue(kApiFieldStudy, On 2015/10/09 00:38:37, nparker wrote: > Meta: We should document how to use this kill switch. I'm imagining an > emergency a year from now and people not knowing that this thing exists or how > to use it. Acknowledged. https://codereview.chromium.org/1388343003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:52: variations::GetVariationParamValue(kApiFieldStudy, On 2015/10/09 00:38:37, nparker wrote: > Since you use this twice, I'd make a helper func (in the anon-namespace) like > IsApiKillSwitchOn(permission_type) or some such. Done.
If I understand correctly, the kill switch you implement here will disable all permissions for all APIs. It sounds a bit too much. Why would we want to do that? I thought that we wanted a kill switch per-API. Do you have a design doc explaining the idea? https://codereview.chromium.org/1388343003/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_uma_util.h (right): https://codereview.chromium.org/1388343003/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_uma_util.h:30: static const std::string GetPermissionString(ContentSettingsType permission); I don't think this needs to be part of the class. Are you using any member in the implementation?
On 2015/10/09 12:07:54, Mounir Lamouri wrote: > If I understand correctly, the kill switch you implement here will disable all > permissions for all APIs. It sounds a bit too much. Why would we want to do > that? I thought that we wanted a kill switch per-API. Hmm in my reading this does implement a kill switch on a per-API basis? https://codereview.chromium.org/1388343003/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/1388343003/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:26: static const char kApiFieldParamEnabledValue[] = "enabled"; Are these not defined as constants somewhere else? https://codereview.chromium.org/1388343003/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:279: bool PermissionContextBase::IsApiKillSwitchOn() const { Should we add an about:flag to gate this behavior here too?
Yes, this will disable on a per-API basis, and I am currently writing up a doc on how to use the kill switch that I will share with you later today. Kendra https://codereview.chromium.org/1388343003/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/1388343003/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:26: static const char kApiFieldParamEnabledValue[] = "enabled"; On 2015/10/11 22:11:22, raymes wrote: > Are these not defined as constants somewhere else? No. From looking at other examples, these constants appear to be defined locally where the field trial params are requested. The other place they would be defined is in the actual finch trial config. https://codereview.chromium.org/1388343003/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:279: bool PermissionContextBase::IsApiKillSwitchOn() const { On 2015/10/11 22:11:22, raymes wrote: > Should we add an about:flag to gate this behavior here too? There is a way to force the field trial already with a commandline flag. If you were referring to using a different flag, wouldn't that defeat the purpose of being able to role out the kill switch quickly with a finch trial? I might have misunderstood what you meant. https://codereview.chromium.org/1388343003/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_uma_util.h (right): https://codereview.chromium.org/1388343003/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_uma_util.h:30: static const std::string GetPermissionString(ContentSettingsType permission); On 2015/10/09 12:07:54, Mounir Lamouri wrote: > I don't think this needs to be part of the class. Are you using any member in > the implementation? No, that's why it is static. None of the other methods declared here use member variables (in fact there aren't any member variables), so I viewed this class as a collection of related methods to do with PermissionContext. With that definition, do you disagree that it should be part of the class?
I've added a test, please have another look. Kendra
The implementation looks generally fine but I would like to make sure we actually want to go that way before having this code land. There are other approaches that could be taken like blocking the API directly on the Blink side. It would be great to have a design doc that explains the goal of the project, describes the different solutions and pros/cons for each. https://codereview.chromium.org/1388343003/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/1388343003/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:285: PermissionContextUmaUtil::GetPermissionString(permission_type_)); I guess I missed the PermissionContextUmaUtil::GetPermissionString() here. It explains why you are adding this static method and how this is per-permission. I would prefer to not have PermissionContextBase getting the string from PermissionContextUmaUtil. Maybe you could move that to a new file called permission_utils.{h,cc}? https://codereview.chromium.org/1388343003/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/1388343003/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:62: // The field trial responsible for rolling out Plugin Power Saver to users. is the comment correct? https://codereview.chromium.org/1388343003/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:63: static const char kApiKillSwitchFieldParamEnabledValue[]; Could that be in the implementation in the anonymous namespace?
https://codereview.chromium.org/1388343003/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/1388343003/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:62: // The field trial responsible for rolling out Plugin Power Saver to users. On 2015/10/12 10:51:10, Mounir Lamouri wrote: > is the comment correct? Nope. Changed https://codereview.chromium.org/1388343003/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:63: static const char kApiKillSwitchFieldParamEnabledValue[]; On 2015/10/12 10:51:10, Mounir Lamouri wrote: > Could that be in the implementation in the anonymous namespace? Doesn't the namespace need to be named to use it in the test?
lgtm https://codereview.chromium.org/1388343003/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/1388343003/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:24: // API kill switch Finch Trials nit: don't really need this comment now that comments are in the header https://codereview.chromium.org/1388343003/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/1388343003/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:59: // A field trial used to enable thw global API kill switch. nit: thw->the https://codereview.chromium.org/1388343003/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:63: static const char kApiKillSwitchFieldParamEnabledValue[]; nit: you've referred to the kill switch in two different ways in the comments which makes it sound a bit like they are two different things (the global API kill switch vs. an API kill switch). Perhaps just refer to both as "an API kill switch"? (or maybe I'm missing some more complexity here :)
https://codereview.chromium.org/1388343003/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/1388343003/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:285: PermissionContextUmaUtil::GetPermissionString(permission_type_)); On 2015/10/12 10:51:10, Mounir Lamouri wrote: > I guess I missed the PermissionContextUmaUtil::GetPermissionString() here. It > explains why you are adding this static method and how this is per-permission. I > would prefer to not have PermissionContextBase getting the string from > PermissionContextUmaUtil. > > Maybe you could move that to a new file called permission_utils.{h,cc}? I was thinking it would be better design to get rid of these switch statements and add a 'std::string name'_ field to the base class, which would be initialised int eh constructor of each subclass. What do you think? Would you be ok with me leaving this cl as-is and doing this refactoring in a follow-up cl? https://codereview.chromium.org/1388343003/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/1388343003/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:24: // API kill switch Finch Trials On 2015/10/13 01:30:18, raymes wrote: > nit: don't really need this comment now that comments are in the header Done. https://codereview.chromium.org/1388343003/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/1388343003/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:59: // A field trial used to enable thw global API kill switch. On 2015/10/13 01:30:18, raymes wrote: > nit: thw->the Done. https://codereview.chromium.org/1388343003/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:63: static const char kApiKillSwitchFieldParamEnabledValue[]; On 2015/10/13 01:30:18, raymes wrote: > nit: you've referred to the kill switch in two different ways in the comments > which makes it sound a bit like they are two different things (the global API > kill switch vs. an API kill switch). Perhaps just refer to both as "an API kill > switch"? (or maybe I'm missing some more complexity here :) I made them both "global. I want to distinguish this kill switch from the origin-specific one that we;ll do for Safe Browsing.
https://codereview.chromium.org/1388343003/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/1388343003/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:285: PermissionContextUmaUtil::GetPermissionString(permission_type_)); On 2015/10/13 03:06:22, kcarattini wrote: > On 2015/10/12 10:51:10, Mounir Lamouri wrote: > > I guess I missed the PermissionContextUmaUtil::GetPermissionString() here. It > > explains why you are adding this static method and how this is per-permission. > I > > would prefer to not have PermissionContextBase getting the string from > > PermissionContextUmaUtil. > > > > Maybe you could move that to a new file called permission_utils.{h,cc}? > > I was thinking it would be better design to get rid of these switch statements > and add a 'std::string name'_ field to the base class, which would be > initialised int eh constructor of each subclass. What do you think? Would you be > ok with me leaving this cl as-is and doing this refactoring in a follow-up cl? I like this idea :)
On 2015/10/13 03:34:08, raymes wrote: > https://codereview.chromium.org/1388343003/diff/60001/chrome/browser/permissi... > File chrome/browser/permissions/permission_context_base.cc (right): > > https://codereview.chromium.org/1388343003/diff/60001/chrome/browser/permissi... > chrome/browser/permissions/permission_context_base.cc:285: > PermissionContextUmaUtil::GetPermissionString(permission_type_)); > On 2015/10/13 03:06:22, kcarattini wrote: > > On 2015/10/12 10:51:10, Mounir Lamouri wrote: > > > I guess I missed the PermissionContextUmaUtil::GetPermissionString() here. > It > > > explains why you are adding this static method and how this is > per-permission. > > I > > > would prefer to not have PermissionContextBase getting the string from > > > PermissionContextUmaUtil. > > > > > > Maybe you could move that to a new file called permission_utils.{h,cc}? > > > > I was thinking it would be better design to get rid of these switch statements > > and add a 'std::string name'_ field to the base class, which would be > > initialised int eh constructor of each subclass. What do you think? Would you > be > > ok with me leaving this cl as-is and doing this refactoring in a follow-up cl? > > I like this idea :) Changed the param from 'enabled' to 'blocked' at Ben's request. Mounir, I've talked with Raymes about the various options of where to put the kill switch and we think it makes sense to have it here in PermissionContext at this time (happy to discuss on the doc if you have comments). We would like to address the bigger issue of how to structure the overall permissions/chooser/content settings code at some point in the future as well, but not have it block this cl. Please take another look. Thanks, Kendra
mlamouri@chromium.org changed reviewers: + bauerb@chromium.org
+bauerb@ to have a look at the finch trial usage. https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:25: const char PermissionContextBase::kApiKillSwitchFieldStudy[] = "ApiKillSwitch"; I would prefer if the name of the switch was more descriptive. What about "PermissionsAutoDeny" ? https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:28: "blocked"; I think we should just try to match the string "enabled". https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:53: if (IsApiKillSwitchOn()) { In order to make sure this is always called, I think you should make ::RequestPermission() not virtual. It is calling ::DecidePermission() which is already virtual anyway. https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:152: std::string()); I think you can refactor the two blocks above by calling GetPermissionStatus() here. It would also automatically use |ShouldAutoDeny()| when ::DecidePermission() is called. https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:281: bool PermissionContextBase::IsApiKillSwitchOn() const { nit: maybe rename: ShouldAutoDeny()? https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:63: static const char kApiKillSwitchFieldParamBlockedValue[]; Could you have these two const char inside the .cc file instead of having them static on the class? https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:89: virtual bool IsApiKillSwitchOn() const; I don't think this needs to be virtual. https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_uma_util.h (right): https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.h:30: static const std::string GetPermissionString(ContentSettingsType permission); Could you move that to a new permission_util.h file?
What's the rationale behind this change? Note that Finch is not great for actual kill switches, as it will take a browser restart to apply the experiment, and on most platforms Finch flags aren't fetched until the first time the browser runs, so on the very first time the experiment might not be applied. https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:53: if (IsApiKillSwitchOn()) { On 2015/10/20 13:35:45, Mounir Lamouri wrote: > In order to make sure this is always called, I think you should make > ::RequestPermission() not virtual. It is calling ::DecidePermission() which is > already virtual anyway. I don't think that works, as this method is overridden by GeolocationPermissionContext. (It does mean that you'll need to check the kill switch in that class as well though.) https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:89: virtual bool IsApiKillSwitchOn() const; Add a comment that this method is public for testing? (Or make it protected and friend the test class.)
https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:53: if (IsApiKillSwitchOn()) { On 2015/10/20 at 14:27:17, Bernhard Bauer wrote: > On 2015/10/20 13:35:45, Mounir Lamouri wrote: > > In order to make sure this is always called, I think you should make > > ::RequestPermission() not virtual. It is calling ::DecidePermission() which is > > already virtual anyway. > > I don't think that works, as this method is overridden by GeolocationPermissionContext. > > (It does mean that you'll need to check the kill switch in that class as well though.) The logic in GeolocationPermissionContext::RequestPermission could probably move to ::DecidePermission().
Bernhard, I shared the doc explaining the feature with you. We are planning to move to the Finch Kill Switch infrastructure once it is ready, but are starting with the Field Study until then (limitations of this approach acknowledged). Thanks, Kendra https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:25: const char PermissionContextBase::kApiKillSwitchFieldStudy[] = "ApiKillSwitch"; On 2015/10/20 13:35:45, Mounir Lamouri wrote: > I would prefer if the name of the switch was more descriptive. What about > "PermissionsAutoDeny" ? I switched to "Permission" from "Api" since that's more accurate. I think "kill switch" is an accepted term for what we're doing here, though. Especially since we plan to move to the Finch Kill Switch infrastructure which has the term in its name. https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:28: "blocked"; On 2015/10/20 13:35:45, Mounir Lamouri wrote: > I think we should just try to match the string "enabled". I actually moved from "enabled" to "blocked" because Ben found it confusing to have the term "enabled" used when an API is disabled by a kill switch. I think we should go for clarity and stick with "blocked". https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:53: if (IsApiKillSwitchOn()) { On 2015/10/20 14:27:17, Bernhard Bauer wrote: > On 2015/10/20 13:35:45, Mounir Lamouri wrote: > > In order to make sure this is always called, I think you should make > > ::RequestPermission() not virtual. It is calling ::DecidePermission() which is > > already virtual anyway. > > I don't think that works, as this method is overridden by > GeolocationPermissionContext. > > (It does mean that you'll need to check the kill switch in that class as well > though.) GeolocationPermissionContext calls the base class method so it still picks up some of this functionality (albeit after the extensions check). https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:53: if (IsApiKillSwitchOn()) { On 2015/10/20 14:32:28, Mounir Lamouri wrote: > On 2015/10/20 at 14:27:17, Bernhard Bauer wrote: > > On 2015/10/20 13:35:45, Mounir Lamouri wrote: > > > In order to make sure this is always called, I think you should make > > > ::RequestPermission() not virtual. It is calling ::DecidePermission() which > is > > > already virtual anyway. > > > > I don't think that works, as this method is overridden by > GeolocationPermissionContext. > > > > (It does mean that you'll need to check the kill switch in that class as well > though.) > > The logic in GeolocationPermissionContext::RequestPermission could probably move > to ::DecidePermission(). I'm happy to do that in a followup cl, and then I could make RequestPermission not virtual. https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:152: std::string()); On 2015/10/20 13:35:45, Mounir Lamouri wrote: > I think you can refactor the two blocks above by calling GetPermissionStatus() > here. It would also automatically use |ShouldAutoDeny()| when > ::DecidePermission() is called. Happy to do all the refactoring in a followup cl. I think it's more clear to keep the changes separate. https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:281: bool PermissionContextBase::IsApiKillSwitchOn() const { On 2015/10/20 13:35:45, Mounir Lamouri wrote: > nit: maybe rename: ShouldAutoDeny()? Changed from Api to Permission. https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:63: static const char kApiKillSwitchFieldParamBlockedValue[]; On 2015/10/20 13:35:45, Mounir Lamouri wrote: > Could you have these two const char inside the .cc file instead of having them > static on the class? They are here because they are used in the test as well. I also plan to add a kill switch for camera/mic in another file. https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:89: virtual bool IsApiKillSwitchOn() const; On 2015/10/20 14:27:17, Bernhard Bauer wrote: > Add a comment that this method is public for testing? (Or make it protected and > friend the test class.) Done. https://codereview.chromium.org/1388343003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:89: virtual bool IsApiKillSwitchOn() const; On 2015/10/20 13:35:45, Mounir Lamouri wrote: > I don't think this needs to be virtual. Done.
Thanks for the additional context! Could you also link the design doc from the bug or something? https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:54: // First check if this permission has been disabled through Finch. s/ through Finch/ if you plan on controlling this via a different mechanism in the future, otherwise the comment will just rot. (Also, don't use the term "Finch" in code, as it's a -- not particularly secret, but still internal -- code name. The public name is "field trials". https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:60: static const char kPermissionsKillSwitchFieldStudy[]; Explain that this is public for testing? https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:90: // camera and microphone, and for testing. It's a bit weird to put this into PermissionContextBase for permissions that don't actually use it. Would it make more sense to put it into a class used by either? Also, on which instance of PermissionContextBase would camera and microphone call this method anyway? https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:31: static const char* kPermissionsKillSwitchFieldStudy = No need for static, as const variables automatically get internal linkage anyway. (And in any case, anonymous namespaces would be prefered over static.) https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:254: TestPermissionContext permission_context( This should fit on one line. https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_util.cc (right): https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_util.cc:33: return ""; Use an empty std::string() constructor to save a couple of instructions. https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_util.h (right): https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_util.h:15: static const std::string GetPermissionString(ContentSettingsType permission); The const is unnecessary, as the string is returned by value anyway.
lgtm with bauerb@ comments applied. However, I would really appreciate to see a document explaining the next steps with regards to the project. I think there is still a lot to do for the kill switch to be efficient. For one, making sure all permissions really go trough it. https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:60: static const char kPermissionsKillSwitchFieldStudy[]; On 2015/10/21 at 10:29:03, Bernhard Bauer wrote: > Explain that this is public for testing? Maybe this could move to permission_util? https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:90: // camera and microphone, and for testing. On 2015/10/21 at 10:29:03, Bernhard Bauer wrote: > It's a bit weird to put this into PermissionContextBase for permissions that don't actually use it. Would it make more sense to put it into a class used by either? Also, on which instance of PermissionContextBase would camera and microphone call this method anyway? I wouldn't mind having this moved to permission_util. It would only require to take a ContentSettingsType. https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_util.cc (right): https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_util.cc:9: // Permissions.Action.Geolocation etc.. nit: it shouldn't match RAPPOR and Finch but the other way around. Maybe you should remove this comment. https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_util.h (right): https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_util.h:8: #include "base/logging.h" Why do you need logging.h?
https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:90: // camera and microphone, and for testing. On 2015/10/21 10:29:03, Bernhard Bauer wrote: > It's a bit weird to put this into PermissionContextBase for permissions that > don't actually use it. Would it make more sense to put it into a class used by > either? Also, on which instance of PermissionContextBase would camera and > microphone call this method anyway? Camera and Mic do use PermissionContextBase, they just don't use the RequestPermission method at the moment. Right now they have a separate implementation but there is every intention of refactoring it to use more of PermissionContextBase like the other permissions. In the meantime, there is MediaStreamDevicePermissionContext used by MediaPermission that this method could be called on.
Documentation added to the bug. Kendra https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:54: // First check if this permission has been disabled through Finch. On 2015/10/21 10:29:03, Bernhard Bauer wrote: > s/ through Finch/ if you plan on controlling this via a different mechanism in > the future, otherwise the comment will just rot. > > (Also, don't use the term "Finch" in code, as it's a -- not particularly secret, > but still internal -- code name. The public name is "field trials". Done. https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:60: static const char kPermissionsKillSwitchFieldStudy[]; On 2015/10/21 11:26:06, Mounir Lamouri wrote: > On 2015/10/21 at 10:29:03, Bernhard Bauer wrote: > > Explain that this is public for testing? > > Maybe this could move to permission_util? Added comment about testing. See my comment about moving to permission util below. https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:90: // camera and microphone, and for testing. On 2015/10/21 11:26:06, Mounir Lamouri wrote: > On 2015/10/21 at 10:29:03, Bernhard Bauer wrote: > > It's a bit weird to put this into PermissionContextBase for permissions that > don't actually use it. Would it make more sense to put it into a class used by > either? Also, on which instance of PermissionContextBase would camera and > microphone call this method anyway? > > I wouldn't mind having this moved to permission_util. It would only require to > take a ContentSettingsType. I can see the reasoning for putting this in PermissionUtil. If we end up wanting the kill switch for permissions that don't use PermissionContext then it makes more sense to put it there. However, the discussions about the future of permissions code are still ongoing and it's unclear to me how it will all turn out. Since at the moment, the only permissions using the kill switch are in fact PermissionContext permissions, it makes sense to me to keep it in the PermissionContextBase class. If we do decide to expand the idea of permission beyond PermissionContext then there will definitely be some code restructuring going on and I can move it out then. How does that sound? https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:31: static const char* kPermissionsKillSwitchFieldStudy = On 2015/10/21 10:29:04, Bernhard Bauer wrote: > No need for static, as const variables automatically get internal linkage > anyway. (And in any case, anonymous namespaces would be prefered over static.) Done. https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:254: TestPermissionContext permission_context( On 2015/10/21 10:29:03, Bernhard Bauer wrote: > This should fit on one line. Done. https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_util.cc (right): https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_util.cc:9: // Permissions.Action.Geolocation etc.. On 2015/10/21 11:26:06, Mounir Lamouri wrote: > nit: it shouldn't match RAPPOR and Finch but the other way around. Maybe you > should remove this comment. The comment was meant for people editing the code to be aware what metrics are dependent on a certain string being returned. It probably makes sense to define these somewhere else. I was thinking event that each PermissionContext subclass could have its own "name" field or something, https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_util.cc:33: return ""; On 2015/10/21 10:29:04, Bernhard Bauer wrote: > Use an empty std::string() constructor to save a couple of instructions. Done. https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_util.h (right): https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_util.h:8: #include "base/logging.h" On 2015/10/21 11:26:06, Mounir Lamouri wrote: > Why do you need logging.h? Moved to the .cc file, needed for NOTREACHED() https://codereview.chromium.org/1388343003/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_util.h:15: static const std::string GetPermissionString(ContentSettingsType permission); On 2015/10/21 10:29:04, Bernhard Bauer wrote: > The const is unnecessary, as the string is returned by value anyway. Done.
Code-wise LGTM, but I do share Mounir's concerns. It would be good to have a longer-term plan for this.
The CQ bit was checked by kcarattini@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1388343003/#ps180001 (title: "Rebase")
Thanks, Bernhard and Mounir. I will update the doc with longer term planning and we can discuss there. Thanks, Kendra
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1388343003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1388343003/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/2ee48ad5fc593755cdaa2a99b81579092c729476 Cr-Commit-Position: refs/heads/master@{#356183} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
