Support for different weight-functions for time-slicing.

Support for different weight-functions for time-slicing.


BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137229

[GeorgeY, 2012-04-17 00:06:20]

Please add Brett for history changes as well.

https://chromiumcodereview.appspot.com/10060003/diff/1/chrome/browser/history...
File chrome/browser/history/visit_filter.cc (left):

https://chromiumcodereview.appspot.com/10060003/diff/1/chrome/browser/history...
chrome/browser/history/visit_filter.cc:23: void
VisitFilter::SetTimeInRangeFilter(base::Time begin_time_of_the_day,
This function is still in the header.

https://chromiumcodereview.appspot.com/10060003/diff/1/chrome/browser/history...
File chrome/browser/history/visit_filter.cc (right):

https://chromiumcodereview.appspot.com/10060003/diff/1/chrome/browser/history...
chrome/browser/history/visit_filter.cc:113: double
VisitFilter::GetVisitScore(const VisitRow& visit) const {
Please add an unit-test for this function.

https://chromiumcodereview.appspot.com/10060003/diff/1/chrome/browser/history...
File chrome/browser/history/visit_filter.h (right):

https://chromiumcodereview.appspot.com/10060003/diff/1/chrome/browser/history...
chrome/browser/history/visit_filter.h:59: void SetFilterTime(const base::Time&
filter_time);
Comments, please - what does it do, etc. + nit: empty line after the function
declaration

https://chromiumcodereview.appspot.com/10060003/diff/1/chrome/browser/history...
chrome/browser/history/visit_filter.h:64: void SetFilterWidth(const
base::TimeDelta& filter_width);
Please also indicate that it is *only* relevant to SetFilterTime(), and *not* to
SetDayOfTheWeekFilter()

https://chromiumcodereview.appspot.com/10060003/diff/1/chrome/browser/history...
chrome/browser/history/visit_filter.h:155: // present in the other one after
this method returns.
From the comment it is not obvious what is the difference between this function
and IntersectTimeVectors()

[Rune Fevang, 2012-04-18 00:39:31]

http://codereview.chromium.org/10060003/diff/1/chrome/browser/history/visit_f...
File chrome/browser/history/visit_filter.cc (left):

http://codereview.chromium.org/10060003/diff/1/chrome/browser/history/visit_f...
chrome/browser/history/visit_filter.cc:23: void
VisitFilter::SetTimeInRangeFilter(base::Time begin_time_of_the_day,
On 2012/04/17 00:06:20, GeorgeY wrote:
> This function is still in the header.

Done.

http://codereview.chromium.org/10060003/diff/1/chrome/browser/history/visit_f...
File chrome/browser/history/visit_filter.cc (right):

http://codereview.chromium.org/10060003/diff/1/chrome/browser/history/visit_f...
chrome/browser/history/visit_filter.cc:113: double
VisitFilter::GetVisitScore(const VisitRow& visit) const {
On 2012/04/17 00:06:20, GeorgeY wrote:
> Please add an unit-test for this function.

Done.

http://codereview.chromium.org/10060003/diff/1/chrome/browser/history/visit_f...
File chrome/browser/history/visit_filter.h (right):

http://codereview.chromium.org/10060003/diff/1/chrome/browser/history/visit_f...
chrome/browser/history/visit_filter.h:59: void SetFilterTime(const base::Time&
filter_time);
On 2012/04/17 00:06:20, GeorgeY wrote:
> Comments, please - what does it do, etc. + nit: empty line after the function
> declaration

Done.

http://codereview.chromium.org/10060003/diff/1/chrome/browser/history/visit_f...
chrome/browser/history/visit_filter.h:64: void SetFilterWidth(const
base::TimeDelta& filter_width);
On 2012/04/17 00:06:20, GeorgeY wrote:
> Please also indicate that it is *only* relevant to SetFilterTime(), and *not*
to
> SetDayOfTheWeekFilter()

Done.

http://codereview.chromium.org/10060003/diff/1/chrome/browser/history/visit_f...
chrome/browser/history/visit_filter.h:155: // present in the other one after
this method returns.
On 2012/04/17 00:06:20, GeorgeY wrote:
> From the comment it is not obvious what is the difference between this
function
> and IntersectTimeVectors()

I removed this function. I originally added it because I had three ways of
restricting time in UpdateTimeVector(), and the nested-if structure started
becoming confusing.

[GeorgeY, 2012-04-18 23:31:36]

Two nits :)

https://chromiumcodereview.appspot.com/10060003/diff/5001/chrome/browser/hist...
File chrome/browser/history/visit_filter.h (right):

https://chromiumcodereview.appspot.com/10060003/diff/5001/chrome/browser/hist...
chrome/browser/history/visit_filter.h:79: ORDER_BY_TIME_GAUSSIAN,
nit: please add a short comment there

https://chromiumcodereview.appspot.com/10060003/diff/5001/chrome/browser/hist...
File chrome/browser/history/visit_filter_unittest.cc (right):

https://chromiumcodereview.appspot.com/10060003/diff/5001/chrome/browser/hist...
chrome/browser/history/visit_filter_unittest.cc:257: EXPECT_EQ(0.5,
filter.GetVisitScore(visit));
Please do not use direct comparison: it is great opportunity for test to become
flaky. For example, use EXPECT_NEAR instead with generous error.

[Rune Fevang, 2012-04-19 00:17:53]

https://chromiumcodereview.appspot.com/10060003/diff/5001/chrome/browser/hist...
File chrome/browser/history/visit_filter_unittest.cc (right):

https://chromiumcodereview.appspot.com/10060003/diff/5001/chrome/browser/hist...
chrome/browser/history/visit_filter_unittest.cc:257: EXPECT_EQ(0.5,
filter.GetVisitScore(visit));
On 2012/04/18 23:31:36, GeorgeY wrote:
> Please do not use direct comparison: it is great opportunity for test to
become
> flaky. For example, use EXPECT_NEAR instead with generous error.

I figured 1.0 and 0.5 should be pretty easy to calculate exactly, but I can
change that. I used EXPECT_DOUBLE_EQ for the other values, is that OK or do you
prefer EXPECT_NEAR?

[Rune Fevang, 2012-04-19 23:05:06]

https://chromiumcodereview.appspot.com/10060003/diff/5001/chrome/browser/hist...
File chrome/browser/history/visit_filter.h (right):

https://chromiumcodereview.appspot.com/10060003/diff/5001/chrome/browser/hist...
chrome/browser/history/visit_filter.h:79: ORDER_BY_TIME_GAUSSIAN,
On 2012/04/18 23:31:36, GeorgeY wrote:
> nit: please add a short comment there

Done.

https://chromiumcodereview.appspot.com/10060003/diff/5001/chrome/browser/hist...
File chrome/browser/history/visit_filter_unittest.cc (right):

https://chromiumcodereview.appspot.com/10060003/diff/5001/chrome/browser/hist...
chrome/browser/history/visit_filter_unittest.cc:257: EXPECT_EQ(0.5,
filter.GetVisitScore(visit));
On 2012/04/19 00:17:53, Rune Fevang wrote:
> On 2012/04/18 23:31:36, GeorgeY wrote:
> > Please do not use direct comparison: it is great opportunity for test to
> become
> > flaky. For example, use EXPECT_NEAR instead with generous error.
> 
> I figured 1.0 and 0.5 should be pretty easy to calculate exactly, but I can
> change that. I used EXPECT_DOUBLE_EQ for the other values, is that OK or do
you
> prefer EXPECT_NEAR?

Done.

[GeorgeY, 2012-04-19 23:10:55]

On 2012/04/19 23:05:06, Rune Fevang wrote:
>
https://chromiumcodereview.appspot.com/10060003/diff/5001/chrome/browser/hist...
> File chrome/browser/history/visit_filter.h (right):
> 
>
https://chromiumcodereview.appspot.com/10060003/diff/5001/chrome/browser/hist...
> chrome/browser/history/visit_filter.h:79: ORDER_BY_TIME_GAUSSIAN,
> On 2012/04/18 23:31:36, GeorgeY wrote:
> > nit: please add a short comment there
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10060003/diff/5001/chrome/browser/hist...
> File chrome/browser/history/visit_filter_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10060003/diff/5001/chrome/browser/hist...
> chrome/browser/history/visit_filter_unittest.cc:257: EXPECT_EQ(0.5,
> filter.GetVisitScore(visit));
> On 2012/04/19 00:17:53, Rune Fevang wrote:
> > On 2012/04/18 23:31:36, GeorgeY wrote:
> > > Please do not use direct comparison: it is great opportunity for test to
> > become
> > > flaky. For example, use EXPECT_NEAR instead with generous error.
> > 
> > I figured 1.0 and 0.5 should be pretty easy to calculate exactly, but I can
> > change that. I used EXPECT_DOUBLE_EQ for the other values, is that OK or do
> you
> > prefer EXPECT_NEAR?
> 
> Done.

lgtm

[Rune Fevang, 2012-05-02 22:56:55]

+estade for OWNERS review.

[Evan Stade, 2012-05-03 00:06:54]

SuggestionsPageHandler lgtm

https://chromiumcodereview.appspot.com/10060003/diff/21002/chrome/browser/ui/...
File chrome/browser/ui/webui/ntp/suggestions_page_handler.cc (right):

https://chromiumcodereview.appspot.com/10060003/diff/21002/chrome/browser/ui/...
chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:142: 
remove \n

https://chromiumcodereview.appspot.com/10060003/diff/21002/chrome/browser/ui/...
chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:144: 
remove \n

https://chromiumcodereview.appspot.com/10060003/diff/21002/chrome/browser/ui/...
chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:242: if
(base::StringToUint(filter_width_switch, &filter_width)) {
no curlies

https://chromiumcodereview.appspot.com/10060003/diff/21002/chrome/browser/ui/...
chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:257: 
remove \n

[Rune Fevang, 2012-05-03 20:50:58]

https://chromiumcodereview.appspot.com/10060003/diff/21002/chrome/browser/ui/...
File chrome/browser/ui/webui/ntp/suggestions_page_handler.cc (right):

https://chromiumcodereview.appspot.com/10060003/diff/21002/chrome/browser/ui/...
chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:142: 
On 2012/05/03 00:06:57, Evan Stade wrote:
> remove \n

Done.

https://chromiumcodereview.appspot.com/10060003/diff/21002/chrome/browser/ui/...
chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:144: 
On 2012/05/03 00:06:57, Evan Stade wrote:
> remove \n

Done.

https://chromiumcodereview.appspot.com/10060003/diff/21002/chrome/browser/ui/...
chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:242: if
(base::StringToUint(filter_width_switch, &filter_width)) {
On 2012/05/03 00:06:57, Evan Stade wrote:
> no curlies

Done.

https://chromiumcodereview.appspot.com/10060003/diff/21002/chrome/browser/ui/...
chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:257: 
On 2012/05/03 00:06:57, Evan Stade wrote:
> remove \n

Done.

[Rune Fevang, 2012-05-11 22:33:45]

Brett, will you be able to have a look at this CL? Is there someone else I can
ask if not?

[brettw, 2012-05-14 17:48:53]

On Fri, May 11, 2012 at 3:33 PM,  <rfevang@chromium.org> wrote:
> Brett, will you be able to have a look at this CL? Is there someone else I
> can
> ask if not?

Sorry, will do now.

Brett

[brettw, 2012-05-14 18:03:12]

LGTM. No substantive comments, just some style nits.

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/browser/his...
File chrome/browser/history/visit_filter.cc (right):

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/browser/his...
chrome/browser/history/visit_filter.cc:8: #include "math.h"
This should be <math.h> and it should be in its own group above <algorithm>
since it's a C include.

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/browser/his...
chrome/browser/history/visit_filter.cc:148: } break;
Put break in the {}, same below.

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/browser/his...
chrome/browser/history/visit_filter.cc:150: {
This should be on the previous line.

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/browser/his...
chrome/browser/history/visit_filter.cc:155: else
Use {} when there are >1 line.

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/browser/his...
chrome/browser/history/visit_filter.cc:166: base::TimeDelta
VisitFilter::GetTimeDifference(base::Time t1, base::Time t2) {
Can you rename this GetTimeOfDayDifference? Otherwise it just sounds like you're
subtracting them.

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/browser/his...
chrome/browser/history/visit_filter.cc:178: if (difference >
base::TimeDelta::FromHours(12)) {
No {} for consistency.

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/browser/his...
File chrome/browser/history/visit_filter.h (right):

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/browser/his...
chrome/browser/history/visit_filter.h:49: void SetFilterTime(const base::Time&
filter_time);
Feel free to pass Time[Delta] by value (you don't have to change anything, just
FYI) since it's just a wrapper around an int64.

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/browser/his...
File chrome/browser/history/visit_filter_unittest.cc (right):

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/browser/his...
chrome/browser/history/visit_filter_unittest.cc:240: TEST_F(VisitFilterTest,
GetVisitScore) {
Thanks for this test.

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/common/chro...
File chrome/common/chrome_switches.cc (right):

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/common/chro...
chrome/common/chrome_switches.cc:1150: const char kSuggestionNtpFilterWidth[]   
  = "suggestion-ntp-filter-width";
Please try to remember to remove these when you're done developing this feature.

[Rune Fevang, 2012-05-15 01:32:41]

Thanks!

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/browser/his...
File chrome/browser/history/visit_filter.cc (right):

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/browser/his...
chrome/browser/history/visit_filter.cc:8: #include "math.h"
On 2012/05/14 18:03:12, brettw wrote:
> This should be <math.h> and it should be in its own group above <algorithm>
> since it's a C include.

Done.

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/browser/his...
chrome/browser/history/visit_filter.cc:148: } break;
On 2012/05/14 18:03:12, brettw wrote:
> Put break in the {}, same below.

Done.

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/browser/his...
chrome/browser/history/visit_filter.cc:150: {
On 2012/05/14 18:03:12, brettw wrote:
> This should be on the previous line.

Done.

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/browser/his...
chrome/browser/history/visit_filter.cc:155: else
On 2012/05/14 18:03:12, brettw wrote:
> Use {} when there are >1 line.

Done.

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/browser/his...
chrome/browser/history/visit_filter.cc:166: base::TimeDelta
VisitFilter::GetTimeDifference(base::Time t1, base::Time t2) {
On 2012/05/14 18:03:12, brettw wrote:
> Can you rename this GetTimeOfDayDifference? Otherwise it just sounds like
you're
> subtracting them.

Done.

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/browser/his...
chrome/browser/history/visit_filter.cc:178: if (difference >
base::TimeDelta::FromHours(12)) {
On 2012/05/14 18:03:12, brettw wrote:
> No {} for consistency.

Done.

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/common/chro...
File chrome/common/chrome_switches.cc (right):

https://chromiumcodereview.appspot.com/10060003/diff/37002/chrome/common/chro...
chrome/common/chrome_switches.cc:1150: const char kSuggestionNtpFilterWidth[]   
  = "suggestion-ntp-filter-width";
On 2012/05/14 18:03:12, brettw wrote:
> Please try to remember to remove these when you're done developing this
feature.

Will do.

[commit-bot: I haz the power, 2012-05-15 01:33:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rfevang@chromium.org/10060003/40003

[commit-bot: I haz the power, 2012-05-15 01:56:05]

Try job failure for 10060003-40003 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-05-15 06:46:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rfevang@chromium.org/10060003/44003

[commit-bot: I haz the power, 2012-05-15 07:51:23]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android, revision is 137083, job name
was 10060003-44003 (previous was lost) (previous was lost) (previous was lost)
(previous was lost).

[commit-bot: I haz the power, 2012-05-15 18:25:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rfevang@chromium.org/10060003/44003

[commit-bot: I haz the power, 2012-05-15 20:27:04]

Change committed as 137229