Skip to content

Commit

Permalink
[launcher-lacros] Construct open tab results from crosapi mojom.
Browse files Browse the repository at this point in the history
This CL refactors OpenTabResults to be constructed from our SearchResult
crosapi mojom. Utimately, this will allow open tab results to be
instantiated in lacros world. However, our non-lacros code must now make
the detour:
  AutocompleteMatch -> crosapi::SearchResult -> OpenTabResult.
This is only temporary, as non-lacros code is eventually going away.

Design doc at go/launcher-lacros-refactor.

Bug: 1228587
Change-Id: If52d4b5918361468ee06883b5bdc1200a0fe42e6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3775354
Reviewed-by: Rachel Wong <wrong@chromium.org>
Commit-Queue: Michael Martis <martis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1028556}
  • Loading branch information
martis-chromium authored and Chromium LUCI CQ committed Jul 27, 2022
1 parent 3f67948 commit 99d65cf
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 52 deletions.
10 changes: 5 additions & 5 deletions chrome/browser/ui/app_list/search/omnibox_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -196,9 +196,6 @@ void OmniboxProvider::PopulateFromACResult(const AutocompleteResult& result) {

if (!is_zero_state_input_ && IsAnswer(match) &&
!ShouldFilterAnswer(match, last_query_)) {
// TODO(1228587): update the rest of the result types to accept a crosapi
// SearchResultPtr so that they can also be used from
// Lacros.
new_results.emplace_back(std::make_unique<OmniboxAnswerResult>(
profile_, list_controller_,
crosapi::CreateAnswerResult(match, controller_.get(), last_query_,
Expand All @@ -207,8 +204,11 @@ void OmniboxProvider::PopulateFromACResult(const AutocompleteResult& result) {
} else if (match.type == AutocompleteMatchType::OPEN_TAB) {
DCHECK(last_tokenized_query_.has_value());
new_results.emplace_back(std::make_unique<OpenTabResult>(
profile_, list_controller_, &favicon_cache_,
last_tokenized_query_.value(), match));
profile_, list_controller_,
crosapi::CreateResult(
match, controller_.get(), &favicon_cache_,
BookmarkModelFactory::GetForBrowserContext(profile_), input_),
last_tokenized_query_.value()));
} else {
// We can use an unretained pointer here since we own both the
// autocomplete controller (which lives for the entirety of our lifetime)
Expand Down
64 changes: 31 additions & 33 deletions chrome/browser/ui/app_list/search/open_tab_result.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "base/strings/strcat.h"
#include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h"
#include "chrome/browser/chromeos/launcher_search/search_util.h"
#include "chrome/browser/ui/app_list/app_list_controller_delegate.h"
#include "chrome/browser/ui/app_list/search/common/icon_constants.h"
#include "chrome/browser/ui/app_list/search/common/search_result_util.h"
Expand All @@ -18,9 +19,7 @@
#include "chrome/browser/ui/app_list/search/search_tags_util.h"
#include "chromeos/ash/components/string_matching/tokenized_string.h"
#include "chromeos/ash/components/string_matching/tokenized_string_match.h"
#include "components/omnibox/browser/autocomplete_match.h"
#include "components/omnibox/browser/autocomplete_match_type.h"
#include "components/omnibox/browser/favicon_cache.h"
#include "chromeos/crosapi/mojom/launcher_search.mojom.h"
#include "components/omnibox/browser/vector_icons.h"
#include "components/strings/grit/components_strings.h"
#include "ui/base/l10n/l10n_util.h"
Expand All @@ -34,6 +33,7 @@ namespace {

using ::ash::string_matching::TokenizedString;
using ::ash::string_matching::TokenizedStringMatch;
using CrosApiSearchResult = ::crosapi::mojom::SearchResult;

constexpr char kOpenTabScheme[] = "opentab://";

Expand All @@ -45,29 +45,29 @@ constexpr char16_t kA11yDelimiter[] = u", ";

OpenTabResult::OpenTabResult(Profile* profile,
AppListControllerDelegate* list_controller,
FaviconCache* favicon_cache,
const TokenizedString& query,
const AutocompleteMatch& match)
: profile_(profile),
crosapi::mojom::SearchResultPtr search_result,
const TokenizedString& query)
: consumer_receiver_(this, std::move(search_result->receiver)),
profile_(profile),
list_controller_(list_controller),
favicon_cache_(favicon_cache),
match_(match),
drive_id_(GetDriveId(match.destination_url)) {
DCHECK(match_.destination_url.is_valid());
search_result_(std::move(search_result)),
drive_id_(GetDriveId(*search_result_->destination_url)),
description_(search_result_->description.value_or(u"")) {
DCHECK(search_result_->destination_url->is_valid());

// TODO(crbug.com/1293702): This may not be unique. Once we have a mechanism
// for opening a specific tab, add that info too to ensure uniqueness.
set_id(kOpenTabScheme + match.destination_url.spec());
set_id(kOpenTabScheme + search_result_->destination_url->spec());

SetDisplayType(DisplayType::kList);
SetResultType(ResultType::kOpenTab);
SetMetricsType(ash::OPEN_TAB);
SetCategory(Category::kWeb);

// Ignore `match_.relevance` and manually calculate a relevance score for this
// result.
// Ignore `search_result_->relevance` and manually calculate a relevance
// score for this result.
TokenizedStringMatch string_match;
TokenizedString title(match_.description);
TokenizedString title(description_);
set_relevance(string_match.Calculate(query, title));

UpdateText();
Expand All @@ -83,7 +83,9 @@ OpenTabResult::~OpenTabResult() {

void OpenTabResult::Open(int event_flags) {
list_controller_->OpenURL(
profile_, match_.destination_url, match_.transition,
profile_, *search_result_->destination_url,
crosapi::PageTransitionToUiPageTransition(
search_result_->page_transition),
ui::DispositionFromEventFlags(event_flags,
WindowOpenDisposition::SWITCH_TO_TAB));
}
Expand All @@ -98,11 +100,11 @@ void OpenTabResult::OnColorModeChanged(bool dark_mode_enabled) {
}

void OpenTabResult::UpdateText() {
// URL results from the Omnibox have the page title stored in
// `match.description`.
SetTitle(match_.description);
// URL results from the Omnibox have the page title stored in the description.
SetTitle(description_);

std::u16string url = base::UTF8ToUTF16(match_.destination_url.spec());
const std::u16string url =
base::UTF8ToUTF16(search_result_->destination_url->spec());
SetDetailsTextVector(
{CreateStringTextItem(url).SetTextTags({Tag(Tag::URL, 0, url.length())}),
CreateStringTextItem(l10n_util::GetStringFUTF16(
Expand All @@ -111,21 +113,17 @@ void OpenTabResult::UpdateText() {
ash::SearchResultTextItem::OverflowBehavior::kNoElide)});

SetAccessibleName(base::JoinString(
{match_.description, url,
{description_, url,
l10n_util::GetStringFUTF16(IDS_APP_LIST_OPEN_TAB_HINT, u"")},
kA11yDelimiter));
}

void OpenTabResult::UpdateIcon() {
// Use the favicon if it is in cache.
if (favicon_cache_) {
const auto icon = favicon_cache_->GetFaviconForPageUrl(
match_.destination_url, base::BindOnce(&OpenTabResult::OnFaviconFetched,
weak_factory_.GetWeakPtr()));
if (!icon.IsEmpty()) {
SetIcon(IconInfo(icon.AsImageSkia(), kFaviconDimension));
return;
}
// Use the favicon if it was in the cache.
if (search_result_->omnibox_type ==
CrosApiSearchResult::OmniboxType::kFavicon) {
SetIcon(IconInfo(search_result_->cached_favicon, kFaviconDimension));
return;
}

// Otherwise, fall back to using a generic icon.
Expand All @@ -141,10 +139,10 @@ void OpenTabResult::SetGenericIcon() {
kSystemIconDimension));
}

void OpenTabResult::OnFaviconFetched(const gfx::Image& icon) {
void OpenTabResult::OnFaviconReceived(const gfx::ImageSkia& icon) {
// By contract, this is never called with an empty `icon`.
DCHECK(!icon.IsEmpty());
SetIcon(IconInfo(icon.AsImageSkia(), kFaviconDimension));
DCHECK(!icon.isNull());
SetIcon(IconInfo(icon, kFaviconDimension));
}

} // namespace app_list
32 changes: 20 additions & 12 deletions chrome/browser/ui/app_list/search/open_tab_result.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,16 @@
#ifndef CHROME_BROWSER_UI_APP_LIST_SEARCH_OPEN_TAB_RESULT_H_
#define CHROME_BROWSER_UI_APP_LIST_SEARCH_OPEN_TAB_RESULT_H_

#include <string>

#include "ash/public/cpp/style/color_mode_observer.h"
#include "base/memory/weak_ptr.h"
#include "chrome/browser/ui/app_list/search/chrome_search_result.h"
#include "components/omnibox/browser/autocomplete_match.h"
#include "chromeos/crosapi/mojom/launcher_search.mojom.h"
#include "mojo/public/cpp/bindings/receiver.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

class AppListControllerDelegate;
class FaviconCache;
class Profile;

namespace ash::string_matching {
Expand All @@ -22,13 +24,14 @@ class TokenizedString;
namespace app_list {

// Open tab search results. This is produced by the OmniboxProvider.
class OpenTabResult : public ChromeSearchResult, public ash::ColorModeObserver {
class OpenTabResult : public ChromeSearchResult,
public ash::ColorModeObserver,
public crosapi::mojom::SearchResultConsumer {
public:
OpenTabResult(Profile* profile,
AppListControllerDelegate* list_controller,
FaviconCache* favicon_cache,
const ash::string_matching::TokenizedString& query,
const AutocompleteMatch& match);
crosapi::mojom::SearchResultPtr search_result,
const ash::string_matching::TokenizedString& query);
~OpenTabResult() override;

OpenTabResult(const OpenTabResult&) = delete;
Expand All @@ -46,13 +49,18 @@ class OpenTabResult : public ChromeSearchResult, public ash::ColorModeObserver {
void UpdateIcon();
// Creates a generic backup icon: used when rich icons are not available.
void SetGenericIcon();
void OnFaviconFetched(const gfx::Image& icon);

Profile* profile_;
AppListControllerDelegate* list_controller_;
FaviconCache* favicon_cache_;
AutocompleteMatch match_;
absl::optional<std::string> drive_id_;
// crosapi::mojom::SearchResultConsumer:
void OnFaviconReceived(const gfx::ImageSkia& icon) override;

// Handle used to receive a fetched favicon over mojo.
const mojo::Receiver<crosapi::mojom::SearchResultConsumer> consumer_receiver_;

Profile* const profile_;
AppListControllerDelegate* const list_controller_;
const crosapi::mojom::SearchResultPtr search_result_;
const absl::optional<std::string> drive_id_;
const std::u16string description_;
// Whether this open tab result uses a generic backup icon.
bool uses_generic_icon_ = false;

Expand Down
10 changes: 8 additions & 2 deletions chrome/browser/ui/app_list/search/open_tab_result_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@
#include "ash/strings/grit/ash_strings.h"
#include "base/strings/strcat.h"
#include "base/strings/utf_string_conversions.h"
#include "chrome/browser/chromeos/launcher_search/search_util.h"
#include "chrome/browser/ui/app_list/search/common/search_result_util.h"
#include "chromeos/ash/components/string_matching/tokenized_string.h"
#include "components/omnibox/browser/autocomplete_input.h"
#include "components/omnibox/browser/autocomplete_match.h"
#include "components/omnibox/browser/autocomplete_match_type.h"
#include "testing/gmock/include/gmock/gmock.h"
Expand Down Expand Up @@ -37,8 +39,12 @@ class OpenTabResultTest : public testing::Test {
match.destination_url = GURL(url);
match.relevance = 1000;
TokenizedString tokenized_query(query, TokenizedString::Mode::kCamelCase);
return std::make_unique<OpenTabResult>(nullptr, nullptr, nullptr,
tokenized_query, match);
return std::make_unique<OpenTabResult>(
/*profile=*/nullptr, /*list_controller=*/nullptr,
crosapi::CreateResult(match, /*controller=*/nullptr,
/*favicon_cache=*/nullptr,
/*bookmark_model=*/nullptr, AutocompleteInput()),
tokenized_query);
}
};

Expand Down

0 comments on commit 99d65cf

Please sign in to comment.