Add private system extension API to get system update status

Add private system extension API to get system update status

BUG=chromium-os:17585
TEST=browser_tests --gtest_filter=SystemPrivateChromeOsApiTest.GetUpdateStatus

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

[dmazzoni, 2011-11-30 15:46:04]

LGTM.

Nice job.

http://codereview.chromium.org/8747003/diff/6002/chrome/browser/chromeos/acce...
File
chrome/browser/chromeos/accessibility/accessibility_private_extension_api.cc
(right):

http://codereview.chromium.org/8747003/diff/6002/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/accessibility_private_extension_api.cc:1:
#include
"chrome/browser/chromeos/accessibility/accessibility_private_extension_api.h"
Need copyright at top of file

http://codereview.chromium.org/8747003/diff/6002/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/accessibility_private_extension_api.cc:24:
chromeos::DBusThreadManager::Get()->GetUpdateEngineClient()->
Do you need to do NULL checks of any of these? Is it possible for
chromeos::DBusThreadManager::Get to return NULL? What about
GetUpdateEngineClient?

Maybe these might be NULL if running the chromeos=1 version of chrome on a
desktop Linux system without libcros?

http://codereview.chromium.org/8747003/diff/6002/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8747003/diff/6002/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:540: "description": "State of
system update."
Document briefly what these mean. (For example, make it clear that NeedReboot
means that the update has been downloaded and installed and will take effect as
soon as the user reboots.)

What if there's an error retrieving the update status? What happens?

http://codereview.chromium.org/8747003/diff/6002/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:544: "description": "Pregress of
download. [0.0 - 1.0]"
Document that this will be 0 if the update is NotAvailable and 1 if the update
is NeedReboot.

[hashimoto, 2011-12-01 02:28:00]

New patch set.

http://codereview.chromium.org/8747003/diff/6002/chrome/browser/chromeos/acce...
File
chrome/browser/chromeos/accessibility/accessibility_private_extension_api.cc
(right):

http://codereview.chromium.org/8747003/diff/6002/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/accessibility_private_extension_api.cc:1:
#include
"chrome/browser/chromeos/accessibility/accessibility_private_extension_api.h"
On 2011/11/30 15:46:04, Dominic Mazzoni wrote:
> Need copyright at top of file

Done.

http://codereview.chromium.org/8747003/diff/6002/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/accessibility_private_extension_api.cc:24:
chromeos::DBusThreadManager::Get()->GetUpdateEngineClient()->
On 2011/11/30 15:46:04, Dominic Mazzoni wrote:
> Do you need to do NULL checks of any of these? Is it possible for
> chromeos::DBusThreadManager::Get to return NULL? What about
> GetUpdateEngineClient?
> 
> Maybe these might be NULL if running the chromeos=1 version of chrome on a
> desktop Linux system without libcros?

No need for NULL check here because a stub implementation will be used on
desktop Linux.

http://codereview.chromium.org/8747003/diff/6002/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8747003/diff/6002/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:540: "description": "State of
system update."
On 2011/11/30 15:46:04, Dominic Mazzoni wrote:
> Document briefly what these mean. (For example, make it clear that NeedReboot
> means that the update has been downloaded and installed and will take effect
as
> soon as the user reboots.)
Done.

> What if there's an error retrieving the update status? What happens?
Currently 'NotAvailable' is used for error state because the purpose of this API
is to notify users about available update, not about the detail of the update
mechanism.

http://codereview.chromium.org/8747003/diff/6002/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:544: "description": "Pregress of
download. [0.0 - 1.0]"
On 2011/11/30 15:46:04, Dominic Mazzoni wrote:
> Document that this will be 0 if the update is NotAvailable and 1 if the update
> is NeedReboot.

Done.

[Aaron Boodman, 2011-12-01 05:02:51]

We recently added a systemPrivate namespace. I think this would make more sense
in there.

[hashimoto, 2011-12-01 05:11:51]

On 2011/12/01 05:02:51, Aaron Boodman wrote:
> We recently added a systemPrivate namespace. I think this would make more
sense
> in there.

Sounds good.
I'd like to move this function into systemPrivate.
Where can I find the namespace?
I cannot find it on the extension_api.json of r112407.

Thanks,

[Aaron Boodman, 2011-12-01 06:18:24]

Ah, it hasn't landed yet:

http://codereview.chromium.org/8662008/

Soon!

On Wed, Nov 30, 2011 at 9:11 PM,  <hashimoto@chromium.org> wrote:
> On 2011/12/01 05:02:51, Aaron Boodman wrote:
>>
>> We recently added a systemPrivate namespace. I think this would make more
>
> sense
>>
>> in there.
>
>
> Sounds good.
> I'd like to move this function into systemPrivate.
> Where can I find the namespace?
> I cannot find it on the extension_api.json of r112407.
>
> Thanks,
>
> http://codereview.chromium.org/8747003/

[hashimoto, 2011-12-13 09:33:21]

Moved the function from accessibilityPrivate to systemPrivate.chromeos.

PTAL

[Aaron Boodman, 2011-12-14 00:21:56]

On Tue, Dec 13, 2011 at 1:33 AM,  <hashimoto@chromium.org> wrote:
> Moved the function from accessibilityPrivate to systemPrivate.chromeos.

Does it needs to be in .chromeos? This functionality seems applicable
to all Chromes.

- a

[hashimoto, 2011-12-14 01:10:14]

On 2011/12/14 00:21:56, Aaron Boodman wrote:
> On Tue, Dec 13, 2011 at 1:33 AM,  <mailto:hashimoto@chromium.org> wrote:
> > Moved the function from accessibilityPrivate to systemPrivate.chromeos.
> 
> Does it needs to be in .chromeos? This functionality seems applicable
> to all Chromes.
> 
> - a

I made it in .chromeos because UpdateEngineClient is only available on ChromeOS.
Without UpdateEngineClient, it is somehow hard to return the correct download
progress.

Anyway, I will move this into general systemPrivate namespace and make a very
simple implementation for non-ChromeOS platforms which only returns {"state":
"NotAvailable", "download_progress": 0} or {"state": "NeedReboot",
"download_progress": 1}.

[hashimoto, 2011-12-14 06:28:01]

+battre

New patch set.
Moved the function to systemPrivate.

[Aaron Boodman, 2011-12-14 07:33:27]

thanks, lgtm w/ one naming nit.

http://codereview.chromium.org/8747003/diff/21013/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8747003/diff/21013/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:9121: "enum": ["NotAvailable",
"Updating", "NeedReboot"],
Suggest "NeedRestart" (desktop chrome doesn't reboot)

[hashimoto, 2011-12-14 08:14:32]

http://codereview.chromium.org/8747003/diff/21013/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8747003/diff/21013/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:9121: "enum": ["NotAvailable",
"Updating", "NeedReboot"],
On 2011/12/14 07:33:28, Aaron Boodman wrote:
> Suggest "NeedRestart" (desktop chrome doesn't reboot)

Sounds good.
Done.

[battre, 2011-12-14 09:21:27]

LGTM if you address my concern.

http://codereview.chromium.org/8747003/diff/26001/chrome/browser/extensions/s...
File chrome/browser/extensions/system/system_api.cc (right):

http://codereview.chromium.org/8747003/diff/26001/chrome/browser/extensions/s...
chrome/browser/extensions/system/system_api.cc:98: #else
In this branch |state| can be empty. I think it should be initialized to
kNotAvailableState if UpgradeDetector::GetInstance()->notify_upgrade() is false.

[hashimoto, 2011-12-14 09:40:02]

http://codereview.chromium.org/8747003/diff/26001/chrome/browser/extensions/s...
File chrome/browser/extensions/system/system_api.cc (right):

http://codereview.chromium.org/8747003/diff/26001/chrome/browser/extensions/s...
chrome/browser/extensions/system/system_api.cc:98: #else
On 2011/12/14 09:21:27, battre wrote:
> In this branch |state| can be empty. I think it should be initialized to
> kNotAvailableState if UpgradeDetector::GetInstance()->notify_upgrade() is
false.

You're right, thanks.
Fixed.

[commit-bot: I haz the power, 2011-12-15 02:29:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/8747003/14026

[commit-bot: I haz the power, 2011-12-15 05:04:21]

Change committed as 114596