Show the time remaining of download items in the download manager....

Show the time remaining of download items in the download manager.

BUG=9607
TEST=Download a large file and go to the download manager (Ctrl+J). The
remaining time of your current downloads should decreasen as the download is
getting closer to being done.


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

[Peter Kasting, 2009-05-26 22:42:47]

http://codereview.chromium.org/115797/diff/2001/2002
File chrome/browser/dom_ui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/115797/diff/2001/2002#newcode31
Line 31: using base::TimeDelta;
Nit: Don't use using directives unless you absolutely, positively have to (you
don't here).

http://codereview.chromium.org/115797/diff/2001/2002#newcode340
Line 340: if (download->is_paused()) {
Nit: No { } on these arms.

http://codereview.chromium.org/115797/diff/2001/2002#newcode343
Line 343: time_remaining = TimeFormat::TimeRemaining(remaining);
Nit: If this can't return an empty string, then pull the first conditional arm
below up to an "else" after this arm.

http://codereview.chromium.org/115797/diff/2001/2002#newcode350
Line 350: } else {
Nit: No else after return.

[arv (Not doing code reviews), 2009-05-26 22:50:00]

PTAL

http://codereview.chromium.org/115797/diff/2001/2002
File chrome/browser/dom_ui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/115797/diff/2001/2002#newcode31
Line 31: using base::TimeDelta;
On 2009/05/26 22:42:48, pkasting wrote:
> Nit: Don't use using directives unless you absolutely, positively have to (you
> don't here).

Done.

http://codereview.chromium.org/115797/diff/2001/2002#newcode340
Line 340: if (download->is_paused()) {
On 2009/05/26 22:42:48, pkasting wrote:
> Nit: No { } on these arms.

Done.

http://codereview.chromium.org/115797/diff/2001/2002#newcode343
Line 343: time_remaining = TimeFormat::TimeRemaining(remaining);
On 2009/05/26 22:42:48, pkasting wrote:
> Nit: If this can't return an empty string, then pull the first conditional arm
> below up to an "else" after this arm.

It can return an empty string

http://codereview.chromium.org/115797/diff/2001/2002#newcode350
Line 350: } else {
On 2009/05/26 22:42:48, pkasting wrote:
> Nit: No else after return.

Done.

[Peter Kasting, 2009-05-26 22:54:38]

http://codereview.chromium.org/115797/diff/1022/1023
File chrome/browser/dom_ui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/115797/diff/1022/1023#newcode347
Line 347: else
Still have this else after return.

[arv (Not doing code reviews), 2009-05-26 22:59:32]

http://codereview.chromium.org/115797/diff/1022/1023
File chrome/browser/dom_ui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/115797/diff/1022/1023#newcode347
Line 347: else
On 2009/05/26 22:54:38, pkasting wrote:
> Still have this else after return.

Sorry about that. It should be fixed now.

[Peter Kasting, 2009-05-26 23:18:30]

LGTM with nits

http://codereview.chromium.org/115797/diff/1029/1031
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/115797/diff/1029/1031#newcode1557
Line 1557: desc="The status text for a download in progress in the download
manager. This include information such as speed and received byte counts and is
used when we do not know the remaining time">
Nit: include -> includes (two places)

http://codereview.chromium.org/115797/diff/1029/1030
File chrome/browser/dom_ui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/115797/diff/1029/1030#newcode344
Line 344: return
l10n_util::GetStringF(IDS_DOWNLOAD_TAB_PROGRESS_STATUS_TIME_UNKNOWN,
Nit: Now, on _this_ conditional body, since it's >1 line, you do need { } :)

(Random passing note: giving each arg its own line is fairly uncommon within our
code, we usually do as many args as will fit on a line)