[NTP::SectionOrder] Make dismissed category penalty a variation param.

[NTP::SectionOrder] Make dismissed category penalty a variation param.

Make dismissed category penalty a variation param instead of simply
hardcoding the value. Handle penalty equal to 0 correctly (ignore
OnCategoryDismissed call).

We may want to change the value or to disable it completely (by setting
to 0) later.

BUG=677919
Review-Url: https://codereview.chromium.org/2612773003
Cr-Commit-Position: refs/heads/master@{#443996}
Committed: https://chromium.googlesource.com/chromium/src/+/ecbf607e7aced540428687d67a9991d97d115e6e

[vitaliii, 2017-01-05 09:28:17]

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

[vitaliii, 2017-01-05 09:28:18]

Hi jkrcal@,

Could you have a look please?

https://codereview.chromium.org/2612773003/diff/1/components/ntp_snippets/cat...
File components/ntp_snippets/category_rankers/click_based_category_ranker.cc
(right):

https://codereview.chromium.org/2612773003/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_rankers/click_based_category_ranker.cc:69:
return variations::GetVariationParamByFeatureAsInt(
Are you aware of any way to change the value in tests? I would like to test
penalty = 0.

https://codereview.chromium.org/2612773003/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_rankers/click_based_category_ranker.cc:196: if
(penalty == 0) {
Is this ok in your view? Or should I consider penalty = -1 as "disabled" and
"penalty = 0" as "reduce dismissed category clicks to the next category clicks"?

[vitaliii, 2017-01-05 09:29:17]

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

[commit-bot: I haz the power, 2017-01-05 09:29:29]

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-05 10:17:58]

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

[commit-bot: I haz the power, 2017-01-05 10:17:59]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2017-01-05 10:34:37]

https://codereview.chromium.org/2612773003/diff/1/components/ntp_snippets/cat...
File components/ntp_snippets/category_rankers/click_based_category_ranker.cc
(right):

https://codereview.chromium.org/2612773003/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_rankers/click_based_category_ranker.cc:69:
return variations::GetVariationParamByFeatureAsInt(
On 2017/01/05 09:28:18, vitaliii wrote:
> Are you aware of any way to change the value in tests? I would like to test
> penalty = 0.

Sure, see variations::testing::VariationParamsManager, used, e.g., in
https://cs.chromium.org/chromium/src/components/ntp_snippets/remote/schedulin...

This may allow you to remove the function GetDismissedCategoryPenalty().

https://codereview.chromium.org/2612773003/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_rankers/click_based_category_ranker.cc:196: if
(penalty == 0) {
On 2017/01/05 09:28:18, vitaliii wrote:
> Is this ok in your view? Or should I consider penalty = -1 as "disabled" and
> "penalty = 0" as "reduce dismissed category clicks to the next category
clicks"?

This is okay, let's keep it simple.

[vitaliii, 2017-01-05 12:42:04]

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

[commit-bot: I haz the power, 2017-01-05 12:42:13]

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

[vitaliii, 2017-01-05 12:42:28]

Hi jkrcal@,

I added tests, PTAL.

https://codereview.chromium.org/2612773003/diff/1/components/ntp_snippets/cat...
File components/ntp_snippets/category_rankers/click_based_category_ranker.cc
(right):

https://codereview.chromium.org/2612773003/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_rankers/click_based_category_ranker.cc:69:
return variations::GetVariationParamByFeatureAsInt(
On 2017/01/05 10:34:36, jkrcal wrote:
> On 2017/01/05 09:28:18, vitaliii wrote:
> > Are you aware of any way to change the value in tests? I would like to test
> > penalty = 0.
> 
> Sure, see variations::testing::VariationParamsManager, used, e.g., in
>
https://cs.chromium.org/chromium/src/components/ntp_snippets/remote/schedulin...
> 
> This may allow you to remove the function GetDismissedCategoryPenalty().

Done.

GetDismissedCategoryPenalty() allows to expose the default value.

https://codereview.chromium.org/2612773003/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_rankers/click_based_category_ranker.cc:196: if
(penalty == 0) {
On 2017/01/05 10:34:36, jkrcal wrote:
> On 2017/01/05 09:28:18, vitaliii wrote:
> > Is this ok in your view? Or should I consider penalty = -1 as "disabled" and
> > "penalty = 0" as "reduce dismissed category clicks to the next category
> clicks"?
> 
> This is okay, let's keep it simple.

Acknowledged.

[commit-bot: I haz the power, 2017-01-05 13:19:56]

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

[commit-bot: I haz the power, 2017-01-05 13:19:57]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2017-01-05 13:48:08]

lgtm (choose yourself whether to address the last comment)

https://codereview.chromium.org/2612773003/diff/1/components/ntp_snippets/cat...
File components/ntp_snippets/category_rankers/click_based_category_ranker.cc
(right):

https://codereview.chromium.org/2612773003/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_rankers/click_based_category_ranker.cc:69:
return variations::GetVariationParamByFeatureAsInt(
On 2017/01/05 12:42:28, vitaliii wrote:
> On 2017/01/05 10:34:36, jkrcal wrote:
> > On 2017/01/05 09:28:18, vitaliii wrote:
> > > Are you aware of any way to change the value in tests? I would like to
test
> > > penalty = 0.
> > 
> > Sure, see variations::testing::VariationParamsManager, used, e.g., in
> >
>
https://cs.chromium.org/chromium/src/components/ntp_snippets/remote/schedulin...
> > 
> > This may allow you to remove the function GetDismissedCategoryPenalty().
> 
> Done.
> 
> GetDismissedCategoryPenalty() allows to expose the default value.

I think you get better tests if you always override the penalty to 2 (except for
the test where you override it to 0). Penalty 2 shows more interesting behaviour
then (the default) 1. Then you do not need to ask what is the default value.

To keep the code simple, you may use the approach of
ntp_snippets_fetcher_unittest.cc with two test classes (one for value 2, the
other for value 0).

[vitaliii, 2017-01-13 07:21:34]

Patchset #3 (id:40001) has been deleted

[vitaliii, 2017-01-13 07:21:41]

Patchset #3 (id:60001) has been deleted

[vitaliii, 2017-01-16 07:35:12]

Patchset #3 (id:80001) has been deleted

[vitaliii, 2017-01-16 15:59:57]

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

[commit-bot: I haz the power, 2017-01-16 16:00:08]

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

[vitaliii, 2017-01-16 16:00:16]

Hi jkrcal@,

I addressed your comment, no need to look.

https://codereview.chromium.org/2612773003/diff/1/components/ntp_snippets/cat...
File components/ntp_snippets/category_rankers/click_based_category_ranker.cc
(right):

https://codereview.chromium.org/2612773003/diff/1/components/ntp_snippets/cat...
components/ntp_snippets/category_rankers/click_based_category_ranker.cc:69:
return variations::GetVariationParamByFeatureAsInt(
On 2017/01/05 13:48:07, jkrcal wrote:
> On 2017/01/05 12:42:28, vitaliii wrote:
> > On 2017/01/05 10:34:36, jkrcal wrote:
> > > On 2017/01/05 09:28:18, vitaliii wrote:
> > > > Are you aware of any way to change the value in tests? I would like to
> test
> > > > penalty = 0.
> > > 
> > > Sure, see variations::testing::VariationParamsManager, used, e.g., in
> > >
> >
>
https://cs.chromium.org/chromium/src/components/ntp_snippets/remote/schedulin...
> > > 
> > > This may allow you to remove the function GetDismissedCategoryPenalty().
> > 
> > Done.
> > 
> > GetDismissedCategoryPenalty() allows to expose the default value.
> 
> I think you get better tests if you always override the penalty to 2 (except
for
> the test where you override it to 0). Penalty 2 shows more interesting
behaviour
> then (the default) 1. Then you do not need to ask what is the default value.
> 
> To keep the code simple, you may use the approach of
> ntp_snippets_fetcher_unittest.cc with two test classes (one for value 2, the
> other for value 0).

Partially done.

I override to 2 only where this is explicitly needed.

[commit-bot: I haz the power, 2017-01-16 17:11:02]

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

[commit-bot: I haz the power, 2017-01-16 17:11:03]

Dry run: This issue passed the CQ dry run.

[vitaliii, 2017-01-17 06:55:14]

The CQ bit was checked by vitaliii@chromium.org

[vitaliii, 2017-01-17 06:55:14]

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

[commit-bot: I haz the power, 2017-01-17 06:55: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, 2017-01-17 06:59:15]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484636114231150,
"parent_rev": "8a732acc19133a1049c57a31b1b0fb4abcc86dcd", "commit_rev":
"ecbf607e7aced540428687d67a9991d97d115e6e"}

[commit-bot: I haz the power, 2017-01-17 06:59:54]

Description was changed from

==========
[NTP::SectionOrder] Make dismissed category penalty a variation param.

Make dismissed category penalty a variation param instead of simply
hardcoding the value. Handle penalty equal to 0 correctly (ignore
OnCategoryDismissed call).

We may want to change the value or to disable it completely (by setting
to 0) later.

BUG=677919
==========

to

==========
[NTP::SectionOrder] Make dismissed category penalty a variation param.

Make dismissed category penalty a variation param instead of simply
hardcoding the value. Handle penalty equal to 0 correctly (ignore
OnCategoryDismissed call).

We may want to change the value or to disable it completely (by setting
to 0) later.

BUG=677919

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

[commit-bot: I haz the power, 2017-01-17 06:59:55]

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