NTP Snippets: Added error codes for out of quota events

NTP Snippets: Added error codes in case snippet fetcher runs out of quota.

BUG=627073
Committed: https://crrev.com/182c7f2784883ce786cb81ce4f6ec3ab77a7d6f5
Cr-Commit-Position: refs/heads/master@{#414671}

[finkm, 2016-08-25 11:51:37]

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

[finkm, 2016-08-25 11:51:54]

PTAL

[Marc Treib, 2016-08-25 11:58:27]

https://codereview.chromium.org/2273323002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/2273323002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_fetcher.cc:602: result !=
FetchResult::OAUTH_TOKEN_ERROR) {
You should probably add the new errors here, since we want to record fetch time
only for actual network requests. Or, better: Make a new "IsNetworkError" (or
something like that) helper function.

https://codereview.chromium.org/2273323002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_fetcher.h (right):

https://codereview.chromium.org/2273323002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_fetcher.h:59: // and update the histogram
definition.
^^ You'll have to update histograms.xml too

[finkm, 2016-08-25 13:15:52]

On 2016/08/25 at 11:58:27, treib wrote:
>
https://codereview.chromium.org/2273323002/diff/1/components/ntp_snippets/ntp...
> File components/ntp_snippets/ntp_snippets_fetcher.cc (right):
> 
>
https://codereview.chromium.org/2273323002/diff/1/components/ntp_snippets/ntp...
> components/ntp_snippets/ntp_snippets_fetcher.cc:602: result !=
FetchResult::OAUTH_TOKEN_ERROR) {
> You should probably add the new errors here, since we want to record fetch
time only for actual network requests. Or, better: Make a new "IsNetworkError"
(or something like that) helper function.
Done. See new isFetchPreconditionFailed function
> 
>
https://codereview.chromium.org/2273323002/diff/1/components/ntp_snippets/ntp...
> File components/ntp_snippets/ntp_snippets_fetcher.h (right):
> 
>
https://codereview.chromium.org/2273323002/diff/1/components/ntp_snippets/ntp...
> components/ntp_snippets/ntp_snippets_fetcher.h:59: // and update the histogram
definition.
> ^^ You'll have to update histograms.xml too
argh ... done.

[Marc Treib, 2016-08-25 14:19:05]

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

https://codereview.chromium.org/2273323002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.cc:100: bool
isFetchPreconditionFailed(NTPSnippetsFetcher::FetchResult result) {
nit: IsFetchPreconditionFailed (start with a capital letter)

https://codereview.chromium.org/2273323002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.cc:107: default:
Instead of a default, I'd prefer to list the values explicitly. Then, when
someone adds a new value but doesn't list it here, they'll get a compile error.

https://codereview.chromium.org/2273323002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.cc:241:
FetchResult::INTERACTIVE_QUOTA_ERROR :
nit: wrong indentation I think? (You could try "git cl format")

[finkm, 2016-08-25 14:36:24]

On 2016/08/25 at 14:19:05, treib wrote:
>
https://codereview.chromium.org/2273323002/diff/40001/components/ntp_snippets...
> File components/ntp_snippets/ntp_snippets_fetcher.cc (right):
> 
>
https://codereview.chromium.org/2273323002/diff/40001/components/ntp_snippets...
> components/ntp_snippets/ntp_snippets_fetcher.cc:100: bool
isFetchPreconditionFailed(NTPSnippetsFetcher::FetchResult result) {
> nit: IsFetchPreconditionFailed (start with a capital letter)
Done.
> 
>
https://codereview.chromium.org/2273323002/diff/40001/components/ntp_snippets...
> components/ntp_snippets/ntp_snippets_fetcher.cc:107: default:
> Instead of a default, I'd prefer to list the values explicitly. Then, when
someone adds a new value but doesn't list it here, they'll get a compile error.
Done.
> 
>
https://codereview.chromium.org/2273323002/diff/40001/components/ntp_snippets...
> components/ntp_snippets/ntp_snippets_fetcher.cc:241:
FetchResult::INTERACTIVE_QUOTA_ERROR :
> nit: wrong indentation I think? (You could try "git cl format")
Done.

[Marc Treib, 2016-08-25 14:41:06]

LGTM, thanks! (You'll also need approval from a metrics owner for the histograms
change.)

Description nit: You could mention "NTP Snippets" or something like that.
That'll make live easier for anyone who has to search the commit log for
anything.

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

https://codereview.chromium.org/2273323002/diff/60001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.cc:112: case
NTPSnippetsFetcher::FetchResult::RESULT_MAX:
nit: RESULT_MAX should never happen, so you could just "break;" in that case, so
it'll hit the NOTREACHED() below.

[finkm, 2016-08-25 14:59:07]

Description was changed from

==========
Added error codes for out of quota events

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

to

==========
NTP Snippets: Added error codes in case snippet fetcher runs out of quota.

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

[finkm, 2016-08-25 14:59:07]

finkm@chromium.org changed reviewers:
+ jwd@chromium.org

[finkm, 2016-08-25 14:59:56]

+ jwd for histograms.xml

[jwd, 2016-08-25 17:28:33]

lgtm

[finkm, 2016-08-25 17:48:45]

The CQ bit was checked by finkm@chromium.org

[commit-bot: I haz the power, 2016-08-25 17:49:29]

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-08-25 20:57:23]

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

[commit-bot: I haz the power, 2016-08-25 20:57:24]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[finkm, 2016-08-26 07:16:31]

The CQ bit was checked by finkm@chromium.org

[commit-bot: I haz the power, 2016-08-26 07:16:51]

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-08-26 08:17:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-26 08:20:45]

Description was changed from

==========
NTP Snippets: Added error codes in case snippet fetcher runs out of quota.

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

to

==========
NTP Snippets: Added error codes in case snippet fetcher runs out of quota.

BUG=627073

Committed: https://crrev.com/182c7f2784883ce786cb81ce4f6ec3ab77a7d6f5
Cr-Commit-Position: refs/heads/master@{#414671}
==========

[commit-bot: I haz the power, 2016-08-26 08:20:46]

Patchset 4 (id:??) landed as
https://crrev.com/182c7f2784883ce786cb81ce4f6ec3ab77a7d6f5
Cr-Commit-Position: refs/heads/master@{#414671}