Improve settings override bubble to indicate policy installed extensions

chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc

Index: chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
diff --git a/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc b/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
index 6ab2dacebf97ebdb3cae5f686afb1a08fbb9aa18..4d1fb5384d1771bb14be54319c89b0ad21e4aa3b 100644
--- a/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
+++ b/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
@@ -9,6 +9,7 @@
 #include "chrome/grit/locale_settings.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/views/controls/button/label_button.h"
+#include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/link.h"
 #include "ui/views/layout/box_layout.h"
@@ -25,7 +26,7 @@ ToolbarActionsBarBubbleViews::ToolbarActionsBarBubbleViews(
                                       views::BubbleBorder::TOP_RIGHT),
       delegate_(std::move(delegate)),
       item_list_(nullptr),

[catmullings, 2016/08/13 00:35:53]

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. 

[Devlin, 2016/08/22 20:59:42]

We shouldn't.  Links have extra behavior, and you can't assign a Label to a
Link.

[catmullings, 2016/08/31 00:03:31]

Done.

-      learn_more_button_(nullptr) {
+      link_(nullptr) {
   set_close_on_deactivate(delegate_->ShouldCloseOnDeactivate());
 }
 
@@ -37,13 +38,53 @@ void ToolbarActionsBarBubbleViews::Show() {
 }
 
 views::View* ToolbarActionsBarBubbleViews::CreateExtraView() {
-  base::string16 text = delegate_->GetLearnMoreButtonText();
-  if (text.empty())
+  std::unique_ptr<ToolbarActionsBarBubbleDelegate::ExtraViewInfo>
+      extra_view_info = delegate_->GetExtraViewInfo();
+
+  base::string16 text = extra_view_info->text;

[Devlin, 2016/08/22 20:59:42]

we need to check if extra_view_info is null, right?

[Devlin, 2016/08/22 20:59:42]

Let's also avoid the copy of the base::string16.

[catmullings, 2016/08/31 00:03:30]

Done.

[catmullings, 2016/08/31 00:03:30]

Done.

+  int resource_id = extra_view_info->resource_id;
+
+  if (text.empty() && resource_id != -1) {

[Devlin, 2016/08/22 20:59:42]

Why do we return if resource_id != -1?

[catmullings, 2016/08/31 00:03:30]

Should be == .

Done.

     return nullptr;
+  }
+
+  views::View* parent = nullptr;
+
+  if (resource_id != -1 && !text.empty()) {
+    parent = new views::View();
+  }
+
+  if (resource_id != -1) {
+    views::ImageView* icon = new views::ImageView();
+    icon->SetImage(
+        ResourceBundle::GetSharedInstance().GetImageSkiaNamed(resource_id));
+    if (text.empty()) {

[Devlin, 2016/08/22 20:59:42]

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?

[catmullings, 2016/08/31 00:03:31]

Will followup with you about this offline. 

[catmullings, 2016/09/06 19:16:23]

What do you think of this change?

+      return icon;
+    } else {
+      parent->AddChildView(icon);
+    }
+  }
+
+  if (!text.empty()) {
+    if (extra_view_info->is_text_linked) {
+      link_ = new views::Link(text);
+      link_->set_listener(this);
+      if (resource_id == -1) {
+        return link_;
+      } else {
+        parent->AddChildView(link_);
+      }
+    } else {
+      views::Label* label = new views::Label(text);
+      if (resource_id == -1) {
+        return label;
+      } else {
+        parent->AddChildView(label);
+      }
+    }
+  }
 
-  learn_more_button_ = new views::Link(text);
-  learn_more_button_->set_listener(this);
-  return learn_more_button_;
+  return parent;
 }
 
 base::string16 ToolbarActionsBarBubbleViews::GetWindowTitle() const {