diff options
Diffstat (limited to 'chromium/components/ntp_snippets/contextual')
28 files changed, 610 insertions, 128 deletions
diff --git a/chromium/components/ntp_snippets/contextual/contextual_content_suggestions_service.cc b/chromium/components/ntp_snippets/contextual/contextual_content_suggestions_service.cc index c88ce6ad113..7c7ba9aeb0a 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_content_suggestions_service.cc +++ b/chromium/components/ntp_snippets/contextual/contextual_content_suggestions_service.cc @@ -11,11 +11,11 @@ #include "base/bind.h" #include "base/memory/ptr_util.h" +#include "components/ntp_snippets/contextual/contextual_content_suggestions_service_proxy.h" #include "components/ntp_snippets/contextual/contextual_suggestions_result.h" #include "components/ntp_snippets/remote/cached_image_fetcher.h" #include "components/ntp_snippets/remote/remote_suggestions_database.h" #include "components/ntp_snippets/remote/remote_suggestions_provider_impl.h" -#include "contextual_content_suggestions_service_proxy.h" #include "ui/gfx/image/image.h" namespace contextual_suggestions { @@ -43,6 +43,7 @@ ContextualContentSuggestionsService::ContextualContentSuggestionsService( std::unique_ptr<ContextualSuggestionsReporterProvider> reporter_provider) : contextual_suggestions_database_( std::move(contextual_suggestions_database)), + fetch_cache_(kFetchCacheCapacity), contextual_suggestions_fetcher_( std::move(contextual_suggestions_fetcher)), image_fetcher_(std::move(image_fetcher)), @@ -56,15 +57,17 @@ void ContextualContentSuggestionsService::FetchContextualSuggestionClusters( FetchClustersCallback callback, ReportFetchMetricsCallback metrics_callback) { // TODO(pnoland): Also check that the url is safe. - if (IsEligibleURL(url)) { + ContextualSuggestionsResult result; + if (IsEligibleURL(url) && !fetch_cache_.GetSuggestionsResult(url, &result)) { FetchClustersCallback internal_callback = base::BindOnce( &ContextualContentSuggestionsService::FetchDone, base::Unretained(this), - std::move(callback), metrics_callback); + url, std::move(callback), metrics_callback); contextual_suggestions_fetcher_->FetchContextualSuggestionsClusters( url, std::move(internal_callback), metrics_callback); + } else if (result.peek_conditions.confidence < kMinimumConfidence) { + BelowConfidenceThresholdFetchDone(std::move(callback), metrics_callback); } else { - std::move(callback).Run( - ContextualSuggestionsResult("", {}, PeekConditions())); + std::move(callback).Run(result); } } @@ -78,19 +81,38 @@ void ContextualContentSuggestionsService::FetchContextualSuggestionImage( } void ContextualContentSuggestionsService::FetchDone( + const GURL& url, FetchClustersCallback callback, ReportFetchMetricsCallback metrics_callback, ContextualSuggestionsResult result) { + // We still want to cache low confidence results so that we avoid doing + // unnecessary fetches. + if (result.clusters.size() > 0) { + fetch_cache_.AddSuggestionsResult(url, result); + } + if (result.peek_conditions.confidence < kMinimumConfidence) { - metrics_callback.Run(contextual_suggestions::FETCH_BELOW_THRESHOLD); - std::move(callback).Run( - ContextualSuggestionsResult("", {}, PeekConditions())); + BelowConfidenceThresholdFetchDone(std::move(callback), metrics_callback); return; } std::move(callback).Run(result); } +ContextualSuggestionsDebuggingReporter* +ContextualContentSuggestionsService::GetDebuggingReporter() { + return reporter_provider_->GetDebuggingReporter(); +} + +base::flat_map<GURL, ContextualSuggestionsResult> +ContextualContentSuggestionsService::GetAllCachedResultsForDebugging() { + return fetch_cache_.GetAllCachedResultsForDebugging(); +} + +void ContextualContentSuggestionsService::ClearCachedResultsForDebugging() { + fetch_cache_.Clear(); +} + std::unique_ptr< contextual_suggestions::ContextualContentSuggestionsServiceProxy> ContextualContentSuggestionsService::CreateProxy() { @@ -99,4 +121,12 @@ ContextualContentSuggestionsService::CreateProxy() { this, reporter_provider_->CreateReporter()); } +void ContextualContentSuggestionsService::BelowConfidenceThresholdFetchDone( + FetchClustersCallback callback, + ReportFetchMetricsCallback metrics_callback) { + metrics_callback.Run(contextual_suggestions::FETCH_BELOW_THRESHOLD); + std::move(callback).Run(ContextualSuggestionsResult("", {}, PeekConditions(), + ServerExperimentInfos())); +} + } // namespace contextual_suggestions diff --git a/chromium/components/ntp_snippets/contextual/contextual_content_suggestions_service.h b/chromium/components/ntp_snippets/contextual/contextual_content_suggestions_service.h index 23214ede416..1100a89f4f9 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_content_suggestions_service.h +++ b/chromium/components/ntp_snippets/contextual/contextual_content_suggestions_service.h @@ -16,6 +16,8 @@ #include "components/keyed_service/core/keyed_service.h" #include "components/ntp_snippets/callbacks.h" #include "components/ntp_snippets/content_suggestion.h" +#include "components/ntp_snippets/contextual/contextual_suggestions_cache.h" +#include "components/ntp_snippets/contextual/contextual_suggestions_debugging_reporter.h" #include "components/ntp_snippets/contextual/contextual_suggestions_fetcher.h" #include "components/ntp_snippets/contextual/contextual_suggestions_reporter.h" #include "services/metrics/public/cpp/ukm_source_id.h" @@ -27,6 +29,8 @@ class RemoteSuggestionsDatabase; namespace contextual_suggestions { +static constexpr int kFetchCacheCapacity = 10; + class ContextualContentSuggestionsServiceProxy; // Retrieves contextual suggestions for a given URL and fetches images @@ -57,17 +61,34 @@ class ContextualContentSuggestionsService : public KeyedService { const GURL& url, ntp_snippets::ImageFetchedCallback callback); - void FetchDone(FetchClustersCallback callback, + void FetchDone(const GURL& url, + FetchClustersCallback callback, ReportFetchMetricsCallback metrics_callback, ContextualSuggestionsResult result); + // Used to surface metrics events via chrome://eoc-internals. + ContextualSuggestionsDebuggingReporter* GetDebuggingReporter(); + + // Expose cached results for debugging. + base::flat_map<GURL, ContextualSuggestionsResult> + GetAllCachedResultsForDebugging(); + + // Clear the cached results for debugging. + void ClearCachedResultsForDebugging(); + std::unique_ptr<ContextualContentSuggestionsServiceProxy> CreateProxy(); private: + void BelowConfidenceThresholdFetchDone( + FetchClustersCallback callback, + ReportFetchMetricsCallback metrics_callback); // Cache for images of contextual suggestions, needed by CachedImageFetcher. std::unique_ptr<ntp_snippets::RemoteSuggestionsDatabase> contextual_suggestions_database_; + // Cache of contextual suggestions fetch results, keyed by the context url. + ContextualSuggestionsCache fetch_cache_; + // Performs actual network request to fetch contextual suggestions. std::unique_ptr<ContextualSuggestionsFetcher> contextual_suggestions_fetcher_; diff --git a/chromium/components/ntp_snippets/contextual/contextual_content_suggestions_service_proxy_unittest.cc b/chromium/components/ntp_snippets/contextual/contextual_content_suggestions_service_proxy_unittest.cc index e0de6e85134..5bbe7dcd40f 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_content_suggestions_service_proxy_unittest.cc +++ b/chromium/components/ntp_snippets/contextual/contextual_content_suggestions_service_proxy_unittest.cc @@ -6,6 +6,7 @@ #include <memory> #include <string> +#include <utility> #include "base/bind.h" #include "components/ntp_snippets/contextual/contextual_content_suggestions_service.h" @@ -23,8 +24,8 @@ namespace contextual_suggestions { namespace { -static const std::string kTestPeekText("Test peek test"); -static const std::string kValidFromUrl = "http://some.url"; +static constexpr char kTestPeekText[] = "Test peek test"; +static constexpr char kValidFromUrl[] = "http://some.url"; class FakeContextualContentSuggestionsService : public ContextualContentSuggestionsService { @@ -47,14 +48,13 @@ class FakeContextualContentSuggestionsService FetchClustersCallback clusters_callback_; }; -} // namespace - FakeContextualContentSuggestionsService:: FakeContextualContentSuggestionsService() : ContextualContentSuggestionsService(nullptr, nullptr, nullptr, nullptr) {} FakeContextualContentSuggestionsService:: ~FakeContextualContentSuggestionsService() {} +} // namespace class ContextualContentSuggestionsServiceProxyTest : public testing::Test { public: @@ -83,7 +83,10 @@ TEST_F(ContextualContentSuggestionsServiceProxyTest, proxy()->FetchContextualSuggestions(GURL(kValidFromUrl), mock_cluster_callback.ToOnceCallback()); - service()->RunClustersCallback(ContextualSuggestionsResult()); + service()->RunClustersCallback( + ContextualSuggestionsResult(kTestPeekText, std::vector<Cluster>(), + PeekConditions(), ServerExperimentInfos())); + EXPECT_TRUE(mock_cluster_callback.has_run); } diff --git a/chromium/components/ntp_snippets/contextual/contextual_content_suggestions_service_unittest.cc b/chromium/components/ntp_snippets/contextual/contextual_content_suggestions_service_unittest.cc index b44bebfa921..015b1569f74 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_content_suggestions_service_unittest.cc +++ b/chromium/components/ntp_snippets/contextual/contextual_content_suggestions_service_unittest.cc @@ -13,6 +13,7 @@ #include "base/macros.h" #include "base/message_loop/message_loop.h" #include "base/run_loop.h" +#include "base/strings/string_number_conversions.h" #include "base/test/mock_callback.h" #include "components/image_fetcher/core/image_fetcher_impl.h" #include "components/ntp_snippets/category_info.h" @@ -82,10 +83,10 @@ class FakeContextualSuggestionsFetcher : public ContextualSuggestionsFetcher { // Always fetches a fake image if the given URL is valid. class FakeCachedImageFetcher : public CachedImageFetcher { public: - FakeCachedImageFetcher(PrefService* pref_service) + explicit FakeCachedImageFetcher(PrefService* pref_service) : CachedImageFetcher(std::unique_ptr<image_fetcher::ImageFetcher>(), pref_service, - nullptr){}; + nullptr) {} void FetchSuggestionImage(const ContentSuggestion::ID&, const GURL& image_url, @@ -202,4 +203,116 @@ TEST_F(ContextualContentSuggestionsServiceTest, EXPECT_EQ(mock_callback.response_peek_text, std::string()); } +TEST_F(ContextualContentSuggestionsServiceTest, ShouldCacheResults) { + MockClustersCallback mock_callback; + MockClustersCallback mock_callback2; + std::vector<Cluster> clusters; + GURL context_url("http://www.from.url"); + + clusters.emplace_back(ClusterBuilder("Title") + .AddSuggestion(SuggestionBuilder(context_url) + .Title("Title1") + .PublisherName("from.url") + .Snippet("Summary") + .ImageId("abc") + .Build()) + .Build()); + fetcher()->SetFakeResponse(clusters); + source()->FetchContextualSuggestionClusters( + context_url, mock_callback.ToOnceCallback(), base::DoNothing()); + base::RunLoop().RunUntilIdle(); + + EXPECT_TRUE(mock_callback.has_run); + + // The correct result should be present even though we haven't set the fake + // response. + source()->FetchContextualSuggestionClusters( + context_url, mock_callback2.ToOnceCallback(), base::DoNothing()); + EXPECT_TRUE(mock_callback2.has_run); + ExpectResponsesMatch( + mock_callback2, + ContextualSuggestionsResult("peek text", clusters, PeekConditions(), + ServerExperimentInfos())); +} + +TEST_F(ContextualContentSuggestionsServiceTest, ShouldEvictOldCachedResults) { + std::vector<Cluster> clusters; + clusters.emplace_back( + ClusterBuilder("Title") + .AddSuggestion(SuggestionBuilder(GURL("http://foobar.com")) + .Title("Title1") + .PublisherName("from.url") + .Snippet("Summary") + .ImageId("abc") + .Build()) + .Build()); + + for (int i = 0; i < kFetchCacheCapacity + 1; i++) { + MockClustersCallback mock_callback; + GURL context_url("http://www.from.url/" + base::NumberToString(i)); + + fetcher()->SetFakeResponse(clusters); + source()->FetchContextualSuggestionClusters( + context_url, mock_callback.ToOnceCallback(), base::DoNothing()); + base::RunLoop().RunUntilIdle(); + + ExpectResponsesMatch( + mock_callback, + ContextualSuggestionsResult("peek text", clusters, PeekConditions(), + ServerExperimentInfos())); + } + + // Urls numbered kFetchCacheCapacity through 1 should be cached still; 0 + // should have been evicted. + for (int i = kFetchCacheCapacity; i > 0; i--) { + GURL context_url("http://www.from.url/" + base::NumberToString(i)); + MockClustersCallback mock_callback; + source()->FetchContextualSuggestionClusters( + context_url, mock_callback.ToOnceCallback(), base::DoNothing()); + ExpectResponsesMatch( + mock_callback, + ContextualSuggestionsResult("peek text", clusters, PeekConditions(), + ServerExperimentInfos())); + } + + GURL context_url("http://www.from.url/0"); + MockClustersCallback mock_callback; + source()->FetchContextualSuggestionClusters( + context_url, mock_callback.ToOnceCallback(), base::DoNothing()); + EXPECT_EQ(mock_callback.response_clusters.size(), 0u); +} + +TEST_F(ContextualContentSuggestionsServiceTest, + ShouldNotReturnCachedLowConfidenceResults) { + MockClustersCallback mock_callback; + MockClustersCallback mock_callback2; + std::vector<Cluster> clusters; + GURL context_url("http://www.from.url"); + + clusters.emplace_back(ClusterBuilder("Title") + .AddSuggestion(SuggestionBuilder(context_url) + .Title("Title1") + .PublisherName("from.url") + .Snippet("Summary") + .ImageId("abc") + .Build()) + .Build()); + PeekConditions peek_conditions; + peek_conditions.confidence = 0; + fetcher()->SetFakeResponse(clusters, peek_conditions); + source()->FetchContextualSuggestionClusters( + context_url, mock_callback.ToOnceCallback(), base::DoNothing()); + base::RunLoop().RunUntilIdle(); + + EXPECT_TRUE(mock_callback.has_run); + ExpectResponsesMatch(mock_callback, ContextualSuggestionsResult()); + + // The cached result we get back should be empty, since its confidence is + // below the threshold. + source()->FetchContextualSuggestionClusters( + context_url, mock_callback2.ToOnceCallback(), base::DoNothing()); + EXPECT_TRUE(mock_callback2.has_run); + ExpectResponsesMatch(mock_callback2, ContextualSuggestionsResult()); +} + } // namespace contextual_suggestions diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestion.cc b/chromium/components/ntp_snippets/contextual/contextual_suggestion.cc index 1155b7fc6e1..d71dcb30825 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_suggestion.cc +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestion.cc @@ -2,6 +2,9 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include <string> +#include <utility> + #include "components/ntp_snippets/contextual/contextual_suggestion.h" namespace contextual_suggestions { @@ -26,6 +29,9 @@ ContextualSuggestion::ContextualSuggestion( ContextualSuggestion::~ContextualSuggestion() = default; +ContextualSuggestion& ContextualSuggestion::operator=( + const ContextualSuggestion&) = default; + SuggestionBuilder::SuggestionBuilder(const GURL& url) { suggestion_.url = url; suggestion_.id = url.spec(); diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestion.h b/chromium/components/ntp_snippets/contextual/contextual_suggestion.h index d14e64a00a9..28f4b513a06 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_suggestion.h +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestion.h @@ -5,6 +5,8 @@ #ifndef COMPONENTS_NTP_SNIPPETS_CONTEXTUAL_CONTEXTUAL_SUGGESTION_H_ #define COMPONENTS_NTP_SNIPPETS_CONTEXTUAL_CONTEXTUAL_SUGGESTION_H_ +#include <string> + #include "url/gurl.h" namespace contextual_suggestions { @@ -16,6 +18,8 @@ struct ContextualSuggestion { ContextualSuggestion(ContextualSuggestion&&) noexcept; ~ContextualSuggestion(); + ContextualSuggestion& operator=(const ContextualSuggestion&); + // The ID identifying the suggestion. std::string id; @@ -48,7 +52,7 @@ struct ContextualSuggestion { // order has to be guessed at. class SuggestionBuilder { public: - SuggestionBuilder(const GURL& url); + explicit SuggestionBuilder(const GURL& url); SuggestionBuilder& Title(const std::string& title); SuggestionBuilder& PublisherName(const std::string& publisher_name); diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestions_cache.cc b/chromium/components/ntp_snippets/contextual/contextual_suggestions_cache.cc new file mode 100644 index 00000000000..07b282d48f7 --- /dev/null +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestions_cache.cc @@ -0,0 +1,41 @@ +// Copyright 2018 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/ntp_snippets/contextual/contextual_suggestions_cache.h" + +namespace contextual_suggestions { + +ContextualSuggestionsCache::ContextualSuggestionsCache(size_t capacity) + : cache_(capacity) {} +ContextualSuggestionsCache::~ContextualSuggestionsCache() = default; + +base::flat_map<GURL, ContextualSuggestionsResult> +ContextualSuggestionsCache::GetAllCachedResultsForDebugging() { + return base::flat_map<GURL, ContextualSuggestionsResult>(cache_.begin(), + cache_.end()); +} + +bool ContextualSuggestionsCache::GetSuggestionsResult( + const GURL& url, + ContextualSuggestionsResult* result) { + auto cache_iter = cache_.Get(url); + if (cache_iter == cache_.end()) { + return false; + } + + *result = cache_iter->second; + return true; +} + +void ContextualSuggestionsCache::AddSuggestionsResult( + const GURL& url, + ContextualSuggestionsResult result) { + cache_.Put(url, result); +} + +void ContextualSuggestionsCache::Clear() { + cache_.Clear(); +} + +} // namespace contextual_suggestions diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestions_cache.h b/chromium/components/ntp_snippets/contextual/contextual_suggestions_cache.h new file mode 100644 index 00000000000..b1639bd88b8 --- /dev/null +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestions_cache.h @@ -0,0 +1,44 @@ +// Copyright 2018 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_NTP_SNIPPETS_CONTEXTUAL_CONTEXTUAL_SUGGESTIONS_CACHE_H_ +#define COMPONENTS_NTP_SNIPPETS_CONTEXTUAL_CONTEXTUAL_SUGGESTIONS_CACHE_H_ + +#include "base/containers/flat_map.h" +#include "base/containers/mru_cache.h" +#include "components/ntp_snippets/contextual/contextual_suggestions_result.h" +#include "url/gurl.h" + +namespace contextual_suggestions { + +// Wrapper for an LRU cache of ContextualSuggestionResult objects, keyed by +// context URL. +class ContextualSuggestionsCache { + public: + explicit ContextualSuggestionsCache(size_t capacity); + ~ContextualSuggestionsCache(); + + // Attempts to find a result for |url| in this cache, returning true and + // putting the result into |result| if successful. + bool GetSuggestionsResult(const GURL& url, + ContextualSuggestionsResult* result); + // Adds |result| to this cache for the key |url|, overwriting any previous + // value associated with |url| and potentially evicting the oldest item in the + // cache. + void AddSuggestionsResult(const GURL& url, + ContextualSuggestionsResult result); + // Removes all items from the cache. + void Clear(); + + // Returns all suggestion results for debugging purposes. + base::flat_map<GURL, ContextualSuggestionsResult> + GetAllCachedResultsForDebugging(); + + private: + base::MRUCache<GURL, ContextualSuggestionsResult> cache_; +}; + +} // namespace contextual_suggestions + +#endif // COMPONENTS_NTP_SNIPPETS_CONTEXTUAL_CONTEXTUAL_SUGGESTIONS_CACHE_H_ diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestions_debugging_reporter.cc b/chromium/components/ntp_snippets/contextual/contextual_suggestions_debugging_reporter.cc index c3f9a48f2d5..f7a644f3411 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_suggestions_debugging_reporter.cc +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestions_debugging_reporter.cc @@ -23,6 +23,10 @@ ContextualSuggestionsDebuggingReporter::GetEvents() const { return events_; } +void ContextualSuggestionsDebuggingReporter::ClearEvents() { + events_.clear(); +} + void ContextualSuggestionsDebuggingReporter::SetupForPage( const std::string& url, ukm::SourceId source_id) { @@ -44,6 +48,9 @@ void ContextualSuggestionsDebuggingReporter::RecordEvent( case UI_PEEK_REVERSE_SCROLL: current_event_.sheet_peeked = true; return; + case UI_BUTTON_SHOWN: + current_event_.button_shown = true; + return; case UI_OPENED: current_event_.sheet_opened = true; return; @@ -70,10 +77,12 @@ void ContextualSuggestionsDebuggingReporter::Flush() { // Check if we've already sent an event with this url to the cache. If so, // remove it before adding another one. const std::string current_url = current_event_.url; - std::remove_if(events_.begin(), events_.end(), - [current_url](ContextualSuggestionsDebuggingEvent event) { - return current_url == event.url; - }); + auto itr = + std::remove_if(events_.begin(), events_.end(), + [current_url](ContextualSuggestionsDebuggingEvent event) { + return current_url == event.url; + }); + events_.erase(itr, events_.end()); events_.push_back(current_event_); // If the cache is too large, then remove the least recently used. @@ -81,4 +90,4 @@ void ContextualSuggestionsDebuggingReporter::Flush() { events_.erase(events_.begin()); } -} // namespace contextual_suggestions
\ No newline at end of file +} // namespace contextual_suggestions diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestions_debugging_reporter.h b/chromium/components/ntp_snippets/contextual/contextual_suggestions_debugging_reporter.h index 2baf1d91e41..585cf289458 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_suggestions_debugging_reporter.h +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestions_debugging_reporter.h @@ -35,6 +35,9 @@ struct ContextualSuggestionsDebuggingEvent { // Whether the sheet ever peeked. bool sheet_peeked = false; + + // Whether the button has even been shown. + bool button_shown = false; }; // Reporter specialized for caching information for debugging purposes. @@ -47,6 +50,9 @@ class ContextualSuggestionsDebuggingReporter // Get all events currently in the buffer. const std::vector<ContextualSuggestionsDebuggingEvent>& GetEvents() const; + // Clear the debugging cache of events. + void ClearEvents(); + // ContextualSuggestionsReporter void SetupForPage(const std::string& url, ukm::SourceId source_id) override; void RecordEvent( diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestions_features.cc b/chromium/components/ntp_snippets/contextual/contextual_suggestions_features.cc index 376e0afae5b..9321557f35a 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_suggestions_features.cc +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestions_features.cc @@ -9,11 +9,13 @@ namespace contextual_suggestions { const base::Feature kContextualSuggestionsBottomSheet{ "ContextualSuggestionsBottomSheet", base::FEATURE_DISABLED_BY_DEFAULT}; -const base::Feature kContextualSuggestionsEnterprisePolicyBypass{ - "ContextualSuggestionsEnterprisePolicyBypass", - base::FEATURE_DISABLED_BY_DEFAULT}; +const base::Feature kContextualSuggestionsButton{ + "ContextualSuggestionsButton", base::FEATURE_DISABLED_BY_DEFAULT}; const base::Feature kContextualSuggestionsSlimPeekUI{ "ContextualSuggestionsSlimPeekUI", base::FEATURE_DISABLED_BY_DEFAULT}; +const base::Feature kContextualSuggestionsOptOut{ + "ContextualSuggestionsOptOut", base::FEATURE_ENABLED_BY_DEFAULT}; + } // namespace contextual_suggestions diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestions_features.h b/chromium/components/ntp_snippets/contextual/contextual_suggestions_features.h index 9ca7f7a0e9e..11d28fac2a1 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_suggestions_features.h +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestions_features.h @@ -10,8 +10,9 @@ namespace contextual_suggestions { extern const base::Feature kContextualSuggestionsBottomSheet; -extern const base::Feature kContextualSuggestionsEnterprisePolicyBypass; +extern const base::Feature kContextualSuggestionsButton; extern const base::Feature kContextualSuggestionsSlimPeekUI; +extern const base::Feature kContextualSuggestionsOptOut; } // namespace contextual_suggestions diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestions_fetch.cc b/chromium/components/ntp_snippets/contextual/contextual_suggestions_fetch.cc index e4139f3bbb3..442509405c5 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_suggestions_fetch.cc +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestions_fetch.cc @@ -79,7 +79,7 @@ Cluster PivotToCluster(const PivotCluster& pivot) { return cluster_builder.Build(); } -PeekConditions PeekConditionsFromResponse( +PeekConditions GetPeekConditionsFromResponse( const GetPivotsResponse& response_proto) { AutoPeekConditions proto_conditions = response_proto.pivots().auto_peek_conditions(); @@ -95,7 +95,7 @@ PeekConditions PeekConditionsFromResponse( return peek_conditions; } -std::vector<Cluster> ClustersFromResponse( +std::vector<Cluster> GetClustersFromResponse( const GetPivotsResponse& response_proto) { std::vector<Cluster> clusters; Pivots pivots = response_proto.pivots(); @@ -122,7 +122,7 @@ std::vector<Cluster> ClustersFromResponse( return clusters; } -std::string PeekTextFromResponse(const GetPivotsResponse& response_proto) { +std::string GetPeekTextFromResponse(const GetPivotsResponse& response_proto) { return response_proto.pivots().peek_text().text(); } @@ -153,12 +153,26 @@ const std::string SerializedPivotsRequest(const std::string& url, return pivot_request.SerializeAsString(); } +ServerExperimentInfos GetServerExperimentInfosFromResponse( + const GetPivotsResponse& response_proto) { + ServerExperimentInfos field_trials; + for (auto experiment_info : response_proto.pivots().experiment_info()) { + std::string trial_name = experiment_info.experiment_group_name(); + std::string group_name = experiment_info.experiment_arm_name(); + if (!trial_name.empty() && !group_name.empty()) + field_trials.emplace_back(std::move(trial_name), std::move(group_name)); + } + + return field_trials; +} + ContextualSuggestionsResult ResultFromResponse( const GetPivotsResponse& response_proto) { return ContextualSuggestionsResult( - PeekTextFromResponse(response_proto), - ClustersFromResponse(response_proto), - PeekConditionsFromResponse(response_proto)); + GetPeekTextFromResponse(response_proto), + GetClustersFromResponse(response_proto), + GetPeekConditionsFromResponse(response_proto), + GetServerExperimentInfosFromResponse(response_proto)); } } // namespace @@ -177,6 +191,10 @@ const std::string ContextualSuggestionsFetch::GetFetchEndpoint() { fetch_endpoint = base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII( kFetchEndpointUrlKey); + } else if (base::FeatureList::IsEnabled( + contextual_suggestions::kContextualSuggestionsButton)) { + fetch_endpoint = base::GetFieldTrialParamValueByFeature( + kContextualSuggestionsButton, kFetchEndpointUrlKey); } else { fetch_endpoint = base::GetFieldTrialParamValueByFeature( kContextualSuggestionsBottomSheet, kFetchEndpointUrlKey); @@ -206,8 +224,6 @@ void ContextualSuggestionsFetch::Start( std::unique_ptr<network::SimpleURLLoader> ContextualSuggestionsFetch::MakeURLLoader() const { - // TODO(pnoland, https://crbug.com/831693): Update this once there's an - // opt-out setting. net::NetworkTrafficAnnotationTag traffic_annotation = net::DefineNetworkTrafficAnnotation("ntp_contextual_suggestions_fetch", R"( @@ -226,8 +242,8 @@ ContextualSuggestionsFetch::MakeURLLoader() const { policy { cookies_allowed: NO setting: - "This feature can be disabled by the flag " - "enable-contextual-suggestions-bottom-sheet." + "This feature can be disabled by turning off the " + "'Suggest related pages' in Chrome for Android settings" policy_exception_justification: "Not implemented. The feature is " "currently Android-only and disabled for all enterprise users. " "A policy will be added before enabling for enterprise users." @@ -274,11 +290,9 @@ void ContextualSuggestionsFetch::OnURLLoaderComplete( std::unique_ptr<std::string> result) { ContextualSuggestionsResult suggestions_result; - int32_t response_code = 0; - int32_t error_code = url_loader_->NetError(); if (result) { - response_code = url_loader_->ResponseInfo()->headers->response_code(); - + int32_t response_code = + url_loader_->ResponseInfo()->headers->response_code(); if (response_code == net::HTTP_OK) { // The response comes in the format (length, bytes) where length is a // varint32 encoded int. Rather than hand-rolling logic to skip the @@ -294,22 +308,38 @@ void ContextualSuggestionsFetch::OnURLLoaderComplete( } } } - - UMA_HISTOGRAM_COUNTS_1M("ContextualSuggestions.FetchResponseSizeKB", - static_cast<int>(result->length() / 1024)); } - ReportFetchMetrics(error_code, response_code, - suggestions_result.clusters.size(), + ReportFetchMetrics(suggestions_result.clusters.size(), std::move(metrics_callback)); std::move(request_completed_callback_).Run(std::move(suggestions_result)); } void ContextualSuggestionsFetch::ReportFetchMetrics( - int32_t error_code, - int32_t response_code, size_t clusters_size, ReportFetchMetricsCallback metrics_callback) { + int32_t error_code = url_loader_->NetError(); + int32_t response_code = 0; + + base::UmaHistogramSparse("ContextualSuggestions.FetchErrorCode", error_code); + if (error_code == net::OK) { + const network::ResourceResponseHead* response_info = + url_loader_->ResponseInfo(); + response_code = response_info->headers->response_code(); + if (response_code > 0) { + base::UmaHistogramSparse("ContextualSuggestions.FetchResponseCode", + response_code); + + UMA_HISTOGRAM_COUNTS_1M("ContextualSuggestions.FetchResponseNetworkBytes", + response_info->encoded_data_length); + } + + base::TimeDelta latency_delta = + response_info->response_time - response_info->request_time; + UMA_HISTOGRAM_COUNTS_1M("ContextualSuggestions.FetchLatencyMilliseconds", + latency_delta.InMilliseconds()); + } + ContextualSuggestionsEvent event; if (error_code != net::OK) { event = FETCH_ERROR; @@ -323,12 +353,6 @@ void ContextualSuggestionsFetch::ReportFetchMetrics( event = FETCH_COMPLETED; } - base::UmaHistogramSparse("ContextualSuggestions.FetchErrorCode", error_code); - if (response_code > 0) { - base::UmaHistogramSparse("ContextualSuggestions.FetchResponseCode", - response_code); - } - std::move(metrics_callback).Run(event); } diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestions_fetch.h b/chromium/components/ntp_snippets/contextual/contextual_suggestions_fetch.h index 4fb5acbff93..8e200040151 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_suggestions_fetch.h +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestions_fetch.h @@ -45,9 +45,7 @@ class ContextualSuggestionsFetch { net::HttpRequestHeaders MakeHeaders() const; void OnURLLoaderComplete(ReportFetchMetricsCallback metrics_callback, std::unique_ptr<std::string> result); - void ReportFetchMetrics(int32_t error_code, - int32_t response_code, - size_t clusters_size, + void ReportFetchMetrics(size_t clusters_size, ReportFetchMetricsCallback metrics_callback); // The url for which we're fetching suggestions. diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestions_fetcher_impl_unittest.cc b/chromium/components/ntp_snippets/contextual/contextual_suggestions_fetcher_impl_unittest.cc index 89f10c6156f..02a9487b009 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_suggestions_fetcher_impl_unittest.cc +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestions_fetcher_impl_unittest.cc @@ -12,7 +12,7 @@ #include "base/run_loop.h" #include "base/strings/string_number_conversions.h" #include "base/test/bind_test_util.h" -#include "base/test/histogram_tester.h" +#include "base/test/metrics/histogram_tester.h" #include "base/test/scoped_task_environment.h" #include "components/ntp_snippets/contextual/contextual_suggestions_result.h" #include "components/ntp_snippets/contextual/contextual_suggestions_test_utils.h" @@ -20,8 +20,8 @@ #include "components/ntp_snippets/contextual/proto/get_pivots_request.pb.h" #include "components/ntp_snippets/contextual/proto/get_pivots_response.pb.h" #include "net/http/http_status_code.h" -#include "services/network/public/cpp/shared_url_loader_factory.h" #include "services/network/public/cpp/url_loader_completion_status.h" +#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h" #include "services/network/test/test_url_loader_factory.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" @@ -51,41 +51,6 @@ using testing::ElementsAre; namespace { -// TODO(pnoland): de-dupe this and the identical class in -// feed_networking_host_unittest.cc -class TestSharedURLLoaderFactory : public SharedURLLoaderFactory { - public: - void CreateLoaderAndStart(network::mojom::URLLoaderRequest request, - int32_t routing_id, - int32_t request_id, - uint32_t options, - const network::ResourceRequest& url_request, - network::mojom::URLLoaderClientPtr client, - const net::MutableNetworkTrafficAnnotationTag& - traffic_annotation) override { - test_factory_.CreateLoaderAndStart(std::move(request), routing_id, - request_id, options, url_request, - std::move(client), traffic_annotation); - } - - void Clone(network::mojom::URLLoaderFactoryRequest request) override { - NOTREACHED(); - } - - std::unique_ptr<network::SharedURLLoaderFactoryInfo> Clone() override { - NOTREACHED(); - return nullptr; - } - - TestURLLoaderFactory* test_factory() { return &test_factory_; } - - protected: - ~TestSharedURLLoaderFactory() override = default; - - private: - TestURLLoaderFactory test_factory_; -}; - Cluster DefaultCluster() { return ClusterBuilder("Articles") .AddSuggestion(SuggestionBuilder(GURL("http://www.foobar.com")) @@ -132,7 +97,8 @@ void PopulatePeekConditions(AutoPeekConditions* proto_conditions, std::string SerializedResponseProto( const std::string& peek_text, std::vector<Cluster> clusters, - PeekConditions peek_conditions = PeekConditions()) { + PeekConditions peek_conditions = PeekConditions(), + ServerExperimentInfos experiment_infos = ServerExperimentInfos()) { GetPivotsResponse response_proto; Pivots* pivots = response_proto.mutable_pivots(); PopulatePeekConditions(pivots->mutable_auto_peek_conditions(), @@ -149,6 +115,12 @@ std::string SerializedResponseProto( } } + for (const auto& experiment_info : experiment_infos) { + ExperimentInfo* experiment = pivots->add_experiment_info(); + experiment->set_experiment_group_name(experiment_info.name); + experiment->set_experiment_arm_name(experiment_info.group); + } + // The fetch parsing logic expects the response to come as (length, bytes) // where length is varint32 encoded, but ignores the actual length read. // " " is a valid varint32(32) so this works for now. @@ -178,9 +150,11 @@ std::string SerializedResponseProto(const std::string& peek_text, class ContextualSuggestionsFetcherTest : public testing::Test { public: ContextualSuggestionsFetcherTest() { - loader_factory_ = base::MakeRefCounted<TestSharedURLLoaderFactory>(); + shared_url_loader_factory_ = + base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>( + &test_factory_); fetcher_ = std::make_unique<ContextualSuggestionsFetcherImpl>( - loader_factory_, "en"); + shared_url_loader_factory_, "en"); } ~ContextualSuggestionsFetcherTest() override {} @@ -197,8 +171,7 @@ class ContextualSuggestionsFetcherTest : public testing::Test { status.decoded_body_length = response_data.length(); } - loader_factory_->test_factory()->AddResponse(fetch_url, head, response_data, - status); + test_factory_.AddResponse(fetch_url, head, response_data, status); } void SendAndAwaitResponse( @@ -217,13 +190,12 @@ class ContextualSuggestionsFetcherTest : public testing::Test { ContextualSuggestionsFetcher& fetcher() { return *fetcher_; } - TestURLLoaderFactory* test_factory() { - return loader_factory_->test_factory(); - } + TestURLLoaderFactory* test_factory() { return &test_factory_; } private: base::test::ScopedTaskEnvironment scoped_task_environment_; - scoped_refptr<TestSharedURLLoaderFactory> loader_factory_; + network::TestURLLoaderFactory test_factory_; + scoped_refptr<network::SharedURLLoaderFactory> shared_url_loader_factory_; std::unique_ptr<ContextualSuggestionsFetcherImpl> fetcher_; DISALLOW_COPY_AND_ASSIGN(ContextualSuggestionsFetcherTest); @@ -238,9 +210,10 @@ TEST_F(ContextualSuggestionsFetcherTest, SingleSuggestionResponse) { &metrics_callback); EXPECT_TRUE(callback.has_run); - ExpectResponsesMatch(std::move(callback), - ContextualSuggestionsResult( - "Peek Text", DefaultClusters(), PeekConditions())); + ExpectResponsesMatch( + std::move(callback), + ContextualSuggestionsResult("Peek Text", DefaultClusters(), + PeekConditions(), ServerExperimentInfos())); EXPECT_EQ(metrics_callback.events, std::vector<ContextualSuggestionsEvent>( {contextual_suggestions::FETCH_COMPLETED})); @@ -304,10 +277,12 @@ TEST_F(ContextualSuggestionsFetcherTest, ExpectResponsesMatch( std::move(callback), ContextualSuggestionsResult("Peek Text", std::move(clusters_copy), - PeekConditions())); + PeekConditions(), ServerExperimentInfos())); - histogram_tester.ExpectTotalCount("ContextualSuggestions.FetchResponseSizeKB", - 1); + histogram_tester.ExpectTotalCount( + "ContextualSuggestions.FetchResponseNetworkBytes", 1); + histogram_tester.ExpectTotalCount( + "ContextualSuggestions.FetchLatencyMilliseconds", 1); } TEST_F(ContextualSuggestionsFetcherTest, FlatResponse) { @@ -323,7 +298,7 @@ TEST_F(ContextualSuggestionsFetcherTest, FlatResponse) { ExpectResponsesMatch( std::move(callback), ContextualSuggestionsResult("Peek Text", std::move(expected_clusters), - PeekConditions())); + PeekConditions(), ServerExperimentInfos())); } TEST_F(ContextualSuggestionsFetcherTest, PeekConditionsAreParsed) { @@ -342,9 +317,32 @@ TEST_F(ContextualSuggestionsFetcherTest, PeekConditionsAreParsed) { &metrics_callback); EXPECT_TRUE(callback.has_run); - ExpectResponsesMatch(std::move(callback), - ContextualSuggestionsResult( - "Peek Text", DefaultClusters(), peek_conditions)); + ExpectResponsesMatch( + std::move(callback), + ContextualSuggestionsResult("Peek Text", DefaultClusters(), + peek_conditions, ServerExperimentInfos())); +} + +TEST_F(ContextualSuggestionsFetcherTest, ServerExperimentInfosAreParsed) { + MockClustersCallback callback; + MockMetricsCallback metrics_callback; + ServerExperimentInfos experiment_infos; + experiment_infos.emplace_back("trial1", "group1"); + experiment_infos.emplace_back("trial2", "group2"); + SetFakeResponse(SerializedResponseProto("Peek Text", DefaultClusters(), + PeekConditions(), experiment_infos)); + + SendAndAwaitResponse(GURL("http://www.article.com"), &callback, + &metrics_callback); + + EXPECT_TRUE(callback.has_run); + ExpectResponsesMatch( + std::move(callback), + ContextualSuggestionsResult("Peek Text", DefaultClusters(), + PeekConditions(), experiment_infos)); + EXPECT_EQ(metrics_callback.events, + std::vector<ContextualSuggestionsEvent>( + {contextual_suggestions::FETCH_COMPLETED})); } TEST_F(ContextualSuggestionsFetcherTest, HtmlEntitiesAreUnescaped) { @@ -373,7 +371,7 @@ TEST_F(ContextualSuggestionsFetcherTest, HtmlEntitiesAreUnescaped) { ExpectResponsesMatch( std::move(callback), ContextualSuggestionsResult("Peek Text", std::move(expected_clusters), - PeekConditions())); + PeekConditions(), ServerExperimentInfos())); } TEST_F(ContextualSuggestionsFetcherTest, RequestHeaderSetCorrectly) { @@ -510,9 +508,10 @@ TEST_F(ContextualSuggestionsFetcherTest, ResponseWithUnsetFields) { EXPECT_TRUE(callback.has_run); EXPECT_EQ(callback.response_clusters.size(), 1u); - ExpectResponsesMatch(std::move(callback), - ContextualSuggestionsResult( - "", std::move(expected_clusters), PeekConditions())); + ExpectResponsesMatch( + std::move(callback), + ContextualSuggestionsResult("", std::move(expected_clusters), + PeekConditions(), ServerExperimentInfos())); } TEST_F(ContextualSuggestionsFetcherTest, CorruptResponse) { diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestions_metrics_reporter.cc b/chromium/components/ntp_snippets/contextual/contextual_suggestions_metrics_reporter.cc index e588db611a0..08b4fe7abc6 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_suggestions_metrics_reporter.cc +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestions_metrics_reporter.cc @@ -14,6 +14,7 @@ namespace contextual_suggestions { ContextualSuggestionsMetricsReporter::ContextualSuggestionsMetricsReporter() : sheet_peeked_(false), + button_shown_(false), sheet_opened_(false), sheet_closed_(false), any_suggestion_downloaded_(false), @@ -64,6 +65,11 @@ void ContextualSuggestionsMetricsReporter::RecordUmaMetrics( return; sheet_peeked_ = true; break; + case UI_BUTTON_SHOWN: + if (button_shown_) + return; + button_shown_ = true; + break; case UI_OPENED: if (sheet_opened_) return; @@ -97,6 +103,7 @@ void ContextualSuggestionsMetricsReporter::RecordUmaMetrics( void ContextualSuggestionsMetricsReporter::ResetUma() { sheet_peeked_ = false; + button_shown_ = false; sheet_opened_ = false; sheet_closed_ = false; any_suggestion_downloaded_ = false; diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestions_metrics_reporter.h b/chromium/components/ntp_snippets/contextual/contextual_suggestions_metrics_reporter.h index 0cd8bff2ed4..5a6ca75d6f0 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_suggestions_metrics_reporter.h +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestions_metrics_reporter.h @@ -43,6 +43,8 @@ class ContextualSuggestionsMetricsReporter // Internal UMA state data. // Whether the sheet ever peeked. bool sheet_peeked_; + // Whether the button was ever shown. + bool button_shown_; // Whether the sheet was ever opened. bool sheet_opened_; // Whether the sheet was closed. diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestions_metrics_reporter_unittest.cc b/chromium/components/ntp_snippets/contextual/contextual_suggestions_metrics_reporter_unittest.cc index ddf6e47fd9f..99512b43519 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_suggestions_metrics_reporter_unittest.cc +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestions_metrics_reporter_unittest.cc @@ -3,8 +3,9 @@ // found in the LICENSE file. #include "components/ntp_snippets/contextual/contextual_suggestions_metrics_reporter.h" + #include "base/macros.h" -#include "base/test/histogram_tester.h" +#include "base/test/metrics/histogram_tester.h" #include "base/test/scoped_task_environment.h" #include "components/ntp_snippets/contextual/contextual_suggestions_ukm_entry.h" #include "components/ukm/test_ukm_recorder.h" @@ -35,6 +36,7 @@ const int kSuggestionDownloaded = 11; const int kSuggestionClicked = 12; const int kUiDismissedWithoutOpen = 13; const int kUiDismissedAfterOpen = 14; +const int kUiButtonShown = 15; } // namespace class ContextualSuggestionsMetricsReporterTest : public ::testing::Test { @@ -196,4 +198,51 @@ TEST_F(ContextualSuggestionsMetricsReporterTest, EnumNotReorderedTest) { GetReporter().Flush(); } +TEST_F(ContextualSuggestionsMetricsReporterTest, ButtonTest) { + base::HistogramTester histogram_tester; + GetReporter().SetupForPage(kTestNavigationUrl, GetSourceId()); + GetReporter().RecordEvent(FETCH_REQUESTED); + GetReporter().RecordEvent(FETCH_COMPLETED); + GetReporter().RecordEvent(UI_BUTTON_SHOWN); + GetReporter().RecordEvent(UI_OPENED); + GetReporter().RecordEvent(SUGGESTION_DOWNLOADED); + GetReporter().RecordEvent(SUGGESTION_CLICKED); + // Flush data to write to UKM. + GetReporter().Flush(); + // Check that we wrote something to UKM. Details of UKM reporting are tested + // in a different test suite. + TestUkmRecorder* test_ukm_recorder = GetTestUkmRecorder(); + std::vector<const ukm::mojom::UkmEntry*> entry_vector = + test_ukm_recorder->GetEntriesByName(ContextualSuggestions::kEntryName); + EXPECT_EQ(1U, entry_vector.size()); + const ukm::mojom::UkmEntry* first_entry = entry_vector[0]; + EXPECT_TRUE(test_ukm_recorder->EntryHasMetric( + first_entry, ContextualSuggestions::kFetchStateName)); + EXPECT_EQ(static_cast<int64_t>(FetchState::COMPLETED), + *(test_ukm_recorder->GetEntryMetric( + first_entry, ContextualSuggestions::kFetchStateName))); + // Test that the expected histogram events were written. + histogram_tester.ExpectBucketCount(kEventsHistogramName, kUninitialized, 0); + histogram_tester.ExpectBucketCount(kEventsHistogramName, kFetchDelayed, 0); + histogram_tester.ExpectBucketCount(kEventsHistogramName, kFetchRequested, 1); + histogram_tester.ExpectBucketCount(kEventsHistogramName, kFetchError, 0); + histogram_tester.ExpectBucketCount(kEventsHistogramName, kFetchServerBusy, 0); + histogram_tester.ExpectBucketCount(kEventsHistogramName, kFetchBelowThreshold, + 0); + histogram_tester.ExpectBucketCount(kEventsHistogramName, kFetchEmpty, 0); + histogram_tester.ExpectBucketCount(kEventsHistogramName, kFetchCompleted, 1); + histogram_tester.ExpectBucketCount(kEventsHistogramName, kUiButtonShown, 1); + histogram_tester.ExpectBucketCount(kEventsHistogramName, kUiOpened, 1); + histogram_tester.ExpectBucketCount(kEventsHistogramName, kUiClosedObsolete, + 0); + histogram_tester.ExpectBucketCount(kEventsHistogramName, + kSuggestionDownloaded, 1); + histogram_tester.ExpectBucketCount(kEventsHistogramName, kSuggestionClicked, + 1); + histogram_tester.ExpectBucketCount(kEventsHistogramName, + kUiDismissedWithoutOpen, 0); + histogram_tester.ExpectBucketCount(kEventsHistogramName, + kUiDismissedAfterOpen, 0); +} + } // namespace contextual_suggestions diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestions_reporter.cc b/chromium/components/ntp_snippets/contextual/contextual_suggestions_reporter.cc index 12ca2fbbf88..ec68d6fe992 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_suggestions_reporter.cc +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestions_reporter.cc @@ -28,6 +28,11 @@ ContextualSuggestionsReporterProvider::CreateReporter() { return reporter; } +ContextualSuggestionsDebuggingReporter* +ContextualSuggestionsReporterProvider::GetDebuggingReporter() { + return debugging_reporter_.get(); +} + ContextualSuggestionsReporter::ContextualSuggestionsReporter() = default; ContextualSuggestionsReporter::~ContextualSuggestionsReporter() = default; diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestions_reporter.h b/chromium/components/ntp_snippets/contextual/contextual_suggestions_reporter.h index b57e6e04466..2c01ea5a215 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_suggestions_reporter.h +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestions_reporter.h @@ -29,6 +29,8 @@ class ContextualSuggestionsReporterProvider { virtual std::unique_ptr<ContextualSuggestionsReporter> CreateReporter(); + virtual ContextualSuggestionsDebuggingReporter* GetDebuggingReporter(); + private: // The debugging reporter is shared between instances of top-level reporter. // Since multiple objects need a reference to this, it's kept as a unique @@ -84,9 +86,11 @@ enum ContextualSuggestionsEvent { // The UI was dismissed after having been opened. This means the sheet was // closed from any position after it was expanded at least once. UI_DISMISSED_AFTER_OPEN = 14, + // The UI button was shown to the user. + UI_BUTTON_SHOWN = 15, // Special name that marks the maximum value in an Enum used for UMA. // https://cs.chromium.org/chromium/src/tools/metrics/histograms/README.md. - kMaxValue = UI_DISMISSED_AFTER_OPEN, + kMaxValue = UI_BUTTON_SHOWN, }; // Tracks various metrics based on reports of events that take place diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestions_result.cc b/chromium/components/ntp_snippets/contextual/contextual_suggestions_result.cc index a562a865467..1033fb5c80f 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_suggestions_result.cc +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestions_result.cc @@ -16,16 +16,18 @@ PeekConditions::PeekConditions() Cluster::Cluster() = default; +Cluster::Cluster(const Cluster& other) = default; + // MSVC doesn't support defaulted move constructors, so we have to define it // ourselves. Cluster::Cluster(Cluster&& other) noexcept : title(std::move(other.title)), suggestions(std::move(other.suggestions)) {} -Cluster::Cluster(const Cluster&) = default; - Cluster::~Cluster() = default; +Cluster& Cluster::operator=(const Cluster&) = default; + ClusterBuilder::ClusterBuilder(const std::string& title) { cluster_.title = title; } @@ -53,15 +55,37 @@ Cluster ClusterBuilder::Build() { return std::move(cluster_); } +ServerExperimentInfo::ServerExperimentInfo() = default; + +ServerExperimentInfo::ServerExperimentInfo(std::string name, std::string group) + : name(name), group(group) {} + +ServerExperimentInfo::ServerExperimentInfo(const ServerExperimentInfo& other) = + default; + +ServerExperimentInfo::ServerExperimentInfo( + ServerExperimentInfo&& other) noexcept + : name(std::move(other.name)), group(std::move(other.group)) {} + +ServerExperimentInfo::~ServerExperimentInfo() = default; + +ServerExperimentInfo& ServerExperimentInfo::operator=( + ServerExperimentInfo&& other) = default; + +ServerExperimentInfo& ServerExperimentInfo::operator=( + const ServerExperimentInfo& other) = default; + ContextualSuggestionsResult::ContextualSuggestionsResult() = default; ContextualSuggestionsResult::ContextualSuggestionsResult( std::string peek_text, std::vector<Cluster> clusters, - PeekConditions peek_conditions) + PeekConditions peek_conditions, + ServerExperimentInfos experiment_infos) : clusters(clusters), peek_text(peek_text), - peek_conditions(peek_conditions) {} + peek_conditions(peek_conditions), + experiment_infos(std::move(experiment_infos)) {} ContextualSuggestionsResult::ContextualSuggestionsResult( const ContextualSuggestionsResult& other) = default; @@ -72,11 +96,15 @@ ContextualSuggestionsResult::ContextualSuggestionsResult( ContextualSuggestionsResult&& other) noexcept : clusters(std::move(other.clusters)), peek_text(std::move(other.peek_text)), - peek_conditions(std::move(other.peek_conditions)) {} + peek_conditions(std::move(other.peek_conditions)), + experiment_infos(std::move(other.experiment_infos)) {} ContextualSuggestionsResult::~ContextualSuggestionsResult() = default; ContextualSuggestionsResult& ContextualSuggestionsResult::operator=( ContextualSuggestionsResult&& other) = default; +ContextualSuggestionsResult& ContextualSuggestionsResult::operator=( + const ContextualSuggestionsResult& other) = default; + } // namespace contextual_suggestions diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestions_result.h b/chromium/components/ntp_snippets/contextual/contextual_suggestions_result.h index 6dadf379a21..b140c319f03 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_suggestions_result.h +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestions_result.h @@ -7,6 +7,12 @@ #include "components/ntp_snippets/contextual/contextual_suggestion.h" +#include <string> +#include <utility> +#include <vector> + +#include "base/containers/flat_map.h" + namespace contextual_suggestions { // Encapsulates conditions under which to show or "peek" the contextual @@ -36,6 +42,8 @@ struct Cluster { Cluster(Cluster&&) noexcept; ~Cluster(); + Cluster& operator=(const Cluster&); + std::string title; std::vector<ContextualSuggestion> suggestions; }; @@ -43,7 +51,7 @@ struct Cluster { // Allows concise construction of a cluster. class ClusterBuilder { public: - ClusterBuilder(const std::string& title); + explicit ClusterBuilder(const std::string& title); // Allow copying for ease of validation when testing. ClusterBuilder(const ClusterBuilder& other); @@ -54,22 +62,42 @@ class ClusterBuilder { Cluster cluster_; }; +// Synthetic field trials driven by the server. +struct ServerExperimentInfo { + ServerExperimentInfo(); + ServerExperimentInfo(std::string name, std::string group); + ServerExperimentInfo(const ServerExperimentInfo&); + ServerExperimentInfo(ServerExperimentInfo&&) noexcept; + ~ServerExperimentInfo(); + + ServerExperimentInfo& operator=(ServerExperimentInfo&&); + ServerExperimentInfo& operator=(const ServerExperimentInfo&); + + std::string name; + std::string group; +}; + +using ServerExperimentInfos = std::vector<ServerExperimentInfo>; + // Struct that holds the data from a ContextualSuggestions response that we care // about for UI purposes. struct ContextualSuggestionsResult { ContextualSuggestionsResult(); ContextualSuggestionsResult(std::string peek_text, std::vector<Cluster> clusters, - PeekConditions peek_conditions); + PeekConditions peek_conditions, + ServerExperimentInfos experiment_infos); ContextualSuggestionsResult(const ContextualSuggestionsResult&); ContextualSuggestionsResult(ContextualSuggestionsResult&&) noexcept; ~ContextualSuggestionsResult(); ContextualSuggestionsResult& operator=(ContextualSuggestionsResult&&); + ContextualSuggestionsResult& operator=(const ContextualSuggestionsResult&); std::vector<Cluster> clusters; std::string peek_text; PeekConditions peek_conditions; + ServerExperimentInfos experiment_infos; }; using FetchClustersCallback = @@ -77,4 +105,4 @@ using FetchClustersCallback = } // namespace contextual_suggestions -#endif // COMPONENTS_NTP_SNIPPETS_CONTEXTUAL_CONTEXTUAL_SUGGESTIONS_RESULT_H_
\ No newline at end of file +#endif // COMPONENTS_NTP_SNIPPETS_CONTEXTUAL_CONTEXTUAL_SUGGESTIONS_RESULT_H_ diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestions_test_utils.cc b/chromium/components/ntp_snippets/contextual/contextual_suggestions_test_utils.cc index 69002ca4e87..bfccb0fffe8 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_suggestions_test_utils.cc +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestions_test_utils.cc @@ -19,6 +19,7 @@ void MockClustersCallback::Done(ContextualSuggestionsResult result) { response_peek_text = result.peek_text; response_clusters = std::move(result.clusters); peek_conditions = result.peek_conditions; + experiment_infos = result.experiment_infos; } FetchClustersCallback MockClustersCallback::ToOnceCallback() { @@ -46,7 +47,7 @@ void ExpectSuggestionsMatch(const ContextualSuggestion& actual, void ExpectClustersMatch(const Cluster& actual, const Cluster& expected) { EXPECT_EQ(actual.title, expected.title); ASSERT_EQ(actual.suggestions.size(), expected.suggestions.size()); - for (size_t i = 0; i < actual.suggestions.size(); i++) { + for (size_t i = 0; i < actual.suggestions.size(); ++i) { ExpectSuggestionsMatch(std::move(actual.suggestions[i]), std::move(expected.suggestions[i])); } @@ -60,16 +61,27 @@ void ExpectPeekConditionsMatch(const PeekConditions& actual, EXPECT_EQ(actual.maximum_number_of_peeks, expected.maximum_number_of_peeks); } +void ExpectServerExperimentInfosMatch(const ServerExperimentInfos& actual, + const ServerExperimentInfos& expected) { + ASSERT_EQ(expected.size(), actual.size()); + for (size_t i = 0; i < actual.size(); ++i) { + EXPECT_EQ(expected[i].name, actual[i].name); + EXPECT_EQ(expected[i].group, actual[i].group); + } +} + void ExpectResponsesMatch(const MockClustersCallback& callback, const ContextualSuggestionsResult& expected_result) { EXPECT_EQ(callback.response_peek_text, expected_result.peek_text); ExpectPeekConditionsMatch(callback.peek_conditions, expected_result.peek_conditions); ASSERT_EQ(callback.response_clusters.size(), expected_result.clusters.size()); - for (size_t i = 0; i < callback.response_clusters.size(); i++) { + for (size_t i = 0; i < callback.response_clusters.size(); ++i) { ExpectClustersMatch(std::move(callback.response_clusters[i]), std::move(expected_result.clusters[i])); } + ExpectServerExperimentInfosMatch(callback.experiment_infos, + expected_result.experiment_infos); } } // namespace contextual_suggestions diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestions_test_utils.h b/chromium/components/ntp_snippets/contextual/contextual_suggestions_test_utils.h index b0265d2310e..a94d2838db8 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_suggestions_test_utils.h +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestions_test_utils.h @@ -27,6 +27,7 @@ class MockClustersCallback { std::string response_peek_text; std::vector<Cluster> response_clusters; PeekConditions peek_conditions; + ServerExperimentInfos experiment_infos; }; class MockMetricsCallback { diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestions_ukm_entry.cc b/chromium/components/ntp_snippets/contextual/contextual_suggestions_ukm_entry.cc index d06920c6157..1771fdcabeb 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_suggestions_ukm_entry.cc +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestions_ukm_entry.cc @@ -66,6 +66,10 @@ void ContextualSuggestionsUkmEntry::RecordEventMetrics( trigger_event_ = TriggerEvent::REVERSE_SCROLL; StartTimerIfNeeded(); break; + case UI_BUTTON_SHOWN: + trigger_event_ = TriggerEvent::TOOLBAR_BUTTON; + StartTimerIfNeeded(); + break; case UI_OPENED: was_sheet_opened_ = true; StartTimerIfNeeded(); diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestions_ukm_entry.h b/chromium/components/ntp_snippets/contextual/contextual_suggestions_ukm_entry.h index c340419a95b..f632dc2e2ac 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_suggestions_ukm_entry.h +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestions_ukm_entry.h @@ -35,6 +35,7 @@ enum class FetchState { enum class TriggerEvent { NEVER_SHOWN, REVERSE_SCROLL, + TOOLBAR_BUTTON, }; // Writes a single UKM entry that describes the latest state of the event stream diff --git a/chromium/components/ntp_snippets/contextual/contextual_suggestions_ukm_entry_unittest.cc b/chromium/components/ntp_snippets/contextual/contextual_suggestions_ukm_entry_unittest.cc index 8af8952196d..b711e58c9e4 100644 --- a/chromium/components/ntp_snippets/contextual/contextual_suggestions_ukm_entry_unittest.cc +++ b/chromium/components/ntp_snippets/contextual/contextual_suggestions_ukm_entry_unittest.cc @@ -104,4 +104,25 @@ TEST_F(ContextualSuggestionsUkmEntryTest, ExpectedOperationTest) { EXPECT_EQ(1, GetEntryMetric(ContextualSuggestions::kTriggerEventName)); } +TEST_F(ContextualSuggestionsUkmEntryTest, ExpectedOperationButtonTest) { + ukm_entry_->RecordEventMetrics(FETCH_DELAYED); + ukm_entry_->RecordEventMetrics(FETCH_REQUESTED); + ukm_entry_->RecordEventMetrics(FETCH_COMPLETED); + ukm_entry_->RecordEventMetrics(UI_BUTTON_SHOWN); + ukm_entry_->RecordEventMetrics(UI_OPENED); + ukm_entry_->RecordEventMetrics(SUGGESTION_DOWNLOADED); + ukm_entry_->RecordEventMetrics(SUGGESTION_DOWNLOADED); + ukm_entry_->RecordEventMetrics(SUGGESTION_CLICKED); + ukm_entry_->Flush(); + EXPECT_EQ(1, GetEntryMetric(ContextualSuggestions::kAnyDownloadedName)); + EXPECT_EQ(1, GetEntryMetric(ContextualSuggestions::kAnySuggestionTakenName)); + EXPECT_EQ(0, GetEntryMetric(ContextualSuggestions::kClosedFromPeekName)); + EXPECT_EQ(1, GetEntryMetric(ContextualSuggestions::kEverOpenedName)); + EXPECT_EQ(static_cast<int64_t>(FetchState::COMPLETED), + GetEntryMetric(ContextualSuggestions::kFetchStateName)); + EXPECT_LT(0, + GetEntryMetric(ContextualSuggestions::kShowDurationBucketMinName)); + EXPECT_EQ(2, GetEntryMetric(ContextualSuggestions::kTriggerEventName)); +} + } // namespace contextual_suggestions diff --git a/chromium/components/ntp_snippets/contextual/proto/get_pivots_response.proto b/chromium/components/ntp_snippets/contextual/proto/get_pivots_response.proto index 72b54f6bc16..79c8548517a 100644 --- a/chromium/components/ntp_snippets/contextual/proto/get_pivots_response.proto +++ b/chromium/components/ntp_snippets/contextual/proto/get_pivots_response.proto @@ -23,6 +23,9 @@ message Pivots { // The text to present to the user in the peek. optional PeekText peek_text = 4; + + // Information about experiments running on this request. + repeated ExperimentInfo experiment_info = 5; } // One related response. @@ -139,3 +142,19 @@ message RasterImage { // is determined by the "Content-Type" of the HTTP response for the URL. optional Url url = 2; } + +// Synthetic experiment IDs, which Chrome may log in UMA. +message ExperimentInfo { + // Name of experiment. A single live-experiment will have one + // experiment_group_name, and two or more experiment_arm_name's. + // + // This corresponds to an experiment group in GWS, and a field trial in Chrome + // Variations. + optional string experiment_group_name = 1; + + // Name of experiment arm. + // + // This corresponds to an experiment arm in GWS, and a field trial group in + // Chrome Variations. + optional string experiment_arm_name = 2; +} |