Use improved VariationParamsManager to hide details.

Use improved VariationParamsManager to hide details.

Resolved a TODO: The testing::variations::VariationParamsManager
replaces a mock.

In order to enable Feature-associated params, the tested RequestBuilder
needed to be mocked. Since CL 645447, Feature-association works now and
exposing implementation details and mocking is now unnecessary.

BUG=634892
Committed: https://crrev.com/d252262639919ba0dedc8a35d737ac79c53c624d
Cr-Commit-Position: refs/heads/master@{#438122}

[fhorschig, 2016-12-06 16:07:39]

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

[commit-bot: I haz the power, 2016-12-06 16:07:49]

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

[fhorschig, 2016-12-06 16:08:58]

fhorschig@chromium.org changed reviewers:
+ jkrcal@chromium.org, tschumann@chromium.org

[fhorschig, 2016-12-06 16:08:59]

Would you please take a look at this small, TODO-resolving and test-improving
CL?

[commit-bot: I haz the power, 2016-12-06 16:47:03]

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

[commit-bot: I haz the power, 2016-12-06 16:47:03]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2016-12-06 16:50:48]

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

https://codereview.chromium.org/2552813005/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:212:
std::set<std::string>(
Do you need to wrap the initializer list with "std::set<std::string>()"?

https://codereview.chromium.org/2552813005/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:310:
std::unique_ptr<variations::testing::VariationParamsManager> params_manager_;
What do you mean by "resets properly"? Cannot you call ClearAllVariationParams()
and SetVariationParamsWithFeatureAssociations() in
SetFetchingPersonalizationVariation()?

[tschumann, 2016-12-06 17:58:16]

lgtm

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

https://codereview.chromium.org/2552813005/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:201:
NTPSnippetsFetcherTest()
nit: As you're touching this code, can we clean up the class hierarchy?

I'm not a huge fan of subclassing tests; in this case it's actually easy enough,
assuming we make that clear.
We should rename this class to NTPSnippetsFetcherTestBase (we can delete the
default constructor on that one) and then have two very simple subclasses,
NTPSnippetsFetcherTest (bonus points for renaming to something like
ChromeReaderSnippetsFetcherTest) and NTPSnippetsContentSuggestionsFetcherTest.

[fhorschig, 2016-12-12 14:16:38]

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

https://codereview.chromium.org/2552813005/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:201:
NTPSnippetsFetcherTest()
On 2016/12/06 17:58:16, tschumann wrote:
> nit: As you're touching this code, can we clean up the class hierarchy?
> 
> I'm not a huge fan of subclassing tests; in this case it's actually easy
enough,
> assuming we make that clear.
> We should rename this class to NTPSnippetsFetcherTestBase (we can delete the
> default constructor on that one) and then have two very simple subclasses,
> NTPSnippetsFetcherTest (bonus points for renaming to something like
> ChromeReaderSnippetsFetcherTest) and NTPSnippetsContentSuggestionsFetcherTest.

Makes sense. Done.

https://codereview.chromium.org/2552813005/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:212:
std::set<std::string>(
On 2016/12/06 16:50:47, jkrcal wrote:
> Do you need to wrap the initializer list with "std::set<std::string>()"?

For the unique_ptr: Sadly, yes.

https://codereview.chromium.org/2552813005/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:310:
std::unique_ptr<variations::testing::VariationParamsManager> params_manager_;
On 2016/12/06 16:50:47, jkrcal wrote:
> What do you mean by "resets properly"? Cannot you call
ClearAllVariationParams()
> and SetVariationParamsWithFeatureAssociations() in
> SetFetchingPersonalizationVariation()?

Thanks for pointing this out! This is the big vision after crbug.com/672010 is
fixed. Currently, I have to destroy the manager.

[jkrcal, 2016-12-12 14:27:15]

lgtm

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

https://codereview.chromium.org/2552813005/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:212:
std::set<std::string>(
On 2016/12/12 14:16:37, fhorschig wrote:
> On 2016/12/06 16:50:47, jkrcal wrote:
> > Do you need to wrap the initializer list with "std::set<std::string>()"?
> 
> For the unique_ptr: Sadly, yes.

Interesting :)

[fhorschig, 2016-12-12 14:46:56]

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

[commit-bot: I haz the power, 2016-12-12 14:47:20]

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-12-12 15:44:05]

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

[commit-bot: I haz the power, 2016-12-12 15:44:06]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[fhorschig, 2016-12-12 16:45:45]

Patchset #3 (id:40001) has been deleted

[fhorschig, 2016-12-13 10:13:45]

The CQ bit was checked by fhorschig@chromium.org

[fhorschig, 2016-12-13 10:13:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
tschumann@chromium.org, jkrcal@chromium.org
Link to the patchset: https://codereview.chromium.org/2552813005/#ps60001
(title: "Rebased,")

[commit-bot: I haz the power, 2016-12-13 10:14:00]

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-12-13 11:05:21]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481624025176290,
"parent_rev": "b1dad9491ff34b3b63a0bb8fa76d9fced4b26923", "commit_rev":
"995647973bc46a2bd784e3100428103d9534b82b"}

[commit-bot: I haz the power, 2016-12-13 11:05:52]

Description was changed from

==========
Use improved VariationParamsManager to hide details.

Resolved a TODO: The testing::variations::VariationParamsManager
replaces a mock.

In order to enable Feature-associated params, the tested RequestBuilder
needed to be mocked. Since CL 645447, Feature-association works now and
exposing implementation details and mocking is now unnecessary.

BUG=634892
==========

to

==========
Use improved VariationParamsManager to hide details.

Resolved a TODO: The testing::variations::VariationParamsManager
replaces a mock.

In order to enable Feature-associated params, the tested RequestBuilder
needed to be mocked. Since CL 645447, Feature-association works now and
exposing implementation details and mocking is now unnecessary.

BUG=634892

Review-Url: https://codereview.chromium.org/2552813005
==========

[commit-bot: I haz the power, 2016-12-13 11:05:53]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-12-13 11:08:51]

Description was changed from

==========
Use improved VariationParamsManager to hide details.

Resolved a TODO: The testing::variations::VariationParamsManager
replaces a mock.

In order to enable Feature-associated params, the tested RequestBuilder
needed to be mocked. Since CL 645447, Feature-association works now and
exposing implementation details and mocking is now unnecessary.

BUG=634892

Review-Url: https://codereview.chromium.org/2552813005
==========

to

==========
Use improved VariationParamsManager to hide details.

Resolved a TODO: The testing::variations::VariationParamsManager
replaces a mock.

In order to enable Feature-associated params, the tested RequestBuilder
needed to be mocked. Since CL 645447, Feature-association works now and
exposing implementation details and mocking is now unnecessary.

BUG=634892

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

[commit-bot: I haz the power, 2016-12-13 11:08:52]

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