[Remote suggestions] Move the throttler into the Scheduler

[Remote suggestions] Move the throttler into the Scheduler

This CL moves the throttler into the Scheduler -- which is now the
central place to decide whether to and when to fetch.

This change also helps in employing I/O scheduler in the Scheduler for
prioritization of background fethes. I/O scheduler tasks cannot run on
the main thread and thus cannot use PrefService. As throttler depends
on prefs, the easiest way is to ask for quota before posting the task
to I/O scheduler.

This change removes the quota_error fetch status reporting from UMA.
This is no problem as all the quota requests are reported to UMA from
the throttler anyway.

BUG=694325
Review-Url: https://codereview.chromium.org/2714863004
Cr-Commit-Position: refs/heads/master@{#454258}
Committed: https://chromium.googlesource.com/chromium/src/+/0a06fdaf8ba732ae795bcb1f2331523babc88dd8

[jkrcal, 2017-02-24 09:34:42]

jkrcal@chromium.org changed reviewers:
+ treib@chromium.org

[jkrcal, 2017-02-24 09:34:42]

Marc, could you PTAL?

[tschumann, 2017-02-24 10:18:08]

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/json_request.h (right):

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/json_request.h:45:
INTERACTIVE_QUOTA_ERROR_OBSOLETE,
If it's nothing widely adopted, I'd prefer prefixing enum names so that it's
more obvious.

Also, how we deal with UMA is a bit weird.
-- histograms.xml relies on the numeric value -- best to assign them
specifically.
-- the old constants themselves don't need to be preserved; it's important we
don't reuse codes. A comment with 'next available id' would also do the trick,
hm?

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h
(right):

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:151:
bool DemandQuotaForRequest(bool interactive_request);
nit: would AcquireQuota() be a better name? We have much better context here, so
we can afford shorter names.

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
File
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:704:
scheduling_provider_->Fetch(category, known_suggestions, FetchDoneCallback());
shouldn't we verify the result codes?

[Marc Treib, 2017-02-24 15:50:35]

Thanks for doing this cleanup!
Some comments below, but nothing major.

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/json_request.h (right):

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/json_request.h:45:
INTERACTIVE_QUOTA_ERROR_OBSOLETE,
On 2017/02/24 10:18:08, tschumann wrote:
> If it's nothing widely adopted, I'd prefer prefixing enum names so that it's
> more obvious.
> 
> Also, how we deal with UMA is a bit weird.
> -- histograms.xml relies on the numeric value -- best to assign them
> specifically.
> -- the old constants themselves don't need to be preserved; it's important we
> don't reuse codes. A comment with 'next available id' would also do the trick,
> hm?

+1 to making a prefix. In fact, why not use "DEPRECATED_" which is already used
above.

I don't much like those "next available ID" comments; they're too easy to miss.

No strong feelings about explicit numeric values.

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/remote_suggestions_fetcher.cc (right):

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/remote_suggestions_fetcher.cc:64: case
FetchResult::DEPRECATED_EMPTY_HOSTS:
Could you move this into the "invalid" section too?

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/remote_suggestions_fetcher.cc:92: case
FetchResult::DEPRECATED_EMPTY_HOSTS:
Also here

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h
(right):

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:160:
std::unique_ptr<FetchStatusCallback> callback,
Unrelated to this CL, but: Why are we passing around unique_ptrs to callbacks?
Just to make them optional? Callbacks are already "nullable" by themselves, so
let's not add a second layer of nullability.
Or is there some other reason I'm missing?

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
File
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:693:
// As long as the quota suffices, the call gets through.
Maybe mention that both ReloadSuggestions and Fetch consume quota? Took me a bit
to figure this out.
(Also, is there any reason to interleave the calls? It kinda seems to me like
we're testing two different things here, but I might be missing something.)

[tschumann, 2017-02-25 17:02:19]

tschumann@chromium.org changed reviewers:
+ tschumann@chromium.org

[tschumann, 2017-02-25 17:02:20]

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/json_request.h (right):

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/json_request.h:45:
INTERACTIVE_QUOTA_ERROR_OBSOLETE,
On 2017/02/24 15:50:35, Marc Treib wrote:
> On 2017/02/24 10:18:08, tschumann wrote:
> > If it's nothing widely adopted, I'd prefer prefixing enum names so that it's
> > more obvious.
> > 
> > Also, how we deal with UMA is a bit weird.
> > -- histograms.xml relies on the numeric value -- best to assign them
> > specifically.
> > -- the old constants themselves don't need to be preserved; it's important
we
> > don't reuse codes. A comment with 'next available id' would also do the
trick,
> > hm?
> 
> +1 to making a prefix. In fact, why not use "DEPRECATED_" which is already
used
> above.
> 
> I don't much like those "next available ID" comments; they're too easy to
miss.
> 
> No strong feelings about explicit numeric values.

The numbers have the advantage that it's much easier to see histograms.xml being
in-sync (no need to count the order).
I'm unhappy about the fact that the code needs to deal with values which are not
used anymore (see the switch statements in this file). 
There is a manual (untested) step involved anyways (updating histograms.xml), so
I'd much more prefer the next_id comment (we can also add a comment before
RESULT_MAX explaining to take care of it).

Also, any reason we have the RESULT_MAX value in here?  given that there are
unused states in between, there doesn't seem to be much use for it.

[Marc Treib, 2017-02-27 09:18:44]

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/json_request.h (right):

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/json_request.h:45:
INTERACTIVE_QUOTA_ERROR_OBSOLETE,
On 2017/02/25 17:02:20, tschumann wrote:
> On 2017/02/24 15:50:35, Marc Treib wrote:
> > On 2017/02/24 10:18:08, tschumann wrote:
> > > If it's nothing widely adopted, I'd prefer prefixing enum names so that
it's
> > > more obvious.
> > > 
> > > Also, how we deal with UMA is a bit weird.
> > > -- histograms.xml relies on the numeric value -- best to assign them
> > > specifically.
> > > -- the old constants themselves don't need to be preserved; it's important
> we
> > > don't reuse codes. A comment with 'next available id' would also do the
> trick,
> > > hm?
> > 
> > +1 to making a prefix. In fact, why not use "DEPRECATED_" which is already
> used
> > above.
> > 
> > I don't much like those "next available ID" comments; they're too easy to
> miss.
> > 
> > No strong feelings about explicit numeric values.
> 
> The numbers have the advantage that it's much easier to see histograms.xml
being
> in-sync (no need to count the order).
> I'm unhappy about the fact that the code needs to deal with values which are
not
> used anymore (see the switch statements in this file). 
> There is a manual (untested) step involved anyways (updating histograms.xml),
so
> I'd much more prefer the next_id comment (we can also add a comment before
> RESULT_MAX explaining to take care of it).
> 
> Also, any reason we have the RESULT_MAX value in here?  given that there are
> unused states in between, there doesn't seem to be much use for it.

The MAX is there for the UMA macros, they need a "max_valid_input+1" param.

[jkrcal, 2017-02-27 10:26:22]

jkrcal@chromium.org changed reviewers:
+ isherman@chromium.org

[jkrcal, 2017-02-27 10:26:23]

Thanks for all the comments. PTAL, again!

Ilya, could you PTAL at histograms.xml?

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/json_request.h (right):

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/json_request.h:45:
INTERACTIVE_QUOTA_ERROR_OBSOLETE,
On 2017/02/27 09:18:43, Marc Treib wrote:
> On 2017/02/25 17:02:20, tschumann wrote:
> > On 2017/02/24 15:50:35, Marc Treib wrote:
> > > On 2017/02/24 10:18:08, tschumann wrote:
> > > > If it's nothing widely adopted, I'd prefer prefixing enum names so that
> it's
> > > > more obvious.
> > > > 
> > > > Also, how we deal with UMA is a bit weird.
> > > > -- histograms.xml relies on the numeric value -- best to assign them
> > > > specifically.
> > > > -- the old constants themselves don't need to be preserved; it's
important
> > we
> > > > don't reuse codes. A comment with 'next available id' would also do the
> > trick,
> > > > hm?
> > > 
> > > +1 to making a prefix. In fact, why not use "DEPRECATED_" which is already
> > used
> > > above.
> > > 
> > > I don't much like those "next available ID" comments; they're too easy to
> > miss.
> > > 
> > > No strong feelings about explicit numeric values.
> > 
> > The numbers have the advantage that it's much easier to see histograms.xml
> being
> > in-sync (no need to count the order).
> > I'm unhappy about the fact that the code needs to deal with values which are
> not
> > used anymore (see the switch statements in this file). 
> > There is a manual (untested) step involved anyways (updating
histograms.xml),
> so
> > I'd much more prefer the next_id comment (we can also add a comment before
> > RESULT_MAX explaining to take care of it).
> > 
> > Also, any reason we have the RESULT_MAX value in here?  given that there are
> > unused states in between, there doesn't seem to be much use for it.
> 
> The MAX is there for the UMA macros, they need a "max_valid_input+1" param.

Added numeric values which allowed removing the deprecated ones.
I kept the RESULT_MAX due to the reason mentioned by Marc and because it further
emphasises what is the next available ID.

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/remote_suggestions_fetcher.cc (right):

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/remote_suggestions_fetcher.cc:64: case
FetchResult::DEPRECATED_EMPTY_HOSTS:
On 2017/02/24 15:50:35, Marc Treib wrote:
> Could you move this into the "invalid" section too?

Done.

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/remote_suggestions_fetcher.cc:92: case
FetchResult::DEPRECATED_EMPTY_HOSTS:
On 2017/02/24 15:50:35, Marc Treib wrote:
> Also here

Done.

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h
(right):

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:151:
bool DemandQuotaForRequest(bool interactive_request);
On 2017/02/24 10:18:08, tschumann wrote:
> nit: would AcquireQuota() be a better name? We have much better context here,
so
> we can afford shorter names.

Done.

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:160:
std::unique_ptr<FetchStatusCallback> callback,
On 2017/02/24 15:50:35, Marc Treib wrote:
> Unrelated to this CL, but: Why are we passing around unique_ptrs to callbacks?
> Just to make them optional? Callbacks are already "nullable" by themselves, so
> let's not add a second layer of nullability.
> Or is there some other reason I'm missing?

This is a known issue. Added a TODO with a bug link to the
remote_suggestions_provider.h interface.

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
File
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:693:
// As long as the quota suffices, the call gets through.
On 2017/02/24 15:50:35, Marc Treib wrote:
> Maybe mention that both ReloadSuggestions and Fetch consume quota? Took me a
bit
> to figure this out.
> (Also, is there any reason to interleave the calls? It kinda seems to me like
> we're testing two different things here, but I might be missing something.)

Added a comment. The reason to mix these two is to test that they consume the
same quota (and that the consumption adds up).

https://codereview.chromium.org/2714863004/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:704:
scheduling_provider_->Fetch(category, known_suggestions, FetchDoneCallback());
On 2017/02/24 10:18:08, tschumann wrote:
> shouldn't we verify the result codes?

Done.

[Marc Treib, 2017-02-27 11:15:14]

lgtm

[Ilya Sherman, 2017-02-27 19:29:59]

histograms.xml lgtm

[Ilya Sherman, 2017-02-27 19:32:26]

By the way, a suggestion for the C++ code (but LGTM either way)

https://codereview.chromium.org/2714863004/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/json_request.h (right):

https://codereview.chromium.org/2714863004/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/json_request.h:38: SUCCESS = 0,
I'd suggest keeping a comment line like

  // DEPRECATED_EMPTY_HOSTS = 1,

to document what value used to be here, and to discourage a well-intentioned
project member from "helpfully fixing" this code in the future.  (Ditto for the
other two deprecated values).

[jkrcal, 2017-03-02 13:38:33]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2017-03-02 13:38:33]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2714863004/#ps60001
(title: "Ilya's comment")

[commit-bot: I haz the power, 2017-03-02 13:38:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 15:05:55]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488461913097960,
"parent_rev": "66956e6ee256c827808b4dc4a05c32ab077747d5", "commit_rev":
"0a06fdaf8ba732ae795bcb1f2331523babc88dd8"}

[commit-bot: I haz the power, 2017-03-02 15:06:30]

Description was changed from

==========
[Remote suggestions] Move the throttler into the Scheduler

This CL moves the throttler into the Scheduler -- which is now the
central place to decide whether to and when to fetch.

This change also helps in employing I/O scheduler in the Scheduler for
prioritization of background fethes. I/O scheduler tasks cannot run on
the main thread and thus cannot use PrefService. As throttler depends
on prefs, the easiest way is to ask for quota before posting the task
to I/O scheduler.

This change removes the quota_error fetch status reporting from UMA.
This is no problem as all the quota requests are reported to UMA from
the throttler anyway.

BUG=694325
==========

to

==========
[Remote suggestions] Move the throttler into the Scheduler

This CL moves the throttler into the Scheduler -- which is now the
central place to decide whether to and when to fetch.

This change also helps in employing I/O scheduler in the Scheduler for
prioritization of background fethes. I/O scheduler tasks cannot run on
the main thread and thus cannot use PrefService. As throttler depends
on prefs, the easiest way is to ask for quota before posting the task
to I/O scheduler.

This change removes the quota_error fetch status reporting from UMA.
This is no problem as all the quota requests are reported to UMA from
the throttler anyway.

BUG=694325

Review-Url: https://codereview.chromium.org/2714863004
Cr-Commit-Position: refs/heads/master@{#454258}
Committed:
https://chromium.googlesource.com/chromium/src/+/0a06fdaf8ba732ae795bcb1f2331...
==========

[commit-bot: I haz the power, 2017-03-02 15:06:31]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/0a06fdaf8ba732ae795bcb1f2331...