Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2616813003:
[NTP::SectionOrder] Ensure decreasing clicks when category dismissed. (Closed)
Description[NTP::SectionOrder] Ensure decreasing clicks when category dismissed.
Ensure that dismissing a category cannot increase its clicks and use
the previous category clicks, not the next one.
Due to PassingMargin, it is possible that lower categories have more
clicks. Before this CL, this could lead to dismissed category clicks
increase, which was not intended. Now we take mininum of new and old
clicks.
In addition to this before this CL, click count was reduced based on
the next category in the list, after this CL, on the previous one.
BUG=678586
Review-Url: https://codereview.chromium.org/2616813003
Cr-Commit-Position: refs/heads/master@{#443237}
Committed: https://chromium.googlesource.com/chromium/src/+/3af30d62df054e880fa5e0fd285ddbac847325f5
Patch Set 1 #
Total comments: 5
Patch Set 2 : clean rebase. #Patch Set 3 : jkrcal@ comment. #
Created: 3 years, 11 months ago
(Patch set is too large to download)
Messages
Total messages: 30 (21 generated)
The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
vitaliii@chromium.org changed reviewers: + jkrcal@chromium.org
[small CL] Hi jkrcal@, Could you have a look please?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I am not sure this is a real issue :) https://codereview.chromium.org/2616813003/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (left): https://codereview.chromium.org/2616813003/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:201: current->clicks = std::max(next_clicks - kPassingMargin, 0); isn't the right hand side (due to passing margin) always lower than the current clicks count?
Hi jkrcal@, PTAL. https://codereview.chromium.org/2616813003/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (left): https://codereview.chromium.org/2616813003/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:201: current->clicks = std::max(next_clicks - kPassingMargin, 0); On 2017/01/05 10:37:35, jkrcal wrote: > isn't the right hand side (due to passing margin) always lower than the current > clicks count? No. Let's say we have categories A, B, C. Denote the passing margin as P. Imagine the following click counts: A -> 0 clicks B -> P - 1 C -> 2*P - 2 If the penalty is 1, after dismissing A we get: B -> P - 1 A -> (in old code P-2, in new code 0) C -> 2*P - 2
drive-by https://codereview.chromium.org/2616813003/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (left): https://codereview.chromium.org/2616813003/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:201: current->clicks = std::max(next_clicks - kPassingMargin, 0); On 2017/01/05 12:13:42, vitaliii wrote: > On 2017/01/05 10:37:35, jkrcal wrote: > > isn't the right hand side (due to passing margin) always lower than the > current > > clicks count? > No. > > Let's say we have categories A, B, C. Denote the passing margin as P. Imagine > the following click counts: > A -> 0 clicks > B -> P - 1 > C -> 2*P - 2 > > If the penalty is 1, after dismissing A we get: > B -> P - 1 > A -> (in old code P-2, in new code 0) > C -> 2*P - 2 please update the comment explaining how this would be, e.g. // The next category may have more clicks (but not enough to pass the passing-margin), therefore we ensure...
https://codereview.chromium.org/2616813003/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (left): https://codereview.chromium.org/2616813003/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:201: current->clicks = std::max(next_clicks - kPassingMargin, 0); On 2017/01/05 12:25:40, tschumann wrote: > On 2017/01/05 12:13:42, vitaliii wrote: > > On 2017/01/05 10:37:35, jkrcal wrote: > > > isn't the right hand side (due to passing margin) always lower than the > > current > > > clicks count? > > No. > > > > Let's say we have categories A, B, C. Denote the passing margin as P. Imagine > > the following click counts: > > A -> 0 clicks > > B -> P - 1 > > C -> 2*P - 2 > > > > If the penalty is 1, after dismissing A we get: > > B -> P - 1 > > A -> (in old code P-2, in new code 0) > > C -> 2*P - 2 > please update the comment explaining how this would be, e.g. > // The next category may have more clicks (but not enough to pass the > passing-margin), therefore we ensure... Please explain in the comment that the issue can only occur if penalty >=2. And please update the notion of next in the code according to offline discussion.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run
Hi jkrcal@, PTAL. https://codereview.chromium.org/2616813003/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (left): https://codereview.chromium.org/2616813003/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:201: current->clicks = std::max(next_clicks - kPassingMargin, 0); On 2017/01/05 12:39:55, jkrcal wrote: > On 2017/01/05 12:25:40, tschumann wrote: > > On 2017/01/05 12:13:42, vitaliii wrote: > > > On 2017/01/05 10:37:35, jkrcal wrote: > > > > isn't the right hand side (due to passing margin) always lower than the > > > current > > > > clicks count? > > > No. > > > > > > Let's say we have categories A, B, C. Denote the passing margin as P. > Imagine > > > the following click counts: > > > A -> 0 clicks > > > B -> P - 1 > > > C -> 2*P - 2 > > > > > > If the penalty is 1, after dismissing A we get: > > > B -> P - 1 > > > A -> (in old code P-2, in new code 0) > > > C -> 2*P - 2 > > please update the comment explaining how this would be, e.g. > > // The next category may have more clicks (but not enough to pass the > > passing-margin), therefore we ensure... > > Please explain in the comment that the issue can only occur if penalty >=2. > > And please update the notion of next in the code according to offline > discussion. Former - done. Latter - crbug.com/680487.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [NTP::SectionOrder] Ensure decreasing clicks when category dismissed. Ensure that dismissing a category cannot increase its clicks. Due to PassingMargin, it is possible that lower categories have more clicks. Before this CL, this could lead to dismissed category clicks increase, which was not intended. Now we take mininum of new and old clicks. BUG=None ========== to ========== [NTP::SectionOrder] Ensure decreasing clicks when category dismissed. Ensure that dismissing a category cannot increase its clicks and use the previous category clicks, not the next one. Due to PassingMargin, it is possible that lower categories have more clicks. Before this CL, this could lead to dismissed category clicks increase, which was not intended. Now we take mininum of new and old clicks. In addition to this before this CL, click count was reduced based on the next category in the list, after this CL, on the previous one. BUG=678586 ==========
Description was changed from ========== [NTP::SectionOrder] Ensure decreasing clicks when category dismissed. Ensure that dismissing a category cannot increase its clicks and use the previous category clicks, not the next one. Due to PassingMargin, it is possible that lower categories have more clicks. Before this CL, this could lead to dismissed category clicks increase, which was not intended. Now we take mininum of new and old clicks. In addition to this before this CL, click count was reduced based on the next category in the list, after this CL, on the previous one. BUG=678586 ========== to ========== [NTP::SectionOrder] Ensure decreasing clicks when category dismissed. Ensure that dismissing a category cannot increase its clicks and use the previous category clicks, not the next one. Due to PassingMargin, it is possible that lower categories have more clicks. Before this CL, this could lead to dismissed category clicks increase, which was not intended. Now we take mininum of new and old clicks. In addition to this before this CL, click count was reduced based on the next category in the list, after this CL, on the previous one. BUG=678586 ==========
lgtm
The CQ bit was unchecked by vitaliii@chromium.org
The CQ bit was checked by vitaliii@chromium.org
The CQ bit was unchecked by vitaliii@chromium.org
Description was changed from ========== [NTP::SectionOrder] Ensure decreasing clicks when category dismissed. Ensure that dismissing a category cannot increase its clicks and use the previous category clicks, not the next one. Due to PassingMargin, it is possible that lower categories have more clicks. Before this CL, this could lead to dismissed category clicks increase, which was not intended. Now we take mininum of new and old clicks. In addition to this before this CL, click count was reduced based on the next category in the list, after this CL, on the previous one. BUG=678586 ========== to ========== [NTP::SectionOrder] Ensure decreasing clicks when category dismissed. Ensure that dismissing a category cannot increase its clicks and use the previous category clicks, not the next one. Due to PassingMargin, it is possible that lower categories have more clicks. Before this CL, this could lead to dismissed category clicks increase, which was not intended. Now we take mininum of new and old clicks. In addition to this before this CL, click count was reduced based on the next category in the list, after this CL, on the previous one. BUG=678586 ==========
Description was changed from ========== [NTP::SectionOrder] Ensure decreasing clicks when category dismissed. Ensure that dismissing a category cannot increase its clicks and use the previous category clicks, not the next one. Due to PassingMargin, it is possible that lower categories have more clicks. Before this CL, this could lead to dismissed category clicks increase, which was not intended. Now we take mininum of new and old clicks. In addition to this before this CL, click count was reduced based on the next category in the list, after this CL, on the previous one. BUG=678586 ========== to ========== [NTP::SectionOrder] Ensure decreasing clicks when category dismissed. Ensure that dismissing a category cannot increase its clicks and use the previous category clicks, not the next one. Due to PassingMargin, it is possible that lower categories have more clicks. Before this CL, this could lead to dismissed category clicks increase, which was not intended. Now we take mininum of new and old clicks. In addition to this before this CL, click count was reduced based on the next category in the list, after this CL, on the previous one. BUG=678586 ==========
The CQ bit was checked by vitaliii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484233617441700,
"parent_rev": "be82572903bce4c189a2c23548343bd03c3af14a", "commit_rev":
"3af30d62df054e880fa5e0fd285ddbac847325f5"}
Message was sent while issue was closed.
Description was changed from ========== [NTP::SectionOrder] Ensure decreasing clicks when category dismissed. Ensure that dismissing a category cannot increase its clicks and use the previous category clicks, not the next one. Due to PassingMargin, it is possible that lower categories have more clicks. Before this CL, this could lead to dismissed category clicks increase, which was not intended. Now we take mininum of new and old clicks. In addition to this before this CL, click count was reduced based on the next category in the list, after this CL, on the previous one. BUG=678586 ========== to ========== [NTP::SectionOrder] Ensure decreasing clicks when category dismissed. Ensure that dismissing a category cannot increase its clicks and use the previous category clicks, not the next one. Due to PassingMargin, it is possible that lower categories have more clicks. Before this CL, this could lead to dismissed category clicks increase, which was not intended. Now we take mininum of new and old clicks. In addition to this before this CL, click count was reduced based on the next category in the list, after this CL, on the previous one. BUG=678586 Review-Url: https://codereview.chromium.org/2616813003 Cr-Commit-Position: refs/heads/master@{#443237} Committed: https://chromium.googlesource.com/chromium/src/+/3af30d62df054e880fa5e0fd285d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as https://chromium.googlesource.com/chromium/src/+/3af30d62df054e880fa5e0fd285d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
