Updated KioskAppManager to track whether an app was auto-launched.

Updated KioskAppManager to track whether an app was auto-launched.

We want to restrict certain behavior (such as network reporting) to
auto-launched kiosk apps. This change updates KioskAppManager to track
whether a given app was auto-launched (which is different from whether a
given app is *currently* set to auto-launch, because policies can change
while an app is running).

BUG=452968
Committed: https://crrev.com/6340b5d595dd1e63c96f4c0c7192c408b05dce12
Cr-Commit-Position: refs/heads/master@{#315110}

[Andrew T Wilson (Slow), 2015-02-04 13:28:51]

atwilson@chromium.org changed reviewers:
+ bartfab@chromium.org

[Andrew T Wilson (Slow), 2015-02-04 13:28:52]

PTAL

[bartfab (slow), 2015-02-06 12:07:39]

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:137: DCHECK_EQ(app_id,
auto_launch_app_id_);
Nit: Expected value first, actual value second.

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/app_launch_controller.cc (right):

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/app_launch_controller.cc:150:
kAccountsPrefDeviceLocalAccountAutoLoginDelay, &delay)) {
Nit: #include "chromeos/settings/cros_settings_names.h"

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/app_launch_controller.cc:153: DCHECK_EQ(0, delay)
<< "Kiosks do not support non-zero auto-login delays";
It is great that you are putting this check in place but the brittleness of the
arrangement makes me feel uneasy. If someone was to implement non-zero delay
support in the future, they will just remove this check as the problem of not
supporting non-zero delay auto-login is "solved." They would not know that the
non-zero aspect is actually important to us.

How about we make it explicit that non-zero delay actually matters, by renaming
the various "auto_launched" and "AutoLaunched" to
"auto_launched_with_zero_delay" and "AutoLaunchedWithZeroDelay" and add comments
explaining why this matters?

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/kiosk_browsertest.cc:1266: // not to inject an
auto-launch policy before it runs.
Nit: I see my comment is missing a "possible." Would you mind slipping this in
as part of your CL?

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/ui/login_display_host.h (right):

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/login_display_host.h:93: // being auto-launched
on startup.
Nit: "on startup" is somewhat ambiguous. It sounds synonymous to "on boot" to me
but this is not actually what happens. The auto-launch code is triggered
whenever we reach the login screen, whether on boot, on logout or on crash.

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/ui/login_display_host_impl.h (right):

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/login_display_host_impl.h:86: void
StartAppLaunch(
Nit: The style guide says:

"For function declarations and definitions, put each argument on a separate line
unless the whole declaration fits on one line."

[Andrew T Wilson (Slow), 2015-02-06 13:24:20]

atwilson@chromium.org changed reviewers:
+ dpolukhin@chromium.org

[Andrew T Wilson (Slow), 2015-02-06 13:24:21]

bartfab: PTAL
dpolukhin: can I get your owners rubberstamp?

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:137: DCHECK_EQ(app_id,
auto_launch_app_id_);
On 2015/02/06 12:07:39, bartfab wrote:
> Nit: Expected value first, actual value second.

Done.

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/app_launch_controller.cc (right):

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/app_launch_controller.cc:150:
kAccountsPrefDeviceLocalAccountAutoLoginDelay, &delay)) {
On 2015/02/06 12:07:39, bartfab wrote:
> Nit: #include "chromeos/settings/cros_settings_names.h"

Done.

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/app_launch_controller.cc:153: DCHECK_EQ(0, delay)
<< "Kiosks do not support non-zero auto-login delays";
On 2015/02/06 12:07:39, bartfab wrote:
> It is great that you are putting this check in place but the brittleness of
the
> arrangement makes me feel uneasy. If someone was to implement non-zero delay
> support in the future, they will just remove this check as the problem of not
> supporting non-zero delay auto-login is "solved." They would not know that the
> non-zero aspect is actually important to us.
> 
> How about we make it explicit that non-zero delay actually matters, by
renaming
> the various "auto_launched" and "AutoLaunched" to
> "auto_launched_with_zero_delay" and "AutoLaunchedWithZeroDelay" and add
comments
> explaining why this matters?

Done.

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/kiosk_browsertest.cc:1266: // not to inject an
auto-launch policy before it runs.
On 2015/02/06 12:07:39, bartfab wrote:
> Nit: I see my comment is missing a "possible." Would you mind slipping this in
> as part of your CL?

Done.

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/ui/login_display_host.h (right):

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/login_display_host.h:93: // being auto-launched
on startup.
On 2015/02/06 12:07:39, bartfab wrote:
> Nit: "on startup" is somewhat ambiguous. It sounds synonymous to "on boot" to
me
> but this is not actually what happens. The auto-launch code is triggered
> whenever we reach the login screen, whether on boot, on logout or on crash.

Done.

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/ui/login_display_host_impl.h (right):

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/login_display_host_impl.h:86: void
StartAppLaunch(
On 2015/02/06 12:07:39, bartfab wrote:
> Nit: The style guide says:
> 
> "For function declarations and definitions, put each argument on a separate
line
> unless the whole declaration fits on one line."

Done.

https://codereview.chromium.org/900553006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/login_display_host_impl.h:86: void
StartAppLaunch(
On 2015/02/06 12:07:39, bartfab wrote:
> Nit: The style guide says:
> 
> "For function declarations and definitions, put each argument on a separate
line
> unless the whole declaration fits on one line."

Done.

[Andrew T Wilson (Slow), 2015-02-06 13:34:33]

atwilson@chromium.org changed reviewers:
+ xiyuan@chromium.org

[Andrew T Wilson (Slow), 2015-02-06 13:34:34]

Or, xiyuan can you owner-stamp in case Dmitry doesn't get to it?

[Dmitry Polukhin, 2015-02-06 13:41:32]

lgtm

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/existing_user_controller.cc:926:
host_->StartAppLaunch(app_id, diagnostic_mode, false /* auto_start */);
Nit, I think better to create names local variable like
const bool auto_start = false;

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1197: true /*
auto_launch */);
ditto

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:1116: true /* auto_launch
*/);
ditto

[bartfab (slow), 2015-02-06 14:14:08]

lgtm

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:104: const KioskAppData&
data, bool is_extension_pending,
Nit: The style guide says:

"For function declarations and definitions, put each argument on a separate line
unless the whole declaration fits on one line."

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:116:
KioskAppManager::App::~App() {}
Nit: Add a blank line above.

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.h (right):

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager.h:62: App(const KioskAppData&
data, bool is_extension_pending,
Nit: The style guide says:

"For function declarations and definitions, put each argument on a separate line
unless the whole declaration fits on one line."

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager.h:128: // Returns true if
current app was auto launched.
Nit: ... with zero delay.

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager.h:129: bool
IsCurrentAppAutoLaunched() const;
Nit: WithZeroDelay.

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager.h:275: std::string
currently_auto_launched_app_;
Nit: For consistency, _with_zero_delay.

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc (right):

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc:364: // Make
sure that if an app was auto launched, it is reflected in the
Nit: "with zero delay"

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc:379: // App
should still report it was auto launched even though it is no longer
Nit: "with zero delay"

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/app_launch_controller.h (right):

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/app_launch_controller.h:49: // auto-launched.
Nit: "with zero delay"

[xiyuan, 2015-02-06 16:56:35]

lgtm

[Andrew T Wilson (Slow), 2015-02-06 20:43:36]

New patchsets have been uploaded after l-g-t-m from
dpolukhin@chromium.org,bartfab@chromium.org,xiyuan@chromium.org

[Andrew T Wilson (Slow), 2015-02-06 20:43:50]

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:104: const KioskAppData&
data, bool is_extension_pending,
On 2015/02/06 14:14:07, bartfab wrote:
> Nit: The style guide says:
> 
> "For function declarations and definitions, put each argument on a separate
line
> unless the whole declaration fits on one line."

Done.

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:116:
KioskAppManager::App::~App() {}
On 2015/02/06 14:14:07, bartfab wrote:
> Nit: Add a blank line above.

Done.

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.h (right):

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager.h:62: App(const KioskAppData&
data, bool is_extension_pending,
On 2015/02/06 14:14:07, bartfab wrote:
> Nit: The style guide says:
> 
> "For function declarations and definitions, put each argument on a separate
line
> unless the whole declaration fits on one line."

Done.

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager.h:128: // Returns true if
current app was auto launched.
On 2015/02/06 14:14:07, bartfab wrote:
> Nit: ... with zero delay.

Done.

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager.h:129: bool
IsCurrentAppAutoLaunched() const;
On 2015/02/06 14:14:07, bartfab wrote:
> Nit: WithZeroDelay.

Done.

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager.h:275: std::string
currently_auto_launched_app_;
On 2015/02/06 14:14:07, bartfab wrote:
> Nit: For consistency, _with_zero_delay.

Done, but this is an incredibly unwieldy 44 character name now, and I think it's
a poor tradeoff of usability/readability vs consistency.

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc (right):

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc:364: // Make
sure that if an app was auto launched, it is reflected in the
On 2015/02/06 14:14:08, bartfab wrote:
> Nit: "with zero delay"

Done.

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc:379: // App
should still report it was auto launched even though it is no longer
On 2015/02/06 14:14:07, bartfab wrote:
> Nit: "with zero delay"

Done.

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/app_launch_controller.h (right):

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/app_launch_controller.h:49: // auto-launched.
On 2015/02/06 14:14:08, bartfab wrote:
> Nit: "with zero delay"

Done.

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/existing_user_controller.cc:926:
host_->StartAppLaunch(app_id, diagnostic_mode, false /* auto_start */);
On 2015/02/06 13:41:32, Dmitry Polukhin wrote:
> Nit, I think better to create names local variable like
> const bool auto_start = false;

Done.

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1197: true /*
auto_launch */);
On 2015/02/06 13:41:32, Dmitry Polukhin wrote:
> ditto

Done.

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/900553006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:1116: true /* auto_launch
*/);
On 2015/02/06 13:41:32, Dmitry Polukhin wrote:
> ditto

Done.

[Andrew T Wilson (Slow), 2015-02-06 20:43:58]

The CQ bit was checked by atwilson@chromium.org

[commit-bot: I haz the power, 2015-02-06 20:45:09]

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

[commit-bot: I haz the power, 2015-02-06 21:31:44]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-06 21:32:37]

Patchset 5 (id:??) landed as
https://crrev.com/6340b5d595dd1e63c96f4c0c7192c408b05dce12
Cr-Commit-Position: refs/heads/master@{#315110}