Skip to content

Commit

Permalink
[EoC] Replace legacy fetching with skeleton
Browse files Browse the repository at this point in the history
Replace JsonFetcher with ContextualSuggestionsFetch.
Add no-op/null implementations of methods.
Move Cluster definition to its own file.
Make ContextualSuggestion a struct, and give it a builder.
Adjust bridge to properly use new ContextualSuggestion struct.
Add deps on services/network and content/public.

R:fgorski@chromium.org, mmenke@chromium.org

Bug: 807697
Change-Id: I6a41937a3ae934283f57a8516e2017531adb063d
Reviewed-on: https://chromium-review.googlesource.com/1000174
Commit-Queue: Filip Gorski <fgorski@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550099}
  • Loading branch information
Patrick Noland authored and Commit Bot committed Apr 12, 2018
1 parent 09fa83a commit ed66cd5
Show file tree
Hide file tree
Showing 25 changed files with 399 additions and 1,455 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ using base::android::ConvertUTF8ToJavaString;
using base::android::JavaParamRef;
using base::android::ScopedJavaGlobalRef;
using base::android::ScopedJavaLocalRef;
using Cluster = ntp_snippets::ContextualContentSuggestionsService::Cluster;
using Cluster = ntp_snippets::Cluster;

namespace contextual_suggestions {

Expand Down Expand Up @@ -154,10 +154,10 @@ void ContextualSuggestionsBridge::OnSuggestionsAvailable(
env, j_result, ConvertUTF8ToJavaString(env, cluster.title));
for (auto& suggestion : cluster.suggestions) {
Java_ContextualSuggestionsBridge_addSuggestionToLastCluster(
env, j_result, ConvertUTF8ToJavaString(env, suggestion->id()),
ConvertUTF8ToJavaString(env, suggestion->title()),
ConvertUTF8ToJavaString(env, suggestion->publisher_name()),
ConvertUTF8ToJavaString(env, suggestion->url().spec()));
env, j_result, ConvertUTF8ToJavaString(env, suggestion.id),
ConvertUTF8ToJavaString(env, suggestion.title),
ConvertUTF8ToJavaString(env, suggestion.publisher_name),
ConvertUTF8ToJavaString(env, suggestion.url.spec()));
}
}
RunCallbackAndroid(j_callback, j_result);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,7 @@ class ContextualSuggestionsBridge {
void OnSuggestionsAvailable(
base::android::ScopedJavaGlobalRef<jobject> j_callback,
std::string peek_text,
std::vector<ntp_snippets::ContextualContentSuggestionsService::Cluster>
clusters);
std::vector<ntp_snippets::Cluster> clusters);

void OnImageFetched(base::android::ScopedJavaGlobalRef<jobject> j_callback,
const gfx::Image& image);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@
#include "chrome/browser/ntp_snippets/contextual_content_suggestions_service_factory.h"

#include "base/files/file_path.h"
#include "base/memory/ptr_util.h"
#include "base/memory/singleton.h"
#include "build/build_config.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/search/suggestions/image_decoder_impl.h"
#include "chrome/browser/signin/identity_manager_factory.h"
#include "components/image_fetcher/core/image_decoder.h"
#include "components/image_fetcher/core/image_fetcher.h"
#include "components/image_fetcher/core/image_fetcher_impl.h"
Expand All @@ -20,7 +22,9 @@
#include "components/ntp_snippets/remote/remote_suggestions_database.h"
#include "components/prefs/pref_service.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/storage_partition.h"
#include "content/public/common/service_manager_connection.h"
#include "net/url_request/url_request_context_getter.h"
#include "services/data_decoder/public/cpp/safe_json_parser.h"

#if defined(OS_ANDROID)
Expand Down Expand Up @@ -70,7 +74,6 @@ ContextualContentSuggestionsServiceFactory::
: BrowserContextKeyedServiceFactory(
"ContextualContentSuggestionsService",
BrowserContextDependencyManager::GetInstance()) {
DependsOn(IdentityManagerFactory::GetInstance());
}

ContextualContentSuggestionsServiceFactory::
Expand All @@ -86,16 +89,14 @@ ContextualContentSuggestionsServiceFactory::BuildServiceInstanceFor(
}

PrefService* pref_service = profile->GetPrefs();
identity::IdentityManager* identity_manager =
IdentityManagerFactory::GetForProfile(profile);
scoped_refptr<net::URLRequestContextGetter> request_context =
profile->GetRequestContext();
content::StoragePartition* storage_partition =
content::BrowserContext::GetDefaultStoragePartition(context);
auto contextual_suggestions_fetcher =
std::make_unique<ContextualSuggestionsFetcherImpl>(
identity_manager, request_context, pref_service,
base::Bind(&data_decoder::SafeJsonParser::Parse,
content::ServiceManagerConnection::GetForProcess()
->GetConnector()));
storage_partition->GetURLLoaderFactoryForBrowserProcess(),
g_browser_process->GetApplicationLocale());
const base::FilePath::CharType kDatabaseFolder[] =
FILE_PATH_LITERAL("contextualSuggestionsDatabase");
base::FilePath database_dir(profile->GetPath().Append(kDatabaseFolder));
Expand Down
12 changes: 9 additions & 3 deletions components/ntp_snippets/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,16 @@ static_library("ntp_snippets") {
"content_suggestions_provider.h",
"content_suggestions_service.cc",
"content_suggestions_service.h",
"contextual/cluster.cc",
"contextual/cluster.h",
"contextual/contextual_content_suggestions_service.cc",
"contextual/contextual_content_suggestions_service.h",
"contextual/contextual_content_suggestions_service_proxy.cc",
"contextual/contextual_content_suggestions_service_proxy.h",
"contextual/contextual_json_request.cc",
"contextual/contextual_json_request.h",
"contextual/contextual_suggestion.cc",
"contextual/contextual_suggestion.h",
"contextual/contextual_suggestions_fetch.cc",
"contextual/contextual_suggestions_fetch.h",
"contextual/contextual_suggestions_fetcher.h",
"contextual/contextual_suggestions_fetcher_impl.cc",
"contextual/contextual_suggestions_fetcher_impl.h",
Expand Down Expand Up @@ -172,6 +174,8 @@ static_library("ntp_snippets") {
"//components/web_resource",
"//services/identity/public/cpp",
"//services/metrics/public/cpp:metrics_cpp",
"//services/network/public/cpp",
"//services/network/public/mojom",
"//third_party/icu/",
"//ui/gfx",
]
Expand Down Expand Up @@ -211,7 +215,6 @@ source_set("unit_tests") {
"content_suggestions_service_unittest.cc",
"contextual/contextual_content_suggestions_service_proxy_unittest.cc",
"contextual/contextual_content_suggestions_service_unittest.cc",
"contextual/contextual_json_request_unittest.cc",
"contextual/contextual_suggestions_fetcher_impl_unittest.cc",
"contextual/contextual_suggestions_metrics_reporter_unittest.cc",
"contextual/contextual_suggestions_ukm_entry_unittest.cc",
Expand Down Expand Up @@ -275,6 +278,9 @@ source_set("unit_tests") {
"//net:test_support",
"//services/identity/public/cpp",
"//services/identity/public/cpp:test_support",
"//services/network:test_support",
"//services/network/public/cpp",
"//services/network/public/mojom",
"//testing/gtest",
"//third_party/icu/",
"//ui/gfx:test_support",
Expand Down
3 changes: 3 additions & 0 deletions components/ntp_snippets/DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ include_rules = [
"+google_apis",
"+services/identity/public/cpp",
"+services/metrics/public/cpp",
"+services/network/public/cpp",
"+services/network/public/mojom",
"+services/network/test",
"+ui/base",
"+ui/gfx",
]
46 changes: 46 additions & 0 deletions components/ntp_snippets/contextual/cluster.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// 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/cluster.h"

#include "components/ntp_snippets/contextual/contextual_suggestion.h"

namespace contextual_suggestions {

Cluster::Cluster() = 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() = default;

ClusterBuilder::ClusterBuilder(const std::string& title) {
cluster_.title = title;
}

ClusterBuilder::ClusterBuilder(const ClusterBuilder& other) {
cluster_.title = other.cluster_.title;
for (const auto& suggestion : other.cluster_.suggestions) {
AddSuggestion(SuggestionBuilder(suggestion.url)
.Title(suggestion.title)
.PublisherName(suggestion.publisher_name)
.Snippet(suggestion.snippet)
.ImageId(suggestion.image_id)
.Build());
}
}

ClusterBuilder& ClusterBuilder::AddSuggestion(ContextualSuggestion suggestion) {
cluster_.suggestions.emplace_back(std::move(suggestion));
return *this;
}

Cluster ClusterBuilder::Build() {
return std::move(cluster_);
}

} // namespace contextual_suggestions
49 changes: 49 additions & 0 deletions components/ntp_snippets/contextual/cluster.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// 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_CLUSTER_H_
#define COMPONENTS_NTP_SNIPPETS_CONTEXTUAL_CLUSTER_H_

#include "components/ntp_snippets/contextual/contextual_suggestion.h"

using ntp_snippets::ContextualSuggestion;
using ntp_snippets::SuggestionBuilder;

namespace contextual_suggestions {

// A structure representing a suggestion cluster.
struct Cluster {
Cluster();
Cluster(Cluster&&) noexcept;
~Cluster();

std::string title;
std::vector<ContextualSuggestion> suggestions;

private:
DISALLOW_COPY_AND_ASSIGN(Cluster);
};

// Allows concise construction of a cluster and copying, which cluster
// doesn't allow.
class ClusterBuilder {
public:
ClusterBuilder(const std::string& title);

// Allow copying for ease of validation when testing.
ClusterBuilder(const ClusterBuilder& other);
ClusterBuilder& AddSuggestion(ContextualSuggestion suggestion);
Cluster Build();

private:
Cluster cluster_;
};

using FetchClustersCallback =
base::OnceCallback<void(std::string peek_text,
std::vector<Cluster> clusters)>;

} // namespace contextual_suggestions

#endif // COMPONENTS_NTP_SNIPPETS_CONTEXTUAL_CLUSTER_H_
Original file line number Diff line number Diff line change
Expand Up @@ -10,28 +10,18 @@
#include <utility>

#include "base/bind.h"
#include "base/memory/ptr_util.h"
#include "components/ntp_snippets/contextual/cluster.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 "ui/gfx/image/image.h"

namespace ntp_snippets {
namespace {
static const char kSamplePeekText[] = "Peek text";
static const char kSampleClusterTitle[] = "Cluster title filler";
} // namespace

using contextual_suggestions::ContextualSuggestionsMetricsReporter;

ContextualContentSuggestionsService::Cluster::Cluster() = default;

ContextualContentSuggestionsService::Cluster::Cluster(const std::string& title)
: title(title) {}

ContextualContentSuggestionsService::Cluster::Cluster(Cluster&& other) =
default;

ContextualContentSuggestionsService::Cluster::~Cluster() = default;
using contextual_suggestions::Cluster;
using contextual_suggestions::FetchClustersCallback;

ContextualContentSuggestionsService::ContextualContentSuggestionsService(
std::unique_ptr<ContextualSuggestionsFetcher>
Expand All @@ -50,24 +40,16 @@ ContextualContentSuggestionsService::ContextualContentSuggestionsService(
ContextualContentSuggestionsService::~ContextualContentSuggestionsService() =
default;

// TODO(pnoland): remove this
void ContextualContentSuggestionsService::FetchContextualSuggestions(
const GURL& url,
FetchContextualSuggestionsCallback callback) {
contextual_suggestions_fetcher_->FetchContextualSuggestions(
url,
base::BindOnce(
&ContextualContentSuggestionsService::DidFetchContextualSuggestions,
base::Unretained(this), url, std::move(callback)));
}
FetchContextualSuggestionsCallback callback) {}

void ContextualContentSuggestionsService::FetchContextualSuggestionClusters(
const GURL& url,
FetchContextualSuggestionClustersCallback callback) {
// TODO(pnoland): Fetch suggestions using the new fetcher.
contextual_suggestions_fetcher_->FetchContextualSuggestions(
url, base::BindOnce(&ContextualContentSuggestionsService::
DidFetchContextualSuggestionsClusterWrapper,
base::Unretained(this), std::move(callback)));
FetchClustersCallback callback) {
contextual_suggestions_fetcher_->FetchContextualSuggestionsClusters(
url, std::move(callback));
}

void ContextualContentSuggestionsService::FetchContextualSuggestionImage(
Expand Down Expand Up @@ -111,40 +93,6 @@ void ContextualContentSuggestionsService::ReportEvent(
metrics_reporter_->RecordEvent(event);
}

// TODO(gaschler): Cache contextual suggestions at run-time.
void ContextualContentSuggestionsService::DidFetchContextualSuggestions(
const GURL& url,
FetchContextualSuggestionsCallback callback,
Status status,
ContextualSuggestionsFetcher::OptionalSuggestions fetched_suggestions) {
std::vector<ContentSuggestion> suggestions;
if (fetched_suggestions.has_value()) {
for (const std::unique_ptr<ContextualSuggestion>& suggestion :
fetched_suggestions.value()) {
suggestions.emplace_back(suggestion->ToContentSuggestion());
ContentSuggestion::ID id = suggestions.back().id();
GURL image_url = suggestion->salient_image_url();
image_url_by_id_[id.id_within_category()] = image_url;
}
}
std::move(callback).Run(status, url, std::move(suggestions));
}

void ContextualContentSuggestionsService::
DidFetchContextualSuggestionsClusterWrapper(
FetchContextualSuggestionClustersCallback callback,
Status status,
ContextualSuggestionsFetcher::OptionalSuggestions fetched_suggestions) {
std::vector<Cluster> clusters;
if (fetched_suggestions.has_value()) {
Cluster cluster;
cluster.title = kSampleClusterTitle;
cluster.suggestions = std::move(fetched_suggestions.value());
clusters.push_back(std::move(cluster));
}
std::move(callback).Run(kSamplePeekText, std::move(clusters));
}

void ContextualContentSuggestionsService::Shutdown() {
if (last_ukm_source_id_ != ukm::kInvalidSourceId)
metrics_reporter_->Flush();
Expand Down
Loading

0 comments on commit ed66cd5

Please sign in to comment.