Skip to content

Commit

Permalink
[M115 merge][realbox] Fix NOTREACHED crashes due to icon mappings.
Browse files Browse the repository at this point in the history
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 5eb06b5)

Bug: 1448353, 1445439
Change-Id: I153ff932dc88245e8c67a1daf4641b60ccfec596
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4563838
Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
Commit-Queue: Prudhvikumar Bommana <pbommana@google.com>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Cr-Original-Commit-Position: refs/heads/main@{#1149682}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4574655
Auto-Submit: Khalid Peer <khalidpeer@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Khalid Peer <khalidpeer@chromium.org>
Cr-Commit-Position: refs/branch-heads/5790@{#139}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
  • Loading branch information
Khalid Peer authored and Chromium LUCI CQ committed May 30, 2023
1 parent 403eaca commit 8b3aac4
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 29 deletions.
62 changes: 44 additions & 18 deletions chrome/browser/ui/webui/realbox/realbox_handler.cc
Expand Up @@ -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";
Expand Down Expand Up @@ -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;
Expand All @@ -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.
Expand Down Expand Up @@ -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
Expand Down
33 changes: 23 additions & 10 deletions chrome/browser/ui/webui/realbox/realbox_handler_browsertest.cc
Expand Up @@ -6,6 +6,7 @@

#include <string>
#include <unordered_map>
#include <utility>

#include <gtest/gtest.h>
#include "base/check.h"
Expand All @@ -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<bool> {
class BrowserTestWithParam
: public InProcessBrowserTest,
public testing::WithParamInterface<std::pair<bool, bool>> {
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;
Expand All @@ -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<bool, bool>(true, true),
std::pair<bool, bool>(true, false),
std::pair<bool, bool>(false, true),
std::pair<bool, bool>(false, false)));

// Tests that all Omnibox match vector icons map to an equivalent SVG for use in
// the NTP Realbox.
Expand All @@ -59,7 +74,7 @@ IN_PROC_BROWSER_TEST_P(BrowserTestWithParam, MatchVectorIcons) {
type != AutocompleteMatchType::NUM_TYPES; type++) {
AutocompleteMatch match;
match.type = static_cast<AutocompleteMatchType::Type>(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);
Expand Down Expand Up @@ -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);
Expand All @@ -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<OmniboxPedalId, scoped_refptr<OmniboxPedal>> pedals =
GetPedalImplementations(/*incognito=*/true, /*guest=*/false,
/*testing=*/true);
Expand Down
2 changes: 1 addition & 1 deletion components/omnibox/common/omnibox_features.cc
Expand Up @@ -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,
Expand Down

0 comments on commit 8b3aac4

Please sign in to comment.