Fix - Permission icons are seen red in colour in site info overlay

Bug: 1096944
Change-Id: If9f17a927771731f8c009a724a3e34d84f73fadb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2299629
Commit-Queue: Ravjit Singh Uppal <ravjit@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792190}

chrome/browser/ui/BUILD.gn

@@ -3591,6 +3591,8 @@ static_library("ui") {
       "views/page_info/page_info_bubble_view_base.h",
       "views/page_info/page_info_hover_button.cc",
       "views/page_info/page_info_hover_button.h",
+      "views/page_info/permission_icon.cc",
+      "views/page_info/permission_icon.h",
       "views/page_info/permission_selector_row.cc",
       "views/page_info/permission_selector_row.h",
       "views/page_info/permission_selector_row_observer.h",

chrome/browser/ui/views/page_info/chosen_object_view.cc

@@ -74,8 +74,6 @@ ChosenObjectView::ChosenObjectView(
   // Create the label that displays the chosen object name.
   auto label =
       std::make_unique<views::Label>(display_name, CONTEXT_BODY_TEXT_LARGE);
-  icon_->SetImage(
-      PageInfoUI::GetChosenObjectIcon(*info_, false, label->GetEnabledColor()));
   layout->AddView(std::move(label));
 
   // Create the delete button.
@@ -138,10 +136,7 @@ ChosenObjectView::~ChosenObjectView() {}
 void ChosenObjectView::ButtonPressed(views::Button* sender,
                                      const ui::Event& event) {
   // Change the icon to reflect the selected setting.
-  icon_->SetImage(PageInfoUI::GetChosenObjectIcon(
-      *info_, true,
-      views::style::GetColor(*this, views::style::CONTEXT_LABEL,
-                             views::style::STYLE_PRIMARY)));
+  UpdateIconImage(/*is_deleted=*/true);
 
   DCHECK(delete_button_->GetVisible());
   delete_button_->SetVisible(false);
@@ -150,3 +145,16 @@ void ChosenObjectView::ButtonPressed(views::Button* sender,
     observer.OnChosenObjectDeleted(*info_);
   }
 }
+
+void ChosenObjectView::OnThemeChanged() {
+  views::View::OnThemeChanged();
+  UpdateIconImage(/*is_deleted=*/false);
+}
+
+void ChosenObjectView::UpdateIconImage(bool is_deleted) const {
+  // TODO(crbug.com/1096944): Why are we using label color for an icon?
+  icon_->SetImage(PageInfoUI::GetChosenObjectIcon(
+      *info_, is_deleted,
+      views::style::GetColor(*this, views::style::CONTEXT_LABEL,
+                             views::style::STYLE_PRIMARY)));
+}

chrome/browser/ui/views/page_info/chosen_object_view.h

@@ -29,10 +29,17 @@ class ChosenObjectView : public views::View, public views::ButtonListener {
 
   void AddObserver(ChosenObjectViewObserver* observer);
 
- private:
+  // views:View:
+  void OnThemeChanged() override;
+
   // views::ButtonListener implementation.
   void ButtonPressed(views::Button* sender, const ui::Event& event) override;
 
+ private:
+  SkColor GetObjectIconColor() const;
+
+  void UpdateIconImage(bool is_deleted) const;
+
   views::ImageView* icon_;             // Owned by the views hierarchy.
   views::ImageButton* delete_button_;  // Owned by the views hierarchy.
 

chrome/browser/ui/views/page_info/permission_icon.cc

+// Copyright 2020 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/page_info/permission_icon.h"
+
+#include "chrome/browser/ui/views/chrome_typography.h"
+
+PermissionIcon::PermissionIcon(
+    const PageInfoUI::PermissionInfo& permission_info)
+    : permission_info_(permission_info) {}
+
+void PermissionIcon::OnPermissionChanged(
+    const PageInfoUI::PermissionInfo& permission_info) {
+  permission_info_ = permission_info;
+  UpdateImage();
+}
+
+void PermissionIcon::OnThemeChanged() {
+  NonAccessibleImageView::OnThemeChanged();
+  UpdateImage();
+}
+
+void PermissionIcon::UpdateImage() {
+  SetImage(PageInfoUI::GetPermissionIcon(
+      permission_info_,
+      views::style::GetColor(*this, views::style::CONTEXT_LABEL,
+                             views::style::STYLE_PRIMARY)));
+}

chrome/browser/ui/views/page_info/permission_icon.h

+// Copyright 2020 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.
+
+#ifndef CHROME_BROWSER_UI_VIEWS_PAGE_INFO_PERMISSION_ICON_H_
+#define CHROME_BROWSER_UI_VIEWS_PAGE_INFO_PERMISSION_ICON_H_
+
+#include "chrome/browser/ui/views/accessibility/non_accessible_image_view.h"
+#include "components/page_info/page_info_ui.h"
+#include "third_party/skia/include/core/SkColor.h"
+
+class PermissionIcon : public NonAccessibleImageView {
+ public:
+  explicit PermissionIcon(const PageInfoUI::PermissionInfo& permission_info);
+
+  PermissionIcon(const PermissionIcon&) = delete;
+  PermissionIcon& operator=(const PermissionIcon&) = delete;
+
+  ~PermissionIcon() override = default;
+
+  void OnPermissionChanged(const PageInfoUI::PermissionInfo& permission_info);
+
+  // NonAccessibleImageView:
+  void OnThemeChanged() override;
+
+ private:
+  SkColor GetIconColor() const;
+
+  void UpdateImage();
+
+  PageInfoUI::PermissionInfo permission_info_;
+};
+
+#endif  // CHROME_BROWSER_UI_VIEWS_PAGE_INFO_PERMISSION_ICON_H_

chrome/browser/ui/views/page_info/permission_selector_row.cc

@@ -14,6 +14,7 @@
 #include "chrome/browser/ui/views/chrome_layout_provider.h"
 #include "chrome/browser/ui/views/chrome_typography.h"
 #include "chrome/browser/ui/views/page_info/page_info_bubble_view.h"
+#include "chrome/browser/ui/views/page_info/permission_icon.h"
 #include "components/page_info/page_info_ui.h"
 #include "components/strings/grit/components_strings.h"
 #include "ui/accessibility/ax_node_data.h"
@@ -160,7 +161,7 @@ PermissionSelectorRow::PermissionSelectorRow(
     const GURL& url,
     const PageInfoUI::PermissionInfo& permission,
     views::GridLayout* layout)
-    : profile_(profile), icon_(nullptr), combobox_(nullptr) {
+    : profile_(profile) {
   const int list_item_padding = ChromeLayoutProvider::Get()->GetDistanceMetric(
                                     DISTANCE_CONTROL_LIST_VERTICAL) /
                                 2;
@@ -168,13 +169,11 @@ PermissionSelectorRow::PermissionSelectorRow(
                               views::GridLayout::kFixedSize, list_item_padding);
 
   // Create the permission icon and label.
-  icon_ = layout->AddView(std::make_unique<NonAccessibleImageView>());
+  icon_ = layout->AddView(std::make_unique<PermissionIcon>(permission));
   // Create the label that displays the permission type.
   auto label = std::make_unique<views::Label>(
       PageInfoUI::PermissionTypeToUIString(permission.type),
       CONTEXT_BODY_TEXT_LARGE);
-  icon_->SetImage(
-      PageInfoUI::GetPermissionIcon(permission, label->GetEnabledColor()));
   label_ = layout->AddView(std::move(label));
   // Create the menu model.
   menu_model_ = std::make_unique<PermissionMenuModel>(
@@ -285,8 +284,7 @@ void PermissionSelectorRow::InitializeComboboxView(
 void PermissionSelectorRow::PermissionChanged(
     const PageInfoUI::PermissionInfo& permission) {
   // Change the permission icon to reflect the selected setting.
-  icon_->SetImage(
-      PageInfoUI::GetPermissionIcon(permission, label_->GetEnabledColor()));
+  icon_->OnPermissionChanged(permission);
 
   bool use_default = permission.setting == CONTENT_SETTING_DEFAULT;
   auto* combobox = static_cast<internal::PermissionCombobox*>(combobox_);

chrome/browser/ui/views/page_info/permission_selector_row.h

@@ -16,6 +16,7 @@
 #include "components/content_settings/core/common/content_settings_types.h"
 #include "components/page_info/page_info_ui.h"
 
+class PermissionIcon;
 class Profile;
 
 namespace internal {
@@ -28,7 +29,6 @@ class PageInfoBubbleViewTestApi;
 
 namespace views {
 class GridLayout;
-class ImageView;
 class Label;
 class View;
 class Combobox;
@@ -79,9 +79,9 @@ class PermissionSelectorRow {
   std::unique_ptr<internal::ComboboxModelAdapter> combobox_model_adapter_;
 
   // These are all owned by the views hierarchy:
-  views::ImageView* icon_;
-  views::Label* label_;
-  views::Combobox* combobox_;
+  PermissionIcon* icon_ = nullptr;
+  views::Label* label_ = nullptr;
+  views::Combobox* combobox_ = nullptr;
 
   base::ObserverList<PermissionSelectorRowObserver, false>::Unchecked
       observer_list_;