Adding a request counter to the snippets fetcher.

Adding a request counter to the snippets fetcher.

This CL applies the RequestCounter to NTPSnippetsFetcher.

BUG=627073
Committed: https://crrev.com/2d194db915724d87d566e558d87296a4f6d367da
Cr-Commit-Position: refs/heads/master@{#407769}

[jkrcal, 2016-07-21 18:43:35]

Description was changed from

==========
Adding a request counter to the snippets fetcher.

This CL applies the RequestCounter to NTPSnippetsFetcher. As an initial
CL, it does not distinguish forced fetches initiated from the UI. Thus,
all fetches are counted agains the quota. This will be extended in a
follow-up CL.

BUG=627073
==========

to

==========
Adding a request counter to the snippets fetcher.

This CL applies the RequestCounter to NTPSnippetsFetcher. 

BUG=627073
==========

[jkrcal, 2016-07-21 18:55:11]

The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 18:55:56]

Dry run: 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, 2016-07-21 18:59:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 18:59:11]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jkrcal, 2016-07-22 13:44:26]

jkrcal@chromium.org changed reviewers:
+ bauerb@chromium.org, noyau@chromium.org, treib@chromium.org

[jkrcal, 2016-07-22 13:44:27]

Marc, PTAL at the components/ntp_snippets/ code.
Berhnard, PTAL at the java + bridge code.
Éric, PTAL at the ios factory.

[Marc Treib, 2016-07-22 13:52:53]

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.cc:79: const int
kArticlesQuotaPerDay = 20;
This sounds like "number of articles per day", but it's actually the number of
fetches.

Also, should we build in the option to override this via variation params?

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.h (right):

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:98: // Fetches snippets only if
the daily quota not exceeded, unless |force| is
force_request (or rename the actual param below)

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher_unittest.cc (right):

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher_unittest.cc:177:
NTPSnippetsFetcher& snippets_fetcher() { return *snippets_fetcher_.get(); }
I think the .get() isn't needed?

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:443:
FetchSnippetsFromHosts(hosts, false);
/* force_request */ ?

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:656: FetchSnippets(false);
here too

[Bernhard Bauer, 2016-07-22 13:59:14]

lgtm

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.h (right):

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:98: // Fetches snippets only if
the daily quota not exceeded, unless |force| is
Nit: The name of the parameter is |force_request|; you could update either to
match.

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:107: // Fetches snippets from the
server for specified hosts (overriding
Nit: can you add an empty line before this comment?

[jkrcal, 2016-07-25 10:05:38]

Thanks!

Éric, PTAL at the iOS factory.

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.cc:79: const int
kArticlesQuotaPerDay = 20;
On 2016/07/22 13:52:52, Marc Treib wrote:
> This sounds like "number of articles per day", but it's actually the number of
> fetches.
> 
> Also, should we build in the option to override this via variation params?

Done.

Overriding by variation params is built in the RequestThrottler :) This is just
the default quota value if no variation param is given.

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.h (right):

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:98: // Fetches snippets only if
the daily quota not exceeded, unless |force| is
On 2016/07/22 13:59:14, Bernhard Bauer wrote:
> Nit: The name of the parameter is |force_request|; you could update either to
> match.

Done.

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher_unittest.cc (right):

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher_unittest.cc:177:
NTPSnippetsFetcher& snippets_fetcher() { return *snippets_fetcher_.get(); }
On 2016/07/22 13:52:52, Marc Treib wrote:
> I think the .get() isn't needed?

Done. Thanks! I am still not used to the magic of operator overloading :)

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:443:
FetchSnippetsFromHosts(hosts, false);
On 2016/07/22 13:52:52, Marc Treib wrote:
> /* force_request */ ?

Done.

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:656: FetchSnippets(false);
On 2016/07/22 13:52:52, Marc Treib wrote:
> here too

Done.

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:107: // Fetches snippets from the
server for specified hosts (overriding
On 2016/07/22 13:59:14, Bernhard Bauer wrote:
> Nit: can you add an empty line before this comment?

Done.

[jkrcal, 2016-07-25 10:05:47]

The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 10:06:04]

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

[jkrcal, 2016-07-25 10:08:26]

https://codereview.chromium.org/2162533002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/2162533002/diff/120001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_fetcher.cc:79: const int
kContentFetchesQuotaPerDay = 30;
I still wonder what is the right limit. I think 30 is more appropriate if we
keep updating every 30mins (when charging on wifi). Even with such a quota,
throttling could hit the user under normal conditions. Is there any strong
reason to update as often as every 30 mins?

[Marc Treib, 2016-07-25 10:11:55]

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.cc:79: const int
kArticlesQuotaPerDay = 20;
On 2016/07/25 10:05:38, jkrcal wrote:
> On 2016/07/22 13:52:52, Marc Treib wrote:
> > This sounds like "number of articles per day", but it's actually the number
of
> > fetches.
> > 
> > Also, should we build in the option to override this via variation params?
> 
> Done.
> 
> Overriding by variation params is built in the RequestThrottler :) This is
just
> the default quota value if no variation param is given.

I see... this is surprising to me. Since everything else related to the quota is
defined in the RequestThrottler, should we maybe move the default quota
definition there too?
If not, please at least add a "Default" somewhere in the name, to make it clear
to the reader that it might be overridden.

https://codereview.chromium.org/2162533002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/2162533002/diff/120001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_fetcher.cc:79: const int
kContentFetchesQuotaPerDay = 30;
On 2016/07/25 10:08:26, jkrcal wrote:
> I still wonder what is the right limit. I think 30 is more appropriate if we
> keep updating every 30mins (when charging on wifi). Even with such a quota,
> throttling could hit the user under normal conditions. Is there any strong
> reason to update as often as every 30 mins?

IMO regular users shouldn't hit the throttling limit - this is just a safeguard
against bad behavior. So maybe 50 for now?

[jkrcal, 2016-07-25 10:26:59]

Thanks, Marc!

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/2162533002/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.cc:79: const int
kArticlesQuotaPerDay = 20;
On 2016/07/25 10:11:55, Marc Treib wrote:
> On 2016/07/25 10:05:38, jkrcal wrote:
> > On 2016/07/22 13:52:52, Marc Treib wrote:
> > > This sounds like "number of articles per day", but it's actually the
number
> of
> > > fetches.
> > > 
> > > Also, should we build in the option to override this via variation params?
> > 
> > Done.
> > 
> > Overriding by variation params is built in the RequestThrottler :) This is
> just
> > the default quota value if no variation param is given.
> 
> I see... this is surprising to me. Since everything else related to the quota
is
> defined in the RequestThrottler, should we maybe move the default quota
> definition there too?
> If not, please at least add a "Default" somewhere in the name, to make it
clear
> to the reader that it might be overridden.

Makes sense. Done.

https://codereview.chromium.org/2162533002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/2162533002/diff/120001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_fetcher.cc:79: const int
kContentFetchesQuotaPerDay = 30;
On 2016/07/25 10:11:55, Marc Treib wrote:
> On 2016/07/25 10:08:26, jkrcal wrote:
> > I still wonder what is the right limit. I think 30 is more appropriate if we
> > keep updating every 30mins (when charging on wifi). Even with such a quota,
> > throttling could hit the user under normal conditions. Is there any strong
> > reason to update as often as every 30 mins?
> 
> IMO regular users shouldn't hit the throttling limit - this is just a
safeguard
> against bad behavior. So maybe 50 for now?

Ok, changed to 50. Does fetching every 30 mins make sense?

[Marc Treib, 2016-07-25 11:07:46]

https://codereview.chromium.org/2162533002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/2162533002/diff/120001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_fetcher.cc:79: const int
kContentFetchesQuotaPerDay = 30;
On 2016/07/25 10:26:59, jkrcal wrote:
> On 2016/07/25 10:11:55, Marc Treib wrote:
> > On 2016/07/25 10:08:26, jkrcal wrote:
> > > I still wonder what is the right limit. I think 30 is more appropriate if
we
> > > keep updating every 30mins (when charging on wifi). Even with such a
quota,
> > > throttling could hit the user under normal conditions. Is there any strong
> > > reason to update as often as every 30 mins?
> > 
> > IMO regular users shouldn't hit the throttling limit - this is just a
> safeguard
> > against bad behavior. So maybe 50 for now?
> 
> Ok, changed to 50. Does fetching every 30 mins make sense?

Probably not, but that's a separate discussion :)
For now, I'd like to keep the default values consistent at least.

[noyau (Ping after 24h), 2016-07-25 11:54:52]

https://codereview.chromium.org/2162533002/diff/140001/ios/chrome/browser/ntp...
File ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc
(right):

https://codereview.chromium.org/2162533002/diff/140001/ios/chrome/browser/ntp...
ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc:123:
chrome_browser_state->GetPrefs(), base::Bind(&ParseJson),
GetPrefs() is called twice on this long line. Can you extract it in a local
variable? And reuse it for the StatusService below as well?

[jkrcal, 2016-07-26 08:30:11]

Thanks, Éric!

Berhnard, I addressed your post-commit comment from 2158843002 here. Could you
PTAL once again at request_throttler.*?

https://codereview.chromium.org/2162533002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/2162533002/diff/120001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_fetcher.cc:79: const int
kContentFetchesQuotaPerDay = 30;
On 2016/07/25 11:07:46, Marc Treib wrote:
> On 2016/07/25 10:26:59, jkrcal wrote:
> > On 2016/07/25 10:11:55, Marc Treib wrote:
> > > On 2016/07/25 10:08:26, jkrcal wrote:
> > > > I still wonder what is the right limit. I think 30 is more appropriate
if
> we
> > > > keep updating every 30mins (when charging on wifi). Even with such a
> quota,
> > > > throttling could hit the user under normal conditions. Is there any
strong
> > > > reason to update as often as every 30 mins?
> > > 
> > > IMO regular users shouldn't hit the throttling limit - this is just a
> > safeguard
> > > against bad behavior. So maybe 50 for now?
> > 
> > Ok, changed to 50. Does fetching every 30 mins make sense?
> 
> Probably not, but that's a separate discussion :)
> For now, I'd like to keep the default values consistent at least.

Acknowledged.

https://codereview.chromium.org/2162533002/diff/140001/ios/chrome/browser/ntp...
File ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc
(right):

https://codereview.chromium.org/2162533002/diff/140001/ios/chrome/browser/ntp...
ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc:123:
chrome_browser_state->GetPrefs(), base::Bind(&ParseJson),
On 2016/07/25 11:54:52, noyau wrote:
> GetPrefs() is called twice on this long line. Can you extract it in a local
> variable? And reuse it for the StatusService below as well?

Done.

[noyau (Ping after 24h), 2016-07-26 08:34:13]

lgtm

[jkrcal, 2016-07-26 08:37:09]

The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 08:37:27]

Dry run: 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, 2016-07-26 08:42:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-26 08:42:41]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Bernhard Bauer, 2016-07-26 09:52:46]

Thanks!

https://codereview.chromium.org/2162533002/diff/160001/components/ntp_snippet...
File components/ntp_snippets/request_throttler.h (right):

https://codereview.chromium.org/2162533002/diff/160001/components/ntp_snippet...
components/ntp_snippets/request_throttler.h:59: struct RequestTypeInfo {
Do you need to define the struct here or would a declaration be sufficient?

https://codereview.chromium.org/2162533002/diff/160001/components/ntp_snippet...
components/ntp_snippets/request_throttler.h:67: static const RequestTypeInfo
kRequestTypeInfo[1];
Same here for the array size -- could you just leave the size out?

[jkrcal, 2016-07-26 10:05:07]

Thanks!

https://codereview.chromium.org/2162533002/diff/160001/components/ntp_snippet...
File components/ntp_snippets/request_throttler.h (right):

https://codereview.chromium.org/2162533002/diff/160001/components/ntp_snippet...
components/ntp_snippets/request_throttler.h:59: struct RequestTypeInfo {
On 2016/07/26 09:52:45, Bernhard Bauer wrote:
> Do you need to define the struct here or would a declaration be sufficient?

Done.

https://codereview.chromium.org/2162533002/diff/160001/components/ntp_snippet...
components/ntp_snippets/request_throttler.h:67: static const RequestTypeInfo
kRequestTypeInfo[1];
On 2016/07/26 09:52:45, Bernhard Bauer wrote:
> Same here for the array size -- could you just leave the size out?

Ah, right!

https://codereview.chromium.org/2162533002/diff/180001/components/ntp_snippet...
File components/ntp_snippets/request_throttler_unittest.cc (right):

https://codereview.chromium.org/2162533002/diff/180001/components/ntp_snippet...
components/ntp_snippets/request_throttler_unittest.cc:29: throttler_->quota_ =
kCounterQuota;
Marc, moving the quota into the RequestTypeInfo complicated the unit test. Is
accessing a private field from a friend class ok or is a new public method
SetQuotaForTest() preferred?

[jkrcal, 2016-07-26 10:05:14]

The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 10:05:25]

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

[Bernhard Bauer, 2016-07-26 10:07:53]

LGTM, thanks!

[Marc Treib, 2016-07-26 10:10:17]

lgtm

https://codereview.chromium.org/2162533002/diff/180001/components/ntp_snippet...
File components/ntp_snippets/request_throttler_unittest.cc (right):

https://codereview.chromium.org/2162533002/diff/180001/components/ntp_snippet...
components/ntp_snippets/request_throttler_unittest.cc:29: throttler_->quota_ =
kCounterQuota;
On 2016/07/26 10:05:07, jkrcal wrote:
> Marc, moving the quota into the RequestTypeInfo complicated the unit test. Is
> accessing a private field from a friend class ok or is a new public method
> SetQuotaForTest() preferred?

I guess it's a matter of preference; I've seen both and am okay with either. I
slightly prefer the friend method, i.e. what you've done here.

[commit-bot: I haz the power, 2016-07-26 11:15:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-26 11:15:46]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2016-07-26 11:23:16]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2016-07-26 11:23:16]

The patchset sent to the CQ was uploaded after l-g-t-m from noyau@chromium.org
Link to the patchset: https://codereview.chromium.org/2162533002/#ps200001
(title: "Bernhard's comments")

[commit-bot: I haz the power, 2016-07-26 11:23:30]

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, 2016-07-26 11:27:09]

Description was changed from

==========
Adding a request counter to the snippets fetcher.

This CL applies the RequestCounter to NTPSnippetsFetcher. 

BUG=627073
==========

to

==========
Adding a request counter to the snippets fetcher.

This CL applies the RequestCounter to NTPSnippetsFetcher. 

BUG=627073
==========

[commit-bot: I haz the power, 2016-07-26 11:27:10]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-07-26 11:29:10]

Description was changed from

==========
Adding a request counter to the snippets fetcher.

This CL applies the RequestCounter to NTPSnippetsFetcher. 

BUG=627073
==========

to

==========
Adding a request counter to the snippets fetcher.

This CL applies the RequestCounter to NTPSnippetsFetcher.

BUG=627073

Committed: https://crrev.com/2d194db915724d87d566e558d87296a4f6d367da
Cr-Commit-Position: refs/heads/master@{#407769}
==========

[commit-bot: I haz the power, 2016-07-26 11:29:11]

Patchset 11 (id:??) landed as
https://crrev.com/2d194db915724d87d566e558d87296a4f6d367da
Cr-Commit-Position: refs/heads/master@{#407769}