Improve settings override bubble to indicate policy installed extensions

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.h"
 
 #include "chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h"
 #include "chrome/browser/ui/view_ids.h"
 #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"
 #include "ui/views/layout/layout_constants.h"
 
 namespace {
 const int kListPadding = 10;
 }
 
 ToolbarActionsBarBubbleViews::ToolbarActionsBarBubbleViews(
     views::View* anchor_view,
     std::unique_ptr<ToolbarActionsBarBubbleDelegate> delegate)
     : views::BubbleDialogDelegateView(anchor_view,
                                       views::BubbleBorder::TOP_RIGHT),
       delegate_(std::move(delegate)),
       item_list_(nullptr),
-      learn_more_button_(nullptr) {
+      link_(nullptr) {
   set_close_on_deactivate(delegate_->ShouldCloseOnDeactivate());
 }
 
 ToolbarActionsBarBubbleViews::~ToolbarActionsBarBubbleViews() {}
 
 void ToolbarActionsBarBubbleViews::Show() {
   delegate_->OnBubbleShown();
   GetWidget()->Show();
 }
 
 views::View* ToolbarActionsBarBubbleViews::CreateExtraView() {

[Devlin, 2016/09/07 18:37:50]

Nice! This method already reads a lot better than it did.

[catmullings, 2016/09/08 15:42:30]

Done.

-  base::string16 text = delegate_->GetLearnMoreButtonText();
-  if (text.empty())
+  std::unique_ptr<ToolbarActionsBarBubbleDelegate::ExtraViewInfo>
+      extra_view_info = delegate_->GetExtraViewInfo();
+
+  if (!extra_view_info)
     return nullptr;
 
-  learn_more_button_ = new views::Link(text);
-  learn_more_button_->set_listener(this);
-  return learn_more_button_;
+  base::string16& text = extra_view_info->text;

[Devlin, 2016/09/07 18:37:50]

const&

[catmullings, 2016/09/08 15:42:30]

Done.

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

[Devlin, 2016/09/07 18:37:49]

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.

[catmullings, 2016/09/08 15:42:30]

This is much cleaner and easier to follow.
Change is made.

[catmullings, 2016/09/08 15:42:30]

Done.

+    return icon;
+  }
+
+  if (!text.empty())
+    return ConvertTextToView(text, extra_view_info->is_text_linked);
+
+  return nullptr;
 }
 
 base::string16 ToolbarActionsBarBubbleViews::GetWindowTitle() const {
   return delegate_->GetHeadingText();
 }
 
 bool ToolbarActionsBarBubbleViews::Cancel() {
   delegate_->OnBubbleClosed(
       ToolbarActionsBarBubbleDelegate::CLOSE_DISMISS_USER_ACTION);
   return true;
@@ skipping 57 matching lines @@
                                         : delegate_->GetDismissButtonText();
 }
 
 void ToolbarActionsBarBubbleViews::LinkClicked(views::Link* link,
                                                int event_flags) {
   delegate_->OnBubbleClosed(ToolbarActionsBarBubbleDelegate::CLOSE_LEARN_MORE);
   // Reset delegate so we don't send extra OnBubbleClosed()s.
   delegate_.reset();
   GetWidget()->Close();
 }
+
+views::View* ToolbarActionsBarBubbleViews::ConvertTextToView(
+    const base::string16& text,
+    bool is_text_linked) {
+  if (is_text_linked) {
+    link_ = new views::Link(text);

[Devlin, 2016/09/07 18:37:50]

I think we can make link_ a views::Label* label_, and use it in both cases
(views::Link inherits from views::Label).

[catmullings, 2016/09/08 15:42:29]

I removed this method since it is no longer need in CreateExtraView().

[catmullings, 2016/09/08 15:42:30]

Done.

+    link_->set_listener(this);
+    return link_;
+  } else {
+    views::Label* label = new views::Label(text);
+    return label;
+  }
+}