Skip to content

Commit

Permalink
[NTP::SectionOrder] Replace CategoryFactory with a category ranker.
Browse files Browse the repository at this point in the history
This CL:
1) Adds CategoryRanker interface;
2) Adds a default constant category ranker implementation (based on
   CategoryFactory);
3) Replaces CategoryFactory with a new ranker;
4) Factory methods are moved to Category.

This is the initial step of moving to personalised section order.

BUG=673659

Review-Url: https://codereview.chromium.org/2568033005
Cr-Commit-Position: refs/heads/master@{#439451}
  • Loading branch information
vitaliii authored and Commit bot committed Dec 19, 2016
1 parent 216a94b commit 7456f5a
Show file tree
Hide file tree
Showing 43 changed files with 764 additions and 578 deletions.
43 changes: 20 additions & 23 deletions chrome/browser/android/ntp/ntp_snippets_bridge.cc
Original file line number Diff line number Diff line change
Expand Up @@ -173,12 +173,8 @@ static void OnSuggestionTargetVisited(JNIEnv* env,
const JavaParamRef<jclass>& caller,
jint j_category_id,
jlong visit_time_ms) {
Profile* profile = ProfileManager::GetLastUsedProfile();
ntp_snippets::ContentSuggestionsService* content_suggestions_service =
ContentSuggestionsServiceFactory::GetForProfile(profile);
ntp_snippets::metrics::OnSuggestionTargetVisited(
content_suggestions_service->category_factory()->FromIDValue(
j_category_id),
Category::FromIDValue(j_category_id),
base::TimeDelta::FromMilliseconds(visit_time_ms));
}

Expand Down Expand Up @@ -218,7 +214,7 @@ int NTPSnippetsBridge::GetCategoryStatus(JNIEnv* env,
const JavaParamRef<jobject>& obj,
jint j_category_id) {
return static_cast<int>(content_suggestions_service_->GetCategoryStatus(
CategoryFromIDValue(j_category_id)));
Category::FromIDValue(j_category_id)));
}

base::android::ScopedJavaLocalRef<jobject> NTPSnippetsBridge::GetCategoryInfo(
Expand All @@ -227,7 +223,7 @@ base::android::ScopedJavaLocalRef<jobject> NTPSnippetsBridge::GetCategoryInfo(
jint j_category_id) {
base::Optional<CategoryInfo> info =
content_suggestions_service_->GetCategoryInfo(
CategoryFromIDValue(j_category_id));
Category::FromIDValue(j_category_id));
if (!info) {
return base::android::ScopedJavaLocalRef<jobject>(env, nullptr);
}
Expand All @@ -243,7 +239,7 @@ ScopedJavaLocalRef<jobject> NTPSnippetsBridge::GetSuggestionsForCategory(
JNIEnv* env,
const base::android::JavaParamRef<jobject>& obj,
jint j_category_id) {
Category category = CategoryFromIDValue(j_category_id);
Category category = Category::FromIDValue(j_category_id);
return ToJavaSuggestionList(
env, category,
content_suggestions_service_->GetSuggestionsForCategory(category));
Expand All @@ -257,7 +253,7 @@ void NTPSnippetsBridge::FetchSuggestionImage(
const JavaParamRef<jobject>& j_callback) {
base::android::ScopedJavaGlobalRef<jobject> callback(j_callback);
content_suggestions_service_->FetchSuggestionImage(
ContentSuggestion::ID(CategoryFromIDValue(j_category_id),
ContentSuggestion::ID(Category::FromIDValue(j_category_id),
ConvertJavaStringToUTF8(env, id_within_category)),
base::Bind(&NTPSnippetsBridge::OnImageFetched,
weak_ptr_factory_.GetWeakPtr(), callback));
Expand All @@ -272,7 +268,7 @@ void NTPSnippetsBridge::Fetch(
AppendJavaStringArrayToStringVector(env, j_displayed_suggestions,
&known_suggestion_ids);

Category category = CategoryFromIDValue(j_category_id);
Category category = Category::FromIDValue(j_category_id);
content_suggestions_service_->Fetch(
category, std::set<std::string>(known_suggestion_ids.begin(),
known_suggestion_ids.end()),
Expand All @@ -288,7 +284,7 @@ void NTPSnippetsBridge::DismissSuggestion(
jint j_category_id,
jint category_position,
const JavaParamRef<jstring>& id_within_category) {
Category category = CategoryFromIDValue(j_category_id);
Category category = Category::FromIDValue(j_category_id);

content_suggestions_service_->DismissSuggestion(ContentSuggestion::ID(
category, ConvertJavaStringToUTF8(env, id_within_category)));
Expand All @@ -310,7 +306,7 @@ void NTPSnippetsBridge::DismissSuggestion(
void NTPSnippetsBridge::DismissCategory(JNIEnv* env,
const JavaParamRef<jobject>& obj,
jint j_category_id) {
Category category = CategoryFromIDValue(j_category_id);
Category category = Category::FromIDValue(j_category_id);

content_suggestions_service_->DismissCategory(category);

Expand All @@ -337,7 +333,7 @@ void NTPSnippetsBridge::OnPageShown(
std::vector<std::pair<Category, int>> suggestions_per_category;
for (size_t i = 0; i < categories_int.size(); i++) {
suggestions_per_category.push_back(
std::make_pair(CategoryFromIDValue(categories_int[i]),
std::make_pair(Category::FromIDValue(categories_int[i]),
suggestions_per_category_int[i]));
}
ntp_snippets::metrics::OnPageShown(suggestions_per_category);
Expand All @@ -358,7 +354,7 @@ void NTPSnippetsBridge::OnSuggestionShown(JNIEnv* env,
ntp_snippets::prefs::kLastSuccessfulBackgroundFetchTime));

ntp_snippets::metrics::OnSuggestionShown(
global_position, CategoryFromIDValue(j_category_id), category_position,
global_position, Category::FromIDValue(j_category_id), category_position,
base::Time::FromJavaTime(publish_timestamp_ms),
last_background_fetch_time, score);
if (global_position == 0) {
Expand All @@ -376,9 +372,14 @@ void NTPSnippetsBridge::OnSuggestionOpened(JNIEnv* env,
jfloat score,
int windowOpenDisposition) {
ntp_snippets::metrics::OnSuggestionOpened(
global_position, CategoryFromIDValue(j_category_id), category_position,
global_position, Category::FromIDValue(j_category_id), category_position,
base::Time::FromJavaTime(publish_timestamp_ms), score,
static_cast<WindowOpenDisposition>(windowOpenDisposition));
// TODO(vitaliii): Add ContentSuggestionsService::OnSuggestionOpened and
// notify the ranker and the classifier there instead. Do not expose both of
// them at all. See crbug.com/674080.
content_suggestions_service_->category_ranker()->OnSuggestionOpened(
Category::FromIDValue(j_category_id));
content_suggestions_service_->user_classifier()->OnEvent(
ntp_snippets::UserClassifier::Metric::SUGGESTIONS_USED);
}
Expand All @@ -391,24 +392,24 @@ void NTPSnippetsBridge::OnSuggestionMenuOpened(JNIEnv* env,
jlong publish_timestamp_ms,
jfloat score) {
ntp_snippets::metrics::OnSuggestionMenuOpened(
global_position, CategoryFromIDValue(j_category_id), category_position,
global_position, Category::FromIDValue(j_category_id), category_position,
base::Time::FromJavaTime(publish_timestamp_ms), score);
}

void NTPSnippetsBridge::OnMoreButtonShown(JNIEnv* env,
const JavaParamRef<jobject>& obj,
jint j_category_id,
jint position) {
ntp_snippets::metrics::OnMoreButtonShown(CategoryFromIDValue(j_category_id),
ntp_snippets::metrics::OnMoreButtonShown(Category::FromIDValue(j_category_id),
position);
}

void NTPSnippetsBridge::OnMoreButtonClicked(JNIEnv* env,
const JavaParamRef<jobject>& obj,
jint j_category_id,
jint position) {
ntp_snippets::metrics::OnMoreButtonClicked(CategoryFromIDValue(j_category_id),
position);
ntp_snippets::metrics::OnMoreButtonClicked(
Category::FromIDValue(j_category_id), position);
content_suggestions_service_->user_classifier()->OnEvent(
ntp_snippets::UserClassifier::Metric::SUGGESTIONS_USED);
}
Expand Down Expand Up @@ -483,10 +484,6 @@ void NTPSnippetsBridge::OnSuggestionsFetched(
ToJavaSuggestionList(env, category, suggestions));
}

Category NTPSnippetsBridge::CategoryFromIDValue(jint id) {
return content_suggestions_service_->category_factory()->FromIDValue(id);
}

// static
bool NTPSnippetsBridge::Register(JNIEnv* env) {
return RegisterNativesImpl(env);
Expand Down
2 changes: 0 additions & 2 deletions chrome/browser/android/ntp/ntp_snippets_bridge.h
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,6 @@ class NTPSnippetsBridge
ntp_snippets::Status status,
std::vector<ntp_snippets::ContentSuggestion> suggestions);

ntp_snippets::Category CategoryFromIDValue(jint id);

ntp_snippets::ContentSuggestionsService* content_suggestions_service_;
history::HistoryService* history_service_;
base::CancelableTaskTracker tracker_;
Expand Down
64 changes: 27 additions & 37 deletions chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
#include "components/keyed_service/content/browser_context_dependency_manager.h"
#include "components/keyed_service/core/service_access_type.h"
#include "components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h"
#include "components/ntp_snippets/category_factory.h"
#include "components/ntp_snippets/category_rankers/constant_category_ranker.h"
#include "components/ntp_snippets/content_suggestions_service.h"
#include "components/ntp_snippets/features.h"
#include "components/ntp_snippets/ntp_snippets_constants.h"
Expand Down Expand Up @@ -75,7 +75,6 @@ using content::BrowserThread;
using history::HistoryService;
using image_fetcher::ImageFetcherImpl;
using ntp_snippets::BookmarkSuggestionsProvider;
using ntp_snippets::CategoryFactory;
using ntp_snippets::ContentSuggestionsService;
using ntp_snippets::ForeignSessionsSuggestionsProvider;
using ntp_snippets::NTPSnippetsFetcher;
Expand All @@ -100,52 +99,47 @@ void ClearScheduledTasks() {
#if defined(OS_ANDROID)
void RegisterRecentTabProvider(OfflinePageModel* offline_page_model,
ContentSuggestionsService* service,
CategoryFactory* category_factory,
PrefService* pref_service) {
auto provider = base::MakeUnique<RecentTabSuggestionsProvider>(
service, category_factory, offline_page_model, pref_service);
service, offline_page_model, pref_service);
service->RegisterProvider(std::move(provider));
}

void RegisterDownloadsProvider(OfflinePageModel* offline_page_model,
DownloadManager* download_manager,
ContentSuggestionsService* service,
CategoryFactory* category_factory,
PrefService* pref_service) {
bool download_manager_ui_enabled =
base::FeatureList::IsEnabled(chrome::android::kDownloadsUiFeature);
auto provider = base::MakeUnique<DownloadSuggestionsProvider>(
service, category_factory, offline_page_model, download_manager,
pref_service, download_manager_ui_enabled);
service, offline_page_model, download_manager, pref_service,
download_manager_ui_enabled);
service->RegisterProvider(std::move(provider));
}
#endif // OS_ANDROID

void RegisterBookmarkProvider(BookmarkModel* bookmark_model,
ContentSuggestionsService* service,
CategoryFactory* category_factory,
PrefService* pref_service) {
auto provider = base::MakeUnique<BookmarkSuggestionsProvider>(
service, category_factory, bookmark_model, pref_service);
service, bookmark_model, pref_service);
service->RegisterProvider(std::move(provider));
}

#if defined(OS_ANDROID)
void RegisterPhysicalWebPageProvider(
ContentSuggestionsService* service,
CategoryFactory* category_factory,
PhysicalWebDataSource* physical_web_data_source,
PrefService* pref_service) {
auto provider = base::MakeUnique<PhysicalWebPageSuggestionsProvider>(
service, category_factory, physical_web_data_source, pref_service);
service, physical_web_data_source, pref_service);
service->RegisterProvider(std::move(provider));
}
#endif // OS_ANDROID

void RegisterArticleProvider(SigninManagerBase* signin_manager,
OAuth2TokenService* token_service,
ContentSuggestionsService* service,
CategoryFactory* category_factory,
LanguageModel* language_model,
PrefService* pref_service,
Profile* profile) {
Expand All @@ -167,15 +161,14 @@ void RegisterArticleProvider(SigninManagerBase* signin_manager,
bool is_stable_channel =
chrome::GetChannel() == version_info::Channel::STABLE;
auto provider = base::MakeUnique<RemoteSuggestionsProvider>(
service, service->category_factory(), pref_service,
g_browser_process->GetApplicationLocale(), service->user_classifier(),
scheduler, base::MakeUnique<NTPSnippetsFetcher>(
signin_manager, token_service, request_context,
pref_service, category_factory, language_model,
base::Bind(&safe_json::SafeJsonParser::Parse),
is_stable_channel ? google_apis::GetAPIKey()
: google_apis::GetNonStableAPIKey(),
service->user_classifier()),
service, pref_service, g_browser_process->GetApplicationLocale(),
service->category_ranker(), service->user_classifier(), scheduler,
base::MakeUnique<NTPSnippetsFetcher>(
signin_manager, token_service, request_context, pref_service,
language_model, base::Bind(&safe_json::SafeJsonParser::Parse),
is_stable_channel ? google_apis::GetAPIKey()
: google_apis::GetNonStableAPIKey(),
service->user_classifier()),
base::MakeUnique<ImageFetcherImpl>(base::MakeUnique<ImageDecoderImpl>(),
request_context.get()),
base::MakeUnique<ImageDecoderImpl>(),
Expand All @@ -188,12 +181,11 @@ void RegisterArticleProvider(SigninManagerBase* signin_manager,

void RegisterForeignSessionsProvider(SyncService* sync_service,
ContentSuggestionsService* service,
CategoryFactory* category_factory,
PrefService* pref_service) {
std::unique_ptr<TabDelegateSyncAdapter> sync_adapter =
base::MakeUnique<TabDelegateSyncAdapter>(sync_service);
auto provider = base::MakeUnique<ForeignSessionsSuggestionsProvider>(
service, category_factory, std::move(sync_adapter), pref_service);
service, std::move(sync_adapter), pref_service);
service->RegisterProvider(std::move(provider));
}

Expand Down Expand Up @@ -251,16 +243,18 @@ KeyedService* ContentSuggestionsServiceFactory::BuildServiceInstanceFor(
HistoryService* history_service = HistoryServiceFactory::GetForProfile(
profile, ServiceAccessType::EXPLICIT_ACCESS);
PrefService* pref_service = profile->GetPrefs();
auto* service = new ContentSuggestionsService(state, signin_manager,
history_service, pref_service);
auto category_ranker =
base::MakeUnique<ntp_snippets::ConstantCategoryRanker>();
auto* service =
new ContentSuggestionsService(state, signin_manager, history_service,
pref_service, std::move(category_ranker));
if (state == State::DISABLED) {
// Since we won't initialise the services, they won't get a chance to
// unschedule their tasks. We do it explicitly here instead.
ClearScheduledTasks();
return service;
}

CategoryFactory* category_factory = service->category_factory();
#if defined(OS_ANDROID)
OfflinePageModel* offline_page_model =
OfflinePageModelFactory::GetForBrowserContext(profile);
Expand All @@ -281,8 +275,7 @@ KeyedService* ContentSuggestionsServiceFactory::BuildServiceInstanceFor(
#if defined(OS_ANDROID)
if (base::FeatureList::IsEnabled(
ntp_snippets::kRecentOfflineTabSuggestionsFeature)) {
RegisterRecentTabProvider(offline_page_model, service, category_factory,
pref_service);
RegisterRecentTabProvider(offline_page_model, service, pref_service);
}

bool show_asset_downloads =
Expand All @@ -293,35 +286,32 @@ KeyedService* ContentSuggestionsServiceFactory::BuildServiceInstanceFor(
RegisterDownloadsProvider(
show_offline_page_downloads ? offline_page_model : nullptr,
show_asset_downloads ? download_manager : nullptr, service,
category_factory, pref_service);
pref_service);
}
#endif // OS_ANDROID

// |bookmark_model| can be null in tests.
if (base::FeatureList::IsEnabled(ntp_snippets::kBookmarkSuggestionsFeature) &&
bookmark_model) {
RegisterBookmarkProvider(bookmark_model, service, category_factory,
pref_service);
RegisterBookmarkProvider(bookmark_model, service, pref_service);
}

#if defined(OS_ANDROID)
if (base::FeatureList::IsEnabled(
ntp_snippets::kPhysicalWebPageSuggestionsFeature)) {
RegisterPhysicalWebPageProvider(service, category_factory,
physical_web_data_source, pref_service);
RegisterPhysicalWebPageProvider(service, physical_web_data_source,
pref_service);
}
#endif // OS_ANDROID

if (base::FeatureList::IsEnabled(ntp_snippets::kArticleSuggestionsFeature)) {
RegisterArticleProvider(signin_manager, token_service, service,
category_factory, language_model, pref_service,
profile);
language_model, pref_service, profile);
}

if (base::FeatureList::IsEnabled(
ntp_snippets::kForeignSessionsSuggestionsFeature)) {
RegisterForeignSessionsProvider(sync_service, service, category_factory,
pref_service);
RegisterForeignSessionsProvider(sync_service, service, pref_service);
}

return service;
Expand Down
5 changes: 2 additions & 3 deletions chrome/browser/ntp_snippets/download_suggestions_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -129,14 +129,13 @@ std::unique_ptr<OfflinePageModelQuery> BuildOfflinePageDownloadsQuery(

DownloadSuggestionsProvider::DownloadSuggestionsProvider(
ContentSuggestionsProvider::Observer* observer,
ntp_snippets::CategoryFactory* category_factory,
offline_pages::OfflinePageModel* offline_page_model,
content::DownloadManager* download_manager,
PrefService* pref_service,
bool download_manager_ui_enabled)
: ContentSuggestionsProvider(observer, category_factory),
: ContentSuggestionsProvider(observer),
category_status_(CategoryStatus::AVAILABLE_LOADING),
provided_category_(category_factory->FromKnownCategory(
provided_category_(Category::FromKnownCategory(
ntp_snippets::KnownCategories::DOWNLOADS)),
offline_page_model_(offline_page_model),
download_manager_(download_manager),
Expand Down
2 changes: 0 additions & 2 deletions chrome/browser/ntp_snippets/download_suggestions_provider.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
#include "base/memory/weak_ptr.h"
#include "components/ntp_snippets/callbacks.h"
#include "components/ntp_snippets/category.h"
#include "components/ntp_snippets/category_factory.h"
#include "components/ntp_snippets/category_status.h"
#include "components/ntp_snippets/content_suggestion.h"
#include "components/ntp_snippets/content_suggestions_provider.h"
Expand Down Expand Up @@ -42,7 +41,6 @@ class DownloadSuggestionsProvider
public:
DownloadSuggestionsProvider(
ContentSuggestionsProvider::Observer* observer,
ntp_snippets::CategoryFactory* category_factory,
offline_pages::OfflinePageModel* offline_page_model,
content::DownloadManager* download_manager,
PrefService* pref_service,
Expand Down
Loading

0 comments on commit 7456f5a

Please sign in to comment.