[Remote suggestions] Clean up variation params in the fetcher.

[Remote suggestions] Clean up variation params in the fetcher.

This CL removes one obsolete variation param and associate another one by
the feature name.

BUG=686778
Review-Url: https://codereview.chromium.org/2665743002
Cr-Commit-Position: refs/heads/master@{#447992}
Committed: https://chromium.googlesource.com/chromium/src/+/f18de6670998bc95c8cfda363cb84291471f2394

[jkrcal, 2017-01-30 16:57:46]

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

[jkrcal, 2017-01-30 16:57:47]

Marc, here we go! (part 1)

[Marc Treib, 2017-01-30 17:02:57]

Yay for removing the personalization param!
One concern with re-associating the other param; maybe you've already thought
about this?

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

https://codereview.chromium.org/2665743002/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/remote_suggestions_fetcher.cc:115:
ntp_snippets::kArticleSuggestionsFeature, kContentSuggestionsBackend);
This will break the current configs, right?

Should we maybe try looking it up by feature, and if that fails, fall back to
study name? Or handle it all server-side in the configs? WDYT?

[jkrcal, 2017-01-30 17:11:02]

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

https://codereview.chromium.org/2665743002/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/remote_suggestions_fetcher.cc:115:
ntp_snippets::kArticleSuggestionsFeature, kContentSuggestionsBackend);
On 2017/01/30 17:02:57, Marc Treib wrote:
> This will break the current configs, right?
> 
> Should we maybe try looking it up by feature, and if that fails, fall back to
> study name? Or handle it all server-side in the configs? WDYT?

I would handle it in the configs, server-side.

After the new config lands, it will affect the Canary/Dev config. As the config
anyway specifies the default end-point, it will not break anything. I will adapt
the canary/dev config afterwards and move it into the NTPArticle... file.

We only need to keep it in mind when preparing configs for M58 Beta / Stable. I
will add comments to the respective finch config files to make sure we do not
forget.

Does it make sense?

[jkrcal, 2017-01-30 17:20:56]

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

[commit-bot: I haz the power, 2017-01-30 17:21:19]

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, 2017-01-30 17:27:44]

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

[commit-bot: I haz the power, 2017-01-30 17:27:45]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[jkrcal, 2017-01-30 17:41:25]

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

[commit-bot: I haz the power, 2017-01-30 17:41:48]

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, 2017-01-30 18:20:40]

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

[commit-bot: I haz the power, 2017-01-30 18:20: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_...)

[Marc Treib, 2017-01-31 09:01:43]

Thanks! LGTM once you fix the failing tests ;)

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

https://codereview.chromium.org/2665743002/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/remote_suggestions_fetcher.cc:115:
ntp_snippets::kArticleSuggestionsFeature, kContentSuggestionsBackend);
On 2017/01/30 17:11:01, jkrcal wrote:
> On 2017/01/30 17:02:57, Marc Treib wrote:
> > This will break the current configs, right?
> > 
> > Should we maybe try looking it up by feature, and if that fails, fall back
to
> > study name? Or handle it all server-side in the configs? WDYT?
> 
> I would handle it in the configs, server-side.
> 
> After the new config lands, it will affect the Canary/Dev config. As the
config
> anyway specifies the default end-point, it will not break anything. I will
adapt
> the canary/dev config afterwards and move it into the NTPArticle... file.
> 
> We only need to keep it in mind when preparing configs for M58 Beta / Stable.
I
> will add comments to the respective finch config files to make sure we do not
> forget.
> 
> Does it make sense?

Actually on second thought, since the ZineServer endpoint is the default now
anyway (I think already in M57), we should just remove the param from the
Dev/Canary configs.

[jkrcal, 2017-02-03 09:14:24]

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

[commit-bot: I haz the power, 2017-02-03 09:14:32]

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

[jkrcal, 2017-02-03 09:16:05]

The CQ bit was unchecked by jkrcal@chromium.org

[jkrcal, 2017-02-03 09:20:27]

Patchset #3 (id:40001) has been deleted

[jkrcal, 2017-02-03 09:23:13]

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

[commit-bot: I haz the power, 2017-02-03 09:23:25]

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

[jkrcal, 2017-02-03 10:17:49]

Marc, please take another look.

https://codereview.chromium.org/2665743002/diff/80001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (left):

https://codereview.chromium.org/2665743002/diff/80001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:543: const FeatureEntry::FeatureParam
I've forgotten to remove this in the previous patch list.

https://codereview.chromium.org/2665743002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
(left):

https://codereview.chromium.org/2665743002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:419:
utils_.fake_signin_manager()->SignIn("foo@bar.com");
I emulate the previous (not-signed-in) behaviour) by not being signed-in.

https://codereview.chromium.org/2665743002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
(right):

https://codereview.chromium.org/2665743002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:95:
"https://alpha-chromecontentsuggestions-pa.sandbox.googleapis.com/v1/"
Changed this to avoid warnings from
RemoteSuggestionsFetcher::UsesChromeContentSuggestionsAPI().

https://codereview.chromium.org/2665743002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:377:
fake_token_service_(nullptr),
Added to increase clarity that null token service is provided to the fetcher
(since we are not signed-in, it is not used and does not crash).

Update: This is silly. I will remove it completely and inline the nullptr in
another patch set.

[Marc Treib, 2017-02-03 10:52:08]

Still LGTM, with one request for a TODO/bug

https://codereview.chromium.org/2665743002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
(left):

https://codereview.chromium.org/2665743002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:419:
utils_.fake_signin_manager()->SignIn("foo@bar.com");
On 2017/02/03 10:17:49, jkrcal wrote:
> I emulate the previous (not-signed-in) behaviour) by not being signed-in.

So the previous state was signed-in, but personalization turned off, so the
sign-in state didn't actually do anything? Awesome...

https://codereview.chromium.org/2665743002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
(right):

https://codereview.chromium.org/2665743002/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:415:
utils_.fake_signin_manager(), /*token_service=*/nullptr,
Does this work without any "may be null in tests" in the fetcher?!
I guess due to the non-signed-in state, the fetcher never actually does anything
with the service?

I suppose this is okay for now (this CL doesn't make the situation any worse),
but we really should add tests for this. Did we have a bug already? Either way,
we should at least add a TODO, also to document the current state.

[jkrcal, 2017-02-03 11:04:16]

Thanks!

https://codereview.chromium.org/2665743002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
(right):

https://codereview.chromium.org/2665743002/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:415:
utils_.fake_signin_manager(), /*token_service=*/nullptr,
On 2017/02/03 10:52:08, Marc Treib wrote:
> Does this work without any "may be null in tests" in the fetcher?!
> I guess due to the non-signed-in state, the fetcher never actually does
anything
> with the service?
> 
> I suppose this is okay for now (this CL doesn't make the situation any worse),
> but we really should add tests for this. Did we have a bug already? Either
way,
> we should at least add a TODO, also to document the current state.

I think the tests belong to the fetcher_unittest. Added a TODO there.

[jkrcal, 2017-02-03 11:04:20]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2017-02-03 11:04:21]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2665743002/#ps120001
(title: "Add a comment")

[Marc Treib, 2017-02-03 11:10:46]

https://codereview.chromium.org/2665743002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
(right):

https://codereview.chromium.org/2665743002/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:415:
utils_.fake_signin_manager(), /*token_service=*/nullptr,
On 2017/02/03 11:04:16, jkrcal wrote:
> On 2017/02/03 10:52:08, Marc Treib wrote:
> > Does this work without any "may be null in tests" in the fetcher?!
> > I guess due to the non-signed-in state, the fetcher never actually does
> anything
> > with the service?
> > 
> > I suppose this is okay for now (this CL doesn't make the situation any
worse),
> > but we really should add tests for this. Did we have a bug already? Either
> way,
> > we should at least add a TODO, also to document the current state.
> 
> I think the tests belong to the fetcher_unittest. Added a TODO there.

That makes sense! We have a separate TODO on the Fetcher class to make it an
interface, so that we could inject a fake fetcher here.

[commit-bot: I haz the power, 2017-02-03 11:12:31]

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-02-03 14:37:24]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1486119860314100,
"parent_rev": "51583da63dae5f4c3baa9a229160b2c027afffb4", "commit_rev":
"f18de6670998bc95c8cfda363cb84291471f2394"}

[commit-bot: I haz the power, 2017-02-03 14:37:55]

Description was changed from

==========
[Remote suggestions] Clean up variation params in the fetcher.

This CL removes one obsolete variation param and associate another one by
the feature name.

BUG=686778
==========

to

==========
[Remote suggestions] Clean up variation params in the fetcher.

This CL removes one obsolete variation param and associate another one by
the feature name.

BUG=686778

Review-Url: https://codereview.chromium.org/2665743002
Cr-Commit-Position: refs/heads/master@{#447992}
Committed:
https://chromium.googlesource.com/chromium/src/+/f18de6670998bc95c8cfda363cb8...
==========

[commit-bot: I haz the power, 2017-02-03 14:37:57]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/f18de6670998bc95c8cfda363cb8...