Getting the personalization info in chrome://snippets-internals correct.

Getting the personalization settings in chrome://snippets-internals display correctly.

The personalization settings have been changed by recent CLs, this CL keeps snippets-internals up-to-date.

BUG=603907, 606320
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/a4df8ada22f3d15ad7153a369c780795797dfa1d
Cr-Commit-Position: refs/heads/master@{#394794}

[jkrcal, 2016-05-12 14:38:05]

Description was changed from

==========
Getting the personalization info in chrome://snippets-internals correct.

BUG=603907,606320
==========

to

==========
Getting the personalization info in chrome://snippets-internals correct.

BUG=603907,606320
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[jkrcal, 2016-05-17 14:46:33]

Description was changed from

==========
Getting the personalization info in chrome://snippets-internals correct.

BUG=603907,606320
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Getting the personalization settings in chrome://snippets-internals display
correctly.

The personalization settings have been changed by recent CLs, this CL keeps
snippets-internals up-to-date.

BUG=603907,606320
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[jkrcal, 2016-05-17 14:46:53]

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

[jkrcal, 2016-05-17 14:46:54]

PTAL.

[Bernhard Bauer, 2016-05-18 10:08:04]

https://codereview.chromium.org/1978513002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/1978513002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:170:
"chrome.SnippetsInternals.hideHostRestricts");
If you add a dedicated method here anyway, you could pass |host_restricted| in
as a parameter and update switch-restrict-to-hosts from there, so you only have
a single call here.

https://codereview.chromium.org/1978513002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:176:
SendString("switch-personalized", "Only personalized");
Use a switch statement so the compiler can check you've covered all the cases
(this implies explicitly writing all of values instead of using a default:
fallback).

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

https://codereview.chromium.org/1978513002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:68: // Enumaration listing all
possible variants of dealing with personalization.
"Enumeration"

https://codereview.chromium.org/1978513002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:69: enum class Personalization {
kPersonal, kNonPersonal, kBoth };
Put the values on individual lines :)

https://codereview.chromium.org/1978513002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:101: // Returns the last json
fetched from the server.
Nit: JSON

https://codereview.chromium.org/1978513002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:102: const Personalization
personalization() const { return personalization_; }
Const for a return type that is by itself immutable is unnecessary.

https://codereview.chromium.org/1978513002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:105: bool UseHostRestriction()
const;
Nit: "host restrictions" (plural). Also, "uses", so it isn't confused with an
imperative.

https://codereview.chromium.org/1978513002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:107: bool UseAuthentication()
const;
"Uses" here as well.

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

https://codereview.chromium.org/1978513002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:102: return
snippets_fetcher_->UseAuthentication();
I think at this point it might be easier to just return the |snippets_fetcher_|.

[jkrcal, 2016-05-19 07:30:28]

Thanks for the comments!
PTAL, again.

https://codereview.chromium.org/1978513002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/1978513002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:170:
"chrome.SnippetsInternals.hideHostRestricts");
On 2016/05/18 10:08:03, Bernhard Bauer wrote:
> If you add a dedicated method here anyway, you could pass |host_restricted| in
> as a parameter and update switch-restrict-to-hosts from there, so you only
have
> a single call here.

Done.

https://codereview.chromium.org/1978513002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:176:
SendString("switch-personalized", "Only personalized");
On 2016/05/18 10:08:03, Bernhard Bauer wrote:
> Use a switch statement so the compiler can check you've covered all the cases
> (this implies explicitly writing all of values instead of using a default:
> fallback).

Done.

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

https://codereview.chromium.org/1978513002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:68: // Enumaration listing all
possible variants of dealing with personalization.
On 2016/05/18 10:08:03, Bernhard Bauer wrote:
> "Enumeration"

Done.

https://codereview.chromium.org/1978513002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:69: enum class Personalization {
kPersonal, kNonPersonal, kBoth };
On 2016/05/18 10:08:04, Bernhard Bauer wrote:
> Put the values on individual lines :)

One line is what "git cl format" produces. Should I fight against it? ;)

https://codereview.chromium.org/1978513002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:101: // Returns the last json
fetched from the server.
On 2016/05/18 10:08:04, Bernhard Bauer wrote:
> Nit: JSON

Done.

https://codereview.chromium.org/1978513002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:102: const Personalization
personalization() const { return personalization_; }
On 2016/05/18 10:08:04, Bernhard Bauer wrote:
> Const for a return type that is by itself immutable is unnecessary.

Thanks. Interestingly, it even does not compile on iOS.

https://codereview.chromium.org/1978513002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:105: bool UseHostRestriction()
const;
On 2016/05/18 10:08:03, Bernhard Bauer wrote:
> Nit: "host restrictions" (plural). Also, "uses", so it isn't confused with an
> imperative.

Done.

https://codereview.chromium.org/1978513002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:107: bool UseAuthentication()
const;
On 2016/05/18 10:08:03, Bernhard Bauer wrote:
> "Uses" here as well.

Done.

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

https://codereview.chromium.org/1978513002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:102: return
snippets_fetcher_->UseAuthentication();
On 2016/05/18 10:08:04, Bernhard Bauer wrote:
> I think at this point it might be easier to just return the
|snippets_fetcher_|.

Done.

[jkrcal, 2016-05-19 08:14:49]

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

[commit-bot: I haz the power, 2016-05-19 08:15:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1978513002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1978513002/60001

[Bernhard Bauer, 2016-05-19 08:51:19]

LGTM w/ nits:

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

https://codereview.chromium.org/1978513002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:69: enum class Personalization {
kPersonal, kNonPersonal, kBoth };
On 2016/05/19 07:30:28, jkrcal wrote:
> On 2016/05/18 10:08:04, Bernhard Bauer wrote:
> > Put the values on individual lines :)
> 
> One line is what "git cl format" produces. Should I fight against it? ;)

Yeah, it would be nice. I don't think clang-format grasps the concept of trying
to avoid churn when adding more values.

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

https://codereview.chromium.org/1978513002/diff/60001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:88: NTPSnippetsFetcher*
snippets_fetcher() const {
You could return a const pointer here so it can be only used for reading.

[commit-bot: I haz the power, 2016-05-19 09:23:00]

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

[commit-bot: I haz the power, 2016-05-19 09:23:01]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2016-05-19 10:58:17]

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

https://codereview.chromium.org/1978513002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:69: enum class Personalization {
kPersonal, kNonPersonal, kBoth };
On 2016/05/19 08:51:19, Bernhard Bauer wrote:
> On 2016/05/19 07:30:28, jkrcal wrote:
> > On 2016/05/18 10:08:04, Bernhard Bauer wrote:
> > > Put the values on individual lines :)
> > 
> > One line is what "git cl format" produces. Should I fight against it? ;)
> 
> Yeah, it would be nice. I don't think clang-format grasps the concept of
trying
> to avoid churn when adding more values.

I am afraid I do not understand. Do you suggest me to put it on separate lines
and _not_ run "git cl format" afterwards?

Is there some way I can force this format to persist in the future (when someone
else calls git cl format)?

[Bernhard Bauer, 2016-05-19 11:05:46]

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

https://codereview.chromium.org/1978513002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:69: enum class Personalization {
kPersonal, kNonPersonal, kBoth };
On 2016/05/19 10:58:16, jkrcal wrote:
> On 2016/05/19 08:51:19, Bernhard Bauer wrote:
> > On 2016/05/19 07:30:28, jkrcal wrote:
> > > On 2016/05/18 10:08:04, Bernhard Bauer wrote:
> > > > Put the values on individual lines :)
> > > 
> > > One line is what "git cl format" produces. Should I fight against it? ;)
> > 
> > Yeah, it would be nice. I don't think clang-format grasps the concept of
> trying
> > to avoid churn when adding more values.
> 
> I am afraid I do not understand. Do you suggest me to put it on separate lines
> and _not_ run "git cl format" afterwards?

That is correct. There are areas of the code base that enforce clang-format
cleanness, but this area doesn't, and generally clang-format does not override
style decisions (or the style guide).

> Is there some way I can force this format to persist in the future (when
someone
> else calls git cl format)?

Not that I know. (You could try making the names longer of course ;-) )

[jkrcal, 2016-05-19 13:14:32]

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

[commit-bot: I haz the power, 2016-05-19 13:14:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1978513002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1978513002/80001

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

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

[commit-bot: I haz the power, 2016-05-19 14:27:11]

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

[jkrcal, 2016-05-19 16:36:25]

Thanks, Bernhard!

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

https://codereview.chromium.org/1978513002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:69: enum class Personalization {
kPersonal, kNonPersonal, kBoth };
On 2016/05/19 11:05:46, Bernhard Bauer wrote:
> On 2016/05/19 10:58:16, jkrcal wrote:
> > On 2016/05/19 08:51:19, Bernhard Bauer wrote:
> > > On 2016/05/19 07:30:28, jkrcal wrote:
> > > > On 2016/05/18 10:08:04, Bernhard Bauer wrote:
> > > > > Put the values on individual lines :)
> > > > 
> > > > One line is what "git cl format" produces. Should I fight against it? ;)
> > > 
> > > Yeah, it would be nice. I don't think clang-format grasps the concept of
> > trying
> > > to avoid churn when adding more values.
> > 
> > I am afraid I do not understand. Do you suggest me to put it on separate
lines
> > and _not_ run "git cl format" afterwards?
> 
> That is correct. There are areas of the code base that enforce clang-format
> cleanness, but this area doesn't, and generally clang-format does not override
> style decisions (or the style guide).
> 
> > Is there some way I can force this format to persist in the future (when
> someone
> > else calls git cl format)?
> 
> Not that I know. (You could try making the names longer of course ;-) )

Done.

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

https://codereview.chromium.org/1978513002/diff/60001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:88: NTPSnippetsFetcher*
snippets_fetcher() const {
On 2016/05/19 08:51:19, Bernhard Bauer wrote:
> You could return a const pointer here so it can be only used for reading.

Done.

[jkrcal, 2016-05-19 16:36:36]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2016-05-19 16:36:37]

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/1978513002/#ps80001
(title: "After code review #2")

[commit-bot: I haz the power, 2016-05-19 16:37:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1978513002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1978513002/80001

[commit-bot: I haz the power, 2016-05-19 16:41:17]

Description was changed from

==========
Getting the personalization settings in chrome://snippets-internals display
correctly.

The personalization settings have been changed by recent CLs, this CL keeps
snippets-internals up-to-date.

BUG=603907,606320
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Getting the personalization settings in chrome://snippets-internals display
correctly.

The personalization settings have been changed by recent CLs, this CL keeps
snippets-internals up-to-date.

BUG=603907,606320
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-05-19 16:41:18]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-19 16:43:55]

Description was changed from

==========
Getting the personalization settings in chrome://snippets-internals display
correctly.

The personalization settings have been changed by recent CLs, this CL keeps
snippets-internals up-to-date.

BUG=603907,606320
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Getting the personalization settings in chrome://snippets-internals display
correctly.

The personalization settings have been changed by recent CLs, this CL keeps
snippets-internals up-to-date.

BUG=603907,606320
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/a4df8ada22f3d15ad7153a369c780795797dfa1d
Cr-Commit-Position: refs/heads/master@{#394794}
==========

[commit-bot: I haz the power, 2016-05-19 16:43:56]

Patchset 5 (id:??) landed as
https://crrev.com/a4df8ada22f3d15ad7153a369c780795797dfa1d
Cr-Commit-Position: refs/heads/master@{#394794}