Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2206693002:
Improve settings override bubble to indicate policy installed extensions (Closed)
|
Created:
4 years, 4 months ago by catmullings Modified:
4 years, 1 month ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, tfarina, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImprove settings override bubble to indicate policy installed extensions
Pertaining to settings override bubbles, remove button to
"Restore settings" and add a badge indicating that the extension was
installed by an administrator.
BUG=630734
Committed: https://crrev.com/22bc237867193148a4a4e14ac925a41905fff817
Cr-Commit-Position: refs/heads/master@{#429376}
Patch Set 1 : Non-Mac implementation of badge/extra view, that is, admin icon and text #
Total comments: 31
Patch Set 2 : Addressed patch 1 code review #
Total comments: 42
Patch Set 3 : Addressed patch 2 code review #Patch Set 4 : Addressed patch 3 code review #
Total comments: 17
Patch Set 5 : Addressed path 4 code review; initial attempt to remove "Restore Settings" button #
Total comments: 15
Patch Set 6 : Tests added; Addressed code review comments patch 5 #
Total comments: 6
Patch Set 7 : Fixed failing tests ToolbarActionsBarBubbleViewsTest.TestClickLearnMoreLink and TestBubbleLayoutAct… #
Total comments: 31
Patch Set 8 : Fixes and tests after bubble screenshot verification #
Total comments: 3
Patch Set 9 : Mac implementation with test #
Total comments: 36
Patch Set 10 #Patch Set 11 : Rebase master #
Total comments: 28
Patch Set 12 #
Total comments: 2
Patch Set 13 : Rebase master. Addressing mac code review #
Total comments: 19
Patch Set 14 #Messages
Total messages: 121 (83 generated)
catmullings@chromium.org changed reviewers: + rdcronin@google.com
Initial implementation of extra view / badge on non-mac platforms. Will follow up on removing the "Restore settings" button in the popup for policy extensions.
rdevlin.cronin@chromium.org changed reviewers: + rdevlin.cronin@chromium.org - rdcronin@google.com
Nice! A few other high-level thoughts. - We should add tests. :) - Since this is cross-platform code, it'd probably be best to implement this on mac at the same time. If we want to split up the patch sets, we can, but it'll need to be explicitly segmented in the code. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... File chrome/browser/ui/extensions/blocked_action_bubble_delegate.cc (right): https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... chrome/browser/ui/extensions/blocked_action_bubble_delegate.cc:70: return NULL; prefer nullptr in new code. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... File chrome/browser/ui/extensions/blocked_action_bubble_delegate.h (right): https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... chrome/browser/ui/extensions/blocked_action_bubble_delegate.h:31: GetExtraViewInfo() override; I think this is incorrect indentation, but git cl format changes a lot - has this been formatted? https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... File chrome/browser/ui/extensions/extension_message_bubble_bridge.cc (right): https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:11: #include "chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h" This is included in the .h; no need for it here. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:87: extensions::ExtensionIdList list = controller_->GetExtensionIdList(); const & to avoid a copy. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:92: std::unique_ptr<ToolbarActionsBarBubbleDelegate::ExtraViewInfo> nit: since this is in the scope of the bridge class, which implements ToolbarActionsBarBubbleDelegate, we can actually just refer to this as "ExtraViewInfo" https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:99: id, extensions::ExtensionRegistry::EVERYTHING); I think we should only be worried about enabled extensions here. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:115: // TODO: Stop after first policy extension encountered. Figure out how For this first version, I think it's reasonable to *only* show the policy indication on warnings that are for a specific extension. It's strictly better than the current version, and it ensures we never e.g. imply everything is policy installed when only one extension is. My recommendation would be: - Add a method on ExtensionMessageBubbleDelegate for CanBePolicyInstalled() (not attached to that name - feel free to come up with something better) that returns true for the bubbles we're concerned about (proxy, settings, new tab page). - In the case of those bubbles, there should only ever be a single extension. In that case, we can here do something like: DCHECK_EQ(1u, list.size()); const extensions::Extension* extension = extensions::ExtensionRegistry::Get(controller_->profile())->enabled_extensions().GetByID(list[0]); if (extensions::Manifest::IsPolicyLocation(extension->location())) { ... extra view stuff. } WDYT? https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:119: return extra_view_info; We still need to handle the case where the extension isn't policy installed and we should show the learn more link. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/toolbar/t... File chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h (right): https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:11: #include "ui/views/controls/image_view.h" This is a layering violation, since this class is used on all platforms. The views-specific implementation needs to only be in files in c/b/ui/views/. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:23: struct ExtraViewInfo { Document this struct and its fields. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:24: views::ImageView* icon; So instead of ImageView here, we'd want to use a gfx::Image or int resource_id. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:75: virtual std::unique_ptr<ExtraViewInfo> GetExtraViewInfo() = 0; Add a comment explaining this method. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:47: return nullptr; What if we wanted to *just* show an icon, with no text? We don't necessarily need to support that yet, but it seems logical to allow. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:55: // If not, should I still instantiate the learn_more_button_? right now, it would be, and keeping it like this isn't terrible. But I'd probably recommend renaming this to be "link_" or something similar instead of "learn_more_button_", which would alleviate the concern. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:59: return button; What about the case of a link + an icon?
Addressed patch 1 code review comments. No test yet. Still need to remove the "Restore Session" button. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... File chrome/browser/ui/extensions/blocked_action_bubble_delegate.cc (right): https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... chrome/browser/ui/extensions/blocked_action_bubble_delegate.cc:70: return NULL; On 2016/08/02 20:34:05, Devlin (ooo aug12) wrote: > prefer nullptr in new code. Done. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... File chrome/browser/ui/extensions/blocked_action_bubble_delegate.h (right): https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... chrome/browser/ui/extensions/blocked_action_bubble_delegate.h:31: GetExtraViewInfo() override; On 2016/08/02 20:34:05, Devlin wrote: > I think this is incorrect indentation, but git cl format changes a lot - has > this been formatted? Yes, this is what git cl format produced. Though I noticed that other files have overflowed function definitions tab indented. Which is preferred? git cl format? https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... File chrome/browser/ui/extensions/extension_message_bubble_bridge.cc (right): https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:11: #include "chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h" On 2016/08/02 20:34:05, Devlin (ooo aug12) wrote: > This is included in the .h; no need for it here. Done. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:87: extensions::ExtensionIdList list = controller_->GetExtensionIdList(); On 2016/08/02 20:34:05, Devlin (ooo aug12) wrote: > const & to avoid a copy. Done. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:92: std::unique_ptr<ToolbarActionsBarBubbleDelegate::ExtraViewInfo> On 2016/08/02 20:34:05, Devlin (ooo aug12) wrote: > nit: since this is in the scope of the bridge class, which implements > ToolbarActionsBarBubbleDelegate, we can actually just refer to this as > "ExtraViewInfo" Done. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:99: id, extensions::ExtensionRegistry::EVERYTHING); On 2016/08/02 20:34:05, Devlin (ooo aug12) wrote: > I think we should only be worried about enabled extensions here. With the changes below, this is no longer relevant, right? Done. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:115: // TODO: Stop after first policy extension encountered. Figure out how On 2016/08/02 20:34:05, Devlin (ooo aug12) wrote: > For this first version, I think it's reasonable to *only* show the policy > indication on warnings that are for a specific extension. It's strictly better > than the current version, and it ensures we never e.g. imply everything is > policy installed when only one extension is. > > My recommendation would be: > - Add a method on ExtensionMessageBubbleDelegate for CanBePolicyInstalled() (not > attached to that name - feel free to come up with something better) that returns I don't mind the name CanBePolicyInstalled(); it describes the expected functionality. I named it IsPolicyInstallable() instead, but open to alternatives, including your suggested name. > true for the bubbles we're concerned about (proxy, settings, new tab page). > - In the case of those bubbles, there should only ever be a single extension. In > that case, we can here do something like: > DCHECK_EQ(1u, list.size()); Why do you suggest checking DCHECK_EQ(1u, list.size()); ? Wouldn't it be possible that list.size > 1? > const extensions::Extension* extension = > extensions::ExtensionRegistry::Get(controller_->profile())->enabled_extensions().GetByID(list[0]); > if (extensions::Manifest::IsPolicyLocation(extension->location())) { > ... extra view stuff. > } > > WDYT? Done. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:119: return extra_view_info; On 2016/08/02 20:34:05, Devlin (ooo aug12) wrote: > We still need to handle the case where the extension isn't policy installed and > we should show the learn more link. Done. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/toolbar/t... File chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h (right): https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:11: #include "ui/views/controls/image_view.h" On 2016/08/02 20:34:05, Devlin (ooo aug12) wrote: > This is a layering violation, since this class is used on all platforms. The > views-specific implementation needs to only be in files in c/b/ui/views/. Done. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:23: struct ExtraViewInfo { On 2016/08/02 20:34:06, Devlin (ooo aug12) wrote: > Document this struct and its fields. Done. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:24: views::ImageView* icon; On 2016/08/02 20:34:06, Devlin (ooo aug12) wrote: > So instead of ImageView here, we'd want to use a gfx::Image or int resource_id. Done. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:75: virtual std::unique_ptr<ExtraViewInfo> GetExtraViewInfo() = 0; On 2016/08/02 20:34:05, Devlin (ooo aug12) wrote: > Add a comment explaining this method. Done. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:47: return nullptr; On 2016/08/02 20:34:06, Devlin (ooo aug12) wrote: > What if we wanted to *just* show an icon, with no text? We don't necessarily > need to support that yet, but it seems logical to allow. Done. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:55: // If not, should I still instantiate the learn_more_button_? On 2016/08/02 20:34:06, Devlin (ooo aug12) wrote: > right now, it would be, and keeping it like this isn't terrible. But I'd > probably recommend renaming this to be "link_" or something similar instead of > "learn_more_button_", which would alleviate the concern. Done. https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:59: return button; On 2016/08/02 20:34:06, Devlin (ooo aug12) wrote: > What about the case of a link + an icon? Done. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/extension_message_bubble_bridge.cc (right): https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:84: std::unique_ptr<ToolbarActionsBarBubbleDelegate::ExtraViewInfo> When I remove the "ToolbarActionsBarBubbleDelegate::" from the declaration, I get a compiler error. C++ question: Why must it be left here, but can be left out when using the ExtraViewInfo struct within the method? https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:102: if (controller_->delegate()->IsPolicyInstallable() && Equivalent to CanBePolicyInstalled(). I didn't quite understand your code review comment wrt when to invoke this. I assumed it should be here, but it feels redundant, considering the next check on line 103 extensions::Manifest::IsPolicyLocation(extension->location()). Was this how you intended the check to be used? Why is this extra check for delegate()->IsPolicyInstallable() included, instead of only having the one on line 103? https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:109: extra_view_info->resource_id = -1; See my question/comment in chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h (right): https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:25: // The resource id referencing the image icon. If has a value of -1, then no What is the value for a resource_id when it won't be set? I assumed it would be -1. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:28: item_list_(nullptr), In the ExtraViewInfo struct, if the text is not meant to be a link (!is_text_linked) and just a regular views::Label, can I use this private member? The method item_list() (defined in the .h) is only used in test files.
https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... File chrome/browser/ui/extensions/extension_message_bubble_bridge.cc (right): https://codereview.chromium.org/2206693002/diff/1/chrome/browser/ui/extension... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:115: // TODO: Stop after first policy extension encountered. Figure out how On 2016/08/13 00:35:52, catmullings wrote: > On 2016/08/02 20:34:05, Devlin (ooo aug12) wrote: > > For this first version, I think it's reasonable to *only* show the policy > > indication on warnings that are for a specific extension. It's strictly > better > > than the current version, and it ensures we never e.g. imply everything is > > policy installed when only one extension is. > > > > My recommendation would be: > > - Add a method on ExtensionMessageBubbleDelegate for CanBePolicyInstalled() > (not > > attached to that name - feel free to come up with something better) that > returns > > I don't mind the name CanBePolicyInstalled(); it describes the expected > functionality. I named it IsPolicyInstallable() instead, but open to > alternatives, including your suggested name. > > > true for the bubbles we're concerned about (proxy, settings, new tab page). > > - In the case of those bubbles, there should only ever be a single extension. > In > > that case, we can here do something like: > > DCHECK_EQ(1u, list.size()); > > Why do you suggest checking DCHECK_EQ(1u, list.size()); ? Wouldn't it be > possible that list.size > 1? With the bubbles that this is relevant for, we should only ever be warning about a single extension. Which is important, because otherwise it's unclear what the UI is (i.e., how to say "Some of these N extensions are policy-installed, others aren't.") > > > const extensions::Extension* extension = > > > extensions::ExtensionRegistry::Get(controller_->profile())->enabled_extensions().GetByID(list[0]); > > if (extensions::Manifest::IsPolicyLocation(extension->location())) { > > ... extra view stuff. > > } > > > > WDYT? > > Done. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/blocked_action_bubble_delegate.cc (right): https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/blocked_action_bubble_delegate.cc:9: #include "grit/components_scaled_resources.h" needed? https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/blocked_action_bubble_delegate.cc:12: #include "ui/base/resource/resource_bundle.h" ditto https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/extension_message_bubble_bridge.cc (right): https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:84: std::unique_ptr<ToolbarActionsBarBubbleDelegate::ExtraViewInfo> On 2016/08/13 00:35:53, catmullings wrote: > When I remove the "ToolbarActionsBarBubbleDelegate::" from the declaration, I > get a compiler error. > > C++ question: Why must it be left here, but can be left out when using the > ExtraViewInfo struct within the method? Within the method, you are within the scope of the class, and therefore (normally) don't need to specify - it's the same reason you can (normally) access methods or members without the class specification. Outside of the class, though, the compiler wouldn't know which struct you're referring to, so it needs the full specification. *normally - sometimes, you might want to refer to a parent class's implementation, member, etc - then you would have to use the class specification. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:88: if (list.empty()) Can this happen? https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:91: DCHECK_EQ(1u, list.size()); This isn't quite true, right? Some bubbles can have more than one extension. This should go within the block for IsPolicyInstallable() (name pending) https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:100: std::unique_ptr<ExtraViewInfo> extra_view_info(new ExtraViewInfo); nit: It doesn't really matter with classes, but I slightly prefer explicit construction (i.e., new ExtraViewInfo() - note the added parens) https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:102: if (controller_->delegate()->IsPolicyInstallable() && On 2016/08/13 00:35:53, catmullings wrote: > Equivalent to CanBePolicyInstalled(). > > I didn't quite understand your code review comment wrt when to invoke this. I > assumed it should be here, but it feels redundant, considering the next check on > line 103 extensions::Manifest::IsPolicyLocation(extension->location()). > > Was this how you intended the check to be used? > Why is this extra check for delegate()->IsPolicyInstallable() included, instead > of only having the one on line 103? CanBePolicyInstalled/IsPolicyInstallable aren't great names for what we're trying to capture (I threw it up there as a strawman). At a high level, we only want to show this extra decoration for specific bubbles - ones where we expect that there could be policy-installed extensions, and where there's only one controlling extension. The method on the delegate would be to indicate that the bubble is of that type. We don't want to just deduce that on the offchance that another bubble sometimes satisfies that criteria, but isn't guaranteed to. Does that make sense? https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:109: extra_view_info->resource_id = -1; Let's have defaults like this handled by an ExtraViewInfo ctor. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h (right): https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:25: // The resource id referencing the image icon. If has a value of -1, then no On 2016/08/13 00:35:53, catmullings wrote: > What is the value for a resource_id when it won't be set? I assumed it would be > -1. Yep, -1. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:28: item_list_(nullptr), On 2016/08/13 00:35:53, catmullings wrote: > In the ExtraViewInfo struct, if the text is not meant to be a link > (!is_text_linked) and just a regular views::Label, can I use this private > member? The method item_list() (defined in the .h) is only used in test files. We shouldn't. Links have extra behavior, and you can't assign a Label to a Link. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:44: base::string16 text = extra_view_info->text; we need to check if extra_view_info is null, right? https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:44: base::string16 text = extra_view_info->text; Let's also avoid the copy of the base::string16. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:47: if (text.empty() && resource_id != -1) { Why do we return if resource_id != -1? https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:61: if (text.empty()) { it looks like there's a lot of duplicate checking here for text.empty() and resource_id [!,=]= -1, and the many different returns from various branches also make it a bit confusing. Can we clean it up a bit to make it more readable?
Addressed patch 2 code review. One comment to follow up in person. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/blocked_action_bubble_delegate.cc (right): https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/blocked_action_bubble_delegate.cc:9: #include "grit/components_scaled_resources.h" On 2016/08/22 20:59:41, Devlin wrote: > needed? Done. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/blocked_action_bubble_delegate.cc:12: #include "ui/base/resource/resource_bundle.h" On 2016/08/22 20:59:41, Devlin wrote: > ditto Done. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/extension_message_bubble_bridge.cc (right): https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:84: std::unique_ptr<ToolbarActionsBarBubbleDelegate::ExtraViewInfo> On 2016/08/22 20:59:41, Devlin wrote: > On 2016/08/13 00:35:53, catmullings wrote: > > When I remove the "ToolbarActionsBarBubbleDelegate::" from the declaration, I > > get a compiler error. > > > > C++ question: Why must it be left here, but can be left out when using the > > ExtraViewInfo struct within the method? > > Within the method, you are within the scope of the class, and therefore > (normally) don't need to specify - it's the same reason you can (normally) > access methods or members without the class specification. Outside of the > class, though, the compiler wouldn't know which struct you're referring to, so > it needs the full specification. > > *normally - sometimes, you might want to refer to a parent class's > implementation, member, etc - then you would have to use the class > specification. Done. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:88: if (list.empty()) On 2016/08/22 20:59:41, Devlin wrote: > Can this happen? W/o digging too deep into the stack, yes, that line can return true if the following happens: https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_mess... https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:91: DCHECK_EQ(1u, list.size()); On 2016/08/22 20:59:41, Devlin wrote: > This isn't quite true, right? Some bubbles can have more than one extension. > This should go within the block for IsPolicyInstallable() (name pending) Yup, originally I had planned to put it in IsPolicyInstallable(), but GetExtensionIdList() is a method on the controller class, not the delegate. The delegate class doesn't have a similar a GetExtensionIdList method because it only contains a single extension_id as a private member. Given that there would only be a single extension_id associated with a delegate, is it necessary to enforce the DCHECK_EQ then? https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:100: std::unique_ptr<ExtraViewInfo> extra_view_info(new ExtraViewInfo); On 2016/08/22 20:59:41, Devlin wrote: > nit: It doesn't really matter with classes, but I slightly prefer explicit > construction (i.e., new ExtraViewInfo() - note the added parens) When/why is explicit construction better than using a smart pointer? Particularly, in this case. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:102: if (controller_->delegate()->IsPolicyInstallable() && On 2016/08/22 20:59:41, Devlin wrote: > On 2016/08/13 00:35:53, catmullings wrote: > > Equivalent to CanBePolicyInstalled(). > > > > I didn't quite understand your code review comment wrt when to invoke this. I > > assumed it should be here, but it feels redundant, considering the next check > on > > line 103 extensions::Manifest::IsPolicyLocation(extension->location()). > > > > Was this how you intended the check to be used? > > Why is this extra check for delegate()->IsPolicyInstallable() included, > instead > > of only having the one on line 103? > > CanBePolicyInstalled/IsPolicyInstallable aren't great names for what we're > trying to capture (I threw it up there as a strawman). At a high level, we only > want to show this extra decoration for specific bubbles - ones where we expect > that there could be policy-installed extensions, and where there's only one > controlling extension. The method on the delegate would be to indicate that the > bubble is of that type. What you say makes sense, but I'm don't understand why the name CanBePolicyInstalled/IsPolicyInstallable doesn't capture this rationale. Particularly when you say the following: > We don't want to just deduce that on the offchance that > another bubble sometimes satisfies that criteria, but isn't guaranteed to. Isn't that the purpose of saying "can be" policy installed? > Does that make sense? https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:109: extra_view_info->resource_id = -1; On 2016/08/22 20:59:41, Devlin wrote: > Let's have defaults like this handled by an ExtraViewInfo ctor. Done. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h (right): https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:25: // The resource id referencing the image icon. If has a value of -1, then no On 2016/08/22 20:59:41, Devlin wrote: > On 2016/08/13 00:35:53, catmullings wrote: > > What is the value for a resource_id when it won't be set? I assumed it would > be > > -1. > > Yep, -1. Done. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:28: item_list_(nullptr), On 2016/08/22 20:59:42, Devlin wrote: > On 2016/08/13 00:35:53, catmullings wrote: > > In the ExtraViewInfo struct, if the text is not meant to be a link > > (!is_text_linked) and just a regular views::Label, can I use this private > > member? The method item_list() (defined in the .h) is only used in test files. > > > We shouldn't. Links have extra behavior, and you can't assign a Label to a > Link. Done. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:44: base::string16 text = extra_view_info->text; On 2016/08/22 20:59:42, Devlin wrote: > Let's also avoid the copy of the base::string16. Done. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:44: base::string16 text = extra_view_info->text; On 2016/08/22 20:59:42, Devlin wrote: > we need to check if extra_view_info is null, right? Done. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:47: if (text.empty() && resource_id != -1) { On 2016/08/22 20:59:42, Devlin wrote: > Why do we return if resource_id != -1? Should be == . Done. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:61: if (text.empty()) { On 2016/08/22 20:59:42, Devlin wrote: > it looks like there's a lot of duplicate checking here for text.empty() and > resource_id [!,=]= -1, and the many different returns from various branches also > make it a bit confusing. Can we clean it up a bit to make it more readable? Will followup with you about this offline.
(Just responding) https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/extension_message_bubble_bridge.cc (right): https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:88: if (list.empty()) On 2016/08/31 00:03:30, catmullings wrote: > On 2016/08/22 20:59:41, Devlin wrote: > > Can this happen? > > W/o digging too deep into the stack, yes, that line can return true if the > following happens: > > https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_mess... But if that's true, ShouldShow() returns false, which means we should never get so far as constructing the bubble. Right? https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:91: DCHECK_EQ(1u, list.size()); On 2016/08/31 00:03:30, catmullings wrote: > On 2016/08/22 20:59:41, Devlin wrote: > > This isn't quite true, right? Some bubbles can have more than one extension. > > This should go within the block for IsPolicyInstallable() (name pending) > > Yup, originally I had planned to put it in IsPolicyInstallable(), but > GetExtensionIdList() is a method on the controller class, not the delegate. > > The delegate class doesn't have a similar a GetExtensionIdList method because it > only contains a single extension_id as a private member. Given that there would > only be a single extension_id associated with a delegate, is it necessary to > enforce the DCHECK_EQ then? I don't quite follow. A bubble can be shown for multiple extensions - take a look at the DevModeBubbleDelegate. The list returned from GetExtensionIdList() is constructed by looping over the set of extensions and adding any for which ShouldIncludeExtension() is true - which can be >1. Certain delegates, such as the settings override bubbles, are restricted to a single extension - but that's a property of the specific delegate, rather than a generic delegate or the bubble controller on the whole. Does that make sense? https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:100: std::unique_ptr<ExtraViewInfo> extra_view_info(new ExtraViewInfo); On 2016/08/31 00:03:30, catmullings wrote: > On 2016/08/22 20:59:41, Devlin wrote: > > nit: It doesn't really matter with classes, but I slightly prefer explicit > > construction (i.e., new ExtraViewInfo() - note the added parens) > > When/why is explicit construction better than using a smart pointer? > Particularly, in this case. They're orthogonal. Smart ptrs are ownership, but you still here construct the object with "new ExtraViewInfo". Explicit construction refers to the difference between new Foo and new Foo() both of which can be used in conjunction with a smart ptr (i.e., it would just be std::unique_ptr<Foo> foo(new Foo())). There's a decent stack overflow answer explaining the difference between those here: http://stackoverflow.com/questions/620137/do-the-parentheses-after-the-type-n... Now, in our code base, it doesn't usually make much difference, because we very rarely allow/encourage the use of compiler-generated constructors. But it's just a bit cleaner, IMO, to avoid the risk of leaving members uninitialized. All that said, we've also very recently (last week?) started preferring MakeUnique, so it might be best to just use that here. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:102: if (controller_->delegate()->IsPolicyInstallable() && On 2016/08/31 00:03:30, catmullings wrote: > What you say makes sense, but I'm don't understand why the name > CanBePolicyInstalled/IsPolicyInstallable doesn't capture this rationale. > > Particularly when you say the following: > > > We don't want to just deduce that on the offchance that > > another bubble sometimes satisfies that criteria, but isn't guaranteed to. > > Isn't that the purpose of saying "can be" policy installed? From reading the name, "CanBePolicyInstalled" implies one thing - that an extension for which the bubble is being shown could possibly be installed by enterprise policy. But it doesn't imply that there's only a single controlling extension, which, for this UI, is important. Concrete example: Potentially, "suspicious" (not in the webstore) extensions might be installed by policy, but there could be >1 of them, some of which are policy-installed and some of which aren't. "CanBePolicyInstalled" would return true for that - they could be policy installed - but we don't want to show the additional decoration in that case.
Description was changed from ========== Improve policy extension warning bubble Remove button to "Restore settings" and add a badge indicating that the extension was installed by an administrator. BUG=630734 ========== to ========== Pertaining to settings override bubbles, remove button to "Restore settings" and add a badge indicating that the extension was installed by an administrator. BUG=630734 ==========
Still need to remove the "Restore Settings" button, and then I will add a test. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/extension_message_bubble_bridge.cc (right): https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:88: if (list.empty()) On 2016/08/31 18:46:45, Devlin wrote: > On 2016/08/31 00:03:30, catmullings wrote: > > On 2016/08/22 20:59:41, Devlin wrote: > > > Can this happen? > > > > W/o digging too deep into the stack, yes, that line can return true if the > > following happens: > > > > > https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_mess... > > But if that's true, ShouldShow() returns false, which means we should never get > so far as constructing the bubble. Right? Done. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:91: DCHECK_EQ(1u, list.size()); On 2016/08/31 18:46:45, Devlin wrote: > On 2016/08/31 00:03:30, catmullings wrote: > > On 2016/08/22 20:59:41, Devlin wrote: > > > This isn't quite true, right? Some bubbles can have more than one > extension. > > > This should go within the block for IsPolicyInstallable() (name pending) > > > > Yup, originally I had planned to put it in IsPolicyInstallable(), but > > GetExtensionIdList() is a method on the controller class, not the delegate. > > > > The delegate class doesn't have a similar a GetExtensionIdList method because > it > > only contains a single extension_id as a private member. Given that there > would > > only be a single extension_id associated with a delegate, is it necessary to > > enforce the DCHECK_EQ then? > > I don't quite follow. A bubble can be shown for multiple extensions - take a > look at the DevModeBubbleDelegate. The list returned from GetExtensionIdList() > is constructed by looping over the set of extensions and adding any for which > ShouldIncludeExtension() is true - which can be >1. Certain delegates, such as > the settings override bubbles, are restricted to a single extension - but that's > a property of the specific delegate, rather than a generic delegate or the > bubble controller on the whole. Does that make sense? Fixed. Responding to explain and justify the change. Moved the DCHECK_EQ to inside the code block under the (temporarily named) IsPolicyInstallable() if-statement check below. In that case, we would only hit the DCHECK_EQ if IsPolicyInstallable() is true, which is the expected case for delegates/bubbles like Settings API, NTP, and proxy. Settings override delegates are are restricted to a single extension, so we can assert that list.size() == 1. Done. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:100: std::unique_ptr<ExtraViewInfo> extra_view_info(new ExtraViewInfo); On 2016/08/31 18:46:45, Devlin wrote: > On 2016/08/31 00:03:30, catmullings wrote: > > On 2016/08/22 20:59:41, Devlin wrote: > > > nit: It doesn't really matter with classes, but I slightly prefer explicit > > > construction (i.e., new ExtraViewInfo() - note the added parens) > > > > When/why is explicit construction better than using a smart pointer? > > Particularly, in this case. > > They're orthogonal. Smart ptrs are ownership, but you still here construct the > object with "new ExtraViewInfo". Explicit construction refers to the difference > between > new Foo > and > new Foo() > both of which can be used in conjunction with a smart ptr (i.e., it would just > be std::unique_ptr<Foo> foo(new Foo())). > > There's a decent stack overflow answer explaining the difference between those > here: > http://stackoverflow.com/questions/620137/do-the-parentheses-after-the-type-n... > > Now, in our code base, it doesn't usually make much difference, because we very > rarely allow/encourage the use of compiler-generated constructors. But it's > just a bit cleaner, IMO, to avoid the risk of leaving members uninitialized. > > All that said, we've also very recently (last week?) started preferring > MakeUnique, so it might be best to just use that here. Done. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:102: if (controller_->delegate()->IsPolicyInstallable() && On 2016/08/31 18:46:45, Devlin wrote: > On 2016/08/31 00:03:30, catmullings wrote: > > What you say makes sense, but I'm don't understand why the name > > CanBePolicyInstalled/IsPolicyInstallable doesn't capture this rationale. > > > > Particularly when you say the following: > > > > > We don't want to just deduce that on the offchance that > > > another bubble sometimes satisfies that criteria, but isn't guaranteed to. > > > > Isn't that the purpose of saying "can be" policy installed? > > From reading the name, "CanBePolicyInstalled" implies one thing - that an > extension for which the bubble is being shown could possibly be installed by > enterprise policy. But it doesn't imply that there's only a single controlling > extension, which, for this UI, is important. > > Concrete example: > Potentially, "suspicious" (not in the webstore) extensions might be installed by > policy, but there could be >1 of them, some of which are policy-installed and > some of which aren't. "CanBePolicyInstalled" would return true for that - they > could be policy installed - but we don't want to show the additional decoration > in that case. Done. https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/2206693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:61: if (text.empty()) { On 2016/08/31 00:03:31, catmullings wrote: > On 2016/08/22 20:59:42, Devlin wrote: > > it looks like there's a lot of duplicate checking here for text.empty() and > > resource_id [!,=]= -1, and the many different returns from various branches > also > > make it a bit confusing. Can we clean it up a bit to make it more readable? > > Will followup with you about this offline. What do you think of this change?
Great! This is looking a lot better. https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/extension_message_bubble_bridge.cc (right): https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:87: const extensions::ExtensionIdList& list = controller_->GetExtensionIdList(); Let's move list and extension into the block where use use them so we don't do any extra work in other cases. https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:93: if (!extension) I think we can DCHECK this in the block where we use it. https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h (right): https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:34: ExtraViewInfo() : resource_id(-1) {} nits: ctors before members, and this should be defined in the .cc file. Also, you'll need to init is_text_linked. https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:40: views::View* ToolbarActionsBarBubbleViews::CreateExtraView() { Nice! This method already reads a lot better than it did. https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:47: base::string16& text = extra_view_info->text; const& https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:66: ResourceBundle::GetSharedInstance().GetImageSkiaNamed(resource_id)); It still looks like we have a bit of duplicate code here. What if we did something like (starting at line 49): views::ImageView* icon = nullptr; if (resource_id != -1) { icon = new views::ImageView(); icon->SetImage(...); } // Note: this uses a label_ member in lieu of the link_ member. if (!text.empty()) { if (is_text_linked) { views::Link* link = new Link(text); link->set_listener(this); label_ = link; } else { label_ = new views::Label(text); } } if (icon && label;) { views::View* parent = new views::View(); parent->AddChildView(icon); parent->AddChildView(label_); return parent; } return icon ? icon : label_; This way, each item is only instantiated in one spot. https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:156: link_ = new views::Link(text); I think we can make link_ a views::Label* label_, and use it in both cases (views::Link inherits from views::Label).
Patchset #5 (id:80001) has been deleted
Addressed path 4 code review; initial attempt to remove "Restore Settings" button. https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/extension_message_bubble_bridge.cc (right): https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:87: const extensions::ExtensionIdList& list = controller_->GetExtensionIdList(); On 2016/09/07 18:37:49, Devlin wrote: > Let's move list and extension into the block where use use them so we don't do > any extra work in other cases. The list variable is used to construct the following extension variable, so it can't go inside the if-block below. https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:93: if (!extension) On 2016/09/07 18:37:49, Devlin wrote: > I think we can DCHECK this in the block where we use it. Converted this to a DCHECK. However, I can't quite tuck this into the if-block below b/c the extension variable is used to perform the check. Thoughts? Also, how do you know that we should enforce a DCHECK instead of return nullptr? https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h (right): https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:34: ExtraViewInfo() : resource_id(-1) {} On 2016/09/07 18:37:49, Devlin wrote: > nits: ctors before members, and this should be defined in the .cc file. Also, > you'll need to init is_text_linked. Leaving ctor here b/c there is no accompanying .cc file for this .h file. It's less harmful to leave the ctor here since it is initializing primitive types. https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:34: ExtraViewInfo() : resource_id(-1) {} On 2016/09/07 18:37:49, Devlin wrote: > nits: ctors before members, and this should be defined in the .cc file. Also, > you'll need to init is_text_linked. Done. https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:40: views::View* ToolbarActionsBarBubbleViews::CreateExtraView() { On 2016/09/07 18:37:50, Devlin wrote: > Nice! This method already reads a lot better than it did. Done. https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:47: base::string16& text = extra_view_info->text; On 2016/09/07 18:37:50, Devlin wrote: > const& Done. https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:66: ResourceBundle::GetSharedInstance().GetImageSkiaNamed(resource_id)); On 2016/09/07 18:37:49, Devlin wrote: > It still looks like we have a bit of duplicate code here. What if we did > something like (starting at line 49): > views::ImageView* icon = nullptr; > if (resource_id != -1) { > icon = new views::ImageView(); > icon->SetImage(...); > } > > // Note: this uses a label_ member in lieu of the link_ member. > if (!text.empty()) { > if (is_text_linked) { > views::Link* link = new Link(text); > link->set_listener(this); > label_ = link; > } else { > label_ = new views::Label(text); > } > } > > if (icon && label;) { > views::View* parent = new views::View(); > parent->AddChildView(icon); > parent->AddChildView(label_); > return parent; > } > > return icon ? icon : label_; > > This way, each item is only instantiated in one spot. This is much cleaner and easier to follow. Change is made. https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:66: ResourceBundle::GetSharedInstance().GetImageSkiaNamed(resource_id)); On 2016/09/07 18:37:49, Devlin wrote: > It still looks like we have a bit of duplicate code here. What if we did > something like (starting at line 49): > views::ImageView* icon = nullptr; > if (resource_id != -1) { > icon = new views::ImageView(); > icon->SetImage(...); > } > > // Note: this uses a label_ member in lieu of the link_ member. > if (!text.empty()) { > if (is_text_linked) { > views::Link* link = new Link(text); > link->set_listener(this); > label_ = link; > } else { > label_ = new views::Label(text); > } > } > > if (icon && label;) { > views::View* parent = new views::View(); > parent->AddChildView(icon); > parent->AddChildView(label_); > return parent; > } > > return icon ? icon : label_; > > This way, each item is only instantiated in one spot. Done. https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:156: link_ = new views::Link(text); On 2016/09/07 18:37:50, Devlin wrote: > I think we can make link_ a views::Label* label_, and use it in both cases > (views::Link inherits from views::Label). I removed this method since it is no longer need in CreateExtraView(). https://codereview.chromium.org/2206693002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:156: link_ = new views::Link(text); On 2016/09/07 18:37:50, Devlin wrote: > I think we can make link_ a views::Label* label_, and use it in both cases > (views::Link inherits from views::Label). Done. https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/ui/exte... File chrome/browser/ui/extensions/extension_message_bubble_bridge.cc (right): https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:47: const extensions::ExtensionIdList& list = controller_->GetExtensionIdList(); Lines 47 - 59 are obviously duplicated code from the GetExtraViewInfo() method below. If this is the proper way and place to check whether to remove the "Restore Settings" button, then I will tuck this code into a helper function. WDYT? https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:60: return base::string16(); Initial attempt to remove the "Restore Settings" button if an extension is policy installed. https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:73: if (icon) The line "return icon ? icon : label_;" results in a compile error regarding views::Label * and views::ImageView * being incompatible types, so I expanded the one line if statement.
https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_message_bubble_controller.h (right): https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_message_bubble_controller.h:90: // Returns true if the extension is of a type that can be policy installed. nit: not if the extension is of type, but rather if the bubble is informing about a single extension that could be policy-installed. https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/ui/exte... File chrome/browser/ui/extensions/extension_message_bubble_bridge.cc (right): https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:47: const extensions::ExtensionIdList& list = controller_->GetExtensionIdList(); On 2016/09/08 15:42:30, catmullings wrote: > Lines 47 - 59 are obviously duplicated code from the GetExtraViewInfo() method > below. If this is the proper way and place to check whether to remove the > "Restore Settings" button, then I will tuck this code into a helper function. > WDYT? Sounds reasonable to me, though note that you have a few stragglers here (e.g. line 55) https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h (right): https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:28: int resource_id = -1; nit: don't mix ctor initialization and in-class initialization (either is fine, but prefer one) https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:73: if (icon) On 2016/09/08 15:42:30, catmullings wrote: > The line "return icon ? icon : label_;" results in a compile error regarding > views::Label * and views::ImageView * being incompatible types, so I expanded > the one line if statement. This is fine. If you wanted to fix the compile error, I think the following would work: return icon ? static_cast<views::View*>(icon) : static_cast<views::View*>(label_); https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:75: else no else after return.
Patchset #6 (id:120001) has been deleted
Tests added; Addressed code review comments patch 5 https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_message_bubble_controller.h (right): https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_message_bubble_controller.h:90: // Returns true if the extension is of a type that can be policy installed. On 2016/09/12 22:51:43, Devlin wrote: > nit: not if the extension is of type, but rather if the bubble is informing > about a single extension that could be policy-installed. Done. https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/ui/exte... File chrome/browser/ui/extensions/extension_message_bubble_bridge.cc (right): https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:47: const extensions::ExtensionIdList& list = controller_->GetExtensionIdList(); On 2016/09/12 22:51:43, Devlin wrote: > On 2016/09/08 15:42:30, catmullings wrote: > > Lines 47 - 59 are obviously duplicated code from the GetExtraViewInfo() method > > below. If this is the proper way and place to check whether to remove the > > "Restore Settings" button, then I will tuck this code into a helper function. > > WDYT? > > Sounds reasonable to me, though note that you have a few stragglers here (e.g. > line 55) Mkay. Line 55 should be the only straggler, since I need the list variable to construct the extension variable, which is used to do the IsPolicyLocation() check. https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:47: const extensions::ExtensionIdList& list = controller_->GetExtensionIdList(); On 2016/09/12 22:51:43, Devlin wrote: > On 2016/09/08 15:42:30, catmullings wrote: > > Lines 47 - 59 are obviously duplicated code from the GetExtraViewInfo() method > > below. If this is the proper way and place to check whether to remove the > > "Restore Settings" button, then I will tuck this code into a helper function. > > WDYT? > > Sounds reasonable to me, though note that you have a few stragglers here (e.g. > line 55) Done. https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h (right): https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:28: int resource_id = -1; On 2016/09/12 22:51:43, Devlin wrote: > nit: don't mix ctor initialization and in-class initialization (either is fine, > but prefer one) Done. https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:73: if (icon) On 2016/09/12 22:51:43, Devlin wrote: > On 2016/09/08 15:42:30, catmullings wrote: > > The line "return icon ? icon : label_;" results in a compile error regarding > > views::Label * and views::ImageView * being incompatible types, so I expanded > > the one line if statement. > > This is fine. If you wanted to fix the compile error, I think the following > would work: > return icon ? static_cast<views::View*>(icon) : > static_cast<views::View*>(label_); Done. https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:75: else On 2016/09/12 22:51:43, Devlin wrote: > no else after return. No longer relevant by the one line if statement change. https://codereview.chromium.org/2206693002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:75: else On 2016/09/12 22:51:43, Devlin wrote: > no else after return. Done. https://codereview.chromium.org/2206693002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/2206693002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:55: views::Label* label = nullptr; I replaced label_, the private variable, with a local variable named label. label_ is assigned somewhere before this method is called, which is a problem for line 67 which checks if both views already exist. This was causing a problem during a test when the parameter extra_view_info only has the icon set, but this code would think that the label is set as well. https://codereview.chromium.org/2206693002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc (right): https://codereview.chromium.org/2206693002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:293: std::unique_ptr<views::View> extra_view(TestCreateExtraView()); Should I wrap this in a unique_ptr to ensure the view is destroyed after it goes out of scope? TestCreateExtraView() calls CreateExtraView(), which uses "new" to construct a view. views::View * CreateExtraView is the method signature defined in a higher parent, abstract class, so if I were to change that to return a unique_ptr there, multiple changes would need to be made. https://codereview.chromium.org/2206693002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:295: EXPECT_EQ("ImageView", std::string(extra_view->GetClassName())); Is this a sufficient way to check? The same question applies to my similar checks in the tests below. Other ideas would be to check the: - Width/height of the resulting image, if I can dig up the expected dimensions. - Image actually points to IDR_OMNIBOX ... Though I don't see how to obtain this from the ImageView nor ImageSkia classes.
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Can you also add a screenshot when you get a chance? https://codereview.chromium.org/2206693002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc (right): https://codereview.chromium.org/2206693002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:293: std::unique_ptr<views::View> extra_view(TestCreateExtraView()); On 2016/09/22 00:00:11, catmullings wrote: > Should I wrap this in a unique_ptr to ensure the view is destroyed after it goes > out of scope? TestCreateExtraView() calls CreateExtraView(), which uses "new" to > construct a view. > > views::View * CreateExtraView is the method signature defined in a higher > parent, abstract class, so if I were to change that to return a unique_ptr > there, multiple changes would need to be made. Wrapping the result is good. You could change the TestCreateExtraView() method so that the result is guaranteed to be wrapped. https://codereview.chromium.org/2206693002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:295: EXPECT_EQ("ImageView", std::string(extra_view->GetClassName())); On 2016/09/22 00:00:11, catmullings wrote: > Is this a sufficient way to check? The same question applies to my similar > checks in the tests below. > > Other ideas would be to check the: > - Width/height of the resulting image, if I can dig up the expected dimensions. > - Image actually points to IDR_OMNIBOX ... Though I don't see how to obtain this > from the ImageView nor ImageSkia classes. You should be able to use AreImagesEqual() to compare the image of the ImageView with that of the resource, and they should match. Similarly for below, you can check the text of the link/label. https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/extension_message_bubble_bridge_unittest.cc (right): https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_message_bubble_bridge_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. test files should (normally) be near the files they test. So this should go in chrome/browser/ui/extensions/, near where the extension_message_bubble_bridge file is. https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_message_bubble_bridge_unittest.cc:51: namespace extensions { nit: \n https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_message_bubble_bridge_unittest.cc:60: base::FilePath test_data_dir_; Why do we need this instead of ExtensionServiceTestBase::data_dir_? https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_message_bubble_bridge_unittest.cc:73: extensions::ExtensionWebUIOverrideRegistrar::GetFactoryInstance() no need for extensions:: prefix https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_message_bubble_bridge_unittest.cc:113: ASSERT_EQ(-1, extra_view_info->resource_id); prefer EXPECT_EQ in cases like this where a failure wouldn't invalidate the rest of the test or cause a crash. https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_message_bubble_bridge_unittest.cc:160: bridge->GetActionButtonText()); Given the only change between this and the first test is this assertion, I think we can probably just add this assertion there (and similar for the following test). https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/extension_service_test_with_install.cc (right): https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_service_test_with_install.cc:133: base::FilePath crx_path; With functions like these, it's best to house the implementation in a single place and just fill in the missing pieces, rather than copying the code. As an example, PackAndInstallCRX on line 122 just calls PackAndInstallCRX(), adding in reasonable defaults for file path and flags. So here, I think we should add a new version of PackAndInstallCRX that includes a location, and have others call into that. https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/ui/exte... File chrome/browser/ui/extensions/extension_message_bubble_bridge.cc (right): https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:56: extensions::Manifest::IsPolicyLocation(extension->location())) this seems like it should be pulled into a helper method, since its repeated here and in GetExtraViewInfo https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:58: else no else after return https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/test_toolbar_actions_bar_bubble_delegate.cc (right): https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/test_toolbar_actions_bar_bubble_delegate.cc:36: return extra_view_info; return base::MakeUnique<ExtraViewInfo>(...) https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.h (right): https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.h:28: friend class ToolbarActionsBarBubbleViewsTest; put friend classes in the private: section of the class. https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.h:59: views::Label* label_; Is this used? https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc (right): https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:282: ToolbarActionsBarBubbleDelegate::ExtraViewInfo extra_view_info; Don't we need to set this on the delegate?
Patchset #9 (id:200001) has been deleted
Patchset #8 (id:180001) has been deleted
https://codereview.chromium.org/2206693002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc (right): https://codereview.chromium.org/2206693002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:295: EXPECT_EQ("ImageView", std::string(extra_view->GetClassName())); On 2016/09/26 20:13:09, Devlin wrote: > On 2016/09/22 00:00:11, catmullings wrote: > > Is this a sufficient way to check? The same question applies to my similar > > checks in the tests below. > > > > Other ideas would be to check the: > > - Width/height of the resulting image, if I can dig up the expected > dimensions. > > - Image actually points to IDR_OMNIBOX ... Though I don't see how to obtain > this > > from the ImageView nor ImageSkia classes. > > You should be able to use AreImagesEqual() to compare the image of the ImageView > with that of the resource, and they should match. Similarly for below, you can > check the text of the link/label. Done. https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/extension_message_bubble_bridge_unittest.cc (right): https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_message_bubble_bridge_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/09/26 20:13:09, Devlin wrote: > test files should (normally) be near the files they test. So this should go in > chrome/browser/ui/extensions/, near where the extension_message_bubble_bridge > file is. Done. https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_message_bubble_bridge_unittest.cc:51: namespace extensions { On 2016/09/26 20:13:09, Devlin wrote: > nit: \n Done. https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_message_bubble_bridge_unittest.cc:60: base::FilePath test_data_dir_; On 2016/09/26 20:13:09, Devlin wrote: > Why do we need this instead of ExtensionServiceTestBase::data_dir_? Cool. Didn't know that ExtensionServiceTestBase::data_dir_ referred to the test data dir. Done. https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_message_bubble_bridge_unittest.cc:73: extensions::ExtensionWebUIOverrideRegistrar::GetFactoryInstance() On 2016/09/26 20:13:09, Devlin wrote: > no need for extensions:: prefix How do you know that the extensions:: prefix can be dropped? More generally, how does one know when or when not to include it? I see the extensions:: prefix in many places where the "namespace extensions {}" is used. https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_message_bubble_bridge_unittest.cc:113: ASSERT_EQ(-1, extra_view_info->resource_id); On 2016/09/26 20:13:09, Devlin wrote: > prefer EXPECT_EQ in cases like this where a failure wouldn't invalidate the rest > of the test or cause a crash. Done. https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_message_bubble_bridge_unittest.cc:160: bridge->GetActionButtonText()); On 2016/09/26 20:13:09, Devlin wrote: > Given the only change between this and the first test is this assertion, I think > we can probably just add this assertion there (and similar for the following > test). Done. https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/extension_service_test_with_install.cc (right): https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_service_test_with_install.cc:133: base::FilePath crx_path; On 2016/09/26 20:13:09, Devlin wrote: > With functions like these, it's best to house the implementation in a single > place and just fill in the missing pieces, rather than copying the code. As an > example, PackAndInstallCRX on line 122 just calls PackAndInstallCRX(), adding in > reasonable defaults for file path and flags. So here, I think we should add a > new version of PackAndInstallCRX that includes a location, and have others call > into that. Wrt "add a new version of PackAndInstallCRX that includes a location", do you mean create something like the PackAndInstallCRX() at line 100, except swap out line 111 with lines 140 - 149? https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/ui/exte... File chrome/browser/ui/extensions/extension_message_bubble_bridge.cc (right): https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:56: extensions::Manifest::IsPolicyLocation(extension->location())) On 2016/09/26 20:13:09, Devlin wrote: > this seems like it should be pulled into a helper method, since its repeated > here and in GetExtraViewInfo Done. https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:58: else On 2016/09/26 20:13:09, Devlin wrote: > no else after return Done. https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/test_toolbar_actions_bar_bubble_delegate.cc (right): https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/test_toolbar_actions_bar_bubble_delegate.cc:36: return extra_view_info; On 2016/09/26 20:13:09, Devlin wrote: > return base::MakeUnique<ExtraViewInfo>(...) Done. https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.h (right): https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.h:28: friend class ToolbarActionsBarBubbleViewsTest; On 2016/09/26 20:13:09, Devlin wrote: > put friend classes in the private: section of the class. Done. https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.h:59: views::Label* label_; On 2016/09/26 20:13:10, Devlin wrote: > Is this used? Nice catch. Done. https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc (right): https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:282: ToolbarActionsBarBubbleDelegate::ExtraViewInfo extra_view_info; On 2016/09/26 20:13:10, Devlin wrote: > Don't we need to set this on the delegate? Done. https://codereview.chromium.org/2206693002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc (right): https://codereview.chromium.org/2206693002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:383: std::string classType = v->GetClassName(); Wasn't sure how to prevent a copy in converting from a const char * to a string. std::string & classType = ... didn't compile.
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.
Just responding to comments for now - will take another look once the Mac implementation's there. https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/extension_message_bubble_bridge_unittest.cc (right): https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_message_bubble_bridge_unittest.cc:73: extensions::ExtensionWebUIOverrideRegistrar::GetFactoryInstance() On 2016/10/06 18:24:19, catmullings wrote: > On 2016/09/26 20:13:09, Devlin wrote: > > no need for extensions:: prefix > > How do you know that the extensions:: prefix can be dropped? > > More generally, how does one know when or when not to include it? I see the > extensions:: prefix in many places where the "namespace extensions {}" is used. As a rule of thumb, you don't need to use the extensions:: prefix any time you are in the namespace extensions {} block. Code that does is often because the namespace was added afterwards and no one updated the innards. There are a few exceptions to this if you have conflicting types in different namespaces (you can often see this when code specifies "::switches", e.g.), but that's pretty rare. https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/extension_service_test_with_install.cc (right): https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_service_test_with_install.cc:133: base::FilePath crx_path; On 2016/10/06 18:24:19, catmullings wrote: > On 2016/09/26 20:13:09, Devlin wrote: > > With functions like these, it's best to house the implementation in a single > > place and just fill in the missing pieces, rather than copying the code. As > an > > example, PackAndInstallCRX on line 122 just calls PackAndInstallCRX(), adding > in > > reasonable defaults for file path and flags. So here, I think we should add a > > new version of PackAndInstallCRX that includes a location, and have others > call > > into that. > > Wrt "add a new version of PackAndInstallCRX that includes a location", do you > mean create something like the PackAndInstallCRX() at line 100, except swap out > line 111 with lines 140 - 149? Not quite. With these types of functions, where the majority of the implementation is the same and it's just one line that might be different, it's best to have only one version actually do the implementation and have the rest call into that version. The version with the implementation takes all possible arguments, and the other versions call into that version, providing defaults for any missing arguments. You can see we already do this for most "PackAndInstallCRX()" function versions here - e.g. the ones on line 114 and 122 both just call into PackAndInstallCRX providing flag and/or pem path values. So for this, instead of writing a new function with the full implementation, we should add a location argument to the "main" PackAndInstallCRX() function, and have others provide the default install location. Does that make sense? https://codereview.chromium.org/2206693002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc (right): https://codereview.chromium.org/2206693002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:383: std::string classType = v->GetClassName(); On 2016/10/06 18:24:19, catmullings wrote: > Wasn't sure how to prevent a copy in converting from a const char * to a string. > std::string & classType = ... didn't compile. there isn't a way, because there's actually no copy here. Since GetClassName() returns a const char*, there's no std::string to copy. If this weren't a test, you'd probably want to just assign the result to a const char* to avoid copying the bytes, but in a test, using a std::string is fine (and probably better).
Patchset #10 (id:260001) has been deleted
Patchset #9 (id:240001) has been deleted
https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/extension_message_bubble_bridge_unittest.cc (right): https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_message_bubble_bridge_unittest.cc:73: extensions::ExtensionWebUIOverrideRegistrar::GetFactoryInstance() On 2016/10/06 18:24:19, catmullings wrote: > On 2016/09/26 20:13:09, Devlin wrote: > > no need for extensions:: prefix > > How do you know that the extensions:: prefix can be dropped? My guess would be that ExtensionWebUIOverrideRegistrar::GetFactoryInstance() wouldn't clash with anything in this file under the extensions namespace. > More generally, how does one know when or when not to include it? I see the > extensions:: prefix in many places where the "namespace extensions {}" is used. https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_message_bubble_bridge_unittest.cc:73: extensions::ExtensionWebUIOverrideRegistrar::GetFactoryInstance() On 2016/10/10 15:27:28, Devlin wrote: > On 2016/10/06 18:24:19, catmullings wrote: > > On 2016/09/26 20:13:09, Devlin wrote: > > > no need for extensions:: prefix > > > > How do you know that the extensions:: prefix can be dropped? > > > > More generally, how does one know when or when not to include it? I see the > > extensions:: prefix in many places where the "namespace extensions {}" is > used. > > As a rule of thumb, you don't need to use the extensions:: prefix any time you > are in the namespace extensions {} block. Code that does is often because the > namespace was added afterwards and no one updated the innards. There are a few > exceptions to this if you have conflicting types in different namespaces (you > can often see this when code specifies "::switches", e.g.), but that's pretty > rare. Done. https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/extension_service_test_with_install.cc (right): https://codereview.chromium.org/2206693002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_service_test_with_install.cc:133: base::FilePath crx_path; On 2016/10/10 15:27:28, Devlin wrote: > On 2016/10/06 18:24:19, catmullings wrote: > > On 2016/09/26 20:13:09, Devlin wrote: > > > With functions like these, it's best to house the implementation in a single > > > place and just fill in the missing pieces, rather than copying the code. As > > an > > > example, PackAndInstallCRX on line 122 just calls PackAndInstallCRX(), > adding > > in > > > reasonable defaults for file path and flags. So here, I think we should add > a > > > new version of PackAndInstallCRX that includes a location, and have others > > call > > > into that. > > > > Wrt "add a new version of PackAndInstallCRX that includes a location", do you > > mean create something like the PackAndInstallCRX() at line 100, except swap > out > > line 111 with lines 140 - 149? > > Not quite. With these types of functions, where the majority of the > implementation is the same and it's just one line that might be different, it's > best to have only one version actually do the implementation and have the rest > call into that version. The version with the implementation takes all possible > arguments, and the other versions call into that version, providing defaults for > any missing arguments. You can see we already do this for most > "PackAndInstallCRX()" function versions here - e.g. the ones on line 114 and 122 > both just call into PackAndInstallCRX providing flag and/or pem path values. > > So for this, instead of writing a new function with the full implementation, we > should add a location argument to the "main" PackAndInstallCRX() function, and > have others provide the default install location. Does that make sense? Done. https://codereview.chromium.org/2206693002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc (right): https://codereview.chromium.org/2206693002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:383: std::string classType = v->GetClassName(); On 2016/10/10 15:27:29, Devlin wrote: > On 2016/10/06 18:24:19, catmullings wrote: > > Wasn't sure how to prevent a copy in converting from a const char * to a > string. > > std::string & classType = ... didn't compile. > > there isn't a way, because there's actually no copy here. Since GetClassName() > returns a const char*, there's no std::string to copy. If this weren't a test, > you'd probably want to just assign the result to a const char* to avoid copying > the bytes, but in a test, using a std::string is fine (and probably better). Done. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/extension_service_test_with_install.cc (right): https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/extension_service_test_with_install.cc:118: installer->InstallCrx(crx_path); This line replaces: return InstallCRX(crx_path, install_state, creation_flags); However, the new line doesn't have an option to input creation_flags. I didn't see any method in CrxInstaller that does. How should I accomodate for the creation_flags? https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/extension_service_test_with_install.cc:129: Extension::NO_FLAGS, Manifest::Location::INTERNAL); What is the default for Manifest::Location among the enum Locations (https://cs.chromium.org/chromium/src/extensions/common/manifest.h?q=manifest....) ? I tried Manifest::Location::INTERNAL, but that doesn't work (i.e. breaks my unit_test), so it's simply serving as a placeholder for now.
https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/extension_service_test_with_install.cc (right): https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/extension_service_test_with_install.cc:118: installer->InstallCrx(crx_path); On 2016/10/12 18:58:58, catmullings wrote: > This line replaces: > > return InstallCRX(crx_path, install_state, creation_flags); > > However, the new line doesn't have an option to input creation_flags. I didn't > see any method in CrxInstaller that does. How should I accomodate for the > creation_flags? Instead, we should just be adding a new parameter to InstallCRX to allow for the location. Remember, the goal here is to basically have all these methods funnel down into one central version. A good litmus test would be that we should ideally only have one place be creating a CrxInstaller. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/extension_service_test_with_install.cc:129: Extension::NO_FLAGS, Manifest::Location::INTERNAL); On 2016/10/12 18:58:58, catmullings wrote: > What is the default for Manifest::Location among the enum Locations > (https://cs.chromium.org/chromium/src/extensions/common/manifest.h?q=manifest....) > ? > > I tried Manifest::Location::INTERNAL, but that doesn't work (i.e. breaks my > unit_test), so it's simply serving as a placeholder for now. INTERNAL is the default value (you can see it in CrxInstaller's ctor here: https://cs.chromium.org/chromium/src/chrome/browser/extensions/crx_installer....). What type of failure are you getting? https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm (right): https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:199: NSImage* i = gfx::NSImageFromImageSkia( I think this would be more efficient as ResourceBundle::GetSharedInstance().GetNativeImageNamed(id).ToNSImage(). https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:203: iconView_ = [[NSImageView alloc] initWithFrame:frame]; maybe init it with the image instead? https://developer.apple.com/reference/appkit/nsimageview/1644708-imageviewwit... That might also default the size to be correct. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:205: setImage:gfx::NSImageFromImageSkia(*( We should be able to avoid re-fetching the image. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:232: (label_) ? [label_ frame].size no need for parens https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:233: : NSMakeSize(NSWidth([link_ frame]), NSHeight([link_ frame])); Why not use the size property on frame? https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:327: [link_ setFrame:NSMakeRect(currentMaxWidth - extraViewTextSize.width, this would be cleaner if we define the frame above (since it's the same) and then just do if (link_) [link_ setFrame:frame] else [label_ setFrame:frame] https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/exte... File chrome/browser/ui/extensions/extension_message_bubble_bridge.cc (right): https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:53: const extensions::ExtensionIdList& list = controller_->GetExtensionIdList(); maybe add a DCHECK(!list.empty()) here? https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:61: return base::string16(); Add a comment like: // Don't prompt to remove policy-installed extensions. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h (right): https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:16: CLOSE_LINK, I think it's probably fine to keep this as Learn more. The only time we close the bubble with a link is when it's a learn more link. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc (right): https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:31: return (bubble_) ? bubble_->CreateExtraView() : nullptr; no need for parens, but maybe just DCHECK(bubble_) here - does it make sense to call it any other time? https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:122: EXPECT_FALSE(bubble()->GetDialogClientView()->cancel_button()); EXPECT_FALSE(link), too? https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:294: TEST_F(ToolbarActionsBarBubbleViewsTest, TestCreateExtraViewEmpty) { This could just be lumped into the NoButtons test above, right? https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:316: EXPECT_EQ("ImageView", std::string(extra_view->GetClassName())); Overall, it might be easiest to just store the views::ImageView on the bubble object so you can return it directly and compare it, rather than trying to coerce the value. But I don't feel strongly, if you prefer this way.
Patchset #10 (id:300001) has been deleted
Patchset #10 (id:320001) has been deleted
https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/extension_service_test_with_install.cc (right): https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/extension_service_test_with_install.cc:118: installer->InstallCrx(crx_path); On 2016/10/12 20:16:53, Devlin wrote: > On 2016/10/12 18:58:58, catmullings wrote: > > This line replaces: > > > > return InstallCRX(crx_path, install_state, creation_flags); > > > > However, the new line doesn't have an option to input creation_flags. I didn't > > see any method in CrxInstaller that does. How should I accomodate for the > > creation_flags? > > Instead, we should just be adding a new parameter to InstallCRX to allow for the > location. Remember, the goal here is to basically have all these methods funnel > down into one central version. A good litmus test would be that we should > ideally only have one place be creating a CrxInstaller. Done. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/extension_service_test_with_install.cc:129: Extension::NO_FLAGS, Manifest::Location::INTERNAL); On 2016/10/12 20:16:53, Devlin wrote: > On 2016/10/12 18:58:58, catmullings wrote: > > What is the default for Manifest::Location among the enum Locations > > > (https://cs.chromium.org/chromium/src/extensions/common/manifest.h?q=manifest....) > > ? > > > > I tried Manifest::Location::INTERNAL, but that doesn't work (i.e. breaks my > > unit_test), so it's simply serving as a placeholder for now. > > INTERNAL is the default value (you can see it in CrxInstaller's ctor here: > https://cs.chromium.org/chromium/src/chrome/browser/extensions/crx_installer....). > What type of failure are you getting? In addressing your other CL comment above, refactoring the InstallCRX functions ended fixing this. My unit tests pass. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/extension_service_test_with_install.cc:129: Extension::NO_FLAGS, Manifest::Location::INTERNAL); On 2016/10/12 20:16:53, Devlin wrote: > On 2016/10/12 18:58:58, catmullings wrote: > > What is the default for Manifest::Location among the enum Locations > > > (https://cs.chromium.org/chromium/src/extensions/common/manifest.h?q=manifest....) > > ? > > > > I tried Manifest::Location::INTERNAL, but that doesn't work (i.e. breaks my > > unit_test), so it's simply serving as a placeholder for now. > > INTERNAL is the default value (you can see it in CrxInstaller's ctor here: > https://cs.chromium.org/chromium/src/chrome/browser/extensions/crx_installer....). > What type of failure are you getting? Done. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm (right): https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:199: NSImage* i = gfx::NSImageFromImageSkia( On 2016/10/12 20:16:54, Devlin wrote: > I think this would be more efficient as > ResourceBundle::GetSharedInstance().GetNativeImageNamed(id).ToNSImage(). Done. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:203: iconView_ = [[NSImageView alloc] initWithFrame:frame]; On 2016/10/12 20:16:53, Devlin wrote: > maybe init it with the image instead? > https://developer.apple.com/reference/appkit/nsimageview/1644708-imageviewwit... > > That might also default the size to be correct. As discussed, disregarding this suggestion because imageViewWithImage only works for MacOS 10.12+, but Chrome can only compile an older version. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:205: setImage:gfx::NSImageFromImageSkia(*( On 2016/10/12 20:16:53, Devlin wrote: > We should be able to avoid re-fetching the image. Done. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:232: (label_) ? [label_ frame].size On 2016/10/12 20:16:53, Devlin wrote: > no need for parens Done. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:233: : NSMakeSize(NSWidth([link_ frame]), NSHeight([link_ frame])); On 2016/10/12 20:16:53, Devlin wrote: > Why not use the size property on frame? I can. I was following the patterns below for setting the button sizes. See line 241 below. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:233: : NSMakeSize(NSWidth([link_ frame]), NSHeight([link_ frame])); On 2016/10/12 20:16:53, Devlin wrote: > Why not use the size property on frame? Done. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:327: [link_ setFrame:NSMakeRect(currentMaxWidth - extraViewTextSize.width, On 2016/10/12 20:16:53, Devlin wrote: > this would be cleaner if we define the frame above (since it's the same) and > then just do > if (link_) > [link_ setFrame:frame] > else > [label_ setFrame:frame] Yup, much better. I'll do that. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:327: [link_ setFrame:NSMakeRect(currentMaxWidth - extraViewTextSize.width, On 2016/10/12 20:16:53, Devlin wrote: > this would be cleaner if we define the frame above (since it's the same) and > then just do > if (link_) > [link_ setFrame:frame] > else > [label_ setFrame:frame] Done. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/exte... File chrome/browser/ui/extensions/extension_message_bubble_bridge.cc (right): https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:53: const extensions::ExtensionIdList& list = controller_->GetExtensionIdList(); On 2016/10/12 20:16:54, Devlin wrote: > maybe add a DCHECK(!list.empty()) here? Done. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:61: return base::string16(); On 2016/10/12 20:16:54, Devlin wrote: > Add a comment like: // Don't prompt to remove policy-installed extensions. I added a slightly longer comment. Lmk if it's verbose. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:61: return base::string16(); On 2016/10/12 20:16:54, Devlin wrote: > Add a comment like: // Don't prompt to remove policy-installed extensions. Done. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h (right): https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:16: CLOSE_LINK, On 2016/10/12 20:16:54, Devlin wrote: > I think it's probably fine to keep this as Learn more. The only time we close > the bubble with a link is when it's a learn more link. Done. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc (right): https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:31: return (bubble_) ? bubble_->CreateExtraView() : nullptr; On 2016/10/12 20:16:54, Devlin wrote: > no need for parens, but maybe just DCHECK(bubble_) here - does it make sense to > call it any other time? Added the DCHECK(bubble_). Lmk if this is sufficient. I'm not sure what you meant by "call it any other time". https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:122: EXPECT_FALSE(bubble()->GetDialogClientView()->cancel_button()); On 2016/10/12 20:16:54, Devlin wrote: > EXPECT_FALSE(link), too? Done. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:294: TEST_F(ToolbarActionsBarBubbleViewsTest, TestCreateExtraViewEmpty) { On 2016/10/12 20:16:54, Devlin wrote: > This could just be lumped into the NoButtons test above, right? Done. https://codereview.chromium.org/2206693002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:316: EXPECT_EQ("ImageView", std::string(extra_view->GetClassName())); On 2016/10/12 20:16:54, Devlin wrote: > Overall, it might be easiest to just store the views::ImageView on the bubble > object so you can return it directly and compare it, rather than trying to > coerce the value. > > But I don't feel strongly, if you prefer this way. I think I will leave this as it is.
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: Exceeded global retry quota
Patchset #11 (id:360001) has been deleted
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_...)
Patchset #11 (id:380001) has been deleted
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #11 (id:400001) has been deleted
Patchset #11 (id:420001) has been deleted
Patchset #11 (id:440001) has been deleted
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #11 (id:460001) has been deleted
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...
Patchset #11 (id:480001) has been deleted
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: Exceeded global retry quota
Patchset #11 (id:500001) has been deleted
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.
Nice! I wanna take another quick look tomorrow morning, but overall this looks great. https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.h (right): https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.h:36: // The extra view text as link-style button. Optional. nit: need proper grammar in comments, so this should be "as *a* link-style button". Ditto below. https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm (right): https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:203: gfx::Image(gfx::CreateVectorIcon(resource_id, 16, gfx::kChromeIconGrey)) What is this 16? https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/exte... File chrome/browser/ui/extensions/settings_api_bubble_helpers.cc (right): https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/exte... chrome/browser/ui/extensions/settings_api_bubble_helpers.cc:75: /*#if !defined(OS_WIN) Was this meant to be left in?
Nice! A few more nits, but certainly nothing major. I think you can probably also add a cocoa reviewer to this once you upload the next patch set. Avi (avi@) has the most familiarity in this area, so I'd probably recommend him. https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm (right): https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:202: NSImage* i = nit: it's nicer to use more descriptive names when we can (especially since 'i' is frequently used for iteration). Maybe just 'image'? https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:208: extraViewIconSize = i.size; nit: I'd prefer we use the view's frame's size here, since that's what we're adding. https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac_unittest.mm (left): https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac_unittest.mm:163: nit: try not to make extraneous whitespace changes. https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/exte... File chrome/browser/ui/extensions/extension_message_bubble_bridge.cc (right): https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:16: #include "ui/base/resource/resource_bundle.h" needed? https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/exte... File chrome/browser/ui/extensions/extension_message_bubble_bridge_unittest.cc (right): https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_message_bubble_bridge_unittest.cc:52: namespace extensions { nit: Tests should be in the same namespace as the object they're testing, so this shouldn't be in extensions::. https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_message_bubble_bridge_unittest.cc:94: base::FilePath fp(data_dir().AppendASCII("api_test/override/newtab/")); nit: prefer a short name over an acronym for variables in most cases [1]. So here, maybe s/fp/path. (also below on line 120) [1] https://google.github.io/styleguide/cppguide.html#General_Naming_Rules https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/test_toolbar_actions_bar_bubble_delegate.h (right): https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/test_toolbar_actions_bar_bubble_delegate.h:77: // Extra view information inputted into the bubble. input isn't quite right here. There's no input involved. Rather, I think this would just be "Information about the extra view to show, if any." https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:53: icon = new views::ImageView(); nit: I know this is all safe, but this method is long enough that we should probably put this in unique_ptrs until we return them or add them as child views, just so that if someone else comes along and puts an early return for some reason at line 69, we don't leak. https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc (right): https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:312: EXPECT_EQ("ImageView", std::string(extra_view->GetClassName())); the static cast below will fail if this isn't true, so this should be ASSERT_EQ. Also, we should use ImageView::GetClassName() instead of the string literal "ImageView". https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:334: EXPECT_EQ("Link", std::string(extra_view->GetClassName())); Here and below as well, prefer FooView::GetClassName() and ASSERT_EQ https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:379: std::string classType = v->GetClassName(); s/classType/class_name, and use a const char*.
Patchset #12 (id:540001) has been deleted
catmullings@chromium.org changed reviewers: + avi@chromium.org
Adding avi@ as a cocoa reviewer. avi: ui/cocoa/extensions/* contain the relevant files for review. https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.h (right): https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.h:36: // The extra view text as link-style button. Optional. On 2016/10/20 00:23:42, Devlin wrote: > nit: need proper grammar in comments, so this should be "as *a* link-style > button". Ditto below. Done. https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm (right): https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:202: NSImage* i = On 2016/10/20 17:04:07, Devlin wrote: > nit: it's nicer to use more descriptive names when we can (especially since 'i' > is frequently used for iteration). Maybe just 'image'? Done. https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:203: gfx::Image(gfx::CreateVectorIcon(resource_id, 16, gfx::kChromeIconGrey)) On 2016/10/20 00:23:42, Devlin wrote: > What is this 16? I'm following the pattern/format from here: https://codereview.chromium.org/2365263006/patch/160001/170048 https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:208: extraViewIconSize = i.size; On 2016/10/20 17:04:07, Devlin wrote: > nit: I'd prefer we use the view's frame's size here, since that's what we're > adding. Done. https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac_unittest.mm (left): https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac_unittest.mm:163: On 2016/10/20 17:04:07, Devlin wrote: > nit: try not to make extraneous whitespace changes. Done. https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/exte... File chrome/browser/ui/extensions/extension_message_bubble_bridge.cc (right): https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_message_bubble_bridge.cc:16: #include "ui/base/resource/resource_bundle.h" On 2016/10/20 17:04:07, Devlin wrote: > needed? Done. https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/exte... File chrome/browser/ui/extensions/extension_message_bubble_bridge_unittest.cc (right): https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_message_bubble_bridge_unittest.cc:52: namespace extensions { On 2016/10/20 17:04:07, Devlin wrote: > nit: Tests should be in the same namespace as the object they're testing, so > this shouldn't be in extensions::. Done. https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_message_bubble_bridge_unittest.cc:94: base::FilePath fp(data_dir().AppendASCII("api_test/override/newtab/")); On 2016/10/20 17:04:07, Devlin wrote: > nit: prefer a short name over an acronym for variables in most cases [1]. So > here, maybe s/fp/path. > > (also below on line 120) > > [1] https://google.github.io/styleguide/cppguide.html#General_Naming_Rules Done. https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/exte... File chrome/browser/ui/extensions/settings_api_bubble_helpers.cc (right): https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/exte... chrome/browser/ui/extensions/settings_api_bubble_helpers.cc:75: /*#if !defined(OS_WIN) On 2016/10/20 00:23:42, Devlin wrote: > Was this meant to be left in? Done. https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/test_toolbar_actions_bar_bubble_delegate.h (right): https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/test_toolbar_actions_bar_bubble_delegate.h:77: // Extra view information inputted into the bubble. On 2016/10/20 17:04:07, Devlin wrote: > input isn't quite right here. There's no input involved. Rather, I think this > would just be "Information about the extra view to show, if any." Done. https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:53: icon = new views::ImageView(); On 2016/10/20 17:04:07, Devlin wrote: > nit: I know this is all safe, but this method is long enough that we should > probably put this in unique_ptrs until we return them or add them as child > views, just so that if someone else comes along and puts an early return for > some reason at line 69, we don't leak. Done. https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc (right): https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:312: EXPECT_EQ("ImageView", std::string(extra_view->GetClassName())); On 2016/10/20 17:04:07, Devlin wrote: > the static cast below will fail if this isn't true, so this should be ASSERT_EQ. > Also, we should use ImageView::GetClassName() instead of the string literal > "ImageView". Done. https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:334: EXPECT_EQ("Link", std::string(extra_view->GetClassName())); On 2016/10/20 17:04:07, Devlin wrote: > Here and below as well, prefer FooView::GetClassName() and ASSERT_EQ Done. https://codereview.chromium.org/2206693002/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:379: std::string classType = v->GetClassName(); On 2016/10/20 17:04:07, Devlin wrote: > s/classType/class_name, and use a const char*. Why is const char* better than std::string?
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.
Cat, your new code in ui/cocoa/extensions/* fits in, but it's the old code that is weeird. https://codereview.chromium.org/2206693002/diff/560001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm (right): https://codereview.chromium.org/2206693002/diff/560001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:224: [[[self window] contentView] addSubview:link_]; This is not your change but this class is inconsistent about ownership. -addTextFieldWithString:etcetc: (line 150) puts ownership in the view hierarchy, total ref count of 1. -addButtonWithString: (line 169) doesn't; it does alloc/init, and adds to the view hierarchy, so now there is a ref count of 2. The old code here to create the learnMoreButton_ ended up with a ref count of 2. Your new code is similar with the ref count. What is going on here? I would have thought that when we added all of these items to the view hierarchy we would let the view have sole ownership. But many of these items are saved to ivars and I can't see the equivalent release.
@Avi: Responding to your mac code question https://codereview.chromium.org/2206693002/diff/560001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm (right): https://codereview.chromium.org/2206693002/diff/560001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:224: [[[self window] contentView] addSubview:link_]; On 2016/10/21 13:19:43, Avi wrote: > This is not your change but this class is inconsistent about ownership. > > -addTextFieldWithString:etcetc: (line 150) puts ownership in the view hierarchy, > total ref count of 1. -addButtonWithString: (line 169) doesn't; it does > alloc/init, and adds to the view hierarchy, so now there is a ref count of 2. > > The old code here to create the learnMoreButton_ ended up with a ref count of 2. > Your new code is similar with the ref count. > > What is going on here? I would have thought that when we added all of these > items to the view hierarchy we would let the view have sole ownership. But many > of these items are saved to ivars and I can't see the equivalent release. I think these objects are saved to ivars because they are easily accessible from the ToolActionsBarBubbleMac object. For example, the ToolbarActionsBarBubbleMacUnittest accesses these variables to test and check if they are set or not: https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/extensions/toolb... Would you prefer if sole ownership of these objects went to the view hierarchy (i.e. not also stored as internal vars)? That would create some extra steps to access those items outside the class, but lmk if there's a straightforward way to access them.
@Avi, disregard my previous comment. One clarification, how does the ownership/ref counting work in the two methods? For addTextFieldWithString,the ref count is: +1 (alloc) +1 (addSubview) -1 (autorelease) -1 (scoped_nsobject dtor) So the final ref count is 0? And nothing has ownership? For addButtonWithString, the ref count is: +1 (alloc) +1 (addSubView) So the final ref count is 2, when it should really just be 1, in which ownership is left with the view hierarchy. What would be the preferred treatment of ownership for these two methods? One approach would be: +1 (alloc), +1 (addSubview), and then -1 (autorelease/release) to keep the final ref count as 1 and leave ownership with the view hierarchy.
On 2016/10/27 21:07:47, catmullings wrote: > For addTextFieldWithString,the ref count is: > +1 (alloc) > +1 (addSubview) > -1 (autorelease) > -1 (scoped_nsobject dtor) > So the final ref count is 0? And nothing has ownership? No, because when you do .autorelease() on the scoped_nsobject, it moves ownership from itself to the autorelease pool, so that last release in the scoped_nsobject doesn't happen. In this case, ownership is by the view. > For addButtonWithString, the ref count is: > +1 (alloc) > +1 (addSubView) > So the final ref count is 2, when it should really just be 1, in which ownership > is left with the view hierarchy. > > What would be the preferred treatment of ownership for these two methods? > One approach would be: +1 (alloc), +1 (addSubview), and then -1 > (autorelease/release) to keep the final ref count as 1 and leave ownership with > the view hierarchy. Since the ivars in which we hold them are just raw pointers, that makes sense. That way we can easily access them via ivar but the view does the cleanup.
On 2016/10/27 21:37:14, Avi wrote: > On 2016/10/27 21:07:47, catmullings wrote: > > For addTextFieldWithString,the ref count is: > > +1 (alloc) > > +1 (addSubview) > > -1 (autorelease) > > -1 (scoped_nsobject dtor) > > So the final ref count is 0? And nothing has ownership? > > No, because when you do .autorelease() on the scoped_nsobject, it moves > ownership from itself to the autorelease pool, so that last release in the > scoped_nsobject doesn't happen. In this case, ownership is by the view. > > > For addButtonWithString, the ref count is: > > +1 (alloc) > > +1 (addSubView) > > So the final ref count is 2, when it should really just be 1, in which > ownership > > is left with the view hierarchy. > > > > What would be the preferred treatment of ownership for these two methods? > > One approach would be: +1 (alloc), +1 (addSubview), and then -1 > > (autorelease/release) to keep the final ref count as 1 and leave ownership > with > > the view hierarchy. > > Since the ivars in which we hold them are just raw pointers, that makes sense. > That way we can easily access them via ivar but the view does the cleanup. Ok, sounds good. Also, along with adding the autorelease, would you recommend wrapping the alloc with a scoped_nsobject? I'm relatively new to Objective-C, and I'm trying to understand how the scoped_nsobject is useful. The scoped_nsobject improves the code's expressiveness -- clearly defining ownership -- but, in this case, it seems useless b/c the scoped_nsobject immediately loses its ownership by function end. Wouldn't it be fine just to alloc and release? No scoped_nsobject. of course, if scoped_nsbobject is good practice/style, then I'm not opposed to using it here.
On 2016/10/27 23:27:30, catmullings wrote: > On 2016/10/27 21:37:14, Avi wrote: > > On 2016/10/27 21:07:47, catmullings wrote: > > > For addTextFieldWithString,the ref count is: > > > +1 (alloc) > > > +1 (addSubview) > > > -1 (autorelease) > > > -1 (scoped_nsobject dtor) > > > So the final ref count is 0? And nothing has ownership? > > > > No, because when you do .autorelease() on the scoped_nsobject, it moves > > ownership from itself to the autorelease pool, so that last release in the > > scoped_nsobject doesn't happen. In this case, ownership is by the view. > > > > > For addButtonWithString, the ref count is: > > > +1 (alloc) > > > +1 (addSubView) > > > So the final ref count is 2, when it should really just be 1, in which > > ownership > > > is left with the view hierarchy. > > > > > > What would be the preferred treatment of ownership for these two methods? > > > One approach would be: +1 (alloc), +1 (addSubview), and then -1 > > > (autorelease/release) to keep the final ref count as 1 and leave ownership > > with > > > the view hierarchy. > > > > Since the ivars in which we hold them are just raw pointers, that makes sense. > > That way we can easily access them via ivar but the view does the cleanup. > > Ok, sounds good. > > Also, along with adding the autorelease, would you recommend wrapping the alloc > with a scoped_nsobject? > I'm relatively new to Objective-C, and I'm trying to understand how the > scoped_nsobject is useful. > The scoped_nsobject improves the code's expressiveness -- clearly defining > ownership -- but, in this case, it seems useless b/c the scoped_nsobject > immediately loses its ownership by function end. > Wouldn't it be fine just to alloc and release? No scoped_nsobject. > > of course, if scoped_nsbobject is good practice/style, then I'm not opposed to > using it here. You can't release right after alloc because then it will die. -alloc (count = 1) -dealloc (count = 0, dead) -add to view (UaF) You could do NSView* thing = [[[x alloc] initSomehow] autorelease] That's a reasonable thing to do in ObjC code. scoped_nsobject is really more useful as a C++ construct.
Patchset #13 (id:580001) has been deleted
Rebased master and made some code changes and comments for mac review. @Avi: toolbar_actions_bar_bubble_mac.mm https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm (right): https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:77: [[InfoBubbleWindow alloc] Does scoped_nsobject handle the release of an object? https://cs.chromium.org/chromium/src/base/mac/scoped_nsobject.h If not, I'll add an autorelease call. https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:138: [[NSMutableParagraphStyle alloc] init]); Same question above applies here. https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:221: link_ = [HyperlinkButtonCell buttonWithString:linkString.string]; buttonWithString:etc calls alloc and autorelease (https://cs.chromium.org/chromium/src/ui/base/cocoa/controls/hyperlink_button_...) and the retain call shouldn't be necessary here.
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.
lgtm https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm (right): https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:77: [[InfoBubbleWindow alloc] On 2016/10/29 02:14:50, catmullings wrote: > Does scoped_nsobject handle the release of an object? Yes. It's an RAII container for Objective C objects and releases when it leaves scope. https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:138: [[NSMutableParagraphStyle alloc] init]); On 2016/10/29 02:14:50, catmullings wrote: > Same question above applies here. This looks good too. The scoped_nsobject will release the original retain we get from -alloc, and the attribute dictionary will take an ownership for itself. https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm:221: link_ = [HyperlinkButtonCell buttonWithString:linkString.string]; On 2016/10/29 02:14:50, catmullings wrote: > buttonWithString:etc calls alloc and autorelease > > (https://cs.chromium.org/chromium/src/ui/base/cocoa/controls/hyperlink_button_...) > > and the retain call shouldn't be necessary here. Correct. This is what used to be called a "convenience initializer". Like line 217's -attributedStringWithString:... , this method -buttonWithString: returns an autoreleased object. This is an Objective C idiom and you can rely on it.
Just a few last nits. Also, please remember the format for Rietveld descriptions. [Title - This becomes the commit message] [Description] [BUG] With all fields preferably wrapped to a git-friendly 72 characters. https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/exte... File chrome/browser/ui/extensions/settings_api_bubble_helpers.cc (right): https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/exte... chrome/browser/ui/extensions/settings_api_bubble_helpers.cc:75: #if !defined(OS_WIN) && !defined(OS_MACOSX) We shouldn't be making this change as part of this CL. https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h (right): https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:32: gfx::VectorIconId resource_id; nit: \n https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:34: base::string16 text; nit: \n https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:51: std::unique_ptr<views::ImageView> icon(nullptr); nit: unique_ptr default initializes to nullptr, so just std::unique_ptr<views::imageView> icon; is sufficient. https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:55: gfx::CreateVectorIcon(resource_id, 16, gfx::kChromeIconGrey)); if we keep this as a hard-coded 16, can we add it as a constant up near kListPadding? https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:58: views::Label* label = nullptr; similar to the ImageView, let's wrap these in smart ptrs to make sure we don't leak. https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc (right): https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:311: EXPECT_TRUE(extra_view); nit: This should be an ASSERT_TRUE, since the test will crash on the next line if it's false. (Same for other tests)
Description was changed from ========== Pertaining to settings override bubbles, remove button to "Restore settings" and add a badge indicating that the extension was installed by an administrator. BUG=630734 ========== to ========== Pertaining to settings override bubbles, remove button to "Restore settings" and add a badge indicating that the extension was installed by an administrator. BUG=630734 ==========
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...
https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/exte... File chrome/browser/ui/extensions/settings_api_bubble_helpers.cc (right): https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/exte... chrome/browser/ui/extensions/settings_api_bubble_helpers.cc:75: #if !defined(OS_WIN) && !defined(OS_MACOSX) On 2016/11/01 00:21:21, Devlin wrote: > We shouldn't be making this change as part of this CL. Done. https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h (right): https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:32: gfx::VectorIconId resource_id; On 2016/11/01 00:21:21, Devlin wrote: > nit: \n Done. https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:34: base::string16 text; On 2016/11/01 00:21:21, Devlin wrote: > nit: \n Done. https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:51: std::unique_ptr<views::ImageView> icon(nullptr); On 2016/11/01 00:21:22, Devlin wrote: > nit: unique_ptr default initializes to nullptr, so just > std::unique_ptr<views::imageView> icon; > is sufficient. Done. https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:55: gfx::CreateVectorIcon(resource_id, 16, gfx::kChromeIconGrey)); On 2016/11/01 00:21:22, Devlin wrote: > if we keep this as a hard-coded 16, can we add it as a constant up near > kListPadding? Done. https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:58: views::Label* label = nullptr; On 2016/11/01 00:21:22, Devlin wrote: > similar to the ImageView, let's wrap these in smart ptrs to make sure we don't > leak. Done.
https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/exte... File chrome/browser/ui/extensions/settings_api_bubble_helpers.cc (right): https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/exte... chrome/browser/ui/extensions/settings_api_bubble_helpers.cc:75: #if !defined(OS_WIN) && !defined(OS_MACOSX) On 2016/11/01 00:21:21, Devlin wrote: > We shouldn't be making this change as part of this CL. Done. https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h (right): https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:32: gfx::VectorIconId resource_id; On 2016/11/01 00:21:21, Devlin wrote: > nit: \n Done. https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:34: base::string16 text; On 2016/11/01 00:21:21, Devlin wrote: > nit: \n Done. https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:51: std::unique_ptr<views::ImageView> icon(nullptr); On 2016/11/01 00:21:22, Devlin wrote: > nit: unique_ptr default initializes to nullptr, so just > std::unique_ptr<views::imageView> icon; > is sufficient. Done. https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:55: gfx::CreateVectorIcon(resource_id, 16, gfx::kChromeIconGrey)); On 2016/11/01 00:21:22, Devlin wrote: > if we keep this as a hard-coded 16, can we add it as a constant up near > kListPadding? Done. https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:58: views::Label* label = nullptr; On 2016/11/01 00:21:22, Devlin wrote: > similar to the ImageView, let's wrap these in smart ptrs to make sure we don't > leak. Done.
On 2016/11/01 00:21:22, Devlin wrote: > Just a few last nits. Also, please remember the format for Rietveld > descriptions. > > [Title - This becomes the commit message] > > [Description] > > [BUG] > > With all fields preferably wrapped to a git-friendly 72 characters. Aside from the description not wrapped properly (fixed), everything looks good to me? > > https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/exte... > File chrome/browser/ui/extensions/settings_api_bubble_helpers.cc (right): > > https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/exte... > chrome/browser/ui/extensions/settings_api_bubble_helpers.cc:75: #if > !defined(OS_WIN) && !defined(OS_MACOSX) > We shouldn't be making this change as part of this CL. > > https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/tool... > File chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h (right): > > https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/tool... > chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:32: > gfx::VectorIconId resource_id; > nit: \n > > https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/tool... > chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h:34: > base::string16 text; > nit: \n > > https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/view... > File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc > (right): > > https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/view... > chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:51: > std::unique_ptr<views::ImageView> icon(nullptr); > nit: unique_ptr default initializes to nullptr, so just > std::unique_ptr<views::imageView> icon; > is sufficient. > > https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/view... > chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:55: > gfx::CreateVectorIcon(resource_id, 16, gfx::kChromeIconGrey)); > if we keep this as a hard-coded 16, can we add it as a constant up near > kListPadding? > > https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/view... > chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:58: > views::Label* label = nullptr; > similar to the ImageView, let's wrap these in smart ptrs to make sure we don't > leak. > > https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/view... > File > chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc > (right): > > https://codereview.chromium.org/2206693002/diff/600001/chrome/browser/ui/view... > chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:311: > EXPECT_TRUE(extra_view); > nit: This should be an ASSERT_TRUE, since the test will crash on the next line > if it's false. (Same for other tests)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/11/01 02:48:13, catmullings wrote: > On 2016/11/01 00:21:22, Devlin wrote: > > Just a few last nits. Also, please remember the format for Rietveld > > descriptions. > > > > [Title - This becomes the commit message] > > > > [Description] > > > > [BUG] > > > > With all fields preferably wrapped to a git-friendly 72 characters. > > Aside from the description not wrapped properly (fixed), everything looks good > to me? You're missing the title line. This is handled automatically when you do git cl upload, but if you change the description you need to remember to include it. As it stands now, the commit title in the history would be "Pertaining to settings override bubbles, remove button to". :)
Description was changed from ========== Pertaining to settings override bubbles, remove button to "Restore settings" and add a badge indicating that the extension was installed by an administrator. BUG=630734 ========== to ========== Improve settings override bubble to indicate policy installed extensions Pertaining to settings override bubbles, remove button to "Restore settings" and add a badge indicating that the extension was installed by an administrator. BUG=630734 ==========
On 2016/11/01 14:54:26, Devlin wrote: > On 2016/11/01 02:48:13, catmullings wrote: > > On 2016/11/01 00:21:22, Devlin wrote: > > > Just a few last nits. Also, please remember the format for Rietveld > > > descriptions. > > > > > > [Title - This becomes the commit message] > > > > > > [Description] > > > > > > [BUG] > > > > > > With all fields preferably wrapped to a git-friendly 72 characters. > > > > Aside from the description not wrapped properly (fixed), everything looks good > > to me? > > You're missing the title line. This is handled automatically when you do git cl > upload, but if you change the description you need to remember to include it. > As it stands now, the commit title in the history would be "Pertaining to > settings override bubbles, remove button to". :) I see. I interpreted [Title - ... ] to be the heading of the CL, not the first line of the description. Title in the description has been added.
lgtm!
The CQ bit was checked by catmullings@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org Link to the patchset: https://codereview.chromium.org/2206693002/#ps620001 (title: "")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Improve settings override bubble to indicate policy installed extensions Pertaining to settings override bubbles, remove button to "Restore settings" and add a badge indicating that the extension was installed by an administrator. BUG=630734 ========== to ========== Improve settings override bubble to indicate policy installed extensions Pertaining to settings override bubbles, remove button to "Restore settings" and add a badge indicating that the extension was installed by an administrator. BUG=630734 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:620001)
Message was sent while issue was closed.
Description was changed from ========== Improve settings override bubble to indicate policy installed extensions Pertaining to settings override bubbles, remove button to "Restore settings" and add a badge indicating that the extension was installed by an administrator. BUG=630734 ========== to ========== Improve settings override bubble to indicate policy installed extensions Pertaining to settings override bubbles, remove button to "Restore settings" and add a badge indicating that the extension was installed by an administrator. BUG=630734 Committed: https://crrev.com/22bc237867193148a4a4e14ac925a41905fff817 Cr-Commit-Position: refs/heads/master@{#429376} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/22bc237867193148a4a4e14ac925a41905fff817 Cr-Commit-Position: refs/heads/master@{#429376}
Message was sent while issue was closed.
Patchset #15 (id:640001) has been deleted |
