Revert "Fixed color bugs in CustomTabBarView"

This reverts commit a1342aa6.

Reason for revert: Broke MSAN bots:
https://ci.chromium.org/p/chromium/builders/ci/Linux%20MSan%20Tests/21649
https://ci.chromium.org/p/chromium/builders/ci/Linux%20ChromiumOS%20MSan%20Tests/17229

Stack trace from crash:
==29357==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x56541d07524c in views::Label::SetBackgroundColor(unsigned int) ./../../ui/views/controls/label.cc:141:7
    #1 0x5654292ce540 in IconLabelBubbleView::OnThemeChanged() ./../../chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:308:12
    #2 0x565429331693 in LocationIconView::UpdateTextVisibility(bool) ./../../chrome/browser/ui/views/location_bar/location_icon_view.cc:195:3
    #3 0x56542932f4ff in LocationIconView::Update(bool) ./../../chrome/browser/ui/views/location_bar/location_icon_view.cc:214:3
    #4 0x56541d29bb17 in views::View::PropagateAddNotifications(views::ViewHierarchyChangedDetails const&, bool) ./../../ui/views/view.cc:2326:5
    #5 0x56541d29ba53 in views::View::PropagateAddNotifications(views::ViewHierarchyChangedDetails const&, bool) ./../../ui/views/view.cc:2322:14
    #6 0x56541d2972cf in views::View::AddChildViewAtImpl(views::View*, int) ./../../ui/views/view.cc:2218:9
[...]

Original change's description:
> Fixed color bugs in CustomTabBarView
> 
> Fixed CustomTabBarView to set and update its colors only once
> attached to a widget, preventing the fallback to the default
> NativeTheme.
> 
> Updated CustomTabBarView to respond to theme changes.
> 
> Removed the dependency on GetThemeProviderForProfile from
> CustomTabBarView and from LocationBarView.
> 
> Bug: None
> Change-Id: If505b0bede8f112329dcef5f2a7aab9e67dcfd2e
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1989085
> Commit-Queue: Thomas Lukaszewicz <tluk@chromium.org>
> Reviewed-by: Peter Kasting <pkasting@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#729904}

TBR=pkasting@chromium.org,tluk@chromium.org

Change-Id: I768a90892980c5522075ed7cda38e2678b797552
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: None
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1994661Reviewed-by: Fabrice de Gans-Riberi <fdegans@chromium.org>
Commit-Queue: Fabrice de Gans-Riberi <fdegans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#730020}

chrome/browser/ui/views/location_bar/custom_tab_bar_view.cc

@@ -54,8 +54,12 @@
 namespace {
 
 std::unique_ptr<views::ImageButton> CreateCloseButton(
-    views::ButtonListener* listener) {
+    views::ButtonListener* listener,
+    SkColor color) {
   auto close_button = CreateVectorImageButton(listener);
+  SetImageFromVectorIconWithColor(
+      close_button.get(), vector_icons::kCloseRoundedIcon,
+      GetLayoutConstant(LOCATION_BAR_ICON_SIZE), color);
   close_button->SetTooltipText(l10n_util::GetStringUTF16(IDS_APP_CLOSE));
   close_button->SetBorder(views::CreateEmptyBorder(
       gfx::Insets(GetLayoutConstant(LOCATION_BAR_CHILD_INTERIOR_PADDING))));
@@ -81,7 +85,7 @@ bool ShouldDisplayUrl(content::WebContents* contents) {
 // page.
 class CustomTabBarTitleOriginView : public views::View {
  public:
-  CustomTabBarTitleOriginView() {
+  explicit CustomTabBarTitleOriginView(SkColor background_color) {
     auto title_label = std::make_unique<views::Label>(
         base::string16(), CONTEXT_BODY_TEXT_LARGE,
         views::style::TextStyle::STYLE_PRIMARY);
@@ -90,6 +94,7 @@ class CustomTabBarTitleOriginView : public views::View {
         views::style::STYLE_SECONDARY,
         gfx::DirectionalityMode::DIRECTIONALITY_AS_URL);
 
+    title_label->SetBackgroundColor(background_color);
     title_label->SetElideBehavior(gfx::ElideBehavior::ELIDE_TAIL);
     title_label->SetHorizontalAlignment(gfx::HorizontalAlignment::ALIGN_LEFT);
     title_label->SetProperty(views::kFlexBehaviorKey,
@@ -97,6 +102,7 @@ class CustomTabBarTitleOriginView : public views::View {
                                  views::MinimumFlexSizeRule::kScaleToMinimum,
                                  views::MaximumFlexSizeRule::kPreferred));
 
+    location_label->SetBackgroundColor(background_color);
     location_label->SetElideBehavior(gfx::ElideBehavior::ELIDE_HEAD);
     location_label->SetHorizontalAlignment(
         gfx::HorizontalAlignment::ALIGN_LEFT);
@@ -120,11 +126,6 @@ class CustomTabBarTitleOriginView : public views::View {
     location_label_->SetVisible(!location.empty());
   }
 
-  void SetColors(SkColor background_color) {
-    title_label_->SetBackgroundColor(background_color);
-    location_label_->SetBackgroundColor(background_color);
-  }
-
   int GetMinimumWidth() const {
     // As labels are not multi-line, the layout will calculate a minimum size
     // that would fit the entire text (potentially a long url). Instead, set a
@@ -173,16 +174,38 @@ CustomTabBarView::CustomTabBarView(BrowserView* browser_view,
       delegate_(delegate),
       browser_(browser_view->browser()) {
   set_context_menu_controller(this);
+  base::Optional<SkColor> optional_theme_color =
+      browser_->app_controller()->GetThemeColor();
+
+  const bool dark_mode = GetNativeTheme()->ShouldUseDarkColors();
+  const SkColor default_frame_color =
+#if defined(OS_CHROMEOS)
+      // Ash system frames differ from ChromeOS browser frames.
+      ash::kDefaultFrameColor;
+#else
+      ThemeProperties::GetDefaultColor(ThemeProperties::COLOR_FRAME, false,
+                                       dark_mode);
+#endif
+
+  title_bar_color_ = optional_theme_color.value_or(default_frame_color);
+
+  background_color_ = dark_mode ? default_frame_color : SK_ColorWHITE;
+
+  SetBackground(views::CreateSolidBackground(background_color_));
+
+  const SkColor foreground_color =
+      color_utils::GetColorWithMaxContrast(background_color_);
 
   const gfx::FontList& font_list = views::style::GetFont(
       CONTEXT_OMNIBOX_PRIMARY, views::style::STYLE_PRIMARY);
 
-  close_button_ = AddChildView(CreateCloseButton(this));
+  close_button_ = AddChildView(CreateCloseButton(this, foreground_color));
 
   location_icon_view_ =
       AddChildView(std::make_unique<LocationIconView>(font_list, this, this));
 
-  auto title_origin_view = std::make_unique<CustomTabBarTitleOriginView>();
+  auto title_origin_view =
+      std::make_unique<CustomTabBarTitleOriginView>(background_color_);
   title_origin_view->SetProperty(
       views::kFlexBehaviorKey, views::FlexSpecification::ForSizeRule(
                                    views::MinimumFlexSizeRule::kScaleToMinimum,
@@ -209,6 +232,55 @@ const char* CustomTabBarView::GetClassName() const {
   return kViewClassName;
 }
 
+void CustomTabBarView::TabChangedAt(content::WebContents* contents,
+                                    int index,
+                                    TabChangeType change_type) {
+  if (!contents)
+    return;
+
+  // If the toolbar should not be shown don't update the UI, as the toolbar may
+  // be animating out and it looks messy.
+  Browser* browser = chrome::FindBrowserWithWebContents(contents);
+  if (!browser->app_controller()->ShouldShowCustomTabBar())
+    return;
+
+  content::NavigationEntry* entry = contents->GetController().GetVisibleEntry();
+  base::string16 title, location;
+  if (entry) {
+    title = Browser::FormatTitleForDisplay(entry->GetTitleForDisplay());
+    if (ShouldDisplayUrl(contents))
+      location = url_formatter::FormatUrl(entry->GetVirtualURL().GetOrigin(),
+                                          url_formatter::kFormatUrlOmitDefaults,
+                                          net::UnescapeRule::NORMAL, nullptr,
+                                          nullptr, nullptr);
+  }
+
+  title_origin_view_->Update(title, location);
+  location_icon_view_->Update(/*suppress animations = */ false);
+
+  // Hide location icon if we're already hiding the origin.
+  location_icon_view_->SetVisible(!location.empty());
+
+  last_title_ = title;
+  last_location_ = location;
+
+  web_app::AppBrowserController* app_controller =
+      chrome::FindBrowserWithWebContents(contents)->app_controller();
+  const bool started_in_scope =
+      app_controller->IsUrlInAppScope(app_controller->initial_url());
+
+  // Only show the 'X' button if:
+  // a) The current url is not in scope (no point showing a back to app button
+  // while in scope).
+  // And b), if the window started in scope (this is
+  // important for popup windows, which may be opened outside the app).
+  close_button_->SetVisible(
+      started_in_scope &&
+      !app_controller->IsUrlInAppScope(contents->GetLastCommittedURL()));
+
+  Layout();
+}
+
 gfx::Size CustomTabBarView::CalculatePreferredSize() const {
   // ToolbarView::GetMinimumSize() uses the preferred size of its children, so
   // tell it the minimum size this control will fit into (its layout will
@@ -258,83 +330,27 @@ void CustomTabBarView::ChildPreferredSizeChanged(views::View* child) {
   SchedulePaint();
 }
 
-void CustomTabBarView::OnThemeChanged() {
-  base::Optional<SkColor> optional_theme_color =
-      browser_->app_controller()->GetThemeColor();
-
-  const bool dark_mode = GetNativeTheme()->ShouldUseDarkColors();
-  const SkColor default_frame_color =
-#if defined(OS_CHROMEOS)
-      // Ash system frames differ from ChromeOS browser frames.
-      ash::kDefaultFrameColor;
-#else
-      ThemeProperties::GetDefaultColor(ThemeProperties::COLOR_FRAME, false,
-                                       dark_mode);
-#endif
-
-  title_bar_color_ = optional_theme_color.value_or(default_frame_color);
-
-  background_color_ = dark_mode ? default_frame_color : SK_ColorWHITE;
-
-  SetBackground(views::CreateSolidBackground(background_color_));
-
-  const SkColor foreground_color =
-      color_utils::GetColorWithMaxContrast(background_color_);
-
-  SetImageFromVectorIconWithColor(
-      close_button_, vector_icons::kCloseRoundedIcon,
-      GetLayoutConstant(LOCATION_BAR_ICON_SIZE), foreground_color);
-
-  title_origin_view_->SetColors(background_color_);
+void CustomTabBarView::ShowContextMenuForViewImpl(
+    views::View* source,
+    const gfx::Point& point,
+    ui::MenuSourceType source_type) {
+  if (!context_menu_model_) {
+    context_menu_model_ = std::make_unique<ui::SimpleMenuModel>(this);
+    context_menu_model_->AddItemWithStringId(IDC_COPY_URL, IDS_COPY_URL);
+  }
+  context_menu_runner_ = std::make_unique<views::MenuRunner>(
+      context_menu_model_.get(),
+      views::MenuRunner::HAS_MNEMONICS | views::MenuRunner::CONTEXT_MENU);
+  context_menu_runner_->RunMenuAt(
+      views::View::GetWidget(), nullptr, gfx::Rect(point, gfx::Size()),
+      views::MenuAnchorPosition::kTopLeft, source_type);
 }
 
-void CustomTabBarView::TabChangedAt(content::WebContents* contents,
-                                    int index,
-                                    TabChangeType change_type) {
-  if (!contents)
-    return;
-
-  // If the toolbar should not be shown don't update the UI, as the toolbar may
-  // be animating out and it looks messy.
-  Browser* browser = chrome::FindBrowserWithWebContents(contents);
-  if (!browser->app_controller()->ShouldShowCustomTabBar())
-    return;
-
-  content::NavigationEntry* entry = contents->GetController().GetVisibleEntry();
-  base::string16 title, location;
-  if (entry) {
-    title = Browser::FormatTitleForDisplay(entry->GetTitleForDisplay());
-    if (ShouldDisplayUrl(contents))
-      location = url_formatter::FormatUrl(entry->GetVirtualURL().GetOrigin(),
-                                          url_formatter::kFormatUrlOmitDefaults,
-                                          net::UnescapeRule::NORMAL, nullptr,
-                                          nullptr, nullptr);
+void CustomTabBarView::ExecuteCommand(int command_id, int event_flags) {
+  if (command_id == IDC_COPY_URL) {
+    base::RecordAction(base::UserMetricsAction("CopyCustomTabBarUrl"));
+    chrome::ExecuteCommand(browser_, command_id);
   }
-
-  title_origin_view_->Update(title, location);
-  location_icon_view_->Update(/*suppress animations = */ false);
-
-  // Hide location icon if we're already hiding the origin.
-  location_icon_view_->SetVisible(!location.empty());
-
-  last_title_ = title;
-  last_location_ = location;
-
-  web_app::AppBrowserController* app_controller =
-      chrome::FindBrowserWithWebContents(contents)->app_controller();
-  const bool started_in_scope =
-      app_controller->IsUrlInAppScope(app_controller->initial_url());
-
-  // Only show the 'X' button if:
-  // a) The current url is not in scope (no point showing a back to app button
-  // while in scope).
-  // And b), if the window started in scope (this is
-  // important for popup windows, which may be opened outside the app).
-  close_button_->SetVisible(
-      started_in_scope &&
-      !app_controller->IsUrlInAppScope(contents->GetLastCommittedURL()));
-
-  Layout();
 }
 
 SkColor CustomTabBarView::GetIconLabelBubbleSurroundingForegroundColor() const {
@@ -357,11 +373,6 @@ void CustomTabBarView::OnLocationIconPressed(const ui::MouseEvent& event) {}
 
 void CustomTabBarView::OnLocationIconDragged(const ui::MouseEvent& event) {}
 
-SkColor CustomTabBarView::GetSecurityChipColor(
-    security_state::SecurityLevel security_level) const {
-  return GetOmniboxSecurityChipColor(GetThemeProvider(), security_level);
-}
-
 bool CustomTabBarView::ShowPageInfoDialog() {
   return ::ShowPageInfoDialog(
       GetWebContents(),
@@ -370,8 +381,11 @@ bool CustomTabBarView::ShowPageInfoDialog() {
       bubble_anchor_util::Anchor::kCustomTabBar);
 }
 
-const LocationBarModel* CustomTabBarView::GetLocationBarModel() const {
-  return delegate_->GetLocationBarModel();
+SkColor CustomTabBarView::GetSecurityChipColor(
+    security_state::SecurityLevel security_level) const {
+  return GetOmniboxSecurityChipColor(
+      &ThemeService::GetThemeProviderForProfile(browser_->profile()),
+      security_level);
 }
 
 gfx::ImageSkia CustomTabBarView::GetLocationIcon(
@@ -382,6 +396,10 @@ gfx::ImageSkia CustomTabBarView::GetLocationIcon(
       GetSecurityChipColor(GetLocationBarModel()->GetSecurityLevel()));
 }
 
+const LocationBarModel* CustomTabBarView::GetLocationBarModel() const {
+  return delegate_->GetLocationBarModel();
+}
+
 void CustomTabBarView::ButtonPressed(views::Button* sender,
                                      const ui::Event& event) {
   GoBackToApp();
@@ -391,11 +409,6 @@ void CustomTabBarView::GoBackToAppForTesting() {
   GoBackToApp();
 }
 
-bool CustomTabBarView::IsShowingOriginForTesting() const {
-  return title_origin_view_ != nullptr &&
-         title_origin_view_->IsShowingOriginForTesting();
-}
-
 void CustomTabBarView::GoBackToApp() {
   content::WebContents* web_contents = GetWebContents();
   web_app::AppBrowserController* app_controller =
@@ -443,25 +456,7 @@ void CustomTabBarView::AppInfoClosedCallback(
   GetFocusManager()->SetFocusedView(location_icon_view_);
 }
 
-void CustomTabBarView::ExecuteCommand(int command_id, int event_flags) {
-  if (command_id == IDC_COPY_URL) {
-    base::RecordAction(base::UserMetricsAction("CopyCustomTabBarUrl"));
-    chrome::ExecuteCommand(browser_, command_id);
-  }
-}
-
-void CustomTabBarView::ShowContextMenuForViewImpl(
-    views::View* source,
-    const gfx::Point& point,
-    ui::MenuSourceType source_type) {
-  if (!context_menu_model_) {
-    context_menu_model_ = std::make_unique<ui::SimpleMenuModel>(this);
-    context_menu_model_->AddItemWithStringId(IDC_COPY_URL, IDS_COPY_URL);
-  }
-  context_menu_runner_ = std::make_unique<views::MenuRunner>(
-      context_menu_model_.get(),
-      views::MenuRunner::HAS_MNEMONICS | views::MenuRunner::CONTEXT_MENU);
-  context_menu_runner_->RunMenuAt(
-      views::View::GetWidget(), nullptr, gfx::Rect(point, gfx::Size()),
-      views::MenuAnchorPosition::kTopLeft, source_type);
+bool CustomTabBarView::IsShowingOriginForTesting() const {
+  return title_origin_view_ != nullptr &&
+         title_origin_view_->IsShowingOriginForTesting();
 }

chrome/browser/ui/views/location_bar/custom_tab_bar_view.h

@@ -13,6 +13,7 @@
 #include "ui/base/models/simple_menu_model.h"
 #include "ui/views/accessible_pane_view.h"
 #include "ui/views/context_menu_controller.h"
+#include "ui/views/controls/button/button.h"
 
 namespace gfx {
 class Rect;
@@ -21,7 +22,6 @@ class Rect;
 namespace views {
 class FlexLayout;
 class MenuRunner;
-class ImageButton;
 }
 
 class CustomTabBarTitleOriginView;
@@ -47,19 +47,20 @@ class CustomTabBarView : public views::AccessiblePaneView,
 
   LocationIconView* location_icon_view() { return location_icon_view_; }
 
-  // views::AccessiblePaneView:
+  // views::View:
   gfx::Rect GetAnchorBoundsInScreen() const override;
   const char* GetClassName() const override;
-  gfx::Size CalculatePreferredSize() const override;
-  void OnPaintBackground(gfx::Canvas* canvas) override;
-  void ChildPreferredSizeChanged(views::View* child) override;
-  void OnThemeChanged() override;
 
   // TabstripModelObserver:
   void TabChangedAt(content::WebContents* contents,
                     int index,
                     TabChangeType change_type) override;
 
+  // views::View:
+  gfx::Size CalculatePreferredSize() const override;
+  void OnPaintBackground(gfx::Canvas* canvas) override;
+  void ChildPreferredSizeChanged(views::View* child) override;
+
   // IconLabelBubbleView::Delegate:
   SkColor GetIconLabelBubbleSurroundingForegroundColor() const override;
   SkColor GetIconLabelBubbleBackgroundColor() const override;
@@ -82,7 +83,7 @@ class CustomTabBarView : public views::AccessiblePaneView,
   // Methods for testing.
   base::string16 title_for_testing() const { return last_title_; }
   base::string16 location_for_testing() const { return last_location_; }
-  views::ImageButton* close_button_for_testing() const { return close_button_; }
+  views::Button* close_button_for_testing() const { return close_button_; }
   ui::SimpleMenuModel* context_menu_for_testing() const {
     return context_menu_model_.get();
   }
@@ -112,7 +113,7 @@ class CustomTabBarView : public views::AccessiblePaneView,
   base::string16 last_title_;
   base::string16 last_location_;
 
-  views::ImageButton* close_button_ = nullptr;
+  views::Button* close_button_ = nullptr;
   LocationBarView::Delegate* delegate_ = nullptr;
   LocationIconView* location_icon_view_ = nullptr;
   CustomTabBarTitleOriginView* title_origin_view_ = nullptr;

chrome/browser/ui/views/location_bar/custom_tab_bar_view_browsertest.cc

@@ -26,7 +26,6 @@
 #include "content/public/test/test_navigation_observer.h"
 #include "net/dns/mock_host_resolver.h"
 #include "ui/base/clipboard/clipboard.h"
-#include "ui/views/controls/button/image_button.h"
 
 namespace {
 

chrome/browser/ui/views/location_bar/location_bar_view.cc

@@ -305,8 +305,8 @@ bool LocationBarView::IsInitialized() const {
 }
 
 SkColor LocationBarView::GetColor(OmniboxPart part) const {
-  DCHECK(GetWidget());
-  return GetOmniboxColor(GetThemeProvider(), part);
+  return GetOmniboxColor(&ThemeService::GetThemeProviderForProfile(profile_),
+                         part);
 }
 
 SkColor LocationBarView::GetOpaqueBorderColor() const {

chrome/browser/ui/views/location_bar/location_icon_view.cc

@@ -36,6 +36,7 @@ LocationIconView::LocationIconView(
   DCHECK(delegate_);
 
   SetID(VIEW_ID_LOCATION_ICON);
+  Update(true);
   SetUpForAnimation();
 
   // Readability is guaranteed by the omnibox theme.
@@ -100,10 +101,6 @@ void LocationIconView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   node_data->role = ax::mojom::Role::kPopUpButton;
 }
 
-void LocationIconView::AddedToWidget() {
-  Update(true);
-}
-
 int LocationIconView::GetMinimumLabelTextWidth() const {
   int width = 0;
 

chrome/browser/ui/views/location_bar/location_icon_view.h

@@ -72,7 +72,6 @@ class LocationIconView : public IconLabelBubbleView {
   bool IsBubbleShowing() const override;
   bool OnMousePressed(const ui::MouseEvent& event) override;
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override;
-  void AddedToWidget() override;
 
   // Returns what the minimum width for the label text.
   int GetMinimumLabelTextWidth() const;