Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2821123002:
Differentiate new permissions from old in extension permission update bubble (Closed)
|
Created:
3 years, 8 months ago by catmullings Modified:
3 years, 7 months ago Reviewers:
Devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, srahim+watch_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDifferentiate new permissions from old in extension permission update bubble
The current extension permission update bubble puts both old and new
permissions together in one list. This CL changes the bubble to only show the
new permissions.
BUG=443216
Review-Url: https://codereview.chromium.org/2821123002
Cr-Commit-Position: refs/heads/master@{#468074}
Committed: https://chromium.googlesource.com/chromium/src/+/70e6ee11261bfa85cf090fc27336be04572e59b7
Patch Set 1 : Linux / Windows implementation #Patch Set 2 #
Total comments: 11
Patch Set 3 #
Total comments: 4
Patch Set 4 #Patch Set 5 : Git rebase master #
Messages
Total messages: 26 (16 generated)
The CQ bit was checked by catmullings@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by catmullings@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
catmullings@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Nice! Can we add some screenshots either linked here or on the bug? https://codereview.chromium.org/2821123002/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2821123002/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:3513: <message name="IDS_EXTENSION_DISABLED_ERROR_LABEL" desc="Text displayed when an extension was disabled due to a new upgrade requiring an explicit permission check from the user."> We no longer need two separate strings here. https://codereview.chromium.org/2821123002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_disabled_ui.cc (left): https://codereview.chromium.org/2821123002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_disabled_ui.cc:265: return l10n_util::GetStringUTF16(IDS_EXTENSIONS_UNINSTALL); I'm a little worried to re-open this to bikeshedding, but what about actually keeping the uninstall string instead of remove? I remember you brought up the good point that "remove" made it unclear if it were the extension or the permissions; "uninstall" might address that, while still being short? Maybe worth chatting with Shimi about quickly? I'd be fine either way; just thought I'd mention it. https://codereview.chromium.org/2821123002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_disabled_ui.cc (right): https://codereview.chromium.org/2821123002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_disabled_ui.cc:219: std::unique_ptr<const extensions::PermissionSet> granted = no need for extensions:: prefix (this whole file) https://codereview.chromium.org/2821123002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/2821123002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:205: extensions::ExtensionPrefs* extension_prefs() { return extension_prefs_; } We should probably just let folks get ExtensionPrefs through the ExtensionPrefs::Get() method. https://codereview.chromium.org/2821123002/diff/20001/extensions/common/permi... File extensions/common/permissions/permissions_data.h (right): https://codereview.chromium.org/2821123002/diff/20001/extensions/common/permi... extensions/common/permissions/permissions_data.h:145: PermissionMessages GetPermissionMessages( I wonder if this would be cleaner as GetNewPermissionMessages(const PermissionSet& granted_permissions), and have this to the diff logic. That way, external callers don't have to worry about grabbing the active_permissions from this class only to pass them right back in. So the call site would look something like: PermissionMessages messages = extension->permissions_data()->GetNewPermissionMessages(*granted_permissions); WDYT?
https://codereview.chromium.org/2821123002/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2821123002/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:3513: <message name="IDS_EXTENSION_DISABLED_ERROR_LABEL" desc="Text displayed when an extension was disabled due to a new upgrade requiring an explicit permission check from the user."> On 2017/04/19 19:18:33, Devlin wrote: > We no longer need two separate strings here. Done. https://codereview.chromium.org/2821123002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_disabled_ui.cc (left): https://codereview.chromium.org/2821123002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_disabled_ui.cc:265: return l10n_util::GetStringUTF16(IDS_EXTENSIONS_UNINSTALL); On 2017/04/19 19:18:33, Devlin wrote: > I'm a little worried to re-open this to bikeshedding, but what about actually > keeping the uninstall string instead of remove? I remember you brought up the > good point that "remove" made it unclear if it were the extension or the > permissions; "uninstall" might address that, while still being short? Maybe > worth chatting with Shimi about quickly? > > I'd be fine either way; just thought I'd mention it. Sure, will look into it. https://codereview.chromium.org/2821123002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_disabled_ui.cc (right): https://codereview.chromium.org/2821123002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_disabled_ui.cc:219: std::unique_ptr<const extensions::PermissionSet> granted = On 2017/04/19 19:18:33, Devlin wrote: > no need for extensions:: prefix (this whole file) Done. https://codereview.chromium.org/2821123002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/2821123002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:205: extensions::ExtensionPrefs* extension_prefs() { return extension_prefs_; } On 2017/04/19 19:18:33, Devlin wrote: > We should probably just let folks get ExtensionPrefs through the > ExtensionPrefs::Get() method. Done. https://codereview.chromium.org/2821123002/diff/20001/extensions/common/permi... File extensions/common/permissions/permissions_data.h (right): https://codereview.chromium.org/2821123002/diff/20001/extensions/common/permi... extensions/common/permissions/permissions_data.h:145: PermissionMessages GetPermissionMessages( On 2017/04/19 19:18:33, Devlin wrote: > I wonder if this would be cleaner as GetNewPermissionMessages(const > PermissionSet& granted_permissions), and have this to the diff logic. That way, > external callers don't have to worry about grabbing the active_permissions from > this class only to pass them right back in. > > So the call site would look something like: > PermissionMessages messages = > extension->permissions_data()->GetNewPermissionMessages(*granted_permissions); > > WDYT? Yup, that's sounds good. Will make that change. https://codereview.chromium.org/2821123002/diff/20001/extensions/common/permi... extensions/common/permissions/permissions_data.h:145: PermissionMessages GetPermissionMessages( On 2017/04/19 19:18:33, Devlin wrote: > I wonder if this would be cleaner as GetNewPermissionMessages(const > PermissionSet& granted_permissions), and have this to the diff logic. That way, > external callers don't have to worry about grabbing the active_permissions from > this class only to pass them right back in. > > So the call site would look something like: > PermissionMessages messages = > extension->permissions_data()->GetNewPermissionMessages(*granted_permissions); > > WDYT? Done.
lgtm! On 2017/04/19 19:18:34, Devlin wrote: > Can we add some screenshots either linked here or on the bug? ^^ Don't forget this. :) https://codereview.chromium.org/2821123002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_disabled_ui.cc (right): https://codereview.chromium.org/2821123002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_disabled_ui.cc:55: using extensions::Extension; nit: now we can remove these "using" lines. https://codereview.chromium.org/2821123002/diff/40001/extensions/common/permi... File extensions/common/permissions/permissions_data.h (right): https://codereview.chromium.org/2821123002/diff/40001/extensions/common/permi... extensions/common/permissions/permissions_data.h:144: // display at install time, in a nested format ready for display. We should differentiate this from the other method, e.g.: // Returns the list of permission details for permissions that are included in active_permissions(), but not present in |granted_permissions|. These are returned in a nested format, ready for display.
> On 2017/04/19 19:18:34, Devlin wrote: > > Can we add some screenshots either linked here or on the bug? Screenshots of the bubble can be found here: https://bugs.chromium.org/p/chromium/issues/detail?id=443216#c33
https://codereview.chromium.org/2821123002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_disabled_ui.cc (right): https://codereview.chromium.org/2821123002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_disabled_ui.cc:55: using extensions::Extension; On 2017/04/20 16:40:53, Devlin wrote: > nit: now we can remove these "using" lines. Done. https://codereview.chromium.org/2821123002/diff/40001/extensions/common/permi... File extensions/common/permissions/permissions_data.h (right): https://codereview.chromium.org/2821123002/diff/40001/extensions/common/permi... extensions/common/permissions/permissions_data.h:144: // display at install time, in a nested format ready for display. On 2017/04/20 16:40:53, Devlin wrote: > We should differentiate this from the other method, e.g.: > // Returns the list of permission details for permissions that are included in > active_permissions(), but not present in |granted_permissions|. These are > returned in a nested format, ready for display. Done.
The CQ bit was checked by catmullings@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2821123002/#ps60001 (title: "")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by catmullings@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2821123002/#ps80001 (title: "Git rebase master")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493400930531300,
"parent_rev": "80ca9270c0de1eb57260a76d1d91caecf6bfc47e", "commit_rev":
"70e6ee11261bfa85cf090fc27336be04572e59b7"}
Message was sent while issue was closed.
Description was changed from ========== Differentiate new permissions from old in extension permission update bubble The current extension permission update bubble puts both old and new permissions together in one list. This CL changes the bubble to only show the new permissions. BUG=443216 ========== to ========== Differentiate new permissions from old in extension permission update bubble The current extension permission update bubble puts both old and new permissions together in one list. This CL changes the bubble to only show the new permissions. BUG=443216 Review-Url: https://codereview.chromium.org/2821123002 Cr-Commit-Position: refs/heads/master@{#468074} Committed: https://chromium.googlesource.com/chromium/src/+/70e6ee11261bfa85cf090fc27336... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/70e6ee11261bfa85cf090fc27336... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
