chrome://downloads updateDateDisplay more often

chrome://downloads updateDateDisplay more often.
Also use an empty since string for future dates instead of 'Today'.

Reviewers:
estade: resources/downloads/downloads.js, webui/downloads_dom_handler.cc

Done:
rdsmith, asanka: *

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

[benjhayden, 2012-09-17 16:05:40]

PTAL

[asanka, 2012-09-17 16:48:14]

http://codereview.chromium.org/10918274/diff/2001/chrome/browser/resources/do...
File chrome/browser/resources/downloads/downloads.js (right):

http://codereview.chromium.org/10918274/diff/2001/chrome/browser/resources/do...
chrome/browser/resources/downloads/downloads.js:108: //
Download.prototype.update may change its nodeSince_ and nodeDate_, so
Is nodeSince_ updated for all the download items after a clock adjustment?

[benjhayden, 2012-09-17 17:02:38]

Time is hard.

http://codereview.chromium.org/10918274/diff/2001/chrome/browser/resources/do...
File chrome/browser/resources/downloads/downloads.js (right):

http://codereview.chromium.org/10918274/diff/2001/chrome/browser/resources/do...
chrome/browser/resources/downloads/downloads.js:108: //
Download.prototype.update may change its nodeSince_ and nodeDate_, so
On 2012/09/17 16:48:14, asanka wrote:
> Is nodeSince_ updated for all the download items after a clock adjustment?

Yes, downloads_dom_handler.cc since_string uses TimeFormat::RelativeDate(),
which calls Time::Now(), which tracks the system clock according to base/time.h.
But no, merely changing the clock doesn't trigger any events that
DownloadsDOMHandler watches for. The next time a download is created or
destroyed, all the items will be updated at once. A single OnDownloadUpdated
event without download creation or deletion might mean that one entry in
chrome://downloads reflects the new time but the rest don't. This fix at least
prevents there from being two 'Today' sections with the same date, though there
may be two 'Today' sections with different dates. I could probably hack some
javascript together to detect same since strings with different dates and call
getDownloads, but I think that the only way to get in this state (after this fix
is in) would be if you changed the system clock while a large file was actually
downloading or a dangerous file was waiting to be confirmed, which seems rare
and not worth the effort.

[asanka, 2012-09-17 17:50:14]

http://codereview.chromium.org/10918274/diff/2001/chrome/browser/resources/do...
File chrome/browser/resources/downloads/downloads.js (right):

http://codereview.chromium.org/10918274/diff/2001/chrome/browser/resources/do...
chrome/browser/resources/downloads/downloads.js:108: //
Download.prototype.update may change its nodeSince_ and nodeDate_, so
On 2012/09/17 17:02:38, benjhayden_chromium wrote:
> On 2012/09/17 16:48:14, asanka wrote:
> > Is nodeSince_ updated for all the download items after a clock adjustment?
> 
> Yes, downloads_dom_handler.cc since_string uses TimeFormat::RelativeDate(),
> which calls Time::Now(), which tracks the system clock according to
base/time.h.
> But no, merely changing the clock doesn't trigger any events that
> DownloadsDOMHandler watches for. The next time a download is created or
> destroyed, all the items will be updated at once. A single OnDownloadUpdated
> event without download creation or deletion might mean that one entry in
> chrome://downloads reflects the new time but the rest don't. This fix at least
> prevents there from being two 'Today' sections with the same date, though
there
> may be two 'Today' sections with different dates. I could probably hack some
> javascript together to detect same since strings with different dates and call
> getDownloads, but I think that the only way to get in this state (after this
fix
> is in) would be if you changed the system clock while a large file was
actually
> downloading or a dangerous file was waiting to be confirmed, which seems rare
> and not worth the effort.

Yeah. I wasn't sure what the intended fix was. It seems there are other places
where we assume that downloads are created in wall clock order, and this issue
is partly about that assumption being false.

Also, it looks like TimeFormat::RelativeDate() will return 'Today' for all dates
that are in the future. So if there is a download in history with a start date
more than a day in the future, that item as well as any downloads that were in
fact started "today" will all get the 'Today' label. So even with this fix,
there would be two "Today" sections. Isn't it?

[benjhayden, 2012-09-17 18:09:42]

http://codereview.chromium.org/10918274/diff/2001/chrome/browser/resources/do...
File chrome/browser/resources/downloads/downloads.js (right):

http://codereview.chromium.org/10918274/diff/2001/chrome/browser/resources/do...
chrome/browser/resources/downloads/downloads.js:108: //
Download.prototype.update may change its nodeSince_ and nodeDate_, so
On 2012/09/17 17:50:14, asanka wrote:
> On 2012/09/17 17:02:38, benjhayden_chromium wrote:
> > On 2012/09/17 16:48:14, asanka wrote:
> > > Is nodeSince_ updated for all the download items after a clock adjustment?
> > 
> > Yes, downloads_dom_handler.cc since_string uses TimeFormat::RelativeDate(),
> > which calls Time::Now(), which tracks the system clock according to
> base/time.h.
> > But no, merely changing the clock doesn't trigger any events that
> > DownloadsDOMHandler watches for. The next time a download is created or
> > destroyed, all the items will be updated at once. A single OnDownloadUpdated
> > event without download creation or deletion might mean that one entry in
> > chrome://downloads reflects the new time but the rest don't. This fix at
least
> > prevents there from being two 'Today' sections with the same date, though
> there
> > may be two 'Today' sections with different dates. I could probably hack some
> > javascript together to detect same since strings with different dates and
call
> > getDownloads, but I think that the only way to get in this state (after this
> fix
> > is in) would be if you changed the system clock while a large file was
> actually
> > downloading or a dangerous file was waiting to be confirmed, which seems
rare
> > and not worth the effort.
> 
> Yeah. I wasn't sure what the intended fix was. It seems there are other places
> where we assume that downloads are created in wall clock order, and this issue
> is partly about that assumption being false.
> 
> Also, it looks like TimeFormat::RelativeDate() will return 'Today' for all
dates
> that are in the future. So if there is a download in history with a start date
> more than a day in the future, that item as well as any downloads that were in
> fact started "today" will all get the 'Today' label. So even with this fix,
> there would be two "Today" sections. Isn't it?

I like car analogies, even though I know next to nothing about cars.
This would be like propping your car up on blocks and running it in reverse
until the odometer ran negative. Of course the car wasn't driven -100 miles.
You're just asking it to lie to you.
I suppose we could detect future downloads and change the since string from
Today to 'Pass the sonic screwdriver' or 'Back... to the Future!' or some other
scifi reference.

[Randy Smith (Not in Mondays), 2012-09-17 19:55:49]

I'm hesitant to suggest this (as I don't think a massive, complex CL is the
right answer to this problem, and simple suggestions around time often turn into
massive, complex CLs) but can you track whether or not the system clocks been
updated in DownloadsDomHandler and do a full update iff it has been?

[Randy Smith (Not in Mondays), 2012-09-17 19:56:57]

On 2012/09/17 19:55:49, rdsmith wrote:
> I'm hesitant to suggest this (as I don't think a massive, complex CL is the
> right answer to this problem, and simple suggestions around time often turn
into
> massive, complex CLs) but can you track whether or not the system clocks been
> updated in DownloadsDomHandler and do a full update iff it has been?

To be explicit: I'm hoping the non-change cost of this could be dropped down to
a call to GetTime() (and maybe GetTimeTicks()) in DDH, and then when we believe
the clock has changed we push the Big Red Button.  Avoiding pushing the button
when we don't have to is the obvious tricky part of this approach.

[Randy Smith (Not in Mondays), 2012-09-17 19:58:27]

On 2012/09/17 18:09:42, benjhayden_chromium wrote:
>
http://codereview.chromium.org/10918274/diff/2001/chrome/browser/resources/do...
> File chrome/browser/resources/downloads/downloads.js (right):
> 
>
http://codereview.chromium.org/10918274/diff/2001/chrome/browser/resources/do...
> chrome/browser/resources/downloads/downloads.js:108: //
> Download.prototype.update may change its nodeSince_ and nodeDate_, so
> On 2012/09/17 17:50:14, asanka wrote:
> > On 2012/09/17 17:02:38, benjhayden_chromium wrote:
> > > On 2012/09/17 16:48:14, asanka wrote:
> > > > Is nodeSince_ updated for all the download items after a clock
adjustment?
> > > 
> > > Yes, downloads_dom_handler.cc since_string uses
TimeFormat::RelativeDate(),
> > > which calls Time::Now(), which tracks the system clock according to
> > base/time.h.
> > > But no, merely changing the clock doesn't trigger any events that
> > > DownloadsDOMHandler watches for. The next time a download is created or
> > > destroyed, all the items will be updated at once. A single
OnDownloadUpdated
> > > event without download creation or deletion might mean that one entry in
> > > chrome://downloads reflects the new time but the rest don't. This fix at
> least
> > > prevents there from being two 'Today' sections with the same date, though
> > there
> > > may be two 'Today' sections with different dates. I could probably hack
some
> > > javascript together to detect same since strings with different dates and
> call
> > > getDownloads, but I think that the only way to get in this state (after
this
> > fix
> > > is in) would be if you changed the system clock while a large file was
> > actually
> > > downloading or a dangerous file was waiting to be confirmed, which seems
> rare
> > > and not worth the effort.
> > 
> > Yeah. I wasn't sure what the intended fix was. It seems there are other
places
> > where we assume that downloads are created in wall clock order, and this
issue
> > is partly about that assumption being false.
> > 
> > Also, it looks like TimeFormat::RelativeDate() will return 'Today' for all
> dates
> > that are in the future. So if there is a download in history with a start
date
> > more than a day in the future, that item as well as any downloads that were
in
> > fact started "today" will all get the 'Today' label. So even with this fix,
> > there would be two "Today" sections. Isn't it?
> 
> I like car analogies, even though I know next to nothing about cars.
> This would be like propping your car up on blocks and running it in reverse
> until the odometer ran negative. Of course the car wasn't driven -100 miles.
> You're just asking it to lie to you.
> I suppose we could detect future downloads and change the since string from
> Today to 'Pass the sonic screwdriver' or 'Back... to the Future!' or some
other
> scifi reference.

While the scifi references appeal to me, for most users they'd be a bad UE,
because they'd make absolutely no sense and give no hint as to what the problem
is.  "Date in future?", or probably better, the actual date even though it's in
the future, would be preferred.

[benjhayden, 2012-09-17 20:17:43]

On 2012/09/17 19:58:27, rdsmith wrote:
> On 2012/09/17 18:09:42, benjhayden_chromium wrote:
> >
>
http://codereview.chromium.org/10918274/diff/2001/chrome/browser/resources/do...
> > File chrome/browser/resources/downloads/downloads.js (right):
> > 
> >
>
http://codereview.chromium.org/10918274/diff/2001/chrome/browser/resources/do...
> > chrome/browser/resources/downloads/downloads.js:108: //
> > Download.prototype.update may change its nodeSince_ and nodeDate_, so
> > On 2012/09/17 17:50:14, asanka wrote:
> > > On 2012/09/17 17:02:38, benjhayden_chromium wrote:
> > > > On 2012/09/17 16:48:14, asanka wrote:
> > > > > Is nodeSince_ updated for all the download items after a clock
> adjustment?
> > > > 
> > > > Yes, downloads_dom_handler.cc since_string uses
> TimeFormat::RelativeDate(),
> > > > which calls Time::Now(), which tracks the system clock according to
> > > base/time.h.
> > > > But no, merely changing the clock doesn't trigger any events that
> > > > DownloadsDOMHandler watches for. The next time a download is created or
> > > > destroyed, all the items will be updated at once. A single
> OnDownloadUpdated
> > > > event without download creation or deletion might mean that one entry in
> > > > chrome://downloads reflects the new time but the rest don't. This fix at
> > least
> > > > prevents there from being two 'Today' sections with the same date,
though
> > > there
> > > > may be two 'Today' sections with different dates. I could probably hack
> some
> > > > javascript together to detect same since strings with different dates
and
> > call
> > > > getDownloads, but I think that the only way to get in this state (after
> this
> > > fix
> > > > is in) would be if you changed the system clock while a large file was
> > > actually
> > > > downloading or a dangerous file was waiting to be confirmed, which seems
> > rare
> > > > and not worth the effort.
> > > 
> > > Yeah. I wasn't sure what the intended fix was. It seems there are other
> places
> > > where we assume that downloads are created in wall clock order, and this
> issue
> > > is partly about that assumption being false.
> > > 
> > > Also, it looks like TimeFormat::RelativeDate() will return 'Today' for all
> > dates
> > > that are in the future. So if there is a download in history with a start
> date
> > > more than a day in the future, that item as well as any downloads that
were
> in
> > > fact started "today" will all get the 'Today' label. So even with this
fix,
> > > there would be two "Today" sections. Isn't it?
> > 
> > I like car analogies, even though I know next to nothing about cars.
> > This would be like propping your car up on blocks and running it in reverse
> > until the odometer ran negative. Of course the car wasn't driven -100 miles.
> > You're just asking it to lie to you.
> > I suppose we could detect future downloads and change the since string from
> > Today to 'Pass the sonic screwdriver' or 'Back... to the Future!' or some
> other
> > scifi reference.
> 
> While the scifi references appeal to me, for most users they'd be a bad UE,
> because they'd make absolutely no sense and give no hint as to what the
problem
> is.  "Date in future?", or probably better, the actual date even though it's
in
> the future, would be preferred.

The date is already displayed. There's also another string, the "since string",
that contains "Today", "Yesterday", etc.
Instead of a scifi reference, perhaps "Tomorrow", "Next week", "Next month",
etc?

I'll look at how hard it would be to detect system clock jumping, but it's
separate from the since string.

[Randy Smith (Not in Mondays), 2012-09-17 20:23:37]

On 2012/09/17 20:17:43, benjhayden_chromium wrote:
> On 2012/09/17 19:58:27, rdsmith wrote:
> > On 2012/09/17 18:09:42, benjhayden_chromium wrote:
> > >
> >
>
http://codereview.chromium.org/10918274/diff/2001/chrome/browser/resources/do...
> > > File chrome/browser/resources/downloads/downloads.js (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10918274/diff/2001/chrome/browser/resources/do...
> > > chrome/browser/resources/downloads/downloads.js:108: //
> > > Download.prototype.update may change its nodeSince_ and nodeDate_, so
> > > On 2012/09/17 17:50:14, asanka wrote:
> > > > On 2012/09/17 17:02:38, benjhayden_chromium wrote:
> > > > > On 2012/09/17 16:48:14, asanka wrote:
> > > > > > Is nodeSince_ updated for all the download items after a clock
> > adjustment?
> > > > > 
> > > > > Yes, downloads_dom_handler.cc since_string uses
> > TimeFormat::RelativeDate(),
> > > > > which calls Time::Now(), which tracks the system clock according to
> > > > base/time.h.
> > > > > But no, merely changing the clock doesn't trigger any events that
> > > > > DownloadsDOMHandler watches for. The next time a download is created
or
> > > > > destroyed, all the items will be updated at once. A single
> > OnDownloadUpdated
> > > > > event without download creation or deletion might mean that one entry
in
> > > > > chrome://downloads reflects the new time but the rest don't. This fix
at
> > > least
> > > > > prevents there from being two 'Today' sections with the same date,
> though
> > > > there
> > > > > may be two 'Today' sections with different dates. I could probably
hack
> > some
> > > > > javascript together to detect same since strings with different dates
> and
> > > call
> > > > > getDownloads, but I think that the only way to get in this state
(after
> > this
> > > > fix
> > > > > is in) would be if you changed the system clock while a large file was
> > > > actually
> > > > > downloading or a dangerous file was waiting to be confirmed, which
seems
> > > rare
> > > > > and not worth the effort.
> > > > 
> > > > Yeah. I wasn't sure what the intended fix was. It seems there are other
> > places
> > > > where we assume that downloads are created in wall clock order, and this
> > issue
> > > > is partly about that assumption being false.
> > > > 
> > > > Also, it looks like TimeFormat::RelativeDate() will return 'Today' for
all
> > > dates
> > > > that are in the future. So if there is a download in history with a
start
> > date
> > > > more than a day in the future, that item as well as any downloads that
> were
> > in
> > > > fact started "today" will all get the 'Today' label. So even with this
> fix,
> > > > there would be two "Today" sections. Isn't it?
> > > 
> > > I like car analogies, even though I know next to nothing about cars.
> > > This would be like propping your car up on blocks and running it in
reverse
> > > until the odometer ran negative. Of course the car wasn't driven -100
miles.
> > > You're just asking it to lie to you.
> > > I suppose we could detect future downloads and change the since string
from
> > > Today to 'Pass the sonic screwdriver' or 'Back... to the Future!' or some
> > other
> > > scifi reference.
> > 
> > While the scifi references appeal to me, for most users they'd be a bad UE,
> > because they'd make absolutely no sense and give no hint as to what the
> problem
> > is.  "Date in future?", or probably better, the actual date even though it's
> in
> > the future, would be preferred.
> 
> The date is already displayed. 

Ah, sorry; key point I failed to note. 

> There's also another string, the "since string",
> that contains "Today", "Yesterday", etc.
> Instead of a scifi reference, perhaps "Tomorrow", "Next week", "Next month",
> etc?

Actually, in this case a scifi reference is fine by me (though I prefer "Back
.... to the future!" to "Pass the sonic screwdriver" as you won't get the second
unless you're a Dr. Who fan, and you'll probably get the first even if you're
not a Back to the Future fan.  And BTTF is more mainstream, at least in the US
:-}.)

Or you could just say "Future!".  Anything to let the user know that something
weird is going on, and we're not completely clueless about it :-}.

> 
> I'll look at how hard it would be to detect system clock jumping, but it's
> separate from the since string.

[asanka, 2012-09-17 20:26:28]

On 2012/09/17 20:17:43, benjhayden_chromium wrote:
> On 2012/09/17 19:58:27, rdsmith wrote:
> > 
> > While the scifi references appeal to me, for most users they'd be a bad UE,
> > because they'd make absolutely no sense and give no hint as to what the
> problem
> > is.  "Date in future?", or probably better, the actual date even though it's
> in
> > the future, would be preferred.
> 
> The date is already displayed. There's also another string, the "since
string",
> that contains "Today", "Yesterday", etc.
> Instead of a scifi reference, perhaps "Tomorrow", "Next week", "Next month",
> etc?
> 
> I'll look at how hard it would be to detect system clock jumping, but it's
> separate from the since string.

I think this is a very rare edge case which could happen, for example, due to
clock adjustments from an NTP sync. I'd be content with a blank "since string"
for dates in the future beyond what would be considered "Today."

Another problem is that once an entry is added to the database with a date in
the future, it would be stuck on the downloads page above current/new downloads.

[benjhayden, 2012-09-20 20:21:10]

PTAL

[asanka, 2012-09-20 21:08:41]

LGTM

The TimeFormat::RelativeDate() is only used by
chrome/browser/ui/webui/downloads_dom_handler and
chrome/browser/ui/webui/history_ui. Given that the history UI probably also
doesn't want to treat the future as today, it might make sense to make the
change in time_format.cc.

[Randy Smith (Not in Mondays), 2012-09-21 18:01:53]

LGTM>

[commit-bot: I haz the power, 2012-09-21 18:11:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10918274/24001

[commit-bot: I haz the power, 2012-09-21 18:11:05]

Presubmit check for 10918274-24001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/webui
    chrome/browser/resources

Presubmit checks took 1.3s to calculate.

[benjhayden, 2012-09-21 19:59:30]

James: PTAL?

[benjhayden, 2012-09-24 20:16:15]

Evan, PTAL?

[Evan Stade, 2012-09-25 06:17:08]

resources lgtm

[commit-bot: I haz the power, 2012-09-25 14:56:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10918274/30002

[commit-bot: I haz the power, 2012-09-25 14:56:09]

Presubmit check for 10918274-30002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 7.8s to calculate.

[James Hawkins, 2012-09-25 15:12:24]

lgtm

[commit-bot: I haz the power, 2012-09-25 15:31:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10918274/30002

[commit-bot: I haz the power, 2012-09-25 18:25:28]

Change committed as 158615