Show progress bar as soon as update state changed to UPDATE_STATUS_UPDATE_AVAILABLE

Show progress bar as soon as update state changed to UPDATE_STATUS_UPDATE_AVAILABLE

BUG=chromium-os:15263
TEST=manual

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

[Nikita (slow), 2011-06-07 15:31:03]

LGTM

http://codereview.chromium.org/7057055/diff/5001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/update_screen.cc (right):

http://codereview.chromium.org/7057055/diff/5001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/update_screen.cc:90: if (ignore_idle_status_ &&
status > UPDATE_STATUS_IDLE) {
This approach feels a bit dangerous because it relies on the fact that
UPDATE_STATUS_CHECKING_FOR_UPDATE is sent at least once.
I think it's the case but if it regresses (1) would make stuck on this screen.

(1) No update, no error
IDLE > CHECKING_FOR_UPDATE > IDLE

(2) Has update, installed
IDLE > CHECKING_FOR_UPDATE > ... > UPDATED_NEED_REBOOT

(3) Error
IDLE > CHECKING_FOR_UPDATE > ... > ERROR

http://codereview.chromium.org/7057055/diff/5001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/update_screen.h (right):

http://codereview.chromium.org/7057055/diff/5001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/update_screen.h:94: // Note, this is fasle by
default.
nit: false

[Dmitry Polukhin, 2011-06-08 07:24:20]

http://codereview.chromium.org/7057055/diff/5001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/update_screen.cc (right):

http://codereview.chromium.org/7057055/diff/5001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/update_screen.cc:90: if (ignore_idle_status_ &&
status > UPDATE_STATUS_IDLE) {
On 2011/06/07 15:31:03, Nikita Kostylev wrote:
> This approach feels a bit dangerous because it relies on the fact that
> UPDATE_STATUS_CHECKING_FOR_UPDATE is sent at least once.
> I think it's the case but if it regresses (1) would make stuck on this screen.
> 
> (1) No update, no error
> IDLE > CHECKING_FOR_UPDATE > IDLE
> 
> (2) Has update, installed
> IDLE > CHECKING_FOR_UPDATE > ... > UPDATED_NEED_REBOOT
> 
> (3) Error
> IDLE > CHECKING_FOR_UPDATE > ... > ERROR

My approach assumes that status other than IDLE will be sent at least once
before getting back to IDLE. I added code to set ignore_idle_status_ to callback
from RequestUpdateCheck. But in this case if initial IDLE status is sent after
callback from RequestUpdateCheck, update screen will be skipped. Overall with
current update API it is very hard to get result of your updated check. It would
be better if updated engine directly notifies caller (via callback passed to
UpdateCheck) about status of its update check.

http://codereview.chromium.org/7057055/diff/5001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/update_screen.h (right):

http://codereview.chromium.org/7057055/diff/5001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/update_screen.h:94: // Note, this is fasle by
default.
On 2011/06/07 15:31:03, Nikita Kostylev wrote:
> nit: false

Done.

[petkov, 2011-06-08 18:28:59]

http://codereview.chromium.org/7057055/diff/5001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/update_screen.cc (right):

http://codereview.chromium.org/7057055/diff/5001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/update_screen.cc:90: if (ignore_idle_status_ &&
status > UPDATE_STATUS_IDLE) {
On 2011/06/08 07:24:21, Dmitry Polukhin wrote:
> On 2011/06/07 15:31:03, Nikita Kostylev wrote:
> > This approach feels a bit dangerous because it relies on the fact that
> > UPDATE_STATUS_CHECKING_FOR_UPDATE is sent at least once.
> > I think it's the case but if it regresses (1) would make stuck on this
screen.
> > 
> > (1) No update, no error
> > IDLE > CHECKING_FOR_UPDATE > IDLE
> > 
> > (2) Has update, installed
> > IDLE > CHECKING_FOR_UPDATE > ... > UPDATED_NEED_REBOOT
> > 
> > (3) Error
> > IDLE > CHECKING_FOR_UPDATE > ... > ERROR
> 
> My approach assumes that status other than IDLE will be sent at least once
> before getting back to IDLE. I added code to set ignore_idle_status_ to
callback
> from RequestUpdateCheck. But in this case if initial IDLE status is sent after
> callback from RequestUpdateCheck, update screen will be skipped. Overall with
> current update API it is very hard to get result of your updated check. It
would
> be better if updated engine directly notifies caller (via callback passed to
> UpdateCheck) about status of its update check.

You could use update_engine's GetStatus D-Bus method to inquire about its status
at any given point -- I think there's already libcros support for it. If the
status is IDLE _after_ you've called AttemptUpdate then there's definitely no
update. Does this solve the issue?

[Dmitry Polukhin, 2011-06-09 12:51:32]

http://codereview.chromium.org/7057055/diff/5001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/update_screen.cc (right):

http://codereview.chromium.org/7057055/diff/5001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/update_screen.cc:90: if (ignore_idle_status_ &&
status > UPDATE_STATUS_IDLE) {
On 2011/06/08 18:29:00, petkov wrote:
> On 2011/06/08 07:24:21, Dmitry Polukhin wrote:
> > On 2011/06/07 15:31:03, Nikita Kostylev wrote:
> > > This approach feels a bit dangerous because it relies on the fact that
> > > UPDATE_STATUS_CHECKING_FOR_UPDATE is sent at least once.
> > > I think it's the case but if it regresses (1) would make stuck on this
> screen.
> > > 
> > > (1) No update, no error
> > > IDLE > CHECKING_FOR_UPDATE > IDLE
> > > 
> > > (2) Has update, installed
> > > IDLE > CHECKING_FOR_UPDATE > ... > UPDATED_NEED_REBOOT
> > > 
> > > (3) Error
> > > IDLE > CHECKING_FOR_UPDATE > ... > ERROR
> > 
> > My approach assumes that status other than IDLE will be sent at least once
> > before getting back to IDLE. I added code to set ignore_idle_status_ to
> callback
> > from RequestUpdateCheck. But in this case if initial IDLE status is sent
after
> > callback from RequestUpdateCheck, update screen will be skipped. Overall
with
> > current update API it is very hard to get result of your updated check. It
> would
> > be better if updated engine directly notifies caller (via callback passed to
> > UpdateCheck) about status of its update check.
> 
> You could use update_engine's GetStatus D-Bus method to inquire about its
status
> at any given point -- I think there's already libcros support for it. If the
> status is IDLE _after_ you've called AttemptUpdate then there's definitely no
> update. Does this solve the issue?

Is it guaranteed that update engine will not delay update ping for some time and
change status on return from AttemptUpdate? Also I'm worry about async
communication between Chrome and update engine i.e. Chrome may use old cached
update status. If updated engine always sends CHECKING_FOR_UPDATE status,
current approach should be reliable. As far as I can see in update engine code
it always sends CHECKING_FOR_UPDATE status.

[petkov, 2011-06-09 13:24:20]

http://codereview.chromium.org/7057055/diff/5001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/update_screen.cc (right):

http://codereview.chromium.org/7057055/diff/5001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/update_screen.cc:90: if (ignore_idle_status_ &&
status > UPDATE_STATUS_IDLE) {
On 2011/06/09 12:51:32, Dmitry Polukhin wrote:
> On 2011/06/08 18:29:00, petkov wrote:
> > On 2011/06/08 07:24:21, Dmitry Polukhin wrote:
> > > On 2011/06/07 15:31:03, Nikita Kostylev wrote:
> > > > This approach feels a bit dangerous because it relies on the fact that
> > > > UPDATE_STATUS_CHECKING_FOR_UPDATE is sent at least once.
> > > > I think it's the case but if it regresses (1) would make stuck on this
> > screen.
> > > > 
> > > > (1) No update, no error
> > > > IDLE > CHECKING_FOR_UPDATE > IDLE
> > > > 
> > > > (2) Has update, installed
> > > > IDLE > CHECKING_FOR_UPDATE > ... > UPDATED_NEED_REBOOT
> > > > 
> > > > (3) Error
> > > > IDLE > CHECKING_FOR_UPDATE > ... > ERROR
> > > 
> > > My approach assumes that status other than IDLE will be sent at least once
> > > before getting back to IDLE. I added code to set ignore_idle_status_ to
> > callback
> > > from RequestUpdateCheck. But in this case if initial IDLE status is sent
> after
> > > callback from RequestUpdateCheck, update screen will be skipped. Overall
> with
> > > current update API it is very hard to get result of your updated check. It
> > would
> > > be better if updated engine directly notifies caller (via callback passed
to
> > > UpdateCheck) about status of its update check.
> > 
> > You could use update_engine's GetStatus D-Bus method to inquire about its
> status
> > at any given point -- I think there's already libcros support for it. If the
> > status is IDLE _after_ you've called AttemptUpdate then there's definitely
no
> > update. Does this solve the issue?
> 
> Is it guaranteed that update engine will not delay update ping for some time
and
> change status on return from AttemptUpdate? Also I'm worry about async
> communication between Chrome and update engine i.e. Chrome may use old cached
> update status. If updated engine always sends CHECKING_FOR_UPDATE status,
> current approach should be reliable. As far as I can see in update engine code
> it always sends CHECKING_FOR_UPDATE status.

That's right. As long as the status is IDLE when the AttemptUpdate call is
received, it's guaranteed that update_engine will send CHECKING_FOR_UPDATE
before returning from the AttemptUpdate call.