From cc69e24bfc8cd6fef53e2b94379ea26b6ce3f90f Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Tue, 2 Jun 2026 23:53:13 +0900 Subject: [PATCH] Revert "Fixed download toolbar icon regressions with insecure file" (#36929) Reverts #36874 Original PR had one more commit but it's missed. --- browser/ui/views/download/bubble/BUILD.gn | 24 --- ...nload_toolbar_ui_controller_browsertest.cc | 147 ------------------ ...le-download_toolbar_ui_controller.cc.patch | 14 +- .../download_toolbar_ui_controller.cc.toml | 13 +- test/BUILD.gn | 1 - 5 files changed, 13 insertions(+), 186 deletions(-) delete mode 100644 browser/ui/views/download/bubble/download_toolbar_ui_controller_browsertest.cc diff --git a/browser/ui/views/download/bubble/BUILD.gn b/browser/ui/views/download/bubble/BUILD.gn index 5f19f4cd432..dd7572f2062 100644 --- a/browser/ui/views/download/bubble/BUILD.gn +++ b/browser/ui/views/download/bubble/BUILD.gn @@ -3,30 +3,6 @@ # License, v. 2.0. If a copy of the MPL was not distributed with this file, # You can obtain one at https://mozilla.org/MPL/2.0/. -source_set("browser_tests") { - testonly = true - defines = [ "HAS_OUT_OF_PROC_TEST_RUNNER" ] - - sources = [ "download_toolbar_ui_controller_browsertest.cc" ] - - deps = [ - "//base", - "//base/test:test_support", - "//chrome/browser", - "//chrome/browser/download", - "//chrome/browser/ui", - "//chrome/browser/ui/download", - "//chrome/browser/ui/views/download", - "//chrome/test:test_support", - "//components/download/public/common:test_support", - "//components/keyed_service/content", - "//components/vector_icons", - "//content/test:test_support", - "//testing/gmock", - "//testing/gtest", - ] -} - source_set("unit_tests") { testonly = true diff --git a/browser/ui/views/download/bubble/download_toolbar_ui_controller_browsertest.cc b/browser/ui/views/download/bubble/download_toolbar_ui_controller_browsertest.cc deleted file mode 100644 index 96f09208859..00000000000 --- a/browser/ui/views/download/bubble/download_toolbar_ui_controller_browsertest.cc +++ /dev/null @@ -1,147 +0,0 @@ -/* Copyright (c) 2026 The Brave Authors. All rights reserved. - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this file, - * You can obtain one at https://mozilla.org/MPL/2.0/. */ - -#include "chrome/browser/ui/views/download/bubble/download_toolbar_ui_controller.h" - -#include "base/callback_list.h" -#include "base/functional/bind.h" -#include "base/memory/raw_ptr.h" -#include "chrome/browser/download/bubble/download_bubble_update_service.h" -#include "chrome/browser/download/bubble/download_bubble_update_service_factory.h" -#include "chrome/browser/download/download_item_model.h" -#include "chrome/browser/profiles/profile.h" -#include "chrome/browser/ui/download/download_display.h" -#include "chrome/browser/ui/views/frame/browser_view.h" -#include "chrome/browser/ui/views/frame/toolbar_button_provider.h" -#include "chrome/browser/ui/views/toolbar/toolbar_button.h" -#include "chrome/test/base/in_process_browser_test.h" -#include "components/download/public/common/download_item.h" -#include "components/download/public/common/mock_download_item.h" -#include "components/keyed_service/content/browser_context_dependency_manager.h" -#include "components/vector_icons/vector_icons.h" -#include "content/public/browser/browser_context.h" -#include "content/public/test/browser_test.h" -#include "testing/gmock/include/gmock/gmock.h" -#include "testing/gtest/include/gtest/gtest.h" - -using ::testing::NiceMock; -using ::testing::Return; - -namespace { - -// Minimal fake that lets tests control which download items are visible, so -// HasInsecureDownloads() returns a predictable value without needing real -// network activity. -class FakeDownloadBubbleUpdateService : public DownloadBubbleUpdateService { - public: - explicit FakeDownloadBubbleUpdateService(Profile* profile) - : DownloadBubbleUpdateService(profile) {} - - bool IsInitialized() const override { return true; } - - bool GetAllModelsToDisplay( - std::vector& models, - const webapps::AppId* /*web_app_id*/, - bool /*force_backfill*/) override { - models.clear(); - for (download::DownloadItem* item : items_) { - models.push_back(DownloadItemModel::Wrap(item)); - } - return true; - } - - void AddItem(download::DownloadItem* item) { items_.push_back(item); } - - private: - std::vector> items_; -}; - -std::unique_ptr BuildFakeService( - content::BrowserContext* context) { - return std::make_unique( - Profile::FromBrowserContext(context)); -} - -} // namespace - -class DownloadToolbarInsecureIconTest : public InProcessBrowserTest { - public: - void SetUpInProcessBrowserTestFixture() override { - InProcessBrowserTest::SetUpInProcessBrowserTestFixture(); - // Register the fake service factory before the browser (and its profile) - // is created, so DownloadBubbleUIController picks it up from the start and - // no dangling raw_ptr is left to the replaced service. - subscription_ = - BrowserContextDependencyManager::GetInstance() - ->RegisterCreateServicesCallbackForTesting( - base::BindRepeating([](content::BrowserContext* context) { - DownloadBubbleUpdateServiceFactory::GetInstance() - ->SetTestingFactory( - context, base::BindRepeating(&BuildFakeService)); - })); - } - - FakeDownloadBubbleUpdateService* fake_service() { - return static_cast( - DownloadBubbleUpdateServiceFactory::GetForProfile( - browser()->profile())); - } - - ToolbarButton* download_button() { - return BrowserView::GetBrowserViewForBrowser(browser()) - ->toolbar_button_provider() - ->GetDownloadButton(); - } - - DownloadToolbarUIController* controller() { - return DownloadToolbarUIController::From(browser()); - } - - private: - base::CallbackListSubscription subscription_; -}; - -// Verifies that UpdateIcon_BraveImpl sets / clears the toolbar button colour -// override according to the insecure-download status of the current models. -// Covers: no insecure download, WARN status, and BLOCK status. -IN_PROC_BROWSER_TEST_F(DownloadToolbarInsecureIconTest, InsecureDownloadIcon) { - auto* ctrl = controller(); - ASSERT_NE(nullptr, ctrl) << "controller should be available"; - ctrl->Show(); - - auto* btn = download_button(); - ASSERT_NE(nullptr, btn) << "download button should be present after Show()"; - - NiceMock item; - - // No insecure downloads — colour override must be absent. - ctrl->UpdateDownloadIcon( - {.new_active = DownloadDisplay::IconActive::kActive}); - EXPECT_FALSE(btn->HasIconEnabledColorsOverride()) << "state: no items"; - EXPECT_TRUE(!btn->HasVectorIcons() || - &btn->GetVectorIcon() != &vector_icons::kNotSecureWarningIcon) - << "state: no items; HasVectorIcons=" << btn->HasVectorIcons(); - - // WARN status: warning colour override and warning vector icon must be set. - ON_CALL(item, GetInsecureDownloadStatus()) - .WillByDefault( - Return(download::DownloadItem::InsecureDownloadStatus::WARN)); - fake_service()->AddItem(&item); - ctrl->UpdateDownloadIcon( - {.new_state = DownloadDisplay::IconState::kProgress}); - EXPECT_TRUE(btn->HasIconEnabledColorsOverride()) << "state: WARN"; - EXPECT_EQ(&btn->GetVectorIcon(), &vector_icons::kNotSecureWarningIcon) - << "state: WARN"; - - // BLOCK status is treated the same way. - ON_CALL(item, GetInsecureDownloadStatus()) - .WillByDefault( - Return(download::DownloadItem::InsecureDownloadStatus::BLOCK)); - ctrl->UpdateDownloadIcon( - {.new_state = DownloadDisplay::IconState::kComplete}); - EXPECT_TRUE(btn->HasIconEnabledColorsOverride()) << "state: BLOCK"; - EXPECT_EQ(&btn->GetVectorIcon(), &vector_icons::kNotSecureWarningIcon) - << "state: BLOCK"; -} diff --git a/patches/chrome-browser-ui-views-download-bubble-download_toolbar_ui_controller.cc.patch b/patches/chrome-browser-ui-views-download-bubble-download_toolbar_ui_controller.cc.patch index 34e8db33017..c71be7d7abf 100644 --- a/patches/chrome-browser-ui-views-download-bubble-download_toolbar_ui_controller.cc.patch +++ b/patches/chrome-browser-ui-views-download-bubble-download_toolbar_ui_controller.cc.patch @@ -1,5 +1,5 @@ diff --git a/chrome/browser/ui/views/download/bubble/download_toolbar_ui_controller.cc b/chrome/browser/ui/views/download/bubble/download_toolbar_ui_controller.cc -index 5360031b8a2b29a58572f1caa5531f168c710335..717ed2c360e8350ee97722ef6589d6bd115e5a3c 100644 +index 5360031b8a2b29a58572f1caa5531f168c710335..5d9e72d1fe4e398435fa46e620dfa25935da340b 100644 --- a/chrome/browser/ui/views/download/bubble/download_toolbar_ui_controller.cc +++ b/chrome/browser/ui/views/download/bubble/download_toolbar_ui_controller.cc @@ -686,6 +686,7 @@ void DownloadToolbarUIController::UpdateIcon() { @@ -10,11 +10,11 @@ index 5360031b8a2b29a58572f1caa5531f168c710335..717ed2c360e8350ee97722ef6589d6bd action_item_->SetImage(ui::ImageModel::FromVectorIcon(*new_icon, icon_color)); // Update the toolbar button's tooltip. -@@ -716,6 +717,7 @@ void DownloadToolbarUIController::UpdateIcon() { - GetImageBadge(browser_view_) - ->UpdateImage(is_active, progress_download_count, progress_color, - badge_background_color); +@@ -735,6 +736,7 @@ void DownloadToolbarUIController::UpdateIcon() { + return; + } + progress_ring->SetDownloading(progress_info_.progress_percentage); + UpdateIcon_BraveImpl(browser_view_, action_item_); + } - // Do not show the progress ring when there is no in progress download. - if (state_ == IconState::kComplete || progress_info_.download_count == 0) { + void DownloadToolbarUIController::OpenPrimaryDialog() { diff --git a/rewrite/chrome/browser/ui/views/download/bubble/download_toolbar_ui_controller.cc.toml b/rewrite/chrome/browser/ui/views/download/bubble/download_toolbar_ui_controller.cc.toml index cd5c4884e7d..3bd06a9bb36 100644 --- a/rewrite/chrome/browser/ui/views/download/bubble/download_toolbar_ui_controller.cc.toml +++ b/rewrite/chrome/browser/ui/views/download/bubble/download_toolbar_ui_controller.cc.toml @@ -4,15 +4,14 @@ # You can obtain one at https://mozilla.org/MPL/2.0/. [[substitution]] -description = '''Override to show insecure icon on insecure download link. +description = '''Override to show insecure icon on insecure download link.' -This replacement adds a `UpdateIcon_BraveImpl` call after the upstream image -update so it runs even when UpdateIcon() returns early afterwards. +This replacement adds a `UpdateIcon_BraveImpl` call at the end of the function. ''' -re_pattern = '(badge_background_color\);)' -replace = '''\1 - UpdateIcon_BraveImpl(browser_view_, action_item_);''' -re_flags = ['MULTILINE'] +re_pattern = '(DownloadToolbarUIController::UpdateIcon\(.*?\n)^\}' +replace = '''\1 UpdateIcon_BraveImpl(browser_view_, action_item_); +}''' +re_flags = ['MULTILINE', 'DOTALL'] [[substitution]] description = 'Override for the icon colour.' diff --git a/test/BUILD.gn b/test/BUILD.gn index 2e9a94b28fc..4eaecc7ff13 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -1398,7 +1398,6 @@ test("brave_browser_tests") { "//brave/browser/ui/page_info", "//brave/browser/ui/tabs:tab_strip", "//brave/browser/ui/toolbar:brave_app_menu_browser_tests", - "//brave/browser/ui/views/download/bubble:browser_tests", "//brave/browser/ui/webui/brave_new_tab_page_refresh:browser_tests", "//brave/browser/ui/webui/side_panel/customize_chrome:browser_tests", "//brave/browser/ui/whats_new:browser_test",