Skip to content

Commit

Permalink
[journeys] Do not show omnibox action on URLs from noisy visits (behi…
Browse files Browse the repository at this point in the history 
…nd flag)

(cherry picked from commit 6d2f399)

Bug: 1321953
Change-Id: Iac167ad2ea51e80678c709e19fbf06049aceff3d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3621562
Reviewed-by: Tommy Li <tommycli@chromium.org>
Commit-Queue: Sophie Chang <sophiechang@chromium.org>
Reviewed-by: Michael Crouse <mcrouse@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#998982}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3635519
Auto-Submit: Sophie Chang <sophiechang@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/5005@{#584}
Cr-Branched-From: 5b4d945-refs/heads/main@{#992738}
  • Loading branch information
Sophie Chang authored and Chromium LUCI CQ committed May 9, 2022
1 parent fe2c822 commit 4802660
Show file tree
Hide file tree
Showing 5 changed files with 211 additions and 4 deletions.
15 changes: 12 additions & 3 deletions chrome/browser/about_flags.cc
View file
Expand Up @@ -987,12 +987,21 @@ const FeatureEntry::FeatureVariation kJourneysVariations[] = {
{"All Supported Locales", kJourneysAllLocalesParams,
std::size(kJourneysAllLocalesParams), nullptr},
};
const FeatureEntry::FeatureParam kJourneysOmniboxActionOnURLsParams[] = {
const FeatureEntry::FeatureParam kJourneysOmniboxActionOnAllURLsParams[] = {
{"omnibox_action_on_urls", "true"},
{"omnibox_action_on_noisy_urls", "true"},
};
const FeatureEntry::FeatureParam kJourneysOmniboxActionOnNonNoisyURLsParams[] =
{
{"omnibox_action_on_urls", "true"},
{"omnibox_action_on_noisy_urls", "false"},
};
const FeatureEntry::FeatureVariation kJourneysOmniboxActionVariations[] = {
{"Action Chips on URLs", kJourneysOmniboxActionOnURLsParams,
std::size(kJourneysOmniboxActionOnURLsParams), nullptr},
{"Action Chips on All URLs", kJourneysOmniboxActionOnAllURLsParams,
std::size(kJourneysOmniboxActionOnAllURLsParams), nullptr},
{"Action Chips on Non-Noisy URLs",
kJourneysOmniboxActionOnNonNoisyURLsParams,
std::size(kJourneysOmniboxActionOnNonNoisyURLsParams), nullptr},
};
const FeatureEntry::FeatureParam
kJourneysOnDeviceClusteringLabelingNoContentClusteringParams[] = {
Expand Down
4 changes: 4 additions & 0 deletions components/history_clusters/core/config.cc
View file
Expand Up @@ -73,6 +73,10 @@ Config::Config() {
internal::kOmniboxAction, "omnibox_action_on_urls",
omnibox_action_on_urls);

omnibox_action_on_noisy_urls = base::GetFieldTrialParamByFeatureAsBool(
internal::kOmniboxAction, "omnibox_action_on_noisy_urls",
omnibox_action_on_noisy_urls);

non_user_visible_debug =
base::FeatureList::IsEnabled(internal::kNonUserVisibleDebug);

Expand Down
4 changes: 4 additions & 0 deletions components/history_clusters/core/config.h
View file
Expand Up @@ -80,6 +80,10 @@ struct Config {
// does nothing if `omnibox_action` is disabled.
bool omnibox_action_on_urls = false;

// If enabled, allows the Omnibox Action chip to appear on URLs from noisy
// visits. This does nothing if `omnibox_action_on_urls` is false.
bool omnibox_action_on_noisy_urls = true;

// Enables debug info in non-user-visible surfaces, like Chrome Inspector.
// Does nothing if `kJourneys` is disabled.
bool non_user_visible_debug = false;
Expand Down
9 changes: 9 additions & 0 deletions components/history_clusters/core/history_clusters_service.cc
View file
Expand Up @@ -456,6 +456,15 @@ void HistoryClustersService::PopulateClusterKeywordCache(
// Push a simplified form of the URL for each visit into the cache.
if (url_keyword_accumulator->size() < max_keyword_phrases) {
for (const auto& visit : cluster.visits) {
if (visit.engagement_score >
GetConfig().noisy_cluster_visits_engagement_threshold &&
!GetConfig().omnibox_action_on_noisy_urls) {
// Do not add a noisy visit to the URL keyword accumulator if not
// enabled via flag. Note that this is at the visit-level rather than
// at the cluster-level, which is handled by the NoisyClusterFinalizer
// in the ClusteringBackend.
continue;
}
url_keyword_accumulator->insert(
(!visit.annotated_visit.content_annotations.search_normalized_url
.is_empty())
Expand Down
183 changes: 182 additions & 1 deletion components/history_clusters/core/history_clusters_service_unittest.cc
View file
Expand Up @@ -29,6 +29,7 @@
#include "components/history_clusters/core/features.h"
#include "components/history_clusters/core/history_clusters_service_test_api.h"
#include "components/history_clusters/core/history_clusters_types.h"
#include "components/history_clusters/core/history_clusters_util.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "url/gurl.h"

Expand Down Expand Up @@ -58,13 +59,16 @@ class TestClusteringBackend : public ClusteringBackend {

// Fetches a scored visit by an ID. `visit_id` must be valid. This is a
// convenience method used for constructing the fake response.
history::ClusterVisit GetVisitById(int visit_id, float score = 0.5) {
history::ClusterVisit GetVisitById(int visit_id,
float score = 0.5,
int engagement_score = 0) {
for (auto& visit : last_clustered_visits_) {
if (visit.visit_row.visit_id == visit_id) {
history::ClusterVisit cluster_visit;
cluster_visit.annotated_visit = visit;
cluster_visit.normalized_url = visit.url_row.url();
cluster_visit.score = score;
cluster_visit.engagement_score = engagement_score;
return cluster_visit;
}
}
Expand Down Expand Up @@ -738,6 +742,183 @@ TEST_F(HistoryClustersServiceTest, DoesQueryMatchAnyClusterSecondaryCache) {
EXPECT_TRUE(history_clusters_service_->DoesQueryMatchAnyCluster("peach"));
}

TEST_F(HistoryClustersServiceTest, DoesURLMatchAnyClusterWithNoisyURLs) {
Config config;
config.omnibox_action_on_urls = true;
config.omnibox_action_on_noisy_urls = true;
SetConfigForTesting(config);

AddHardcodedTestDataToHistoryService();

// Verify that initially, the test URL doesn't match anything, but this
// query should have kicked off a cache population request. This is the URL
// for visit 5.
EXPECT_FALSE(history_clusters_service_->DoesURLMatchAnyCluster(
ComputeURLKeywordForLookup(GURL("https://second-1-day-old-visit.com/"))));

// Helper to flush out the multiple history and cluster backend requests made
// by `DoesURLMatchAnyCluster()`. It won't populate the cache until all its
// requests have been completed. It makes 1 request (to each) per unique day
// with at least 1 visit; i.e. `number_of_days_with_visits`.
const auto flush_keyword_requests = [&](size_t number_of_days_with_visits) {
test_clustering_backend_->WaitForGetClustersCall();

std::vector<history::Cluster> clusters;
clusters.push_back(history::Cluster(
0,
{
test_clustering_backend_->GetVisitById(5),
test_clustering_backend_->GetVisitById(
/*visit_id=*/2, /*score=*/0.0, /*engagement_score=*/20.0),
},
{u"apples", u"oranges", u"z", u"apples bananas"},
/*should_show_on_prominent_ui_surfaces=*/true));
clusters.push_back(
history::Cluster(0,
{
test_clustering_backend_->GetVisitById(5),
test_clustering_backend_->GetVisitById(2),
},
{u"sensitive"},
/*should_show_on_prominent_ui_surfaces=*/false));
clusters.push_back(
history::Cluster(0,
{
test_clustering_backend_->GetVisitById(2),
},
{u"singlevisit"},
/*should_show_on_prominent_ui_surfaces=*/true));

test_clustering_backend_->FulfillCallback(clusters);

// `DoesQueryMatchAnyCluster()` will continue making history and cluster
// backend requests until it has exhausted history. We have to flush out
// these requests before it will populate the cache.
for (size_t i = 0; i < number_of_days_with_visits - 1; ++i) {
test_clustering_backend_->WaitForGetClustersCall();
history::BlockUntilHistoryProcessesPendingRequests(
history_service_.get());
test_clustering_backend_->FulfillCallback({});
}
// One last wait to flush out the last, empty history request.
history::BlockUntilHistoryProcessesPendingRequests(history_service_.get());
};

// Hardcoded test visits span 3 days (1-day-old, 2-days-old, and 60-day-old).
flush_keyword_requests(3);

// Now the exact query should match the populated cache.
EXPECT_TRUE(history_clusters_service_->DoesURLMatchAnyCluster(
ComputeURLKeywordForLookup(GURL("https://second-1-day-old-visit.com/"))));

// Github should be shown since we are including visits from noisy URLs.
EXPECT_TRUE(history_clusters_service_->DoesURLMatchAnyCluster(
ComputeURLKeywordForLookup(GURL("https://github.com/"))));

// Deleting a history entry should clear the keyword cache.
history_service_->DeleteURLs({GURL{"https://google.com/"}});
history::BlockUntilHistoryProcessesPendingRequests(history_service_.get());
EXPECT_FALSE(history_clusters_service_->DoesURLMatchAnyCluster(
ComputeURLKeywordForLookup(GURL("https://second-1-day-old-visit.com/"))));

// Visits now span 2 days (1-day-old and 60-day-old) since we deleted the only
// 2-day-old visit.
flush_keyword_requests(2);

// The keyword cache should be repopulated.
EXPECT_TRUE(history_clusters_service_->DoesURLMatchAnyCluster(
ComputeURLKeywordForLookup(GURL("https://second-1-day-old-visit.com/"))));
}

TEST_F(HistoryClustersServiceTest, DoesURLMatchAnyClusterNoNoisyURLs) {
Config config;
config.omnibox_action_on_urls = true;
config.omnibox_action_on_noisy_urls = false;
SetConfigForTesting(config);

AddHardcodedTestDataToHistoryService();

// Verify that initially, the test URL doesn't match anything, but this
// query should have kicked off a cache population request. This is the URL
// for visit 5.
EXPECT_FALSE(history_clusters_service_->DoesURLMatchAnyCluster(
ComputeURLKeywordForLookup(GURL("https://second-1-day-old-visit.com/"))));

// Helper to flush out the multiple history and cluster backend requests made
// by `DoesURLMatchAnyCluster()`. It won't populate the cache until all its
// requests have been completed. It makes 1 request (to each) per unique day
// with at least 1 visit; i.e. `number_of_days_with_visits`.
const auto flush_keyword_requests = [&](size_t number_of_days_with_visits) {
test_clustering_backend_->WaitForGetClustersCall();

std::vector<history::Cluster> clusters;
clusters.push_back(history::Cluster(
0,
{
test_clustering_backend_->GetVisitById(5),
test_clustering_backend_->GetVisitById(
/*visit_id=*/2, /*score=*/0.0, /*engagement_score=*/20.0),
},
{u"apples", u"oranges", u"z", u"apples bananas"},
/*should_show_on_prominent_ui_surfaces=*/true));
clusters.push_back(
history::Cluster(0,
{
test_clustering_backend_->GetVisitById(5),
test_clustering_backend_->GetVisitById(2),
},
{u"sensitive"},
/*should_show_on_prominent_ui_surfaces=*/false));
clusters.push_back(
history::Cluster(0,
{
test_clustering_backend_->GetVisitById(2),
},
{u"singlevisit"},
/*should_show_on_prominent_ui_surfaces=*/true));

test_clustering_backend_->FulfillCallback(clusters);

// `DoesQueryMatchAnyCluster()` will continue making history and cluster
// backend requests until it has exhausted history. We have to flush out
// these requests before it will populate the cache.
for (size_t i = 0; i < number_of_days_with_visits - 1; ++i) {
test_clustering_backend_->WaitForGetClustersCall();
history::BlockUntilHistoryProcessesPendingRequests(
history_service_.get());
test_clustering_backend_->FulfillCallback({});
}
// One last wait to flush out the last, empty history request.
history::BlockUntilHistoryProcessesPendingRequests(history_service_.get());
};

// Hardcoded test visits span 3 days (1-day-old, 2-days-old, and 60-day-old).
flush_keyword_requests(3);

// Now the exact query should match the populated cache.
EXPECT_TRUE(history_clusters_service_->DoesURLMatchAnyCluster(
ComputeURLKeywordForLookup(GURL("https://second-1-day-old-visit.com/"))));

// Github should never be shown (highly-engaged for cluster 1, sensitive for
// cluster 2, single visit cluster for cluster 3).
EXPECT_FALSE(history_clusters_service_->DoesURLMatchAnyCluster(
ComputeURLKeywordForLookup(GURL("https://github.com/"))));

// Deleting a history entry should clear the keyword cache.
history_service_->DeleteURLs({GURL{"https://google.com/"}});
history::BlockUntilHistoryProcessesPendingRequests(history_service_.get());
EXPECT_FALSE(history_clusters_service_->DoesURLMatchAnyCluster(
ComputeURLKeywordForLookup(GURL("https://second-1-day-old-visit.com/"))));

// Visits now span 2 days (1-day-old and 60-day-old) since we deleted the only
// 2-day-old visit.
flush_keyword_requests(2);

// The keyword cache should be repopulated.
EXPECT_TRUE(history_clusters_service_->DoesURLMatchAnyCluster(
ComputeURLKeywordForLookup(GURL("https://second-1-day-old-visit.com/"))));
}

class HistoryClustersServiceMaxKeywordsTest
: public HistoryClustersServiceTestBase {
public:
Expand Down

0 comments on commit 4802660

Please sign in to comment.