Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
396033002:
Support "always allow" for runtime script execution (Closed)
DescriptionSupport "always allow" for runtime script execution
BUG=391922
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290039
Patch Set 1 #
Total comments: 12
Patch Set 2 : Moved logic to active script controller #Patch Set 3 : Test attempt #
Total comments: 2
Patch Set 4 : Added host permissions to explicit hosts #Patch Set 5 : Discriminate between explicit and scriptable hosts, other minor changes #
Total comments: 29
Patch Set 6 : Minor logic changes, added UMA #Patch Set 7 : Replaced persisted permissions with granted permissions #
Total comments: 21
Patch Set 8 : Minor changes #Patch Set 9 : Set up for pattern set union changes #Patch Set 10 : Added "MatchesSingleOrigin" #
Total comments: 2
Patch Set 11 : Histogram updates #
Total comments: 23
Patch Set 12 : Refactoring, minor changes #
Total comments: 35
Patch Set 13 : Cleanup extravaganza #
Total comments: 6
Patch Set 14 : Removed UMA, minor changes #
Total comments: 2
Patch Set 15 : Moar tests #Patch Set 16 : Formatting #
Total comments: 30
Patch Set 17 : Fixed major issues #
Total comments: 8
Patch Set 18 : Origin function changes #Patch Set 19 : ReloadExtension in unittest #
Total comments: 28
Patch Set 20 : Minor changes #Patch Set 21 : String param reference #
Total comments: 16
Patch Set 22 : Test changes #
Total comments: 3
Patch Set 23 : S'more changes #
Total comments: 12
Patch Set 24 : Final fixes #Messages
Total messages: 81 (0 generated)
Kalman, Here's my attempt at that script injection feature request. I've tested it, and the permissions do indeed persist between sessions, and seem to function properly. This code isn't quite cleaned up and pretty yet; I wanted to get some feedback about some design questions I had first. Looking forward to hearing from you! https://codereview.chromium.org/396033002/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/396033002/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_context_menu_model.cc:127: script_controller->OnActiveTabPermissionGranted(extension); Grant active tab permission so that the page action disappears as if it were left clicked. https://codereview.chromium.org/396033002/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_context_menu_model.cc:146: updater.AddPermissions(extension, new_permissions.get()); Add current permissions for this session, since persisted permissions get loaded at startup. https://codereview.chromium.org/396033002/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_context_menu_model.cc:151: prefs->AddPersistedPermission(extension, &pattern); Currently, I have this clearing persisted permissions before adding so that it doesn't get cluttered during testing. https://codereview.chromium.org/396033002/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_context_menu_model.cc:153: } I'm not sure how much of this code belongs in this switch statement. I figured I could get some feedback on how to refactor this so that this file needs fewer includes, and this case can simply make a couple of function calls to do what it needs to do. https://codereview.chromium.org/396033002/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_context_menu_model.cc:224: AddItemWithStringId(ALWAYS_ALLOW, IDS_EXTENSIONS_ALWAYS_ALLOW); Any suggestions on a clean way to resolve this TODO? If the extension has a page action and has content scripts permission, this item will show up by default. But if the extension already has permission to inject on the current URL, this shouldn't show up. https://codereview.chromium.org/396033002/diff/1/chrome/browser/extensions/pe... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/396033002/diff/1/chrome/browser/extensions/pe... chrome/browser/extensions/permissions_updater.cc:186: } Here we retrieve any persisted permissions and add them to granted_scriptable_hosts, so that any requests for permission will know about them. https://codereview.chromium.org/396033002/diff/1/chrome/browser/extensions/pe... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/396033002/diff/1/chrome/browser/extensions/pe... chrome/browser/extensions/permissions_updater.h:13: Will remove (see below) https://codereview.chromium.org/396033002/diff/1/chrome/browser/extensions/pe... chrome/browser/extensions/permissions_updater.h:64: Oops-- I originally added these here, but decided they belonged in prefs and forgot to remove the prototypes. Ignore these for now-- they'll be gone in the next patch set. https://codereview.chromium.org/396033002/diff/1/extensions/browser/extension... File extensions/browser/extension_prefs.cc (right): https://codereview.chromium.org/396033002/diff/1/extensions/browser/extension... extensions/browser/extension_prefs.cc:1156: } If persisted permissions do not exist, we initialize the key to a new ListValue to hold them. Then we append the pattern string to this list. I wasn't sure if it would be better practice to pass a URLPattern into this method, or a URL, or even the pattern string itself. What do you think? https://codereview.chromium.org/396033002/diff/1/extensions/browser/extension... extensions/browser/extension_prefs.cc:1161: void ExtensionPrefs::ClearPersistedPermissions(const Extension* extension) { This method will allow us to place a "Clear persisted permissions" button in the extension settings list in order to remove any persisted permissions.
+devlin, let's see who gets to this first. I'm almost done with my backlog of reviews.
This is a pretty good start. Only big comment is moving the logic to ActiveScriptController and adding tests. Ping again once those are done and it's cleaned up a bit, and we'll go from there. :) Good job! https://codereview.chromium.org/396033002/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/396033002/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_context_menu_model.cc:126: // Allow current tab to run injection. Let's move this logic into ActiveScriptController - that's where most of the rest is, and it avoids the slight hack of saying OnActiveTabPermissionGranted(). https://codereview.chromium.org/396033002/diff/1/extensions/browser/extension... File extensions/browser/extension_prefs.cc (right): https://codereview.chromium.org/396033002/diff/1/extensions/browser/extension... extensions/browser/extension_prefs.cc:1156: } On 2014/07/15 21:56:38, gpdavis wrote: > If persisted permissions do not exist, we initialize the key to a new ListValue > to hold them. Then we append the pattern string to this list. > > I wasn't sure if it would be better practice to pass a URLPattern into this > method, or a URL, or even the pattern string itself. What do you think? Probably an URLPattern. Url is too specific; Prefs shouldn't have to know how to parse it.
I went ahead and made an attempt at writing a unit test, but I'm running into some problems. Everything seems to work when I test it manually, but something seems to be askew with the test environment, and these checks are failing. Can you give me a hand? https://codereview.chromium.org/396033002/diff/50010/chrome/browser/extension... File chrome/browser/extensions/active_script_controller_unittest.cc (right): https://codereview.chromium.org/396033002/diff/50010/chrome/browser/extension... chrome/browser/extensions/active_script_controller_unittest.cc:363: EXPECT_FALSE(RequiresUserConsent(extension)); This check fails. I think this has to do with active permissions in the test environment; adding a persisted permission grants an active permission using the same pattern that's passed in as a persisted permission, ensuring that there will be an active permission until the persisted permissions are loaded in the next session. It seems like in the test environment, these active permissions do not exist, so we need user consent to inject on google.com again. https://codereview.chromium.org/396033002/diff/50010/chrome/browser/extension... chrome/browser/extensions/active_script_controller_unittest.cc:369: EXPECT_FALSE(RequiresUserConsent(extension)); This check also fails, but I'm not sure why. The relevant code that pulls the persisted permissions is triggered, and the pattern is added, but for some reason, the page still requires user consent.
On 2014/07/21 20:25:41, gpdavis wrote: > I went ahead and made an attempt at writing a unit test, but I'm running into > some problems. Everything seems to work when I test it manually, but something > seems to be askew with the test environment, and these checks are failing. Can > you give me a hand? > > https://codereview.chromium.org/396033002/diff/50010/chrome/browser/extension... > File chrome/browser/extensions/active_script_controller_unittest.cc (right): > > https://codereview.chromium.org/396033002/diff/50010/chrome/browser/extension... > chrome/browser/extensions/active_script_controller_unittest.cc:363: > EXPECT_FALSE(RequiresUserConsent(extension)); > This check fails. I think this has to do with active permissions in the test > environment; adding a persisted permission grants an active permission using the > same pattern that's passed in as a persisted permission, ensuring that there > will be an active permission until the persisted permissions are loaded in the > next session. It seems like in the test environment, these active permissions > do not exist, so we need user consent to inject on http://google.com again. > > https://codereview.chromium.org/396033002/diff/50010/chrome/browser/extension... > chrome/browser/extensions/active_script_controller_unittest.cc:369: > EXPECT_FALSE(RequiresUserConsent(extension)); > This check also fails, but I'm not sure why. The relevant code that pulls the > persisted permissions is triggered, and the pattern is added, but for some > reason, the page still requires user consent. My guess: It's because you're adding the permission to scriptable hosts.
On 2014/07/24 22:06:42, Devlin wrote: > My guess: It's because you're adding the permission to scriptable hosts. Yep, that was it. Thanks! Is there any reason I might not want to be adding these persisting permissions to both explicit and scriptable hosts?
On 2014/07/30 18:38:33, gpdavis wrote: > Is there any reason I might not want to be adding these persisting permissions > to both explicit and scriptable hosts? There are reasons (they represent different permissions), but I think that since the user is opting in to allow the extension to run on domain x, it's okay to add it to both sets (and would perhaps be confusing not to). Ben, sound reasonable?
On 2014/07/30 21:34:32, Devlin wrote: > On 2014/07/30 18:38:33, gpdavis wrote: > > Is there any reason I might not want to be adding these persisting permissions > > to both explicit and scriptable hosts? > > There are reasons (they represent different permissions), but I think that since > the user is opting in to allow the extension to run on domain x, it's okay to > add it to both sets (and would perhaps be confusing not to). > > Ben, sound reasonable? that sounds right, but it all depends on what the extension requested anyway. if an extension requests google.com and yahoo.com scriptable hosts, google.com and chromium.org explicit hosts, then - allowing always on google.com should add to both - allowing on yahoo.com should oly add to scriptable - allowing on chromium.org should only add to explicit maybe that's what you already implemented.
On 2014/07/30 22:01:00, kalman wrote: > On 2014/07/30 21:34:32, Devlin wrote: > > On 2014/07/30 18:38:33, gpdavis wrote: > > > Is there any reason I might not want to be adding these persisting > permissions > > > to both explicit and scriptable hosts? > > > > There are reasons (they represent different permissions), but I think that > since > > the user is opting in to allow the extension to run on domain x, it's okay to > > add it to both sets (and would perhaps be confusing not to). > > > > Ben, sound reasonable? > > that sounds right, but it all depends on what the extension requested anyway. if > an extension requests http://google.com and http://yahoo.com scriptable hosts, http://google.com and > http://chromium.org explicit hosts, then > - allowing always on http://google.com should add to both > - allowing on http://yahoo.com should oly add to scriptable > - allowing on http://chromium.org should only add to explicit > > maybe that's what you already implemented. I added in logic that can make this discrimination and add to explicit and/or scriptable hosts. I also added in logic to determine whether there's an active script injection request action so that we know whether to display "always allow" or not. I was looking into how we might go about adding functionality to allow the user to clear these persisted permissions. The logic is there in ClearPersistedPermissions, but I need to figure out where this should be triggered from. I looked into the permissions popup in chrome://extensions, which lists all the active permissions. Maybe we could add a button to that box for clearing these permissions? The only caveat is that this dialog box already shows a button for revoking retained files permissions if any have been granted. Do you think it'd be reasonable to add a new button for persisted permissions, even if the revoke button is shown? https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
On 2014/07/31 21:27:41, gpdavis wrote: > On 2014/07/30 22:01:00, kalman wrote: > > On 2014/07/30 21:34:32, Devlin wrote: > > > On 2014/07/30 18:38:33, gpdavis wrote: > > > > Is there any reason I might not want to be adding these persisting > > permissions > > > > to both explicit and scriptable hosts? > > > > > > There are reasons (they represent different permissions), but I think that > > since > > > the user is opting in to allow the extension to run on domain x, it's okay > to > > > add it to both sets (and would perhaps be confusing not to). > > > > > > Ben, sound reasonable? > > > > that sounds right, but it all depends on what the extension requested anyway. > if > > an extension requests http://google.com and http://yahoo.com scriptable hosts, > http://google.com and > > http://chromium.org explicit hosts, then > > - allowing always on http://google.com should add to both > > - allowing on http://yahoo.com should oly add to scriptable > > - allowing on http://chromium.org should only add to explicit > > > > maybe that's what you already implemented. > > I added in logic that can make this discrimination and add to explicit and/or > scriptable hosts. > I also added in logic to determine whether there's an active script injection > request action so that we know whether to display "always allow" or not. > > I was looking into how we might go about adding functionality to allow the user > to clear these persisted permissions. The logic is there in > ClearPersistedPermissions, but I need to figure out where this should be > triggered from. I looked into the permissions popup in chrome://extensions, > which lists all the active permissions. Maybe we could add a button to that box > for clearing these permissions? The only caveat is that this dialog box already > shows a button for revoking retained files permissions if any have been granted. > Do you think it'd be reasonable to add a new button for persisted permissions, > even if the revoke button is shown? > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... I wouldn't expect for you to need to write much special code for this. Certainly not in this patch. Those file permissions are entirely different, *but* there is an effort underway to provide a UI for revoking optional permissions. The permissions you're adding in here are quite similar to optional permissions and should basically follow most of the same code paths. When that optional permission revocation stuff is in place (maybe it already is?) we'll probably display the results in the same area, merged into the same list. but again, don't worry about it.
+sashab as an FYI, see previous comment.
On 2014/07/31 21:33:20, kalman wrote: > +sashab as an FYI, see previous comment. Thanks. Yes, we are working on making the App Info dialog available for extensions (crbug.com/395495), and that's probably the place you want to put this revoke action. You might even want to put all the controls in there for granting 'allow' to the specific hosts, but the UI might be tricky because of the number of different combinations. There is already a UI in place for revoking file permissions, and yes, there is a work in progress for revoking optional permissions as well. The code part is not actually the challenge; agreeing on the UI for that is difficult, since the permissions are not individually revokable. You can follow that in crbug.com/383712, but we could get 'revoking persisted permissions' in first if we can agree on a UI (since maybe revoking them all at once makes sense here). Again though, as kalman said, this is definitely work for a later CL :-)
On 2014/08/01 00:50:05, sasha_b wrote: > On 2014/07/31 21:33:20, kalman wrote: > > +sashab as an FYI, see previous comment. > > Thanks. Yes, we are working on making the App Info dialog available for > extensions (crbug.com/395495), and that's probably the place you want to put > this revoke action. You might even want to put all the controls in there for > granting 'allow' to the specific hosts, but the UI might be tricky because of > the number of different combinations. > > There is already a UI in place for revoking file permissions, and yes, there is > a work in progress for revoking optional permissions as well. The code part is > not actually the challenge; agreeing on the UI for that is difficult, since the > permissions are not individually revokable. You can follow that in > crbug.com/383712, but we could get 'revoking persisted permissions' in first if > we can agree on a UI (since maybe revoking them all at once makes sense here). > > Again though, as kalman said, this is definitely work for a later CL :-) Okay, great. Thanks for the insight! I figured any UI related changes would be difficult to agree upon, which is why I brought it up as a possibility before I went ahead and implemented anything in regards to revoking permissions. @kalman, are there any other issues for me to address with this patch?
I was going to let Devlin finish this off but he's OOO tomorrow. I'll have a look, I haven't actually read any code here just lurking on the comments.
https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:111: OnClicked(extension); it might be safer to do this at the end in case some logic turns around and expects the new host permissions are in place? https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:123: PermissionsParser::GetRequiredPermissions(extension)); shouldn't this be querying the withheld permissions? there's no point granting an extension permissions it already has. scoped_refptr<const PermissionSet> withheld_permissions = extension->permissions_data()->withheld_permissions(); (note that withheld is a subset of required at the moment, so your code *works*, but they won't always be.) https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:139: we should add UMA for this. I think a useful metric would be how many pages the user has persisted permissions for (after this method executes of course), which you can approximate using the size of the effective permission set: UMA_HISTOGRAM_COUNTS_100( "Extensions.ActiveScriptController.PersistedPermissionCount", extensions()->permissions_data()->GetEffectiveHostPermissions().size()); https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:140: // Update persisted permissions for extension. PermissionsUpdater should already do this? https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:149: if (!enabled_ || pending_requests_.count(extension->id()) == 0) why this second check? again while it seems like checking it won't be a bug, but the |active_script_actions_| check below is a more idiomatic check. this whole method can be return enabled_ && active_script_actions_.count(extension->id()) > 0; https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller_unittest.cc (right): https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:351: could you also check the effective permission set, and the prefs? this persisting permissions shouldn't just affect the active permission controller, it should affect the global host permissions of the extension. https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:196: AddItemWithStringId(ALWAYS_ALLOW, IDS_EXTENSIONS_ALWAYS_ALLOW); nit: the condition is 2 lines, so the body should be in {}. https://codereview.chromium.org/396033002/diff/110001/extensions/browser/exte... File extensions/browser/extension_prefs.h (right): https://codereview.chromium.org/396033002/diff/110001/extensions/browser/exte... extensions/browser/extension_prefs.h:406: PermissionSet* GetPersistedPermissions(const std::string& extension_id); I'm not sure whether this needs to be a whole separate thing. Can you use |AddGrantedPermissions| instead? that's what the optional permissions stuff uses and it seems sufficiently generic. i also don't see Add or Clear ever called.
https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:111: OnClicked(extension); On 2014/08/01 18:15:12, kalman wrote: > it might be safer to do this at the end in case some logic turns around and > expects the new host permissions are in place? Sure thing. https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:123: PermissionsParser::GetRequiredPermissions(extension)); On 2014/08/01 18:15:11, kalman wrote: > shouldn't this be querying the withheld permissions? there's no point granting > an extension permissions it already has. > > scoped_refptr<const PermissionSet> withheld_permissions = > extension->permissions_data()->withheld_permissions(); > > (note that withheld is a subset of required at the moment, so your code *works*, > but they won't always be.) That seems reasonable. I chose RequiredPermissions because I was under the impression that this corresponded to the manifest permissions that don't change: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... I was essentially trying to find out if the host matches anything in either the "permissions" key, or the "content_scripts" key (which is populated via SetScriptableHosts before Finalize is called). I suppose it does make more sense to just check the withheld permissions considering that's how we got into this section of code in the first place. I'll do that. https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:139: On 2014/08/01 18:15:11, kalman wrote: > we should add UMA for this. I think a useful metric would be how many pages the > user has persisted permissions for (after this method executes of course), which > you can approximate using the size of the effective permission set: > > UMA_HISTOGRAM_COUNTS_100( > "Extensions.ActiveScriptController.PersistedPermissionCount", > extensions()->permissions_data()->GetEffectiveHostPermissions().size()); Okay, sure. Should I add both you and devlin as owners in histograms.xml? https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:140: // Update persisted permissions for extension. On 2014/08/01 18:15:12, kalman wrote: > PermissionsUpdater should already do this? Update Persisted Permissions? The updater adds to active permissions so that the extension has permission to inject scripts right away. Then we add to persisted permissions, which are loaded into active permissions when we InitializePermissions at the start of a new session: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... Without the first update (active), we grant active tab permissions, but the extension requests permission to inject on the same host in a different tab. It's not until the browser is restarted that the extension is always allowed to inject on that host. Without the second update (persisted), the extension needs permission again for that host when the browser is restarted. Hopefully this explanation makes sense without being too verbose, but I'd be more than happy to clarify further. https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:149: if (!enabled_ || pending_requests_.count(extension->id()) == 0) On 2014/08/01 18:15:12, kalman wrote: > why this second check? again while it seems like checking it won't be a bug, but > the |active_script_actions_| check below is a more idiomatic check. > > this whole method can be > > return enabled_ && active_script_actions_.count(extension->id()) > 0; Ah, I see. That makes sense. I guess I was going for a preliminary check with this line, but I think I misinterpreted it, because it does make a lot more sense to just check active_script_actions_. https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:196: AddItemWithStringId(ALWAYS_ALLOW, IDS_EXTENSIONS_ALWAYS_ALLOW); On 2014/08/01 18:15:12, kalman wrote: > nit: the condition is 2 lines, so the body should be in {}. Done. https://codereview.chromium.org/396033002/diff/110001/extensions/browser/exte... File extensions/browser/extension_prefs.h (right): https://codereview.chromium.org/396033002/diff/110001/extensions/browser/exte... extensions/browser/extension_prefs.h:406: PermissionSet* GetPersistedPermissions(const std::string& extension_id); On 2014/08/01 18:15:12, kalman wrote: > I'm not sure whether this needs to be a whole separate thing. Can you use > |AddGrantedPermissions| instead? that's what the optional permissions stuff uses > and it seems sufficiently generic. > > i also don't see Add or Clear ever called. But permissions from the manifest are added directly to granted_permissions. How are we to discriminate between permissions that have been allowed to persist by the user, and permissions that were pulled directly from the manifest? (If this is confusing, I can try and clarify). Clear and Add are called here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... I called both for testing purposes, since there's currently no way to clear them other than to reinstall the extension entirely.
So we don't have too many reviewers at once on this one, kalman@'s primary on this one, but I'd like to have a look before it goes in. :) Cheers!
So we don't have too many reviewers at once on this one, kalman@'s primary on this one, but I'd like to have a look before it goes in. :) Cheers!
https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:139: On 2014/08/01 20:06:06, gpdavis wrote: > On 2014/08/01 18:15:11, kalman wrote: > > we should add UMA for this. I think a useful metric would be how many pages > the > > user has persisted permissions for (after this method executes of course), > which > > you can approximate using the size of the effective permission set: > > > > UMA_HISTOGRAM_COUNTS_100( > > "Extensions.ActiveScriptController.PersistedPermissionCount", > > extensions()->permissions_data()->GetEffectiveHostPermissions().size()); > > Okay, sure. Should I add both you and devlin as owners in histograms.xml? no, histograms.xml has its own owners. git-cl upload should suggest a reviewer for that. https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:140: // Update persisted permissions for extension. On 2014/08/01 20:06:06, gpdavis wrote: > On 2014/08/01 18:15:12, kalman wrote: > > PermissionsUpdater should already do this? > > Update Persisted Permissions? > > The updater adds to active permissions so that the extension has permission to > inject scripts right away. Then we add to persisted permissions, which are > loaded into active permissions when we InitializePermissions at the start of a > new session: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > Without the first update (active), we grant active tab permissions, but the > extension requests permission to inject on the same host in a different tab. > It's not until the browser is restarted that the extension is always allowed to > inject on that host. Without the second update (persisted), the extension needs > permission again for that host when the browser is restarted. > > Hopefully this explanation makes sense without being too verbose, but I'd be > more than happy to clarify further. I mean PermissionsUpdater::AddPermissions: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... I don't quite follow your explanation, sorry. It looks to me like in the code above calling AddPermissions does persist the permissions: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext.... Perhaps answer this: how is this different from granting optional permissions? (it looks like you might have to take into account withheld permissions in GetBoundedActivePermissions: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...) https://codereview.chromium.org/396033002/diff/110001/extensions/browser/exte... File extensions/browser/extension_prefs.h (right): https://codereview.chromium.org/396033002/diff/110001/extensions/browser/exte... extensions/browser/extension_prefs.h:406: PermissionSet* GetPersistedPermissions(const std::string& extension_id); On 2014/08/01 20:06:06, gpdavis wrote: > On 2014/08/01 18:15:12, kalman wrote: > > I'm not sure whether this needs to be a whole separate thing. Can you use > > |AddGrantedPermissions| instead? that's what the optional permissions stuff > uses > > and it seems sufficiently generic. > > > > i also don't see Add or Clear ever called. > > But permissions from the manifest are added directly to granted_permissions. > How are we to discriminate between permissions that have been allowed to persist > by the user, and permissions that were pulled directly from the manifest? (If > this is confusing, I can try and clarify). > > Clear and Add are called here: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > I called both for testing purposes, since there's currently no way to clear them > other than to reinstall the extension entirely. Ah - so there is a concept of "active permissions". this is currently the "required permissions" (pulled directly from the manifest) + "optional permissions" *that have been granted*. I'm imagining this "always allow" functionality to be basically adding to the active permissions, except from a 3rd source - withheld permissions.
https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:139: On 2014/08/04 23:53:13, kalman wrote: > On 2014/08/01 20:06:06, gpdavis wrote: > > On 2014/08/01 18:15:11, kalman wrote: > > > we should add UMA for this. I think a useful metric would be how many pages > > the > > > user has persisted permissions for (after this method executes of course), > > which > > > you can approximate using the size of the effective permission set: > > > > > > UMA_HISTOGRAM_COUNTS_100( > > > "Extensions.ActiveScriptController.PersistedPermissionCount", > > > extensions()->permissions_data()->GetEffectiveHostPermissions().size()); > > > > Okay, sure. Should I add both you and devlin as owners in histograms.xml? > > no, histograms.xml has its own owners. git-cl upload should suggest a reviewer > for that. Oh, no, I meant for the <owner></owner> tags in the file itself (see the histograms.xml diff in the next patch set). https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:140: // Update persisted permissions for extension. On 2014/08/04 23:53:13, kalman wrote: > On 2014/08/01 20:06:06, gpdavis wrote: > > On 2014/08/01 18:15:12, kalman wrote: > > > PermissionsUpdater should already do this? > > > > Update Persisted Permissions? > > > > The updater adds to active permissions so that the extension has permission to > > inject scripts right away. Then we add to persisted permissions, which are > > loaded into active permissions when we InitializePermissions at the start of a > > new session: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > > > Without the first update (active), we grant active tab permissions, but the > > extension requests permission to inject on the same host in a different tab. > > It's not until the browser is restarted that the extension is always allowed > to > > inject on that host. Without the second update (persisted), the extension > needs > > permission again for that host when the browser is restarted. > > > > Hopefully this explanation makes sense without being too verbose, but I'd be > > more than happy to clarify further. > > > I mean PermissionsUpdater::AddPermissions: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > I don't quite follow your explanation, sorry. It looks to me like in the code > above calling AddPermissions does persist the permissions: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext.... > > Perhaps answer this: how is this different from granting optional permissions? > > (it looks like you might have to take into account withheld permissions in > GetBoundedActivePermissions: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...) Ahh, I see what you're saying. AddPermissions already adds to granted_permissions. But the problem is that this isn't consulted when the permissions are initialized: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... So these aren't really persisted, because they're not loaded into active permissions at the start of a new session. But I understand what you're saying now. I'll upload a patch set with a solution that doesn't involve persisted permissions, so refer to my comment in the new patch set and we'll see if it's reasonable. https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller_unittest.cc (right): https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:351: On 2014/08/01 18:15:12, kalman wrote: > could you also check the effective permission set, and the prefs? this > persisting permissions shouldn't just affect the active permission controller, > it should affect the global host permissions of the extension. Let's re-explore this unit test once we're happy with the way things are implemented. https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:136: updater.AddPermissions(extension, new_permissions.get()); So here we AddPermissions, which will immediately make it active for the current session, and add to granted which will allow us to persist the permissions between sessions. https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:189: ActivateGrantedHosts(extension); Once we're done initializing permissions (we use BoundedActivePermissions to set up granted and withheld permissions within this function), we call ActivateGrantedHosts... https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:285: withheld); ... ActivateGrantedHosts will crawl through the granted permissions and move them to active if they dont imply all hosts. Create new explicit and scriptable hosts URLPatternSets that are initialized with the current active hosts. Iterate through granted_permissions and copy any permissions that are not all hosts into the new host permissions. Then we SetPermissions. What do you think about this approach? Is there any harm to adding all granted permissions that aren't for all hosts? This approach passes all the current unittests. If this is acceptable, I can go ahead and clean this patch up and work on expanding the unit tests. https://codereview.chromium.org/396033002/diff/170001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/396033002/diff/170001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6889: + <owner>rdevlin.cronin@chromium.org</owner> This is what I meant when I asked about adding you guys as owner of the histogram.
https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:140: // Update persisted permissions for extension. > Ahh, I see what you're saying. AddPermissions already adds to > granted_permissions. But the problem is that this isn't consulted when the > permissions are initialized: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > So these aren't really persisted, because they're not loaded into active > permissions at the start of a new session. But I understand what you're saying > now. I'll upload a patch set with a solution that doesn't involve persisted > permissions, so refer to my comment in the new patch set and we'll see if it's > reasonable. It looks like GetBoundedActivePermissions reads in the active permissions from prefs. That's this: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... and given this "always run" should be saving to the active permissions in prefs, it should be loaded. am I understanding you correctly? if this isn't working something is going wrong with this required vs withheld permission stuff. check to see what the active permissions end up in prefs. given an extension with permissions: http://*/* optional_permissions: https://*/* Preferences should show permissions: http://*/* optional_permissions: https://*/* active_permissions: [] because they got withheld. then you should be able to go to http://example.com, see an always-run button, click it, then see in Preferenes permissions: http://*/* optional_permissions: https://*/* active_permissions: ["http://example.com/*"] then reload and see it agian. https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:113: pattern.SetHost(url.host()); should probably also set port here. https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:135: Profile::FromBrowserContext(web_contents()->GetBrowserContext())); you don't need to cast this to a Profile*, it takes a BrowserContext. so just web_contents()->GetBrowserContext() https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:138: UMA_HISTOGRAM_COUNTS_100( hmmmmm so I don't think this actually is right, sorry. An extension can request: { "https://*.google.com/*", "http://*/*" } the http://*/* will be withheld, but the https won't, so the GetEffectiveHostPermissions() count will be off by 1. a better estimate would be EffectiveHostPermissions.size - union(ScriptableHostPermissions, RequiredHostPermissions).size https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:139: "Extensions.ActiveScriptController.PersistedPermissionCount", AlwaysRunCount would be better. https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:118: case ALWAYS_ALLOW: { "run" is a better term than "allow" I think. more meaningful to the user, so we might as well use that internally as well. https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:121: extensions::ActiveScriptController::GetForWebContents(web_contents) elsewhere we seem to guard against a null web contents. we probably should here as well. and this pattern is used a couple of times - might want to make a GetActiveWebContents() method on this class. https://codereview.chromium.org/396033002/diff/170001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/396033002/diff/170001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6889: + <owner>rdevlin.cronin@chromium.org</owner> On 2014/08/07 20:50:39, gpdavis wrote: > This is what I meant when I asked about adding you guys as owner of the > histogram. ohhh. yes, do that :)
https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:140: // Update persisted permissions for extension. On 2014/08/07 22:03:45, kalman wrote: > > Ahh, I see what you're saying. AddPermissions already adds to > > granted_permissions. But the problem is that this isn't consulted when the > > permissions are initialized: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > > > So these aren't really persisted, because they're not loaded into active > > permissions at the start of a new session. But I understand what you're > saying > > now. I'll upload a patch set with a solution that doesn't involve persisted > > permissions, so refer to my comment in the new patch set and we'll see if it's > > reasonable. > > It looks like GetBoundedActivePermissions reads in the active permissions from > prefs. That's this: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > and given this "always run" should be saving to the active permissions in prefs, > it should be loaded. > > am I understanding you correctly? > > if this isn't working something is going wrong with this required vs withheld > permission stuff. check to see what the active permissions end up in prefs. > given an extension with > > permissions: http://*/* > optional_permissions: https://*/* > > Preferences should show > > permissions: http://*/* > optional_permissions: https://*/* > active_permissions: [] > > because they got withheld. > > then you should be able to go to http://example.com, see an always-run button, > click it, then see in Preferenes > > permissions: http://*/* > optional_permissions: https://*/* > active_permissions: ["http://example.com/*"] > > then reload and see it agian. I believe this is the line that's causing problems: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... The host does indeed show up in the active permissions in preferences. However, I believe they're getting filtered out because GetBoundedActivePermissions creates this intersection; http://example.com doesn't end up in the Bounded Permissions because it's not a part of the manifest default or optional permissions. "// a) active permissions must be a subset of optional + default permissions" So what I was doing was going through and explicitly taking values from granted permissions and passing them to SetPermissions as active permissions. Does this make more sense? If I hit the always-run button for a host and trace it, it ends up in the active_permissions of Preferences, but after GetBoundedActivePermissions is called in InitializePermissions (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...), it no longer exists, and I believe the above reason is why; it gets shaved off because it's not a part of the extension's required or optional permissions.
https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:140: // Update persisted permissions for extension. > The host does indeed show up in the active permissions in preferences. However, > I believe they're getting filtered out because GetBoundedActivePermissions > creates this intersection; http://example.com doesn't end up in the Bounded > Permissions because it's not a part of the manifest default or optional > permissions. > > "// a) active permissions must be a subset of optional + default permissions" > > So what I was doing was going through and explicitly taking values from granted > permissions and passing them to SetPermissions as active permissions. > > Does this make more sense? > > If I hit the always-run button for a host and trace it, it ends up in the > active_permissions of Preferences, but after GetBoundedActivePermissions is > called in InitializePermissions > (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...), > it no longer exists, and I believe the above reason is why; it gets shaved off > because it's not a part of the extension's required or optional permissions. ah I see. so that condition needs to take into account withheld permissions as well?
https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:140: // Update persisted permissions for extension. On 2014/08/08 00:24:29, kalman wrote: > > > The host does indeed show up in the active permissions in preferences. > However, > > I believe they're getting filtered out because GetBoundedActivePermissions > > creates this intersection; http://example.com doesn't end up in the Bounded > > Permissions because it's not a part of the manifest default or optional > > permissions. > > > > "// a) active permissions must be a subset of optional + default permissions" > > > > So what I was doing was going through and explicitly taking values from > granted > > permissions and passing them to SetPermissions as active permissions. > > > > Does this make more sense? > > > > If I hit the always-run button for a host and trace it, it ends up in the > > active_permissions of Preferences, but after GetBoundedActivePermissions is > > called in InitializePermissions > > > (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...), > > it no longer exists, and I believe the above reason is why; it gets shaved off > > because it's not a part of the extension's required or optional permissions. > > ah I see. so that condition needs to take into account withheld permissions as > well? You mean as a part of the bound, meaning that active permissions are a part of the bounded active permissions if they're a subset of required, optional, and withheld? The problem is that if we have a withheld permission that implies all hosts, we're gonna get a script injection request for a particular site (http://example.com). Upon granting, http://example.com will be added to active permissions. Then this won't work, because the specific host won't be part of the subset, as http://example.com isn't found in the withheld permissions; the permission that implies all hosts is. This is why I made the separate method to add non-all-host permissions from granted to active. https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:113: pattern.SetHost(url.host()); On 2014/08/07 22:03:45, kalman wrote: > should probably also set port here. Done. https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:135: Profile::FromBrowserContext(web_contents()->GetBrowserContext())); On 2014/08/07 22:03:45, kalman wrote: > you don't need to cast this to a Profile*, it takes a BrowserContext. so just > web_contents()->GetBrowserContext() Done. https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:138: UMA_HISTOGRAM_COUNTS_100( On 2014/08/07 22:03:45, kalman wrote: > hmmmmm so I don't think this actually is right, sorry. An extension can request: > > { > "https://*.google.com/*", > "http://*/*" > } > > the http://*/* will be withheld, but the https won't, so the > GetEffectiveHostPermissions() count will be off by 1. > > a better estimate would be EffectiveHostPermissions.size - > union(ScriptableHostPermissions, RequiredHostPermissions).size Which permissions are you talking about here? Withheld scriptable hosts and extension required permissions (as in, PermissionsParser::GetRequiredPermissions()?). https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:139: "Extensions.ActiveScriptController.PersistedPermissionCount", On 2014/08/07 22:03:45, kalman wrote: > AlwaysRunCount would be better. Done. https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:118: case ALWAYS_ALLOW: { On 2014/08/07 22:03:46, kalman wrote: > "run" is a better term than "allow" I think. more meaningful to the user, so we > might as well use that internally as well. Done. https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:121: extensions::ActiveScriptController::GetForWebContents(web_contents) On 2014/08/07 22:03:45, kalman wrote: > elsewhere we seem to guard against a null web contents. we probably should here > as well. > > and this pattern is used a couple of times - might want to make a > GetActiveWebContents() method on this class. Done. Should we be caching the result of GetActiveWebContents, or just making the call twice? WebContents* web_contents = GetActiveWebContents(); if (!web_contents) return; ...(web_contents)... Or... if (!GetActiveWebContents()) return; ...(GetActiveWebContents())...
+rpaquay - need https://codereview.chromium.org/181043006/ for this to work. https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:139: On 2014/08/07 20:50:39, gpdavis wrote: > On 2014/08/04 23:53:13, kalman wrote: > > On 2014/08/01 20:06:06, gpdavis wrote: > > > On 2014/08/01 18:15:11, kalman wrote: > > > > we should add UMA for this. I think a useful metric would be how many > pages > > > the > > > > user has persisted permissions for (after this method executes of course), > > > which > > > > you can approximate using the size of the effective permission set: > > > > > > > > UMA_HISTOGRAM_COUNTS_100( > > > > "Extensions.ActiveScriptController.PersistedPermissionCount", > > > > > extensions()->permissions_data()->GetEffectiveHostPermissions().size()); > > > > > > Okay, sure. Should I add both you and devlin as owners in histograms.xml? > > > > no, histograms.xml has its own owners. git-cl upload should suggest a reviewer > > for that. > > Oh, no, I meant for the <owner></owner> tags in the file itself (see the > histograms.xml diff in the next patch set). I thought I replied to this question. yes, <owner> tag. https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:140: // Update persisted permissions for extension. > You mean as a part of the bound, meaning that active permissions are a part of > the bounded active permissions if they're a subset of required, optional, and > withheld? > > The problem is that if we have a withheld permission that implies all hosts, > we're gonna get a script injection request for a particular site > (http://example.com). Upon granting, http://example.com will be added to active > permissions. Then this won't work, because the specific host won't be part of > the subset, as http://example.com isn't found in the withheld permissions; the > permission that implies all hosts is. > > This is why I made the separate method to add non-all-host permissions from > granted to active. ah I see the confusion here, on both our parts. in most cases host permissions are aware that http://example.com is within <all_urls>. If you were to do (taking {...} as URLPatternSet notation): {http://example.com/*, https://google.com/*} U {http://*/*} you *should* get {http://*/*, https://google.com/*} not {http://example.com/*, https://google.com/*, http://*/*} likewise intersection would be {http://example.com/*} not {} and this *should* be carried through to the permission set level. but it doesn't look like it is: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... --> https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... i.e. it uses STLSetIntersection not URLPatternSet intersection. now I *thought* it had been fixed, and my memory was almost correct, except that it didn't actually get committed: https://codereview.chromium.org/181043006/ so... hmm! that's problematic. I'll follow up on that today. https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:138: UMA_HISTOGRAM_COUNTS_100( On 2014/08/08 00:51:43, gpdavis wrote: > On 2014/08/07 22:03:45, kalman wrote: > > hmmmmm so I don't think this actually is right, sorry. An extension can > request: > > > > { > > "https://*.google.com/*", > > "http://*/*" > > } > > > > the http://*/* will be withheld, but the https won't, so the > > GetEffectiveHostPermissions() count will be off by 1. > > > > a better estimate would be EffectiveHostPermissions.size - > > union(ScriptableHostPermissions, RequiredHostPermissions).size > > Which permissions are you talking about here? Withheld scriptable hosts and > extension required permissions (as in, > PermissionsParser::GetRequiredPermissions()?). ah. oh man, optional permissions do complicate things here. alright, I think that's fine and we can ignore optional permissions. - it's a corner case. - I think that we normalise optional permissions such that they can't be a subset of required permissions. - even if not, if we include the number of optional permissions granted as well, that's not necessarily wrong. so the answer to your question is yes, Required. https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:121: extensions::ActiveScriptController::GetForWebContents(web_contents) On 2014/08/08 00:51:43, gpdavis wrote: > On 2014/08/07 22:03:45, kalman wrote: > > elsewhere we seem to guard against a null web contents. we probably should > here > > as well. > > > > and this pattern is used a couple of times - might want to make a > > GetActiveWebContents() method on this class. > > Done. Should we be caching the result of GetActiveWebContents, or just making > the call twice? > > WebContents* web_contents = GetActiveWebContents(); > if (!web_contents) > return; > ...(web_contents)... > > Or... > > if (!GetActiveWebContents()) > return; > ...(GetActiveWebContents())... not fussed... I guess the first one is idiomatic.
So this last patchset will pass all tests if I add an early return in GetBoundedActivePermissions so that active permissions don't get bounded. Once the new changes go through for URLPatternSet union, this should work just fine. I made some other minor comment/naming changes to reflect the new behavior as well. https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:139: On 2014/08/08 14:31:28, kalman wrote: > On 2014/08/07 20:50:39, gpdavis wrote: > > On 2014/08/04 23:53:13, kalman wrote: > > > On 2014/08/01 20:06:06, gpdavis wrote: > > > > On 2014/08/01 18:15:11, kalman wrote: > > > > > we should add UMA for this. I think a useful metric would be how many > > pages > > > > the > > > > > user has persisted permissions for (after this method executes of > course), > > > > which > > > > > you can approximate using the size of the effective permission set: > > > > > > > > > > UMA_HISTOGRAM_COUNTS_100( > > > > > "Extensions.ActiveScriptController.PersistedPermissionCount", > > > > > > > extensions()->permissions_data()->GetEffectiveHostPermissions().size()); > > > > > > > > Okay, sure. Should I add both you and devlin as owners in histograms.xml? > > > > > > no, histograms.xml has its own owners. git-cl upload should suggest a > reviewer > > > for that. > > > > Oh, no, I meant for the <owner></owner> tags in the file itself (see the > > histograms.xml diff in the next patch set). > > I thought I replied to this question. yes, <owner> tag. I added this comment right before I added the one you responded to :) https://codereview.chromium.org/396033002/diff/110001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:140: // Update persisted permissions for extension. On 2014/08/08 14:31:28, kalman wrote: > > You mean as a part of the bound, meaning that active permissions are a part of > > the bounded active permissions if they're a subset of required, optional, and > > withheld? > > > > The problem is that if we have a withheld permission that implies all hosts, > > we're gonna get a script injection request for a particular site > > (http://example.com). Upon granting, http://example.com will be added to > active > > permissions. Then this won't work, because the specific host won't be part of > > the subset, as http://example.com isn't found in the withheld permissions; the > > permission that implies all hosts is. > > > > This is why I made the separate method to add non-all-host permissions from > > granted to active. > > ah I see the confusion here, on both our parts. in most cases host permissions > are aware that http://example.com is within <all_urls>. If you were to do > (taking {...} as URLPatternSet notation): > > {http://example.com/*, https://google.com/*%7D U {http://*/*} > > you *should* get > > {http://*/*, https://google.com/*%7D > > not > > {http://example.com/*, https://google.com/*, http://*/*} > > likewise intersection would be > > {http://example.com/*} not {} > > and this *should* be carried through to the permission set level. but it doesn't > look like it is: > > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... > --> > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... > > i.e. it uses STLSetIntersection not URLPatternSet intersection. > > now I *thought* it had been fixed, and my memory was almost correct, except that > it didn't actually get committed: > > https://codereview.chromium.org/181043006/ > > so... hmm! that's problematic. I'll follow up on that today. Ah! That does make a lot of sense. I wonder why that patch never went through... So I guess with that fixed, this will be simple. https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:138: UMA_HISTOGRAM_COUNTS_100( On 2014/08/08 14:31:28, kalman wrote: > On 2014/08/08 00:51:43, gpdavis wrote: > > On 2014/08/07 22:03:45, kalman wrote: > > > hmmmmm so I don't think this actually is right, sorry. An extension can > > request: > > > > > > { > > > "https://*.google.com/*", > > > "http://*/*" > > > } > > > > > > the http://*/* will be withheld, but the https won't, so the > > > GetEffectiveHostPermissions() count will be off by 1. > > > > > > a better estimate would be EffectiveHostPermissions.size - > > > union(ScriptableHostPermissions, RequiredHostPermissions).size > > > > Which permissions are you talking about here? Withheld scriptable hosts and > > extension required permissions (as in, > > PermissionsParser::GetRequiredPermissions()?). > > ah. oh man, optional permissions do complicate things here. > > alright, I think that's fine and we can ignore optional permissions. > - it's a corner case. > - I think that we normalise optional permissions such that they can't be a > subset of required permissions. > - even if not, if we include the number of optional permissions granted as well, > that's not necessarily wrong. > > so the answer to your question is yes, Required. So then, the size of withheld_permissions->scriptable_hosts() U PermissionsParser::GetRequiredPermissions()->scriptable_hosts()? https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/396033002/diff/170001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:121: extensions::ActiveScriptController::GetForWebContents(web_contents) On 2014/08/08 14:31:28, kalman wrote: > On 2014/08/08 00:51:43, gpdavis wrote: > > On 2014/08/07 22:03:45, kalman wrote: > > > elsewhere we seem to guard against a null web contents. we probably should > > here > > > as well. > > > > > > and this pattern is used a couple of times - might want to make a > > > GetActiveWebContents() method on this class. > > > > Done. Should we be caching the result of GetActiveWebContents, or just making > > the call twice? > > > > WebContents* web_contents = GetActiveWebContents(); > > if (!web_contents) > > return; > > ...(web_contents)... > > > > Or... > > > > if (!GetActiveWebContents()) > > return; > > ...(GetActiveWebContents())... > > not fussed... I guess the first one is idiomatic. Done.
isherman@chromium.org: Please review minor changes in tools/metrics/histograms/histograms.xml Thanks!
>
> Ah! That does make a lot of sense. I wonder why that patch never went
> through...
>
> So I guess with that fixed, this will be simple.
>
alright bad news. everything I've been saying was with the assumption that patch
went in, but it didn't, and it's because it actually wasn't right. it's
basically impossible to implement those compound set operations correctly in the
current infrastrcture, for example,
{"http://*.google.com} - {"http://www.google.com"} == {set which contains
*.google.com but not http://www.google.com but we can't represent that in
URLPatternSet}
I think we *could* make it work with some very serious refactoring but it's not
worth it. I have a better idea, and it's based on your idea Greg, so we should
implement that. I'll file a bug on it, CC you, and perhaps an older version of
your patch can be used to fix that.
On 2014/08/08 20:08:20, kalman wrote:
> >
> > Ah! That does make a lot of sense. I wonder why that patch never went
> > through...
> >
> > So I guess with that fixed, this will be simple.
> >
>
> alright bad news. everything I've been saying was with the assumption that
patch
> went in, but it didn't, and it's because it actually wasn't right. it's
> basically impossible to implement those compound set operations correctly in
the
> current infrastrcture, for example,
>
> {"http://*.google.com} - {"http://www.google.com"} == {set which contains
> *.google.com but not http://www.google.com but we can't represent that in
> URLPatternSet}
>
> I think we *could* make it work with some very serious refactoring but it's
not
> worth it. I have a better idea, and it's based on your idea Greg, so we should
> implement that. I'll file a bug on it, CC you, and perhaps an older version of
> your patch can be used to fix that.
I was referring specifically to
http://code.google.com/p/chromium/issues/detail?id=310815 but now I can't repro
that bug.
maybe it's possible to do something like early-return for bounded permissions.
actually here we go! temporarily, early-return if the extension has any withheld
permissions. does that work? we can work on the better solution afterwards, but
in the meantime let's get this code up and running.
work for you?
How's this look? Do you have any suggestions for further testing for MatchesSingleOrigin?
https://codereview.chromium.org/396033002/diff/230001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/396033002/diff/230001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6959: + The number of pages the user has allowed to always run script injections. Please document when this is recorded. https://codereview.chromium.org/396033002/diff/230001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6959: + The number of pages the user has allowed to always run script injections. I don't quite understand what this sentence means. Could you please try rephrasing to be a little clearer?
https://codereview.chromium.org/396033002/diff/250001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/396033002/diff/250001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6961: + context menu of a script injection request. Hopefully this is a bit more clear.
More chances for code cleanup, hooray. https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:114: pattern.SetPath("/*"); I think you should do something similar to: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... and in fact it (a) should really use the same code, and (b) that other code should have a .GetOrigin() after the .GetURL() I think. hm, ok, here's another plan: move *that* to URLPatternSet, and have URLPatternSet.AddOrigin(const GURL&), which basically does this: URLPatternSet::AddOrigin(int valid_schemes, const GURL& origin) { DCHECK_EQ(origin.GetOrigin(), origin); URLPattern origin_pattern(valid_schemes); CHECK(origin_pattern.Parse(origin.GetOrigin().spec()); AddPattern(origin_pattern); } then use it in ActiveTabScriptController and the new place. Sheesh, sorry to keep you running around, but it's also the code clean :) https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:119: if (!web_contents) please phrase this if (web_contents) { extensions::ActiveScriptController::GetForWebContents(web_contents) ->AlwaysRunOnVisibleHost(extension); } break; https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:196: AddItemWithStringId(ALWAYS_RUN, IDS_EXTENSIONS_ALWAYS_RUN); This is actually really subtle. The way it's written now, if the extension is showing a script badge then it's going to get this menu item, *whether or not the context menu is actually on the badge*. E.g. you'll see "always run" on the browser action context menu. Fixing this is probably going to be a PITA, threading through knowledge of where in the UI this ExtensionContextMenuModel is hosted, which I don't think it's done. I'm also not sure it really matters, particularly given eventually there will only be 1 action anyway. Plus, this UI is just a prototype. So no need to change this code, but you might want to mention it in a comment, like // Add the "always allow" if there is a script injection request for // this extension. Note that this will add it to *all* extension action context // menus, not just the one attached to the script injection request icon, // but that's ok. https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.h (right): https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.h:35: ALWAYS_RUN, add it to the bottom. https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:71: // to persist "always run" script injection hosts. Mention why they were filtered out in the first place. https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:88: you could write this much more concisely if you had a FilterSingleOriginPatterns method, such that you can call adjusted_active = PermissionSet::CreateUnion( adjusted_active, new PermissionSet(APIPermissionSet(), ManifestPermissionSet(), FilterSingleOriginPermissions(active_permissions->explicit_hosts()), FilterSingleOriginPermissions(active_permissions->scriptable_hosts()); formatting modulo git-cl format. https://codereview.chromium.org/396033002/diff/250001/extensions/common/url_p... File extensions/common/url_pattern.cc (right): https://codereview.chromium.org/396033002/diff/250001/extensions/common/url_p... extensions/common/url_pattern.cc:470: return !(ImpliesAllHosts() || match_subdomains_); nit: I find negation of nested conditions harder to parse than unwrapping them like !ImpliesAllHosts && !match_subdomains. https://codereview.chromium.org/396033002/diff/250001/extensions/common/url_p... File extensions/common/url_pattern_unittest.cc (right): https://codereview.chromium.org/396033002/diff/250001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:824: EXPECT_TRUE(pattern3.MatchesSingleOrigin()); I think it's unnecessarily complex to be making these in 3 stages. you can do it all at once, since parsing should succeed: EXPECT_FALSE(URLPattern(URLPattern::SCHEME_ALL, "http://*/*") .MatchesSingleOrigin()); EXPECT_FALSE(URLPattern(URLPattern::SCHEME_ALL, "http://*.google.com/*") .MatchesSingleOrigin()); ... also test wildcard scheme like *://google.com, wildcards like *.com, longer paths, and paths that aren't * - assuming that last one parses, but it would appear so: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
https://codereview.chromium.org/396033002/diff/250001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/396033002/diff/250001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6959: + The number of times the user has given a host persistent permission to run Hmm, the number of times since when? The beginning of time? It might be more appropriate to measure this user interaction by logging an enumerated histogram that has buckets for (1) script injection request occurred, (2) the context menu of the request was clicked, and (3) "Always Run" was chosen from the context menu -- or something like that. Though, whether that's better depends on what exactly you're trying to understand about this feature.
kalman, can I also get your input on isherman@'s histogram feedback? https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:114: pattern.SetPath("/*"); On 2014/08/11 22:59:29, kalman wrote: > I think you should do something similar to: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > and in fact it (a) should really use the same code, and (b) that other code > should have a .GetOrigin() after the .GetURL() I think. > > hm, ok, here's another plan: move *that* to URLPatternSet, and have > URLPatternSet.AddOrigin(const GURL&), which basically does this: > > URLPatternSet::AddOrigin(int valid_schemes, const GURL& origin) { > DCHECK_EQ(origin.GetOrigin(), origin); > URLPattern origin_pattern(valid_schemes); > CHECK(origin_pattern.Parse(origin.GetOrigin().spec()); > AddPattern(origin_pattern); > } > > then use it in ActiveTabScriptController and the new place. > > Sheesh, sorry to keep you running around, but it's also the code clean :) Sweet deal. No worries; I see the value in factoring this out. https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:119: if (!web_contents) On 2014/08/11 22:59:30, kalman wrote: > please phrase this > > if (web_contents) { > extensions::ActiveScriptController::GetForWebContents(web_contents) > ->AlwaysRunOnVisibleHost(extension); > } > break; Solid. I think I'm starting to get used to where early returning is appropriate. https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:196: AddItemWithStringId(ALWAYS_RUN, IDS_EXTENSIONS_ALWAYS_RUN); On 2014/08/11 22:59:30, kalman wrote: > This is actually really subtle. The way it's written now, if the extension is > showing a script badge then it's going to get this menu item, *whether or not > the context menu is actually on the badge*. > > E.g. you'll see "always run" on the browser action context menu. > > Fixing this is probably going to be a PITA, threading through knowledge of where > in the UI this ExtensionContextMenuModel is hosted, which I don't think it's > done. > > I'm also not sure it really matters, particularly given eventually there will > only be 1 action anyway. Plus, this UI is just a prototype. > > So no need to change this code, but you might want to mention it in a comment, > like > > // Add the "always allow" if there is a script injection request for > // this extension. Note that this will add it to *all* extension action context > // menus, not just the one attached to the script injection request icon, > // but that's ok. Wow, that is subtle. Nice catch. I added the comment. https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.h (right): https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.h:35: ALWAYS_RUN, On 2014/08/11 22:59:30, kalman wrote: > add it to the bottom. Done. https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:71: // to persist "always run" script injection hosts. On 2014/08/11 22:59:30, kalman wrote: > Mention why they were filtered out in the first place. Done. https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:88: On 2014/08/11 22:59:30, kalman wrote: > you could write this much more concisely if you had a FilterSingleOriginPatterns > method, such that you can call > > adjusted_active = PermissionSet::CreateUnion( > adjusted_active, > new PermissionSet(APIPermissionSet(), > ManifestPermissionSet(), > > FilterSingleOriginPermissions(active_permissions->explicit_hosts()), > > FilterSingleOriginPermissions(active_permissions->scriptable_hosts()); > > formatting modulo git-cl format. Done. https://codereview.chromium.org/396033002/diff/250001/extensions/common/url_p... File extensions/common/url_pattern.cc (right): https://codereview.chromium.org/396033002/diff/250001/extensions/common/url_p... extensions/common/url_pattern.cc:470: return !(ImpliesAllHosts() || match_subdomains_); On 2014/08/11 22:59:30, kalman wrote: > nit: I find negation of nested conditions harder to parse than unwrapping them > like !ImpliesAllHosts && !match_subdomains. Done. https://codereview.chromium.org/396033002/diff/250001/extensions/common/url_p... File extensions/common/url_pattern_unittest.cc (right): https://codereview.chromium.org/396033002/diff/250001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:824: EXPECT_TRUE(pattern3.MatchesSingleOrigin()); On 2014/08/11 22:59:30, kalman wrote: > I think it's unnecessarily complex to be making these in 3 stages. you can do it > all at once, since parsing should succeed: > > EXPECT_FALSE(URLPattern(URLPattern::SCHEME_ALL, "http://*/*") > .MatchesSingleOrigin()); > EXPECT_FALSE(URLPattern(URLPattern::SCHEME_ALL, "http://*.google.com/*") > .MatchesSingleOrigin()); > ... > > also test wildcard scheme like *://google.com, wildcards like *.com, longer > paths, and paths that aren't * - assuming that last one parses, but it would > appear so: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... Shouldn't MatchesSingleOrigin also check the scheme (and port)? Because an origin is a scheme, host, and port, correct?
Ok, I think we're basically done here. Pull the UMA out into a follow-up and we can discuss that there. Other than that, just a couple of nits. https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:119: if (!web_contents) On 2014/08/12 00:52:05, gpdavis wrote: > On 2014/08/11 22:59:30, kalman wrote: > > please phrase this > > > > if (web_contents) { > > extensions::ActiveScriptController::GetForWebContents(web_contents) > > ->AlwaysRunOnVisibleHost(extension); > > } > > break; > > Solid. I think I'm starting to get used to where early returning is > appropriate. I admit that a lot of it is aesthetic, but here, returning in the middle of a switch statement in one of the cases is a bit confusing. https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/396033002/diff/250001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:71: // to persist "always run" script injection hosts. On 2014/08/12 00:52:05, gpdavis wrote: > On 2014/08/11 22:59:30, kalman wrote: > > Mention why they were filtered out in the first place. > > Done. The comment still doesn't work for me, but I think it's because this is actually in the wrong place. See newer comment. https://codereview.chromium.org/396033002/diff/250001/extensions/common/url_p... File extensions/common/url_pattern_unittest.cc (right): https://codereview.chromium.org/396033002/diff/250001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:824: EXPECT_TRUE(pattern3.MatchesSingleOrigin()); On 2014/08/12 00:52:06, gpdavis wrote: > On 2014/08/11 22:59:30, kalman wrote: > > I think it's unnecessarily complex to be making these in 3 stages. you can do > it > > all at once, since parsing should succeed: > > > > EXPECT_FALSE(URLPattern(URLPattern::SCHEME_ALL, "http://*/*") > > .MatchesSingleOrigin()); > > EXPECT_FALSE(URLPattern(URLPattern::SCHEME_ALL, "http://*.google.com/*") > > .MatchesSingleOrigin()); > > ... > > > > also test wildcard scheme like *://google.com, wildcards like *.com, longer > > paths, and paths that aren't * - assuming that last one parses, but it would > > appear so: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > Shouldn't MatchesSingleOrigin also check the scheme (and port)? Because an > origin is a scheme, host, and port, correct? Yes it should check the scheme and port. https://codereview.chromium.org/396033002/diff/250001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/396033002/diff/250001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6959: + The number of times the user has given a host persistent permission to run On 2014/08/12 00:02:11, Ilya Sherman wrote: > Hmm, the number of times since when? The beginning of time? > > It might be more appropriate to measure this user interaction by logging an > enumerated histogram that has buckets for (1) script injection request occurred, > (2) the context menu of the request was clicked, and (3) "Always Run" was chosen > from the context menu -- or something like that. Though, whether that's better > depends on what exactly you're trying to understand about this feature. That metric you mention is interesting, but I would imagine hard to analyse - you would expect (3) to be low compared to the others given that "always run" will only be checked once per domain, which persists, so they'd never be shown anything again. Hm actually now that we have this feature it probably *is* worth recording that, since you'd expect all these numbers to potentially drop, but who knows what the correlation is due to. Short story: I agree that we should do this, but we should log that in LogUMA (also requires tracking it as a separate counter). I'll comment on the CL the changes we should make for that. *However* I'm also interested in the behaviour of users with respect to how many they always-allow. I think that's relevant. The distribution of that would be interesting - do users never do it (never see it, never care?). Only do it on very few sites? Do it on most sites they visit? I'm not totally confident that the data we get will actually be conclusive, but I'm willing to try. https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:117: extensions::UserScript::ValidUserScriptSchemes(), url.GetOrigin()); don't need extensions:: here or below. https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:257: permitted_extensions_.insert(extension->id()); Ok - this has exposed a bug in our current UMA of this file. RunPendingForExtension is called from 2 places: the ActiveTabPermissionGranter, and OnClicked. The former will add to |permitted_extensions_| here even though it may be that nothing was actually permitted. I think the problem really is that we use |permitted_extensions_| for "this extension was allowed to run" when really it's "this extension ran". We should split those up - add 3 counters, - ran due to active tab - ran due to clicked on - ran due to always-allow and have the call sites of RunPendingForExtension set them (perhaps RunPendingForExtension should be changed to return a bool whether it ran). However - all of this UMA stuff is getting sufficiently complex that we should probably do it in a follow-up. https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:368: pending_requests_.size()); As Ilya suggests, let's also log the always-run extensions here. We need to make sure that those aren't included in PermittedExtensions though. https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... File chrome/browser/extensions/active_tab_permission_granter.cc (right): https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... chrome/browser/extensions/active_tab_permission_granter.cc:50: // Origin adding could fail if this is an unsupported URL e.g. chrome://. comment isn't for here (it should be inside the AddOrigin call). https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... chrome/browser/extensions/active_tab_permission_granter.cc:52: web_contents()->GetURL().GetOrigin()); cleanup: use GetVisibleURL() here. https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:39: URLPatternSet FilterSingleOriginPermissions(URLPatternSet permissions) { const URLPatternSet& permissions https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:205: granted_scriptable_hosts); I actually think this would be a better place to add those single origin permissions back - even though it's effectively the same, it makes more sense in context here. Then you could add a comment like: // After withholding permissions, add back any origins to the active set that may have been lost during the set operations that would have dropped them. For example, the union of <all_urls> and <anything> is <all_urls>, so we may lose the <anything>. However, that <anything> is important once <all_urls> is stripped during withholding. Now - there is a slight problem that only GetBoundedActivePermissions knows about the permissions it needs to re-add. You can fix that by changing GetBoundedActivePermissions to be passed the active permission set rather than the extension prefs, then move it in here. https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... File extensions/common/url_pattern.cc (right): https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... extensions/common/url_pattern.cc:470: return scheme_ != "*" && !ImpliesAllHosts() && !match_subdomains_; you should check the port here as well. and as a nit, it would be nice to arrange the checks in the order that they appear, so like return !ImpliesAllHosts() && scheme_ != "*" && port_ != "*" && !match_subdomains_; https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... File extensions/common/url_pattern_set.cc (right): https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... extensions/common/url_pattern_set.cc:147: URLPattern origin_pattern(valid_schemes); add that comment about URL parsing potentially failing if |origin| doesn't match |valid_schemes|. https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... File extensions/common/url_pattern_unittest.cc (right): https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:816: EXPECT_FALSE(URLPattern(URLPattern::SCHEME_ALL, "http://*.google.com/*") mix up the http and https in here? https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:818: EXPECT_TRUE(URLPattern(URLPattern::SCHEME_ALL, "http://www.google.com/") could you test http://google.com here as well? https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:821: .MatchesSingleOrigin()); also test a wildcard port https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:828: .MatchesSingleOrigin()); also add a couple of tests with a URLPattern mask that doesn't match (like, just match http but test https).
https://codereview.chromium.org/396033002/diff/250001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/396033002/diff/250001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6959: + The number of times the user has given a host persistent permission to run On 2014/08/12 19:49:26, kalman wrote: > On 2014/08/12 00:02:11, Ilya Sherman wrote: > > Hmm, the number of times since when? The beginning of time? > > > > It might be more appropriate to measure this user interaction by logging an > > enumerated histogram that has buckets for (1) script injection request > occurred, > > (2) the context menu of the request was clicked, and (3) "Always Run" was > chosen > > from the context menu -- or something like that. Though, whether that's > better > > depends on what exactly you're trying to understand about this feature. > > That metric you mention is interesting, but I would imagine hard to analyse - > you would expect (3) to be low compared to the others given that "always run" > will only be checked once per domain, which persists, so they'd never be shown > anything again. Hm actually now that we have this feature it probably *is* worth > recording that, since you'd expect all these numbers to potentially drop, but > who knows what the correlation is due to. > > Short story: I agree that we should do this, but we should log that in LogUMA > (also requires tracking it as a separate counter). I'll comment on the CL the > changes we should make for that. > > *However* I'm also interested in the behaviour of users with respect to how many > they always-allow. I think that's relevant. The distribution of that would be > interesting - do users never do it (never see it, never care?). Only do it on > very few sites? Do it on most sites they visit? I'm not totally confident that > the data we get will actually be conclusive, but I'm willing to try. Keep in mind that if you're interested in knowing how many sites users have whitelisted, you might want to log that based on a different trigger -- e.g. once per UMA upload, or once per Chrome launch, or something like that. If you only log at the time that a user adds a site, you'll get more samples uploaded from users who add sites more frequently. You could certainly try to account for that bias when viewing the data, but it might be better to try to reduce the bias to begin with.
> > Keep in mind that if you're interested in knowing how many sites users have > whitelisted, you might want to log that based on a different trigger -- e.g. > once per UMA upload, or once per Chrome launch, or something like that. If you > only log at the time that a user adds a site, you'll get more samples uploaded > from users who add sites more frequently. You could certainly try to account > for that bias when viewing the data, but it might be better to try to reduce the > bias to begin with. That makes sense. Devlin, we could do this at the same time we log how many extensions are opted out of <all_urls>.
https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:117: extensions::UserScript::ValidUserScriptSchemes(), url.GetOrigin()); On 2014/08/12 19:49:27, kalman wrote: > don't need extensions:: here or below. Done. https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:257: permitted_extensions_.insert(extension->id()); On 2014/08/12 19:49:27, kalman wrote: > Ok - this has exposed a bug in our current UMA of this file. > RunPendingForExtension is called from 2 places: the ActiveTabPermissionGranter, > and OnClicked. The former will add to |permitted_extensions_| here even though > it may be that nothing was actually permitted. > > I think the problem really is that we use |permitted_extensions_| for "this > extension was allowed to run" when really it's "this extension ran". We should > split those up - add 3 counters, > - ran due to active tab > - ran due to clicked on > - ran due to always-allow > > and have the call sites of RunPendingForExtension set them (perhaps > RunPendingForExtension should be changed to return a bool whether it ran). > > However - all of this UMA stuff is getting sufficiently complex that we should > probably do it in a follow-up. Alright, let's do all of that in a follow-up CL then. I'll address the non-UMA comments. https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... File chrome/browser/extensions/active_tab_permission_granter.cc (right): https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... chrome/browser/extensions/active_tab_permission_granter.cc:50: // Origin adding could fail if this is an unsupported URL e.g. chrome://. On 2014/08/12 19:49:27, kalman wrote: > comment isn't for here (it should be inside the AddOrigin call). Done. https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... chrome/browser/extensions/active_tab_permission_granter.cc:52: web_contents()->GetURL().GetOrigin()); On 2014/08/12 19:49:27, kalman wrote: > cleanup: use GetVisibleURL() here. Done. https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:39: URLPatternSet FilterSingleOriginPermissions(URLPatternSet permissions) { On 2014/08/12 19:49:27, kalman wrote: > const URLPatternSet& permissions Done. https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:205: granted_scriptable_hosts); On 2014/08/12 19:49:27, kalman wrote: > I actually think this would be a better place to add those single origin > permissions back - even though it's effectively the same, it makes more sense in > context here. Then you could add a comment like: > > // After withholding permissions, add back any origins to the active set that > may have been lost during the set operations that would have dropped them. For > example, the union of <all_urls> and <anything> is <all_urls>, so we may lose > the <anything>. However, that <anything> is important once <all_urls> is > stripped during withholding. > > Now - there is a slight problem that only GetBoundedActivePermissions knows > about the permissions it needs to re-add. You can fix that by changing > GetBoundedActivePermissions to be passed the active permission set rather than > the extension prefs, then move it in here. Done. https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... File extensions/common/url_pattern.cc (right): https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... extensions/common/url_pattern.cc:470: return scheme_ != "*" && !ImpliesAllHosts() && !match_subdomains_; On 2014/08/12 19:49:27, kalman wrote: > you should check the port here as well. > > and as a nit, it would be nice to arrange the checks in the order that they > appear, so like > > return !ImpliesAllHosts() && scheme_ != "*" && port_ != "*" && > !match_subdomains_; When I test this out (logging the visible url's port in the ActiveScriptController before its origin is added) and hit "always run" on the google homepage, the port is blank, so it gets added as "*" when it's parsed in AddOrigin. How should the port be dealt with? https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... File extensions/common/url_pattern_set.cc (right): https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... extensions/common/url_pattern_set.cc:147: URLPattern origin_pattern(valid_schemes); On 2014/08/12 19:49:27, kalman wrote: > add that comment about URL parsing potentially failing if |origin| doesn't match > |valid_schemes|. Done. https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... File extensions/common/url_pattern_unittest.cc (right): https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:816: EXPECT_FALSE(URLPattern(URLPattern::SCHEME_ALL, "http://*.google.com/*") On 2014/08/12 19:49:27, kalman wrote: > mix up the http and https in here? Sure. https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:818: EXPECT_TRUE(URLPattern(URLPattern::SCHEME_ALL, "http://www.google.com/") On 2014/08/12 19:49:27, kalman wrote: > could you test http://google.com here as well? http://google.com doesn't parse due to an invalid path, but I can add http://google.com/ Should http://google.com/ match a single origin? It currently returns true. Shouldn't match_subdomains_ be true in this case? https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:821: .MatchesSingleOrigin()); On 2014/08/12 19:49:27, kalman wrote: > also test a wildcard port See previous comment about port issue. Calling port() for the visible URL at https://www.google.com/?gws_rd=ssl returns an empty string. https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:828: .MatchesSingleOrigin()); On 2014/08/12 19:49:27, kalman wrote: > also add a couple of tests with a URLPattern mask that doesn't match (like, just > match http but test https). You mean like... EXPECT_FALSE(URLPattern(URLPattern::SCHEME_ALL, "http://*.google.com/*").MatchesURL("https://www.google.com/")); ?
https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:257: permitted_extensions_.insert(extension->id()); On 2014/08/12 21:19:54, gpdavis wrote: > On 2014/08/12 19:49:27, kalman wrote: > > Ok - this has exposed a bug in our current UMA of this file. > > RunPendingForExtension is called from 2 places: the > ActiveTabPermissionGranter, > > and OnClicked. The former will add to |permitted_extensions_| here even though > > it may be that nothing was actually permitted. > > > > I think the problem really is that we use |permitted_extensions_| for "this > > extension was allowed to run" when really it's "this extension ran". We should > > split those up - add 3 counters, > > - ran due to active tab > > - ran due to clicked on > > - ran due to always-allow > > > > and have the call sites of RunPendingForExtension set them (perhaps > > RunPendingForExtension should be changed to return a bool whether it ran). > > > > However - all of this UMA stuff is getting sufficiently complex that we should > > probably do it in a follow-up. > > Alright, let's do all of that in a follow-up CL then. I'll address the non-UMA > comments. Looks like you haven't removed the UMA from this patch? I'm saying you should store away the UMA changes (delete them), submit this CL, then have a 2nd patch to add them back + make those fixes. and by "storage and delete" I mean both in this file and the histograms.xml changes. https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... File extensions/common/url_pattern.cc (right): https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... extensions/common/url_pattern.cc:470: return scheme_ != "*" && !ImpliesAllHosts() && !match_subdomains_; On 2014/08/12 21:19:55, gpdavis wrote: > On 2014/08/12 19:49:27, kalman wrote: > > you should check the port here as well. > > > > and as a nit, it would be nice to arrange the checks in the order that they > > appear, so like > > > > return !ImpliesAllHosts() && scheme_ != "*" && port_ != "*" && > > !match_subdomains_; > > When I test this out (logging the visible url's port in the > ActiveScriptController before its origin is added) and hit "always run" on the > google homepage, the port is blank, so it gets added as "*" when it's parsed in > AddOrigin. > > How should the port be dealt with? Ah ok. Yeah, forget about port, whatever. Just comment that it's not interesting. https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... File extensions/common/url_pattern_unittest.cc (right): https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:818: EXPECT_TRUE(URLPattern(URLPattern::SCHEME_ALL, "http://www.google.com/") On 2014/08/12 21:19:55, gpdavis wrote: > On 2014/08/12 19:49:27, kalman wrote: > > could you test http://google.com here as well? > > http://google.com doesn't parse due to an invalid path, but I can add > http://google.com/ Yes add http://google.com/ and/or http://google.com/* > > Should http://google.com/ match a single origin? It currently returns true. > Shouldn't match_subdomains_ be true in this case? Yes, it should return true, and match_subdomains_ should be false. Just because a URL doesn't specify any subdomains doesn't mean it matches all of them - that's what *.google.com is. https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:828: .MatchesSingleOrigin()); On 2014/08/12 21:19:56, gpdavis wrote: > On 2014/08/12 19:49:27, kalman wrote: > > also add a couple of tests with a URLPattern mask that doesn't match (like, > just > > match http but test https). > > You mean like... > > EXPECT_FALSE(URLPattern(URLPattern::SCHEME_ALL, > "http://*.google.com/*").MatchesURL("https://www.google.com/")); > > ? No, I mean like EXPECT_FALSE(URLPattern(URLPattern::SCHEME_HTTP, "https://google.com/*"), .MatchesSingleOrigin()); because the scheme mask is HTTP but the scheme is https. https://codereview.chromium.org/396033002/diff/310001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/396033002/diff/310001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:159: GetBoundedActivePermissions(extension, active_permissions); active_permissions.get() ? https://codereview.chromium.org/396033002/diff/310001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:197: &granted_explicit_hosts); I'm not so sure about writing the union to one of the inputs :\ i.e. the fact that |granted_explicit_hosts| is used both as an in-param and an out-param looks problematic. Luckily, you can write this much nicer anyway: granted_explicit_hosts.AddPatterns(FilterSingleOriginPermissions(..)); granted_scriptable_hosts...
https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/270001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:257: permitted_extensions_.insert(extension->id()); On 2014/08/12 23:13:17, kalman wrote: > On 2014/08/12 21:19:54, gpdavis wrote: > > On 2014/08/12 19:49:27, kalman wrote: > > > Ok - this has exposed a bug in our current UMA of this file. > > > RunPendingForExtension is called from 2 places: the > > ActiveTabPermissionGranter, > > > and OnClicked. The former will add to |permitted_extensions_| here even > though > > > it may be that nothing was actually permitted. > > > > > > I think the problem really is that we use |permitted_extensions_| for "this > > > extension was allowed to run" when really it's "this extension ran". We > should > > > split those up - add 3 counters, > > > - ran due to active tab > > > - ran due to clicked on > > > - ran due to always-allow > > > > > > and have the call sites of RunPendingForExtension set them (perhaps > > > RunPendingForExtension should be changed to return a bool whether it ran). > > > > > > However - all of this UMA stuff is getting sufficiently complex that we > should > > > probably do it in a follow-up. > > > > Alright, let's do all of that in a follow-up CL then. I'll address the > non-UMA > > comments. > > Looks like you haven't removed the UMA from this patch? I'm saying you should > store away the UMA changes (delete them), submit this CL, then have a 2nd patch > to add them back + make those fixes. > > and by "storage and delete" I mean both in this file and the histograms.xml > changes. Oops-- sorry about that. Had so many changes in this patch floating around in my head and I forgot to remove those. Removing now... https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... File extensions/common/url_pattern.cc (right): https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... extensions/common/url_pattern.cc:470: return scheme_ != "*" && !ImpliesAllHosts() && !match_subdomains_; On 2014/08/12 23:13:18, kalman wrote: > On 2014/08/12 21:19:55, gpdavis wrote: > > On 2014/08/12 19:49:27, kalman wrote: > > > you should check the port here as well. > > > > > > and as a nit, it would be nice to arrange the checks in the order that they > > > appear, so like > > > > > > return !ImpliesAllHosts() && scheme_ != "*" && port_ != "*" && > > > !match_subdomains_; > > > > When I test this out (logging the visible url's port in the > > ActiveScriptController before its origin is added) and hit "always run" on the > > google homepage, the port is blank, so it gets added as "*" when it's parsed > in > > AddOrigin. > > > > How should the port be dealt with? > > Ah ok. Yeah, forget about port, whatever. Just comment that it's not > interesting. Done. https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... File extensions/common/url_pattern_unittest.cc (right): https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:818: EXPECT_TRUE(URLPattern(URLPattern::SCHEME_ALL, "http://www.google.com/") On 2014/08/12 23:13:18, kalman wrote: > On 2014/08/12 21:19:55, gpdavis wrote: > > On 2014/08/12 19:49:27, kalman wrote: > > > could you test http://google.com here as well? > > > > http://google.com doesn't parse due to an invalid path, but I can add > > http://google.com/ > > Yes add http://google.com/ and/or http://google.com/* > > > > > Should http://google.com/ match a single origin? It currently returns true. > > Shouldn't match_subdomains_ be true in this case? > > Yes, it should return true, and match_subdomains_ should be false. Just because > a URL doesn't specify any subdomains doesn't mean it matches all of them - > that's what *.google.com is. Gotcha. https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:828: .MatchesSingleOrigin()); On 2014/08/12 23:13:18, kalman wrote: > On 2014/08/12 21:19:56, gpdavis wrote: > > On 2014/08/12 19:49:27, kalman wrote: > > > also add a couple of tests with a URLPattern mask that doesn't match (like, > > just > > > match http but test https). > > > > You mean like... > > > > EXPECT_FALSE(URLPattern(URLPattern::SCHEME_ALL, > > "http://*.google.com/*").MatchesURL("https://www.google.com/")); > > > > ? > > No, I mean like > > EXPECT_FALSE(URLPattern(URLPattern::SCHEME_HTTP, "https://google.com/*"), > .MatchesSingleOrigin()); > > because the scheme mask is HTTP but the scheme is https. Ahh, that's what you meant by URLPattern mask. But your example failed a check because it won't parse if the schemes don't match, crashing the test. I'm not sure I understand why we should be testing this anyway. Aren't we just testing MatchesSingleOrigin? https://codereview.chromium.org/396033002/diff/310001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/396033002/diff/310001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:159: GetBoundedActivePermissions(extension, active_permissions); On 2014/08/12 23:13:18, kalman wrote: > active_permissions.get() ? Ah, oops, missed that. Looks like the compiler knows how to convert the smart pointer, but there's no reason not to be concise. Done. https://codereview.chromium.org/396033002/diff/310001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:197: &granted_explicit_hosts); On 2014/08/12 23:13:18, kalman wrote: > I'm not so sure about writing the union to one of the inputs :\ i.e. the fact > that |granted_explicit_hosts| is used both as an in-param and an out-param looks > problematic. > > Luckily, you can write this much nicer anyway: > > granted_explicit_hosts.AddPatterns(FilterSingleOriginPermissions(..)); > granted_scriptable_hosts... Fair enough. That is much simpler anyway.
lgtm https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... File extensions/common/url_pattern_unittest.cc (right): https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:828: .MatchesSingleOrigin()); On 2014/08/13 00:08:24, gpdavis wrote: > On 2014/08/12 23:13:18, kalman wrote: > > On 2014/08/12 21:19:56, gpdavis wrote: > > > On 2014/08/12 19:49:27, kalman wrote: > > > > also add a couple of tests with a URLPattern mask that doesn't match > (like, > > > just > > > > match http but test https). > > > > > > You mean like... > > > > > > EXPECT_FALSE(URLPattern(URLPattern::SCHEME_ALL, > > > "http://*.google.com/*").MatchesURL("https://www.google.com/")); > > > > > > ? > > > > No, I mean like > > > > EXPECT_FALSE(URLPattern(URLPattern::SCHEME_HTTP, "https://google.com/*"), > > .MatchesSingleOrigin()); > > > > because the scheme mask is HTTP but the scheme is https. > > Ahh, that's what you meant by URLPattern mask. But your example failed a check > because it won't parse if the schemes don't match, crashing the test. > > I'm not sure I understand why we should be testing this anyway. Aren't we just > testing MatchesSingleOrigin? Oh yeah, I'm thinking of AddOrigin. Sorry! https://codereview.chromium.org/396033002/diff/310001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/396033002/diff/310001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:159: GetBoundedActivePermissions(extension, active_permissions); On 2014/08/13 00:08:24, gpdavis wrote: > On 2014/08/12 23:13:18, kalman wrote: > > active_permissions.get() ? > > Ah, oops, missed that. Looks like the compiler knows how to convert the smart > pointer, but there's no reason not to be concise. Done. Wow, what platform are you compiling on? https://codereview.chromium.org/396033002/diff/330001/extensions/common/url_p... File extensions/common/url_pattern_unittest.cc (right): https://codereview.chromium.org/396033002/diff/330001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:832: .MatchesSingleOrigin()); Ok, HERE we go. Please add a test with a URLPattern mask that isn't ALL :)
Good to go? :) https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... File extensions/common/url_pattern_unittest.cc (right): https://codereview.chromium.org/396033002/diff/270001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:828: .MatchesSingleOrigin()); On 2014/08/13 01:02:59, kalman wrote: > On 2014/08/13 00:08:24, gpdavis wrote: > > On 2014/08/12 23:13:18, kalman wrote: > > > On 2014/08/12 21:19:56, gpdavis wrote: > > > > On 2014/08/12 19:49:27, kalman wrote: > > > > > also add a couple of tests with a URLPattern mask that doesn't match > > (like, > > > > just > > > > > match http but test https). > > > > > > > > You mean like... > > > > > > > > EXPECT_FALSE(URLPattern(URLPattern::SCHEME_ALL, > > > > "http://*.google.com/*").MatchesURL("https://www.google.com/")); > > > > > > > > ? > > > > > > No, I mean like > > > > > > EXPECT_FALSE(URLPattern(URLPattern::SCHEME_HTTP, "https://google.com/*"), > > > .MatchesSingleOrigin()); > > > > > > because the scheme mask is HTTP but the scheme is https. > > > > Ahh, that's what you meant by URLPattern mask. But your example failed a > check > > because it won't parse if the schemes don't match, crashing the test. > > > > I'm not sure I understand why we should be testing this anyway. Aren't we > just > > testing MatchesSingleOrigin? > > Oh yeah, I'm thinking of AddOrigin. Sorry! No problem! https://codereview.chromium.org/396033002/diff/310001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/396033002/diff/310001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:159: GetBoundedActivePermissions(extension, active_permissions); On 2014/08/13 01:02:59, kalman wrote: > On 2014/08/13 00:08:24, gpdavis wrote: > > On 2014/08/12 23:13:18, kalman wrote: > > > active_permissions.get() ? > > > > Ah, oops, missed that. Looks like the compiler knows how to convert the smart > > pointer, but there's no reason not to be concise. Done. > > Wow, what platform are you compiling on? Ubuntu 12.04. I didn't even get a compiler warning about it *shrug* https://codereview.chromium.org/396033002/diff/330001/extensions/common/url_p... File extensions/common/url_pattern_unittest.cc (right): https://codereview.chromium.org/396033002/diff/330001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:832: .MatchesSingleOrigin()); On 2014/08/13 01:02:59, kalman wrote: > Ok, HERE we go. Please add a test with a URLPattern mask that isn't ALL :) Can do!
lgtm, good to go! modulo running git-cl format.
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gpdavis.chromium@gmail.com/396033002/3...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
chrome/browser/extensions/extension_context_menu_model.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/extensions/extension_context_menu_model.cc
Hunk #1 FAILED at 6.
Hunk #2 succeeded at 31 (offset 5 lines).
Hunk #3 succeeded at 121 (offset 41 lines).
Hunk #4 succeeded at 164 (offset 51 lines).
Hunk #5 succeeded at 245 with fuzz 2 (offset 60 lines).
Hunk #6 succeeded at 287 with fuzz 2 (offset 77 lines).
1 out of 6 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_context_menu_model.cc.rej
Patch: chrome/browser/extensions/extension_context_menu_model.cc
Index: chrome/browser/extensions/extension_context_menu_model.cc
diff --git a/chrome/browser/extensions/extension_context_menu_model.cc
b/chrome/browser/extensions/extension_context_menu_model.cc
index
3334fd9ccc063a8409a9b146992717331460d3b8..9f38577fbd3ceb508d27c66edf4507395c562dbc
100644
--- a/chrome/browser/extensions/extension_context_menu_model.cc
+++ b/chrome/browser/extensions/extension_context_menu_model.cc
@@ -6,6 +6,7 @@
#include "base/prefs/pref_service.h"
#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/extensions/active_script_controller.h"
#include
"chrome/browser/extensions/api/extension_action/extension_action_api.h"
#include "chrome/browser/extensions/extension_action.h"
#include "chrome/browser/extensions/extension_action_manager.h"
@@ -26,6 +27,7 @@
#include "extensions/browser/management_policy.h"
#include "extensions/browser/uninstall_reason.h"
#include "extensions/common/extension.h"
+#include "extensions/common/manifest_constants.h"
#include "grit/chromium_strings.h"
#include "grit/generated_resources.h"
#include "ui/base/l10n/l10n_util.h"
@@ -79,8 +81,7 @@ bool ExtensionContextMenuModel::IsCommandIdEnabled(int
command_id) const {
// homepage, we just disable this menu item.
return extensions::ManifestURL::GetHomepageURL(extension).is_valid();
} else if (command_id == INSPECT_POPUP) {
- WebContents* web_contents =
- browser_->tab_strip_model()->GetActiveWebContents();
+ WebContents* web_contents = GetActiveWebContents();
if (!web_contents)
return false;
@@ -113,6 +114,14 @@ void ExtensionContextMenuModel::ExecuteCommand(int
command_id,
browser_->OpenURL(params);
break;
}
+ case ALWAYS_RUN: {
+ WebContents* web_contents = GetActiveWebContents();
+ if (web_contents) {
+ extensions::ActiveScriptController::GetForWebContents(web_contents)
+ ->AlwaysRunOnVisibleHost(extension);
+ }
+ break;
+ }
case CONFIGURE:
DCHECK(!extensions::ManifestURL::GetOptionsPage(extension).is_empty());
extensions::ExtensionTabUtil::OpenOptionsPage(extension, browser_);
@@ -177,6 +186,18 @@ void ExtensionContextMenuModel::InitMenu(const Extension*
extension) {
base::ReplaceChars(extension_name, "&", "&&", &extension_name);
AddItem(NAME, base::UTF8ToUTF16(extension_name));
AddSeparator(ui::NORMAL_SEPARATOR);
+
+ // Add the "Always Allow" item for adding persisted permissions for script
+ // injections if there is an active action for this extension. Note that this
+ // will add it to *all* extension action context menus, not just the one
+ // attached to the script injection request icon, but that's okay.
+ WebContents* web_contents = GetActiveWebContents();
+ if (web_contents &&
+ extensions::ActiveScriptController::GetForWebContents(web_contents)
+ ->HasActiveScriptAction(extension)) {
+ AddItemWithStringId(ALWAYS_RUN, IDS_EXTENSIONS_ALWAYS_RUN);
+ }
+
AddItemWithStringId(CONFIGURE, IDS_EXTENSIONS_OPTIONS_MENU_ITEM);
AddItem(UNINSTALL, l10n_util::GetStringUTF16(IDS_EXTENSIONS_UNINSTALL));
if (extension_action_manager->GetBrowserAction(*extension))
@@ -190,3 +211,7 @@ const Extension* ExtensionContextMenuModel::GetExtension()
const {
extensions::ExtensionSystem::Get(profile_)->extension_service();
return extension_service->GetExtensionById(extension_id_, false);
}
+
+content::WebContents* ExtensionContextMenuModel::GetActiveWebContents() const {
+ return browser_->tab_strip_model()->GetActiveWebContents();
+}
On 2014/08/13 10:07:29, I haz the power (commit-bot) wrote:
> Failed to apply patch for
> chrome/browser/extensions/extension_context_menu_model.cc:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
> patching file chrome/browser/extensions/extension_context_menu_model.cc
> Hunk #1 FAILED at 6.
> Hunk #2 succeeded at 31 (offset 5 lines).
> Hunk #3 succeeded at 121 (offset 41 lines).
> Hunk #4 succeeded at 164 (offset 51 lines).
> Hunk #5 succeeded at 245 with fuzz 2 (offset 60 lines).
> Hunk #6 succeeded at 287 with fuzz 2 (offset 77 lines).
> 1 out of 6 hunks FAILED -- saving rejects to file
> chrome/browser/extensions/extension_context_menu_model.cc.rej
>
> Patch: chrome/browser/extensions/extension_context_menu_model.cc
> Index: chrome/browser/extensions/extension_context_menu_model.cc
> diff --git a/chrome/browser/extensions/extension_context_menu_model.cc
> b/chrome/browser/extensions/extension_context_menu_model.cc
> index
>
3334fd9ccc063a8409a9b146992717331460d3b8..9f38577fbd3ceb508d27c66edf4507395c562dbc
> 100644
> --- a/chrome/browser/extensions/extension_context_menu_model.cc
> +++ b/chrome/browser/extensions/extension_context_menu_model.cc
> @@ -6,6 +6,7 @@
>
> #include "base/prefs/pref_service.h"
> #include "base/strings/utf_string_conversions.h"
> +#include "chrome/browser/extensions/active_script_controller.h"
> #include
> "chrome/browser/extensions/api/extension_action/extension_action_api.h"
> #include "chrome/browser/extensions/extension_action.h"
> #include "chrome/browser/extensions/extension_action_manager.h"
> @@ -26,6 +27,7 @@
> #include "extensions/browser/management_policy.h"
> #include "extensions/browser/uninstall_reason.h"
> #include "extensions/common/extension.h"
> +#include "extensions/common/manifest_constants.h"
> #include "grit/chromium_strings.h"
> #include "grit/generated_resources.h"
> #include "ui/base/l10n/l10n_util.h"
> @@ -79,8 +81,7 @@ bool ExtensionContextMenuModel::IsCommandIdEnabled(int
> command_id) const {
> // homepage, we just disable this menu item.
> return extensions::ManifestURL::GetHomepageURL(extension).is_valid();
> } else if (command_id == INSPECT_POPUP) {
> - WebContents* web_contents =
> - browser_->tab_strip_model()->GetActiveWebContents();
> + WebContents* web_contents = GetActiveWebContents();
> if (!web_contents)
> return false;
>
> @@ -113,6 +114,14 @@ void ExtensionContextMenuModel::ExecuteCommand(int
> command_id,
> browser_->OpenURL(params);
> break;
> }
> + case ALWAYS_RUN: {
> + WebContents* web_contents = GetActiveWebContents();
> + if (web_contents) {
> + extensions::ActiveScriptController::GetForWebContents(web_contents)
> + ->AlwaysRunOnVisibleHost(extension);
> + }
> + break;
> + }
> case CONFIGURE:
> DCHECK(!extensions::ManifestURL::GetOptionsPage(extension).is_empty());
> extensions::ExtensionTabUtil::OpenOptionsPage(extension, browser_);
> @@ -177,6 +186,18 @@ void ExtensionContextMenuModel::InitMenu(const Extension*
> extension) {
> base::ReplaceChars(extension_name, "&", "&&", &extension_name);
> AddItem(NAME, base::UTF8ToUTF16(extension_name));
> AddSeparator(ui::NORMAL_SEPARATOR);
> +
> + // Add the "Always Allow" item for adding persisted permissions for script
> + // injections if there is an active action for this extension. Note that
this
> + // will add it to *all* extension action context menus, not just the one
> + // attached to the script injection request icon, but that's okay.
> + WebContents* web_contents = GetActiveWebContents();
> + if (web_contents &&
> + extensions::ActiveScriptController::GetForWebContents(web_contents)
> + ->HasActiveScriptAction(extension)) {
> + AddItemWithStringId(ALWAYS_RUN, IDS_EXTENSIONS_ALWAYS_RUN);
> + }
> +
> AddItemWithStringId(CONFIGURE, IDS_EXTENSIONS_OPTIONS_MENU_ITEM);
> AddItem(UNINSTALL, l10n_util::GetStringUTF16(IDS_EXTENSIONS_UNINSTALL));
> if (extension_action_manager->GetBrowserAction(*extension))
> @@ -190,3 +211,7 @@ const Extension* ExtensionContextMenuModel::GetExtension()
> const {
> extensions::ExtensionSystem::Get(profile_)->extension_service();
> return extension_service->GetExtensionById(extension_id_, false);
> }
> +
> +content::WebContents* ExtensionContextMenuModel::GetActiveWebContents() const
{
> + return browser_->tab_strip_model()->GetActiveWebContents();
> +}
I'd still like to take a quick look at this one (if only for my own
edification). :)
https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:111: extensions::URLPatternSet new_explicit_hosts; no extensions:: prefix. https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:126: new extensions::PermissionSet(extensions::APIPermissionSet(), no extensions:: https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:133: extensions::PermissionsUpdater updater(web_contents()->GetBrowserContext()); no extensions:: https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:134: updater.AddPermissions(extension, new_permissions.get()); One line: PermissionsUpdater(web_contents()->GetBrowserContext()).AddPermissions( extension, new_permissions.get()); https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.h (right): https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.h:61: // Returns whether there is an active script injection action for nit: Prefer "Returns true if there..." https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller_unittest.cc (right): https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:325: // Test that the "always run" extension action context menu item will create This is separate from the context menu, and shouldn't need to mention it. https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:367: PermissionsUpdater(profile()).InitializePermissions(extension); Better: unload and reload the extension. https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (left): https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:47: extension_prefs->GetActivePermissions(extension->id()); any particular reason for the change? https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:39: URLPatternSet FilterSingleOriginPermissions(const URLPatternSet& permissions) { Comment on the function https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:192: // For example, the union of <all_urls> and <anything> is <all_urls>, so we anything is bad. Use example.com or something. https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:196: granted_explicit_hosts.AddPatterns( You need to check here that the extension still wants these.
Greg, this failed to patch in, but that's a good thing because Devlin has some comments. However, more importantly, the patch doesn't appear to work. I take linkclump then say "Always run" when I'm on google.com, but: - refreshing the page asks to run again. - restarting chrome asks to run again. The only observable effect I can find is that clicking on the "Permissions" link on chrome://extensions shows the new permissions. And it persists across restarts. Which is awesome! So clearly something is going right. I'm not sure what's going wrong here. Clearly something is going *right*, and you have tests which pass, so generally speaking it looks like the runtime permissions aren't consistent with the ones that the Permissions dialogue reads. Which makes me suspect an issue somewhere in the stack that ActiveScriptController uses. But it's tested! So I'm confused. https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:44: if (iter->MatchesSingleOrigin()) I just realised (after chatting to mek@) that this isn't quite right. Say an extension requests *.google.com in its manifest, and we always-run on http://www.google.com. The active permissions is {*.google.com, http://www.google.com}. Then the extension drops *.google.com from its permissions. The GetBoundedActivePermissions logic correctly removes *.google.com from the active permission set, and www.google.com. That latter is somewhat by accident (and the problem that FilterSingleOriginPermissions etc fixes) *however* it introduces a different bug. www.google.com *should* have been removed. To fix this, we should only add back these permissions *if* they're granted by the bounded active permissions. For *that* we need in addition to MatchesSingleOrigin a method which extracts that origin. Shouldn't be too hard. So we'd have: if (iter->MatchesSingleOrigin() && bounded_active->Matches(iter->ToOrigin()) { // add back. } I'm not liking the name MatchesSingleOrigin anymore. IsOrigin might be better.
https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller_unittest.cc (right): https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:31: const char kAllHostsPermission[] = "*://*/*"; hm I'd actually like to check both this pattern and <all_urls> since I don't trust the <all_urls> logic. we can do that later, but it would involve parameterising all these tests with the Extension to test against, then using it twice, once with */* and again with <all_urls>. but: later.
On 2014/08/13 17:17:47, kalman wrote: > Greg, this failed to patch in, but that's a good thing because Devlin has some > comments. > > However, more importantly, the patch doesn't appear to work. I take linkclump > then say "Always run" when I'm on http://google.com, but: > - refreshing the page asks to run again. > - restarting chrome asks to run again. > > The only observable effect I can find is that clicking on the "Permissions" link > on chrome://extensions shows the new permissions. And it persists across > restarts. Which is awesome! So clearly something is going right. > > I'm not sure what's going wrong here. Clearly something is going *right*, and > you have tests which pass, so generally speaking it looks like the runtime > permissions aren't consistent with the ones that the Permissions dialogue reads. > Which makes me suspect an issue somewhere in the stack that > ActiveScriptController uses. But it's tested! So I'm confused. > > https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... > File chrome/browser/extensions/permissions_updater.cc (right): > > https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... > chrome/browser/extensions/permissions_updater.cc:44: if > (iter->MatchesSingleOrigin()) > I just realised (after chatting to mek@) that this isn't quite right. > > Say an extension requests *.google.com in its manifest, and we always-run on > http://www.google.com. The active permissions is {*.google.com, > http://www.google.com%7D. > > Then the extension drops *.google.com from its permissions. The > GetBoundedActivePermissions logic correctly removes *.google.com from the active > permission set, and http://www.google.com. That latter is somewhat by accident (and the > problem that FilterSingleOriginPermissions etc fixes) *however* it introduces a > different bug. http://www.google.com *should* have been removed. > > To fix this, we should only add back these permissions *if* they're granted by > the bounded active permissions. For *that* we need in addition to > MatchesSingleOrigin a method which extracts that origin. Shouldn't be too hard. > So we'd have: > > if (iter->MatchesSingleOrigin() && > bounded_active->Matches(iter->ToOrigin()) { > // add back. > } > > I'm not liking the name MatchesSingleOrigin anymore. IsOrigin might be better. I believe it failed because it needed a rebase after the contextMenus patch went through. I also figured out why it wasn't working. In the test, I navigated to https://www.google.com. The permission that ended up getting added to the active scriptable hosts was https://www.google.com/, so this worked. However, navigating to google.com redirects you to https://www.google.com/?gws_rd=ssl. The path wildcard was missing from the permission, so it was requesting permission, but the test was passing because it still matched because it didn't have a path. I made some changes to fix this; AddOrigin needed to append the path wildcard. Let me know what you think.
https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:111: extensions::URLPatternSet new_explicit_hosts; On 2014/08/13 16:55:03, Devlin wrote: > no extensions:: prefix. Done. https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:126: new extensions::PermissionSet(extensions::APIPermissionSet(), On 2014/08/13 16:55:03, Devlin wrote: > no extensions:: Done. https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:133: extensions::PermissionsUpdater updater(web_contents()->GetBrowserContext()); On 2014/08/13 16:55:03, Devlin wrote: > no extensions:: Done. https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:134: updater.AddPermissions(extension, new_permissions.get()); On 2014/08/13 16:55:03, Devlin wrote: > One line: > PermissionsUpdater(web_contents()->GetBrowserContext()).AddPermissions( > extension, new_permissions.get()); Done. https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.h (right): https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.h:61: // Returns whether there is an active script injection action for On 2014/08/13 16:55:03, Devlin wrote: > nit: Prefer "Returns true if there..." Done. https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller_unittest.cc (right): https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:31: const char kAllHostsPermission[] = "*://*/*"; On 2014/08/13 17:21:22, kalman wrote: > hm I'd actually like to check both this pattern and <all_urls> since I don't > trust the <all_urls> logic. we can do that later, but it would involve > parameterising all these tests with the Extension to test against, then using it > twice, once with */* and again with <all_urls>. > > but: later. Acknowledged. https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:325: // Test that the "always run" extension action context menu item will create On 2014/08/13 16:55:03, Devlin wrote: > This is separate from the context menu, and shouldn't need to mention it. Done. https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:367: PermissionsUpdater(profile()).InitializePermissions(extension); On 2014/08/13 16:55:04, Devlin wrote: > Better: unload and reload the extension. We don't seem to have an extension service for this unittest. Do you want me to integrate it so I can unload and reload the extension? https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (left): https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:47: extension_prefs->GetActivePermissions(extension->id()); On 2014/08/13 16:55:04, Devlin wrote: > any particular reason for the change? Kalman suggested we pass active permissions in instead of prefs because we need the active permissions in InitializePermissions in the code that was added. https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:39: URLPatternSet FilterSingleOriginPermissions(const URLPatternSet& permissions) { On 2014/08/13 16:55:04, Devlin wrote: > Comment on the function Done. https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:44: if (iter->MatchesSingleOrigin()) On 2014/08/13 17:17:47, kalman wrote: > I just realised (after chatting to mek@) that this isn't quite right. > > Say an extension requests *.google.com in its manifest, and we always-run on > http://www.google.com. The active permissions is {*.google.com, > http://www.google.com%7D. > > Then the extension drops *.google.com from its permissions. The > GetBoundedActivePermissions logic correctly removes *.google.com from the active > permission set, and http://www.google.com. That latter is somewhat by accident (and the > problem that FilterSingleOriginPermissions etc fixes) *however* it introduces a > different bug. http://www.google.com *should* have been removed. > > To fix this, we should only add back these permissions *if* they're granted by > the bounded active permissions. For *that* we need in addition to > MatchesSingleOrigin a method which extracts that origin. Shouldn't be too hard. > So we'd have: > > if (iter->MatchesSingleOrigin() && > bounded_active->Matches(iter->ToOrigin()) { > // add back. > } > > I'm not liking the name MatchesSingleOrigin anymore. IsOrigin might be better. Alright, this makes sense to me. Can you think of a good way to test this case in the unittest? (I figure maybe you can point me in the right direction, but I'll see what I can come up with as well). https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:192: // For example, the union of <all_urls> and <anything> is <all_urls>, so we On 2014/08/13 16:55:04, Devlin wrote: > anything is bad. Use http://example.com or something. Done. https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:196: granted_explicit_hosts.AddPatterns( On 2014/08/13 16:55:04, Devlin wrote: > You need to check here that the extension still wants these. Is this what kalman was suggesting in the above comment on line 44? Checking the manifest permissions to see if these active permissions are still relevant?
https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller_unittest.cc (right): https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:367: PermissionsUpdater(profile()).InitializePermissions(extension); On 2014/08/13 22:18:36, gpdavis wrote: > On 2014/08/13 16:55:04, Devlin wrote: > > Better: unload and reload the extension. > > We don't seem to have an extension service for this unittest. Do you want me to > integrate it so I can unload and reload the extension? Yeah, testing this would require introducing a dependency on an ExtensionService. In theory you can get that from https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/c... with ExtensionSystem::Get(profile())->extension_service() but this test would need to be more dramatically restructured to do real installation rather than just adding to the extension registry. Exercising PermissionsUpdater::InitializePermissions is a little fragile since it looks like PermissionsUpdater isn't really meant to be used in isolation like that, but that *is* the interface that we're presented with. We should also be testing extension re-install/re-enable/reload logic for the whole of ActiveScriptController, not just this feature. https://codereview.chromium.org/396033002/diff/410001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/396033002/diff/410001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:47: const URLPatternSet& active, |active| vs |bounded_active| are implementation details of the caller. Perhaps |permissions| and |bounds|? https://codereview.chromium.org/396033002/diff/410001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:53: if (iter->IsOrigin() && bounded_active.MatchesURL(iter->ToOrigin())) Bleh ToOrigin is kind of pointless actually, since it's implementation would basically be GURL(GetAsString()).GetOrigin(). And like I said elsewhere, IsOrigin() is actually wrong. So I think this condition is better as: if (iter->MatchesSingleOrigin() && bounds.MatchesURL(GURL(iter->GetAsString())) { ... } https://codereview.chromium.org/396033002/diff/410001/extensions/common/url_p... File extensions/common/url_pattern.cc (right): https://codereview.chromium.org/396033002/diff/410001/extensions/common/url_p... extensions/common/url_pattern.cc:470: // We ignore the port because it's not all that interesting. Actually a better explanation would be // Strictly speaking the port is part of the origin, but in URLPattern it defaults to *. It's not very interesting anyway, so leave it out. https://codereview.chromium.org/396033002/diff/410001/extensions/common/url_p... File extensions/common/url_pattern.h (right): https://codereview.chromium.org/396033002/diff/410001/extensions/common/url_p... extensions/common/url_pattern.h:166: bool IsOrigin() const; I'm silly. IsOrigin() is wrong, MatchesSingleOrigin() is right. You should improve the comment here to include "The pattern may include a path."
https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller_unittest.cc (right): https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:367: PermissionsUpdater(profile()).InitializePermissions(extension); On 2014/08/13 22:59:17, kalman wrote: > On 2014/08/13 22:18:36, gpdavis wrote: > > On 2014/08/13 16:55:04, Devlin wrote: > > > Better: unload and reload the extension. > > > > We don't seem to have an extension service for this unittest. Do you want me > to > > integrate it so I can unload and reload the extension? > > Yeah, testing this would require introducing a dependency on an > ExtensionService. In theory you can get that from > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/c... > with ExtensionSystem::Get(profile())->extension_service() > > but this test would need to be more dramatically restructured to do real > installation rather than just adding to the extension registry. Exercising > PermissionsUpdater::InitializePermissions is a little fragile since it looks > like PermissionsUpdater isn't really meant to be used in isolation like that, > but that *is* the interface that we're presented with. > > We should also be testing extension re-install/re-enable/reload logic for the > whole of ActiveScriptController, not just this feature. So... perhaps a later CL? Or what would you like to do for this one? https://codereview.chromium.org/396033002/diff/410001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/396033002/diff/410001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:47: const URLPatternSet& active, On 2014/08/13 22:59:17, kalman wrote: > |active| vs |bounded_active| are implementation details of the caller. Perhaps > |permissions| and |bounds|? Can I leave the comment the same, save for swapping the param names? Or should I move some of that over to where it's called? https://codereview.chromium.org/396033002/diff/410001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:53: if (iter->IsOrigin() && bounded_active.MatchesURL(iter->ToOrigin())) On 2014/08/13 22:59:17, kalman wrote: > Bleh ToOrigin is kind of pointless actually, since it's implementation would > basically be GURL(GetAsString()).GetOrigin(). > > And like I said elsewhere, IsOrigin() is actually wrong. > > So I think this condition is better as: > > if (iter->MatchesSingleOrigin() && > bounds.MatchesURL(GURL(iter->GetAsString())) { > ... > } Done. https://codereview.chromium.org/396033002/diff/410001/extensions/common/url_p... File extensions/common/url_pattern.cc (right): https://codereview.chromium.org/396033002/diff/410001/extensions/common/url_p... extensions/common/url_pattern.cc:470: // We ignore the port because it's not all that interesting. On 2014/08/13 22:59:17, kalman wrote: > Actually a better explanation would be > > // Strictly speaking the port is part of the origin, but in URLPattern it > defaults to *. It's not very interesting anyway, so leave it out. Done. https://codereview.chromium.org/396033002/diff/410001/extensions/common/url_p... File extensions/common/url_pattern.h (right): https://codereview.chromium.org/396033002/diff/410001/extensions/common/url_p... extensions/common/url_pattern.h:166: bool IsOrigin() const; On 2014/08/13 22:59:17, kalman wrote: > I'm silly. IsOrigin() is wrong, MatchesSingleOrigin() is right. You should > improve the comment here to include "The pattern may include a path." Done.
https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller_unittest.cc (right): https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:367: PermissionsUpdater(profile()).InitializePermissions(extension); On 2014/08/13 23:23:06, gpdavis wrote: > On 2014/08/13 22:59:17, kalman wrote: > > On 2014/08/13 22:18:36, gpdavis wrote: > > > On 2014/08/13 16:55:04, Devlin wrote: > > > > Better: unload and reload the extension. > > > > > > We don't seem to have an extension service for this unittest. Do you want > me > > to > > > integrate it so I can unload and reload the extension? > > > > Yeah, testing this would require introducing a dependency on an > > ExtensionService. In theory you can get that from > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/c... > > with ExtensionSystem::Get(profile())->extension_service() > > > > but this test would need to be more dramatically restructured to do real > > installation rather than just adding to the extension registry. Exercising > > PermissionsUpdater::InitializePermissions is a little fragile since it looks > > like PermissionsUpdater isn't really meant to be used in isolation like that, > > but that *is* the interface that we're presented with. > > > > We should also be testing extension re-install/re-enable/reload logic for the > > whole of ActiveScriptController, not just this feature. > > So... perhaps a later CL? Or what would you like to do for this one? Here's a good approximation we can implement without the full ExtensionService stack: const Extension* ReloadExtension(const std::string& id) { registry->RemoveEnabled(id); // need an alternative version of AddExtension which takes an ID return AddExtension(id); } the advantage of that is it does re-create the Extension object and clear away its permissions.
https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller_unittest.cc (right): https://codereview.chromium.org/396033002/diff/390001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:367: PermissionsUpdater(profile()).InitializePermissions(extension); On 2014/08/13 23:32:47, kalman wrote: > On 2014/08/13 23:23:06, gpdavis wrote: > > On 2014/08/13 22:59:17, kalman wrote: > > > On 2014/08/13 22:18:36, gpdavis wrote: > > > > On 2014/08/13 16:55:04, Devlin wrote: > > > > > Better: unload and reload the extension. > > > > > > > > We don't seem to have an extension service for this unittest. Do you want > > me > > > to > > > > integrate it so I can unload and reload the extension? > > > > > > Yeah, testing this would require introducing a dependency on an > > > ExtensionService. In theory you can get that from > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/c... > > > with ExtensionSystem::Get(profile())->extension_service() > > > > > > but this test would need to be more dramatically restructured to do real > > > installation rather than just adding to the extension registry. Exercising > > > PermissionsUpdater::InitializePermissions is a little fragile since it looks > > > like PermissionsUpdater isn't really meant to be used in isolation like > that, > > > but that *is* the interface that we're presented with. > > > > > > We should also be testing extension re-install/re-enable/reload logic for > the > > > whole of ActiveScriptController, not just this feature. > > > > So... perhaps a later CL? Or what would you like to do for this one? > > Here's a good approximation we can implement without the full ExtensionService > stack: > > const Extension* ReloadExtension(const std::string& id) { > registry->RemoveEnabled(id); > // need an alternative version of AddExtension which takes an ID > return AddExtension(id); > } > > the advantage of that is it does re-create the Extension object and clear away > its permissions. Alright, that works.
https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:32: #include "extensions/common/manifest_handlers/permissions_parser.h" Why? https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:110: GURL url = web_contents()->GetVisibleURL(); const & https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:126: new PermissionSet(extensions::APIPermissionSet(), no extensions:: https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:127: extensions::ManifestPermissionSet(), no extensions:: https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller_unittest.cc (right): https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:50: const Extension* AddExtension(const std::string id); const & https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:54: const Extension* ReloadExtension(const std::string id); const & https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:101: return AddExtension(id_util::GenerateId("all_hosts_extension")); All of these ids will always be the same, so why do we have two methods? https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:105: const std::string id) { const & https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:126: const std::string id) { const & https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:361: // Simulate clicking "always run" menu item. Abstract away "clicking the menu item". https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:378: NavigateAndCommit(GURL("https://www.google.com/?gws_rd=ssl")); We should check on returning to a different site with the same origin. https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:35: #include "extensions/common/manifest_constants.h" why?? https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:64: const PermissionSet* active_permissions) { might as well make this take a const refptr& https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:202: // For example, the union of <all_urls> and <"example.com"> is <all_urls>, so drop the brackets around "example.com"
Hey Greg, I think you can save yourself a lot of time here by double-checking what you upload (think git-diff, git-difftool, just searching for certain suspicious patterns). Every review round trip costs time. Devlin's doing a great job pointing out things like removing the extensions:: namespace, const&-ing string parameters, and unnecessary includes, but a more effective way to spend time would be reviewing this CL holistically. Small style nits can distract from that. Cheers, Ben.
On 2014/08/14 14:18:59, kalman wrote: > Hey Greg, > > I think you can save yourself a lot of time here by double-checking what you > upload (think git-diff, git-difftool, just searching for certain suspicious > patterns). Every review round trip costs time. > > Devlin's doing a great job pointing out things like removing the extensions:: > namespace, const&-ing string parameters, and unnecessary includes, but a more > effective way to spend time would be reviewing this CL holistically. Small style > nits can distract from that. > > Cheers, > Ben. Understood. I was mostly looking for a review of the changes we just discussed in unit testing. I'm more than happy to comb through and fix minor things like this (admittedly, I don't know how I missed all those extensions:: namespaces-- I think 6pm takes its toll on attention to detail). I guess what I imagine is confirmation that the design is all correct, and then the nits can be addressed. But I should have mentioned that that was the primary feedback I was looking for. I'll spend some extra time checking for these things. But to save both of us a lot of time, maybe we could iterate the reviews such that the higher level stuff is examined first (and it often is, but sometimes it goes back and forth)? I don't mean to sound lazy (because I do check over patch sets before uploading), but I think I miss a lot fewer of these things once I'm comfortable that the design approach is correct, and the code is ready for a full extra-meticulous cleanup. I hate to waste both of your time on little things that I should have caught myself, and I think this would help me fix them beforehand. In any case, like I said, I'll make more of an effort to find these things in every patch set, and I'll try to point out when things are unpolished and I'm looking for more general design confirmation. Do either of you have any concerns with this? I also don't mean to criticize your review process-- I just want to avoid the excessive patch sets as well, and this might make it easier to consolidate the things I miss. What do you guys think?
> I guess what I imagine is > confirmation that the design is all correct, and then the nits can be addressed. This is harder than it sounds. Nits are like dead pixels. You can try to enjoy a movie, but really that's all you end up thinking about. > But I should have mentioned that that was the primary feedback I was looking > for. Understood, and I see where you're coming from. Perhaps I should have used weaker language than "nit" -- which are stylistic preferences by the reviewer -- and concentrated on the uncontroversial errors that can creep in. It's much easier for you to inspect your own code before you upload it than it is for us. > > I'll spend some extra time checking for these things. But to save both of us a > lot of time, maybe we could iterate the reviews such that the higher level stuff > is examined first (My first comment applies to this as well) > In any case, like I said, I'll make more of an effort to find these things in > every patch set Thanks Greg, much appreciated.
On 2014/08/14 17:59:21, kalman wrote: > > I guess what I imagine is > > confirmation that the design is all correct, and then the nits can be > addressed. > > This is harder than it sounds. Nits are like dead pixels. You can try to enjoy a > movie, but really that's all you end up thinking about. Ah, this is what I was worried about, but it's completely understood. > > But I should have mentioned that that was the primary feedback I was looking > > for. > > Understood, and I see where you're coming from. Perhaps I should have used > weaker language than "nit" -- which are stylistic preferences by the reviewer -- > and concentrated on the uncontroversial errors that can creep in. It's much > easier for you to inspect your own code before you upload it than it is for us. Absolutely, and I certainly don't expect you to make up for me not being thorough enough in my pre-upload inspections. I see what you're saying about uncontroversial errors. > > In any case, like I said, I'll make more of an effort to find these things in > > every patch set > > Thanks Greg, much appreciated. Not a problem at all. Thanks for the pointer to git-difftool; it occurred to me that it'd be nice to have a side-by-side before uploading a patch, and that's what I was looking for.
On 2014/08/14 18:10:10, gpdavis wrote: > On 2014/08/14 17:59:21, kalman wrote: > > > I guess what I imagine is > > > confirmation that the design is all correct, and then the nits can be > > addressed. > > > > This is harder than it sounds. Nits are like dead pixels. You can try to enjoy > a > > movie, but really that's all you end up thinking about. > > Ah, this is what I was worried about, but it's completely understood. > > > > > But I should have mentioned that that was the primary feedback I was looking > > > for. > > > > Understood, and I see where you're coming from. Perhaps I should have used > > weaker language than "nit" -- which are stylistic preferences by the reviewer > -- > > and concentrated on the uncontroversial errors that can creep in. It's much > > easier for you to inspect your own code before you upload it than it is for > us. > > Absolutely, and I certainly don't expect you to make up for me not being > thorough enough in my pre-upload inspections. I see what you're saying about > uncontroversial errors. > > > > > In any case, like I said, I'll make more of an effort to find these things > in > > > every patch set > > > > Thanks Greg, much appreciated. > > Not a problem at all. Thanks for the pointer to git-difftool; it occurred to me > that it'd be nice to have a side-by-side before uploading a patch, and that's > what I was looking for. git difftool is great (my personal choice is meld, but obviously up to you). Another thing is that you don't have to worry about uploading in-progress or unpolished patches at all. We generally only review when you ping us, so if it's not ready, don't worry (I post unfinished patches all the time - for whatever reason, I find Reitveld's diff nicer than most). If there's any worry, you can also post it with a patch set title of IN PROGRESS - DO NOT REVIEW or something. Just FYI. Use whatever process you're most comfortable with. :)
On 2014/08/14 18:43:41, Devlin wrote: > On 2014/08/14 18:10:10, gpdavis wrote: > > On 2014/08/14 17:59:21, kalman wrote: > > > > I guess what I imagine is > > > > confirmation that the design is all correct, and then the nits can be > > > addressed. > > > > > > This is harder than it sounds. Nits are like dead pixels. You can try to > enjoy > > a > > > movie, but really that's all you end up thinking about. > > > > Ah, this is what I was worried about, but it's completely understood. > > > > > > > > But I should have mentioned that that was the primary feedback I was > looking > > > > for. > > > > > > Understood, and I see where you're coming from. Perhaps I should have used > > > weaker language than "nit" -- which are stylistic preferences by the > reviewer > > -- > > > and concentrated on the uncontroversial errors that can creep in. It's much > > > easier for you to inspect your own code before you upload it than it is for > > us. > > > > Absolutely, and I certainly don't expect you to make up for me not being > > thorough enough in my pre-upload inspections. I see what you're saying about > > uncontroversial errors. > > > > > > > > In any case, like I said, I'll make more of an effort to find these things > > in > > > > every patch set > > > > > > Thanks Greg, much appreciated. > > > > Not a problem at all. Thanks for the pointer to git-difftool; it occurred to > me > > that it'd be nice to have a side-by-side before uploading a patch, and that's > > what I was looking for. > > git difftool is great (my personal choice is meld, but obviously up to you). > Another thing is that you don't have to worry about uploading in-progress or > unpolished patches at all. We generally only review when you ping us, so if > it's not ready, don't worry (I post unfinished patches all the time - for > whatever reason, I find Reitveld's diff nicer than most). If there's any worry, > you can also post it with a patch set title of IN PROGRESS - DO NOT REVIEW or > something. > > Just FYI. Use whatever process you're most comfortable with. :) Right on. Thanks for the tip (: I think I like vimtool. Just gotta configure it so it's not a mess of colors. Apologies for the messy patchsets.
https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:32: #include "extensions/common/manifest_handlers/permissions_parser.h" On 2014/08/14 01:20:18, Devlin wrote: > Why? Oops; leftover from patchset 13. Done. https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:110: GURL url = web_contents()->GetVisibleURL(); On 2014/08/14 01:20:18, Devlin wrote: > const & Done. https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:126: new PermissionSet(extensions::APIPermissionSet(), On 2014/08/14 01:20:18, Devlin wrote: > no extensions:: Done. https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:127: extensions::ManifestPermissionSet(), On 2014/08/14 01:20:18, Devlin wrote: > no extensions:: Done. https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller_unittest.cc (right): https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:101: return AddExtension(id_util::GenerateId("all_hosts_extension")); On 2014/08/14 01:20:18, Devlin wrote: > All of these ids will always be the same, so why do we have two methods? Kalman suggested making an AddExtension method that takes an ID for reloading an extension (by removing it and adding a new one with the same ID). I didn't realize GenerateId always generated the same ID for the same input string, but using it to reload an extension without passing an ID seems kinda like a hack to me (since it only works because the ID generated is always the same). Thoughts? We could definitely simplify this by removing the overloaded AddExtension method, and making ReloadExtension simply remove the extension and return AddExtension(). https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:126: const std::string id) { On 2014/08/14 01:20:18, Devlin wrote: > const & This occurred to me, but since ReloadExtension removes the extension, then adds a new one with the same ID, this can't be a reference. I tested it, and it indeed causes the test to crash. AddExtension can take a reference, though, so I changed that one. https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:361: // Simulate clicking "always run" menu item. On 2014/08/14 01:20:18, Devlin wrote: > Abstract away "clicking the menu item". Done. https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:378: NavigateAndCommit(GURL("https://www.google.com/?gws_rd=ssl")); On 2014/08/14 01:20:18, Devlin wrote: > We should check on returning to a different site with the same origin. Good idea. Done. https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:35: #include "extensions/common/manifest_constants.h" On 2014/08/14 01:20:18, Devlin wrote: > why?? Oops, thought we still needed this for determining whether to add "Always Run", but we changed the way that works. I don't want to give you the impression that I'm not checking over added includes (clearly I'm just not being thorough enough). Done. https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:64: const PermissionSet* active_permissions) { On 2014/08/14 01:20:19, Devlin wrote: > might as well make this take a const refptr& Done. https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:202: // For example, the union of <all_urls> and <"example.com"> is <all_urls>, so On 2014/08/14 01:20:19, Devlin wrote: > drop the brackets around "example.com" Done.
https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller_unittest.cc (right): https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:101: return AddExtension(id_util::GenerateId("all_hosts_extension")); On 2014/08/14 20:05:43, gpdavis wrote: > On 2014/08/14 01:20:18, Devlin wrote: > > All of these ids will always be the same, so why do we have two methods? > > Kalman suggested making an AddExtension method that takes an ID for reloading an > extension (by removing it and adding a new one with the same ID). I didn't > realize GenerateId always generated the same ID for the same input string, but > using it to reload an extension without passing an ID seems kinda like a hack to > me (since it only works because the ID generated is always the same). > > Thoughts? We could definitely simplify this by removing the overloaded > AddExtension method, and making ReloadExtension simply remove the extension and > return AddExtension(). I prefer it being explicit. One version generates an ID (even though it doesn't), the other makes it explicit. https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:126: const std::string id) { On 2014/08/14 20:05:43, gpdavis wrote: > On 2014/08/14 01:20:18, Devlin wrote: > > const & > > This occurred to me, but since ReloadExtension removes the extension, then adds > a new one with the same ID, this can't be a reference. I tested it, and it > indeed causes the test to crash. AddExtension can take a reference, though, so > I changed that one. Good point, though the solution here is for the caller to fix that problem, not this function. I.e. do make this a const& but store a copy of the ID before removing the extension.
https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller_unittest.cc (right): https://codereview.chromium.org/396033002/diff/450001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:126: const std::string id) { On 2014/08/14 20:08:07, kalman wrote: > On 2014/08/14 20:05:43, gpdavis wrote: > > On 2014/08/14 01:20:18, Devlin wrote: > > > const & > > > > This occurred to me, but since ReloadExtension removes the extension, then > adds > > a new one with the same ID, this can't be a reference. I tested it, and it > > indeed causes the test to crash. AddExtension can take a reference, though, > so > > I changed that one. > > Good point, though the solution here is for the caller to fix that problem, not > this function. I.e. do make this a const& but store a copy of the ID before > removing the extension. Sweet, I like this idea.
https://codereview.chromium.org/396033002/diff/510001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller_unittest.cc (right): https://codereview.chromium.org/396033002/diff/510001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:130: std::string extension_id(id); nit: prefer " = id" here. https://codereview.chromium.org/396033002/diff/510001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:392: EXPECT_FALSE(RequiresUserConsent(extension)); Also test navigating to a site that wasn't always-allowed and check that it hasn't been allowed. https://codereview.chromium.org/396033002/diff/510001/extensions/common/url_p... File extensions/common/url_pattern_set.cc (right): https://codereview.chromium.org/396033002/diff/510001/extensions/common/url_p... extensions/common/url_pattern_set.cc:153: origin_pattern.SetPath("/*"); Just "*" should do it? You need to add tests for this function.
https://codereview.chromium.org/396033002/diff/510001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller_unittest.cc (right): https://codereview.chromium.org/396033002/diff/510001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:130: std::string extension_id(id); On 2014/08/14 21:03:55, kalman wrote: > nit: prefer " = id" here. Done. https://codereview.chromium.org/396033002/diff/510001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:392: EXPECT_FALSE(RequiresUserConsent(extension)); On 2014/08/14 21:03:55, kalman wrote: > Also test navigating to a site that wasn't always-allowed and check that it > hasn't been allowed. Done. https://codereview.chromium.org/396033002/diff/510001/extensions/common/url_p... File extensions/common/url_pattern_set.cc (right): https://codereview.chromium.org/396033002/diff/510001/extensions/common/url_p... extensions/common/url_pattern_set.cc:153: origin_pattern.SetPath("/*"); On 2014/08/14 21:03:55, kalman wrote: > Just "*" should do it? > > You need to add tests for this function. That's what I thought as well, but without the slash, the browser crashes when "always run" is clicked. Adding tests... Is there a reason we pass in |valid_schemes| here? Can't this method just use URLPattern::SCHEME_ALL by default? Will/should passing in something different affect the pattern that's added at all? It's already intended to match only the scheme of the origin. In fact, it seems like the scheme should be extracted from |origin|.
https://codereview.chromium.org/396033002/diff/510001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/510001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:124: scoped_refptr<extensions::PermissionSet> new_permissions = extensions:: https://codereview.chromium.org/396033002/diff/510001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller_unittest.cc (right): https://codereview.chromium.org/396033002/diff/510001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:45: // Creates an extension with a generated id and adds it to the registry. // Creates an extension all-hosts permission (and optionally with |id|), // and adds it to the registry. const Extension* AddExtension(); const Extension* AddExtension(const std::string& id); Or: Just make a scoped_refptr<const Extension> extension_ member field, and have AddExtension() and ReloadExtension(). https://codereview.chromium.org/396033002/diff/510001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:63: // Simulate clicking "always run" menu item for |extension|. My issue here is with the comment. ActiveScriptController does not (and should not) necessarily know that this has to happen via a click. What if developerPrivate has an "alwaysRun()" function? What if we use this elsewhere for component or trusted extensions in the future? This should simply say something like "Allow the extension to always run on this host." https://codereview.chromium.org/396033002/diff/510001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:152: void ActiveScriptControllerUnitTest::AlwaysRun(const Extension* extension) { May as well just type this out in the one test that uses it.
https://codereview.chromium.org/396033002/diff/510001/extensions/common/url_p... File extensions/common/url_pattern_set.cc (right): https://codereview.chromium.org/396033002/diff/510001/extensions/common/url_p... extensions/common/url_pattern_set.cc:153: origin_pattern.SetPath("/*"); On 2014/08/14 21:59:53, gpdavis wrote: > On 2014/08/14 21:03:55, kalman wrote: > > Just "*" should do it? > > > > You need to add tests for this function. > > That's what I thought as well, but without the slash, the browser crashes when > "always run" is clicked. > > Adding tests... > > Is there a reason we pass in |valid_schemes| here? Can't this method just use > URLPattern::SCHEME_ALL by default? Will/should passing in something different > affect the pattern that's added at all? It's already intended to match only the > scheme of the origin. > > In fact, it seems like the scheme should be extracted from |origin|. Good question. The scheme would reject origins like chrome://foo and chrome-extension://asdf, as just a low-level layer of sanity checking that we don't accidentally give extensions access to those URLs.
lgtm at this point. https://codereview.chromium.org/396033002/diff/530001/extensions/common/url_p... File extensions/common/url_pattern_set_unittest.cc (right): https://codereview.chromium.org/396033002/diff/530001/extensions/common/url_p... extensions/common/url_pattern_set_unittest.cc:426: set.AddOrigin(URLPattern::SCHEME_ALL, GURL("https://www.google.com/")); Huh, I would have thought ToOrigin of google.com/ would be google.com, but it's not failing the DCHECK in there. Ah well. https://codereview.chromium.org/396033002/diff/530001/extensions/common/url_p... extensions/common/url_pattern_set_unittest.cc:432: set.AddOrigin(URLPattern::SCHEME_ALL, GURL("http://en.foo.com/")); there's no difference between this and the google.com - they both have exactly the same structure - so it's not really an interesting check. what *would* be interesting is testing a URL without a subdomain at least.
Wanna look over these last couple of changes real quick? Then we should be good to go (for reals this time) https://codereview.chromium.org/396033002/diff/510001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/510001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:124: scoped_refptr<extensions::PermissionSet> new_permissions = On 2014/08/14 22:01:11, Devlin wrote: > extensions:: Dang, why am I having trouble catching these? I checked this file over a couple times. I'll start grepping for it. https://codereview.chromium.org/396033002/diff/510001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller_unittest.cc (right): https://codereview.chromium.org/396033002/diff/510001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:45: // Creates an extension with a generated id and adds it to the registry. On 2014/08/14 22:01:11, Devlin wrote: > // Creates an extension all-hosts permission (and optionally with |id|), > // and adds it to the registry. > const Extension* AddExtension(); > const Extension* AddExtension(const std::string& id); > > Or: > Just make a scoped_refptr<const Extension> extension_ member field, and have > AddExtension() and ReloadExtension(). I like the alternative better since we never need more than one extension at a time. Done. https://codereview.chromium.org/396033002/diff/510001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:63: // Simulate clicking "always run" menu item for |extension|. On 2014/08/14 22:01:11, Devlin wrote: > My issue here is with the comment. ActiveScriptController does not (and should > not) necessarily know that this has to happen via a click. What if > developerPrivate has an "alwaysRun()" function? What if we use this elsewhere > for component or trusted extensions in the future? This should simply say > something like "Allow the extension to always run on this host." Oh, I see-- I was confused by the wording because you said to abstract it away. Done. https://codereview.chromium.org/396033002/diff/510001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:152: void ActiveScriptControllerUnitTest::AlwaysRun(const Extension* extension) { On 2014/08/14 22:01:11, Devlin wrote: > May as well just type this out in the one test that uses it. Done. https://codereview.chromium.org/396033002/diff/510001/extensions/common/url_p... File extensions/common/url_pattern_set.cc (right): https://codereview.chromium.org/396033002/diff/510001/extensions/common/url_p... extensions/common/url_pattern_set.cc:153: origin_pattern.SetPath("/*"); On 2014/08/14 22:05:33, kalman wrote: > On 2014/08/14 21:59:53, gpdavis wrote: > > On 2014/08/14 21:03:55, kalman wrote: > > > Just "*" should do it? > > > > > > You need to add tests for this function. > > > > That's what I thought as well, but without the slash, the browser crashes when > > "always run" is clicked. > > > > Adding tests... > > > > Is there a reason we pass in |valid_schemes| here? Can't this method just use > > URLPattern::SCHEME_ALL by default? Will/should passing in something different > > affect the pattern that's added at all? It's already intended to match only > the > > scheme of the origin. > > > > In fact, it seems like the scheme should be extracted from |origin|. > > Good question. The scheme would reject origins like chrome://foo and > chrome-extension://asdf, as just a low-level layer of sanity checking that we > don't accidentally give extensions access to those URLs. Ah, okay. I just realized we're passing in ValidUserScriptSchemes, not SCHEME_ALL. So this is fine as is? https://codereview.chromium.org/396033002/diff/530001/extensions/common/url_p... File extensions/common/url_pattern_set_unittest.cc (right): https://codereview.chromium.org/396033002/diff/530001/extensions/common/url_p... extensions/common/url_pattern_set_unittest.cc:432: set.AddOrigin(URLPattern::SCHEME_ALL, GURL("http://en.foo.com/")); On 2014/08/14 23:04:48, kalman wrote: > there's no difference between this and the http://google.com - they both have exactly > the same structure - so it's not really an interesting check. > > what *would* be interesting is testing a URL without a subdomain at least. Sure, I can do that.
lgtm after these fixes. https://codereview.chromium.org/396033002/diff/550001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/550001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:133: .AddPermissions(extension, new_permissions.get()); I realize that there's a potential problem in here. If the host was explicitly in withheld permissions, we don't remove it from withheld. Right now, it doesn't really matter (because we only withhold all-hosts), but it should be fixed at some point. Of course, this is an existing problem, and doesn't need to be fixed in this CL. So for the time being could you just add a // TODO(devlin): Make sure that the permission is removed from // withheld_permissions if appropriate. either here or in PermissionsUpdater::AddPermissions()? https://codereview.chromium.org/396033002/diff/550001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller_unittest.cc (right): https://codereview.chromium.org/396033002/diff/550001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:45: // Creates an extension with all host permission and adds it to the registry. no need to remove the 's' ("all hosts" is the name of the permission) https://codereview.chromium.org/396033002/diff/550001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:372: // Reloading the extension should clear active permissions, but not persisted this is a bit confusing since all we do is actually add the permission to "active permissions". Maybe just say "Reloading the extension should not clear any granted host permissions." https://codereview.chromium.org/396033002/diff/550001/extensions/common/url_p... File extensions/common/url_pattern_set_unittest.cc (right): https://codereview.chromium.org/396033002/diff/550001/extensions/common/url_p... extensions/common/url_pattern_set_unittest.cc:426: set.AddOrigin(URLPattern::SCHEME_ALL, GURL("https://www.google.com/")); Should probably say EXPECT_TRUE() https://codereview.chromium.org/396033002/diff/550001/extensions/common/url_p... extensions/common/url_pattern_set_unittest.cc:432: set.AddOrigin(URLPattern::SCHEME_ALL, GURL("https://google.com/")); Should probably also say EXPECT_TRUE() https://codereview.chromium.org/396033002/diff/550001/extensions/common/url_p... extensions/common/url_pattern_set_unittest.cc:434: EXPECT_TRUE(set.MatchesURL(GURL("https://google.com/foo/bar"))); For kicks and grins, can we also test an AddOrigin() that fails? (Just fails to add, not DCHECKs)
https://codereview.chromium.org/396033002/diff/550001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/396033002/diff/550001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:133: .AddPermissions(extension, new_permissions.get()); On 2014/08/15 15:10:31, Devlin wrote: > I realize that there's a potential problem in here. If the host was explicitly > in withheld permissions, we don't remove it from withheld. Right now, it > doesn't really matter (because we only withhold all-hosts), but it should be > fixed at some point. > > Of course, this is an existing problem, and doesn't need to be fixed in this CL. > So for the time being could you just add a > // TODO(devlin): Make sure that the permission is removed from > // withheld_permissions if appropriate. > either here or in PermissionsUpdater::AddPermissions()? Done. https://codereview.chromium.org/396033002/diff/550001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller_unittest.cc (right): https://codereview.chromium.org/396033002/diff/550001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:45: // Creates an extension with all host permission and adds it to the registry. On 2014/08/15 15:10:31, Devlin wrote: > no need to remove the 's' ("all hosts" is the name of the permission) I'm actually not sure how that happened. Must've been a typo. Done. https://codereview.chromium.org/396033002/diff/550001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller_unittest.cc:372: // Reloading the extension should clear active permissions, but not persisted On 2014/08/15 15:10:31, Devlin wrote: > this is a bit confusing since all we do is actually add the permission to > "active permissions". Maybe just say "Reloading the extension should not clear > any granted host permissions." Done. https://codereview.chromium.org/396033002/diff/550001/extensions/common/url_p... File extensions/common/url_pattern_set_unittest.cc (right): https://codereview.chromium.org/396033002/diff/550001/extensions/common/url_p... extensions/common/url_pattern_set_unittest.cc:426: set.AddOrigin(URLPattern::SCHEME_ALL, GURL("https://www.google.com/")); On 2014/08/15 15:10:31, Devlin wrote: > Should probably say EXPECT_TRUE() Done. https://codereview.chromium.org/396033002/diff/550001/extensions/common/url_p... extensions/common/url_pattern_set_unittest.cc:432: set.AddOrigin(URLPattern::SCHEME_ALL, GURL("https://google.com/")); On 2014/08/15 15:10:31, Devlin wrote: > Should probably also say EXPECT_TRUE() Done. https://codereview.chromium.org/396033002/diff/550001/extensions/common/url_p... extensions/common/url_pattern_set_unittest.cc:434: EXPECT_TRUE(set.MatchesURL(GURL("https://google.com/foo/bar"))); On 2014/08/15 15:10:31, Devlin wrote: > For kicks and grins, can we also test an AddOrigin() that fails? (Just fails to > add, not DCHECKs) Done.
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gpdavis.chromium@gmail.com/396033002/5...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
Message was sent while issue was closed.
Committed patchset #24 (570001) as 290039 |
