Add a new app API to enable watchdog behavior in kiosk apps (issue 1970613003 by afakhry@chromium.org)

[afa...@chromium.org]

Reviewers: xiyuan
CL: https://codereview.chromium.org/1970613003/

Message:
Xiyuan, this is what I have in mind for this API. I need your advice regarding
the TODO in RuntimeAPI::OnRestartWatchdogTimeout() to determine what we should
do when we throttle a restart request.

Description:
Add a new app API to enable watchdog behavior in kiosk apps

This CL adds chrome.runtime.restartOnWatchdog(int seconds, callback) to enable
watchdog behavior in kiosk apps.

BUG=604578
TEST=browser_tests --gtest_filter=RestartOnWatchdogApiTest.RestartOnWatchdogTest

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+300, -3 lines):
A + chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/manifest.json
A chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js
M extensions/browser/api/runtime/runtime_api.h
M extensions/browser/api/runtime/runtime_api.cc
M extensions/browser/api/runtime/runtime_apitest.cc
M extensions/browser/extension_function_histogram_value.h
M extensions/common/api/runtime.json
M tools/metrics/histograms/histograms.xml

[xiy...@chromium.org]

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.cc#newcode343
extensions/browser/api/runtime/runtime_api.cc:343:
watchdog_timer_.Stop();
|watchdog_timer_| is a per-profile resource. We probably should not
allow multiple extensions to use it at the same time. If it is set by
extension A, we should not allow extension B to mess with it. That is,
the first extension grabs the resource and other extensions should get
an error.

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.cc#newcode346
extensions/browser/api/runtime/runtime_api.cc:346:
base::Bind(&RuntimeAPI::OnRestartWatchdogTimeout,
base::Unretained(this),
nit: There is another Start on OneShotTimer interface that we can pass
the |this| as receive and avoid using base::Unretained if the
OneShotTimer is a member of |this|.

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.cc#newcode358
extensions/browser/api/runtime/runtime_api.cc:358: base::TimeTicks now =
base::TimeTicks::Now();
Time is always a trick topic. base::TimeTicks is a tick count and makes
no sense when system reboots [1]. We cannot store that and compare
against it.

We have to use wall clock base::Time for this. We need to be careful
since the wall clock could be skewed and changed. We need to filter out
insane values such as long in the past or in the future. And document
the behavior in API json.

[1] https://www.chromium.org/developers/design-documents/sane-time

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.cc#newcode359
extensions/browser/api/runtime/runtime_api.cc:359: ExtensionPrefs* prefs
= ExtensionPrefs::Get(browser_context_);
Sorry for flipping on my suggestion. Let's store the reboot record in
extension's state store. See AlarmManager::WriteToStorage and its
OnExtensionLoaded.

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.cc#newcode372
extensions/browser/api/runtime/runtime_api.cc:372: // TODO(afakhry):
Figure out what to do here. Do we restart later?
We should still reboot but after the minimum reboot time is satisfied.
Propose to derive a timeout before we set the |watchdog_timer_|, where
we take the function's input and use our last reboot record to extend
that value to avoid rebooting too fast.

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.cc#newcode622
extensions/browser/api/runtime/runtime_api.cc:622:
EXTENSION_FUNCTION_VALIDATE(params.get());
Check ExtensionsBrowserClient::Get()->IsRunningInForcedAppMode() and
fail if not in kiosk mode.

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.cc#newcode646
extensions/browser/api/runtime/runtime_api.cc:646: Respond(ArgumentList(
I thought we would invoke the callback when the watchdog timer is set.
Maybe we should clarify with Matt of what he expects?

If we response when the timer fires, we need to clean up the pending
calls on subsequent calls that cancels previous timeout. Otherwise, we
leave a leak in the renderer.

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.h
File extensions/browser/api/runtime/runtime_api.h (right):

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.h#newcode138
extensions/browser/api/runtime/runtime_api.h:138: base::OneShotTimer
watchdog_timer_;
nit: #include "base/timer/timer.h"

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.h#newcode142
extensions/browser/api/runtime/runtime_api.h:142: base::TimeDelta
minimum_duration_between_restarts_;
nit: #include "base/time/time.h"

https://codereview.chromium.org/1970613003/

[afa...@chromium.org]

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.cc#newcode343
extensions/browser/api/runtime/runtime_api.cc:343:
watchdog_timer_.Stop();

On 2016/05/13 17:56:15, xiyuan wrote:
> |watchdog_timer_| is a per-profile resource. We probably should not
allow
> multiple extensions to use it at the same time. If it is set by
extension A, we
> should not allow extension B to mess with it. That is, the first
extension grabs
> the resource and other extensions should get an error.

Done.

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.cc#newcode346
extensions/browser/api/runtime/runtime_api.cc:346:
base::Bind(&RuntimeAPI::OnRestartWatchdogTimeout,
base::Unretained(this),

On 2016/05/13 17:56:15, xiyuan wrote:
> nit: There is another Start on OneShotTimer interface that we can pass
the
> |this| as receive and avoid using base::Unretained if the OneShotTimer
is a
> member of |this|.

Yes, the problem with that interface is that I won't be able to bind
|extension| and |callback| to the created callback. So I had to use a
manual Bind().

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.cc#newcode358
extensions/browser/api/runtime/runtime_api.cc:358: base::TimeTicks now =
base::TimeTicks::Now();

On 2016/05/13 17:56:15, xiyuan wrote:
> Time is always a trick topic. base::TimeTicks is a tick count and
makes no sense
> when system reboots [1]. We cannot store that and compare against it.
>
> We have to use wall clock base::Time for this. We need to be careful
since the
> wall clock could be skewed and changed. We need to filter out insane
values such
> as long in the past or in the future. And document the behavior in API
json.
>
> [1] https://www.chromium.org/developers/design-documents/sane-time

Thanks for providing the link. Changed it to base::Time.

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.cc#newcode359
extensions/browser/api/runtime/runtime_api.cc:359: ExtensionPrefs* prefs
= ExtensionPrefs::Get(browser_context_);

On 2016/05/13 17:56:15, xiyuan wrote:
> Sorry for flipping on my suggestion. Let's store the reboot record in
> extension's state store. See AlarmManager::WriteToStorage and its
> OnExtensionLoaded.

Done.

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.cc#newcode372
extensions/browser/api/runtime/runtime_api.cc:372: // TODO(afakhry):
Figure out what to do here. Do we restart later?

On 2016/05/13 17:56:15, xiyuan wrote:
> We should still reboot but after the minimum reboot time is satisfied.
Propose
> to derive a timeout before we set the |watchdog_timer_|, where we take
the
> function's input and use our last reboot record to extend that value
to avoid
> rebooting too fast.

Done. Rebooting after the minimum reboot time has passed.

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.cc#newcode622
extensions/browser/api/runtime/runtime_api.cc:622:
EXTENSION_FUNCTION_VALIDATE(params.get());

On 2016/05/13 17:56:15, xiyuan wrote:
> Check ExtensionsBrowserClient::Get()->IsRunningInForcedAppMode() and
fail if not
> in kiosk mode.

I added a TODO here instead, because I'll need more time to think about
how to simulate that in the browser tests that I added.

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.cc#newcode646
extensions/browser/api/runtime/runtime_api.cc:646: Respond(ArgumentList(

On 2016/05/13 17:56:15, xiyuan wrote:
> I thought we would invoke the callback when the watchdog timer is set.
Maybe we
> should clarify with Matt of what he expects?
>
> If we response when the timer fires, we need to clean up the pending
calls on
> subsequent calls that cancels previous timeout. Otherwise, we leave a
leak in
> the renderer.

Done. I made sure any previously scheduled callback is invoked and
cleared before we schedule another one.

Still need to clarify with Matt.

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.h
File extensions/browser/api/runtime/runtime_api.h (right):

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.h#newcode138
extensions/browser/api/runtime/runtime_api.h:138: base::OneShotTimer
watchdog_timer_;

On 2016/05/13 17:56:15, xiyuan wrote:
> nit: #include "base/timer/timer.h"

Done.

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.h#newcode142
extensions/browser/api/runtime/runtime_api.h:142: base::TimeDelta
minimum_duration_between_restarts_;

On 2016/05/13 17:56:15, xiyuan wrote:
> nit: #include "base/time/time.h"

Done.

https://codereview.chromium.org/1970613003/

[xiy...@chromium.org]

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.cc#newcode346
extensions/browser/api/runtime/runtime_api.cc:346:
base::Bind(&RuntimeAPI::OnRestartWatchdogTimeout,
base::Unretained(this),

On 2016/05/14 01:27:35, afakhry wrote:
> On 2016/05/13 17:56:15, xiyuan wrote:
> > nit: There is another Start on OneShotTimer interface that we can
pass the
> > |this| as receive and avoid using base::Unretained if the
OneShotTimer is a
> > member of |this|.
>
> Yes, the problem with that interface is that I won't be able to bind
|extension|
> and |callback| to the created callback. So I had to use a manual
Bind().

Acknowledged.

https://codereview.chromium.org/1970613003/diff/100001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/100001/extensions/browser/api/runtime/runtime_api.cc#newcode340
extensions/browser/api/runtime/runtime_api.cc:340: base::Time now =
base::Time::NowFromSystemTime();
nit: const base::Time

https://codereview.chromium.org/1970613003/diff/100001/extensions/browser/api/runtime/runtime_api.cc#newcode392
extensions/browser/api/runtime/runtime_api.cc:392: int seconds_from_now,
nit: slightly prefer to combine |now| and |seconds_from_now| into one
base::Time, say restart_time.

https://codereview.chromium.org/1970613003/diff/100001/extensions/browser/api/runtime/runtime_api.cc#newcode408
extensions/browser/api/runtime/runtime_api.cc:408: future_restart_time -
last_restart_time;
nit: future_restart_time > last_restart_time ?
future_restart_time - last_restart_time : base::TimeDelta();

Just in case the wall clock on the device has changed between reboots
and makes |last_restart_time| in the future.

https://codereview.chromium.org/1970613003/diff/100001/extensions/browser/api/runtime/runtime_api.cc#newcode421
extensions/browser/api/runtime/runtime_api.cc:421:
base::Unretained(this), extension_id));
nit: replace base::Unretained with weak_ptr_factory_.GetWeakPtr() since
we have it now.

https://codereview.chromium.org/1970613003/diff/100001/extensions/browser/api/runtime/runtime_api.cc#newcode438
extensions/browser/api/runtime/runtime_api.cc:438:
storage->SetExtensionValue(extension_id, kPrefRestartOnWatchdogTime,
We might want to make this happen before attempting to reboot. Chrome
only has 3 seconds or so time and the worry is that it is not enough to
persist the data to disk.

https://codereview.chromium.org/1970613003/diff/100001/extensions/browser/api/runtime/runtime_api.cc#newcode691
extensions/browser/api/runtime/runtime_api.cc:691: return
RespondLater();
RestartDeviceOnWatchdogTimeout() might call Response() synchronously
(e.g. on error). And we should not call this for such cases.

https://codereview.chromium.org/1970613003/

[afa...@chromium.org]

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/60001/extensions/browser/api/runtime/runtime_api.cc#newcode622
extensions/browser/api/runtime/runtime_api.cc:622:
EXTENSION_FUNCTION_VALIDATE(params.get());

On 2016/05/13 17:56:15, xiyuan wrote:

> Check ExtensionsBrowserClient::Get()->IsRunningInForcedAppMode() and
fail if not
> in kiosk mode.

Now Done.

https://codereview.chromium.org/1970613003/diff/100001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/100001/extensions/browser/api/runtime/runtime_api.cc#newcode340
extensions/browser/api/runtime/runtime_api.cc:340: base::Time now =
base::Time::NowFromSystemTime();

On 2016/05/16 18:25:17, xiyuan wrote:
> nit: const base::Time

Done.

https://codereview.chromium.org/1970613003/diff/100001/extensions/browser/api/runtime/runtime_api.cc#newcode392
extensions/browser/api/runtime/runtime_api.cc:392: int seconds_from_now,

On 2016/05/16 18:25:17, xiyuan wrote:
> nit: slightly prefer to combine |now| and |seconds_from_now| into one
> base::Time, say restart_time.

I think this might add a bit of redundancy, since passing |restart_time|
to ScheduleDelayedRestart() will make me need to do the following in the
caller:

base::Time restart_time =
base::Time::NowFromSystemTime() +
base::TimeDelta::FromSeconds(seconds_from_now);

Then do the opposite operation in ScheduleDelayedRestart():

base::Time now = base::Time::NowFromSystemTime();

base::TimeDelta delay_till_restart = restart_time - now;

also the above |now| is slightly later than the actual now when the API
was invoked.

I prefer to leave it the way it is, unless of course you see otherwise.

https://codereview.chromium.org/1970613003/diff/100001/extensions/browser/api/runtime/runtime_api.cc#newcode408
extensions/browser/api/runtime/runtime_api.cc:408: future_restart_time -
last_restart_time;

On 2016/05/16 18:25:17, xiyuan wrote:
> nit: future_restart_time > last_restart_time ?
> future_restart_time - last_restart_time : base::TimeDelta();
>
> Just in case the wall clock on the device has changed between reboots
and makes
> |last_restart_time| in the future.

Done.

https://codereview.chromium.org/1970613003/diff/100001/extensions/browser/api/runtime/runtime_api.cc#newcode421
extensions/browser/api/runtime/runtime_api.cc:421:
base::Unretained(this), extension_id));

On 2016/05/16 18:25:17, xiyuan wrote:
> nit: replace base::Unretained with weak_ptr_factory_.GetWeakPtr()
since we have
> it now.

Done.

https://codereview.chromium.org/1970613003/diff/100001/extensions/browser/api/runtime/runtime_api.cc#newcode438
extensions/browser/api/runtime/runtime_api.cc:438:
storage->SetExtensionValue(extension_id, kPrefRestartOnWatchdogTime,

On 2016/05/16 18:25:17, xiyuan wrote:
> We might want to make this happen before attempting to reboot. Chrome
only has 3
> seconds or so time and the worry is that it is not enough to persist
the data to
> disk.

Done.
My concern was, I didn't want to store "now" unless I make sure that
request is successful. Now that we prevent non-kiosk apps from calling
this API from the beginning (and this is the only way the request can
fail),then it's ok to assume that the request will always be successful.

I added a NOTREACHED() if our assumption is broken.

https://codereview.chromium.org/1970613003/diff/100001/extensions/browser/api/runtime/runtime_api.cc#newcode691
extensions/browser/api/runtime/runtime_api.cc:691: return
RespondLater();

On 2016/05/16 18:25:17, xiyuan wrote:
> RestartDeviceOnWatchdogTimeout() might call Response() synchronously
(e.g. on
> error). And we should not call this for such cases.

Done!
Thanks for catching that. I made RestartDeviceOnWatchdogTimeout() return
a status enum |RestartOnWatchdogStatus|, based on which we either
RespondNow() or RespondLater().

https://codereview.chromium.org/1970613003/

[xiy...@chromium.org]

lgtm




https://codereview.chromium.org/1970613003/diff/120001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/120001/extensions/browser/api/runtime/runtime_api.cc#newcode84
extensions/browser/api/runtime/runtime_api.cc:84: const int
kMinDurationBetweenSuccessiveRestartsHours = 3;
nit: const -> constexpr and maybe update the other "const" as well.

Learned from my other review, constexpr is preferred style.

https://codereview.chromium.org/1970613003/diff/120001/extensions/browser/api/runtime/runtime_api.cc#newcode88
extensions/browser/api/runtime/runtime_api.cc:88: bool
g_allow_non_kiost_apps_restart_api = false;
nit: remove "g_" prefix since it is not really a global and add a
_"for_test" suffix.

g_allow_non_kiost_apps_restart_api ->
allow_non_kiost_apps_restart_api_for_test

https://codereview.chromium.org/1970613003/diff/120001/extensions/browser/api/runtime/runtime_api.cc#newcode433
extensions/browser/api/runtime/runtime_api.cc:433: // by non kiost apps,
and we prevent that from the beginning (unless in
kiost -> kiosk

https://codereview.chromium.org/1970613003/diff/120001/extensions/browser/api/runtime/runtime_api.cc#newcode714
extensions/browser/api/runtime/runtime_api.cc:714: return
RespondNow(Error("Not the first extension to call this API."));
nit: Use a kErrorXXX for error string literals, here and other places.

https://codereview.chromium.org/1970613003/diff/120001/extensions/browser/api/runtime/runtime_api.cc#newcode720
extensions/browser/api/runtime/runtime_api.cc:720: ;
nit: remove?

https://codereview.chromium.org/1970613003/diff/120001/extensions/browser/api/runtime/runtime_api.cc#newcode727
extensions/browser/api/runtime/runtime_api.cc:727: return
RespondLater();
nit: RespondNow with an error.

https://codereview.chromium.org/1970613003/

[afa...@chromium.org]

Thanks Xiyuan!

Devlin, could you please take a look?

https://codereview.chromium.org/1970613003/diff/120001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/120001/extensions/browser/api/runtime/runtime_api.cc#newcode84
extensions/browser/api/runtime/runtime_api.cc:84: const int
kMinDurationBetweenSuccessiveRestartsHours = 3;

On 2016/05/17 21:23:55, xiyuan wrote:
> nit: const -> constexpr and maybe update the other "const" as well.
>
> Learned from my other review, constexpr is preferred style.

Done.

https://codereview.chromium.org/1970613003/diff/120001/extensions/browser/api/runtime/runtime_api.cc#newcode88
extensions/browser/api/runtime/runtime_api.cc:88: bool
g_allow_non_kiost_apps_restart_api = false;

On 2016/05/17 21:23:55, xiyuan wrote:
> nit: remove "g_" prefix since it is not really a global and add a
_"for_test"
> suffix.
>
> g_allow_non_kiost_apps_restart_api ->
> allow_non_kiost_apps_restart_api_for_test
>

Done.

https://codereview.chromium.org/1970613003/diff/120001/extensions/browser/api/runtime/runtime_api.cc#newcode433
extensions/browser/api/runtime/runtime_api.cc:433: // by non kiost apps,
and we prevent that from the beginning (unless in

On 2016/05/17 21:23:55, xiyuan wrote:
> kiost -> kiosk

Done.

https://codereview.chromium.org/1970613003/diff/120001/extensions/browser/api/runtime/runtime_api.cc#newcode714
extensions/browser/api/runtime/runtime_api.cc:714: return
RespondNow(Error("Not the first extension to call this API."));

On 2016/05/17 21:23:55, xiyuan wrote:
> nit: Use a kErrorXXX for error string literals, here and other places.

Done.

https://codereview.chromium.org/1970613003/diff/120001/extensions/browser/api/runtime/runtime_api.cc#newcode720
extensions/browser/api/runtime/runtime_api.cc:720: ;
On 2016/05/17 21:23:55, xiyuan wrote:
> nit: remove?

Done. Not sure where that came from!

https://codereview.chromium.org/1970613003/diff/120001/extensions/browser/api/runtime/runtime_api.cc#newcode727
extensions/browser/api/runtime/runtime_api.cc:727: return
RespondLater();

On 2016/05/17 21:23:55, xiyuan wrote:
> nit: RespondNow with an error.

Done.

https://codereview.chromium.org/1970613003/

[afa...@chromium.org]

Devlin, friendly ping!

https://codereview.chromium.org/1970613003/

[rdevlin...@chromium.org]

Sorry for the delay; still making my way through this one. It's on my radar.

https://codereview.chromium.org/1970613003/

[rdevlin...@chromium.org]

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc#newcode49
extensions/browser/api/runtime/runtime_api.cc:49: constexpr char
kNoBackgroundPageError[] = "You do not have a background page.";
Is constexpr preferred by Chromium style now?

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc#newcode355
extensions/browser/api/runtime/runtime_api.cc:355: // To achieve as much
accuracy as possible, record the time of the call as
If we have a min time of 3 hours, does "as much accuracy as possible"
really matter?

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc#newcode375
extensions/browser/api/runtime/runtime_api.cc:375:
storage->GetExtensionValue(
The fact that we need to store this value implies to me that this is
supposed to carry across device power ons/offs - is that correct? If
so, don't we need to read it at startup somewhere? Also, should we make
sure that we don't restart right after turning on? If it's not supposed
to persist, could we just have this as a property of the runtime api?

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc#newcode740
extensions/browser/api/runtime/runtime_api.cc:740: void
RuntimeRestartOnWatchdogFunction::OnWatchdogTimeout(
This strikes me as a little scary, since we'll almost always leak this.
Is there a reason we can't just let the extension know if the restart
was properly scheduled, rather than executing a callback later?

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.h
File extensions/browser/api/runtime/runtime_api.h (right):

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.h#newcode118
extensions/browser/api/runtime/runtime_api.h:118: void
OnRestartWatchdogTimeout(const std::string& extension_id);
Function docs

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.h#newcode169
extensions/browser/api/runtime/runtime_api.h:169:
OnWatchdogTimeoutCallback current_watchdog_request_callback_;
Are we guaranteed to only have one of these?

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.h#newcode284
extensions/browser/api/runtime/runtime_api.h:284:
DECLARE_EXTENSION_FUNCTION("runtime.restartOnWatchdog",
I still think this is weird naming. Maybe restartAfterDelay? Or perhaps
better, just scheduleRestart?

https://codereview.chromium.org/1970613003/diff/140001/extensions/common/api/runtime.json
File extensions/common/api/runtime.json (right):

https://codereview.chromium.org/1970613003/diff/140001/extensions/common/api/runtime.json#newcode283
extensions/common/api/runtime.json:283: "name": "success",
For consistency, success should be indicated by whether or not lastError
is set, not by a separate return result.

https://codereview.chromium.org/1970613003/

[afa...@chromium.org]

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc#newcode49
extensions/browser/api/runtime/runtime_api.cc:49: constexpr char
kNoBackgroundPageError[] = "You do not have a background page.";

On 2016/05/20 17:26:32, Devlin wrote:
> Is constexpr preferred by Chromium style now?

Yes. Please, see: http://chromium-cpp.appspot.com/#core-whitelist

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc#newcode355
extensions/browser/api/runtime/runtime_api.cc:355: // To achieve as much
accuracy as possible, record the time of the call as

On 2016/05/20 17:26:32, Devlin wrote:
> If we have a min time of 3 hours, does "as much accuracy as possible"
really
> matter?

The minimum time is for any two successive restarts resulting from this
API; i.e. when an app schedules a restart after 2 seconds from now, and
succeeds, the following restart that this app can schedule after restart
is 3 hours later.
Nothing prevents the app from calling this API once every 1 second for
example.

The accuracy here really matters in our browsertests, since we're
talking about a couple of seconds. :D

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc#newcode375
extensions/browser/api/runtime/runtime_api.cc:375:
storage->GetExtensionValue(

On 2016/05/20 17:26:32, Devlin wrote:
> The fact that we need to store this value implies to me that this is
supposed to
> carry across device power ons/offs - is that correct? If so, don't we
need to
> read it at startup somewhere? Also, should we make sure that we don't
restart
> right after turning on? If it's not supposed to persist, could we
just have
> this as a property of the runtime api?

Yes, it is supposed to persist accross device power ons/off.

I don't think restarting after first turning on matters. From what I
understand about the purpose of this API is that it's made for kiosk
apps so that they can call it repeatedly as a way to tell Chrome OS that
the app is functioning and behaving properly.

Let's say we have a device that was just powered on. We have on it a
kiosk app that starts running. The app developer decided to call this
API every 10 seconds (for instance). It calls it the first time. few
seconds later, it hangs forever. It will miss the next API call. Our API
will recognize this as an app failure and will restart the device,
hopefully so that the app can be updated automatically. After restart
however, this app will not be able to schedule any restarts sooner than
3 hours, even if it hangs again after say 15 seconds from the beginning.

Little bit of history: I was told that we're adding this API to avoid a
problem that took place with a kiosk app that caused the whole system to
enter a bad state to point, someone had to be sent to every single
device running that app to restart it manually. (I'm sorry, I don't have
the exact details).

We can raise this issue on the bug thread as well, and ask for
clarification regarding the expected behavior.

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc#newcode740
extensions/browser/api/runtime/runtime_api.cc:740: void
RuntimeRestartOnWatchdogFunction::OnWatchdogTimeout(

On 2016/05/20 17:26:32, Devlin wrote:
> This strikes me as a little scary, since we'll almost always leak
this. Is there
> a reason we can't just let the extension know if the restart was
properly
> scheduled, rather than executing a callback later?

Could you please clarify how we leak it? I tried to make sure, we don't
schedule any new restart until we're completely done with the previous
one. Also we don't schedule anyone (and hence call RespondLater()) until
we're quite sure no errors or invalid arguments or state where
encountered.

This also helps a lot with the browsertests.

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.h
File extensions/browser/api/runtime/runtime_api.h (right):

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.h#newcode118
extensions/browser/api/runtime/runtime_api.h:118: void
OnRestartWatchdogTimeout(const std::string& extension_id);

On 2016/05/20 17:26:32, Devlin wrote:
> Function docs

Done.

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.h#newcode169
extensions/browser/api/runtime/runtime_api.h:169:
OnWatchdogTimeoutCallback current_watchdog_request_callback_;

On 2016/05/20 17:26:33, Devlin wrote:
> Are we guaranteed to only have one of these?

This API is only allowed to be called by only one extension (the first
to invoke it) and it must be running in kiosk mode. Any subsequent calls
to this API will cancel the previous one. So yes, at any point in time,
we have only a single "current" restart request.

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.h#newcode284
extensions/browser/api/runtime/runtime_api.h:284:
DECLARE_EXTENSION_FUNCTION("runtime.restartOnWatchdog",

On 2016/05/20 17:26:32, Devlin wrote:
> I still think this is weird naming. Maybe restartAfterDelay? Or
perhaps better,
> just scheduleRestart?

I agree, the name is weird. I guess the intention was to suggest a
watchdog behavior, that user must call this API repeatedly.

We can raise the question of this API's name on the bug and raise
consensus regarding the best name. I'm fine with any.

https://codereview.chromium.org/1970613003/diff/140001/extensions/common/api/runtime.json
File extensions/common/api/runtime.json (right):

https://codereview.chromium.org/1970613003/diff/140001/extensions/common/api/runtime.json#newcode283
extensions/common/api/runtime.json:283: "name": "success",

On 2016/05/20 17:26:33, Devlin wrote:
> For consistency, success should be indicated by whether or not
lastError is set,
> not by a separate return result.

I'm sorry, I don't get this comment. Not sure which lastError or which
separate return result?

https://codereview.chromium.org/1970613003/

[rdevlin...@chromium.org]

(just responding, no new comments)

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc#newcode49
extensions/browser/api/runtime/runtime_api.cc:49: constexpr char
kNoBackgroundPageError[] = "You do not have a background page.";

On 2016/05/21 01:13:00, afakhry wrote:
> On 2016/05/20 17:26:32, Devlin wrote:
> > Is constexpr preferred by Chromium style now?
>
> Yes. Please, see: http://chromium-cpp.appspot.com/#core-whitelist

From there "Don't go out of the way to convert existing code." So I'd
say make the new ones constexpr, but leave these.

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc#newcode375
extensions/browser/api/runtime/runtime_api.cc:375:
storage->GetExtensionValue(

I think I understand the point of it, my question was why we're storing
it. From my understood use cases (which align with what you described),
there's no reason to store it persistently rather than just as a
property on the API. Am I missing something?

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc#newcode740
extensions/browser/api/runtime/runtime_api.cc:740: void
RuntimeRestartOnWatchdogFunction::OnWatchdogTimeout(

On 2016/05/21 01:13:00, afakhry wrote:
> On 2016/05/20 17:26:32, Devlin wrote:
> > This strikes me as a little scary, since we'll almost always leak
this. Is
> there
> > a reason we can't just let the extension know if the restart was
properly
> > scheduled, rather than executing a callback later?
>
> Could you please clarify how we leak it? I tried to make sure, we
don't schedule
> any new restart until we're completely done with the previous one.
Also we don't
> schedule anyone (and hence call RespondLater()) until we're quite sure
no errors
> or invalid arguments or state where encountered.
>
> This also helps a lot with the browsertests.

For leaking, I was expecting the restart function to be synchronous. So
either we restarted (in which case this never returns, and leaks) or
didn't restart (in which case we wait indefinitely). I suppose
depending on how often we replace the restart request, it may not be all
the time. Even so, this pattern seems really weird. I'd much rather
invoke the callback immediately and just have a state of success vs
failure in scheduling the restart.

https://codereview.chromium.org/1970613003/diff/140001/extensions/common/api/runtime.json
File extensions/common/api/runtime.json (right):

https://codereview.chromium.org/1970613003/diff/140001/extensions/common/api/runtime.json#newcode283
extensions/common/api/runtime.json:283: "name": "success",

On 2016/05/21 01:13:00, afakhry wrote:
> On 2016/05/20 17:26:33, Devlin wrote:
> > For consistency, success should be indicated by whether or not
lastError is
> set,
> > not by a separate return result.
>
> I'm sorry, I don't get this comment. Not sure which lastError or which
separate
> return result?

chrome.runtime.lastError is used to indicate whether an api call
succeeded or failed, and is the canonical way of indicating
success/failure in an API method. Here, you're returning a separate
parameter ('success') to indicate it. We shouldn't deviate from
established patterns.

https://codereview.chromium.org/1970613003/

[xiy...@chromium.org]

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc#newcode375
extensions/browser/api/runtime/runtime_api.cc:375:
storage->GetExtensionValue(

The minimum time 3 hours between API reboots means if last reboot time
is T, the next reboot time must be >= T + 3 hours. I think we would have
to persist T since the device would be rebooted and lose all in memory
states between the two runs. Right?

https://codereview.chromium.org/1970613003/

[rdevlin...@chromium.org]

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc#newcode375
extensions/browser/api/runtime/runtime_api.cc:375:
storage->GetExtensionValue(

Right, but I think we should store that when/if we restart the device,
not when we schedule the restart, since the latter could be common,
while the former should be quite rare.

https://codereview.chromium.org/1970613003/

[xiy...@chromium.org]

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc#newcode375
extensions/browser/api/runtime/runtime_api.cc:375:
storage->GetExtensionValue(

The code here is to read the persist value to calculate the time for
scheduling the reboot. The SetExtensionValue call is around line 455
just before we reboot the device.

https://codereview.chromium.org/1970613003/

[rdevlin...@chromium.org]

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc#newcode375
extensions/browser/api/runtime/runtime_api.cc:375:
storage->GetExtensionValue(

Ah, whoops! Misread this. You're right. Though I think we should
still store this time as a member on the API to avoid re-reading it each
time we schedule a restart.

https://codereview.chromium.org/1970613003/

[afa...@chromium.org]

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc#newcode49
extensions/browser/api/runtime/runtime_api.cc:49: constexpr char
kNoBackgroundPageError[] = "You do not have a background page.";

On 2016/05/23 16:49:39, Devlin wrote:
> On 2016/05/21 01:13:00, afakhry wrote:
> > On 2016/05/20 17:26:32, Devlin wrote:

> > > Is constexpr preferred by Chromium style now?
> >
> > Yes. Please, see: http://chromium-cpp.appspot.com/#core-whitelist
>
> From there "Don't go out of the way to convert existing code." So I'd
say make
> the new ones constexpr, but leave these.

Done.

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc#newcode375
extensions/browser/api/runtime/runtime_api.cc:375:
storage->GetExtensionValue(

We now get it from the pref service.

https://codereview.chromium.org/1970613003/diff/140001/extensions/browser/api/runtime/runtime_api.cc#newcode740
extensions/browser/api/runtime/runtime_api.cc:740: void
RuntimeRestartOnWatchdogFunction::OnWatchdogTimeout(

On 2016/05/23 16:49:39, Devlin wrote:
> On 2016/05/21 01:13:00, afakhry wrote:
> > On 2016/05/20 17:26:32, Devlin wrote:

Done. No more RespondLater(), and the response callback is invoked when
the restart is successfully scheduled. Tests have been modified
accordingly.

https://codereview.chromium.org/1970613003/diff/140001/extensions/common/api/runtime.json
File extensions/common/api/runtime.json (right):

https://codereview.chromium.org/1970613003/diff/140001/extensions/common/api/runtime.json#newcode283
extensions/common/api/runtime.json:283: "name": "success",

On 2016/05/23 16:49:39, Devlin wrote:
> On 2016/05/21 01:13:00, afakhry wrote:

> > On 2016/05/20 17:26:33, Devlin wrote:
> > > For consistency, success should be indicated by whether or not
lastError is
> > set,
> > > not by a separate return result.
> >
> > I'm sorry, I don't get this comment. Not sure which lastError or
which
> separate
> > return result?
>
> chrome.runtime.lastError is used to indicate whether an api call
succeeded or
> failed, and is the canonical way of indicating success/failure in an
API method.
> Here, you're returning a separate parameter ('success') to indicate
it. We
> shouldn't deviate from established patterns.

Done.

https://codereview.chromium.org/1970613003/

[rdevlin...@chromium.org]

Getting closer! I'd still like to see the api renamed, given there's been no
objections. :)


https://codereview.chromium.org/1970613003/diff/180001/chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js
File
chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js
(right):

https://codereview.chromium.org/1970613003/diff/180001/chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js#newcode19
chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js:19:
assertTrue(chrome.runtime.lastError === undefined);
nit: prefer "assertNoLastError()"

https://codereview.chromium.org/1970613003/diff/180001/chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js#newcode24
chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js:24:
// This request will be successfully rescheduled even though it will
rescheduled? When was it first scheduled?

https://codereview.chromium.org/1970613003/diff/180001/chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js#newcode27
chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js:27:
sendMessage('request1', function(response) {
nit: might be a bit easier to read these if the numbers matched the test
number - maybe something like "responseN"?

https://codereview.chromium.org/1970613003/diff/180001/chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js#newcode57
chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js:57:
// scheduled to happen after 3 seconds, even though we requested 1
second.
API question - should we automatically reschedule for the minimum time,
or just throw an error saying "too soon after last restart"?

https://codereview.chromium.org/1970613003/diff/180001/chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js#newcode64
chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js:64:
} else if (response == 'success') {
needed?

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_api.cc#newcode368
extensions/browser/api/runtime/runtime_api.cc:368:
MaybeCancelRunningWatchdogTimer();
Given we call this in both cases (here and on line 416), might just make
sense to move it up to 366 and then return here with a comment "// We
already stopped the running timer."

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_api.cc#newcode390
extensions/browser/api/runtime/runtime_api.cc:390: if
(!watchdog_timer_.IsRunning())
nit: prefer
if (watchdog_timer_.IsRunning())
watchdog_timer_.Stop();

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_api.h
File extensions/browser/api/runtime/runtime_api.h (right):

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_api.h#newcode141
extensions/browser/api/runtime/runtime_api.h:141: void
SetMinDurationBetweenRestartsForTesting(base::TimeDelta delta) {
const base::TimeDelta&

Also, this can probably be hacker_style() since it's just a setter.

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_apitest.cc
File extensions/browser/api/runtime/runtime_apitest.cc (right):

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_apitest.cc#newcode177
extensions/browser/api/runtime/runtime_apitest.cc:177:
quit_closure_.Run();
base::ResetAndReturn(&quit_closure_);

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_apitest.cc#newcode216
extensions/browser/api/runtime/runtime_apitest.cc:216:
ExtensionApiTest::SetUpOnMainThread();
nit: call this first

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_apitest.cc#newcode241
extensions/browser/api/runtime/runtime_apitest.cc:241:
IN_PROC_BROWSER_TEST_F(RestartOnWatchdogApiTest, RestartOnWatchdogTest)
{
Now that you've simplified some of the tests a bit (yay!), we can
actually do all these in a unittest. See extension_function_test_utils
for how to run a function without a JS counterpart. Then we get the
advantage of unittest speed + not needing to go back and forth between
the two test files to understand what's going on. (Sorry for not
mentioning this earlier - it didn't occur to me that we really didn't
need the JS part here for what we're testing.)

https://codereview.chromium.org/1970613003/

[afa...@chromium.org]

Renamed to restartAfterDelay(). PTAL.

https://codereview.chromium.org/1970613003/diff/180001/chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js
File
chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js
(right):

https://codereview.chromium.org/1970613003/diff/180001/chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js#newcode19
chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js:19:
assertTrue(chrome.runtime.lastError === undefined);

On 2016/05/25 21:53:21, Devlin wrote:
> nit: prefer "assertNoLastError()"

Done.

https://codereview.chromium.org/1970613003/diff/180001/chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js#newcode24
chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js:24:
// This request will be successfully rescheduled even though it will

On 2016/05/25 21:53:21, Devlin wrote:
> rescheduled? When was it first scheduled?

Done.

https://codereview.chromium.org/1970613003/diff/180001/chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js#newcode27
chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js:27:
sendMessage('request1', function(response) {

On 2016/05/25 21:53:21, Devlin wrote:
> nit: might be a bit easier to read these if the numbers matched the
test number
> - maybe something like "responseN"?

Done.

https://codereview.chromium.org/1970613003/diff/180001/chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js#newcode57
chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js:57:
// scheduled to happen after 3 seconds, even though we requested 1
second.

On 2016/05/25 21:53:21, Devlin wrote:
> API question - should we automatically reschedule for the minimum
time, or just
> throw an error saying "too soon after last restart"?

Me and Xiyuan agreed to schedule a restart after the minimum time has
passed.

https://codereview.chromium.org/1970613003/diff/180001/chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js#newcode64
chrome/test/data/extensions/api_test/runtime/restartOnWatchdog/test.js:64:
} else if (response == 'success') {

On 2016/05/25 21:53:21, Devlin wrote:
> needed?

No, I was just being explicit. Removed.

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_api.cc#newcode368
extensions/browser/api/runtime/runtime_api.cc:368:
MaybeCancelRunningWatchdogTimer();

On 2016/05/25 21:53:21, Devlin wrote:
> Given we call this in both cases (here and on line 416), might just
make sense
> to move it up to 366 and then return here with a comment "// We
already stopped
> the running timer."

Done.

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_api.cc#newcode390
extensions/browser/api/runtime/runtime_api.cc:390: if
(!watchdog_timer_.IsRunning())

On 2016/05/25 21:53:22, Devlin wrote:
> nit: prefer
> if (watchdog_timer_.IsRunning())
> watchdog_timer_.Stop();

Done.

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_api.h
File extensions/browser/api/runtime/runtime_api.h (right):

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_api.h#newcode141
extensions/browser/api/runtime/runtime_api.h:141: void
SetMinDurationBetweenRestartsForTesting(base::TimeDelta delta) {

On 2016/05/25 21:53:22, Devlin wrote:
> const base::TimeDelta&
>
> Also, this can probably be hacker_style() since it's just a setter.

TimeDelta is just an int64_t and is always passed by value:
https://code.google.com/p/chromium/codesearch#search/&q=.*%5C(base::TimeDelta.*&sq=package:chromium&type=cs

Done for the hacker_style() setter.

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_apitest.cc
File extensions/browser/api/runtime/runtime_apitest.cc (right):

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_apitest.cc#newcode177
extensions/browser/api/runtime/runtime_apitest.cc:177:
quit_closure_.Run();

On 2016/05/25 21:53:22, Devlin wrote:
> base::ResetAndReturn(&quit_closure_);

Done.

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_apitest.cc#newcode216
extensions/browser/api/runtime/runtime_apitest.cc:216:
ExtensionApiTest::SetUpOnMainThread();

On 2016/05/25 21:53:22, Devlin wrote:
> nit: call this first

Done.

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_apitest.cc#newcode241
extensions/browser/api/runtime/runtime_apitest.cc:241:
IN_PROC_BROWSER_TEST_F(RestartOnWatchdogApiTest, RestartOnWatchdogTest)
{

On 2016/05/25 21:53:22, Devlin wrote:
> Now that you've simplified some of the tests a bit (yay!), we can
actually do
> all these in a unittest. See extension_function_test_utils for how to
run a
> function without a JS counterpart. Then we get the advantage of
unittest speed
> + not needing to go back and forth between the two test files to
understand
> what's going on. (Sorry for not mentioning this earlier - it didn't
occur to me
> that we really didn't need the JS part here for what we're testing.)

For this tricky and sensitive API, I think we should keep these browser
tests for the following reasons:
- It makes sense to have this test along side the other runtime api
tests here.
- Isn't a browsertest is the closest test environment to reality?
- I also found it very convenient modifying the separate JS part and
re-run the test immediately without having to recompile browsertests
agin.
- Having the JS part will be helpful to the test team, giving them an
example of how the API is used so that they can add their own tests for
the Test Review bug here:
https://bugs.chromium.org/p/chromium/issues/detail?id=607413

https://codereview.chromium.org/1970613003/

[rdevlin...@chromium.org]

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_apitest.cc
File extensions/browser/api/runtime/runtime_apitest.cc (right):

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_apitest.cc#newcode241
extensions/browser/api/runtime/runtime_apitest.cc:241:
IN_PROC_BROWSER_TEST_F(RestartOnWatchdogApiTest, RestartOnWatchdogTest)
{

Browser tests are orders of magnitude slower and flakier than unit
tests. Additionally, all of the "tricky and sensitive" aspects of this
API are confined here, in the browser process, which can be in a normal
state in a unittest. The renderer component is just a vanilla extension
API with no added logic (as is seen here by the lack of renderer/
modified files), and if our extension API system breaks in the renderer,
we'll have hundreds of other browser tests fail. Unit tests should
almost always be preferred to browser tests, and I don't think this is
an exception.

https://codereview.chromium.org/1970613003/

[afa...@chromium.org]

Please take a look. Converted the browser tests to unit tests as requested.

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_apitest.cc
File extensions/browser/api/runtime/runtime_apitest.cc (right):

https://codereview.chromium.org/1970613003/diff/180001/extensions/browser/api/runtime/runtime_apitest.cc#newcode241
extensions/browser/api/runtime/runtime_apitest.cc:241:
IN_PROC_BROWSER_TEST_F(RestartOnWatchdogApiTest, RestartOnWatchdogTest)
{

Done.

https://codereview.chromium.org/1970613003/

[rdevlin...@chromium.org]

Looks good! :) A few more small things.


https://codereview.chromium.org/1970613003/diff/220001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc
File extensions/browser/api/runtime/restart_after_delay_api_unittest.cc
(right):

https://codereview.chromium.org/1970613003/diff/220001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc#newcode67
extensions/browser/api/runtime/restart_after_delay_api_unittest.cc:67:
base::RunLoop run_loop;
Isn't this racy if the restart happens quickly enough?

https://codereview.chromium.org/1970613003/diff/220001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc#newcode161
extensions/browser/api/runtime/restart_after_delay_api_unittest.cc:161:
base::TimeTicks last_restart_time = WaitForSuccessfulRestart();
Spinning for 3 seconds is really innefficient. What if we reset the min
delay to 0 and schedule it for 0 seconds, then reset it back to 3 before
the next call? Or, you could use a test clock, if you'd prefer that
approach.

https://codereview.chromium.org/1970613003/diff/220001/extensions/browser/api/runtime/runtime_apitest.cc
File extensions/browser/api/runtime/runtime_apitest.cc (right):

https://codereview.chromium.org/1970613003/diff/220001/extensions/browser/api/runtime/runtime_apitest.cc#newcode5
extensions/browser/api/runtime/runtime_apitest.cc:5: #include
"base/callback_helpers.h"
Do we need these still?

https://codereview.chromium.org/1970613003/diff/220001/extensions/browser/test_extensions_browser_client.h
File extensions/browser/test_extensions_browser_client.h (right):

https://codereview.chromium.org/1970613003/diff/220001/extensions/browser/test_extensions_browser_client.h#newcode136
extensions/browser/test_extensions_browser_client.h:136:
user_prefs::TestingPrefServiceSyncable testing_pref_service_;
Can we lazily initialize this?

https://codereview.chromium.org/1970613003/diff/220001/extensions/common/api/runtime.json
File extensions/common/api/runtime.json (right):

https://codereview.chromium.org/1970613003/diff/220001/extensions/common/api/runtime.json#newcode278
extensions/common/api/runtime.json:278: "name":
"onRestartScheduledCallback",
probably just call this "callback"

https://codereview.chromium.org/1970613003/diff/220001/extensions/common/api/runtime.json#newcode279
extensions/common/api/runtime.json:279: "description": "A callback to be
invoked when a restart request was successfully rescheduled or when an
error occurs in which case runtime.lastError can be checked.",
the second part of this ("or when an error occurs...") is probably
unnecessary, since that's true of every API.

https://codereview.chromium.org/1970613003/diff/220001/extensions/common/api/runtime.json#newcode281
extensions/common/api/runtime.json:281: "parameters": [
Do we need this? If so, put the opening and closing braces on the same
line ("[]"); if not, remove.

https://codereview.chromium.org/1970613003/

[afa...@chromium.org]

https://codereview.chromium.org/1970613003/diff/220001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc
File extensions/browser/api/runtime/restart_after_delay_api_unittest.cc
(right):

https://codereview.chromium.org/1970613003/diff/220001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc#newcode67
extensions/browser/api/runtime/restart_after_delay_api_unittest.cc:67:
base::RunLoop run_loop;

On 2016/06/01 21:29:15, Devlin wrote:
> Isn't this racy if the restart happens quickly enough?

Added a bool to detect if the restart was done already before waiting
for it. Thanks.

https://codereview.chromium.org/1970613003/diff/220001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc#newcode161
extensions/browser/api/runtime/restart_after_delay_api_unittest.cc:161:
base::TimeTicks last_restart_time = WaitForSuccessfulRestart();

On 2016/06/01 21:29:15, Devlin wrote:
> Spinning for 3 seconds is really innefficient. What if we reset the
min delay
> to 0 and schedule it for 0 seconds, then reset it back to 3 before the
next
> call? Or, you could use a test clock, if you'd prefer that approach.

Right. Since we reset (using -1) in the previous request, We can now
request with 1 second. Which will be accomplished a second later. I also
lowered the minimum time between two successive restarts to 2 seconds
instead of 3, so the next restart request (with 1 second) will be
throttled to 2 seconds instead.

So now the whole test takes 3 seconds instead of 7.

https://codereview.chromium.org/1970613003/diff/220001/extensions/browser/api/runtime/runtime_apitest.cc
File extensions/browser/api/runtime/runtime_apitest.cc (right):

https://codereview.chromium.org/1970613003/diff/220001/extensions/browser/api/runtime/runtime_apitest.cc#newcode5
extensions/browser/api/runtime/runtime_apitest.cc:5: #include
"base/callback_helpers.h"

On 2016/06/01 21:29:15, Devlin wrote:
> Do we need these still?

No. Removed.

https://codereview.chromium.org/1970613003/diff/220001/extensions/browser/api/runtime/runtime_apitest.cc#newcode15
extensions/browser/api/runtime/runtime_apitest.cc:15: #include
"extensions/test/extension_test_message_listener.h"
Removed too.

https://codereview.chromium.org/1970613003/diff/220001/extensions/browser/test_extensions_browser_client.h
File extensions/browser/test_extensions_browser_client.h (right):

https://codereview.chromium.org/1970613003/diff/220001/extensions/browser/test_extensions_browser_client.h#newcode136
extensions/browser/test_extensions_browser_client.h:136:
user_prefs::TestingPrefServiceSyncable testing_pref_service_;

On 2016/06/01 21:29:15, Devlin wrote:
> Can we lazily initialize this?

What's the point? It will be needed in the constructor to set it for the
|main_context_|, so lazily ends up being right away. Unless you mean
something else?

https://codereview.chromium.org/1970613003/diff/220001/extensions/common/api/runtime.json
File extensions/common/api/runtime.json (right):

https://codereview.chromium.org/1970613003/diff/220001/extensions/common/api/runtime.json#newcode278
extensions/common/api/runtime.json:278: "name":
"onRestartScheduledCallback",

On 2016/06/01 21:29:15, Devlin wrote:
> probably just call this "callback"

Done.

https://codereview.chromium.org/1970613003/diff/220001/extensions/common/api/runtime.json#newcode279
extensions/common/api/runtime.json:279: "description": "A callback to be
invoked when a restart request was successfully rescheduled or when an
error occurs in which case runtime.lastError can be checked.",

On 2016/06/01 21:29:15, Devlin wrote:
> the second part of this ("or when an error occurs...") is probably
unnecessary,
> since that's true of every API.

Done.

https://codereview.chromium.org/1970613003/diff/220001/extensions/common/api/runtime.json#newcode281
extensions/common/api/runtime.json:281: "parameters": [

On 2016/06/01 21:29:15, Devlin wrote:
> Do we need this? If so, put the opening and closing braces on the
same line
> ("[]"); if not, remove.

Done.

https://codereview.chromium.org/1970613003/

[afa...@chromium.org]

Friendly ping.

https://codereview.chromium.org/1970613003/

[rdevlin...@chromium.org]

lgtm, thank you for bearing with me. :)

https://codereview.chromium.org/1970613003/diff/220001/extensions/browser/test_extensions_browser_client.h
File extensions/browser/test_extensions_browser_client.h (right):

https://codereview.chromium.org/1970613003/diff/220001/extensions/browser/test_extensions_browser_client.h#newcode136
extensions/browser/test_extensions_browser_client.h:136:
user_prefs::TestingPrefServiceSyncable testing_pref_service_;

On 2016/06/02 01:43:40, afakhry wrote:
> On 2016/06/01 21:29:15, Devlin wrote:
> > Can we lazily initialize this?
>
> What's the point? It will be needed in the constructor to set it for
the
> |main_context_|, so lazily ends up being right away. Unless you mean
something
> else?

My thought was that we could move the UserPrefs::Set() call into the
initialization of the pref service as well. In practice, though, this
probably doesn't matter much. This should be fine.

https://codereview.chromium.org/1970613003/diff/240001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc
File extensions/browser/api/runtime/restart_after_delay_api_unittest.cc
(right):

https://codereview.chromium.org/1970613003/diff/240001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc#newcode19
extensions/browser/api/runtime/restart_after_delay_api_unittest.cc:19:
class RestartOnWatchdogApiDelegate : public RuntimeAPIDelegate {
Could we just inherit from TestRuntimeAPIDelegate and override the
methods we care about?

https://codereview.chromium.org/1970613003/diff/240001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc#newcode143
extensions/browser/api/runtime/restart_after_delay_api_unittest.cc:143:
ASSERT_FALSE(IsWatchdogTimerRunning());
nit: let's add a test for -2, and, optionally, a "not first extension".

https://codereview.chromium.org/1970613003/diff/240001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc#newcode179
extensions/browser/api/runtime/restart_after_delay_api_unittest.cc:179:
base::TimeTicks this_restart_time = WaitForSuccessfulRestart();
Do we need to wait for this restart, if we already check that a)
restarts work (we checked that with line 167) and b) the scheduled
restart time is correct (checked with line 177)? If not, omit this so
we shave off a couple of seconds.

https://codereview.chromium.org/1970613003/diff/240001/extensions/browser/api/runtime/runtime_api.h
File extensions/browser/api/runtime/runtime_api.h (right):

https://codereview.chromium.org/1970613003/diff/240001/extensions/browser/api/runtime/runtime_api.h#newcode63
extensions/browser/api/runtime/runtime_api.h:63:
SUCCESS_RESTART_CANCELLED,
cancelled vs canceled. Typically, we use american spelling (even though
it's wrong! ;)), but it looks like in this case, they are pretty dead
even split, so I have no preference. But I'd like to at least be
consistent between line 62 and 63 :)

https://codereview.chromium.org/1970613003/

[afa...@chromium.org]

Thank you for your detailed review!

p...@chromium.org: Please review changes in
chrome/browser/prefs/browser_prefs.cc

mpea...@chromium.org: Please review changes in
extensions/browser/extension_function_histogram_value.h
tools/metrics/histograms/histograms.xml

benw...@chromium.org: Please review changes in
extensions/browser/DEPS

https://codereview.chromium.org/1970613003/diff/240001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc
File extensions/browser/api/runtime/restart_after_delay_api_unittest.cc
(right):

https://codereview.chromium.org/1970613003/diff/240001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc#newcode19
extensions/browser/api/runtime/restart_after_delay_api_unittest.cc:19:
class RestartOnWatchdogApiDelegate : public RuntimeAPIDelegate {

On 2016/06/03 21:26:47, Devlin wrote:
> Could we just inherit from TestRuntimeAPIDelegate and override the
methods we
> care about?

Done. This was needed for the browser tests as I needed to keep using
the *real* delegate, which also had to b used to RemoveUpdateObserver()
at the end of the test.

We don't need this in the unit tests. :)

https://codereview.chromium.org/1970613003/diff/240001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc#newcode143
extensions/browser/api/runtime/restart_after_delay_api_unittest.cc:143:
ASSERT_FALSE(IsWatchdogTimerRunning());

On 2016/06/03 21:26:47, Devlin wrote:
> nit: let's add a test for -2, and, optionally, a "not first
extension".

Both done.

https://codereview.chromium.org/1970613003/diff/240001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc#newcode179
extensions/browser/api/runtime/restart_after_delay_api_unittest.cc:179:
base::TimeTicks this_restart_time = WaitForSuccessfulRestart();

On 2016/06/03 21:26:47, Devlin wrote:
> Do we need to wait for this restart, if we already check that a)
restarts work
> (we checked that with line 167) and b) the scheduled restart time is
correct
> (checked with line 177)? If not, omit this so we shave off a couple
of seconds.

Right. We don't have to wait for it. Done. Tests only takes 1 sec now.

https://codereview.chromium.org/1970613003/diff/240001/extensions/browser/api/runtime/runtime_api.h
File extensions/browser/api/runtime/runtime_api.h (right):

https://codereview.chromium.org/1970613003/diff/240001/extensions/browser/api/runtime/runtime_api.h#newcode63
extensions/browser/api/runtime/runtime_api.h:63:
SUCCESS_RESTART_CANCELLED,

On 2016/06/03 21:26:47, Devlin wrote:
> cancelled vs canceled. Typically, we use american spelling (even
though it's
> wrong! ;)), but it looks like in this case, they are pretty dead even
split, so
> I have no preference. But I'd like to at least be consistent between
line 62
> and 63 :)

Done.

https://codereview.chromium.org/1970613003/

[benw...@chromium.org]

lgtm


https://codereview.chromium.org/1970613003/

[mpea...@chromium.org]

histograms.xml lgtm


https://codereview.chromium.org/1970613003/

[afa...@chromium.org]

Devlin, I'm afraid I'll have to ask you for one extra review round after I had
to accommodate the changes requested by Mats Nilsson. Thanks! :)

https://codereview.chromium.org/1970613003/

[rdevlin...@chromium.org]

Also, please remove trace references to "watchdog", since it's no longer in the
API name, and will probably just confuse readers.


https://codereview.chromium.org/1970613003/diff/340001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/340001/extensions/browser/api/runtime/runtime_api.cc#newcode451
extensions/browser/api/runtime/runtime_api.cc:451: void
RuntimeAPI::OnDeviceShutdown() {
This worries me for a couple of reasons:
- We typically want to steer clear of NOTIFICATIONs.
- This means we're blocking shutdown on persisting something to disk,
which is something we really like to avoid.

I wonder if there's a way we can get around this. What if we did
something like:
In the delayed restart, if we succeed in scheduling it, assume it will
succeed, and persist both the last restart time and a bool that says
"restarted due to delayed restart".
In the ctor (or, maybe better yet, even lazily based on the first
restart on delay request) check the "restarted due to delayed" bit, and,
if it's present, we were restarted due to the API and we enforce the
throttle. Then clear the bit, and any future calls (including across
non-delayed restarts) will not have the bit, and we won't throttle.

WDYT?

https://codereview.chromium.org/1970613003/diff/340001/extensions/browser/api/runtime/runtime_api.h
File extensions/browser/api/runtime/runtime_api.h (left):

https://codereview.chromium.org/1970613003/diff/340001/extensions/browser/api/runtime/runtime_api.h#oldcode109
extensions/browser/api/runtime/runtime_api.h:109: // True if we should
dispatch the chrome.runtime.onInstalled event with
What's the reason for moving this?

https://codereview.chromium.org/1970613003/diff/340001/extensions/browser/api/runtime/runtime_api.h
File extensions/browser/api/runtime/runtime_api.h (right):

https://codereview.chromium.org/1970613003/diff/340001/extensions/browser/api/runtime/runtime_api.h#newcode180
extensions/browser/api/runtime/runtime_api.h:180: bool
dispatch_chrome_updated_event_ = false;
Let's not mix initialization styles. Since we initialize other members
(like browser_context_) in the ctor, we should do that with these, too.

https://codereview.chromium.org/1970613003/

[xiy...@chromium.org]

https://codereview.chromium.org/1970613003/diff/340001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/340001/extensions/browser/api/runtime/runtime_api.cc#newcode451
extensions/browser/api/runtime/runtime_api.cc:451: void
RuntimeAPI::OnDeviceShutdown() {

On 2016/06/10 00:34:45, Devlin wrote:
> This worries me for a couple of reasons:
> - We typically want to steer clear of NOTIFICATIONs.
> - This means we're blocking shutdown on persisting something to disk,
which is
> something we really like to avoid.
>
> I wonder if there's a way we can get around this. What if we did
something
> like:
> In the delayed restart, if we succeed in scheduling it, assume it will
succeed,
> and persist both the last restart time and a bool that says "restarted
due to
> delayed restart".
> In the ctor (or, maybe better yet, even lazily based on the first
restart on
> delay request) check the "restarted due to delayed" bit, and, if it's
present,
> we were restarted due to the API and we enforce the throttle. Then
clear the
> bit, and any future calls (including across non-delayed restarts) will
not have
> the bit, and we won't throttle.
>
> WDYT?

This would not work because the restartAfterDelay is supposed to be
called periodically by the app to implement a heart beat. Clearing the
flag on first call essentially makes the throttling ineffective.

I think the throttling is overly complicated to implement with the API.
We should probably take a step back. The purpose of the throttling is to
avoid/break reboot loop for bad apps. Instead of doing this in API, how
about we take another approach that we keep track of auto launch times
and stop launching the app if we do it too fast (say 5 times in last 15
minutes). This way, we break the potential reboot loop. And we do it on
device boot instead of during shutdown. The API would be much simpler
without the throttling logic.

https://codereview.chromium.org/1970613003/

[rdevlin...@chromium.org]

https://codereview.chromium.org/1970613003/diff/340001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/340001/extensions/browser/api/runtime/runtime_api.cc#newcode451
extensions/browser/api/runtime/runtime_api.cc:451: void
RuntimeAPI::OnDeviceShutdown() {

Sorry, I was unclear. I meant we should have a member
did_restart_due_to_delayed_restart_ on the RuntimeAPI, which is set if
the preference bit is set (and then clear the preference bit, but the
member stays set for the lifetime of the RuntimeAPI, which is also the
lifetime of the profile). I believe that would be sufficient, though,
as I said, setting in the RuntimeAPI ctor is also an option.

> I think the throttling is overly complicated to implement with the
API. We
> should probably take a step back. The purpose of the throttling is to
> avoid/break reboot loop for bad apps. Instead of doing this in API,
how about we
> take another approach that we keep track of auto launch times and stop
launching
> the app if we do it too fast (say 5 times in last 15 minutes). This
way, we
> break the potential reboot loop. And we do it on device boot instead
of during
> shutdown. The API would be much simpler without the throttling logic.

I'm fine with this, too, though I'm not as familiar with the code so
don't know how complex it will become. I just don't want to slow down
shutdown if we can avoid it. Anything that avoids that is fine with me.

https://codereview.chromium.org/1970613003/

[afa...@chromium.org]

https://codereview.chromium.org/1970613003/diff/340001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/340001/extensions/browser/api/runtime/runtime_api.cc#newcode451
extensions/browser/api/runtime/runtime_api.cc:451: void
RuntimeAPI::OnDeviceShutdown() {

On 2016/06/10 00:34:45, Devlin wrote:
> This worries me for a couple of reasons:
> - We typically want to steer clear of NOTIFICATIONs.
> - This means we're blocking shutdown on persisting something to disk,
which is
> something we really like to avoid.
>
> I wonder if there's a way we can get around this. What if we did
something
> like:
> In the delayed restart, if we succeed in scheduling it, assume it will
succeed,
> and persist both the last restart time and a bool that says "restarted
due to
> delayed restart".
> In the ctor (or, maybe better yet, even lazily based on the first
restart on
> delay request) check the "restarted due to delayed" bit, and, if it's
present,
> we were restarted due to the API and we enforce the throttle. Then
clear the
> bit, and any future calls (including across non-delayed restarts) will
not have
> the bit, and we won't throttle.
>
> WDYT?

I didn't get this comment until I read your explanation on IM. Thanks
for the clarification over there.
Done.

https://codereview.chromium.org/1970613003/diff/340001/extensions/browser/api/runtime/runtime_api.h
File extensions/browser/api/runtime/runtime_api.h (left):

https://codereview.chromium.org/1970613003/diff/340001/extensions/browser/api/runtime/runtime_api.h#oldcode109
extensions/browser/api/runtime/runtime_api.h:109: // True if we should
dispatch the chrome.runtime.onInstalled event with

On 2016/06/10 00:34:45, Devlin wrote:

> What's the reason for moving this?

I'm sure you know this, and probably would think it's unnecessary, but
the reason is alignment and padding. It's really bad to have a less
alignment restrictive type objects in the middle of more restrictive
ones.

Assuming Linux and 64-bit architecture:
bool is 1 byte ---> alignment requirement is 1 byte (so can be allocated
in any address).

The previous member |browser_context_| is a pointer. Pointers on 64-bit
machines are 8 bytes. ---> alignment requirements is 8 bytes.

The following member |registrar_| has only one member of type
std::vector<>. While the implementation of std::vector may vary, but
let's assume the one I have locally in /usr/include/. It has three
pinters:
pointer _M_start;
pointer _M_finish;
pointer _M_end_of_storage;

Again 8 byte aligned. Let's roughly sketch the memory layout:

+---+---+---+---+---+---+---+---+
| | | | | | | | |
|<----- browser_context_ ------>|
| | | | | | | | |
+---+---+---+---+---+---+---+---+
| | X | X | X | X | X | X | X |
| b | X | X | X | X | X | X | X |
| | X | X | X | X | X | X | X |
+---+---+---+---+---+---+---+---+
| | | | | | | | |
|<--------- _M_start ---------->|
| | | | | | | | |
+---+---+---+---+---+---+---+---+
| | | | | | | | |
|<--------- _M_finish --------->|
| | | | | | | | |
+---+---+---+---+---+---+---+---+
| | | | | | | | |
|<---- _M_end_of_storage ------>|
| | | | | | | | |
+---+---+---+---+---+---+---+---+

'b' is our bool |dispatch_chrome_updated_event_|, and the X's are
useless padding bytes.

So I moved it to the end and placed our two new bools after it, in order
to fill two of those padding bytes.

I could have just placed the new bools after
|dispatch_chrome_updated_event_| while it was still up here, but I have
this personal preference of having the bools at the end of the object.

I didn't want our new bools to introduce extra new padding bytes.

https://codereview.chromium.org/1970613003/diff/340001/extensions/browser/api/runtime/runtime_api.h
File extensions/browser/api/runtime/runtime_api.h (right):

https://codereview.chromium.org/1970613003/diff/340001/extensions/browser/api/runtime/runtime_api.h#newcode180
extensions/browser/api/runtime/runtime_api.h:180: bool
dispatch_chrome_updated_event_ = false;

On 2016/06/10 00:34:45, Devlin wrote:

> Let's not mix initialization styles. Since we initialize other
members (like
> browser_context_) in the ctor, we should do that with these, too.

Done.

https://codereview.chromium.org/1970613003/

[rdevlin...@chromium.org]

I see "done"s, but no new patch set yet?

https://codereview.chromium.org/1970613003/diff/340001/extensions/browser/api/runtime/runtime_api.h
File extensions/browser/api/runtime/runtime_api.h (left):

https://codereview.chromium.org/1970613003/diff/340001/extensions/browser/api/runtime/runtime_api.h#oldcode109
extensions/browser/api/runtime/runtime_api.h:109: // True if we should
dispatch the chrome.runtime.onInstalled event with

Realistically, since this is a singleton per profile, it's probably
never gonna matter, but I'm not going to stand in the way of minor
optimizations :) Thanks for the detailed answer!

https://codereview.chromium.org/1970613003/

[afa...@chromium.org]

Ooops sorry, I forgot to press enter!

https://codereview.chromium.org/1970613003/

[p...@chromium.org]

On 2016/06/13 16:16:00, afakhry wrote:
> Ooops sorry, I forgot to press enter!

chrome/browser/prefs/browser_prefs.cc LGTM

https://codereview.chromium.org/1970613003/

[rdevlin...@chromium.org]

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc
File extensions/browser/api/runtime/restart_after_delay_api_unittest.cc
(right):

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc#newcode23
extensions/browser/api/runtime/restart_after_delay_api_unittest.cc:23:
// Takes ownership of the |real_api_delegate|.
comment out of date

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc#newcode138
extensions/browser/api/runtime/restart_after_delay_api_unittest.cc:138:
std::string RunFunctionGetError(UIThreadExtensionFunction* function,
It looks like all occurrences of RunFunctionGetError() and
RunFunctionAssertNoError() just pass in a new
RuntimeRestartAfterDelayFunction(). Can we omit that argument and just
construct it in the respective functions?

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc#newcode140
extensions/browser/api/runtime/restart_after_delay_api_unittest.cc:140:
const std::string& args) {
can we also just pass in the expected error here, rather than returning
it?

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/runtime_api.cc#newcode184
extensions/browser/api/runtime/runtime_api.cc:184:
registry->RegisterBooleanPref(kPrefLastRestartWasDuetoDelayedRestartApi,
nit: DueTo, not Dueto

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/runtime_api.cc#newcode377
extensions/browser/api/runtime/runtime_api.cc:377: ;
delete

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/runtime_api.cc#newcode452
extensions/browser/api/runtime/runtime_api.cc:452: base::TimeDelta
future_time_since_last_restart =
nit: "future_time_since_last_restart" sounds a little weird to me.
Maybe "delta_since_last_restart"?

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/runtime_api.cc#newcode453
extensions/browser/api/runtime/runtime_api.cc:453: future_restart_time >
last_delayed_restart_time_
When would this be false (exempting cases of e.g. pref corruption)? If
this is false, won't it also cause problems on line 461?

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/runtime_api.cc#newcode492
extensions/browser/api/runtime/runtime_api.cc:492: if (!success &&
!allow_non_kiosk_apps_restart_api_for_test) {
DCHECK?

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/runtime_api.cc#newcode740
extensions/browser/api/runtime/runtime_api.cc:740: if (seconds < -1)
maybe also catch 0?

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/runtime_api.h
File extensions/browser/api/runtime/runtime_api.h (right):

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/runtime_api.h#newcode57
extensions/browser/api/runtime/runtime_api.h:57: enum class
RestartOnWatchdogStatus {
remove all refs to "watchdog"

https://codereview.chromium.org/1970613003/

[afa...@chromium.org]

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc
File extensions/browser/api/runtime/restart_after_delay_api_unittest.cc
(right):

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc#newcode23
extensions/browser/api/runtime/restart_after_delay_api_unittest.cc:23:
// Takes ownership of the |real_api_delegate|.

On 2016/06/14 16:27:21, Devlin wrote:
> comment out of date

Done.

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc#newcode138
extensions/browser/api/runtime/restart_after_delay_api_unittest.cc:138:
std::string RunFunctionGetError(UIThreadExtensionFunction* function,

On 2016/06/14 16:27:21, Devlin wrote:
> It looks like all occurrences of RunFunctionGetError() and
> RunFunctionAssertNoError() just pass in a new
> RuntimeRestartAfterDelayFunction(). Can we omit that argument and
just
> construct it in the respective functions?

Not all calls though, we have one that passes RuntimeRestartFunction.

I made some simplification. PTAL.

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc#newcode140
extensions/browser/api/runtime/restart_after_delay_api_unittest.cc:140:
const std::string& args) {

On 2016/06/14 16:27:21, Devlin wrote:
> can we also just pass in the expected error here, rather than
returning it?

Done.

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/runtime_api.cc
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/runtime_api.cc#newcode184
extensions/browser/api/runtime/runtime_api.cc:184:
registry->RegisterBooleanPref(kPrefLastRestartWasDuetoDelayedRestartApi,

On 2016/06/14 16:27:21, Devlin wrote:
> nit: DueTo, not Dueto

Done.

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/runtime_api.cc#newcode377
extensions/browser/api/runtime/runtime_api.cc:377: ;
On 2016/06/14 16:27:21, Devlin wrote:
> delete

Not sure where that came from. Done!

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/runtime_api.cc#newcode452
extensions/browser/api/runtime/runtime_api.cc:452: base::TimeDelta
future_time_since_last_restart =

On 2016/06/14 16:27:21, Devlin wrote:
> nit: "future_time_since_last_restart" sounds a little weird to me.
Maybe
> "delta_since_last_restart"?

Done.

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/runtime_api.cc#newcode453
extensions/browser/api/runtime/runtime_api.cc:453: future_restart_time >
last_delayed_restart_time_

On 2016/06/14 16:27:21, Devlin wrote:
> When would this be false (exempting cases of e.g. pref corruption)?
If this is
> false, won't it also cause problems on line 461?

Could be corruption, or the device wall clock has changed somehow,
making |last_delayed_restart_time_| in the future.

Thanks for spotting this issue! It should be TimeDelta::Max() to avoid
throttling in this case.

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/runtime_api.cc#newcode492
extensions/browser/api/runtime/runtime_api.cc:492: if (!success &&
!allow_non_kiosk_apps_restart_api_for_test) {

On 2016/06/14 16:27:21, Devlin wrote:
> DCHECK?

Done.

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/runtime_api.cc#newcode740
extensions/browser/api/runtime/runtime_api.cc:740: if (seconds < -1)

On 2016/06/14 16:27:21, Devlin wrote:
> maybe also catch 0?

Done.

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/runtime_api.h
File extensions/browser/api/runtime/runtime_api.h (right):

https://codereview.chromium.org/1970613003/diff/360001/extensions/browser/api/runtime/runtime_api.h#newcode57
extensions/browser/api/runtime/runtime_api.h:57: enum class
RestartOnWatchdogStatus {

On 2016/06/14 16:27:21, Devlin wrote:
> remove all refs to "watchdog"

Done.

https://codereview.chromium.org/1970613003/

[afa...@chromium.org]

friendly ping.

https://codereview.chromium.org/1970613003/

[rdevlin...@chromium.org]

lgtm

Again, thanks for bearing with me!


https://codereview.chromium.org/1970613003/diff/380001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc
File extensions/browser/api/runtime/restart_after_delay_api_unittest.cc
(right):

https://codereview.chromium.org/1970613003/diff/380001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc#newcode109
extensions/browser/api/runtime/restart_after_delay_api_unittest.cc:109:
// The RuntimeAPI should only be accessed (i.e. constructed) only after
the
nitty nit: one too many "only"s on this line.

https://codereview.chromium.org/1970613003/

[afa...@chromium.org]

No worries. Thank you for the detailed and careful review!

https://codereview.chromium.org/1970613003/diff/380001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc
File extensions/browser/api/runtime/restart_after_delay_api_unittest.cc
(right):

https://codereview.chromium.org/1970613003/diff/380001/extensions/browser/api/runtime/restart_after_delay_api_unittest.cc#newcode109
extensions/browser/api/runtime/restart_after_delay_api_unittest.cc:109:
// The RuntimeAPI should only be accessed (i.e. constructed) only after
the

On 2016/06/15 19:55:14, Devlin wrote:
> nitty nit: one too many "only"s on this line.

Done.

https://codereview.chromium.org/1970613003/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1970613003/400001


https://codereview.chromium.org/1970613003/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #21 (id:400001)

https://codereview.chromium.org/1970613003/

[commit-bot@chromium.org via codereview.chromium.org]

CQ bit was unchecked

https://codereview.chromium.org/1970613003/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 21 (id:??) landed as
https://crrev.com/6c1c80d89c29b22117b13760643c7fd1734e2466
Cr-Commit-Position: refs/heads/master@{#400027}

https://codereview.chromium.org/1970613003/