Added estimated time remaining on AU.

Added estimated time remaining on AU.

BUG=chromium:125170
TEST=Manual testing on an Alex device.


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

[Nikita (slow), 2012-05-10 16:04:31]

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen.cc (right):

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:45: const base::TimeDelta
kMinTimeStep = base::TimeDelta::FromMilliseconds(10);
Perhaps set it to at least 1s so that rate is not updated very frequently.

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:46: // Minimum allowed progress
between two consecutive ETAs.
nit: Insert extra line before each comment.

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:119:
actor_->ShowUpdateDownloadingStats(true);
What would it show if we call ExitUpdate(REASON_UPDATE_NON_CRITICAL) later in
the path?

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:167: base::Time
download_current_time = base::Time::Now();
Extract to a separate function.

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:177:
actor_->SetDownloadingRate(download_rate);
Move this call below to group with SetEstimatedTimeLeft().

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:344: bool
UpdateScreen::HasCriticalUpdate() {
Remove test code.

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen.h (right):

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.h:114: // Time of the last
notification of the downloading stage.
nit: Insert empty line before

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen_actor.h (right):

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen_actor.h:10: #include "base/time.h"
nit: insert empty line between different include types.

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe.js (right):

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe.js:161: var units = ['days',
'hours', 'minutes', 'seconds'];
These should be loaded from localizedStrings.

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe.js:168: if (!units[unit + 1] ||
values[unit] == 0) {
Bad localization pattern.
String format itself should be localized, then formatted using translated units
(in C++).

See IDS_DOWNLOAD_TAB_PROGRESS_STATUS and how it is used.

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe.js:182: var units = ['B/s', 'kB/s',
'MB/s', 'GB/s', 'TB/s', 'PB/s'];
If we end up showing this to user than these should be translated i.e. loaded
from localizedStrings.

See IDS_APP_MEBIBYTES_PER_SECOND,
http://code.google.com/codesearch#OAMlx_jo-ck/src/ui/base/text/bytes_formatti...

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_update.html (right):

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_update.html:14:
class="update-stats-label"
HTML attributes should not be formatted, just 4 spaces align as usual.

[ygorshenin1, 2012-05-11 12:42:49]

Issues are fixed, still waiting for the UI review.

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen.cc (right):

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:45: const base::TimeDelta
kMinTimeStep = base::TimeDelta::FromMilliseconds(10);
On 2012/05/10 16:04:31, Nikita Kostylev wrote:
> Perhaps set it to at least 1s so that rate is not updated very frequently.

Done.

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:46: // Minimum allowed progress
between two consecutive ETAs.
On 2012/05/10 16:04:31, Nikita Kostylev wrote:
> nit: Insert extra line before each comment.

Done.

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:119:
actor_->ShowUpdateDownloadingStats(true);
Nothing. These stats will be shown iff curtain is shown.

On 2012/05/10 16:04:31, Nikita Kostylev wrote:
> What would it show if we call ExitUpdate(REASON_UPDATE_NON_CRITICAL) later in
> the path?

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:167: base::Time
download_current_time = base::Time::Now();
On 2012/05/10 16:04:31, Nikita Kostylev wrote:
> Extract to a separate function. 

Done.

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:177:
actor_->SetDownloadingRate(download_rate);
On 2012/05/10 16:04:31, Nikita Kostylev wrote:
> Move this call below to group with SetEstimatedTimeLeft().

Done.

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:344: bool
UpdateScreen::HasCriticalUpdate() {
On 2012/05/10 16:04:31, Nikita Kostylev wrote:
> Remove test code.

Done.

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen.h (right):

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.h:114: // Time of the last
notification of the downloading stage.
On 2012/05/10 16:04:31, Nikita Kostylev wrote:
> nit: Insert empty line before

Done.

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen_actor.h (right):

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen_actor.h:10: #include "base/time.h"
On 2012/05/10 16:04:31, Nikita Kostylev wrote:
> nit: insert empty line between different include types.

Done.

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_update.html (right):

http://codereview.chromium.org/10389064/diff/2002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_update.html:14:
class="update-stats-label"
On 2012/05/10 16:04:31, Nikita Kostylev wrote:
> HTML attributes should not be formatted, just 4 spaces align as usual.

Done.

[Nikita (slow), 2012-05-12 16:00:27]

patchset 5 lgtm

I'll do another round of review if anything significantly changes after UI
review.

[ygorshenin1, 2012-05-14 18:00:47]

PTAL. UI review passed. Link to the current screenshot:
file:///home/ygorshenin/www/update_screen.png

[Nikita (slow), 2012-05-15 07:25:09]

lgtm

http://codereview.chromium.org/10389064/diff/2005/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe.css (right):

http://codereview.chromium.org/10389064/diff/2005/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe.css:470: margin-left: 74px;
Use -webkit-margin-start instead of margin-left since format is RTL friendly.

http://codereview.chromium.org/10389064/diff/2005/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe.js (right):

http://codereview.chromium.org/10389064/diff/2005/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe.js:171: message =
localStrings.getStringF('downloadingTimeLeftStatusHours', 1);
Since you always use this string with 1 hour please define it exactly as is:
"About 1 hour left" so that there're no issues with verb endings when localized.

[ygorshenin1, 2012-05-15 08:12:10]

http://codereview.chromium.org/10389064/diff/2005/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe.css (right):

http://codereview.chromium.org/10389064/diff/2005/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe.css:470: margin-left: 74px;
On 2012/05/15 07:25:09, Nikita Kostylev wrote:
> Use -webkit-margin-start instead of margin-left since format is RTL friendly.

Done.

http://codereview.chromium.org/10389064/diff/2005/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe.js (right):

http://codereview.chromium.org/10389064/diff/2005/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe.js:171: message =
localStrings.getStringF('downloadingTimeLeftStatusHours', 1);
On 2012/05/15 07:25:09, Nikita Kostylev wrote:
> Since you always use this string with 1 hour please define it exactly as is:
> "About 1 hour left" so that there're no issues with verb endings when
localized.

Done.

[commit-bot: I haz the power, 2012-05-15 13:31:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/10389064/3014

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

Change committed as 137131