Enable history counting for time ranges.

Enable history counting for time ranges.

GetHistoryCount() previously counted all of browsing history. This can be now achieved by calling GetHistoryCount(base::Time(), base::Time::Max()). However, it's also possible to specify a narrower time interval.

BUG=510028
Committed: https://crrev.com/7550cde6da3c3508b8a19ff06b6fa9eb1aaeba87
Cr-Commit-Position: refs/heads/master@{#351298}

[msramek, 2015-09-24 15:06:30]

msramek@chromium.org changed reviewers:
+ sdefresne@chromium.org

[msramek, 2015-09-24 15:06:31]

Hi Sylvain,

can you please have a look?

Thanks,
Martin

[msramek, 2015-09-24 15:07:48]

+cc lwchkg@ FYI

When this lands, please use GetHistoryCount(base::Time(), base::Time::Max()) in
https://codereview.chromium.org/1248613003/

[lwchkg, 2015-09-25 04:06:35]

lwchkg@gmail.com changed reviewers:
+ lwchkg@gmail.com

[lwchkg, 2015-09-25 04:06:35]

Thanks for working the unit test for me. BTW, here are a few comments.

WC Leung

https://codereview.chromium.org/1370493002/diff/1/components/history/core/bro...
File components/history/core/browser/visit_database_unittest.cc (right):

https://codereview.chromium.org/1370493002/diff/1/components/history/core/bro...
components/history/core/browser/visit_database_unittest.cc:423: Time now =
today;
The test will fail when the day is near DST switch. You code looks good, but the
implementation of TimeDelta::FromDays is faulty because it does not take care of
DST.

Quote: time.h lines 602-606.
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&s...
> inline TimeDelta TimeDelta::FromDays(int days) {
>   if (days == std::numeric_limits<int>::max())
>     return Max();
>   return TimeDelta(days * Time::kMicrosecondsPerDay);
> }

https://codereview.chromium.org/1370493002/diff/1/components/history/core/bro...
components/history/core/browser/visit_database_unittest.cc:424: 
Anyway, any plans to test time zone and DST handling? Is it possible anyway (may
need admin rights which is impossible in a few OSes)?

[lwchkg, 2015-09-25 04:15:03]

One more comment carried from our past CL:
https://codereview.chromium.org/1345473003/

https://codereview.chromium.org/1370493002/diff/1/components/history/core/bro...
File components/history/core/browser/visit_database.cc (right):

https://codereview.chromium.org/1370493002/diff/1/components/history/core/bro...
components/history/core/browser/visit_database.cc:558: statement.BindInt64(0,
base::Time::kTimeTToMicrosecondsOffset);
Just to remind that you have proposed to change this constant because this
theoretically wrong.

[msramek, 2015-09-25 10:44:35]

https://codereview.chromium.org/1370493002/diff/1/components/history/core/bro...
File components/history/core/browser/visit_database.cc (right):

https://codereview.chromium.org/1370493002/diff/1/components/history/core/bro...
components/history/core/browser/visit_database.cc:558: statement.BindInt64(0,
base::Time::kTimeTToMicrosecondsOffset);
On 2015/09/25 04:15:02, lwchkg wrote:
> Just to remind that you have proposed to change this constant because this
> theoretically wrong.

I keep it in mind, but I'll fix this in a separate CL.

https://codereview.chromium.org/1370493002/diff/1/components/history/core/bro...
File components/history/core/browser/visit_database_unittest.cc (right):

https://codereview.chromium.org/1370493002/diff/1/components/history/core/bro...
components/history/core/browser/visit_database_unittest.cc:423: Time now =
today;
On 2015/09/25 04:06:35, lwchkg wrote:
> The test will fail when the day is near DST switch. You code looks good, but
the
> implementation of TimeDelta::FromDays is faulty because it does not take care
of
> DST.
> 
> Quote: time.h lines 602-606.
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&s...
> > inline TimeDelta TimeDelta::FromDays(int days) {
> >   if (days == std::numeric_limits<int>::max())
> >     return Max();
> >   return TimeDelta(days * Time::kMicrosecondsPerDay);
> > }

Done. Thanks for reminding me, I forgot about DST again.

https://codereview.chromium.org/1370493002/diff/1/components/history/core/bro...
components/history/core/browser/visit_database_unittest.cc:424: 
On 2015/09/25 04:06:35, lwchkg wrote:
> Anyway, any plans to test time zone and DST handling? Is it possible anyway
(may
> need admin rights which is impossible in a few OSes)?

I've added DST-specific tests at the end. This is of course timezone-dependent
(the DST part will not get properly tested if this test is executed in a
timezone that doesn't have it), but I guess this is good enough; our servers
should be in a timezone that uses DST.

[lwchkg, 2015-09-28 18:14:52]

Just some other comments. Please check. Thanks.

https://codereview.chromium.org/1370493002/diff/20001/components/history/core...
File components/history/core/browser/visit_database_unittest.cc (right):

https://codereview.chromium.org/1370493002/diff/20001/components/history/core...
components/history/core/browser/visit_database_unittest.cc:532: for (int i = 0;
i < 366; i++) {
Forward at least one day before the loop start. Otherwise the data with ids 6-9
can interfere with the test, which results in failure due to extra counts.

https://codereview.chromium.org/1370493002/diff/20001/components/history/core...
components/history/core/browser/visit_database_unittest.cc:534: tomorrow = today
+ TimeDelta::FromHours(24);
Is it better to name it after_24_hours?

https://codereview.chromium.org/1370493002/diff/20001/components/history/core...
components/history/core/browser/visit_database_unittest.cc:536: if
(today.LocalMidnight() == tomorrow.LocalMidnight()) {
nits: today.LocalMidnight() can be replaced by just today.

[sdefresne, 2015-09-29 08:21:49]

https://codereview.chromium.org/1370493002/diff/20001/components/history/core...
File components/history/core/browser/visit_database_unittest.cc (right):

https://codereview.chromium.org/1370493002/diff/20001/components/history/core...
components/history/core/browser/visit_database_unittest.cc:428: Time tomorrow =
(today + TimeDelta::FromHours(36)).LocalMidnight();
nit: this confused me, as there can never be visit in the future, what about
renaming the variables to "two_days_ago", "one_day_ago" and "today"? then use
ranges like [one_day_ago, today] instead of [today, tomorrow].

[msramek, 2015-09-29 11:04:41]

https://codereview.chromium.org/1370493002/diff/20001/components/history/core...
File components/history/core/browser/visit_database_unittest.cc (right):

https://codereview.chromium.org/1370493002/diff/20001/components/history/core...
components/history/core/browser/visit_database_unittest.cc:428: Time tomorrow =
(today + TimeDelta::FromHours(36)).LocalMidnight();
On 2015/09/29 08:21:49, sdefresne wrote:
> nit: this confused me, as there can never be visit in the future, what about
> renaming the variables to "two_days_ago", "one_day_ago" and "today"? then use
> ranges like [one_day_ago, today] instead of [today, tomorrow].

Done. Changed to the two previous days. And in the part about DST, we now
iterate a year backwards.

https://codereview.chromium.org/1370493002/diff/20001/components/history/core...
components/history/core/browser/visit_database_unittest.cc:532: for (int i = 0;
i < 366; i++) {
On 2015/09/28 18:14:51, lwchkg wrote:
> Forward at least one day before the loop start. Otherwise the data with ids
6-9
> can interfere with the test, which results in failure due to extra counts.

Thanks for catching this. Since now after sdefresne@'s comment I changed the
iteration to go backwards, I skipped one day in the other direction. I.e. the
data above are in the range [today - 2 days, today], and the iteration tests
days starting at [today - 4 days, today - 4 days + 24hrs] and backwards.

https://codereview.chromium.org/1370493002/diff/20001/components/history/core...
components/history/core/browser/visit_database_unittest.cc:534: tomorrow = today
+ TimeDelta::FromHours(24);
On 2015/09/28 18:14:51, lwchkg wrote:
> Is it better to name it after_24_hours?

Done.

https://codereview.chromium.org/1370493002/diff/20001/components/history/core...
components/history/core/browser/visit_database_unittest.cc:536: if
(today.LocalMidnight() == tomorrow.LocalMidnight()) {
On 2015/09/28 18:14:51, lwchkg wrote:
> nits: today.LocalMidnight() can be replaced by just today.

Done.

[sdefresne, 2015-09-29 12:43:05]

lgtm

[msramek, 2015-09-29 12:50:25]

The CQ bit was checked by msramek@chromium.org

[commit-bot: I haz the power, 2015-09-29 12:51:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1370493002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1370493002/40001

[commit-bot: I haz the power, 2015-09-29 13:05:09]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-29 13:05:47]

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

[lwchkg, 2015-10-01 17:01:48]

Some comments again (there are still some issues with the code.)

https://codereview.chromium.org/1370493002/diff/40001/components/history/core...
File components/history/core/browser/visit_database_unittest.cc (right):

https://codereview.chromium.org/1370493002/diff/40001/components/history/core...
components/history/core/browser/visit_database_unittest.cc:428: Time yesterday =
(today - TimeDelta::FromHours(36)).LocalMidnight();
It should be (today - TimeDelta::FromSeconds(1)).LocalMidnight();

https://codereview.chromium.org/1370493002/diff/40001/components/history/core...
components/history/core/browser/visit_database_unittest.cc:429: Time
two_days_ago = (yesterday - TimeDelta::FromHours(36)).LocalMidnight();
A similar mistake is found here. So effectively you have two_days_ago and
four_days_ago.

[Mike Lerman, 2015-10-01 19:31:47]

On 2015/10/01 17:01:48, lwchkg wrote:
> Some comments again (there are still some issues with the code.)
> 
>
https://codereview.chromium.org/1370493002/diff/40001/components/history/core...
> File components/history/core/browser/visit_database_unittest.cc (right):
> 
>
https://codereview.chromium.org/1370493002/diff/40001/components/history/core...
> components/history/core/browser/visit_database_unittest.cc:428: Time yesterday
=
> (today - TimeDelta::FromHours(36)).LocalMidnight();
> It should be (today - TimeDelta::FromSeconds(1)).LocalMidnight();
> 
>
https://codereview.chromium.org/1370493002/diff/40001/components/history/core...
> components/history/core/browser/visit_database_unittest.cc:429: Time
> two_days_ago = (yesterday - TimeDelta::FromHours(36)).LocalMidnight();
> A similar mistake is found here. So effectively you have two_days_ago and
> four_days_ago.

If you want to address these issues, then please do so in a small follow-up CL.

[msramek, 2015-10-06 16:07:27]

https://codereview.chromium.org/1370493002/diff/40001/components/history/core...
File components/history/core/browser/visit_database_unittest.cc (right):

https://codereview.chromium.org/1370493002/diff/40001/components/history/core...
components/history/core/browser/visit_database_unittest.cc:428: Time yesterday =
(today - TimeDelta::FromHours(36)).LocalMidnight();
On 2015/10/01 17:01:48, lwchkg wrote:
> It should be (today - TimeDelta::FromSeconds(1)).LocalMidnight();

Oops! And it passes the test perfectly anyway, because the intervals are always
based on the three time points (|today|, |yesterday|, |two_days_ago|), no matter
what time they actually represent. I addressed this in
https://codereview.chromium.org/1389923004/

https://codereview.chromium.org/1370493002/diff/40001/components/history/core...
components/history/core/browser/visit_database_unittest.cc:429: Time
two_days_ago = (yesterday - TimeDelta::FromHours(36)).LocalMidnight();
On 2015/10/01 17:01:48, lwchkg wrote:
> A similar mistake is found here. So effectively you have two_days_ago and
> four_days_ago.

Done.