[Remote suggestions] Refactor the scheduler

components/ntp_snippets/remote/remote_suggestions_provider_impl.cc

Index: components/ntp_snippets/remote/remote_suggestions_provider_impl.cc
diff --git a/components/ntp_snippets/remote/remote_suggestions_provider_impl.cc b/components/ntp_snippets/remote/remote_suggestions_provider_impl.cc
index eba4dda04ebb8442ea1d740120c4d246b4542079..1c44cc81f2aab950ce24d67c1fcce57ea0f5db9f 100644
--- a/components/ntp_snippets/remote/remote_suggestions_provider_impl.cc
+++ b/components/ntp_snippets/remote/remote_suggestions_provider_impl.cc
@@ -324,6 +324,10 @@ void RemoteSuggestionsProviderImpl::SetRemoteSuggestionsScheduler(
 }
 
 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);
 }
@@ -368,6 +372,12 @@ void RemoteSuggestionsProviderImpl::Fetch(
                                 "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());
@@ -643,6 +653,9 @@ void RemoteSuggestionsProviderImpl::OnFetchMoreFinished(
   // 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);
 }
@@ -727,6 +740,10 @@ void RemoteSuggestionsProviderImpl::OnFetchFinished(
                          content.dismissed.size());
   }
 
+  if (interactive_request && remote_suggestions_scheduler_) {
+    remote_suggestions_scheduler_->OnInteractiveFetchFinished(status);
+  }
+
   if (callback) {
     callback->Run(status);
   }