Commit 884ac281 authored by gaschler's avatar gaschler Committed by Commit Bot

Replace RemoteSuggestion by ContextualSuggestion in contextual/.

Adds a class ContextualSuggestion that replaces all uses
of RemoteSuggestion in the ContextualContentSuggestionsService.
Contextual suggestions do not fit well into RemoteSuggestion 
because they do not have categories, and their behavior is
expected to diverge further in future.
Also, caching non-copyable RemoteSuggestion would be
inconvenient.

Bug: 749988
Change-Id: I71b7a4630efcb6c8e1a69dd8cbb670b6d9fb72ed
Reviewed-on: https://chromium-review.googlesource.com/689996
Commit-Queue: Andre Gaschler <gaschler@chromium.org>
Reviewed-by: default avatarvitaliii <vitaliii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505055}
parent 26ae2586
...@@ -48,6 +48,8 @@ static_library("ntp_snippets") { ...@@ -48,6 +48,8 @@ static_library("ntp_snippets") {
"contextual/contextual_content_suggestions_service.h", "contextual/contextual_content_suggestions_service.h",
"contextual/contextual_json_request.cc", "contextual/contextual_json_request.cc",
"contextual/contextual_json_request.h", "contextual/contextual_json_request.h",
"contextual/contextual_suggestion.cc",
"contextual/contextual_suggestion.h",
"contextual/contextual_suggestions_fetcher.h", "contextual/contextual_suggestions_fetcher.h",
"contextual/contextual_suggestions_fetcher_impl.cc", "contextual/contextual_suggestions_fetcher_impl.cc",
"contextual/contextual_suggestions_fetcher_impl.h", "contextual/contextual_suggestions_fetcher_impl.h",
......
...@@ -66,10 +66,9 @@ void ContextualContentSuggestionsService::DidFetchContextualSuggestions( ...@@ -66,10 +66,9 @@ void ContextualContentSuggestionsService::DidFetchContextualSuggestions(
ContextualSuggestionsFetcher::OptionalSuggestions fetched_suggestions) { ContextualSuggestionsFetcher::OptionalSuggestions fetched_suggestions) {
std::vector<ContentSuggestion> suggestions; std::vector<ContentSuggestion> suggestions;
if (fetched_suggestions.has_value()) { if (fetched_suggestions.has_value()) {
for (const std::unique_ptr<RemoteSuggestion>& suggestion : for (const std::unique_ptr<ContextualSuggestion>& suggestion :
fetched_suggestions.value()) { fetched_suggestions.value()) {
suggestions.emplace_back(suggestion->ToContentSuggestion( suggestions.emplace_back(suggestion->ToContentSuggestion());
Category::FromKnownCategory(KnownCategories::CONTEXTUAL)));
ContentSuggestion::ID id = suggestions.back().id(); ContentSuggestion::ID id = suggestions.back().id();
GURL image_url = suggestion->salient_image_url(); GURL image_url = suggestion->salient_image_url();
image_url_by_id_[id.id_within_category()] = image_url; image_url_by_id_[id.id_within_category()] = image_url;
......
...@@ -17,11 +17,10 @@ ...@@ -17,11 +17,10 @@
#include "components/image_fetcher/core/image_fetcher_impl.h" #include "components/image_fetcher/core/image_fetcher_impl.h"
#include "components/ntp_snippets/category_info.h" #include "components/ntp_snippets/category_info.h"
#include "components/ntp_snippets/content_suggestion.h" #include "components/ntp_snippets/content_suggestion.h"
#include "components/ntp_snippets/contextual/contextual_suggestion.h"
#include "components/ntp_snippets/contextual/contextual_suggestions_fetcher.h" #include "components/ntp_snippets/contextual/contextual_suggestions_fetcher.h"
#include "components/ntp_snippets/remote/cached_image_fetcher.h" #include "components/ntp_snippets/remote/cached_image_fetcher.h"
#include "components/ntp_snippets/remote/json_to_categories.h" #include "components/ntp_snippets/remote/json_to_categories.h"
#include "components/ntp_snippets/remote/remote_suggestion.h"
#include "components/ntp_snippets/remote/remote_suggestion_builder.h"
#include "components/ntp_snippets/remote/remote_suggestions_database.h" #include "components/ntp_snippets/remote/remote_suggestions_database.h"
#include "components/prefs/pref_registry_simple.h" #include "components/prefs/pref_registry_simple.h"
#include "components/prefs/testing_pref_service.h" #include "components/prefs/testing_pref_service.h"
...@@ -149,14 +148,12 @@ TEST_F(ContextualContentSuggestionsServiceTest, ...@@ -149,14 +148,12 @@ TEST_F(ContextualContentSuggestionsServiceTest,
MockFetchContextualSuggestionsCallback mock_suggestions_callback; MockFetchContextualSuggestionsCallback mock_suggestions_callback;
const std::string kValidFromUrl = "http://some.url"; const std::string kValidFromUrl = "http://some.url";
const std::string kToUrl = "http://another.url"; const std::string kToUrl = "http://another.url";
ContextualSuggestionsFetcher::OptionalSuggestions remote_suggestions = ContextualSuggestionsFetcher::OptionalSuggestions contextual_suggestions =
RemoteSuggestion::PtrVector(); ContextualSuggestion::PtrVector();
remote_suggestions->push_back(test::RemoteSuggestionBuilder() contextual_suggestions->push_back(
.AddId(kToUrl) ContextualSuggestion::CreateForTesting(kToUrl, ""));
.SetUrl(kToUrl) fetcher()->SetFakeResponse(Status::Success(),
.SetAmpUrl(kToUrl) std::move(contextual_suggestions));
.Build());
fetcher()->SetFakeResponse(Status::Success(), std::move(remote_suggestions));
EXPECT_CALL(mock_suggestions_callback, EXPECT_CALL(mock_suggestions_callback,
Run(Property(&Status::IsSuccess, true), GURL(kValidFromUrl), Run(Property(&Status::IsSuccess, true), GURL(kValidFromUrl),
Pointee(ElementsAre(AllOf( Pointee(ElementsAre(AllOf(
...@@ -174,7 +171,8 @@ TEST_F(ContextualContentSuggestionsServiceTest, ...@@ -174,7 +171,8 @@ TEST_F(ContextualContentSuggestionsServiceTest,
ShouldRunCallbackOnEmptyResults) { ShouldRunCallbackOnEmptyResults) {
MockFetchContextualSuggestionsCallback mock_suggestions_callback; MockFetchContextualSuggestionsCallback mock_suggestions_callback;
const std::string kEmpty; const std::string kEmpty;
fetcher()->SetFakeResponse(Status::Success(), RemoteSuggestion::PtrVector()); fetcher()->SetFakeResponse(Status::Success(),
ContextualSuggestion::PtrVector());
EXPECT_CALL(mock_suggestions_callback, Run(Property(&Status::IsSuccess, true), EXPECT_CALL(mock_suggestions_callback, Run(Property(&Status::IsSuccess, true),
GURL(kEmpty), Pointee(IsEmpty()))); GURL(kEmpty), Pointee(IsEmpty())));
source()->FetchContextualSuggestions( source()->FetchContextualSuggestions(
...@@ -186,7 +184,7 @@ TEST_F(ContextualContentSuggestionsServiceTest, ShouldRunCallbackOnError) { ...@@ -186,7 +184,7 @@ TEST_F(ContextualContentSuggestionsServiceTest, ShouldRunCallbackOnError) {
MockFetchContextualSuggestionsCallback mock_suggestions_callback; MockFetchContextualSuggestionsCallback mock_suggestions_callback;
const std::string kEmpty; const std::string kEmpty;
fetcher()->SetFakeResponse(Status(StatusCode::TEMPORARY_ERROR, ""), fetcher()->SetFakeResponse(Status(StatusCode::TEMPORARY_ERROR, ""),
RemoteSuggestion::PtrVector()); ContextualSuggestion::PtrVector());
EXPECT_CALL(mock_suggestions_callback, EXPECT_CALL(mock_suggestions_callback,
Run(Property(&Status::IsSuccess, false), GURL(kEmpty), Run(Property(&Status::IsSuccess, false), GURL(kEmpty),
Pointee(IsEmpty()))); Pointee(IsEmpty())));
...@@ -215,15 +213,12 @@ TEST_F(ContextualContentSuggestionsServiceTest, ...@@ -215,15 +213,12 @@ TEST_F(ContextualContentSuggestionsServiceTest,
const std::string kValidFromUrl = "http://some.url"; const std::string kValidFromUrl = "http://some.url";
const std::string kToUrl = "http://another.url"; const std::string kToUrl = "http://another.url";
const std::string kValidImageUrl = "http://some.url/image.png"; const std::string kValidImageUrl = "http://some.url/image.png";
ContextualSuggestionsFetcher::OptionalSuggestions remote_suggestions = ContextualSuggestionsFetcher::OptionalSuggestions contextual_suggestions =
RemoteSuggestion::PtrVector(); ContextualSuggestion::PtrVector();
remote_suggestions->push_back(test::RemoteSuggestionBuilder() contextual_suggestions->push_back(
.AddId(kToUrl) ContextualSuggestion::CreateForTesting(kToUrl, kValidImageUrl));
.SetUrl(kToUrl) fetcher()->SetFakeResponse(Status::Success(),
.SetAmpUrl(kToUrl) std::move(contextual_suggestions));
.SetImageUrl(kValidImageUrl)
.Build());
fetcher()->SetFakeResponse(Status::Success(), std::move(remote_suggestions));
MockFetchContextualSuggestionsCallback mock_suggestions_callback; MockFetchContextualSuggestionsCallback mock_suggestions_callback;
std::vector<ContentSuggestion> suggestions; std::vector<ContentSuggestion> suggestions;
EXPECT_CALL(mock_suggestions_callback, Run(_, _, _)) EXPECT_CALL(mock_suggestions_callback, Run(_, _, _))
......
// Copyright 2017 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_suggestion.h"
#include "base/memory/ptr_util.h"
#include "base/strings/string_number_conversions.h"
#include "base/strings/stringprintf.h"
#include "base/strings/utf_string_conversions.h"
#include "base/values.h"
#include "components/ntp_snippets/category.h"
namespace {
// dict.Get() specialization for base::Time values
bool GetTimeValue(const base::DictionaryValue& dict,
const std::string& key,
base::Time* time) {
// TODO(gaschler): Replace all usages of GetString(key, &str) by
// FindKey(key)->GetString().
std::string time_value;
return dict.GetString(key, &time_value) &&
base::Time::FromString(time_value.c_str(), time);
}
// dict.Get() specialization for GURL values
bool GetURLValue(const base::DictionaryValue& dict,
const std::string& key,
GURL* url) {
std::string spec;
if (!dict.GetString(key, &spec)) {
return false;
}
*url = GURL(spec);
return url->is_valid();
}
} // namespace
namespace ntp_snippets {
ContextualSuggestion::ContextualSuggestion(const std::string& id) : id_(id) {}
ContextualSuggestion::~ContextualSuggestion() = default;
// static
std::unique_ptr<ContextualSuggestion>
ContextualSuggestion::CreateFromDictionary(const base::DictionaryValue& dict) {
std::string id;
if (!dict.GetString("url", &id) || id.empty()) {
return nullptr;
}
auto suggestion = MakeUnique(id);
GetURLValue(dict, "url", &suggestion->url_);
if (!dict.GetString("title", &suggestion->title_)) {
dict.GetString("source", &suggestion->title_);
}
dict.GetString("snippet", &suggestion->snippet_);
GetTimeValue(dict, "creationTime", &suggestion->publish_date_);
GetURLValue(dict, "imageUrl", &suggestion->salient_image_url_);
if (!dict.GetString("attribution", &suggestion->publisher_name_)) {
dict.GetString("source", &suggestion->publisher_name_);
}
return suggestion;
}
ContentSuggestion ContextualSuggestion::ToContentSuggestion() const {
ContentSuggestion suggestion(
Category::FromKnownCategory(KnownCategories::CONTEXTUAL), id_, url_);
suggestion.set_title(base::UTF8ToUTF16(title_));
suggestion.set_snippet_text(base::UTF8ToUTF16(snippet_));
suggestion.set_publish_date(publish_date_);
suggestion.set_publisher_name(base::UTF8ToUTF16(publisher_name_));
return suggestion;
}
// static
std::unique_ptr<ContextualSuggestion> ContextualSuggestion::CreateForTesting(
const std::string& to_url,
const std::string& image_url) {
auto suggestion = MakeUnique({to_url});
suggestion->url_ = GURL(to_url);
suggestion->salient_image_url_ = GURL(image_url);
return suggestion;
}
// static
std::unique_ptr<ContextualSuggestion> ContextualSuggestion::MakeUnique(
const std::string& id) {
return base::WrapUnique(new ContextualSuggestion(id));
}
} // namespace ntp_snippets
// Copyright 2017 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_SUGGESTION_H_
#define COMPONENTS_NTP_SNIPPETS_CONTEXTUAL_CONTEXTUAL_SUGGESTION_H_
#include <cstdint>
#include <memory>
#include <string>
#include <vector>
#include "base/macros.h"
#include "base/optional.h"
#include "base/time/time.h"
#include "components/ntp_snippets/content_suggestion.h"
#include "url/gurl.h"
namespace base {
class DictionaryValue;
} // namespace base
namespace ntp_snippets {
class ContextualSuggestion {
public:
using PtrVector = std::vector<std::unique_ptr<ContextualSuggestion>>;
~ContextualSuggestion();
// Creates a ContextualSuggestion from a dictionary. Returns a null pointer if
// the dictionary doesn't correspond to a valid suggestion.
static std::unique_ptr<ContextualSuggestion> CreateFromDictionary(
const base::DictionaryValue& dict);
// Converts to general content suggestion form.
ContentSuggestion ToContentSuggestion() const;
// The ID for identifying the suggestion.
const std::string& id() const { return id_; }
// Title of the suggestion.
const std::string& title() const { return title_; }
// The main URL pointing to the content web page.
const GURL& url() const { return url_; }
// The name of the content's publisher.
const std::string& publisher_name() const { return publisher_name_; }
// Summary or relevant extract from the content.
const std::string& snippet() const { return snippet_; }
// Link to an image representative of the content. Do not fetch directly,
// but through the service, which uses caching, to avoid multiple
// network requests.
const GURL& salient_image_url() const { return salient_image_url_; }
static std::unique_ptr<ContextualSuggestion> CreateForTesting(
const std::string& to_url,
const std::string& image_url);
private:
ContextualSuggestion(const std::string& id);
// base::MakeUnique doesn't work if the ctor is private.
static std::unique_ptr<ContextualSuggestion> MakeUnique(
const std::string& id);
std::string id_;
std::string title_;
GURL url_;
std::string publisher_name_;
GURL salient_image_url_;
std::string snippet_;
base::Time publish_date_;
};
} // namespace ntp_snippets
#endif // COMPONENTS_NTP_SNIPPETS_CONTEXTUAL_CONTEXTUAL_SUGGESTION_H_
...@@ -13,7 +13,7 @@ ...@@ -13,7 +13,7 @@
#include "base/optional.h" #include "base/optional.h"
#include "components/ntp_snippets/category.h" #include "components/ntp_snippets/category.h"
#include "components/ntp_snippets/category_info.h" #include "components/ntp_snippets/category_info.h"
#include "components/ntp_snippets/remote/remote_suggestion.h" #include "components/ntp_snippets/contextual/contextual_suggestion.h"
#include "components/ntp_snippets/status.h" #include "components/ntp_snippets/status.h"
namespace ntp_snippets { namespace ntp_snippets {
...@@ -21,7 +21,7 @@ namespace ntp_snippets { ...@@ -21,7 +21,7 @@ namespace ntp_snippets {
// Fetches contextual suggestions from the server. // Fetches contextual suggestions from the server.
class ContextualSuggestionsFetcher { class ContextualSuggestionsFetcher {
public: public:
using OptionalSuggestions = base::Optional<RemoteSuggestion::PtrVector>; using OptionalSuggestions = base::Optional<ContextualSuggestion::PtrVector>;
// If fetching fails, the optional will be empty. // If fetching fails, the optional will be empty.
using SuggestionsAvailableCallback = using SuggestionsAvailableCallback =
......
...@@ -89,18 +89,20 @@ std::string GetFetchEndpoint() { ...@@ -89,18 +89,20 @@ std::string GetFetchEndpoint() {
// |suggestions|. Returns true on success, false if anything went wrong. // |suggestions|. Returns true on success, false if anything went wrong.
bool AddSuggestionsFromListValue(bool content_suggestions_api, bool AddSuggestionsFromListValue(bool content_suggestions_api,
const base::ListValue& list, const base::ListValue& list,
RemoteSuggestion::PtrVector* suggestions) { ContextualSuggestion::PtrVector* suggestions) {
for (const auto& value : list) { for (const auto& value : list) {
const base::DictionaryValue* dict = nullptr; const base::DictionaryValue* dict = nullptr;
if (!value.GetAsDictionary(&dict)) { if (!value.GetAsDictionary(&dict)) {
return false; return false;
} }
std::string s; if (DLOG_IS_ON(INFO)) {
dict->GetAsString(&s); std::string s;
DVLOG(1) << "AddSuggestionsFromListValue " << s; dict->GetAsString(&s);
std::unique_ptr<RemoteSuggestion> suggestion = DVLOG(1) << "AddSuggestionsFromListValue " << s;
RemoteSuggestion::CreateFromContextualSuggestionsDictionary(*dict); }
std::unique_ptr<ContextualSuggestion> suggestion =
ContextualSuggestion::CreateFromDictionary(*dict);
suggestions->push_back(std::move(suggestion)); suggestions->push_back(std::move(suggestion));
} }
return true; return true;
...@@ -242,7 +244,7 @@ void ContextualSuggestionsFetcherImpl::JsonRequestDone( ...@@ -242,7 +244,7 @@ void ContextualSuggestionsFetcherImpl::JsonRequestDone(
return; return;
} }
OptionalSuggestions optional_suggestions = RemoteSuggestion::PtrVector(); OptionalSuggestions optional_suggestions = ContextualSuggestion::PtrVector();
if (!JsonToSuggestions(*result, &optional_suggestions.value())) { if (!JsonToSuggestions(*result, &optional_suggestions.value())) {
DLOG(WARNING) << "Received invalid suggestions: " << last_fetch_json_; DLOG(WARNING) << "Received invalid suggestions: " << last_fetch_json_;
FetchFinished(OptionalSuggestions(), std::move(callback), FetchFinished(OptionalSuggestions(), std::move(callback),
...@@ -272,7 +274,7 @@ void ContextualSuggestionsFetcherImpl::FetchFinished( ...@@ -272,7 +274,7 @@ void ContextualSuggestionsFetcherImpl::FetchFinished(
bool ContextualSuggestionsFetcherImpl::JsonToSuggestions( bool ContextualSuggestionsFetcherImpl::JsonToSuggestions(
const base::Value& parsed, const base::Value& parsed,
RemoteSuggestion::PtrVector* suggestions) { ContextualSuggestion::PtrVector* suggestions) {
const base::DictionaryValue* top_dict = nullptr; const base::DictionaryValue* top_dict = nullptr;
if (!parsed.GetAsDictionary(&top_dict)) { if (!parsed.GetAsDictionary(&top_dict)) {
return false; return false;
......
...@@ -16,8 +16,8 @@ ...@@ -16,8 +16,8 @@
#include "components/ntp_snippets/category.h" #include "components/ntp_snippets/category.h"
#include "components/ntp_snippets/category_info.h" #include "components/ntp_snippets/category_info.h"
#include "components/ntp_snippets/contextual/contextual_json_request.h" #include "components/ntp_snippets/contextual/contextual_json_request.h"
#include "components/ntp_snippets/contextual/contextual_suggestion.h"
#include "components/ntp_snippets/contextual/contextual_suggestions_fetcher.h" #include "components/ntp_snippets/contextual/contextual_suggestions_fetcher.h"
#include "components/ntp_snippets/remote/remote_suggestion.h"
#include "components/ntp_snippets/status.h" #include "components/ntp_snippets/status.h"
#include "net/url_request/url_request_context_getter.h" #include "net/url_request/url_request_context_getter.h"
...@@ -71,7 +71,7 @@ class ContextualSuggestionsFetcherImpl : public ContextualSuggestionsFetcher { ...@@ -71,7 +71,7 @@ class ContextualSuggestionsFetcherImpl : public ContextualSuggestionsFetcher {
const std::string& error_details); const std::string& error_details);
bool JsonToSuggestions(const base::Value& parsed, bool JsonToSuggestions(const base::Value& parsed,
RemoteSuggestion::PtrVector* suggestions); ContextualSuggestion::PtrVector* suggestions);
// Authentication for signed-in users. // Authentication for signed-in users.
SigninManagerBase* signin_manager_; SigninManagerBase* signin_manager_;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment