[Remote suggestions] Refactor the scheduler

components/ntp_snippets/remote/remote_suggestions_provider_impl.cc

 // Copyright 2015 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/remote/remote_suggestions_provider_impl.h"
 
 #include <algorithm>
 #include <iterator>
 #include <utility>
 
@@ skipping 306 matching lines @@
 }
 
 void RemoteSuggestionsProviderImpl::SetRemoteSuggestionsScheduler(
     RemoteSuggestionsScheduler* scheduler) {
   remote_suggestions_scheduler_ = scheduler;
   // Call the observer right away if we've reached any final state.
   NotifyStateChanged();
 }
 
 void RemoteSuggestionsProviderImpl::ReloadSuggestions() {
+  if (remote_suggestions_scheduler_ &&
+      !remote_suggestions_scheduler_->AcquireQuotaForInteractiveFetch()) {

[Marc Treib, 2017/03/23 16:41:15]

Hm. So if there's no scheduler, we don't bother with quota? That seems kinda
dangerous.

[tschumann, 2017/03/23 17:38:27]

+1, let's return early if we don't have a scheduler explicitly.
should we make check part of ready() and simply use that?

[jkrcal, 2017/03/27 10:03:02]

Done. I cannot make it part of ready() because these calls should make it
further when the DB is not ready().

+    return;
+  }
   FetchSuggestions(/*interactive_request=*/true,
                    /*callback=*/nullptr);
 }
 
 void RemoteSuggestionsProviderImpl::RefetchInTheBackground(
     std::unique_ptr<FetchStatusCallback> callback) {
   FetchSuggestions(/*interactive_request=*/false, std::move(callback));
 }
 
 const RemoteSuggestionsFetcher*
@@ skipping 24 matching lines @@
 void RemoteSuggestionsProviderImpl::Fetch(
     const Category& category,
     const std::set<std::string>& known_suggestion_ids,
     const FetchDoneCallback& callback) {
   if (!ready()) {
     CallWithEmptyResults(callback,
                          Status(StatusCode::TEMPORARY_ERROR,
                                 "RemoteSuggestionsProvider is not ready!"));
     return;
   }
+  if (remote_suggestions_scheduler_ &&
+      !remote_suggestions_scheduler_->AcquireQuotaForInteractiveFetch()) {
+    CallWithEmptyResults(callback, Status(StatusCode::TEMPORARY_ERROR,
+                                          "Interactive quota exceeded!"));
+    return;
+  }
   RequestParams params = BuildFetchParams();
   params.excluded_ids.insert(known_suggestion_ids.begin(),
                              known_suggestion_ids.end());
   params.interactive_request = true;
   params.exclusive_category = category;
 
   suggestions_fetcher_->FetchSnippets(
       params,
       base::BindOnce(&RemoteSuggestionsProviderImpl::OnFetchMoreFinished,
                      base::Unretained(this), callback));
@@ skipping 255 matching lines @@
 
   IntegrateSuggestions(existing_content,
                        std::move(fetched_category.suggestions));
 
   // TODO(tschumann): We should properly honor the existing category state,
   // e.g. to make sure we don't serve results after the sign-out. Revisit this:
   // Should Nuke also cancel outstanding requests, or do we want to check the
   // status?
   UpdateCategoryStatus(category, CategoryStatus::AVAILABLE);
   // Notify callers and observers.
+  if (remote_suggestions_scheduler_) {

[tschumann, 2017/03/23 17:38:27]

this should never be nullptr, right? remove the if?
Didn't we think about making this call by wrapping the fetching_callback?
(same below)
That way, we also had a single place to verify the scheduler is not nullptr and
don't have to worry about that state here.

[jkrcal, 2017/03/27 10:03:02]

Correct, removed the ifs here and below.

I am not sure about the wrappers. 
 - We use wrapper callbacks for fetches initialized from the scheduler.
 - It is the responsibility of the provider to ask for quota & inform about
status for interactive fetches. I am not sure it is cleaner to set it up all
here compared to the current situation. The two callbacks have different
signatures, for one of them, I would need to introduce a helper function...

Since Tim is OOO, Marc what it your opinion?

[Marc Treib, 2017/03/27 11:31:00]

Not sure I understand the proposal... who would wrap the callback?

[jkrcal, 2017/03/27 14:09:31]

I am not sure either :D

The only solution I can imagine is that 
 - ReloadSuggestions() actually provides a callback to FetchSuggestions() that
does the work being done here;
 - Fetch() wraps the callback provided to FetchSnippets() to do the work being
done here.

These two callbacks have a different signature, so I see more ugliness than
elegance in this whole idea.

[Marc Treib, 2017/03/27 14:27:20]

Alright, then let's leave it as is for now, and let's see what Tim says once
he's back.

Seems to me this boils down to cleaning up the whole mess between
OnFetchFinished and OnFetchMoreFinished, which is a Good Thing and we should do
that, but it's orthogonal to this CL.

[tschumann, 2017/03/27 15:40:45]

OnFetchMoreFinished is already doing a lot of things, and this CL adds more
responsibilities to it, including accessing state which should always be present
in this particular context (the scheduler) -- but that's not easy to see.
Instead, my suggestion is that Fetch() makes sure the passed in
'fetching_callback' takes care of notifying the scheduler.
This might require an additional helper-function or just an inlined lambda.
However, anything we can do to keep complexity out of OnFetchMoreFinished is a
good thing.

I don't think that's orthogonal to this CL, as the question is where the extra
logic should be implemented.

[jkrcal, 2017/03/27 16:02:22]

Tim, in the current state of the CL, the scheduler is always present as it is
passed in the constructor. Thus, there is not that much of extra logic.

Anyway, if you really feel strongly, I can implement your proposal.

[jkrcal, 2017/03/29 10:55:57]

Okay, done.

+    remote_suggestions_scheduler_->OnInteractiveFetchFinished(status);
+  }
   fetching_callback.Run(Status::Success(), std::move(result));
   NotifyNewSuggestions(category, *existing_content);
 }
 
 void RemoteSuggestionsProviderImpl::OnFetchFinished(
     std::unique_ptr<FetchStatusCallback> callback,
     bool interactive_request,
     Status status,
     RemoteSuggestionsFetcher::OptionalFetchedCategories fetched_categories) {
   if (!ready()) {
@@ skipping 64 matching lines @@
   auto content_it = category_contents_.find(articles_category_);
   DCHECK(content_it != category_contents_.end());
   const CategoryContent& content = content_it->second;
   UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumArticles",
                               content.suggestions.size());
   if (content.suggestions.empty() && !content.dismissed.empty()) {
     UMA_HISTOGRAM_COUNTS("NewTabPage.Snippets.NumArticlesZeroDueToDiscarded",
                          content.dismissed.size());
   }
 
+  if (interactive_request && remote_suggestions_scheduler_) {
+    remote_suggestions_scheduler_->OnInteractiveFetchFinished(status);
+  }
+
   if (callback) {
     callback->Run(status);
   }
 }
 
 void RemoteSuggestionsProviderImpl::ArchiveSuggestions(
     CategoryContent* content,
     RemoteSuggestion::PtrVector* to_archive) {
   // Archive previous suggestions - move them at the beginning of the list.
   content->archived.insert(content->archived.begin(),
@@ skipping 531 matching lines @@
 RemoteSuggestionsProviderImpl::CategoryContent::CategoryContent(
     CategoryContent&&) = default;
 
 RemoteSuggestionsProviderImpl::CategoryContent::~CategoryContent() = default;
 
 RemoteSuggestionsProviderImpl::CategoryContent&
 RemoteSuggestionsProviderImpl::CategoryContent::operator=(CategoryContent&&) =
     default;
 
 }  // namespace ntp_snippets