Skip to content

Commit

Permalink
[Omnibox][PZPS] pref to control visibility of grouped suggestions
Browse files Browse the repository at this point in the history
Certain types of suggestions such as the proactive zero-prefix suggestions
may be associated with suggestion group IDs that indicate their type, e.g.,
trending or onboarding PZPS.

Since proactive zero-prefix suggestions cannot be individually deleted,
users should be able to hide/show all suggestions belonging to a given
group ID. This CL introduces a list pref that controls the visibility of
suggestion group IDs. Convenience methods are added to the omnibox::
namespace to check whether a given suggestion group ID is hidden and to
toggle visibility of a given suggestion group ID. This CL also renames the
files omnibox_pref_names.h/cc to omnibox_prefs.h/cc as they contain more
than just the pref names.

Bug: 1046547
Change-Id: Iea9a03eb70b780c583e3cf8f48d819a9aa9c1f20
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2137866
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Commit-Queue: Moe Ahmadi <mahmadi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#758223}
  • Loading branch information
Moe Ahmadi authored and Commit Bot committed Apr 10, 2020
1 parent 996ba3e commit 1266de8
Show file tree
Hide file tree
Showing 25 changed files with 168 additions and 64 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@
#include "components/nacl/browser/nacl_browser.h"
#include "components/nacl/browser/pnacl_host.h"
#include "components/ntp_snippets/content_suggestions_service.h"
#include "components/omnibox/browser/omnibox_pref_names.h"
#include "components/omnibox/browser/omnibox_prefs.h"
#include "components/open_from_clipboard/clipboard_recent_content.h"
#include "components/password_manager/core/browser/password_store.h"
#include "components/password_manager/core/common/password_manager_features.h"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@
#include "components/favicon/core/favicon_service.h"
#include "components/history/core/browser/history_service.h"
#include "components/language/core/browser/url_language_histogram.h"
#include "components/omnibox/browser/omnibox_pref_names.h"
#include "components/omnibox/browser/omnibox_prefs.h"
#include "components/os_crypt/os_crypt_mocker.h"
#include "components/password_manager/core/browser/mock_password_store.h"
#include "components/password_manager/core/browser/password_manager_test_utils.h"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
#include "components/drive/drive_pref_names.h"
#include "components/embedder_support/pref_names.h"
#include "components/language/core/browser/pref_names.h"
#include "components/omnibox/browser/omnibox_pref_names.h"
#include "components/omnibox/browser/omnibox_prefs.h"
#include "components/password_manager/core/common/password_manager_pref_names.h"
#include "components/payments/core/payment_prefs.h"
#include "components/prefs/pref_service.h"
Expand Down
2 changes: 2 additions & 0 deletions chrome/browser/prefs/browser_prefs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@
#include "components/ntp_tiles/most_visited_sites.h"
#include "components/offline_pages/buildflags/buildflags.h"
#include "components/omnibox/browser/document_provider.h"
#include "components/omnibox/browser/omnibox_prefs.h"
#include "components/omnibox/browser/zero_suggest_provider.h"
#include "components/optimization_guide/optimization_guide_prefs.h"
#include "components/password_manager/core/browser/password_bubble_experiment.h"
Expand Down Expand Up @@ -862,6 +863,7 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry,
web_components_prefs::RegisterProfilePrefs(registry);
TemplateURLPrepopulateData::RegisterProfilePrefs(registry);
translate::TranslatePrefs::RegisterProfilePrefs(registry);
omnibox::RegisterProfilePrefs(registry);
ZeroSuggestProvider::RegisterProfilePrefs(registry);

#if BUILDFLAG(ENABLE_EXTENSIONS)
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/ui/browser_commands.cc
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@
#include "components/find_in_page/find_tab_helper.h"
#include "components/find_in_page/find_types.h"
#include "components/google/core/common/google_util.h"
#include "components/omnibox/browser/omnibox_pref_names.h"
#include "components/omnibox/browser/omnibox_prefs.h"
#include "components/prefs/pref_service.h"
#include "components/services/app_service/public/mojom/types.mojom.h"
#include "components/sessions/core/live_tab_context.h"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
#include "components/google/core/common/google_util.h"
#include "components/offline_pages/buildflags/buildflags.h"
#include "components/omnibox/browser/autocomplete_input.h"
#include "components/omnibox/browser/omnibox_pref_names.h"
#include "components/omnibox/browser/omnibox_prefs.h"
#include "components/pref_registry/pref_registry_syncable.h"
#include "components/prefs/pref_service.h"
#include "components/security_interstitials/content/security_interstitial_tab_helper.h"
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/ui/views/frame/browser_frame_mac.mm
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
#include "chrome/common/pref_names.h"
#include "chrome/grit/generated_resources.h"
#include "components/bookmarks/common/bookmark_pref_names.h"
#include "components/omnibox/browser/omnibox_pref_names.h"
#include "components/omnibox/browser/omnibox_prefs.h"
#import "components/remote_cocoa/app_shim/native_widget_mac_nswindow.h"
#import "components/remote_cocoa/app_shim/native_widget_ns_window_bridge.h"
#import "components/remote_cocoa/app_shim/window_touch_bar_delegate.h"
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/ui/views/omnibox/omnibox_view_views.cc
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
#include "components/omnibox/browser/omnibox_edit_model.h"
#include "components/omnibox/browser/omnibox_field_trial.h"
#include "components/omnibox/browser/omnibox_popup_model.h"
#include "components/omnibox/browser/omnibox_pref_names.h"
#include "components/omnibox/browser/omnibox_prefs.h"
#include "components/omnibox/common/omnibox_features.h"
#include "components/security_state/core/security_state.h"
#include "components/strings/grit/components_strings.h"
Expand Down
5 changes: 3 additions & 2 deletions components/omnibox/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,8 @@ jumbo_static_library("browser") {
"omnibox_popup_model.cc",
"omnibox_popup_model.h",
"omnibox_popup_view.h",
"omnibox_pref_names.cc",
"omnibox_pref_names.h",
"omnibox_prefs.cc",
"omnibox_prefs.h",
"omnibox_view.cc",
"omnibox_view.h",
"on_device_head_model.cc",
Expand Down Expand Up @@ -451,6 +451,7 @@ source_set("unit_tests") {
"omnibox_pedal_provider_unittest.cc",
"omnibox_pedal_unittest.cc",
"omnibox_popup_model_unittest.cc",
"omnibox_prefs_unittest.cc",
"omnibox_view_unittest.cc",
"on_device_head_model_unittest.cc",
"on_device_head_provider_unittest.cc",
Expand Down
9 changes: 5 additions & 4 deletions components/omnibox/browser/autocomplete_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -772,10 +772,11 @@ void AutocompleteController::UpdateHeaders(AutocompleteResult* result) {
for (AutocompleteMatch& match : *result) {
if (match.suggestion_group_id.has_value()) {
int group_id = match.suggestion_group_id.value();
match.RecordAdditionalInfo("suggestion_group_id", group_id);
match.RecordAdditionalInfo(
"header string",
base::UTF16ToUTF8(result->GetHeaderForGroupId(group_id)));
const base::string16 header = result->GetHeaderForGroupId(group_id);
if (!header.empty()) {
match.RecordAdditionalInfo("suggestion_group_id", group_id);
match.RecordAdditionalInfo("header string", base::UTF16ToUTF8(header));
}
}
}
}
Expand Down
1 change: 0 additions & 1 deletion components/omnibox/browser/autocomplete_match.cc
Original file line number Diff line number Diff line change
Expand Up @@ -916,7 +916,6 @@ AutocompleteMatch AutocompleteMatch::DerivePedalSuggestion(
void AutocompleteMatch::RecordAdditionalInfo(const std::string& property,
const std::string& value) {
DCHECK(!property.empty());
DCHECK(!value.empty());
additional_info[property] = value;
}

Expand Down
5 changes: 5 additions & 0 deletions components/omnibox/browser/autocomplete_provider_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,11 @@
#include "components/omnibox/browser/autocomplete_provider_listener.h"
#include "components/omnibox/browser/keyword_provider.h"
#include "components/omnibox/browser/mock_autocomplete_provider_client.h"
#include "components/omnibox/browser/omnibox_prefs.h"
#include "components/omnibox/browser/search_provider.h"
#include "components/omnibox/browser/zero_suggest_provider.h"
#include "components/open_from_clipboard/fake_clipboard_recent_content.h"
#include "components/prefs/testing_pref_service.h"
#include "components/search_engines/search_engines_switches.h"
#include "components/search_engines/template_url.h"
#include "components/search_engines/template_url_service.h"
Expand Down Expand Up @@ -327,6 +329,8 @@ class AutocompleteProviderTest : public testing::Test {
provider_headers_map = headers_map;
}

TestingPrefServiceSimple* GetPrefs() { return &pref_service_; }

AutocompleteResult result_;

private:
Expand All @@ -336,6 +340,7 @@ class AutocompleteProviderTest : public testing::Test {
void ResetControllerWithType(int type);

base::test::TaskEnvironment task_environment_;
TestingPrefServiceSimple pref_service_;
std::unique_ptr<AutocompleteController> controller_;
// Owned by |controller_|.
AutocompleteProviderClientWithClosure* client_;
Expand Down
1 change: 1 addition & 0 deletions components/omnibox/browser/autocomplete_result.h
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,7 @@ class AutocompleteResult {

ACMatches matches_;

// The map of suggestion group IDs to headers.
SearchSuggestionParser::HeadersMap headers_map_;

DISALLOW_COPY_AND_ASSIGN(AutocompleteResult);
Expand Down
2 changes: 1 addition & 1 deletion components/omnibox/browser/document_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
#include "components/omnibox/browser/in_memory_url_index_types.h"
#include "components/omnibox/browser/keyword_provider.h"
#include "components/omnibox/browser/omnibox_field_trial.h"
#include "components/omnibox/browser/omnibox_pref_names.h"
#include "components/omnibox/browser/omnibox_prefs.h"
#include "components/omnibox/browser/search_provider.h"
#include "components/omnibox/common/omnibox_features.h"
#include "components/pref_registry/pref_registry_syncable.h"
Expand Down
2 changes: 1 addition & 1 deletion components/omnibox/browser/document_provider_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
#include "components/omnibox/browser/autocomplete_provider.h"
#include "components/omnibox/browser/autocomplete_provider_listener.h"
#include "components/omnibox/browser/mock_autocomplete_provider_client.h"
#include "components/omnibox/browser/omnibox_pref_names.h"
#include "components/omnibox/browser/omnibox_prefs.h"
#include "components/omnibox/browser/test_scheme_classifier.h"
#include "components/omnibox/common/omnibox_features.h"
#include "components/prefs/pref_registry_simple.h"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
#include "components/omnibox/browser/autocomplete_result.h"
#include "components/omnibox/browser/base_search_provider.h"
#include "components/omnibox/browser/omnibox_field_trial.h"
#include "components/omnibox/browser/omnibox_pref_names.h"
#include "components/omnibox/browser/omnibox_prefs.h"
#include "components/omnibox/common/omnibox_features.h"
#include "components/search_engines/template_url_service.h"
#include "url/gurl.h"
Expand Down
19 changes: 0 additions & 19 deletions components/omnibox/browser/omnibox_pref_names.cc

This file was deleted.

19 changes: 0 additions & 19 deletions components/omnibox/browser/omnibox_pref_names.h

This file was deleted.

54 changes: 54 additions & 0 deletions components/omnibox/browser/omnibox_prefs.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// Copyright 2015 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "components/omnibox/browser/omnibox_prefs.h"

#include "base/logging.h"
#include "base/stl_util.h"
#include "base/values.h"
#include "components/prefs/pref_registry_simple.h"
#include "components/prefs/pref_service.h"
#include "components/prefs/scoped_user_pref_update.h"

namespace omnibox {

// A client-side toggle for document (Drive) suggestions.
// Also gated by a feature and server-side Admin Panel controls.
const char kDocumentSuggestEnabled[] = "documentsuggest.enabled";

// A list of suggestion group IDs for zero suggest that are not allowed to
// appear in the results.
const char kOmniboxHiddenGroupIds[] = "omnibox.hiddenGroupIds";

// Boolean that specifies whether to always show full URLs in the omnibox.
const char kPreventUrlElisionsInOmnibox[] = "omnibox.prevent_url_elisions";

// A cache of zero suggest results using JSON serialized into a string.
const char kZeroSuggestCachedResults[] = "zerosuggest.cachedresults";

void RegisterProfilePrefs(PrefRegistrySimple* registry) {
registry->RegisterListPref(omnibox::kOmniboxHiddenGroupIds,
base::Value(base::Value::Type::LIST));
}

bool IsSuggestionGroupIdHidden(PrefService* prefs, int suggestion_group_id) {
DCHECK(prefs);
const base::ListValue* group_id_values =
prefs->GetList(kOmniboxHiddenGroupIds);
return std::find(group_id_values->begin(), group_id_values->end(),
base::Value(suggestion_group_id)) != group_id_values->end();
}

void ToggleSuggestionGroupIdVisibility(PrefService* prefs,
int suggestion_group_id) {
DCHECK(prefs);
ListPrefUpdate update(prefs, kOmniboxHiddenGroupIds);
if (IsSuggestionGroupIdHidden(prefs, suggestion_group_id)) {
update->EraseListValue(base::Value(suggestion_group_id));
} else {
update->Append(suggestion_group_id);
}
}

} // namespace omnibox
37 changes: 37 additions & 0 deletions components/omnibox/browser/omnibox_prefs.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Copyright 2015 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef COMPONENTS_OMNIBOX_BROWSER_OMNIBOX_PREFS_H_
#define COMPONENTS_OMNIBOX_BROWSER_OMNIBOX_PREFS_H_

#include <vector>

class PrefRegistrySimple;
class PrefService;

namespace omnibox {

// Alphabetical list of preference names specific to the omnibox component.
// Keep alphabetized, and document each in the .cc file.

extern const char kDocumentSuggestEnabled[];
extern const char kOmniboxHiddenGroupIds[];
extern const char kPreventUrlElisionsInOmnibox[];
extern const char kZeroSuggestCachedResults[];

void RegisterProfilePrefs(PrefRegistrySimple* registry);

// Returns whether the given suggestion group ID is allowed to appear in the
// results.
bool IsSuggestionGroupIdHidden(PrefService* prefs, int suggestion_group_id);

// Allows suggestions with the given suggestion group ID to appear in the
// results if they currently are not allowed to or prevents them from
// appearing in the results if they are currently permitted to.
void ToggleSuggestionGroupIdVisibility(PrefService* prefs,
int suggestion_group_id);

} // namespace omnibox

#endif // COMPONENTS_OMNIBOX_BROWSER_OMNIBOX_PREFS_H_
45 changes: 45 additions & 0 deletions components/omnibox/browser/omnibox_prefs_unittest.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "components/omnibox/browser/omnibox_prefs.h"
#include "components/prefs/testing_pref_service.h"
#include "testing/gtest/include/gtest/gtest.h"

using omnibox::IsSuggestionGroupIdHidden;
using omnibox::ToggleSuggestionGroupIdVisibility;

class OmniboxPrefsTest : public ::testing::Test {
public:
OmniboxPrefsTest() = default;

void SetUp() override {
omnibox::RegisterProfilePrefs(GetPrefs()->registry());
}

TestingPrefServiceSimple* GetPrefs() { return &pref_service_; }

private:
TestingPrefServiceSimple pref_service_;

DISALLOW_COPY_AND_ASSIGN(OmniboxPrefsTest);
};

TEST_F(OmniboxPrefsTest, SuggestionGroupId) {
const int kRecommendedForYouGroupId = 1;
const int kRecentSearchesGroupId = 2;

EXPECT_FALSE(
IsSuggestionGroupIdHidden(GetPrefs(), kRecommendedForYouGroupId));
EXPECT_FALSE(IsSuggestionGroupIdHidden(GetPrefs(), kRecentSearchesGroupId));

ToggleSuggestionGroupIdVisibility(GetPrefs(), kRecommendedForYouGroupId);
EXPECT_TRUE(IsSuggestionGroupIdHidden(GetPrefs(), kRecommendedForYouGroupId));
EXPECT_FALSE(IsSuggestionGroupIdHidden(GetPrefs(), kRecentSearchesGroupId));

ToggleSuggestionGroupIdVisibility(GetPrefs(), kRecommendedForYouGroupId);
ToggleSuggestionGroupIdVisibility(GetPrefs(), kRecentSearchesGroupId);
EXPECT_FALSE(
IsSuggestionGroupIdHidden(GetPrefs(), kRecommendedForYouGroupId));
EXPECT_TRUE(IsSuggestionGroupIdHidden(GetPrefs(), kRecentSearchesGroupId));
}
5 changes: 2 additions & 3 deletions components/omnibox/browser/zero_suggest_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
#include "components/omnibox/browser/autocomplete_provider_client.h"
#include "components/omnibox/browser/autocomplete_provider_listener.h"
#include "components/omnibox/browser/omnibox_field_trial.h"
#include "components/omnibox/browser/omnibox_pref_names.h"
#include "components/omnibox/browser/omnibox_prefs.h"
#include "components/omnibox/browser/remote_suggestions_service.h"
#include "components/omnibox/browser/search_provider.h"
#include "components/omnibox/browser/search_suggestion_parser.h"
Expand Down Expand Up @@ -198,8 +198,7 @@ ZeroSuggestProvider* ZeroSuggestProvider::Create(
}

// static
void ZeroSuggestProvider::RegisterProfilePrefs(
user_prefs::PrefRegistrySyncable* registry) {
void ZeroSuggestProvider::RegisterProfilePrefs(PrefRegistrySimple* registry) {
registry->RegisterStringPref(omnibox::kZeroSuggestCachedResults,
std::string());
}
Expand Down
Loading

0 comments on commit 1266de8

Please sign in to comment.