Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
501273002:
Update extension install prompt to reflect withheld permissions (Closed)
DescriptionUpdate extension install prompt to reflect withheld permissions.
Screenshots:
Ubuntu:
http://i.imgur.com/FuzZfcr.jpg
OSX:
http://i.imgur.com/ylHBVCx.png
BUG=407453
Committed: https://crrev.com/0fbac4d6271f2eaacd8d67800ba5dce7781578b4
Cr-Commit-Position: refs/heads/master@{#295773}
Patch Set 1 #Patch Set 2 : Added init calls for other types of installs #
Total comments: 7
Patch Set 3 : Move init calls #Patch Set 4 : Overload initialize #Patch Set 5 : Added withheld permissions to install prompt #
Total comments: 4
Patch Set 6 : Refactor extension install dialog view constructor #
Total comments: 17
Patch Set 7 : Cocoa implementation, move permissions setter #Patch Set 8 : Minor changes #
Total comments: 14
Patch Set 9 : Passing changes to Mac for cocoa work #Patch Set 10 : Refactor adding permissions in cocoa impl #Patch Set 11 : Removed rogue comment #
Total comments: 13
Patch Set 12 : Prompt and Updater changes #
Total comments: 3
Patch Set 13 : Permissions helper method, init flag changes in updater #
Total comments: 11
Patch Set 14 : Added unit test #Patch Set 15 : Comment updates and cleanup #
Total comments: 22
Patch Set 16 : Minor changes #
Total comments: 8
Patch Set 17 : Cocoa changes #
Total comments: 14
Patch Set 18 : Minor changes #
Total comments: 12
Patch Set 19 : Nits #Patch Set 20 : Header update #
Total comments: 4
Patch Set 21 : Cocoa nits #Patch Set 22 : Update cocoa test, add default cases for compiler #Messages
Total messages: 74 (11 generated)
gpdavis.chromium@gmail.com changed reviewers: + kalman@chromium.org
Here's a first iteration for that bug we discussed last week. I've also looked a little into the permissions elevation issues, and it looks to be a can of worms. I'd love to discuss this with you when you have a chance.
I looked into permissions elevation some more, and I'm not sure there is a way to determine whether permissions were elevated due to this flag changing. CheckPermissionsIncrease calls InitializePermissions, and then compares granted (old) permissions to active (new) permissions. These granted permissions include all the withheld permissions, so it's unable to identify when these permissions are no longer being withheld (or rather, that they were ever withheld in the first place). Could we add more data to preferences regarding this? Maybe something that saves information about withheld permissions such that when a new session is started, we can check to see if they are no longer being withheld?
Woo! It works! Oh man I can't believe we needed to this at such a high level, but I think it's actually the right change. I'd expected the changes to support this to be at a lower level (like PermissonsUpdater), but from what I can tell PermissionsUpdater is correct (and this patch does verify that, I think). It's a total accident that you even needed to make these changes. What happens with installing packed extensions by dragging into chrome://extensions? Do you need to update that as well, or does it automatically work? I'm not sure why removing the flag doesn't cause the extension to be disabled and show the upgrade prompt. I'd expect that installing the extension with withheld permissions would mean that the granted permission set wouldn't include <all_urls>. Maybe it does? We should fix that. If the granted permissions don't include <all_urls> then I'd expect that when loading the extension we'd detect that it requires more than it's been granted.
On 2014/08/27 17:33:22, kalman wrote: > Woo! It works! > > Oh man I can't believe we needed to this at such a high level, but I think it's > actually the right change. > > I'd expected the changes to support this to be at a lower level (like > PermissonsUpdater), but from what I can tell PermissionsUpdater is correct (and > this patch does verify that, I think). It's a total accident that you even > needed to make these changes. Seemed to be the best way for this to work to me as well (: The only other option I can think of is to do this whenever an extension is created. That would blanket all cases of extension creation. But it doesn't look like we have a good way to access the profile from extension.cc, so I doubt it'd be as clean as we would hope. > What happens with installing packed extensions by dragging into > chrome://extensions? Do you need to update that as well, or does it > automatically work? Just looked into this, and it didn't work. So I fixed it. Added InitializePermissions calls for other types of installs. I had to make the call for bundled extensions inside the bundle installer. Looks like all of them should have these permissions withheld before the prompt permissions are populated. > I'm not sure why removing the flag doesn't cause the extension to be disabled > and show the upgrade prompt. I'd expect that installing the extension with > withheld permissions would mean that the granted permission set wouldn't include > <all_urls>. Maybe it does? We should fix that. > > If the granted permissions don't include <all_urls> then I'd expect that when > loading the extension we'd detect that it requires more than it's been granted. This is tricky, and I hope I understand it well enough to explain. I did some permissions dumping in CheckPermissionsIncrease to see exactly what's going on. Let me make a separate bug and CL for this, and I can upload a patchset with the log statements and show you what came out of it so it makes more sense.
https://codereview.chromium.org/501273002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/bundle_installer.cc (right): https://codereview.chromium.org/501273002/diff/20001/chrome/browser/extension... chrome/browser/extensions/bundle_installer.cc:56: &error); Would this be a better place to manipulate permissions? https://codereview.chromium.org/501273002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_install_prompt.cc (right): https://codereview.chromium.org/501273002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_install_prompt.cc:566: extension_); A weirdness here is that even though this is a dummy extension we're still going to prefs to look for it (see the implementation of InitializePermissions). It works because that ends up reading NULL for the permissions and we end up using permissions_data()->active_permissions(), which is correct, but still. Weird. And what if InitializePermissions one day starts writing stuff to prefs? We need a "no prefs" version of InitializePermissions :\ which implies a static InitializePermissions-type method ... which is yuck. Ping this stuff to me tomorrow. I can't think of a particularly nice way of doing it right now. https://codereview.chromium.org/501273002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_install_prompt.cc:764: message_provider->GetWarningMessagesDetails(permissions_.get(), type)); Around here should be the place that you pull out the permission warnings, not up top. There are a lot of cases you're missing there. Having it here would pick those up.
https://codereview.chromium.org/501273002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/bundle_installer.cc (right): https://codereview.chromium.org/501273002/diff/20001/chrome/browser/extension... chrome/browser/extensions/bundle_installer.cc:56: &error); On 2014/08/28 03:17:32, kalman wrote: > Would this be a better place to manipulate permissions? I put it below so that we did it just before creating the permissions to pass on, but it does make more sense to just ensure that any extension created in here gets its permissions initialized. Done. https://codereview.chromium.org/501273002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_install_prompt.cc (right): https://codereview.chromium.org/501273002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_install_prompt.cc:566: extension_); On 2014/08/28 03:17:32, kalman wrote: > A weirdness here is that even though this is a dummy extension we're still going > to prefs to look for it (see the implementation of InitializePermissions). It > works because that ends up reading NULL for the permissions and we end up using > permissions_data()->active_permissions(), which is correct, but still. Weird. > And what if InitializePermissions one day starts writing stuff to prefs? > > We need a "no prefs" version of InitializePermissions :\ which implies a static > InitializePermissions-type method ... which is yuck. > > Ping this stuff to me tomorrow. I can't think of a particularly nice way of > doing it right now. Ping :) What if we just add a flag to InitializePermissions? Something like is_dummy_extension, and if it's true, we don't access prefs and we can add a comment that indicates that dummy extensions shouldn't be looked for in prefs? InitializePermissions isn't called all that many times, so this shouldn't create too much of an impact elsewhere. https://codereview.chromium.org/501273002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_install_prompt.cc:764: message_provider->GetWarningMessagesDetails(permissions_.get(), type)); On 2014/08/28 03:17:32, kalman wrote: > Around here should be the place that you pull out the permission warnings, not > up top. There are a lot of cases you're missing there. Having it here would pick > those up. For some reason I felt like I needed to initialize them before permissions_ was set, but now I'm realizing that it's just a pointer, so it doesn't matter, as long as it's initialized before SetPermissions. Oops. So the only missing case here is the bundle, correct? And we can just handle that over in the bundle installer.
kalman@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Looking great. > So the only missing case here is the bundle, correct? And we can just handle that over in the bundle installer. What do you mean? +Devlin here. He's not so keen on landing this without a UI change that shows the withheld permissions as well. I think that's a good idea until we have all the infrastructure in place. https://codereview.chromium.org/501273002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_install_prompt.cc (right): https://codereview.chromium.org/501273002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_install_prompt.cc:566: extension_); On 2014/08/28 17:41:48, gpdavis wrote: > On 2014/08/28 03:17:32, kalman wrote: > > A weirdness here is that even though this is a dummy extension we're still > going > > to prefs to look for it (see the implementation of InitializePermissions). It > > works because that ends up reading NULL for the permissions and we end up > using > > permissions_data()->active_permissions(), which is correct, but still. Weird. > > And what if InitializePermissions one day starts writing stuff to prefs? > > > > We need a "no prefs" version of InitializePermissions :\ which implies a > static > > InitializePermissions-type method ... which is yuck. > > > > Ping this stuff to me tomorrow. I can't think of a particularly nice way of > > doing it right now. > > Ping :) > > What if we just add a flag to InitializePermissions? Something like > is_dummy_extension, and if it's true, we don't access prefs and we can add a > comment that indicates that dummy extensions shouldn't be looked for in prefs? > InitializePermissions isn't called all that many times, so this shouldn't create > too much of an impact elsewhere. Yes! That's the best solution I came up with as well. Modulo a different syntax. Let's use flags, like: class PermissionsUpdater { enum InitFlag { INIT_FLAG_NONE = 0, INIT_FLAG_TRANSIENT = 1<<1, }; void InitializePermissions(extension) { InitializePermissions(extension, INIT_FLAG_NONE); } void InitializePermissions(extension, flag) { if (flag & INIT_FLAG_TRANSIENT) ... // don't console preferences about |extension|. } };
> +Devlin here. He's not so keen on landing this without a UI change that shows > the withheld permissions as well. I think that's a good idea until we have all > the infrastructure in place. A UI change such as: Install LinkClump? -------- It can: * View your browsing history. * Do that other thing. -------- Warning: You've enabled the --scripts-require-action flag which limits the capabilities on this computer. However, your other devices may not support this flag or have it enabled. On this computers, this extension can also: * Read and modify your data on all sites.
On 2014/08/28 18:28:02, kalman wrote:
> Looking great.
>
> > So the only missing case here is the bundle, correct? And we can just
handle
> that over in the bundle installer.
>
> What do you mean?
For a bundle install, extension_ doesn't get set, so that code where we just
added the InitializePermissions call won't get run. But that's okay, since we
initialized the permissions in the bundle installer before the bundle was passed
in. I just wanted to ensure that there weren't any other cases that we're
missing where that InitializePermissions call won't be made.
> +Devlin here. He's not so keen on landing this without a UI change that shows
> the withheld permissions as well. I think that's a good idea until we have all
> the infrastructure in place.
This sounds reasonable to me. Doesn't seem difficult either. I'll investigate.
> Yes! That's the best solution I came up with as well. Modulo a different
syntax.
> Let's use flags, like:
>
> class PermissionsUpdater {
> enum InitFlag {
> INIT_FLAG_NONE = 0,
> INIT_FLAG_TRANSIENT = 1<<1,
> };
>
> void InitializePermissions(extension) {
> InitializePermissions(extension, INIT_FLAG_NONE);
> }
>
> void InitializePermissions(extension, flag) {
> if (flag & INIT_FLAG_TRANSIENT)
> ... // don't console preferences about |extension|.
> }
> };
Sweet. I like it. I'll get started on that.
On 2014/08/28 19:07:04, gpdavis wrote:
> On 2014/08/28 18:28:02, kalman wrote:
> > Looking great.
> >
> > > So the only missing case here is the bundle, correct? And we can just
> handle
> > that over in the bundle installer.
> >
> > What do you mean?
>
> For a bundle install, extension_ doesn't get set, so that code where we just
> added the InitializePermissions call won't get run. But that's okay, since we
> initialized the permissions in the bundle installer before the bundle was
passed
> in. I just wanted to ensure that there weren't any other cases that we're
> missing where that InitializePermissions call won't be made.
Yeah we don't really need to worry about bundles, they're installed behind the
scenes anyway and don't show any kind of install prompt.
>
>
> > +Devlin here. He's not so keen on landing this without a UI change that
shows
> > the withheld permissions as well. I think that's a good idea until we have
all
> > the infrastructure in place.
>
> This sounds reasonable to me. Doesn't seem difficult either. I'll
investigate.
(You can ignore the lines)
>
>
> > Yes! That's the best solution I came up with as well. Modulo a different
> syntax.
> > Let's use flags, like:
> >
> > class PermissionsUpdater {
> > enum InitFlag {
> > INIT_FLAG_NONE = 0,
> > INIT_FLAG_TRANSIENT = 1<<1,
> > };
> >
> > void InitializePermissions(extension) {
> > InitializePermissions(extension, INIT_FLAG_NONE);
> > }
> >
> > void InitializePermissions(extension, flag) {
> > if (flag & INIT_FLAG_TRANSIENT)
> > ... // don't console preferences about |extension|.
> > }
> > };
>
> Sweet. I like it. I'll get started on that.
Bit of a rough patchset, but I wanted to ask you about some of the organization decisions. It took quite a bit of code to add this in, and I'm sure there's gotta be a cleaner way to do it. Any advice? https://codereview.chromium.org/501273002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_install_prompt.cc (right): https://codereview.chromium.org/501273002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_install_prompt.cc:224: bool withheld) { Figured I'd add a parameter to these two methods, but I can split em up into SetWithheldPermissions, etc, if that's preferred. https://codereview.chromium.org/501273002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_install_prompt.cc:255: case WITHHELD_PERMISSIONS_DETAILS: This is necessary for the cocoa implementation, which will need to be updated as well. https://codereview.chromium.org/501273002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/501273002/diff/80001/chrome/browser/extension... chrome/browser/extensions/permissions_updater.h:57: void InitializePermissions(const Extension* extension, InitFlag init_flag); Should we be updating the comment here? I didn't want to add implementation details to the header file. https://codereview.chromium.org/501273002/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://codereview.chromium.org/501273002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:463: } This is ugly. Since this constructor is over 300 lines of code already, maybe we ought to factor a lot of this out? What do you think?
Added a screenshot to the description :)
https://codereview.chromium.org/501273002/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/bundle_installer.cc (right): https://codereview.chromium.org/501273002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/bundle_installer.cc:46: Profile* profile) { s/Profile/BrowserContext https://codereview.chromium.org/501273002/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.cc (right): https://codereview.chromium.org/501273002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.cc:836: permissions_ = extension_->permissions_data()->active_permissions(); This will fantastically break optional permissions' use of the install prompt. OptionalPermissions creates an install prompt and sets its extension and permissions, but the permissions will be set to the *desired* permissions, not to the existing permissions. [1] As such, we need to (in some cases) respect the |permissions_| which were set earlier. https://codereview.chromium.org/501273002/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.h (right): https://codereview.chromium.org/501273002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.h:201: std::vector<base::string16> withheld_permissions_; maybe a struct for these, and then one for withheld and one for regular? https://codereview.chromium.org/501273002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_install_dialog_view.h (right): https://codereview.chromium.org/501273002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. (comment inserted at random location) I think there's also a mac version of this dialog that will need to be updated. https://codereview.chromium.org/501273002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.h:58: void InitView(); Comments. https://codereview.chromium.org/501273002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.h:60: bool MaybeAddPermissions(views::GridLayout* layout, Comments https://codereview.chromium.org/501273002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.h:60: bool MaybeAddPermissions(views::GridLayout* layout, Is there a reason these two functions should be public? https://codereview.chromium.org/501273002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.h:61: ui::ResourceBundle& rb, Just forward-declare resource bundle, rather than include it.
On 2014/09/02 21:07:51, Devlin wrote: > https://codereview.chromium.org/501273002/diff/100001/chrome/browser/extensio... > chrome/browser/extensions/extension_install_prompt.cc:836: permissions_ = > extension_->permissions_data()->active_permissions(); > This will fantastically break optional permissions' use of the install prompt. > > OptionalPermissions creates an install prompt and sets its extension and > permissions, but the permissions will be set to the *desired* permissions, not > to the existing permissions. [1] As such, we need to (in some cases) respect > the |permissions_| which were set earlier. I'm having trouble finding where this occurs. Can you point me in the right direction?
Patchset #7 (id:120001) has been deleted
https://codereview.chromium.org/501273002/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/bundle_installer.cc (right): https://codereview.chromium.org/501273002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/bundle_installer.cc:46: Profile* profile) { On 2014/09/02 21:07:51, Devlin wrote: > s/Profile/BrowserContext Done. https://codereview.chromium.org/501273002/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.cc (right): https://codereview.chromium.org/501273002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.cc:836: permissions_ = extension_->permissions_data()->active_permissions(); On 2014/09/02 21:07:51, Devlin wrote: > This will fantastically break optional permissions' use of the install prompt. > > OptionalPermissions creates an install prompt and sets its extension and > permissions, but the permissions will be set to the *desired* permissions, not > to the existing permissions. [1] As such, we need to (in some cases) respect > the |permissions_| which were set earlier. Okay, I think I figured this out. I've removed the permissions_ setters that just use active permissions, and instead, I added logic that will initialize permissions and set permissions_ if permissions_ hasn't already been set. This way, special cases can manually set permissions_ before ShowConfirmation is called, and they won't get overridden. Can you think of any issues that might arise from this implementation? https://codereview.chromium.org/501273002/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.h (right): https://codereview.chromium.org/501273002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.h:201: std::vector<base::string16> withheld_permissions_; On 2014/09/02 21:07:51, Devlin wrote: > maybe a struct for these, and then one for withheld and one for regular? Sure. It's a complex struct, though, so it requires an out of line constructor. Does this belong in the cc file, or below the struct definition in the header? https://codereview.chromium.org/501273002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_install_dialog_view.h (right): https://codereview.chromium.org/501273002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/09/02 21:07:51, Devlin wrote: > (comment inserted at random location) > I think there's also a mac version of this dialog that will need to be updated. Yep; I'm aware of the cocoa file for this as well. I'll go ahead and make the changes there as well. https://codereview.chromium.org/501273002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.h:58: void InitView(); On 2014/09/02 21:07:51, Devlin wrote: > Comments. Wasn't sure what to say about this, since there weren't previously any comments about it and I didn't want to be redundant. How about something like "Initializes the dialogue view, adding in permissions if they exist."? I kind of feel like that doesn't say very much, but I don't want to be overly verbose either. Advice? https://codereview.chromium.org/501273002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.h:60: bool MaybeAddPermissions(views::GridLayout* layout, On 2014/09/02 21:07:51, Devlin wrote: > Is there a reason these two functions should be public? No there is not. My mistake. Done. https://codereview.chromium.org/501273002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.h:60: bool MaybeAddPermissions(views::GridLayout* layout, On 2014/09/02 21:07:51, Devlin wrote: > Comments How about... "Adds permissions of |perm_type| to the dialogue view if they exist." ? https://codereview.chromium.org/501273002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.h:61: ui::ResourceBundle& rb, On 2014/09/02 21:07:51, Devlin wrote: > Just forward-declare resource bundle, rather than include it. Done.
https://codereview.chromium.org/501273002/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.h (right): https://codereview.chromium.org/501273002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.h:201: std::vector<base::string16> withheld_permissions_; On 2014/09/04 19:40:04, gpdavis wrote: > On 2014/09/02 21:07:51, Devlin wrote: > > maybe a struct for these, and then one for withheld and one for regular? > > Sure. It's a complex struct, though, so it requires an out of line constructor. > Does this belong in the cc file, or below the struct definition in the header? in the cc file. https://codereview.chromium.org/501273002/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.cc (right): https://codereview.chromium.org/501273002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.cc:824: // Initialize |permissions_| if they have not already been set so that This just seems so fragile. Let's make it a little bit better by: - Renaming |permissions_| in ExtensionInstallPrompt to |custom_permissions_|, with a comment like "A custom set of permissions to show in the install prompt instead of the extension's active permissions". Here, make a scoped_refptr<PermissionSet> permissions_to_display; if (custom_permissions_) { permissions_to_display = custom_permissions_; } else if (extension_) { // Initialize permissions so that any permissions that will be withheld don't // end up in the permissions prompt. <update permissions> permissions_to_display = extension_->permissions_data()->active_permissions(); } .... https://codereview.chromium.org/501273002/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.h (right): https://codereview.chromium.org/501273002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.h:190: struct PermissionsInfo { We already have a PermissionsInfo, so this may be confusing. How about InstallPromptPermissions? https://codereview.chromium.org/501273002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.h:192: ~PermissionsInfo(); nit: newline between destructor and members. https://codereview.chromium.org/501273002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.h:198: friend class base::RefCountedThreadSafe<Prompt>; nit: leave friend class above the struct. https://codereview.chromium.org/501273002/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm (right): https://codereview.chromium.org/501273002/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:649: ExtensionInstallPrompt::WITHHELD_PERMISSIONS_DETAILS, i)) { It looks like a helper function might be in order here. :) https://codereview.chromium.org/501273002/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://codereview.chromium.org/501273002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:390: if (prompt_->GetPermissionCount( Maybe make a bool has_permissions, and then use it here and on line 409. https://codereview.chromium.org/501273002/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_install_dialog_view.h (right): https://codereview.chromium.org/501273002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.h:97: bool MaybeAddPermissions(views::GridLayout* layout, I would just call this AddPermissions(). The comment is enough to say that we won't add any if none exist (since that should be fairly obvious).
https://codereview.chromium.org/501273002/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.cc (right): https://codereview.chromium.org/501273002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.cc:824: // Initialize |permissions_| if they have not already been set so that On 2014/09/04 22:06:08, Devlin wrote: > This just seems so fragile. Let's make it a little bit better by: > - Renaming |permissions_| in ExtensionInstallPrompt to |custom_permissions_|, > with a comment like "A custom set of permissions to show in the install prompt > instead of the extension's active permissions". > > Here, make a > scoped_refptr<PermissionSet> permissions_to_display; > if (custom_permissions_) { > permissions_to_display = custom_permissions_; > } else if (extension_) { > // Initialize permissions so that any permissions that will be withheld don't > // end up in the permissions prompt. > <update permissions> > permissions_to_display = extension_->permissions_data()->active_permissions(); > } > > .... Clever! I like it. Done. https://codereview.chromium.org/501273002/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.h (right): https://codereview.chromium.org/501273002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.h:190: struct PermissionsInfo { On 2014/09/04 22:06:08, Devlin wrote: > We already have a PermissionsInfo, so this may be confusing. How about > InstallPromptPermissions? Done. https://codereview.chromium.org/501273002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.h:192: ~PermissionsInfo(); On 2014/09/04 22:06:08, Devlin wrote: > nit: newline between destructor and members. Done. https://codereview.chromium.org/501273002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.h:198: friend class base::RefCountedThreadSafe<Prompt>; On 2014/09/04 22:06:08, Devlin wrote: > nit: leave friend class above the struct. Done. https://codereview.chromium.org/501273002/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm (right): https://codereview.chromium.org/501273002/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:649: ExtensionInstallPrompt::WITHHELD_PERMISSIONS_DETAILS, i)) { On 2014/09/04 22:06:09, Devlin wrote: > It looks like a helper function might be in order here. :) Done. https://codereview.chromium.org/501273002/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://codereview.chromium.org/501273002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:390: if (prompt_->GetPermissionCount( On 2014/09/04 22:06:09, Devlin wrote: > Maybe make a bool has_permissions, and then use it here and on line 409. Done. https://codereview.chromium.org/501273002/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_install_dialog_view.h (right): https://codereview.chromium.org/501273002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.h:97: bool MaybeAddPermissions(views::GridLayout* layout, On 2014/09/04 22:06:09, Devlin wrote: > I would just call this AddPermissions(). The comment is enough to say that we > won't add any if none exist (since that should be fairly obvious). Done.
Getting close! Sorry I didn't catch these earlier. https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.cc (right): https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.cc:248: switch (permissions_type) { Two improvements here: - reduce duplicate code by setting a temp to the permissions to use. - reduce work and code by using vector::insert() Something like: DCHECK_NE(ALL_PERMISSIONS, permissions_type); InstallPromptPermissions& permissions = permissions_type == REGULAR_PERMISSIONS ? prompt_permissions_ : withheld_prompt_permissions_; permissions.details = details; permissions.is_showing_details.clear(); permissions.is_showing_details.insert( permissions.is_showing_details.begin(), permissions.details.size(), false); https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.cc:511: NOTREACHED(); Actually, let's even make a InstallPromptPermissions& GetPermissionsToUse(PermissionsType permissions_type) { DCHECK_NE(ALL_PERMISSIONS, permissions_type); return permissions_type == REGULAR_PERMISSIONS ? prompt_permissions_ : withheld_prompt_permissions_; } Then we use that anywhere we have this pattern (specifically with the NOTREACHED()), and this method becomes: { const InstallPromptPermissions& permissions = GetPermissionsToUse(permissions_type); CHECK_LT(index, permissions.size()); return permissions[index]; } https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:301: ExtensionPrefs::Get(browser_context_)->SetActivePermissions( I'm worried about the fact that SetPermissions (which is called from InitializePermissions()) sets them in the prefs. I think we should pull out the inner logic of InitializePermissions (~185 - 228) into a separate (maybe static?) method, which we then use a) in InitializePermissions and b) when we want to initialize transiently.
Patchset #12 (id:240001) has been deleted
Patchset #12 (id:260001) has been deleted
https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.cc (right): https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.cc:248: switch (permissions_type) { On 2014/09/08 16:09:22, Devlin wrote: > Two improvements here: > - reduce duplicate code by setting a temp to the permissions to use. > - reduce work and code by using vector::insert() > > Something like: > DCHECK_NE(ALL_PERMISSIONS, permissions_type); > InstallPromptPermissions& permissions = > permissions_type == REGULAR_PERMISSIONS ? prompt_permissions_ : > withheld_prompt_permissions_; > permissions.details = details; > permissions.is_showing_details.clear(); > permissions.is_showing_details.insert( > permissions.is_showing_details.begin(), permissions.details.size(), false); Done. https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.cc:511: NOTREACHED(); On 2014/09/08 16:09:22, Devlin wrote: > Actually, let's even make a > > InstallPromptPermissions& > GetPermissionsToUse(PermissionsType permissions_type) { > DCHECK_NE(ALL_PERMISSIONS, permissions_type); > return permissions_type == REGULAR_PERMISSIONS ? prompt_permissions_ : > withheld_prompt_permissions_; > } > > Then we use that anywhere we have this pattern (specifically with the > NOTREACHED()), and this method becomes: > { > const InstallPromptPermissions& permissions = > GetPermissionsToUse(permissions_type); > CHECK_LT(index, permissions.size()); > return permissions[index]; > } I like this idea, but we've got a const problem. The setter methods need a non-const reference, but the getter methods are const. I'll just inline it, unless you'd prefer it another way. https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:301: ExtensionPrefs::Get(browser_context_)->SetActivePermissions( On 2014/09/08 16:09:22, Devlin wrote: > I'm worried about the fact that SetPermissions (which is called from > InitializePermissions()) sets them in the prefs. I think we should pull out the > inner logic of InitializePermissions (~185 - 228) into a separate (maybe > static?) method, which we then use a) in InitializePermissions and b) when we > want to initialize transiently. If we don't make the SetPermissions call for a transient extension, the withholding won't actually happen, and the install prompt won't reflect withheld permissions. We already addressed looking in prefs above (using the init flag), so maybe we could pass that on to SetPermissions? Or how about adding SetPermissionsWithoutPrefs, calling it conditionally based on the init flag in InitializePermissions? https://codereview.chromium.org/501273002/diff/280001/chrome/browser/extensio... File chrome/browser/extensions/external_install_error.cc (right): https://codereview.chromium.org/501273002/diff/280001/chrome/browser/extensio... chrome/browser/extensions/external_install_error.cc:228: prompt_->GetPermission(i, withheld_permissions))); Do you think this warrants some sort of loop to reduce duplicated code? This seemed like the cleanest way to reflect withheld permissions to me.
https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.cc (right): https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.cc:511: NOTREACHED(); On 2014/09/08 20:07:05, gpdavis wrote: > On 2014/09/08 16:09:22, Devlin wrote: > > Actually, let's even make a > > > > InstallPromptPermissions& > > GetPermissionsToUse(PermissionsType permissions_type) { > > DCHECK_NE(ALL_PERMISSIONS, permissions_type); > > return permissions_type == REGULAR_PERMISSIONS ? prompt_permissions_ : > > > withheld_prompt_permissions_; > > } > > > > Then we use that anywhere we have this pattern (specifically with the > > NOTREACHED()), and this method becomes: > > { > > const InstallPromptPermissions& permissions = > > GetPermissionsToUse(permissions_type); > > CHECK_LT(index, permissions.size()); > > return permissions[index]; > > } > > I like this idea, but we've got a const problem. The setter methods need a > non-const reference, but the getter methods are const. I'll just inline it, > unless you'd prefer it another way. Hmm... Lame. I think I might opt for still making the helper method, and just making a const and non-const version. It makes the code in these methods a lot easier to read, IMO. How's the line count vary? https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:301: ExtensionPrefs::Get(browser_context_)->SetActivePermissions( Right, so I would suggest either a) make two methods, one of which is static and does all the work to initialize permissions but doesn't save them in prefs and one which calls the first and then saves them, or b) pass the init flag into the PermissionsUpdater and check it in SetPermissions(). I think I have a slight preference towards a), because it ensures that any time we try to do something like AddPermission(), we must use a real PermissionsUpdater, and if we want to initialize for install, it's guaranteed to be a one-off. https://codereview.chromium.org/501273002/diff/280001/chrome/browser/extensio... File chrome/browser/extensions/external_install_error.cc (right): https://codereview.chromium.org/501273002/diff/280001/chrome/browser/extensio... chrome/browser/extensions/external_install_error.cc:228: prompt_->GetPermission(i, withheld_permissions))); On 2014/09/08 20:07:05, gpdavis wrote: > Do you think this warrants some sort of loop to reduce duplicated code? This > seemed like the cleanest way to reflect withheld permissions to me. Yeah, I was wondering if it would... but I think it's probably okay. Even though it's a few lines, it's really only two function calls, so it's not as prone to breaking.
https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.cc (right): https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.cc:511: NOTREACHED(); On 2014/09/09 15:53:21, Devlin wrote: > On 2014/09/08 20:07:05, gpdavis wrote: > > On 2014/09/08 16:09:22, Devlin wrote: > > > Actually, let's even make a > > > > > > InstallPromptPermissions& > > > GetPermissionsToUse(PermissionsType permissions_type) { > > > DCHECK_NE(ALL_PERMISSIONS, permissions_type); > > > return permissions_type == REGULAR_PERMISSIONS ? prompt_permissions_ : > > > > > withheld_prompt_permissions_; > > > } > > > > > > Then we use that anywhere we have this pattern (specifically with the > > > NOTREACHED()), and this method becomes: > > > { > > > const InstallPromptPermissions& permissions = > > > GetPermissionsToUse(permissions_type); > > > CHECK_LT(index, permissions.size()); > > > return permissions[index]; > > > } > > > > I like this idea, but we've got a const problem. The setter methods need a > > non-const reference, but the getter methods are const. I'll just inline it, > > unless you'd prefer it another way. > > Hmm... Lame. I think I might opt for still making the helper method, and just > making a const and non-const version. It makes the code in these methods a lot > easier to read, IMO. How's the line count vary? The file is six lines longer with the two helper methods. I agree that it makes the code easier to read, but it can only effectively be used in Get/SetPermissions and Get/SetPermissionsDetails. Your call on whether it's worth it. https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:301: ExtensionPrefs::Get(browser_context_)->SetActivePermissions( On 2014/09/09 15:53:21, Devlin wrote: > Right, so I would suggest either > a) make two methods, one of which is static and does all the work to initialize > permissions but doesn't save them in prefs and one which calls the first and > then saves them, or > b) pass the init flag into the PermissionsUpdater and check it in > SetPermissions(). > > I think I have a slight preference towards a), because it ensures that any time > we try to do something like AddPermission(), we must use a real > PermissionsUpdater, and if we want to initialize for install, it's guaranteed to > be a one-off. How does my implementation in patch set 12 look? I figure it makes the most sense to keep the init flag logic in InitializePermissions. That way it's obvious what the flag does, and it doesn't become tangled in a bunch of other code. Is there any particular reason why you'd prefer one of these two suggestions over the way I did it there? https://codereview.chromium.org/501273002/diff/280001/chrome/browser/extensio... File chrome/browser/extensions/external_install_error.cc (right): https://codereview.chromium.org/501273002/diff/280001/chrome/browser/extensio... chrome/browser/extensions/external_install_error.cc:228: prompt_->GetPermission(i, withheld_permissions))); On 2014/09/09 15:53:21, Devlin wrote: > On 2014/09/08 20:07:05, gpdavis wrote: > > Do you think this warrants some sort of loop to reduce duplicated code? This > > seemed like the cleanest way to reflect withheld permissions to me. > > Yeah, I was wondering if it would... but I think it's probably okay. Even > though it's a few lines, it's really only two function calls, so it's not as > prone to breaking. Sweet.
https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.cc (right): https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.cc:511: NOTREACHED(); On 2014/09/09 17:32:41, gpdavis wrote: > On 2014/09/09 15:53:21, Devlin wrote: > > On 2014/09/08 20:07:05, gpdavis wrote: > > > On 2014/09/08 16:09:22, Devlin wrote: > > > > Actually, let's even make a > > > > > > > > InstallPromptPermissions& > > > > GetPermissionsToUse(PermissionsType permissions_type) { > > > > DCHECK_NE(ALL_PERMISSIONS, permissions_type); > > > > return permissions_type == REGULAR_PERMISSIONS ? prompt_permissions_ : > > > > > > > withheld_prompt_permissions_; > > > > } > > > > > > > > Then we use that anywhere we have this pattern (specifically with the > > > > NOTREACHED()), and this method becomes: > > > > { > > > > const InstallPromptPermissions& permissions = > > > > GetPermissionsToUse(permissions_type); > > > > CHECK_LT(index, permissions.size()); > > > > return permissions[index]; > > > > } > > > > > > I like this idea, but we've got a const problem. The setter methods need a > > > non-const reference, but the getter methods are const. I'll just inline it, > > > unless you'd prefer it another way. > > > > Hmm... Lame. I think I might opt for still making the helper method, and just > > making a const and non-const version. It makes the code in these methods a > lot > > easier to read, IMO. How's the line count vary? > > The file is six lines longer with the two helper methods. I agree that it makes > the code easier to read, but it can only effectively be used in > Get/SetPermissions and Get/SetPermissionsDetails. Your call on whether it's > worth it. I say go for it. Readability wins out. https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:301: ExtensionPrefs::Get(browser_context_)->SetActivePermissions( On 2014/09/09 17:32:41, gpdavis wrote: > On 2014/09/09 15:53:21, Devlin wrote: > > Right, so I would suggest either > > a) make two methods, one of which is static and does all the work to > initialize > > permissions but doesn't save them in prefs and one which calls the first and > > then saves them, or > > b) pass the init flag into the PermissionsUpdater and check it in > > SetPermissions(). > > > > I think I have a slight preference towards a), because it ensures that any > time > > we try to do something like AddPermission(), we must use a real > > PermissionsUpdater, and if we want to initialize for install, it's guaranteed > to > > be a one-off. > > How does my implementation in patch set 12 look? I figure it makes the most > sense to keep the init flag logic in InitializePermissions. That way it's > obvious what the flag does, and it doesn't become tangled in a bunch of other > code. Is there any particular reason why you'd prefer one of these two > suggestions over the way I did it there? I don't feel super-strongly either way, but my biggest concern is just that each time you add an "if/else" based on a passed in condition to a function, it gets harder to read, harder to determine which case we want, harder to maintain, harder to test, more fragile, etc. It's better if a function like this could say "Here's what I do to the input permissions" and then callers determine which permissions to input and what to do with the output. It makes the distinction clearer and more deliberate. That said, it's not terribly complicated, so, assuming it doesn't get much worse, it'll probably be fine. But who knows what else we'll need to add to it at some point?
https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.cc (right): https://codereview.chromium.org/501273002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.cc:511: NOTREACHED(); On 2014/09/09 17:46:22, Devlin wrote: > On 2014/09/09 17:32:41, gpdavis wrote: > > On 2014/09/09 15:53:21, Devlin wrote: > > > On 2014/09/08 20:07:05, gpdavis wrote: > > > > On 2014/09/08 16:09:22, Devlin wrote: > > > > > Actually, let's even make a > > > > > > > > > > InstallPromptPermissions& > > > > > GetPermissionsToUse(PermissionsType permissions_type) { > > > > > DCHECK_NE(ALL_PERMISSIONS, permissions_type); > > > > > return permissions_type == REGULAR_PERMISSIONS ? prompt_permissions_ : > > > > > > > > > withheld_prompt_permissions_; > > > > > } > > > > > > > > > > Then we use that anywhere we have this pattern (specifically with the > > > > > NOTREACHED()), and this method becomes: > > > > > { > > > > > const InstallPromptPermissions& permissions = > > > > > GetPermissionsToUse(permissions_type); > > > > > CHECK_LT(index, permissions.size()); > > > > > return permissions[index]; > > > > > } > > > > > > > > I like this idea, but we've got a const problem. The setter methods need > a > > > > non-const reference, but the getter methods are const. I'll just inline > it, > > > > unless you'd prefer it another way. > > > > > > Hmm... Lame. I think I might opt for still making the helper method, and > just > > > making a const and non-const version. It makes the code in these methods a > > lot > > > easier to read, IMO. How's the line count vary? > > > > The file is six lines longer with the two helper methods. I agree that it > makes > > the code easier to read, but it can only effectively be used in > > Get/SetPermissions and Get/SetPermissionsDetails. Your call on whether it's > > worth it. > > I say go for it. Readability wins out. Sounds good! https://codereview.chromium.org/501273002/diff/300001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/501273002/diff/300001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:114: init_flag_(INIT_FLAG_NONE) { How about something like this? The init flag is now a member variable that defaults to none. Calling InitializePermissions with a flag will set that member variable so it doesn't have to be passed around. Perhaps we could add another constructor that takes in a flag, and the methods that change prefs could check to make sure the init flag isn't transient before making changes?
And, hate to sound like a broken record, but... we need tests for this. :) https://codereview.chromium.org/501273002/diff/300001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.cc (right): https://codereview.chromium.org/501273002/diff/300001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.cc:229: ExtensionInstallPrompt::Prompt::InstallPromptPermissions& nit: Same order as in the header file (to the extent possible). https://codereview.chromium.org/501273002/diff/300001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.cc:255: GetPermissionsToUse(permissions_type); ahh, so much cleaner. :D https://codereview.chromium.org/501273002/diff/300001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.h (right): https://codereview.chromium.org/501273002/diff/300001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.h:204: // |permissions_type|. Comes in const and non-const flavors for both I think we can do without the const/non-const comment - the methods speak for themselves :) (It's not an uncommon practice to have both const and non-const.) https://codereview.chromium.org/501273002/diff/300001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/501273002/diff/300001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:114: init_flag_(INIT_FLAG_NONE) { On 2014/09/09 20:28:15, gpdavis wrote: > How about something like this? The init flag is now a member variable that > defaults to none. Calling InitializePermissions with a flag will set that > member variable so it doesn't have to be passed around. > > Perhaps we could add another constructor that takes in a flag, and the methods > that change prefs could check to make sure the init flag isn't transient before > making changes? Yeah, we should make another constructor PermissionsUpdater(flag), and then not even need to pass it in to InitializePermissions(). https://codereview.chromium.org/501273002/diff/300001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:303: if (init_flag_ & INIT_FLAG_NONE) { I think more correct would be if (init_flag_ & INIT_FLAG_TRANSIENT == 0) (We should check the absence of the transience flag, not the presence of no flag)
Not a problem! I made one test with withheld and regular permissions. Are there any other cases we ought to specifically test for? Perhaps adding a withheld permission without any regular ones? https://codereview.chromium.org/501273002/diff/300001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.cc (right): https://codereview.chromium.org/501273002/diff/300001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.cc:229: ExtensionInstallPrompt::Prompt::InstallPromptPermissions& On 2014/09/10 22:00:44, Devlin wrote: > nit: Same order as in the header file (to the extent possible). Done. https://codereview.chromium.org/501273002/diff/300001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.cc:255: GetPermissionsToUse(permissions_type); On 2014/09/10 22:00:44, Devlin wrote: > ahh, so much cleaner. :D Agreed! (: https://codereview.chromium.org/501273002/diff/300001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.h (right): https://codereview.chromium.org/501273002/diff/300001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.h:204: // |permissions_type|. Comes in const and non-const flavors for both On 2014/09/10 22:00:44, Devlin wrote: > I think we can do without the const/non-const comment - the methods speak for > themselves :) (It's not an uncommon practice to have both const and non-const.) Ah, okay, fair enough. https://codereview.chromium.org/501273002/diff/300001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/501273002/diff/300001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:114: init_flag_(INIT_FLAG_NONE) { On 2014/09/10 22:00:44, Devlin wrote: > On 2014/09/09 20:28:15, gpdavis wrote: > > How about something like this? The init flag is now a member variable that > > defaults to none. Calling InitializePermissions with a flag will set that > > member variable so it doesn't have to be passed around. > > > > Perhaps we could add another constructor that takes in a flag, and the methods > > that change prefs could check to make sure the init flag isn't transient > before > > making changes? > > Yeah, we should make another constructor PermissionsUpdater(flag), and then not > even need to pass it in to InitializePermissions(). Awesome. Done. https://codereview.chromium.org/501273002/diff/300001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:303: if (init_flag_ & INIT_FLAG_NONE) { On 2014/09/10 22:00:44, Devlin wrote: > I think more correct would be > if (init_flag_ & INIT_FLAG_TRANSIENT == 0) > > (We should check the absence of the transience flag, not the presence of no > flag) I'm not really sure why I didn't do it this way to begin with *shrug*. Done.
Nice! Could you also add a mac screenshot to the description, for completeness? https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.h (right): https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.h:205: InstallPromptPermissions& GetPermissionsToUse( nit: Let's actually change this "GetPermissionsForType" (my bad). https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt_unittest.cc (right): https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt_unittest.cc:59: 1, // regular_permissions_count nit 1u and 0u (size_ts). https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt_unittest.cc:71: scoped_ptr<FeatureSwitch::ScopedOverride> enable_scripts_switch( This doesn't have to be a scoped_ptr (can be alive for the whole test). https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt_unittest.cc:89: prompt.ConfirmExternalInstall( External install's actually not the best candidate for this. Let's do ConfirmWebstoreInstall, and, for kicks and grins, also ConfirmInstall. https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt_unittest.cc:94: 2, // regular_permissions_count We should mention in a comment what the withheld/regular permissions we expect are. https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:314: const PermissionSet* changed_permissions) { Let's throw in a DCHECK here that we don't have INIT_FLAG_TRANSIENT on. https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.h:36: explicit PermissionsUpdater(content::BrowserContext* browser_context, This doesn't need explicit anymore :) https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.h:97: InitFlag init_flag_; Comment.
Mac screenshot added :) https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.h (right): https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.h:205: InstallPromptPermissions& GetPermissionsToUse( On 2014/09/11 21:18:53, Devlin wrote: > nit: Let's actually change this "GetPermissionsForType" (my bad). Done. https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt_unittest.cc (right): https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt_unittest.cc:59: 1, // regular_permissions_count On 2014/09/11 21:18:53, Devlin wrote: > nit 1u and 0u (size_ts). Done. https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt_unittest.cc:71: scoped_ptr<FeatureSwitch::ScopedOverride> enable_scripts_switch( On 2014/09/11 21:18:53, Devlin wrote: > This doesn't have to be a scoped_ptr (can be alive for the whole test). Done. https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt_unittest.cc:89: prompt.ConfirmExternalInstall( On 2014/09/11 21:18:53, Devlin wrote: > External install's actually not the best candidate for this. Let's do > ConfirmWebstoreInstall, and, for kicks and grins, also ConfirmInstall. Well, ConfirmWebstoreInstall just calls ConfirmInstall after setting the icon. Is it worth testing separately? https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt_unittest.cc:94: 2, // regular_permissions_count On 2014/09/11 21:18:53, Devlin wrote: > We should mention in a comment what the withheld/regular permissions we expect > are. Done. https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:314: const PermissionSet* changed_permissions) { On 2014/09/11 21:18:53, Devlin wrote: > Let's throw in a DCHECK here that we don't have INIT_FLAG_TRANSIENT on. Sure thing. We don't need this anywhere else, do we? https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.h:36: explicit PermissionsUpdater(content::BrowserContext* browser_context, On 2014/09/11 21:18:53, Devlin wrote: > This doesn't need explicit anymore :) Ah, okay, we just need explicit for single parameter constructors, correct? https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.h:97: InitFlag init_flag_; On 2014/09/11 21:18:53, Devlin wrote: > Comment. Done.
Will look over again tomorrow :) https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt_unittest.cc (right): https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt_unittest.cc:89: prompt.ConfirmExternalInstall( On 2014/09/12 00:11:53, gpdavis wrote: > On 2014/09/11 21:18:53, Devlin wrote: > > External install's actually not the best candidate for this. Let's do > > ConfirmWebstoreInstall, and, for kicks and grins, also ConfirmInstall. > > Well, ConfirmWebstoreInstall just calls ConfirmInstall after setting the icon. > Is it worth testing separately? Probably not, then. :) (Silly me, just looked the the method comments.) We could make the argument that they could diverge at some point but... don't worry about it. https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:314: const PermissionSet* changed_permissions) { On 2014/09/12 00:11:54, gpdavis wrote: > On 2014/09/11 21:18:53, Devlin wrote: > > Let's throw in a DCHECK here that we don't have INIT_FLAG_TRANSIENT on. > > Sure thing. We don't need this anywhere else, do we? Whoops! Meant to put this comment in NotifyPermissionsUpdated(). I'm not too worried about it, because Add/Remove permissions both call NotifyPermissionsUpdated(). https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.h:36: explicit PermissionsUpdater(content::BrowserContext* browser_context, On 2014/09/12 00:11:54, gpdavis wrote: > On 2014/09/11 21:18:53, Devlin wrote: > > This doesn't need explicit anymore :) > > Ah, okay, we just need explicit for single parameter constructors, correct? Right. This stack overflow question does a good job of explaining. [1] But basically, explicit means that, if an object's constructor takes a single second object, given that single object, you can implicitly convert to the first object. Very bad in the case of, say, a Browser object (which only needs a Profile). [1] http://stackoverflow.com/questions/121162/what-does-the-explicit-keyword-in-c...
Patchset #16 (id:360001) has been deleted
Have a good evening! https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt_unittest.cc (right): https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt_unittest.cc:89: prompt.ConfirmExternalInstall( On 2014/09/12 00:26:45, Devlin wrote: > On 2014/09/12 00:11:53, gpdavis wrote: > > On 2014/09/11 21:18:53, Devlin wrote: > > > External install's actually not the best candidate for this. Let's do > > > ConfirmWebstoreInstall, and, for kicks and grins, also ConfirmInstall. > > > > Well, ConfirmWebstoreInstall just calls ConfirmInstall after setting the icon. > > > Is it worth testing separately? > > Probably not, then. :) (Silly me, just looked the the method comments.) We > could make the argument that they could diverge at some point but... don't worry > about it. Sounds good! https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:314: const PermissionSet* changed_permissions) { On 2014/09/12 00:26:45, Devlin wrote: > On 2014/09/12 00:11:54, gpdavis wrote: > > On 2014/09/11 21:18:53, Devlin wrote: > > > Let's throw in a DCHECK here that we don't have INIT_FLAG_TRANSIENT on. > > > > Sure thing. We don't need this anywhere else, do we? > > Whoops! Meant to put this comment in NotifyPermissionsUpdated(). > > I'm not too worried about it, because Add/Remove permissions both call > NotifyPermissionsUpdated(). Ah, that makes more sense. The reason I was asking was because of Add/Remove permissions, and NotifyPermissionsUpdated was the place I was thinking of for that very reason. https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/501273002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.h:36: explicit PermissionsUpdater(content::BrowserContext* browser_context, On 2014/09/12 00:26:46, Devlin wrote: > On 2014/09/12 00:11:54, gpdavis wrote: > > On 2014/09/11 21:18:53, Devlin wrote: > > > This doesn't need explicit anymore :) > > > > Ah, okay, we just need explicit for single parameter constructors, correct? > > Right. This stack overflow question does a good job of explaining. [1] But > basically, explicit means that, if an object's constructor takes a single second > object, given that single object, you can implicitly convert to the first > object. Very bad in the case of, say, a Browser object (which only needs a > Profile). > > [1] > http://stackoverflow.com/questions/121162/what-does-the-explicit-keyword-in-c... Yep, I found that with a quick google search :) Just wanted to clarify that that's something we always do for single parameter constructors.
Awesome! Extensions lgtm. You'll need someone from views and cocoa to take a look at those, though.
gpdavis.chromium@gmail.com changed reviewers: + asvitkine@chromium.org, finnur@chromium.org
asvitkine@chromium.org: Please review changes in chrome/browser/ui/cocoa finnur@chromium.org: Please review changes in chrome/browser/ui/views Thanks!
https://codereview.chromium.org/501273002/diff/380001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm (right): https://codereview.chromium.org/501273002/diff/380001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:59: - (bool)appendPermissions:(const ExtensionInstallPrompt::Prompt&)prompt How about the following name, which more closely follows Cocoa permissions: -appendPermissionsToPrompt: withType: heading: children: https://codereview.chromium.org/501273002/diff/380001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:599: NSString* withheld_heading = nil; Using camelCase in ObjC methods. Here and in the code below. https://codereview.chromium.org/501273002/diff/380001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:637: cellAttributes:kBoldText | kAutoExpandCell | Nit: Make a local var for attributes to avoid this wrapping. https://codereview.chromium.org/501273002/diff/380001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:677: if (permissions_count > 0) { Prefer an early return here.
https://codereview.chromium.org/501273002/diff/380001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm (right): https://codereview.chromium.org/501273002/diff/380001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:59: - (bool)appendPermissions:(const ExtensionInstallPrompt::Prompt&)prompt On 2014/09/12 19:32:23, Alexei Svitkine wrote: > How about the following name, which more closely follows Cocoa permissions: > > -appendPermissionsToPrompt: > withType: > heading: > children: Done. https://codereview.chromium.org/501273002/diff/380001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:599: NSString* withheld_heading = nil; On 2014/09/12 19:32:24, Alexei Svitkine wrote: > Using camelCase in ObjC methods. Here and in the code below. Done. https://codereview.chromium.org/501273002/diff/380001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:637: cellAttributes:kBoldText | kAutoExpandCell | On 2014/09/12 19:32:24, Alexei Svitkine wrote: > Nit: Make a local var for attributes to avoid this wrapping. Done. https://codereview.chromium.org/501273002/diff/380001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:677: if (permissions_count > 0) { On 2014/09/12 19:32:24, Alexei Svitkine wrote: > Prefer an early return here. Done.
https://codereview.chromium.org/501273002/diff/400001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm (right): https://codereview.chromium.org/501273002/diff/400001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:682: addObject:[self buildItemWithTitle:SysUTF16ToNSString( Nit: Extract the return value from -buildItem.. to a local var so that wrapping is less crazy. https://codereview.chromium.org/501273002/diff/400001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:691: [children Nit: Ditto. https://codereview.chromium.org/501273002/diff/400001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:706: *heading = SysUTF16ToNSString(prompt.GetPermissionsHeading(type)); Seems your calling code doesn't use the return bool value of this function, so how about just returning a NSString* heading instead of the bool and then you can remove the out param.
Not many comments on the code, but more on current coverage and future consistency. Is this flag the only flag that reduces permissions in this way? It would be weird to do this only for a subset of permissions... Also, do we feel confident that new permissions like this that are added in the future will get the same treatment? It is not like people adding flags will be thinking of the install dialog, right? https://codereview.chromium.org/501273002/diff/400001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/501273002/diff/400001/chrome/app/generated_re... chrome/app/generated_resources.grd:4190: + Warning: You've enabled the --scripts-require-action flag which limits the capabilities on this computer. However, other devices may not support this flag or have it enabled. On these computers, this extension can also: It looks like, according to your screenshot, that this string doesn't all fit in the dialog on Mac (missing the last bit). https://codereview.chromium.org/501273002/diff/400001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.h (right): https://codereview.chromium.org/501273002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.h:87: enum PermissionsType { Can you document these two enums? https://codereview.chromium.org/501273002/diff/400001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt_unittest.cc (right): https://codereview.chromium.org/501273002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt_unittest.cc:59: 1u, // regular_permissions_count nit: Think I'd prefer: // |regular_permissions_count|. (same below, three more occurrences).
On 2014/09/15 11:45:29, Finnur wrote: > Not many comments on the code, but more on current coverage and future > consistency. > > Is this flag the only flag that reduces permissions in this way? It would be > weird to do this only for a subset of permissions... Right now, yes, it is. It's a fairly new and experimental idea, so no other switches (or existing behaviors) do it. If, in the future, other switches features/do, the underlying structure (withheld permissions) is abstract enough to compensate, so the future change would be to change the message. > Also, do we feel confident that new permissions like this that are added in the > future will get the same treatment? It is not like people adding flags will be > thinking of the install dialog, right? Not entirely sure I follow what you mean about "new permissions like this"? If you mean new features that withhold permissions, then they can use the same method we do for these (withheld permissions), and it will "just work" (modulo modifying the install message).
Yes, I mean new features the withhold permissions. I know they can use the same framework to get their message in. My question was more: Will they realize they need to. This isn't compile time enforced (that with-held permissions should add a line in the install dialog).
On 2014/09/15 15:58:27, Finnur wrote: > Yes, I mean new features the withhold permissions. > > I know they can use the same framework to get their message in. My question was > more: Will they realize they need to. This isn't compile time enforced (that > with-held permissions should add a line in the install dialog). Bah... THAT withhold. Not 'the withhold'.
On 2014/09/15 15:58:52, Finnur wrote: > On 2014/09/15 15:58:27, Finnur wrote: > > Yes, I mean new features the withhold permissions. > > > > I know they can use the same framework to get their message in. My question > was > > more: Will they realize they need to. This isn't compile time enforced (that > > with-held permissions should add a line in the install dialog). > > Bah... THAT withhold. Not 'the withhold'. There's actually nothing in this patch that relates to the feature - it's all relating to whether or not the extension has withheld permissions. So if any other feature withholds extension permissions, the install dialog will reflect with no additional work.
Alexei, can you give me a quick hand with the prompt not showing the full withheld warning? (see screenshot and finnur's comment). I took a look and tried to figure out what's going wrong and it looks like the full title is there, but isn't being displayed completely. I'm pretty unfamiliar with objective C, so I'm having some trouble figuring out what's causing this. Your help would be much appreciated :) Also, I ran git cl format since I'm not confident in my objective C formatting. Please let me know if anything's formatted oddly and I'll fix it. https://codereview.chromium.org/501273002/diff/400001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/501273002/diff/400001/chrome/app/generated_re... chrome/app/generated_resources.grd:4190: + Warning: You've enabled the --scripts-require-action flag which limits the capabilities on this computer. However, other devices may not support this flag or have it enabled. On these computers, this extension can also: On 2014/09/15 11:45:28, Finnur wrote: > It looks like, according to your screenshot, that this string doesn't all fit in > the dialog on Mac (missing the last bit). Ah, good catch. I didn't notice since it still said "this extension can" followed by permissions. https://codereview.chromium.org/501273002/diff/400001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt.h (right): https://codereview.chromium.org/501273002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt.h:87: enum PermissionsType { On 2014/09/15 11:45:28, Finnur wrote: > Can you document these two enums? Sure thing. I'm not really sure what to say about DetailsType that isn't self-explanatory, though. It's really only used for getting details for permissions or retained files. I'll add a comment and we can change/remove it as you see fit. https://codereview.chromium.org/501273002/diff/400001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt_unittest.cc (right): https://codereview.chromium.org/501273002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt_unittest.cc:59: 1u, // regular_permissions_count On 2014/09/15 11:45:28, Finnur wrote: > nit: Think I'd prefer: > // |regular_permissions_count|. (same below, three more occurrences). Done. https://codereview.chromium.org/501273002/diff/400001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm (right): https://codereview.chromium.org/501273002/diff/400001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:682: addObject:[self buildItemWithTitle:SysUTF16ToNSString( On 2014/09/13 04:01:30, Alexei Svitkine wrote: > Nit: Extract the return value from -buildItem.. to a local var so that wrapping > is less crazy. Done. https://codereview.chromium.org/501273002/diff/400001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:691: [children On 2014/09/13 04:01:30, Alexei Svitkine wrote: > Nit: Ditto. Done. https://codereview.chromium.org/501273002/diff/400001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:706: *heading = SysUTF16ToNSString(prompt.GetPermissionsHeading(type)); On 2014/09/13 04:01:30, Alexei Svitkine wrote: > Seems your calling code doesn't use the return bool value of this function, so > how about just returning a NSString* heading instead of the bool and then you > can remove the out param. Done.
Have you tried calling setAutoresizesOutlineColumn: on the outline view? https://codereview.chromium.org/501273002/diff/420001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm (right): https://codereview.chromium.org/501273002/diff/420001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:59: - (NSString*)appendPermissionsToPrompt:(const ExtensionInstallPrompt::Prompt&) Can you add a comment documenting this function? I know this file has bad precedent for it and doesn't look like it has the comments, but since this function has a non-trivial amount of params and a return value, it's better to describe how it works. Put the comment above this function declaration. https://codereview.chromium.org/501273002/diff/420001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:60: prompt withType:(ExtensionInstallPrompt::PermissionsType)type I don't think git cl format works well for ObjC. How about this formatting: - (NSString*) appendPermissionsToPrompt:(const ExtensionInstallPrompt::Prompt&)prompt withType:(ExtensionInstallPrompt::PermissionsType)type children:(NSMutableArray*)children; (That way, all the :'s are aligned and you only wrap at the return value, which is sometimes done in C++ too). https://codereview.chromium.org/501273002/diff/420001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:615: withheldHeading = [self Nit: I'd wrap the same way as the line above. https://codereview.chromium.org/501273002/diff/420001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:672: prompt withType:(ExtensionInstallPrompt::PermissionsType)type Use same wrapping I suggested above. https://codereview.chromium.org/501273002/diff/420001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:679: NSDictionary* item = [self Nit: I'd wrap after the = if it still lets you align :'s. https://codereview.chromium.org/501273002/diff/420001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:699: addObject:[self buildDetailToggleItem:type permissionsDetailIndex:i]]; Nit: I'd wrap after the first : here instead.
https://codereview.chromium.org/501273002/diff/400001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt_unittest.cc (right): https://codereview.chromium.org/501273002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt_unittest.cc:59: 1u, // regular_permissions_count ... with the period at the end. :)
On 2014/09/15 16:05:22, Devlin wrote: > On 2014/09/15 15:58:52, Finnur wrote: > > On 2014/09/15 15:58:27, Finnur wrote: > > > Yes, I mean new features the withhold permissions. > > > > > > I know they can use the same framework to get their message in. My question > > was > > > more: Will they realize they need to. This isn't compile time enforced (that > > > with-held permissions should add a line in the install dialog). > > > > Bah... THAT withhold. Not 'the withhold'. > > There's actually nothing in this patch that relates to the feature - it's all > relating to whether or not the extension has withheld permissions. So if any > other feature withholds extension permissions, the install dialog will reflect > with no additional work. Thanks Devlin, that's what I wanted to confirm.
And meant to add... LGTM.
On 2014/09/15 20:48:16, Alexei Svitkine wrote: > Have you tried calling setAutoresizesOutlineColumn: on the outline view? I gave that a try, but no dice. I tried adjusting offsets for the outline view and window size as well and got nothing. Hovering over the line shows the entire title in a yellow box, if that helps at all. https://codereview.chromium.org/501273002/diff/400001/chrome/browser/extensio... File chrome/browser/extensions/extension_install_prompt_unittest.cc (right): https://codereview.chromium.org/501273002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_install_prompt_unittest.cc:59: 1u, // regular_permissions_count On 2014/09/16 10:18:11, Finnur (OOO until Sept 23) wrote: > ... with the period at the end. :) Oops! Wasn't sure if that period was meant to be added in with the comment. Done! :) https://codereview.chromium.org/501273002/diff/420001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm (right): https://codereview.chromium.org/501273002/diff/420001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:59: - (NSString*)appendPermissionsToPrompt:(const ExtensionInstallPrompt::Prompt&) On 2014/09/15 20:48:16, Alexei Svitkine wrote: > Can you add a comment documenting this function? I know this file has bad > precedent for it and doesn't look like it has the comments, but since this > function has a non-trivial amount of params and a return value, it's better to > describe how it works. Put the comment above this function declaration. Done. https://codereview.chromium.org/501273002/diff/420001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:60: prompt withType:(ExtensionInstallPrompt::PermissionsType)type On 2014/09/15 20:48:16, Alexei Svitkine wrote: > I don't think git cl format works well for ObjC. > > How about this formatting: > > - (NSString*) > appendPermissionsToPrompt:(const ExtensionInstallPrompt::Prompt&)prompt > withType:(ExtensionInstallPrompt::PermissionsType)type > children:(NSMutableArray*)children; > > (That way, all the :'s are aligned and you only wrap at the return value, which > is sometimes done in C++ too). Done. https://codereview.chromium.org/501273002/diff/420001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:615: withheldHeading = [self On 2014/09/15 20:48:16, Alexei Svitkine wrote: > Nit: I'd wrap the same way as the line above. Unfortunately this one is 1 character too long to be formatted the same way :( That must be why git cl format did it this way. Are we okay with wrapping before :: ? Like: ExtensionInstallPrompt::PermissionsType ::WITHHELD_PERMISSIONS Unfortunately it doesn't fit with the colons after PermissionsType either. https://codereview.chromium.org/501273002/diff/420001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:672: prompt withType:(ExtensionInstallPrompt::PermissionsType)type On 2014/09/15 20:48:16, Alexei Svitkine wrote: > Use same wrapping I suggested above. Done. https://codereview.chromium.org/501273002/diff/420001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:679: NSDictionary* item = [self On 2014/09/15 20:48:16, Alexei Svitkine wrote: > Nit: I'd wrap after the = if it still lets you align :'s. Not without wrapping the line, unfortunately (it's 82 characters long). Should I keep it the way it is, or format it differently? https://codereview.chromium.org/501273002/diff/420001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:699: addObject:[self buildDetailToggleItem:type permissionsDetailIndex:i]]; On 2014/09/15 20:48:16, Alexei Svitkine wrote: > Nit: I'd wrap after the first : here instead. Done.
I dug more into this header cutoff issue, and I think there may be a bug with cocoa. I updated the header to be longer (and a little more concise), and now it wraps properly onto the next line. It seems like there's some issue with trailing characters getting cut off in particular situations. I tried a handful of things to manipulate the dataCell holding the header, and nothing else worked. In any case, I updated the screenshots to show the new header, so it should be good to go once the cocoa changes are satisfactory :)
Ping :)
Cocoa code looks good % remaining comments. I'm worried about the fact that you simply chose a different wording for the message which "fixed" the problem. I think we should figure out what the actual cause of it and how to fix it properly. Else, we'll get the same problems when the strings are localized. https://codereview.chromium.org/501273002/diff/460001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm (right): https://codereview.chromium.org/501273002/diff/460001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:63: appendPermissionsToPrompt:(const ExtensionInstallPrompt::Prompt&) prompt Sorry after, reading your description above, I actually suggest slightly tweaking the method name to: -appendPermissionsForPrompt: withType: children: https://codereview.chromium.org/501273002/diff/460001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:678: appendPermissionsToPrompt:(const ExtensionInstallPrompt::Prompt&) prompt Nit: No space before prompt, same above.
On 2014/09/18 20:27:54, Alexei Svitkine wrote: > Cocoa code looks good % remaining comments. > > I'm worried about the fact that you simply chose a different wording for the > message which "fixed" the problem. I think we should figure out what the actual > cause of it and how to fix it properly. Else, we'll get the same problems when > the strings are localized. Agreed. Unfortunately, I don't have the time to look further into it. I tried changing properties of the dataCell attributed with the header, but everything already seemed appropriate (wrapping, no truncating), and hovering over the header pops up the full header in a yellow box. After some time, I got stumped.
https://codereview.chromium.org/501273002/diff/460001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm (right): https://codereview.chromium.org/501273002/diff/460001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:63: appendPermissionsToPrompt:(const ExtensionInstallPrompt::Prompt&) prompt On 2014/09/18 20:27:54, Alexei Svitkine wrote: > Sorry after, reading your description above, I actually suggest slightly > tweaking the method name to: > > -appendPermissionsForPrompt: > withType: > children: Done. https://codereview.chromium.org/501273002/diff/460001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:678: appendPermissionsToPrompt:(const ExtensionInstallPrompt::Prompt&) prompt On 2014/09/18 20:27:54, Alexei Svitkine wrote: > Nit: No space before prompt, same above. Done.
Cocoa code LGTM, but I'll defer to the extension reviewers for the judgement on the cutoff issue and the new wording of the warning. If they're OK with this CL landing with that potential issue (when the text is translated) and doesn't get as lucky), then you can go ahead. (But probably worth filing a follow-up bug for.)
On 2014/09/18 20:41:36, Alexei Svitkine wrote: > Cocoa code LGTM, but I'll defer to the extension reviewers for the judgement on > the cutoff issue and the new wording of the warning. > > If they're OK with this CL landing with that potential issue (when the text is > translated) and doesn't get as lucky), then you can go ahead. (But probably > worth filing a follow-up bug for.) This will be fine for now, since the issue sounds cocoa-related instead of related to this patch, and these warnings only show up with the experimental switch on. But please do file a bug (and cc me) so that we fix it. :)
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/501273002/480001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/62713) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/501273002/500001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/501273002/500001
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/501273002/500001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:
While running git apply --index -p1;
fatal: Unable to create
'/b/infra_internal/commit_queue/workdir/chromium/.git/index.lock': File exists.
If no other git process is currently running, this probably means a
git process crashed in this repository earlier. Make sure no other git
process is running and remove the file manually to continue.
Patch:
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc
Index:
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc
diff --git
a/chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc
b/chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc
index
f0f93788f74c928d13d33fb93da635e492a23c46..75d84d4855b58d12b8b115a323ba8aa70bce6e9a
100644
---
a/chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc
+++
b/chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc
@@ -129,12 +129,14 @@ void
ExtensionInstallDialogViewTestBase::SetUpOnMainThread() {
void ExtensionInstallDialogViewTestBase::SetPromptPermissions(
std::vector<base::string16> permissions) {
- prompt_->SetPermissions(permissions);
+ prompt_->SetPermissions(permissions,
+ ExtensionInstallPrompt::REGULAR_PERMISSIONS);
}
void ExtensionInstallDialogViewTestBase::SetPromptDetails(
std::vector<base::string16> details) {
- prompt_->SetPermissionsDetails(details);
+ prompt_->SetPermissionsDetails(details,
+ ExtensionInstallPrompt::REGULAR_PERMISSIONS);
}
void ExtensionInstallDialogViewTestBase::SetPromptRetainedFiles(
Message was sent while issue was closed.
Committed patchset #22 (id:500001) as 9e29c42d5cb20c84ffc026c96e0abd685fec036f
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/0fbac4d6271f2eaacd8d67800ba5dce7781578b4 Cr-Commit-Position: refs/heads/master@{#295773} |
