app_mode: Add runtime.onRestartRequired event.

app_mode: Add runtime.onRestartRequired event.

BUG=242983
R=bartfab@chromium.org, mpcomplete@chromium.org, sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208250

[bartfab (slow), 2013-06-17 23:30:59]

I don't like the idea of adding yet another singleton. I would strongly prefer
that we make the dependency explicit by providing a AutomaticRebootManager* to
the KioskAppUpdateService constructor.

Also, I think tests should be added that the notifications fire at the right
times.

https://codereview.chromium.org/16844020/diff/1/chrome/browser/chromeos/syste...
File chrome/browser/chromeos/system/automatic_reboot_manager.cc (right):

https://codereview.chromium.org/16844020/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/automatic_reboot_manager.cc:126:
AutomaticRebootManager* instance = NULL;
Instead of creating more singletons with implicit dependencies, let's pass an
AutomaticRebootManager* to the KioskAppUpdateService constructor.

https://codereview.chromium.org/16844020/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/automatic_reboot_manager.cc:160:
DCHECK(!instance);
This would require #include <base/logging.h>. However, I do not think we want to
be adding this singleton infrastructure in the first place.

https://codereview.chromium.org/16844020/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/automatic_reboot_manager.cc:204: DCHECK(instance
== this);
Nit: DCHECK_EQ()

https://codereview.chromium.org/16844020/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/16844020/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/runtime/runtime_api.cc:200: scoped_ptr<ListValue>
args(new ListValue);
Nit: #include <base/values.h>

https://codereview.chromium.org/16844020/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/runtime/runtime_api.cc:211: }
Nit: Add a fallback with a DCHECK() for invalid |reason|s.

https://codereview.chromium.org/16844020/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/runtime/runtime_api.cc:213:
DCHECK(system->event_router());
Nit: Include <base/logging.h>

https://codereview.chromium.org/16844020/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/runtime.json (right):

https://codereview.chromium.org/16844020/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/runtime.json:415: "description": "Fired when a
ChromeOS device running in kiosk mode needs to be restarted. The kiosk app
should close all its windows at its earliest convenient time to let the restart
to happen. If the app does nothing, a restart will be enforced after a 24-hour
grace period has passed.",
Nit: s/ChromeOS/Chrome OS/

Also, how about we document this method in a more generic manner and then
mention that it currently is supported for kiosk mode only? I can see us
supporting this for all session types in the future.

[xiyuan, 2013-06-18 00:02:12]

On 2013/06/17 23:30:59, bartfab wrote:
> I don't like the idea of adding yet another singleton. I would strongly prefer
> that we make the dependency explicit by providing a AutomaticRebootManager* to
> the KioskAppUpdateService constructor.
> 

I agree with your argument but not sure how to properly do that.

The prolem is that AutomaticRebootManager* is hosted inside
ChromeBrowserMainPartsChromeos.
However, we don't create KioskAppUpdateService until the app is launched and its
profile loaded sometime later. I have not been able to find a good way to expose
AutomaticRebootManager* instance.

> Also, I think tests should be added that the notifications fire at the right
> times.
> 

Agree. Tests will be added.

[bartfab (slow), 2013-06-18 00:17:44]

On 2013/06/18 00:02:12, xiyuan wrote:
> On 2013/06/17 23:30:59, bartfab wrote:
> > I don't like the idea of adding yet another singleton. I would strongly
prefer
> > that we make the dependency explicit by providing a AutomaticRebootManager*
to
> > the KioskAppUpdateService constructor.
> > 
> 
> I agree with your argument but not sure how to properly do that.
> 
> The prolem is that AutomaticRebootManager* is hosted inside
> ChromeBrowserMainPartsChromeos.
> However, we don't create KioskAppUpdateService until the app is launched and
its
> profile loaded sometime later. I have not been able to find a good way to
expose
> AutomaticRebootManager* instance.

How about we move ownership to browser_process_plartform_part_chromeos.cc
instead? There is precedent for accessors to the objects that it manages.

> > Also, I think tests should be added that the notifications fire at the right
> > times.
> > 
> 
> Agree. Tests will be added.

[xiyuan, 2013-06-18 04:40:02]

On 2013/06/18 00:17:44, bartfab wrote:
> On 2013/06/18 00:02:12, xiyuan wrote:
> > On 2013/06/17 23:30:59, bartfab wrote:
> > > I don't like the idea of adding yet another singleton. I would strongly
> prefer
> > > that we make the dependency explicit by providing a
AutomaticRebootManager*
> to
> > > the KioskAppUpdateService constructor.
> > > 
> > 
> > I agree with your argument but not sure how to properly do that.
> > 
> > The prolem is that AutomaticRebootManager* is hosted inside
> > ChromeBrowserMainPartsChromeos.
> > However, we don't create KioskAppUpdateService until the app is launched and
> its
> > profile loaded sometime later. I have not been able to find a good way to
> expose
> > AutomaticRebootManager* instance.
> 
> How about we move ownership to browser_process_plartform_part_chromeos.cc
> instead? There is precedent for accessors to the objects that it manages.
> 

Thanks for the pointer. Yep, that looks like to good place to host
AutomaticRebootManager.

[xiyuan, 2013-06-20 02:17:12]

+jyasskin

Addressed bartfab@'s comments in #1, added test and removed
onBrowserUpdateAvailable.

Please take a look. Thanks.

https://codereview.chromium.org/16844020/diff/1/chrome/browser/chromeos/syste...
File chrome/browser/chromeos/system/automatic_reboot_manager.cc (right):

https://codereview.chromium.org/16844020/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/automatic_reboot_manager.cc:126:
AutomaticRebootManager* instance = NULL;
On 2013/06/17 23:30:59, bartfab wrote:
> Instead of creating more singletons with implicit dependencies, let's pass an
> AutomaticRebootManager* to the KioskAppUpdateService constructor.

Done.

https://codereview.chromium.org/16844020/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/automatic_reboot_manager.cc:160:
DCHECK(!instance);
On 2013/06/17 23:30:59, bartfab wrote:
> This would require #include <base/logging.h>. However, I do not think we want
to
> be adding this singleton infrastructure in the first place.

Code removed.

https://codereview.chromium.org/16844020/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/automatic_reboot_manager.cc:204: DCHECK(instance
== this);
On 2013/06/17 23:30:59, bartfab wrote:
> Nit: DCHECK_EQ()

Code removed.

https://codereview.chromium.org/16844020/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/16844020/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/runtime/runtime_api.cc:200: scoped_ptr<ListValue>
args(new ListValue);
On 2013/06/17 23:30:59, bartfab wrote:
> Nit: #include <base/values.h>

Done.

https://codereview.chromium.org/16844020/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/runtime/runtime_api.cc:211: }
On 2013/06/17 23:30:59, bartfab wrote:
> Nit: Add a fallback with a DCHECK() for invalid |reason|s.

Done.

https://codereview.chromium.org/16844020/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/runtime/runtime_api.cc:213:
DCHECK(system->event_router());
On 2013/06/17 23:30:59, bartfab wrote:
> Nit: Include <base/logging.h>

Done.

https://codereview.chromium.org/16844020/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/runtime.json (right):

https://codereview.chromium.org/16844020/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/runtime.json:415: "description": "Fired when a
ChromeOS device running in kiosk mode needs to be restarted. The kiosk app
should close all its windows at its earliest convenient time to let the restart
to happen. If the app does nothing, a restart will be enforced after a 24-hour
grace period has passed.",
On 2013/06/17 23:30:59, bartfab wrote:
> Nit: s/ChromeOS/Chrome OS/
> 
> Also, how about we document this method in a more generic manner and then
> mention that it currently is supported for kiosk mode only? I can see us
> supporting this for all session types in the future.

Done.

[Matt Perry, 2013-06-20 22:42:14]

https://codereview.chromium.org/16844020/diff/9001/chrome/common/extensions/a...
File chrome/common/extensions/api/runtime.json (left):

https://codereview.chromium.org/16844020/diff/9001/chrome/common/extensions/a...
chrome/common/extensions/api/runtime.json:355: "name":
"onBrowserUpdateAvailable",
This API has already made it out to the stable channel. We can't delete it now.

[bartfab (slow), 2013-06-21 06:27:58]

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc (right):

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc:57: // Force a
chrome restart (not a logout or reboot) by closing all browsers.
Nit: Update comment and log message. Depending on what is pending (app update,
OS update, periodic reboot), this method will cause either a browser restart or
a device restart.

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc:82:
extensions::RuntimeEventRouter::RestartReason restart_reason;
Nit: Initialize restart_reason to a fallback value here in case the switch below
reaches the default case.

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service.h (right):

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.h:23: class
KioskAppUpdateServiceTest;
Nit: This is not needed. Specifying the class as a friend will forward-declare
it for you.

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:30:
KioskAppUpdateServiceTest()
Nit: No need to break this line. It should be:

KioskAppUpdateServiceTest() : app_(NULL), update_service_(NULL) {}

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:39:
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
Nit: #include "testing/gtest/include/gtest/gtest.h"

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:42:
const base::TimeDelta uptime = base::TimeDelta::FromHours(1);
Nit: #include "base/time.h"

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:43:
const std::string uptime_seconds =
Nit: #include <string>

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:50:
const base::FilePath update_reboot_needed_uptime_file =
There is no need to create this file. Since the file is empty, the
AutomaticRebootManager ignores it anyway. It only finds out that a reboot is
necessary because you call UpdateStatusChanged(). The file does nothing.

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:54:
ASSERT_TRUE(PathService::Override(chromeos::FILE_UPTIME, uptime_file));
Nit: Move this line up to the block of code that creates this file and populates
its contents.

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:67:
update_service_ = KioskAppUpdateServiceFactory::GetForProfile(profile());
Nit: #include "chrome/browser/profiles/profile.h"

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:70:
content::BrowserThread::GetBlockingPool()->FlushForTesting();
Nit: #include "content/public/browser/browser_thread.h"

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:71:
base::RunLoop().RunUntilIdle();
In browser tests, you should be using content::RunAllPendingInMessageLoop()
instead (see a recent thread titled "TestBrowserThreadBundle -- new way to setup
fake BrowserThreads in tests" on chromium-dev).

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:78:
void FireUpdateNeedReboot() {
Grammar nit: s/Update/Updated/

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:79:
UpdateEngineClient::Status status;
Nit: #include "chromeos/dbus/update_engine_client.h"

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:81:
g_browser_process->platform_part()->automatic_reboot_manager()
Nit: #include "chrome/browser/browser_process_platform_part.h"

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:82:
->UpdateStatusChanged(status);
Nit: Break lines after the arrow, not before it.

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
File chrome/browser/chromeos/system/automatic_reboot_manager.cc (right):

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/system/automatic_reboot_manager.cc:37: #include
"chrome/browser/profiles/profile_manager.h"
Nit: Not used.

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
File chrome/browser/chromeos/system/automatic_reboot_manager_observer.h (right):

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/system/automatic_reboot_manager_observer.h:23: virtual
void willShutdownAutomaticRebootManager() = 0;
Capitalization nit: s/will/Will/

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/exte...
File chrome/browser/extensions/api/runtime/runtime_api.cc (right):

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/exte...
chrome/browser/extensions/api/runtime/runtime_api.cc:50: const char
kRestartReason[] = "reason";
Where is this one used?

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/common/exten...
File chrome/common/extensions/api/runtime.json (right):

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/common/exten...
chrome/common/extensions/api/runtime.json:415: "enum":
["app_update","os_update","periodic"]
Is it intentional that there are no spaces after the commans in this enum?

[xiyuan, 2013-06-21 16:50:55]

CL updated. Please take another look. Thanks.

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/ap...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc (right):

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/ap...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc:57: // Force a
chrome restart (not a logout or reboot) by closing all browsers.
On 2013/06/21 06:27:58, bartfab wrote:
> Nit: Update comment and log message. Depending on what is pending (app update,
> OS update, periodic reboot), this method will cause either a browser restart
or
> a device restart.

Done.

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/ap...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc:82:
extensions::RuntimeEventRouter::RestartReason restart_reason;
On 2013/06/21 06:27:58, bartfab wrote:
> Nit: Initialize restart_reason to a fallback value here in case the switch
below
> reaches the default case.

Done.

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/ap...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service.h (right):

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/ap...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.h:23: class
KioskAppUpdateServiceTest;
On 2013/06/21 06:27:58, bartfab wrote:
> Nit: This is not needed. Specifying the class as a friend will forward-declare
> it for you.

Done.

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/ap...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc
(right):

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/ap...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:30:
KioskAppUpdateServiceTest()
On 2013/06/21 06:27:58, bartfab wrote:
> Nit: No need to break this line. It should be:
> 
> KioskAppUpdateServiceTest() : app_(NULL), update_service_(NULL) {}

Done.

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/ap...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:39:
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
On 2013/06/21 06:27:58, bartfab wrote:
> Nit: #include "testing/gtest/include/gtest/gtest.h"

Done.

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/ap...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:42:
const base::TimeDelta uptime = base::TimeDelta::FromHours(1);
On 2013/06/21 06:27:58, bartfab wrote:
> Nit: #include "base/time.h"

Done.

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/ap...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:43:
const std::string uptime_seconds =
On 2013/06/21 06:27:58, bartfab wrote:
> Nit: #include <string>

Done.

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/ap...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:50:
const base::FilePath update_reboot_needed_uptime_file =
On 2013/06/21 06:27:58, bartfab wrote:
> There is no need to create this file. Since the file is empty, the
> AutomaticRebootManager ignores it anyway. It only finds out that a reboot is
> necessary because you call UpdateStatusChanged(). The file does nothing.

Done.

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/ap...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:54:
ASSERT_TRUE(PathService::Override(chromeos::FILE_UPTIME, uptime_file));
On 2013/06/21 06:27:58, bartfab wrote:
> Nit: Move this line up to the block of code that creates this file and
populates
> its contents.

Done.

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/ap...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:67:
update_service_ = KioskAppUpdateServiceFactory::GetForProfile(profile());
On 2013/06/21 06:27:58, bartfab wrote:
> Nit: #include "chrome/browser/profiles/profile.h"

Might not be needed since we only pass Profile* around and do not access its
memebers.

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/ap...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:70:
content::BrowserThread::GetBlockingPool()->FlushForTesting();
On 2013/06/21 06:27:58, bartfab wrote:
> Nit: #include "content/public/browser/browser_thread.h"

Done.

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/ap...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:71:
base::RunLoop().RunUntilIdle();
On 2013/06/21 06:27:58, bartfab wrote:
> In browser tests, you should be using content::RunAllPendingInMessageLoop()
> instead (see a recent thread titled "TestBrowserThreadBundle -- new way to
setup
> fake BrowserThreads in tests" on chromium-dev).

Done.

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/ap...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:78:
void FireUpdateNeedReboot() {
On 2013/06/21 06:27:58, bartfab wrote:
> Grammar nit: s/Update/Updated/

Done.

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/ap...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:79:
UpdateEngineClient::Status status;
On 2013/06/21 06:27:58, bartfab wrote:
> Nit: #include "chromeos/dbus/update_engine_client.h"

Done.

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/ap...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:81:
g_browser_process->platform_part()->automatic_reboot_manager()
On 2013/06/21 06:27:58, bartfab wrote:
> Nit: #include "chrome/browser/browser_process_platform_part.h"

Done.

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/ap...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:82:
->UpdateStatusChanged(status);
On 2013/06/21 06:27:58, bartfab wrote:
> Nit: Break lines after the arrow, not before it.

Done.

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/sy...
File chrome/browser/chromeos/system/automatic_reboot_manager.cc (right):

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/sy...
chrome/browser/chromeos/system/automatic_reboot_manager.cc:37: #include
"chrome/browser/profiles/profile_manager.h"
On 2013/06/21 06:27:58, bartfab wrote:
> Nit: Not used.

Done.

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/sy...
File chrome/browser/chromeos/system/automatic_reboot_manager_observer.h (right):

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/sy...
chrome/browser/chromeos/system/automatic_reboot_manager_observer.h:23: virtual
void willShutdownAutomaticRebootManager() = 0;
On 2013/06/21 06:27:58, bartfab wrote:
> Capitalization nit: s/will/Will/

Done.

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/extensions/...
File chrome/browser/extensions/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/extensions/...
chrome/browser/extensions/api/runtime/runtime_api.cc:50: const char
kRestartReason[] = "reason";
On 2013/06/21 06:27:58, bartfab wrote:
> Where is this one used?

Not used and removed.

https://codereview.chromium.org/16844020/diff/9001/chrome/common/extensions/a...
File chrome/common/extensions/api/runtime.json (left):

https://codereview.chromium.org/16844020/diff/9001/chrome/common/extensions/a...
chrome/common/extensions/api/runtime.json:355: "name":
"onBrowserUpdateAvailable",
On 2013/06/20 22:42:14, Matt Perry wrote:
> This API has already made it out to the stable channel. We can't delete it
now.

Restored.

https://codereview.chromium.org/16844020/diff/9001/chrome/common/extensions/a...
File chrome/common/extensions/api/runtime.json (right):

https://codereview.chromium.org/16844020/diff/9001/chrome/common/extensions/a...
chrome/common/extensions/api/runtime.json:415: "enum":
["app_update","os_update","periodic"]
On 2013/06/21 06:27:58, bartfab wrote:
> Is it intentional that there are no spaces after the commans in this enum?

Nope and fixed.

[bartfab (slow), 2013-06-21 18:18:12]

LGTM with some nits.

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/16844020/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:67:
update_service_ = KioskAppUpdateServiceFactory::GetForProfile(profile());
On 2013/06/21 16:50:56, xiyuan wrote:
> On 2013/06/21 06:27:58, bartfab wrote:
> > Nit: #include "chrome/browser/profiles/profile.h"
> 
> Might not be needed since we only pass Profile* around and do not access its
> memebers.

Following the guideline to IWYU, you should include profile.h because you are
dealing with an instance of Profile in your code, even if you do not access its
members.

https://chromiumcodereview.appspot.com/16844020/diff/21001/chrome/browser/chr...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc (right):

https://chromiumcodereview.appspot.com/16844020/diff/21001/chrome/browser/chr...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc:57: // This method
is invoked after the app update grace period has passed.
Nit: "after the app update grace period" is still inaccurate as it refers to app
updates only.

https://chromiumcodereview.appspot.com/16844020/diff/21001/chrome/browser/chr...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc:60: LOG(WARNING) <<
"Closing all browsers. Depends on the pending restart "
Nit: s/Depends/Depending/

https://chromiumcodereview.appspot.com/16844020/diff/21001/chrome/browser/chr...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/16844020/diff/21001/chrome/browser/chr...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:88:
const extensions::Extension* app_;  // Not owned
Nit: Full stop at end of comment.

https://chromiumcodereview.appspot.com/16844020/diff/21001/chrome/browser/chr...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:89:
KioskAppUpdateService* update_service_;  // Not owned
Nit: Full stop at end of comment.

https://chromiumcodereview.appspot.com/16844020/diff/21001/chrome/browser/ext...
File chrome/browser/extensions/api/runtime/runtime_api.cc (right):

https://chromiumcodereview.appspot.com/16844020/diff/21001/chrome/browser/ext...
chrome/browser/extensions/api/runtime/runtime_api.cc:7: #include <utility>
What is this used for?

https://chromiumcodereview.appspot.com/16844020/diff/21001/chrome/browser/ext...
chrome/browser/extensions/api/runtime/runtime_api.cc:213: NOTREACHED() <<
"Unknown reboot reason=" << reason;
Nit: s/reboot/restart/

https://chromiumcodereview.appspot.com/16844020/diff/21001/chrome/common/exte...
File chrome/common/extensions/api/runtime.json (right):

https://chromiumcodereview.appspot.com/16844020/diff/21001/chrome/common/exte...
chrome/common/extensions/api/runtime.json:415: "description": "Fired when an app
needs to be restarted. The app should close all its windows at its earliest
convenient time to let the restart to happen. If the app does nothing, a restart
will be enforced after a 24-hour grace period has passed. Currently, this event
is only fired for Chrome OS kiosk apps.",
Nit: "the app needs to be restarted" is not quite accurate. It may be the app or
the entire device that needs restarting.

[xiyuan, 2013-06-21 18:33:17]

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/ap...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc
(right):

https://codereview.chromium.org/16844020/diff/9001/chrome/browser/chromeos/ap...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:67:
update_service_ = KioskAppUpdateServiceFactory::GetForProfile(profile());
On 2013/06/21 18:18:12, bartfab wrote:
> On 2013/06/21 16:50:56, xiyuan wrote:
> > On 2013/06/21 06:27:58, bartfab wrote:
> > > Nit: #include "chrome/browser/profiles/profile.h"
> > 
> > Might not be needed since we only pass Profile* around and do not access its
> > memebers.
> 
> Following the guideline to IWYU, you should include profile.h because you are
> dealing with an instance of Profile in your code, even if you do not access
its
> members.

Done.

https://codereview.chromium.org/16844020/diff/21001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc (right):

https://codereview.chromium.org/16844020/diff/21001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc:57: // This method
is invoked after the app update grace period has passed.
On 2013/06/21 18:18:13, bartfab wrote:
> Nit: "after the app update grace period" is still inaccurate as it refers to
app
> updates only.

The timer in this class is only started for app update signal. The other two
cases are handled by AutomaticRebootManager.

https://codereview.chromium.org/16844020/diff/21001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc:60: LOG(WARNING) <<
"Closing all browsers. Depends on the pending restart "
On 2013/06/21 18:18:13, bartfab wrote:
> Nit: s/Depends/Depending/

Done.

https://codereview.chromium.org/16844020/diff/21001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc
(right):

https://codereview.chromium.org/16844020/diff/21001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:88:
const extensions::Extension* app_;  // Not owned
On 2013/06/21 18:18:13, bartfab wrote:
> Nit: Full stop at end of comment.

Done.

https://codereview.chromium.org/16844020/diff/21001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:89:
KioskAppUpdateService* update_service_;  // Not owned
On 2013/06/21 18:18:13, bartfab wrote:
> Nit: Full stop at end of comment.

Done.

https://codereview.chromium.org/16844020/diff/21001/chrome/browser/extensions...
File chrome/browser/extensions/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/16844020/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/runtime/runtime_api.cc:7: #include <utility>
On 2013/06/21 18:18:13, bartfab wrote:
> What is this used for?

For std::pair used in old code in this file.

https://codereview.chromium.org/16844020/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/runtime/runtime_api.cc:213: NOTREACHED() <<
"Unknown reboot reason=" << reason;
On 2013/06/21 18:18:13, bartfab wrote:
> Nit: s/reboot/restart/

Done.

https://codereview.chromium.org/16844020/diff/21001/chrome/common/extensions/...
File chrome/common/extensions/api/runtime.json (right):

https://codereview.chromium.org/16844020/diff/21001/chrome/common/extensions/...
chrome/common/extensions/api/runtime.json:415: "description": "Fired when an app
needs to be restarted. The app should close all its windows at its earliest
convenient time to let the restart to happen. If the app does nothing, a restart
will be enforced after a 24-hour grace period has passed. Currently, this event
is only fired for Chrome OS kiosk apps.",
On 2013/06/21 18:18:13, bartfab wrote:
> Nit: "the app needs to be restarted" is not quite accurate. It may be the app
or
> the entire device that needs restarting.

Done.

[bartfab (slow), 2013-06-21 18:47:09]

Still LGTM.

https://codereview.chromium.org/16844020/diff/21001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc (right):

https://codereview.chromium.org/16844020/diff/21001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc:57: // This method
is invoked after the app update grace period has passed.
On 2013/06/21 18:33:18, xiyuan wrote:
> On 2013/06/21 18:18:13, bartfab wrote:
> > Nit: "after the app update grace period" is still inaccurate as it refers to
> app
> > updates only.
> 
> The timer in this class is only started for app update signal. The other two
> cases are handled by AutomaticRebootManager.

Makes sense. But in that case, the comment changes are actually wrong. This
entire code handles app updates only. Could you revert the comment back to what
it was, so that it talks about app updates only?

[xiyuan, 2013-06-21 19:07:05]

https://codereview.chromium.org/16844020/diff/21001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc (right):

https://codereview.chromium.org/16844020/diff/21001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc:57: // This method
is invoked after the app update grace period has passed.
On 2013/06/21 18:47:09, bartfab wrote:
> On 2013/06/21 18:33:18, xiyuan wrote:
> > On 2013/06/21 18:18:13, bartfab wrote:
> > > Nit: "after the app update grace period" is still inaccurate as it refers
to
> > app
> > > updates only.
> > 
> > The timer in this class is only started for app update signal. The other two
> > cases are handled by AutomaticRebootManager.
> 
> Makes sense. But in that case, the comment changes are actually wrong. This
> entire code handles app updates only. Could you revert the comment back to
what
> it was, so that it talks about app updates only?

Restored.

StartRestartTimer -> StartAppUpdateRestartTimer,
ForceRestart -> ForceAppUpdateRestart
to avoid confusions.

[Matt Perry, 2013-06-21 21:01:30]

https://codereview.chromium.org/16844020/diff/22021/chrome/common/extensions/...
File chrome/common/extensions/api/runtime.json (right):

https://codereview.chromium.org/16844020/diff/22021/chrome/common/extensions/...
chrome/common/extensions/api/runtime.json:355: "name":
"onBrowserUpdateAvailable",
Actually, could you mark this with "nodoc": true? That's our way of deprecating.
It looks like it's only used by 1 extension in the webstore so far, so maybe we
can still get rid of it. Add a TODO with the bug number as well.

+zelidrag who wrote the extension.

[xiyuan, 2013-06-21 21:10:48]

https://codereview.chromium.org/16844020/diff/22021/chrome/common/extensions/...
File chrome/common/extensions/api/runtime.json (right):

https://codereview.chromium.org/16844020/diff/22021/chrome/common/extensions/...
chrome/common/extensions/api/runtime.json:355: "name":
"onBrowserUpdateAvailable",
On 2013/06/21 21:01:30, Matt Perry wrote:
> Actually, could you mark this with "nodoc": true? That's our way of
deprecating.
> It looks like it's only used by 1 extension in the webstore so far, so maybe
we
> can still get rid of it. Add a TODO with the bug number as well.
> 
> +zelidrag who wrote the extension.

Done.

[Matt Perry, 2013-06-21 21:15:10]

https://codereview.chromium.org/16844020/diff/42001/chrome/common/extensions/...
File chrome/common/extensions/api/runtime.json (right):

https://codereview.chromium.org/16844020/diff/42001/chrome/common/extensions/...
chrome/common/extensions/api/runtime.json:358: "nodoc": true,
Could you add a TODO comment here saying this is deprecated in favor of
onRestartRequired, and that we should remove it when we're sure it's unused?

[xiyuan, 2013-06-21 21:20:02]

https://codereview.chromium.org/16844020/diff/42001/chrome/common/extensions/...
File chrome/common/extensions/api/runtime.json (right):

https://codereview.chromium.org/16844020/diff/42001/chrome/common/extensions/...
chrome/common/extensions/api/runtime.json:358: "nodoc": true,
On 2013/06/21 21:15:11, Matt Perry wrote:
> Could you add a TODO comment here saying this is deprecated in favor of
> onRestartRequired, and that we should remove it when we're sure it's unused?

Done.

[Matt Perry, 2013-06-21 21:55:49]

https://codereview.chromium.org/16844020/diff/47001/chrome/browser/extensions...
File chrome/browser/extensions/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/16844020/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/runtime/runtime_api.cc:201: scoped_ptr<ListValue>
args(new ListValue);
nit: use
api::runtime::OnRestartRequired::Create(api::runtime::OnRestartRequired::REASON_APP_UPDATE)
and friends instead.

(This comes from autogenerated types at
out/Debug/gen/chrome/common/extensions/api/runtime.h)

[xiyuan, 2013-06-21 22:34:16]

https://codereview.chromium.org/16844020/diff/47001/chrome/browser/extensions...
File chrome/browser/extensions/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/16844020/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/runtime/runtime_api.cc:201: scoped_ptr<ListValue>
args(new ListValue);
On 2013/06/21 21:55:49, Matt Perry wrote:
> nit: use
>
api::runtime::OnRestartRequired::Create(api::runtime::OnRestartRequired::REASON_APP_UPDATE)
> and friends instead.
> 
> (This comes from autogenerated types at
> out/Debug/gen/chrome/common/extensions/api/runtime.h)

Aha. I was thinking if we have something like this. :)
Changed to use generated code.

[Matt Perry, 2013-06-21 22:43:32]

extensions stuff lgtm with nits

https://codereview.chromium.org/16844020/diff/22023/chrome/browser/extensions...
File chrome/browser/extensions/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/16844020/diff/22023/chrome/browser/extensions...
chrome/browser/extensions/api/runtime/runtime_api.cc:53: const char
kRestartReasonPeriodic[] = "periodic";
no longer needed

https://codereview.chromium.org/16844020/diff/22023/chrome/common/extensions/...
File chrome/common/extensions/api/runtime.json (right):

https://codereview.chromium.org/16844020/diff/22023/chrome/common/extensions/...
chrome/common/extensions/api/runtime.json:423: "description": "The reason that
the event is being dispatched.",
I think it's worth documenting what each enum means - especially "periodic",
which I don't understand.

[xiyuan, 2013-06-21 22:57:17]

https://codereview.chromium.org/16844020/diff/22023/chrome/browser/extensions...
File chrome/browser/extensions/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/16844020/diff/22023/chrome/browser/extensions...
chrome/browser/extensions/api/runtime/runtime_api.cc:53: const char
kRestartReasonPeriodic[] = "periodic";
On 2013/06/21 22:43:32, Matt Perry wrote:
> no longer needed

Removed.

https://codereview.chromium.org/16844020/diff/22023/chrome/common/extensions/...
File chrome/common/extensions/api/runtime.json (right):

https://codereview.chromium.org/16844020/diff/22023/chrome/common/extensions/...
chrome/common/extensions/api/runtime.json:423: "description": "The reason that
the event is being dispatched.",
On 2013/06/21 22:43:32, Matt Perry wrote:
> I think it's worth documenting what each enum means - especially "periodic",
> which I don't understand.

Done.

[xiyuan, 2013-06-21 23:01:29]

+sky for owners review:
chrome/browser/browser_process_platform_part_chromeos.*

[sky, 2013-06-24 16:03:58]

LGTM

[commit-bot: I haz the power, 2013-06-24 16:14:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/16844020/60001

[xiyuan, 2013-06-24 20:18:49]

Committed patchset #10 manually as r208250 (presubmit successful).