From 8b3aac42deaa8b92ed5563faa9475f7aa2842956 Mon Sep 17 00:00:00 2001 From: Khalid Peer Date: Tue, 30 May 2023 18:23:25 +0000 Subject: [PATCH] [M115 merge][realbox] Fix NOTREACHED crashes due to icon mappings. In the course of updating suggestion vector icons in crrev/c/4535099 as part of ChromeRefresh2023, the AutocompleteMatchVectorIconToResourceName function was not updated with the latest vector-icon-to-SVG mappings. As a result, a couple of DCHECK-enabled clients on Canary have been crashing with NOTREACHED errors. Due to a similar reason, the PedalVectorIconToResourceName function has also been triggering crashes for DCHECK-enabled clients on Canary. In order to remediate these crashes, this CL updates the vector-icon-to-SVG mappings to also work for the new ChromeRefresh2023 vector icons. (cherry picked from commit 5eb06b54fb552c53b48b08b9d34dff85134792b3) Bug: 1448353, 1445439 Change-Id: I153ff932dc88245e8c67a1daf4641b60ccfec596 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4563838 Reviewed-by: Moe Ahmadi Commit-Queue: Prudhvikumar Bommana Code-Coverage: Findit Cr-Original-Commit-Position: refs/heads/main@{#1149682} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4574655 Auto-Submit: Khalid Peer Bot-Commit: Rubber Stamper Commit-Queue: Khalid Peer Cr-Commit-Position: refs/branch-heads/5790@{#139} Cr-Branched-From: 1d71a337b1f6e707a13ae074dca1e2c34905eb9f-refs/heads/main@{#1148114} --- .../ui/webui/realbox/realbox_handler.cc | 62 +++++++++++++------ .../realbox/realbox_handler_browsertest.cc | 33 +++++++--- components/omnibox/common/omnibox_features.cc | 2 +- 3 files changed, 68 insertions(+), 29 deletions(-) diff --git a/chrome/browser/ui/webui/realbox/realbox_handler.cc b/chrome/browser/ui/webui/realbox/realbox_handler.cc index 636d09a06c891..e3d93edcf9f05 100644 --- a/chrome/browser/ui/webui/realbox/realbox_handler.cc +++ b/chrome/browser/ui/webui/realbox/realbox_handler.cc @@ -141,6 +141,9 @@ constexpr char kMacShareIconResourceName[] = #elif BUILDFLAG(IS_WIN) constexpr char kWinShareIconResourceName[] = "//resources/cr_components/omnibox/icons/win_share.svg"; +#elif BUILDFLAG(IS_LINUX) +constexpr char kLinuxShareIconResourceName[] = + "//resources/cr_components/omnibox/icons/share.svg"; #else constexpr char kShareIconResourceName[] = "//resources/cr_components/omnibox/icons/share.svg"; @@ -550,25 +553,34 @@ void RealboxHandler::SetupDropdownWebUIDataSource( // static std::string RealboxHandler::AutocompleteMatchVectorIconToResourceName( const gfx::VectorIcon& icon) { - if (icon.name == omnibox::kAnswerCurrencyIcon.name) { + if (icon.name == omnibox::kAnswerCurrencyIcon.name || + icon.name == omnibox::kAnswerCurrencyChromeRefreshIcon.name) { return kAnswerCurrencyIconResourceName; } else if (icon.name == omnibox::kAnswerDefaultIcon.name) { return kAnswerDefaultIconResourceName; - } else if (icon.name == omnibox::kAnswerDictionaryIcon.name) { + } else if (icon.name == omnibox::kAnswerDictionaryIcon.name || + icon.name == omnibox::kAnswerDictionaryChromeRefreshIcon.name) { return kAnswerDictionaryIconResourceName; - } else if (icon.name == omnibox::kAnswerFinanceIcon.name) { + } else if (icon.name == omnibox::kAnswerFinanceIcon.name || + icon.name == omnibox::kAnswerFinanceChromeRefreshIcon.name) { return kAnswerFinanceIconResourceName; - } else if (icon.name == omnibox::kAnswerSunriseIcon.name) { + } else if (icon.name == omnibox::kAnswerSunriseIcon.name || + icon.name == omnibox::kAnswerSunriseChromeRefreshIcon.name) { return kAnswerSunriseIconResourceName; - } else if (icon.name == omnibox::kAnswerTranslationIcon.name) { + } else if (icon.name == omnibox::kAnswerTranslationIcon.name || + icon.name == omnibox::kAnswerTranslationChromeRefreshIcon.name) { return kAnswerTranslationIconResourceName; - } else if (icon.name == omnibox::kAnswerWhenIsIcon.name) { + } else if (icon.name == omnibox::kAnswerWhenIsIcon.name || + icon.name == omnibox::kAnswerWhenIsChromeRefreshIcon.name) { return kAnswerWhenIsIconResourceName; - } else if (icon.name == omnibox::kBookmarkIcon.name) { + } else if (icon.name == omnibox::kBookmarkIcon.name || + icon.name == omnibox::kBookmarkChromeRefreshIcon.name) { return kBookmarkIconResourceName; - } else if (icon.name == omnibox::kCalculatorIcon.name) { + } else if (icon.name == omnibox::kCalculatorIcon.name || + icon.name == omnibox::kCalculatorChromeRefreshIcon.name) { return kCalculatorIconResourceName; - } else if (icon.name == omnibox::kClockIcon.name) { + } else if (icon.name == omnibox::kClockIcon.name || + icon.name == omnibox::kClockChromeRefreshIcon.name) { return kClockIconResourceName; } else if (icon.name == omnibox::kDriveDocsIcon.name) { return kDriveDocsIconResourceName; @@ -590,17 +602,22 @@ std::string RealboxHandler::AutocompleteMatchVectorIconToResourceName( return kDriveVideoIconResourceName; } else if (icon.name == omnibox::kExtensionAppIcon.name) { return kExtensionAppIconResourceName; - } else if (icon.name == omnibox::kJourneysIcon.name) { + } else if (icon.name == omnibox::kJourneysIcon.name || + icon.name == omnibox::kJourneysChromeRefreshIcon.name) { return kJourneysIconResourceName; - } else if (icon.name == omnibox::kPageIcon.name) { + } else if (icon.name == omnibox::kPageIcon.name || + icon.name == omnibox::kPageChromeRefreshIcon.name) { return kPageIconResourceName; } else if (icon.name == omnibox::kPedalIcon.name) { return kPedalsIconResourceName; - } else if (icon.name == omnibox::kProductIcon.name) { + } else if (icon.name == omnibox::kProductIcon.name || + icon.name == omnibox::kProductChromeRefreshIcon.name) { return kChromeProductIconResourceName; - } else if (icon.name == vector_icons::kSearchIcon.name) { + } else if (icon.name == vector_icons::kSearchIcon.name || + icon.name == vector_icons::kSearchChromeRefreshIcon.name) { return kSearchIconResourceName; - } else if (icon.name == omnibox::kTrendingUpIcon.name) { + } else if (icon.name == omnibox::kTrendingUpIcon.name || + icon.name == omnibox::kTrendingUpChromeRefreshIcon.name) { return kTrendingUpIconResourceName; } else if (icon.is_empty()) { return ""; // An empty resource name is effectively a blank icon. @@ -652,22 +669,31 @@ std::string RealboxHandler::PedalVectorIconToResourceName( if (icon.name == omnibox::kIncognitoIcon.name) { return kIncognitoIconResourceName; } - if (icon.name == omnibox::kJourneysIcon.name) { + if (icon.name == omnibox::kJourneysIcon.name || + icon.name == omnibox::kJourneysChromeRefreshIcon.name) { return kJourneysIconResourceName; } if (icon.name == omnibox::kPedalIcon.name) { return kPedalsIconResourceName; } #if BUILDFLAG(IS_MAC) - if (icon.name == omnibox::kShareMacIcon.name) { + if (icon.name == omnibox::kShareMacIcon.name || + icon.name == omnibox::kShareMacChromeRefreshIcon.name) { return kMacShareIconResourceName; } #elif BUILDFLAG(IS_WIN) - if (icon.name == omnibox::kShareWinIcon.name) { + if (icon.name == omnibox::kShareWinIcon.name || + icon.name == omnibox::kShareWinChromeRefreshIcon.name) { return kWinShareIconResourceName; } +#elif BUILDFLAG(IS_LINUX) + if (icon.name == omnibox::kShareIcon.name || + icon.name == omnibox::kShareLinuxChromeRefreshIcon.name) { + return kLinuxShareIconResourceName; + } #else - if (icon.name == omnibox::kShareIcon.name) { + if (icon.name == omnibox::kShareIcon.name || + icon.name == omnibox::kShareChromeRefreshIcon.name) { return kShareIconResourceName; } #endif diff --git a/chrome/browser/ui/webui/realbox/realbox_handler_browsertest.cc b/chrome/browser/ui/webui/realbox/realbox_handler_browsertest.cc index 0af7a0dff9d07..f3e01ed081207 100644 --- a/chrome/browser/ui/webui/realbox/realbox_handler_browsertest.cc +++ b/chrome/browser/ui/webui/realbox/realbox_handler_browsertest.cc @@ -6,6 +6,7 @@ #include #include +#include #include #include "base/check.h" @@ -30,14 +31,23 @@ #include "content/public/test/browser_test.h" #include "content/public/test/prerender_test_util.h" #include "mojo/public/cpp/bindings/remote.h" +#include "ui/base/ui_base_features.h" #include "ui/gfx/vector_icon_types.h" namespace { -class BrowserTestWithParam : public InProcessBrowserTest, - public testing::WithParamInterface { +class BrowserTestWithParam + : public InProcessBrowserTest, + public testing::WithParamInterface> { public: - BrowserTestWithParam() = default; + BrowserTestWithParam() { + const bool is_cr23_enabled = GetParam().second; + if (is_cr23_enabled) { + scoped_feature_list_.InitAndEnableFeature(features::kChromeRefresh2023); + } else { + scoped_feature_list_.InitAndDisableFeature(features::kChromeRefresh2023); + } + } BrowserTestWithParam(const BrowserTestWithParam&) = delete; BrowserTestWithParam& operator=(const BrowserTestWithParam&) = delete; ~BrowserTestWithParam() override = default; @@ -48,9 +58,14 @@ class BrowserTestWithParam : public InProcessBrowserTest, } // namespace -INSTANTIATE_TEST_SUITE_P(RealboxHandlerMatchIconTest, +// Each value listed below represents the following: +// {is_bookmark, is_cr23_enabled} +INSTANTIATE_TEST_SUITE_P(RealboxHandlerIconTest, BrowserTestWithParam, - testing::Bool()); + testing::Values(std::pair(true, true), + std::pair(true, false), + std::pair(false, true), + std::pair(false, false))); // Tests that all Omnibox match vector icons map to an equivalent SVG for use in // the NTP Realbox. @@ -59,7 +74,7 @@ IN_PROC_BROWSER_TEST_P(BrowserTestWithParam, MatchVectorIcons) { type != AutocompleteMatchType::NUM_TYPES; type++) { AutocompleteMatch match; match.type = static_cast(type); - const bool is_bookmark = BrowserTestWithParam::GetParam(); + const bool is_bookmark = BrowserTestWithParam::GetParam().first; const gfx::VectorIcon& vector_icon = match.GetVectorIcon(is_bookmark); const std::string& svg_name = RealboxHandler::AutocompleteMatchVectorIconToResourceName(vector_icon); @@ -87,7 +102,7 @@ IN_PROC_BROWSER_TEST_P(BrowserTestWithParam, AnswerVectorIcons) { SuggestionAnswer answer; answer.set_type(answer_type); match.answer = answer; - const bool is_bookmark = BrowserTestWithParam::GetParam(); + const bool is_bookmark = BrowserTestWithParam::GetParam().first; const gfx::VectorIcon& vector_icon = match.GetVectorIcon(is_bookmark); const std::string& svg_name = RealboxHandler::AutocompleteMatchVectorIconToResourceName(vector_icon); @@ -100,11 +115,9 @@ IN_PROC_BROWSER_TEST_P(BrowserTestWithParam, AnswerVectorIcons) { } } -using RealboxHandlerPedalIconTest = InProcessBrowserTest; - // Tests that all Omnibox Pedal vector icons map to an equivalent SVG for use in // the NTP Realbox. -IN_PROC_BROWSER_TEST_F(RealboxHandlerPedalIconTest, PedalVectorIcons) { +IN_PROC_BROWSER_TEST_P(BrowserTestWithParam, PedalVectorIcons) { std::unordered_map> pedals = GetPedalImplementations(/*incognito=*/true, /*guest=*/false, /*testing=*/true); diff --git a/components/omnibox/common/omnibox_features.cc b/components/omnibox/common/omnibox_features.cc index a52313ee2ea02..613289d9c260e 100644 --- a/components/omnibox/common/omnibox_features.cc +++ b/components/omnibox/common/omnibox_features.cc @@ -334,7 +334,7 @@ BASE_FEATURE(kRichAutocompletion, // Feature used to enable Pedals in the NTP Realbox. BASE_FEATURE(kNtpRealboxPedals, "NtpRealboxPedals", - base::FEATURE_DISABLED_BY_DEFAULT); + base::FEATURE_ENABLED_BY_DEFAULT); // Feature used to enable URL suggestions for inputs that may contain typos. BASE_FEATURE(kOmniboxFuzzyUrlSuggestions,