[NTP Snippets] Clean up NTPSnippetsFetcher

[NTP Snippets] Clean up NTPSnippetsFetcher

BUG=659931
Committed: https://crrev.com/225c59cb046226570ade877bc1bc09de83985aa4
Cr-Commit-Position: refs/heads/master@{#428020}

[Marc Treib, 2016-10-27 11:46:28]

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

[Marc Treib, 2016-10-27 11:46:29]

PTAL!

[Bernhard Bauer, 2016-10-27 12:13:18]

lgtm

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

https://codereview.chromium.org/2454063002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.h:190:
translate::LanguageModel::LanguageInfo ui_language{"", 0.0f};
I thought we fixed that?

https://codereview.chromium.org/2454063002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.h:238: // When a token
request gets canceled, we want to retry once.
Nit: empty line before comments please :)

https://codereview.chromium.org/2454063002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.h:239: bool
oauth_token_retried_;
Initialize to false?

[Marc Treib, 2016-10-27 13:14:58]

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

https://codereview.chromium.org/2454063002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.h:190:
translate::LanguageModel::LanguageInfo ui_language{"", 0.0f};
On 2016/10/27 12:13:18, Bernhard Bauer wrote:
> I thought we fixed that?

Almost - https://codereview.chromium.org/2449873003/ is still waiting for an
approval. I'll rebase it on top of this one after this one lands.

https://codereview.chromium.org/2454063002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.h:238: // When a token
request gets canceled, we want to retry once.
On 2016/10/27 12:13:18, Bernhard Bauer wrote:
> Nit: empty line before comments please :)

Done.

https://codereview.chromium.org/2454063002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.h:239: bool
oauth_token_retried_;
On 2016/10/27 12:13:18, Bernhard Bauer wrote:
> Initialize to false?

It is initialized to false, in the ctor's initializer list, like all the other
members here. I like to keep things consistent :)

That said, we could probably move most of the initialization directly to the
member declarations. Separate CL though.

[Marc Treib, 2016-10-27 13:15:03]

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

[commit-bot: I haz the power, 2016-10-27 13:15:19]

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-10-27 13:25:31]

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

https://codereview.chromium.org/2454063002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.h:239: bool
oauth_token_retried_;
On 2016/10/27 13:14:58, Marc Treib wrote:
> On 2016/10/27 12:13:18, Bernhard Bauer wrote:
> > Initialize to false?
> 
> It is initialized to false, in the ctor's initializer list, like all the other
> members here. I like to keep things consistent :)
> 
> That said, we could probably move most of the initialization directly to the
> member declarations. Separate CL though.

Isn't that what you're doing in a bunch of other places in this CL as well?

[Marc Treib, 2016-10-27 13:31:32]

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

https://codereview.chromium.org/2454063002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.h:239: bool
oauth_token_retried_;
On 2016/10/27 13:25:31, Bernhard Bauer wrote:
> On 2016/10/27 13:14:58, Marc Treib wrote:
> > On 2016/10/27 12:13:18, Bernhard Bauer wrote:
> > > Initialize to false?
> > 
> > It is initialized to false, in the ctor's initializer list, like all the
other
> > members here. I like to keep things consistent :)
> > 
> > That said, we could probably move most of the initialization directly to the
> > member declarations. Separate CL though.
> 
> Isn't that what you're doing in a bunch of other places in this CL as well?

I've changed the params/builder structs, but not the main class itself - that's
kind of a separate thing to me (e.g. because we won't be able to move *all* the
initialization into the header). But if you feel strongly, I can move the things
that can be moved?

[Bernhard Bauer, 2016-10-27 13:34:58]

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

https://codereview.chromium.org/2454063002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.h:239: bool
oauth_token_retried_;
On 2016/10/27 13:31:31, Marc Treib wrote:
> On 2016/10/27 13:25:31, Bernhard Bauer wrote:
> > On 2016/10/27 13:14:58, Marc Treib wrote:
> > > On 2016/10/27 12:13:18, Bernhard Bauer wrote:
> > > > Initialize to false?
> > > 
> > > It is initialized to false, in the ctor's initializer list, like all the
> other
> > > members here. I like to keep things consistent :)
> > > 
> > > That said, we could probably move most of the initialization directly to
the
> > > member declarations. Separate CL though.
> > 
> > Isn't that what you're doing in a bunch of other places in this CL as well?
> 
> I've changed the params/builder structs, but not the main class itself -
that's
> kind of a separate thing to me (e.g. because we won't be able to move *all*
the
> initialization into the header). But if you feel strongly, I can move the
things
> that can be moved?

Personally, I think that sort of thing would fit in nicely with a cleanup CL
like this, but I don't really feel that strongly.

[Marc Treib, 2016-10-27 13:53:01]

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

https://codereview.chromium.org/2454063002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher.h:239: bool
oauth_token_retried_;
On 2016/10/27 13:34:58, Bernhard Bauer wrote:
> On 2016/10/27 13:31:31, Marc Treib wrote:
> > On 2016/10/27 13:25:31, Bernhard Bauer wrote:
> > > On 2016/10/27 13:14:58, Marc Treib wrote:
> > > > On 2016/10/27 12:13:18, Bernhard Bauer wrote:
> > > > > Initialize to false?
> > > > 
> > > > It is initialized to false, in the ctor's initializer list, like all the
> > other
> > > > members here. I like to keep things consistent :)
> > > > 
> > > > That said, we could probably move most of the initialization directly to
> the
> > > > member declarations. Separate CL though.
> > > 
> > > Isn't that what you're doing in a bunch of other places in this CL as
well?
> > 
> > I've changed the params/builder structs, but not the main class itself -
> that's
> > kind of a separate thing to me (e.g. because we won't be able to move *all*
> the
> > initialization into the header). But if you feel strongly, I can move the
> things
> > that can be moved?
> 
> Personally, I think that sort of thing would fit in nicely with a cleanup CL
> like this, but I don't really feel that strongly.

Hm. TBH, I'm not a big fan of splitting initialization like this (half in the
header, half in the ctor initializer list), but maybe it's okay for these "state
bools" (which are ugly anyway...)
I thought the style guide had some guidance on this, but I can't find anything.
Oh well. Done, for these two bools.

[Marc Treib, 2016-10-27 13:53:06]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-10-27 13:53:06]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2454063002/#ps60001
(title: "review 2")

[commit-bot: I haz the power, 2016-10-27 13:53:32]

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-10-27 14:40:15]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-27 14:42:27]

Description was changed from

==========
[NTP Snippets] Clean up NTPSnippetsFetcher

BUG=659931
==========

to

==========
[NTP Snippets] Clean up NTPSnippetsFetcher

BUG=659931

Committed: https://crrev.com/225c59cb046226570ade877bc1bc09de83985aa4
Cr-Commit-Position: refs/heads/master@{#428020}
==========

[commit-bot: I haz the power, 2016-10-27 14:42:28]

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