Fix the incorrectly named time points in VisitDatabaseTest.GetHistoryCount.

Fix the incorrectly named time points in VisitDatabaseTest.GetHistoryCount.

Previously, |yesterday| corresponded to two days ago, and |two_days_ago| corresponded to four days ago. This didn't matter for the correctness of the test, but should be fixed for consistency.

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

[msramek, 2015-10-06 16:08:23]

msramek@chromium.org changed reviewers:
+ lwchkg@gmail.com, sdefresne@chromium.org

[msramek, 2015-10-06 16:08:23]

Hi guys,

please have a look at this - it's a response of lwchkg@'s comments from
https://codereview.chromium.org/1370493002/ after I landed that CL.

Thanks,
Martin

[sdefresne, 2015-10-06 17:14:32]

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

https://codereview.chromium.org/1389923004/diff/1/components/history/core/bro...
components/history/core/browser/visit_database_unittest.cc:428: Time yesterday =
(today - TimeDelta::FromSeconds(1)).LocalMidnight();
Will this work around day light saving change? Why not do "today -
TimeDelta::FromHours(3)" (with the assumption that daylight time saving is at
most one hour change, using a delta of 3 hours should always be yesterday)?

[lwchkg, 2015-10-07 04:59:38]

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

https://codereview.chromium.org/1389923004/diff/1/components/history/core/bro...
components/history/core/browser/visit_database_unittest.cc:428: Time yesterday =
(today - TimeDelta::FromSeconds(1)).LocalMidnight();
On 2015/10/06 17:14:32, sdefresne wrote:
> Will this work around day light saving change? Why not do "today -
> TimeDelta::FromHours(3)" (with the assumption that daylight time saving is at
> most one hour change, using a delta of 3 hours should always be yesterday)? 

Thanks for a good point. DST is simply very hard and confusing indeed. Just had
a look of http://www.timeanddate.com/time/change/brazil/brasilia , and it
reveals that there is NO 18/Oct/2015 MIDNIGHT IN Brazil!

BTW, if the LocalMidnight (if exists) is found correctly, turning back one
second should be enough in both forward and backward shift.
(msramek, is it possible to do the unit test in Brazil time in your computer?)

(Sadly it also means today.LocalMidnight() !=
today.LocalMidnight().LocalMidnight() is possible. I'll try to produce this
stunning result.)

Anyway, looks like we need to audit all implementations using LocalMidnight for
correctness (for both Chromium and SQLite code).

[sdefresne, 2015-10-07 07:42:54]

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

https://codereview.chromium.org/1389923004/diff/1/components/history/core/bro...
components/history/core/browser/visit_database_unittest.cc:428: Time yesterday =
(today - TimeDelta::FromSeconds(1)).LocalMidnight();
On 2015/10/07 at 04:59:37, lwchkg wrote:
> On 2015/10/06 17:14:32, sdefresne wrote:
> > Will this work around day light saving change? Why not do "today -
> > TimeDelta::FromHours(3)" (with the assumption that daylight time saving is
at
> > most one hour change, using a delta of 3 hours should always be yesterday)? 
> 
> Thanks for a good point. DST is simply very hard and confusing indeed. Just
had a look of http://www.timeanddate.com/time/change/brazil/brasilia , and it
reveals that there is NO 18/Oct/2015 MIDNIGHT IN Brazil!
> 
> BTW, if the LocalMidnight (if exists) is found correctly, turning back one
second should be enough in both forward and backward shift.
> (msramek, is it possible to do the unit test in Brazil time in your computer?)
> 
> (Sadly it also means today.LocalMidnight() !=
today.LocalMidnight().LocalMidnight() is possible. I'll try to produce this
stunning result.)
> 
> Anyway, looks like we need to audit all implementations using LocalMidnight
for correctness (for both Chromium and SQLite code).

Or you could fix that initial date instead of using time::Now().

[lwchkg, 2015-10-07 08:55:43]

> Or you could fix that initial date instead of using time::Now().

Does not look good either way. Time::LocalMidnight() can pass a invalid time to
mktime() (or similar functions in different OSes). While Chromium's time
facility guarantees that a time is returned, and the result is not guaranteed to
be a time in the same day (e.g. In Windows, the time 23:00 of the previous day
is returned).

An interesting read can be found here, coincidentally using Brazil time:
http://stackoverflow.com/questions/12251123/mktime-issue-when-converting-a-st...
(Note: that time 21/09/0112 in that page actually refers to 21/10/2012, which is
when the time shift happens.)

It means the tests, including the non-DST related tests, will fail on timezone
changes.

Solution? Not sure. An idea is to add 1 hour if LocalMidnight() returns a time
in 23h, so it become 1h (i.e. the earliest time) of the next day. But I want a
more robust solution if possible.

Anyway, I suspect that other parts of the history code are also affected by this
problem.

[sdefresne, 2015-10-07 13:55:36]

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

https://codereview.chromium.org/1389923004/diff/1/components/history/core/bro...
components/history/core/browser/visit_database_unittest.cc:428: Time yesterday =
(today - TimeDelta::FromSeconds(1)).LocalMidnight();
On 2015/10/07 at 07:42:54, sdefresne wrote:
> On 2015/10/07 at 04:59:37, lwchkg wrote:
> > On 2015/10/06 17:14:32, sdefresne wrote:
> > > Will this work around day light saving change? Why not do "today -
> > > TimeDelta::FromHours(3)" (with the assumption that daylight time saving is
at
> > > most one hour change, using a delta of 3 hours should always be
yesterday)? 
> > 
> > Thanks for a good point. DST is simply very hard and confusing indeed. Just
had a look of http://www.timeanddate.com/time/change/brazil/brasilia , and it
reveals that there is NO 18/Oct/2015 MIDNIGHT IN Brazil!
> > 
> > BTW, if the LocalMidnight (if exists) is found correctly, turning back one
second should be enough in both forward and backward shift.
> > (msramek, is it possible to do the unit test in Brazil time in your
computer?)
> > 
> > (Sadly it also means today.LocalMidnight() !=
today.LocalMidnight().LocalMidnight() is possible. I'll try to produce this
stunning result.)
> > 
> > Anyway, looks like we need to audit all implementations using LocalMidnight
for correctness (for both Chromium and SQLite code).
> 
> Or you could fix that initial date instead of using time::Now().

I was thinking of using a date known not to cause any issue (something like
2015-07-07). I guess the code is no worse now than it is without the patch, and
the name are more correct with this CL, so lgtm.

[msramek, 2015-10-13 18:10:05]

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

https://codereview.chromium.org/1389923004/diff/1/components/history/core/bro...
components/history/core/browser/visit_database_unittest.cc:428: Time yesterday =
(today - TimeDelta::FromSeconds(1)).LocalMidnight();
On 2015/10/07 13:55:36, sdefresne (OOO till 19th Oct.) wrote:
> On 2015/10/07 at 07:42:54, sdefresne wrote:
> > On 2015/10/07 at 04:59:37, lwchkg wrote:
> > > On 2015/10/06 17:14:32, sdefresne wrote:
> > > > Will this work around day light saving change? Why not do "today -
> > > > TimeDelta::FromHours(3)" (with the assumption that daylight time saving
is
> at
> > > > most one hour change, using a delta of 3 hours should always be
> yesterday)? 
> > > 
> > > Thanks for a good point. DST is simply very hard and confusing indeed.
Just
> had a look of http://www.timeanddate.com/time/change/brazil/brasilia , and it
> reveals that there is NO 18/Oct/2015 MIDNIGHT IN Brazil!
> > > 
> > > BTW, if the LocalMidnight (if exists) is found correctly, turning back one
> second should be enough in both forward and backward shift.
> > > (msramek, is it possible to do the unit test in Brazil time in your
> computer?)
> > > 
> > > (Sadly it also means today.LocalMidnight() !=
> today.LocalMidnight().LocalMidnight() is possible. I'll try to produce this
> stunning result.)
> > > 
> > > Anyway, looks like we need to audit all implementations using
LocalMidnight
> for correctness (for both Chromium and SQLite code).
> > 
> > Or you could fix that initial date instead of using time::Now().
> 
> I was thinking of using a date known not to cause any issue (something like
> 2015-07-07). I guess the code is no worse now than it is without the patch,
and
> the name are more correct with this CL, so lgtm.

Ok, I fixed the date in the middle of the summer to avoid trouble, and thus also
moved the comment of DST down to the section that actually uses it.

I tried to run this unittest in the Brazil timezone, and it failed. Not just in
the DST section, but right at the first EXPECT_EQ. As far as I understand, this
is because in our case, LocalMidnight() returns the first valid time in the day
(which could be 1am in Brazil), but in SQL it behaves differently. This is a
problem that should be solved at an entirely different layer; I'll bring
attention to it, but I won't attempt to solve it in this CL.

[lwchkg, 2015-10-14 16:06:12]

I found a mistake getting the time at 07-07-2015. Let's see if fixing it makes
the test pass.

Anyway, when I have time (hopefully this weekend), I'll check the SQLite code to
see if there's any mistake.

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

https://codereview.chromium.org/1389923004/diff/40001/components/history/core...
components/history/core/browser/visit_database_unittest.cc:423:
ASSERT_TRUE(Time::FromUTCString("2015-07-07", &today));
Should be Time::FromString to return a local time.

Your time is actually 21:00 in Brazilian timezone. Adding four hours it's 1:00
in the next day. That's why the test fails.

[lwchkg, 2015-10-14 16:32:27]

> I tried to run this unittest in the Brazil timezone, and it failed. Not just
in
> the DST section, but right at the first EXPECT_EQ. As far as I understand,
this
> is because in our case, LocalMidnight() returns the first valid time in the
day

Not true. According to time.h, different OSes returns the time liberally. It can
be 1am of the day (the first valid time), or 11pm of the previous day. To get a
fix we need to consider both cases.

And also note that the 1am is only 23hrs from the next day, but without a time
shift inside time interval. So the final code will be complex. As sdefresne gave
the l-g-t-m already, it may be a good idea to fix this in another CL.

[msramek, 2015-10-15 08:57:41]

On 2015/10/14 16:32:27, lwchkg wrote:
> > I tried to run this unittest in the Brazil timezone, and it failed. Not just
> in
> > the DST section, but right at the first EXPECT_EQ. As far as I understand,
> this
> > is because in our case, LocalMidnight() returns the first valid time in the
> day
> 
> Not true. According to time.h, different OSes returns the time liberally. It
can
> be 1am of the day (the first valid time), or 11pm of the previous day. To get
a
> fix we need to consider both cases.
> 
> And also note that the 1am is only 23hrs from the next day, but without a time
> shift inside time interval. So the final code will be complex. As sdefresne
gave
> the l-g-t-m already, it may be a good idea to fix this in another CL.

Agreed. Let me just change the FromUTCString to FromString, so that I don't make
it worse.

Thanks for being so thorough with reviewing, lwchkg@!

[msramek, 2015-10-15 08:57:56]

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

https://codereview.chromium.org/1389923004/diff/40001/components/history/core...
components/history/core/browser/visit_database_unittest.cc:423:
ASSERT_TRUE(Time::FromUTCString("2015-07-07", &today));
On 2015/10/14 16:06:11, lwchkg wrote:
> Should be Time::FromString to return a local time.
> 
> Your time is actually 21:00 in Brazilian timezone. Adding four hours it's 1:00
> in the next day. That's why the test fails.

Changed to FromString.

[msramek, 2015-10-15 08:58:53]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-10-15 08:58:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/1389923004/#ps60001
(title: "FromString instead of FromUTCString")

[commit-bot: I haz the power, 2015-10-15 08:59:08]

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

[commit-bot: I haz the power, 2015-10-15 09:35:14]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-15 09:35:57]

Patchset 4 (id:??) landed as
https://crrev.com/f314f82408c9427b2530360164b21f30e98144ff
Cr-Commit-Position: refs/heads/master@{#354242}