Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2599293002:
[NTP::SectionOrder] Implement click counts decay. (Closed)
Description[NTP::SectionOrder] Implement click counts decay.
The click based ranker must "forget" history with time, so that changes
in the user behavior are reflected by the order in reasonable time. This
is done in thic CL using multiplicative click count decay with time.
Count overflow is ignored (it is unlikely to happen due to the decay) and a comment is added.
BUG=675918, 676279
Committed: https://crrev.com/1e8492aea8959ffaf818bb05f28368e524505ab7
Cr-Commit-Position: refs/heads/master@{#441353}
Patch Set 1 #
Total comments: 28
Patch Set 2 : rebase. #Patch Set 3 : rebase. #Patch Set 4 : tschumann@ comments. #
Total comments: 10
Patch Set 5 : rebase. #Patch Set 6 : jkrcal@ comments. #
Total comments: 4
Patch Set 7 : jkrcal@ comments. #
Total comments: 8
Patch Set 8 : rebase. #Patch Set 9 : jkrcal@ & tschumann@ comments. #Patch Set 10 : added "For testing only." to a function comment. #
Total comments: 8
Patch Set 11 : rebase. #Patch Set 12 : tschumann@ nits. #Patch Set 13 : rebase. #Patch Set 14 : tschumann@ comment. #Patch Set 15 : ios factory. #Messages
Total messages: 68 (44 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...
tschumann@, FYI.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tschumann@chromium.org changed reviewers: + tschumann@chromium.org
cool! https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:46: // no need in "forgetting" it. This value defines how many clicks are considered nit: clicks -> total clicks (across categories). https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:54: // of 0.5. please add a word about the reasoning --> this yields a 50% decay per week. https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:135: base::CheckedNumeric<int> checked_clicks = current->clicks; not sure if that's a real concern. Assuming 32 bit signed integers, a user would still need to issue 25000 clicks per second for a whole day to get into this scenario. (and after the day we'd decay). Even if they continue to click, catching up with the decay is already a challenge. Even with 16 bit signed integers, that would still be 25 per minute which is higher than we should expect. https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:278: pref_service_->GetInt64(prefs::kClickBasedCategoryRankerLastDecayTime)); is the case of the value not being present tested in a unit test? https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:291: break; this smells like premature optimization. the vector is tiny. just sum them all. https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:298: bool ClickBasedCategoryRanker::DecayClicksIfNeeded(bool force_at_least_one) { I dont' think the force function is needed anymore. However, it that parameter also contradicts the function name -- which indicates it's optional ;-). https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:316: executed_decays < num_pending_decays && IsEnoughClicksToDecay(); this reads a bit complicated. if you have a helper function bool DecayClicks() you could write it like: int executed_decays = 0; while(num_pending_decays < num_pending_decays && !IsEnoughClicksToDecay()) { DecayClicks(); ++executed_decays; } https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:40: void SetClockForTesting(std::unique_ptr<base::Clock> clock); can we inject a clock into the constructor instead? So that the factory can inject the production one and tests one for testing? From a dependency injection point of view, "new" calls in constructors are a smell -- unless you're instantiating objects to defer work to which is within the responsibility of the class. https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc (right): https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:231: TEST_F(ClickBasedCategoryRankerTest, ShouldDecayClickCountsWithTime) { it would be nice to have a less advanced tests like -- one that verifies a single decay happening. -- one that verifies we don't decay more than once a day even if get a lot of clicks. https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:243: NotifyOnSuggestionOpened(/*times=*/first_clicks, first); what's this doing? // Push category 'first' to the top. ? https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:245: // Simulate time passing by injecting our clock. let's concentrate on the effect: // Let multiple years pass by.
Hi tschumann@, I addressed your comments, PTAL. https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:46: // no need in "forgetting" it. This value defines how many clicks are considered On 2016/12/23 12:49:11, tschumann wrote: > nit: clicks -> total clicks (across categories). Done. https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:54: // of 0.5. On 2016/12/23 12:49:11, tschumann wrote: > please add a word about the reasoning --> this yields a 50% decay per week. Done. https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:135: base::CheckedNumeric<int> checked_clicks = current->clicks; On 2016/12/23 12:49:11, tschumann wrote: > not sure if that's a real concern. > Assuming 32 bit signed integers, a user would still need to issue 25000 clicks > per second for a whole day to get into this scenario. (and after the day we'd > decay). Even if they continue to click, catching up with the decay is already a > challenge. > Even with 16 bit signed integers, that would still be 25 per minute which is > higher than we should expect. Not to take into account the overflow feels weird to me. I agree that it is very unlikely (especially under current setup), but we may miss something making it more plausible (e.g. issues with clock leading to no decays) or change decay frequency. https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:278: pref_service_->GetInt64(prefs::kClickBasedCategoryRankerLastDecayTime)); On 2016/12/23 12:49:11, tschumann wrote: > is the case of the value not being present tested in a unit test? Implicitly - yes (each test initially has clean prefs, I believe). I am not sure why this is important? The value is set in the constructor, if it is missing. https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:291: break; On 2016/12/23 12:49:11, tschumann wrote: > this smells like premature optimization. the vector is tiny. just sum them all. Done. https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:298: bool ClickBasedCategoryRanker::DecayClicksIfNeeded(bool force_at_least_one) { On 2016/12/23 12:49:11, tschumann wrote: > I dont' think the force function is needed anymore. > However, it that parameter also contradicts the function name -- which indicates > it's optional ;-). Whether it is needed depends on whether we handle overflows. As for the parameter, I could not find a better name. Note that having "DecayClicks(bool optional)" feels the same (at least to me). I will wait for jkrcal@ to decide. https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:316: executed_decays < num_pending_decays && IsEnoughClicksToDecay(); On 2016/12/23 12:49:11, tschumann wrote: > this reads a bit complicated. if you have a helper function bool DecayClicks() > you could write it like: > > int executed_decays = 0; > while(num_pending_decays < num_pending_decays && !IsEnoughClicksToDecay()) { > DecayClicks(); > ++executed_decays; > } The name |DecayClicks()| collides badly with |DecayClicksIfNeeded()|. I rewrote as you suggested, but without the helper for now. https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:40: void SetClockForTesting(std::unique_ptr<base::Clock> clock); On 2016/12/23 12:49:11, tschumann wrote: > can we inject a clock into the constructor instead? > So that the factory can inject the production one and tests one for testing? > > From a dependency injection point of view, "new" calls in constructors are a > smell -- unless you're instantiating objects to defer work to which is within > the responsibility of the class. We can, but I am not sure whether injecting it into the constructor would make the situation better. My concerns: 1) We do not expect the factory to use anything except DefaultClock here; 2) NTPSnippetsFetcher has the same function (|SetTickClockForTesting|). https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc (right): https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:231: TEST_F(ClickBasedCategoryRankerTest, ShouldDecayClickCountsWithTime) { On 2016/12/23 12:49:11, tschumann wrote: > it would be nice to have a less advanced tests like > -- one that verifies a single decay happening. > -- one that verifies we don't decay more than once a day even if get a lot of > clicks. I am not sure how to test what you suggest through public interface. The only idea I have is to store last_decay_time_for_testing, allow accessing category click count and provide decay period and factor through static methods. This looks like too many implementation details to me. https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:243: NotifyOnSuggestionOpened(/*times=*/first_clicks, first); On 2016/12/23 12:49:11, tschumann wrote: > what's this doing? > // Push category 'first' to the top. > ? Done. https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:245: // Simulate time passing by injecting our clock. On 2016/12/23 12:49:11, tschumann wrote: > let's concentrate on the effect: > // Let multiple years pass by. Done.
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
Hi jkrcal@, PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:135: base::CheckedNumeric<int> checked_clicks = current->clicks; On 2016/12/29 15:35:26, vitaliii wrote: > On 2016/12/23 12:49:11, tschumann wrote: > > not sure if that's a real concern. > > Assuming 32 bit signed integers, a user would still need to issue 25000 clicks > > per second for a whole day to get into this scenario. (and after the day we'd > > decay). Even if they continue to click, catching up with the decay is already > a > > challenge. > > Even with 16 bit signed integers, that would still be 25 per minute which is > > higher than we should expect. > > Not to take into account the overflow feels weird to me. I agree that it is very > unlikely (especially under current setup), but we may miss something making it > more plausible (e.g. issues with clock leading to no decays) or change decay > frequency. I stand on the side of Tim, here. It complicates the code. It only happens if the code does not work as intended. Nothing really breaks if the counter overflows (the DCHECK >= 0 still catches it in debug builds), the user just gets a weird order. Anyway, if the code does not work as intended, the user may face more sever consequences :) https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:40: void SetClockForTesting(std::unique_ptr<base::Clock> clock); On 2016/12/29 15:35:26, vitaliii wrote: > On 2016/12/23 12:49:11, tschumann wrote: > > can we inject a clock into the constructor instead? > > So that the factory can inject the production one and tests one for testing? > > > > From a dependency injection point of view, "new" calls in constructors are a > > smell -- unless you're instantiating objects to defer work to which is within > > the responsibility of the class. > > We can, but I am not sure whether injecting it into the constructor would make > the situation better. > My concerns: > 1) We do not expect the factory to use anything except DefaultClock here; > 2) NTPSnippetsFetcher has the same function (|SetTickClockForTesting|). I have no opinion here, I am fine with you keeping it as it is and putting it in the agenda for next testing session (Set/OverrideWhateverForTesting()). https://codereview.chromium.org/2599293002/diff/60001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2599293002/diff/60001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:75: if (ReadLastDecayTimeFromPrefs() == base::Time::FromInternalValue(0)) { move this inside of RestoreDefaultOrder Otherwise, DCHECK_NE(last_decay, base::Time::FromInternalValue(0)) in DecayClicksIfNeeded called after a click may fail after ClearHistory. https://codereview.chromium.org/2599293002/diff/60001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:317: const int64_t old_clicks = static_cast<int64_t>(ranked_category.clicks); I still have some reservations about storing clicks as int and doing discrete decay. This may hit back when you work on crbug.com/675920. Please think about this bug before FF so that there is still time to change the counts to doubles if needed. https://codereview.chromium.org/2599293002/diff/60001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:319: old_clicks * kDecayFactorNumerator / kDecayFactorDenominator; Why not store this as one double? (0.91) This may be personal preference: why not directly just the following (without old_clicks variable)? ranked_category.clicks *= kDecayFactor;
Description was changed from ========== [NTP::SectionOrder] Implement clicks decay and prevent overflow. The click based ranker must "forget" history with time, so that changes in the user behavior are reflected by the order in reasonable time. This is done in thic CL using multiplicate click count decay with time. In addition to this the decay is forced when some count may overflow its data type in order to prevent the overflow. BUG=675918,676279 ========== to ========== [NTP::SectionOrder] Implement click counts decay. The click based ranker must "forget" history with time, so that changes in the user behavior are reflected by the order in reasonable time. This is done in thic CL using multiplicative click count decay with time. Count overflow is ignored (it is unlikely to happen due to the decay) and a comment is added. BUG=675918,676279 ==========
Hi jkrcal@, I addressed your comments, please have a look. https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:135: base::CheckedNumeric<int> checked_clicks = current->clicks; On 2017/01/02 10:06:37, jkrcal wrote: > On 2016/12/29 15:35:26, vitaliii wrote: > > On 2016/12/23 12:49:11, tschumann wrote: > > > not sure if that's a real concern. > > > Assuming 32 bit signed integers, a user would still need to issue 25000 > clicks > > > per second for a whole day to get into this scenario. (and after the day > we'd > > > decay). Even if they continue to click, catching up with the decay is > already > > a > > > challenge. > > > Even with 16 bit signed integers, that would still be 25 per minute which is > > > higher than we should expect. > > > > Not to take into account the overflow feels weird to me. I agree that it is > very > > unlikely (especially under current setup), but we may miss something making it > > more plausible (e.g. issues with clock leading to no decays) or change decay > > frequency. > > I stand on the side of Tim, here. It complicates the code. It only happens if > the code does not work as intended. Nothing really breaks if the counter > overflows (the DCHECK >= 0 still catches it in debug builds), the user just gets > a weird order. Anyway, if the code does not work as intended, the user may face > more sever consequences :) Done. https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:40: void SetClockForTesting(std::unique_ptr<base::Clock> clock); On 2017/01/02 10:06:37, jkrcal wrote: > On 2016/12/29 15:35:26, vitaliii wrote: > > On 2016/12/23 12:49:11, tschumann wrote: > > > can we inject a clock into the constructor instead? > > > So that the factory can inject the production one and tests one for testing? > > > > > > From a dependency injection point of view, "new" calls in constructors are a > > > smell -- unless you're instantiating objects to defer work to which is > within > > > the responsibility of the class. > > > > We can, but I am not sure whether injecting it into the constructor would make > > the situation better. > > My concerns: > > 1) We do not expect the factory to use anything except DefaultClock here; > > 2) NTPSnippetsFetcher has the same function (|SetTickClockForTesting|). > > I have no opinion here, I am fine with you keeping it as it is and putting it in > the agenda for next testing session (Set/OverrideWhateverForTesting()). Done. Added to the agenda. https://codereview.chromium.org/2599293002/diff/60001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2599293002/diff/60001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:75: if (ReadLastDecayTimeFromPrefs() == base::Time::FromInternalValue(0)) { On 2017/01/02 10:06:37, jkrcal wrote: > move this inside of RestoreDefaultOrder > > Otherwise, DCHECK_NE(last_decay, base::Time::FromInternalValue(0)) in > DecayClicksIfNeeded called after a click may fail after ClearHistory. Done. However, I do not want to implicitly store time of last clear history. I removed the DCHECK_NE in DecayClicksIfNeeded and instead set prefs value to now() if it is empty. Also I added a test. https://codereview.chromium.org/2599293002/diff/60001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:317: const int64_t old_clicks = static_cast<int64_t>(ranked_category.clicks); On 2017/01/02 10:06:37, jkrcal wrote: > I still have some reservations about storing clicks as int and doing discrete > decay. This may hit back when you work on crbug.com/675920. Please think about > this bug before FF so that there is still time to change the counts to doubles > if needed. Ack. https://codereview.chromium.org/2599293002/diff/60001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:319: old_clicks * kDecayFactorNumerator / kDecayFactorDenominator; On 2017/01/02 10:06:37, jkrcal wrote: > Why not store this as one double? (0.91) To keep everything in integers. > This may be personal preference: why not directly just the following (without > old_clicks variable)? > ranked_category.clicks *= kDecayFactor; old_clicks is int64_t and it is used to prevent overflow during the multiplication.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2599293002/diff/60001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2599293002/diff/60001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:319: old_clicks * kDecayFactorNumerator / kDecayFactorDenominator; On 2017/01/02 13:36:21, vitaliii wrote: > On 2017/01/02 10:06:37, jkrcal wrote: > > Why not store this as one double? (0.91) > > To keep everything in integers. > > > This may be personal preference: why not directly just the following (without > > old_clicks variable)? > > ranked_category.clicks *= kDecayFactor; > > old_clicks is int64_t and it is used to prevent overflow during the > multiplication. If you define kDecayFactor as double, I see no danger of ranked_category.clicks *= kDecayFactor; (ranked_category.clicks gets converted to double which may result in a relative loss of precision in the order of 10e-16 and then converted back to int which introduces an absolute error of 1 in very rare cases when rounding down to integer). The advantage of working with doubles is that you could take |num_pending_decays|th exponent of |kDecayFactor| and multiply it with ranked_category.clicks. This way, you would have only one rounding to int. When decaying a small click count for several days, the effective decay can be significantly larger than the one you specify. Not a big deal after all but the error you introduce by decaying by ints can be way larger than what you would introduce by decaying by doubles. WDYT? https://codereview.chromium.org/2599293002/diff/100001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2599293002/diff/100001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:292: bool ClickBasedCategoryRanker::DecayClicksIfNeeded(bool force_at_least_one) { Please remove the param and simplify the function accordingly. https://codereview.chromium.org/2599293002/diff/100001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc (right): https://codereview.chromium.org/2599293002/diff/100001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:277: raw_test_clock->SetNow(base::Time::Now() + base::TimeDelta::FromDays(1000)); Why do you shift the time between ClearHistory() and the next NotifyOnSuggestionOpened()? To me it would make sense in a test ShouldNotDecayOnFirstClickAfterClearHistory that actually checks that last decay time is deleted by ClearHistory(). In this test it does not seem to be needed. (Anyway, maybe a test ShouldNotDecayOnFirstEverClick also makes sense?)
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...
Hi jkrcal@, I addressed your comments, PTAL. https://codereview.chromium.org/2599293002/diff/60001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2599293002/diff/60001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:319: old_clicks * kDecayFactorNumerator / kDecayFactorDenominator; On 2017/01/02 15:01:21, jkrcal wrote: > On 2017/01/02 13:36:21, vitaliii wrote: > > On 2017/01/02 10:06:37, jkrcal wrote: > > > Why not store this as one double? (0.91) > > > > To keep everything in integers. > > > > > This may be personal preference: why not directly just the following > (without > > > old_clicks variable)? > > > ranked_category.clicks *= kDecayFactor; > > > > old_clicks is int64_t and it is used to prevent overflow during the > > multiplication. > > If you define kDecayFactor as double, I see no danger of > ranked_category.clicks *= kDecayFactor; > (ranked_category.clicks gets converted to double which may result in a relative > loss of precision in the order of 10e-16 and then converted back to int which > introduces an absolute error of 1 in very rare cases when rounding down to > integer). > > The advantage of working with doubles is that you could take > |num_pending_decays|th exponent of |kDecayFactor| and multiply it with > ranked_category.clicks. This way, you would have only one rounding to int. Unfortunately, we cannot do this due to |IsEnoughClicksToDecay()| check. > When decaying a small click count for several days, the effective decay can be > significantly larger than the one you specify. Not a big deal after all but the > error you introduce by decaying by ints can be way larger than what you would > introduce by decaying by doubles. > > WDYT? As my numeric experiments show, for 0, .., 20 clicks and 1, ..., 20 decays the difference between the two approaches is at most 4 (for 0, .., 10 clicks and 1, ..., 10 decays it is 3), which does not look that bad to me. https://codereview.chromium.org/2599293002/diff/100001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2599293002/diff/100001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:292: bool ClickBasedCategoryRanker::DecayClicksIfNeeded(bool force_at_least_one) { On 2017/01/02 15:01:21, jkrcal wrote: > Please remove the param and simplify the function accordingly. Done. https://codereview.chromium.org/2599293002/diff/100001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc (right): https://codereview.chromium.org/2599293002/diff/100001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:277: raw_test_clock->SetNow(base::Time::Now() + base::TimeDelta::FromDays(1000)); On 2017/01/02 15:01:21, jkrcal wrote: > Why do you shift the time between ClearHistory() and the next > NotifyOnSuggestionOpened()? Good point. I removed the shift. > To me it would make sense in a test ShouldNotDecayOnFirstClickAfterClearHistory > that actually checks that last decay time is deleted by ClearHistory(). > > In this test it does not seem to be needed. > > (Anyway, maybe a test ShouldNotDecayOnFirstEverClick also makes sense?) Done. It does, but it is difficult to test through the public interface due to min total click limit. However, I exposed the last decay time instead and wrote the test using it.
Hi jkrcal@, I addressed your comments, PTAL. https://codereview.chromium.org/2599293002/diff/60001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2599293002/diff/60001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:319: old_clicks * kDecayFactorNumerator / kDecayFactorDenominator; On 2017/01/02 15:01:21, jkrcal wrote: > On 2017/01/02 13:36:21, vitaliii wrote: > > On 2017/01/02 10:06:37, jkrcal wrote: > > > Why not store this as one double? (0.91) > > > > To keep everything in integers. > > > > > This may be personal preference: why not directly just the following > (without > > > old_clicks variable)? > > > ranked_category.clicks *= kDecayFactor; > > > > old_clicks is int64_t and it is used to prevent overflow during the > > multiplication. > > If you define kDecayFactor as double, I see no danger of > ranked_category.clicks *= kDecayFactor; > (ranked_category.clicks gets converted to double which may result in a relative > loss of precision in the order of 10e-16 and then converted back to int which > introduces an absolute error of 1 in very rare cases when rounding down to > integer). > > The advantage of working with doubles is that you could take > |num_pending_decays|th exponent of |kDecayFactor| and multiply it with > ranked_category.clicks. This way, you would have only one rounding to int. Unfortunately, we cannot do this due to |IsEnoughClicksToDecay()| check. > When decaying a small click count for several days, the effective decay can be > significantly larger than the one you specify. Not a big deal after all but the > error you introduce by decaying by ints can be way larger than what you would > introduce by decaying by doubles. > > WDYT? As my numeric experiments show, for 0, .., 20 clicks and 1, ..., 20 decays the difference between the two approaches is at most 4 (for 0, .., 10 clicks and 1, ..., 10 decays it is 3), which does not look that bad to me. https://codereview.chromium.org/2599293002/diff/100001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2599293002/diff/100001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:292: bool ClickBasedCategoryRanker::DecayClicksIfNeeded(bool force_at_least_one) { On 2017/01/02 15:01:21, jkrcal wrote: > Please remove the param and simplify the function accordingly. Done. https://codereview.chromium.org/2599293002/diff/100001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc (right): https://codereview.chromium.org/2599293002/diff/100001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:277: raw_test_clock->SetNow(base::Time::Now() + base::TimeDelta::FromDays(1000)); On 2017/01/02 15:01:21, jkrcal wrote: > Why do you shift the time between ClearHistory() and the next > NotifyOnSuggestionOpened()? Good point. I removed the shift. > To me it would make sense in a test ShouldNotDecayOnFirstClickAfterClearHistory > that actually checks that last decay time is deleted by ClearHistory(). > > In this test it does not seem to be needed. > > (Anyway, maybe a test ShouldNotDecayOnFirstEverClick also makes sense?) Done. It does, but it is difficult to test through the public interface due to min total click limit. However, I exposed the last decay time instead and wrote the test using it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm (modulo general reservations about the metric being integer based which is beyond the scope of this CL) https://codereview.chromium.org/2599293002/diff/60001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2599293002/diff/60001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:319: old_clicks * kDecayFactorNumerator / kDecayFactorDenominator; On 2017/01/02 15:58:55, vitaliii wrote: > On 2017/01/02 15:01:21, jkrcal wrote: > > On 2017/01/02 13:36:21, vitaliii wrote: > > > On 2017/01/02 10:06:37, jkrcal wrote: > > > > Why not store this as one double? (0.91) > > > > > > To keep everything in integers. > > > > > > > This may be personal preference: why not directly just the following > > (without > > > > old_clicks variable)? > > > > ranked_category.clicks *= kDecayFactor; > > > > > > old_clicks is int64_t and it is used to prevent overflow during the > > > multiplication. > > > > If you define kDecayFactor as double, I see no danger of > > ranked_category.clicks *= kDecayFactor; > > (ranked_category.clicks gets converted to double which may result in a > relative > > loss of precision in the order of 10e-16 and then converted back to int which > > introduces an absolute error of 1 in very rare cases when rounding down to > > integer). > > > > The advantage of working with doubles is that you could take > > |num_pending_decays|th exponent of |kDecayFactor| and multiply it with > > ranked_category.clicks. This way, you would have only one rounding to int. > > Unfortunately, we cannot do this due to |IsEnoughClicksToDecay()| check. > > > When decaying a small click count for several days, the effective decay can be > > significantly larger than the one you specify. Not a big deal after all but > the > > error you introduce by decaying by ints can be way larger than what you would > > introduce by decaying by doubles. > > > > WDYT? > > As my numeric experiments show, for 0, .., 20 clicks and 1, ..., 20 decays the > difference between the two approaches is at most 4 (for 0, .., 10 clicks and 1, > ..., 10 decays it is 3), which does not look that bad to me. To me this is suboptimal but I do not feel strongly enough to insist :) https://codereview.chromium.org/2599293002/diff/120001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2599293002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.h:40: base::Time GetLastDecayTime() const; nit: ForTesting?
https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:40: void SetClockForTesting(std::unique_ptr<base::Clock> clock); On 2017/01/02 13:36:21, vitaliii wrote: > On 2017/01/02 10:06:37, jkrcal wrote: > > On 2016/12/29 15:35:26, vitaliii wrote: > > > On 2016/12/23 12:49:11, tschumann wrote: > > > > can we inject a clock into the constructor instead? > > > > So that the factory can inject the production one and tests one for > testing? > > > > > > > > From a dependency injection point of view, "new" calls in constructors are > a > > > > smell -- unless you're instantiating objects to defer work to which is > > within > > > > the responsibility of the class. > > > > > > We can, but I am not sure whether injecting it into the constructor would > make > > > the situation better. > > > My concerns: > > > 1) We do not expect the factory to use anything except DefaultClock here; > > > 2) NTPSnippetsFetcher has the same function (|SetTickClockForTesting|). > > > > I have no opinion here, I am fine with you keeping it as it is and putting it > in > > the agenda for next testing session (Set/OverrideWhateverForTesting()). > > Done. > > Added to the agenda. (putting my TL hat on ;-)): We can put it on the agenda but should still fix it here ;-) We have this smell in a lot of places. Here, it's easy to just fix it and that's what we should do. The ForTesting() suffixes are just a crutch in cases where a proper fix takes more effort. https://codereview.chromium.org/2599293002/diff/120001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2599293002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.h:40: base::Time GetLastDecayTime() const; On 2017/01/03 08:25:56, jkrcal wrote: > nit: ForTesting? if it's not part of the usual public interface, we should try to not expose it for testing as well. Do we really need to access these internals for tests?
https://codereview.chromium.org/2599293002/diff/120001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2599293002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:325: if (executed_decays) { please explicitly compare "> 0". Even better: change to "bool executed_decay". https://codereview.chromium.org/2599293002/diff/120001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2599293002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.h:40: base::Time GetLastDecayTime() const; On 2017/01/03 09:27:05, tschumann wrote: > On 2017/01/03 08:25:56, jkrcal wrote: > > nit: ForTesting? > > if it's not part of the usual public interface, we should try to not expose it > for testing as well. Do we really need to access these internals for tests? The test could as well just read from prefs.
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...
Hi tschumann@, I addressed your comments, PTAL! (jkrcal@, no need to look). https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2599293002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/click_based_category_ranker.h:40: void SetClockForTesting(std::unique_ptr<base::Clock> clock); On 2017/01/03 09:27:05, tschumann wrote: > On 2017/01/02 13:36:21, vitaliii wrote: > > On 2017/01/02 10:06:37, jkrcal wrote: > > > On 2016/12/29 15:35:26, vitaliii wrote: > > > > On 2016/12/23 12:49:11, tschumann wrote: > > > > > can we inject a clock into the constructor instead? > > > > > So that the factory can inject the production one and tests one for > > testing? > > > > > > > > > > From a dependency injection point of view, "new" calls in constructors > are > > a > > > > > smell -- unless you're instantiating objects to defer work to which is > > > within > > > > > the responsibility of the class. > > > > > > > > We can, but I am not sure whether injecting it into the constructor would > > make > > > > the situation better. > > > > My concerns: > > > > 1) We do not expect the factory to use anything except DefaultClock here; > > > > 2) NTPSnippetsFetcher has the same function (|SetTickClockForTesting|). > > > > > > I have no opinion here, I am fine with you keeping it as it is and putting > it > > in > > > the agenda for next testing session (Set/OverrideWhateverForTesting()). > > > > Done. > > > > Added to the agenda. > (putting my TL hat on ;-)): > We can put it on the agenda but should still fix it here ;-) > We have this smell in a lot of places. Here, it's easy to just fix it and that's > what we should do. > The ForTesting() suffixes are just a crutch in cases where a proper fix takes > more effort. Done. I set it in the constructor instead. https://codereview.chromium.org/2599293002/diff/60001/components/ntp_snippets... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2599293002/diff/60001/components/ntp_snippets... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:319: old_clicks * kDecayFactorNumerator / kDecayFactorDenominator; On 2017/01/03 08:25:56, jkrcal wrote: > On 2017/01/02 15:58:55, vitaliii wrote: > > On 2017/01/02 15:01:21, jkrcal wrote: > > > On 2017/01/02 13:36:21, vitaliii wrote: > > > > On 2017/01/02 10:06:37, jkrcal wrote: > > > > > Why not store this as one double? (0.91) > > > > > > > > To keep everything in integers. > > > > > > > > > This may be personal preference: why not directly just the following > > > (without > > > > > old_clicks variable)? > > > > > ranked_category.clicks *= kDecayFactor; > > > > > > > > old_clicks is int64_t and it is used to prevent overflow during the > > > > multiplication. > > > > > > If you define kDecayFactor as double, I see no danger of > > > ranked_category.clicks *= kDecayFactor; > > > (ranked_category.clicks gets converted to double which may result in a > > relative > > > loss of precision in the order of 10e-16 and then converted back to int > which > > > introduces an absolute error of 1 in very rare cases when rounding down to > > > integer). > > > > > > The advantage of working with doubles is that you could take > > > |num_pending_decays|th exponent of |kDecayFactor| and multiply it with > > > ranked_category.clicks. This way, you would have only one rounding to int. > > > > Unfortunately, we cannot do this due to |IsEnoughClicksToDecay()| check. > > > > > When decaying a small click count for several days, the effective decay can > be > > > significantly larger than the one you specify. Not a big deal after all but > > the > > > error you introduce by decaying by ints can be way larger than what you > would > > > introduce by decaying by doubles. > > > > > > WDYT? > > > > As my numeric experiments show, for 0, .., 20 clicks and 1, ..., 20 decays the > > difference between the two approaches is at most 4 (for 0, .., 10 clicks and > 1, > > ..., 10 decays it is 3), which does not look that bad to me. > > To me this is suboptimal but I do not feel strongly enough to insist :) Acknowledged. https://codereview.chromium.org/2599293002/diff/120001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2599293002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:325: if (executed_decays) { On 2017/01/03 09:31:16, tschumann wrote: > please explicitly compare "> 0". > Even better: change to "bool executed_decay". Done, "> 0". As for "bool", it is used in the while above, so I cannot make it bool. https://codereview.chromium.org/2599293002/diff/120001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2599293002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.h:40: base::Time GetLastDecayTime() const; On 2017/01/03 09:31:16, tschumann wrote: > On 2017/01/03 09:27:05, tschumann wrote: > > On 2017/01/03 08:25:56, jkrcal wrote: > > > nit: ForTesting? > > > > if it's not part of the usual public interface, we should try to not expose it > > for testing as well. Do we really need to access these internals for tests? > > The test could as well just read from prefs. I remember Mark opposing this. I find exposing how the data is stored in prefs worse than exposing the data itself in the public interface. Also this is terribly useful for debugging (e.g. in snippets-internals) and it is not available through CategoryRanker interface (this is the proper way for clients to use it to handle different rankers well).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with a few nits. Once addressed, feel free to submit. https://codereview.chromium.org/2599293002/diff/120001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2599293002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.h:40: base::Time GetLastDecayTime() const; On 2017/01/03 15:07:18, vitaliii wrote: > On 2017/01/03 09:31:16, tschumann wrote: > > On 2017/01/03 09:27:05, tschumann wrote: > > > On 2017/01/03 08:25:56, jkrcal wrote: > > > > nit: ForTesting? > > > > > > if it's not part of the usual public interface, we should try to not expose > it > > > for testing as well. Do we really need to access these internals for tests? > > > > The test could as well just read from prefs. > > I remember Mark opposing this. I find exposing how the data is stored in prefs > worse than exposing the data itself in the public interface. > > Also this is terribly useful for debugging (e.g. in snippets-internals) and it > is not available through CategoryRanker interface (this is the proper way for > clients to use it to handle different rankers well). Let's leave it in for now, but we should revisit this in the testing sessions. My point is this: unit tests effectively do whitebox testing. Making sure the data is stored in prefs might actually be worth-while testing. IMO, it's fine to test implementation details inside unit tests, but less ok to expose them in the public interface. You have a point towards debugging, but I'm currently also not happy with the current state. The problem is that the content suggestion service is now exposing a lot of implementation details, just so that the snippets-internal page can access their data. Instead, we should invert that dependency: Snippets-Internal could have a specialized "status" section which the content suggestions service can fill. So there would be a FillDebugStatus() method on the service that takes some structure (textbuffer or html container) it can fill. The service can then ask all it's relevant members to provide data to print in there, e.g. the ranker would have a FillDebugStatus() function as well. But again, happy with the CL in the current shape as it is, but we should get this cleaned up some day. https://codereview.chromium.org/2599293002/diff/180001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2599293002/diff/180001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:322: }; superfluous semicolon. https://codereview.chromium.org/2599293002/diff/180001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc (right): https://codereview.chromium.org/2599293002/diff/180001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:58: void ResetRanker() { nit: make ResetRanker take the clock and get rid of the member variable? https://codereview.chromium.org/2599293002/diff/180001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:313: ranker()->ClearHistory(/*begin=*/base::Time::UnixEpoch(), nit: Not sure if I recommended using UnixEpoch() at some point. Looking at the history code, it seems indeed that clearing the full history is usually indicated by [Time(), Time::Max()]. So we should test for this. (https://codesearch.chromium.org/chromium/src/chrome/browser/browsing_data/bro...) and especially: https://codesearch.chromium.org/chromium/src/chrome/browser/browsing_data/bro... It's not clearly documented but seems to be common practice...
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...
Hi tschumann@, I addressed your nits, no need to look. https://codereview.chromium.org/2599293002/diff/120001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker.h (right): https://codereview.chromium.org/2599293002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.h:40: base::Time GetLastDecayTime() const; On 2017/01/03 17:52:33, tschumann wrote: > On 2017/01/03 15:07:18, vitaliii wrote: > > On 2017/01/03 09:31:16, tschumann wrote: > > > On 2017/01/03 09:27:05, tschumann wrote: > > > > On 2017/01/03 08:25:56, jkrcal wrote: > > > > > nit: ForTesting? > > > > > > > > if it's not part of the usual public interface, we should try to not > expose > > it > > > > for testing as well. Do we really need to access these internals for > tests? > > > > > > The test could as well just read from prefs. > > > > I remember Mark opposing this. I find exposing how the data is stored in prefs > > worse than exposing the data itself in the public interface. > > > > Also this is terribly useful for debugging (e.g. in snippets-internals) and it > > is not available through CategoryRanker interface (this is the proper way for > > clients to use it to handle different rankers well). > > Let's leave it in for now, but we should revisit this in the testing sessions. > My point is this: unit tests effectively do whitebox testing. Making sure the > data is stored in prefs might actually be worth-while testing. IMO, it's fine to > test implementation details inside unit tests, but less ok to expose them in the > public interface. > > You have a point towards debugging, but I'm currently also not happy with the > current state. The problem is that the content suggestion service is now > exposing a lot of implementation details, just so that the snippets-internal > page can access their data. Instead, we should invert that dependency: > Snippets-Internal could have a specialized "status" section which the content > suggestions service can fill. So there would be a FillDebugStatus() method on > the service that takes some structure (textbuffer or html container) it can > fill. The service can then ask all it's relevant members to provide data to > print in there, e.g. the ranker would have a FillDebugStatus() function as well. > > But again, happy with the CL in the current shape as it is, but we should get > this cleaned up some day. Acknowledged. (Also I added a test to check whether last decay time is stored in prefs, but through GetLastDecayTime function) https://codereview.chromium.org/2599293002/diff/180001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker.cc (right): https://codereview.chromium.org/2599293002/diff/180001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker.cc:322: }; On 2017/01/03 17:52:33, tschumann wrote: > superfluous semicolon. Done. https://codereview.chromium.org/2599293002/diff/180001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc (right): https://codereview.chromium.org/2599293002/diff/180001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:58: void ResetRanker() { On 2017/01/03 17:52:33, tschumann wrote: > nit: make ResetRanker take the clock and get rid of the member variable? Ack. All tests except decay related ones does not care about clock at all. https://codereview.chromium.org/2599293002/diff/180001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:313: ranker()->ClearHistory(/*begin=*/base::Time::UnixEpoch(), On 2017/01/03 17:52:33, tschumann wrote: > nit: Not sure if I recommended using UnixEpoch() at some point. > Looking at the history code, it seems indeed that clearing the full history is > usually indicated by [Time(), Time::Max()]. So we should test for this. > (https://codesearch.chromium.org/chromium/src/chrome/browser/browsing_data/bro...) > and especially: > https://codesearch.chromium.org/chromium/src/chrome/browser/browsing_data/bro... > > It's not clearly documented but seems to be common practice... Done.
https://codereview.chromium.org/2599293002/diff/180001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc (right): https://codereview.chromium.org/2599293002/diff/180001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:58: void ResetRanker() { On 2017/01/04 08:08:54, vitaliii wrote: > On 2017/01/03 17:52:33, tschumann wrote: > > nit: make ResetRanker take the clock and get rid of the member variable? > > Ack. > > All tests except decay related ones does not care about clock at all. the indirect injection through the member makes the test setup harder to follow. There are only few places that call ResetRanker(), so injecting the clock always there seems simpler. Just put in a DefaultClock() in these cases.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi tschumann@, I addressed your comment, no need to look. https://codereview.chromium.org/2599293002/diff/180001/components/ntp_snippet... File components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc (right): https://codereview.chromium.org/2599293002/diff/180001/components/ntp_snippet... components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:58: void ResetRanker() { On 2017/01/04 08:19:54, tschumann wrote: > On 2017/01/04 08:08:54, vitaliii wrote: > > On 2017/01/03 17:52:33, tschumann wrote: > > > nit: make ResetRanker take the clock and get rid of the member variable? > > > > Ack. > > > > All tests except decay related ones does not care about clock at all. > > the indirect injection through the member makes the test setup harder to follow. > There are only few places that call ResetRanker(), so injecting the clock always > there seems simpler. Just put in a DefaultClock() in these cases. Done.
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org, tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/2599293002/#ps260001 (title: "tschumann@ comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
vitaliii@chromium.org changed reviewers: + noyau@chromium.org
Hi noyau@, Could you have a look at ios factory change?
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ios lgtm
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org, tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/2599293002/#ps280001 (title: "ios factory.")
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": 280001, "attempt_start_ts": 1483526958441990,
"parent_rev": "77b8ac2df59e2bf79c97a14aca478d17519492a1", "commit_rev":
"5906d1185d2a7ecb4762d891d32d7bb2f63a04d1"}
Message was sent while issue was closed.
Description was changed from ========== [NTP::SectionOrder] Implement click counts decay. The click based ranker must "forget" history with time, so that changes in the user behavior are reflected by the order in reasonable time. This is done in thic CL using multiplicative click count decay with time. Count overflow is ignored (it is unlikely to happen due to the decay) and a comment is added. BUG=675918,676279 ========== to ========== [NTP::SectionOrder] Implement click counts decay. The click based ranker must "forget" history with time, so that changes in the user behavior are reflected by the order in reasonable time. This is done in thic CL using multiplicative click count decay with time. Count overflow is ignored (it is unlikely to happen due to the decay) and a comment is added. BUG=675918,676279 Review-Url: https://codereview.chromium.org/2599293002 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== [NTP::SectionOrder] Implement click counts decay. The click based ranker must "forget" history with time, so that changes in the user behavior are reflected by the order in reasonable time. This is done in thic CL using multiplicative click count decay with time. Count overflow is ignored (it is unlikely to happen due to the decay) and a comment is added. BUG=675918,676279 Review-Url: https://codereview.chromium.org/2599293002 ========== to ========== [NTP::SectionOrder] Implement click counts decay. The click based ranker must "forget" history with time, so that changes in the user behavior are reflected by the order in reasonable time. This is done in thic CL using multiplicative click count decay with time. Count overflow is ignored (it is unlikely to happen due to the decay) and a comment is added. BUG=675918,676279 Committed: https://crrev.com/1e8492aea8959ffaf818bb05f28368e524505ab7 Cr-Commit-Position: refs/heads/master@{#441353} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/1e8492aea8959ffaf818bb05f28368e524505ab7 Cr-Commit-Position: refs/heads/master@{#441353} |
