Modify UpdateLibrary to use async RequestUpdateCheck call

Modify UpdateLibrary to use async RequestUpdateCheck call

BUG=chromium-os:12743
TEST=Load settings and About; both should load in a reasonable amount of time. Verify that About / update works properly. Test that a failed update (e.g. disconenct network while in progress) behaves as expected.

Depends on: http://codereview.chromium.org/6648007/

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

[stevenjb, 2011-03-08 22:30:31]

Please take a look, P0 issue for R11, thanks.

[stevenjb, 2011-03-09 06:20:59]

+altimofeev

[altimofeev, 2011-03-09 10:42:26]

http://codereview.chromium.org/6648009/diff/3001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/update_library.h (right):

http://codereview.chromium.org/6648009/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/update_library.h:66: virtual void CheckForUpdate()
= 0;
Please, fix the comment. Also, consider deprecating this method.

http://codereview.chromium.org/6648009/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/update_library.h:69: virtual void
RequestUpdate(chromeos::UpdateCallback callback,
Why don't use RequestUpdateCheck for the consistency with libcros?

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

http://codereview.chromium.org/6648009/diff/3001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/update_screen.cc:138: void update_callback(void*
user_data, UpdateResult result, const char* msg) {
nit: Please, use BumpyCase for the function's name. Also, please, add comment or
use more specific function's name.

http://codereview.chromium.org/6648009/diff/3001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/update_screen.cc:142:
screen->ExitUpdate(UpdateScreen::REASON_UPDATE_INIT_FAILED);
What if update screen has been destroyed before this callback is fired? (e.g.
user skips update).

[stevenjb, 2011-03-10 01:23:15]

http://codereview.chromium.org/6648009/diff/3001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/update_library.h (right):

http://codereview.chromium.org/6648009/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/update_library.h:66: virtual void CheckForUpdate()
= 0;
On 2011/03/09 10:42:27, altimofeev wrote:
> Please, fix the comment. Also, consider deprecating this method.
Removed.

http://codereview.chromium.org/6648009/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/update_library.h:69: virtual void
RequestUpdate(chromeos::UpdateCallback callback,
On 2011/03/09 10:42:27, altimofeev wrote:
> Why don't use RequestUpdateCheck for the consistency with libcros?
Sure. Done.

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

http://codereview.chromium.org/6648009/diff/3001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/update_screen.cc:138: void update_callback(void*
user_data, UpdateResult result, const char* msg) {
On 2011/03/09 10:42:27, altimofeev wrote:
> nit: Please, use BumpyCase for the function's name. Also, please, add comment
or
> use more specific function's name.
Oops. Relapse. Done.

http://codereview.chromium.org/6648009/diff/3001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/update_screen.cc:142:
screen->ExitUpdate(UpdateScreen::REASON_UPDATE_INIT_FAILED);
On 2011/03/09 10:42:27, altimofeev wrote:
> What if update screen has been destroyed before this callback is fired? (e.g.
> user skips update).
Good catch. I will add an instance list to check. Shouldn't need anything fancy
since this all happens on the same thread.

[altimofeev, 2011-03-10 08:27:34]

Thanks!
LGTM with nits:

http://codereview.chromium.org/6648009/diff/8006/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/update_library.h (right):

http://codereview.chromium.org/6648009/diff/8006/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/update_library.h:65: // Request an update check and
call |callback| when the request has completed.
nit: Requests, calls (third person singular)

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

http://codereview.chromium.org/6648009/diff/8006/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/update_screen.h:78: typedef
std::set<UpdateScreen*> InstanceSet;
I would prefer to use a hash_set here. But using set is not a problem too.

http://codereview.chromium.org/6648009/diff/8006/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/about_page_handler.cc (right):

http://codereview.chromium.org/6648009/diff/8006/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/about_page_handler.cc:319:
RequestUpdateCheck(NULL /*no callback*/, NULL /*no userdata*/);
nit: AFAIK, there is no such comment style. But you can move comments to one
line before the method call:
// Request update check without callback and user data.
chromeos::CrosLibrary::Get()->GetUpdateLibrary()->
    RequestUpdateCheck(NULL, NULL);

[Nikita (slow), 2011-03-10 11:32:04]

LGTM

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

http://codereview.chromium.org/6648009/diff/8006/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/update_screen.cc:157: void
StartUpdateCallback(void* user_data, UpdateResult result, const char* msg) {
nit: 80 chars limit, i.e. split params - one per line

http://codereview.chromium.org/6648009/diff/8006/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/about_page_handler.cc (right):

http://codereview.chromium.org/6648009/diff/8006/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/about_page_handler.cc:319:
RequestUpdateCheck(NULL /*no callback*/, NULL /*no userdata*/);
Should place each NULL parameter on a separate line and use // style comment
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Implementation...

[stevenjb, 2011-03-10 17:28:50]

Thanks! Fixed nits, pushing now.

http://codereview.chromium.org/6648009/diff/8006/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/update_library.h (right):

http://codereview.chromium.org/6648009/diff/8006/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/update_library.h:65: // Request an update check and
call |callback| when the request has completed.
On 2011/03/10 08:27:34, altimofeev wrote:
> nit: Requests, calls (third person singular)

Done.

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

http://codereview.chromium.org/6648009/diff/8006/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/update_screen.cc:157: void
StartUpdateCallback(void* user_data, UpdateResult result, const char* msg) {
On 2011/03/10 11:32:04, Nikita Kostylev wrote:
> nit: 80 chars limit, i.e. split params - one per line

Done.

http://codereview.chromium.org/6648009/diff/8006/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/about_page_handler.cc (right):

http://codereview.chromium.org/6648009/diff/8006/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/about_page_handler.cc:319:
RequestUpdateCheck(NULL /*no callback*/, NULL /*no userdata*/);
On 2011/03/10 11:32:04, Nikita Kostylev wrote:
> Should place each NULL parameter on a separate line and use // style comment
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Implementation...

Done.