Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use temporal AutocompleteClassifier to get proper selection navigation url #5407

Merged
merged 5 commits into from
May 28, 2020
Merged

Use temporal AutocompleteClassifier to get proper selection navigation url #5407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8c790ae
Use temporal AutocompleteClassifier to get proper navigation url
simonhong Apr 28, 2020
04deb29
Fix failed test cases and build error
simonhong Apr 29, 2020
9908b06
Fix lint error
simonhong Apr 29, 2020
9cb6427
Fix build failure on android
simonhong Apr 29, 2020
49ea4ef
Don't need to get selection navigation url for normal profile again
simonhong Apr 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions browser/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,6 @@ source_set("browser_process") {
sources = [
"autocomplete/brave_autocomplete_provider_client.cc",
"autocomplete/brave_autocomplete_provider_client.h",
"autocomplete/brave_autocomplete_provider_client_for_classifier.cc",
"autocomplete/brave_autocomplete_provider_client_for_classifier.h",
"autocomplete/brave_autocomplete_scheme_classifier.cc",
"autocomplete/brave_autocomplete_scheme_classifier.h",
"brave_shields/ad_block_pref_service_factory.cc",
Expand Down
44 changes: 0 additions & 44 deletions browser/autocomplete/brave_autocomplete_provider_client_browsertest.cc
View file
Open in desktop

This file was deleted.

30 changes: 0 additions & 30 deletions browser/autocomplete/brave_autocomplete_provider_client_for_classifier.cc
View file
Open in desktop

This file was deleted.

44 changes: 0 additions & 44 deletions browser/autocomplete/brave_autocomplete_provider_client_for_classifier.h
View file
Open in desktop

This file was deleted.

11 changes: 4 additions & 7 deletions browser/autocomplete/brave_autocomplete_scheme_classifier.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,18 @@

#include "base/strings/string_util.h"
#include "brave/common/url_constants.h"
#include "brave/components/brave_webtorrent/browser/buildflags/buildflags.h"
#include "chrome/browser/profiles/profile.h"

#if BUILDFLAG(ENABLE_BRAVE_WEBTORRENT)
#include "brave/components/brave_webtorrent/browser/webtorrent_util.h"
#endif

// See the BraveAutocompleteProviderClient why GetOriginalProfile() is fetched.
// All services except TemplateURLService exposed from AutocompleteClassifier
// uses original profile. So, |profile_| should be original profile same as
// base class does.
BraveAutocompleteSchemeClassifier::BraveAutocompleteSchemeClassifier(
Profile* profile)
: ChromeAutocompleteSchemeClassifier(profile->GetOriginalProfile()),
profile_(profile->GetOriginalProfile()) {
: ChromeAutocompleteSchemeClassifier(profile) {
#if BUILDFLAG(ENABLE_BRAVE_WEBTORRENT)
profile_ = profile;
#endif
}

BraveAutocompleteSchemeClassifier::~BraveAutocompleteSchemeClassifier() {
Expand Down
13 changes: 10 additions & 3 deletions browser/autocomplete/brave_autocomplete_scheme_classifier.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
/* Copyright (c) 2019 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 http://mozilla.org/MPL/2.0/. */

#ifndef BRAVE_BROWSER_AUTOCOMPLETE_BRAVE_AUTOCOMPLETE_SCHEME_CLASSIFIER_H_
#define BRAVE_BROWSER_AUTOCOMPLETE_BRAVE_AUTOCOMPLETE_SCHEME_CLASSIFIER_H_

#include <string>

#include "brave/components/brave_webtorrent/browser/buildflags/buildflags.h"
#include "chrome/browser/autocomplete/chrome_autocomplete_scheme_classifier.h"

class BraveAutocompleteSchemeClassifier : public ChromeAutocompleteSchemeClassifier {
class BraveAutocompleteSchemeClassifier
: public ChromeAutocompleteSchemeClassifier {
public:
explicit BraveAutocompleteSchemeClassifier(Profile* profile);
~BraveAutocompleteSchemeClassifier() override;
Expand All @@ -16,7 +21,9 @@ class BraveAutocompleteSchemeClassifier : public ChromeAutocompleteSchemeClassif
const std::string& scheme) const override;

private:
Profile* profile_;
#if BUILDFLAG(ENABLE_BRAVE_WEBTORRENT)
bsclifton marked this conversation as resolved.
Show resolved Hide resolved
Profile* profile_ = nullptr;
#endif

DISALLOW_COPY_AND_ASSIGN(BraveAutocompleteSchemeClassifier);
};
Expand Down
6 changes: 5 additions & 1 deletion browser/search_engines/guest_window_search_engine_provider_service.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
/* Copyright (c) 2019 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 http://mozilla.org/MPL/2.0/. */

Expand All @@ -21,6 +22,9 @@ class GuestWindowSearchEngineProviderService
~GuestWindowSearchEngineProviderService() override;

private:
FRIEND_TEST_ALL_PREFIXES(SearchEngineProviderServiceTest,
GuestWindowControllerTest);

// TemplateURLServiceObserver overrides:
void OnTemplateURLServiceChanged() override;

Expand Down
18 changes: 14 additions & 4 deletions browser/search_engines/search_engine_provider_service_browsertest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
#include "brave/browser/brave_browser_process_impl.h"
#include "brave/browser/profiles/brave_profile_manager.h"
#include "brave/browser/profiles/profile_util.h"
#include "brave/browser/search_engines/guest_window_search_engine_provider_service.h"
#include "brave/browser/search_engines/search_engine_provider_service_factory.h"
#include "brave/browser/search_engines/search_engine_provider_util.h"
#include "brave/browser/tor/buildflags.h"
#include "brave/browser/ui/browser_commands.h"
Expand Down Expand Up @@ -209,7 +211,8 @@ IN_PROC_BROWSER_TEST_F(SearchEngineProviderServiceTest,
if (brave::IsRegionForQwant(guest_profile))
return;

auto* service = TemplateURLServiceFactory::GetForProfile(guest_profile);
auto* template_service =
TemplateURLServiceFactory::GetForProfile(guest_profile);

// alternative pref is initially disabled.
EXPECT_FALSE(brave::UseAlternativeSearchEngineProviderEnabled(guest_profile));
Expand All @@ -220,14 +223,20 @@ IN_PROC_BROWSER_TEST_F(SearchEngineProviderServiceTest,
TemplateURLPrepopulateData::PREPOPULATED_ENGINE_ID_DUCKDUCKGO;

// Check guest profile's search provider is set to ddg.
EXPECT_EQ(service->GetDefaultSearchProvider()->data().prepopulate_id,
EXPECT_EQ(template_service->GetDefaultSearchProvider()->data().prepopulate_id,
provider_id);

auto bing_data = TemplateURLPrepopulateData::GetPrepopulatedEngine(
guest_profile->GetPrefs(),
TemplateURLPrepopulateData::PREPOPULATED_ENGINE_ID_BING);
TemplateURL bing_url(*bing_data);
service->SetUserSelectedDefaultSearchProvider(&bing_url);
template_service->SetUserSelectedDefaultSearchProvider(&bing_url);

auto* search_engine_provider_service =
static_cast<GuestWindowSearchEngineProviderService*>(
SearchEngineProviderServiceFactory::GetForProfile(guest_profile));
search_engine_provider_service->OnTemplateURLServiceChanged();
bsclifton marked this conversation as resolved.
Show resolved Hide resolved

// Check alternative pref is turned off.
EXPECT_FALSE(brave::UseAlternativeSearchEngineProviderEnabled(guest_profile));

Expand All @@ -236,6 +245,7 @@ IN_PROC_BROWSER_TEST_F(SearchEngineProviderServiceTest,
TemplateURLPrepopulateData::PREPOPULATED_ENGINE_ID_DUCKDUCKGO);
TemplateURL ddg_url(*ddg_data);

service->SetUserSelectedDefaultSearchProvider(&ddg_url);
template_service->SetUserSelectedDefaultSearchProvider(&ddg_url);
search_engine_provider_service->OnTemplateURLServiceChanged();
bsclifton marked this conversation as resolved.
Show resolved Hide resolved
EXPECT_TRUE(brave::UseAlternativeSearchEngineProviderEnabled(guest_profile));
}
7 changes: 7 additions & 0 deletions browser/search_engines/search_engine_provider_service_factory.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,13 @@ KeyedService* InitializeSearchEngineProviderServiceIfNeeded(Profile* profile) {

} // namespace

// static
SearchEngineProviderService* SearchEngineProviderServiceFactory::GetForProfile(
Profile* profile) {
return static_cast<SearchEngineProviderService*>(
GetInstance()->GetServiceForBrowserContext(profile, true));
}

// static
SearchEngineProviderServiceFactory*
SearchEngineProviderServiceFactory::GetInstance() {
Expand Down
17 changes: 13 additions & 4 deletions browser/search_engines/search_engine_provider_service_factory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
/* Copyright (c) 2019 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 http://mozilla.org/MPL/2.0/. */

#ifndef BRAVE_BROWSER_SEARCH_ENGINE_SEARCH_ENGINE_PROVIDER_SERVICE_FACTORY_H_
#define BRAVE_BROWSER_SEARCH_ENGINE_SEARCH_ENGINE_PROVIDER_SERVICE_FACTORY_H_
#ifndef BRAVE_BROWSER_SEARCH_ENGINES_SEARCH_ENGINE_PROVIDER_SERVICE_FACTORY_H_
#define BRAVE_BROWSER_SEARCH_ENGINES_SEARCH_ENGINE_PROVIDER_SERVICE_FACTORY_H_

#include "base/memory/singleton.h"
#include "components/keyed_service/content/browser_context_keyed_service_factory.h"

class Profile;
class SearchEngineProviderService;

// The purpose of this factory is to configure proper search engine provider to
// private/guest/tor profile before it is referenced.
// Also, this factory doesn't have public api. Instead, service is instantiated
Expand All @@ -18,6 +22,11 @@ class SearchEngineProviderServiceFactory
static SearchEngineProviderServiceFactory* GetInstance();

private:
FRIEND_TEST_ALL_PREFIXES(SearchEngineProviderServiceTest,
GuestWindowControllerTest);
// Only for test.
static SearchEngineProviderService* GetForProfile(Profile* profile);

friend
struct base::DefaultSingletonTraits<SearchEngineProviderServiceFactory>;
SearchEngineProviderServiceFactory();
Expand All @@ -36,4 +45,4 @@ class SearchEngineProviderServiceFactory
DISALLOW_COPY_AND_ASSIGN(SearchEngineProviderServiceFactory);
};

#endif // BRAVE_BROWSER_SEARCH_ENGINE_SEARCH_ENGINE_PROVIDER_SERVICE_FACTORY_H_
#endif // BRAVE_BROWSER_SEARCH_ENGINES_SEARCH_ENGINE_PROVIDER_SERVICE_FACTORY_H_
4 changes: 2 additions & 2 deletions chromium_src/chrome/browser/autocomplete/autocomplete_classifier_factory.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#include "brave/browser/autocomplete/brave_autocomplete_provider_client_for_classifier.h"
#include "brave/browser/autocomplete/brave_autocomplete_provider_client.h"
#include "brave/browser/autocomplete/brave_autocomplete_scheme_classifier.h"
#include "components/omnibox/browser/autocomplete_classifier.h"
#include "components/omnibox/browser/autocomplete_controller.h"

#define ChromeAutocompleteProviderClient BraveAutocompleteProviderClientForClassifier // NOLINT
#define ChromeAutocompleteProviderClient BraveAutocompleteProviderClient
#define ChromeAutocompleteSchemeClassifier BraveAutocompleteSchemeClassifier
#include "../../../../../chrome/browser/autocomplete/autocomplete_classifier_factory.cc"
#undef ChromeAutocompleteSchemeClassifier
Expand Down
30 changes: 30 additions & 0 deletions chromium_src/chrome/browser/renderer_context_menu/render_view_context_menu.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,14 @@

#include "chrome/browser/renderer_context_menu/render_view_context_menu.h"

#include "brave/browser/autocomplete/brave_autocomplete_provider_client.h"
#include "brave/browser/autocomplete/brave_autocomplete_scheme_classifier.h"
#include "brave/browser/profiles/profile_util.h"
#include "brave/browser/tor/buildflags.h"
#include "brave/browser/translate/buildflags/buildflags.h"
#include "brave/browser/renderer_context_menu/brave_spelling_options_submenu_observer.h"
#include "components/omnibox/browser/autocomplete_classifier.h"
#include "components/omnibox/browser/autocomplete_controller.h"

#if BUILDFLAG(ENABLE_TOR)
#include "brave/browser/tor/tor_profile_service.h"
Expand All @@ -18,6 +22,31 @@
#undef RenderViewContextMenu
#define RenderViewContextMenu RenderViewContextMenu_Chromium

namespace {

GURL GetSelectionNavigationURL(Profile* profile, const base::string16& text) {
AutocompleteMatch match;
AutocompleteClassifier classifier(
std::make_unique<AutocompleteController>(
std::make_unique<BraveAutocompleteProviderClient>(profile),
AutocompleteClassifier::DefaultOmniboxProviders()),
std::make_unique<BraveAutocompleteSchemeClassifier>(profile));
classifier.Classify(text, false, false,
metrics::OmniboxEventProto::INVALID_SPEC, &match, NULL);
classifier.Shutdown();
return match.destination_url;
}

} // namespace

#define BRAVE_APPEND_SEARCH_PROVIDER \
if (GetProfile()->IsOffTheRecord()) { \
selection_navigation_url_ = \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused here, the selection_navigation_url_ is set from AutocompleteClassifierFactory::GetForProfile(GetProfile())->Classify( so why aren't we just setting the correct url from the AutocompleteClassifier?

Copy link
Member Author

@simonhong simonhong May 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AutocompleteClassifierFactory always uses normal profile.
So we did https://github.com/brave/brave-core/pull/5407/files#diff-ffcd3bd1d7f1c7ee80c4637a3f919142L10. This PR reverts it and instead set correct url by patching here.
As you found previously, that service used normal & incognito profile mixed for incognito profile.
https://github.com/brave/brave-core/pull/5407/files#diff-2325211a427ac6f007ad338411e100aaL14

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bridiver PTAL again.

GetSelectionNavigationURL(GetProfile(), params_.selection_text); \
if (!selection_navigation_url_.is_valid()) \
return; \
}

// Use our subclass to initialize SpellingOptionsSubMenuObserver.
#define SpellingOptionsSubMenuObserver BraveSpellingOptionsSubMenuObserver

Expand All @@ -27,6 +56,7 @@

// Make it clear which class we mean here.
#undef RenderViewContextMenu
#undef BRAVE_APPEND_SEARCH_PROVIDER

BraveRenderViewContextMenu::BraveRenderViewContextMenu(
content::RenderFrameHost* render_frame_host,
Expand Down
17 changes: 0 additions & 17 deletions patches/chrome-browser-autocomplete-autocomplete_classifier_factory.cc.patch
View file
Open in desktop

This file was deleted.

Loading