Add the ability to show a demo app on OOBE if a machine is derelict.

Add the ability to show a demo app on OOBE if a machine is derelict.
If a machine has been idle for 8 hours+, we should start showing a demo app everytime the machine is idle for 5 minutes.
This CL depends on https://chrome-internal-review.googlesource.com/#/c/154276/

R=xiyuan@chromium.org, zelidrag@chromium.org
BUG=336585
TEST=Use the derelict-* flags to test demo mode app launching.

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

[xiyuan, 2014-02-11 03:42:45]

https://codereview.chromium.org/156493004/diff/1/chrome/browser/chromeos/idle...
File chrome/browser/chromeos/idle_detector.cc (right):

https://codereview.chromium.org/156493004/diff/1/chrome/browser/chromeos/idle...
chrome/browser/chromeos/idle_detector.cc:27: active_callback_.Run();
OnUserActivity is called quite frequently and we should avoid unnecessary calls.

Can we support null callback and skip call if callback.is_null()?

https://codereview.chromium.org/156493004/diff/1/chrome/browser/chromeos/idle...
File chrome/browser/chromeos/idle_detector.h (right):

https://codereview.chromium.org/156493004/diff/1/chrome/browser/chromeos/idle...
chrome/browser/chromeos/idle_detector.h:27: // Resets |timer_| to fire when the
logout dialog should be shown.
nit: update the comment

https://codereview.chromium.org/156493004/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc (right):

https://codereview.chromium.org/156493004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:21: }
nit: insert an empty line

https://codereview.chromium.org/156493004/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/demo_app/manifest.json (right):

https://codereview.chromium.org/156493004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/demo_app/manifest.json:9: "launch": {
V1 app is not officially supported and code is not maintained. Suggest to make
it a platform app.

https://codereview.chromium.org/156493004/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (right):

https://codereview.chromium.org/156493004/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:183: 
nit: nuke one empty line

https://codereview.chromium.org/156493004/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:266: new
IdleDetector(base::Bind(&base::DoNothing),
Think we should use a base::Closure() instead of DoNothing to avoid unnecessary
calls.

https://codereview.chromium.org/156493004/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:274:
detector_->Start(derelict_detection_timeout_);
nit: simplify to
detector_->Start(is_derelict_ ? derelict_idle_timeout_ :
derelict_detection_timeout_);

https://codereview.chromium.org/156493004/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:301:
base::StringToInt64(
nit: Handle StringToInt64 failure.

https://codereview.chromium.org/156493004/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:397:
input_methods_list->Append(input_method);
This does not look like intended.

[rkc, 2014-02-11 04:35:57]

https://codereview.chromium.org/156493004/diff/1/chrome/browser/chromeos/idle...
File chrome/browser/chromeos/idle_detector.cc (right):

https://codereview.chromium.org/156493004/diff/1/chrome/browser/chromeos/idle...
chrome/browser/chromeos/idle_detector.cc:27: active_callback_.Run();
On 2014/02/11 03:42:45, xiyuan wrote:
> OnUserActivity is called quite frequently and we should avoid unnecessary
calls.
> 
> Can we support null callback and skip call if callback.is_null()?

Done.

https://codereview.chromium.org/156493004/diff/1/chrome/browser/chromeos/idle...
File chrome/browser/chromeos/idle_detector.h (right):

https://codereview.chromium.org/156493004/diff/1/chrome/browser/chromeos/idle...
chrome/browser/chromeos/idle_detector.h:27: // Resets |timer_| to fire when the
logout dialog should be shown.
On 2014/02/11 03:42:45, xiyuan wrote:
> nit: update the comment

Done.

https://codereview.chromium.org/156493004/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc (right):

https://codereview.chromium.org/156493004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:21: }
On 2014/02/11 03:42:45, xiyuan wrote:
> nit: insert an empty line

Done.

https://codereview.chromium.org/156493004/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/demo_app/manifest.json (right):

https://codereview.chromium.org/156493004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/demo_app/manifest.json:9: "launch": {
On 2014/02/11 03:42:45, xiyuan wrote:
> V1 app is not officially supported and code is not maintained. Suggest to make
> it a platform app.

Done.
Also made the corresponding changes to the ChromiumOS repo CL.

https://codereview.chromium.org/156493004/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (right):

https://codereview.chromium.org/156493004/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:183: 
On 2014/02/11 03:42:45, xiyuan wrote:
> nit: nuke one empty line

Done.

https://codereview.chromium.org/156493004/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:266: new
IdleDetector(base::Bind(&base::DoNothing),
On 2014/02/11 03:42:45, xiyuan wrote:
> Think we should use a base::Closure() instead of DoNothing to avoid
unnecessary
> calls.

Done.

https://codereview.chromium.org/156493004/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:274:
detector_->Start(derelict_detection_timeout_);
On 2014/02/11 03:42:45, xiyuan wrote:
> nit: simplify to
> detector_->Start(is_derelict_ ? derelict_idle_timeout_ :
> derelict_detection_timeout_);

Done.

https://codereview.chromium.org/156493004/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:301:
base::StringToInt64(
On 2014/02/11 03:42:45, xiyuan wrote:
> nit: Handle StringToInt64 failure.

Handled here and below.
Done.

https://codereview.chromium.org/156493004/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:397:
input_methods_list->Append(input_method);
On 2014/02/11 03:42:45, xiyuan wrote:
> This does not look like intended.

Weird, not sure where that came from. Fixed.

[xiyuan, 2014-02-11 04:47:44]

LGTM

https://codereview.chromium.org/156493004/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (right):

https://codereview.chromium.org/156493004/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:300:
derelict_detection_timeout = kDerelectDetectionTimeoutSeconds;
nit: Optimize a little bit to avoid duplicate code.

if (!command_line->HasSwitch(switches::kDerelictDetectionTimeout) ||
    !base::StringToInt64(...))
  derelict_detection_timeout = kDerelectDetectionTimeoutSeconds;

[rkc, 2014-02-11 19:44:49]

https://codereview.chromium.org/156493004/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (right):

https://codereview.chromium.org/156493004/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:300:
derelict_detection_timeout = kDerelectDetectionTimeoutSeconds;
On 2014/02/11 04:47:45, xiyuan wrote:
> nit: Optimize a little bit to avoid duplicate code.
> 
> if (!command_line->HasSwitch(switches::kDerelictDetectionTimeout) ||
>     !base::StringToInt64(...))
>   derelict_detection_timeout = kDerelectDetectionTimeoutSeconds;

Fixed, here and below.
Done.

[rkc, 2014-02-11 19:50:55]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-02-11 19:53:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/156493004/290001

[commit-bot: I haz the power, 2014-02-11 20:15:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-11 20:15:20]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[rkc, 2014-02-11 22:06:27]

Adding thestig@ for OWNERS reviews.

[Lei Zhang, 2014-02-11 22:13:58]

lgtm with some nits.

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
File chrome/browser/chromeos/idle_detector.cc (right):

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/idle_detector.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
also 2014, no (c)

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/idle_detector.cc:10: #include "base/lazy_instance.h"
not used

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/idle_detector.cc:12: #include
"base/message_loop/message_loop.h"
not used? probably #include "base/location.h" instead for FROM_HERE

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
File chrome/browser/chromeos/idle_detector.h (right):

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/idle_detector.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
nit: it's 2014, no (c)

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc (right):

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:1: // Copyright (c)
2011 The Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:47:
base::FilePath(FILE_PATH_LITERAL("/usr/share/chromeos-assets/demo_app")));
you don't need FILE_PATH_LITERAL since it's CrOS-only code.

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:66: LOG(ERROR) <<
"Loading the Kiosk Profile failed.";
Wouldn't you want to include GetErrorMessage(error) in this case?

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h (right):

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h:28:
scoped_ptr<KioskProfileLoader> kiosk_profile_loader_;
#include "base/memory/scoped_ptr.h"

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (right):

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:292:
DCHECK(prefs);
nit: You don't need this DCHECK or the one above. If you crash, on the next
line, it's really obvious what went wrong. You also don't DCHECK on line 280.

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:298:
command_line->GetSwitchValueASCII(switches::kDerelictDetectionTimeout),
nit: this line and the next should 4-space indented. Perhaps rename
|command_line| to |cmdline| to give yourself the space needed here.

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:308:
command_line->GetSwitchValueASCII(switches::kDerelictIdleTimeout),
Same nit as line 298.

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.h (right):

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.h:18: class
PrefService;
nit: not needed

[rkc, 2014-02-11 22:37:38]

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
File chrome/browser/chromeos/idle_detector.cc (right):

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/idle_detector.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2014/02/11 22:13:59, Lei Zhang wrote:
> also 2014, no (c)

Fixed, also fixed the year since its a new file - here and in the .h
Done.

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/idle_detector.cc:10: #include "base/lazy_instance.h"
On 2014/02/11 22:13:59, Lei Zhang wrote:
> not used

Done.

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/idle_detector.cc:12: #include
"base/message_loop/message_loop.h"
On 2014/02/11 22:13:59, Lei Zhang wrote:
> not used? probably #include "base/location.h" instead for FROM_HERE

Done.

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
File chrome/browser/chromeos/idle_detector.h (right):

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/idle_detector.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2014/02/11 22:13:59, Lei Zhang wrote:
> nit: it's 2014, no (c)

Done.

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc (right):

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:1: // Copyright (c)
2011 The Chromium Authors. All rights reserved.
On 2014/02/11 22:13:59, Lei Zhang wrote:
> 2014

Done.

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:47:
base::FilePath(FILE_PATH_LITERAL("/usr/share/chromeos-assets/demo_app")));
On 2014/02/11 22:13:59, Lei Zhang wrote:
> you don't need FILE_PATH_LITERAL since it's CrOS-only code.

Done.

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:66: LOG(ERROR) <<
"Loading the Kiosk Profile failed.";
On 2014/02/11 22:13:59, Lei Zhang wrote:
> Wouldn't you want to include GetErrorMessage(error) in this case?

There isn't anything we can do with the error at this point. I'll be
implementing a more fleshed out error handler in a future CL.

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h (right):

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2014/02/11 22:13:59, Lei Zhang wrote:
> 2014

Done.

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h:28:
scoped_ptr<KioskProfileLoader> kiosk_profile_loader_;
On 2014/02/11 22:13:59, Lei Zhang wrote:
> #include "base/memory/scoped_ptr.h"

Done.

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (right):

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:292:
DCHECK(prefs);
On 2014/02/11 22:13:59, Lei Zhang wrote:
> nit: You don't need this DCHECK or the one above. If you crash, on the next
> line, it's really obvious what went wrong. You also don't DCHECK on line 280.

Done.

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:298:
command_line->GetSwitchValueASCII(switches::kDerelictDetectionTimeout),
On 2014/02/11 22:13:59, Lei Zhang wrote:
> nit: this line and the next should 4-space indented. Perhaps rename
> |command_line| to |cmdline| to give yourself the space needed here.

Done.

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:308:
command_line->GetSwitchValueASCII(switches::kDerelictIdleTimeout),
On 2014/02/11 22:13:59, Lei Zhang wrote:
> Same nit as line 298.

Done.

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.h (right):

https://codereview.chromium.org/156493004/diff/450001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.h:18: class
PrefService;
On 2014/02/11 22:13:59, Lei Zhang wrote:
> nit: not needed

Done.

[rkc, 2014-02-11 22:58:38]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-02-11 23:02:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/156493004/580001

[rkc, 2014-02-11 23:52:51]

The CQ bit was unchecked by rkc@chromium.org

[rkc, 2014-02-11 23:58:59]

The last patch fixes 2 major issues with the CL; one is making sure we mount the
user partition ephemeral, the second ensures that we handle the Chrome crash
case correctly.

Please re-review the added changes, thanks!

[xiyuan, 2014-02-12 00:03:22]

SLGTM

https://codereview.chromium.org/156493004/diff/650002/chrome/browser/chromeos...
File chrome/browser/chromeos/login/authenticator.h (right):

https://codereview.chromium.org/156493004/diff/650002/chrome/browser/chromeos...
chrome/browser/chromeos/login/authenticator.h:62: const std::string&
app_user_id, bool force_ephemeral) = 0;
nit: Document the new arg.

https://codereview.chromium.org/156493004/diff/650002/chrome/browser/chromeos...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc (right):

https://codereview.chromium.org/156493004/diff/650002/chrome/browser/chromeos...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:42: 
nit: nuke one empty line

https://codereview.chromium.org/156493004/diff/650002/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/156493004/diff/650002/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.cc:622: if
(chromeos::DemoAppLauncher::IsDemoAppSession(
nit: Add a comment to explain why we terminate session here.

[rkc, 2014-02-12 00:08:06]

https://codereview.chromium.org/156493004/diff/650002/chrome/browser/chromeos...
File chrome/browser/chromeos/login/authenticator.h (right):

https://codereview.chromium.org/156493004/diff/650002/chrome/browser/chromeos...
chrome/browser/chromeos/login/authenticator.h:62: const std::string&
app_user_id, bool force_ephemeral) = 0;
On 2014/02/12 00:03:22, xiyuan wrote:
> nit: Document the new arg.

Done.

https://codereview.chromium.org/156493004/diff/650002/chrome/browser/chromeos...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc (right):

https://codereview.chromium.org/156493004/diff/650002/chrome/browser/chromeos...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:42: 
On 2014/02/12 00:03:22, xiyuan wrote:
> nit: nuke one empty line

Done.

https://codereview.chromium.org/156493004/diff/650002/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/156493004/diff/650002/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.cc:622: if
(chromeos::DemoAppLauncher::IsDemoAppSession(
On 2014/02/12 00:03:22, xiyuan wrote:
> nit: Add a comment to explain why we terminate session here.

Done.

[Lei Zhang, 2014-02-12 00:13:57]

The non-CrOS files didn't change much, so still LGTM

https://codereview.chromium.org/156493004/diff/960001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/156493004/diff/960001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.cc:43: #include
"chrome/browser/lifetime/application_lifetime.h"
is this needed?

[rkc, 2014-02-12 00:20:50]

https://codereview.chromium.org/156493004/diff/960001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/156493004/diff/960001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.cc:43: #include
"chrome/browser/lifetime/application_lifetime.h"
On 2014/02/12 00:13:57, Lei Zhang wrote:
> is this needed?

That's where chrome::AttemptUserExit is defined. Since this is only used in
ChromeOS specific code, moved the include to the ifdef OS_CHROMEOS section
below.

[rkc, 2014-02-12 00:21:13]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-02-12 00:23:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/156493004/1050001

[commit-bot: I haz the power, 2014-02-12 02:21:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-12 02:21:24]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, google_apis_unittests,
gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[rkc, 2014-02-12 02:24:10]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-02-12 02:25:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/156493004/1050001

[commit-bot: I haz the power, 2014-02-12 09:41:47]

Change committed as 250666

[bartfab (slow), 2014-02-13 19:51:52]

I am concerned about the quality of this CL. This is a big feature that touches
the login stack and the kiosk-app stack. Yet, it comes with no tests.

The crash handling mechanisms built into this CL looks to me like it will cause
crash loops.

The code looks like it was hastily written with many style guide violations.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/kiosk_profile_loader.h (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/kiosk_profile_loader.h:21: class
KioskAppManager;
Nit: No longer used.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/kiosk_profile_loader.h:64: std::string
user_id_;
Nit: This can now be made const.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/kiosk_profile_loader.h:65: bool
force_ephemeral_;
Nit: const.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/idle_detector.cc (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/idle_detector.cc:9: #include "base/bind.h"
Nit: Not used.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/idle_detector.cc:10: #include "base/logging.h"
Nit: Not used.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/idle_detector.cc:19: if (ash::Shell::HasInstance() &&
Nit: Multi-line ifs should have curly braces.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/idle_detector.cc:38: if (timer_.IsRunning()) {
Nit: No need for curly braces in single-line ifs.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/idle_detector.cc:41: timer_.Start(FROM_HERE, timeout_,
idle_callback_);
Nit: #include "base/location.h"

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/idle_detector.h (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/idle_detector.h:15: class IdleDetector : public
ash::UserActivityObserver {
The location and name of this file imply that it is something very generic.
However, doing idle detection inside Chrome like this is a hack that should not
be used in the general case. The correct place for idle detection is powerd,
which can be configured to take into account other signals, such as audio and
video activity.

At the very least, the class should be renamed and a comment should be added
that explains it is not meant to be used in the general case that idleness needs
to be determined.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/idle_detector.h:17: IdleDetector(const base::Closure&
on_active_callback,
What is |on_active_callback| meant for? Nobody ever uses it.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/idle_detector.h:32: base::Closure active_callback_;
Nit: #include "base/callback.h"

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/idle_detector.h:35: base::TimeDelta timeout_;
Nit: #include "base/time.h"

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/app_launch_controller.cc (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/app_launch_controller.cc:124:
CHECK(KioskAppManager::Get() &&
Nit 1: Better split this into two separate CHECKs. That way, if one of the two
conditions fails, you will know which one.

Nit 2: #include "base/logging.h"

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/authenticator.h (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/authenticator.h:61: // |force_ephemeral| specifies
whether to force the session to be ephemeral.
Here and elsewhere: Why is the term "force" part of this argument's name? It is
not like it forces a behavior, overriding some default. It switches between
ephemeral and non-ephemeral. It should simply be called |ephemeral|.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/authenticator.h:63: const std::string&
app_user_id, bool force_ephemeral) = 0;
This violates the style guide. Either the entire declaration fits on one line or
each argument goes on a separate line.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:26:
DemoAppLauncher::DemoAppLauncher() : profile_(NULL) {}
Nit: #include "chrome/browser/profiles/profile.h"

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:31: DVLOG(1) <<
"Launching demo app...";
Nit: #include "base/logging.h"

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:40: return user_id
== kDemoAppUserId ? true : false;
Nit: No need for a ternary operator. The result of |user_id == kDemoAppUserId|
is a bool already.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:52: std::string
extension_id = extension_service->component_loader()->Add(
Nit: const

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:54:
base::FilePath("/usr/share/chromeos-assets/demo_app"));
Nit: #include "base/files/file_path.h"

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:56: const
extensions::Extension* extension =
Nit: #include "extensions/common/extension.h"

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:64: profile_,
extension, extensions::LAUNCH_CONTAINER_WINDOW, NEW_WINDOW));
Nit 1: #include "chrome/common/extensions/extension_constants.h"
Nit 2: #include "ui/base/window_open_disposition.h"

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:69:
LoginDisplayHostImpl::default_host()->Finalize();
Nit: #include "chrome/browser/chromeos/login/login_display_host.h"

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h:23: static bool
IsDemoAppSession(const std::string& user_id);
Nit: #include <string>

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h:27: virtual void
OnProfileLoaded(Profile* profile) OVERRIDE;
Nit: #include "base/compiler_specific.h"

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h:30: Profile*
profile_;
This member is unnecessary. |profile_| is only ever used in the one method that
sets it.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h:33:
DISALLOW_COPY_AND_ASSIGN(DemoAppLauncher);
Nit: #include "base/basictypes.h"

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/login_display_host_impl.cc (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/login_display_host_impl.cc:664:
demo_app_launcher_.reset(new DemoAppLauncher());
Nit: No need for the ()

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/login_display_host_impl.h (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/login_display_host_impl.h:16: #include
"chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h"
Nit: Forward-declare |DemoAppLauncher| here and include the demo_app_launcher.h
in the implementation only.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/login_performer.cc (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/login_performer.cc:300: const std::string&
app_user_id, bool force_ephemeral) {
This violates the style guide. Either the entire declaration fits on one line or
each argument goes on a separate line.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/login_performer.h (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/login_performer.h:86: bool force_ephemeral);
Nit: As before, I see no reason to call this argument |force_ephemeral|. Why not
just |ephemeral|?

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/mock_authenticator.cc (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/mock_authenticator.cc:65: const std::string&
app_user_id, bool force_ephemeral) {
This violates the style guide. Either the entire declaration fits on one line or
each argument goes on a separate line.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/mock_authenticator.h (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/mock_authenticator.h:42: const std::string&
app_user_id, bool force_ephemeral) OVERRIDE;
This violates the style guide. Either the entire declaration fits on one line or
each argument goes on a separate line.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/parallel_authenticator.cc:349: const std::string&
app_user_id, bool force_ephemeral) {
This violates the style guide. Either the entire declaration fits on one line or
each argument goes on a separate line.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/parallel_authenticator.cc:360: int ephemeral_flag
= force_ephemeral ? cryptohome::ENSURE_EPHEMERAL : 0;
Nit: const

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/parallel_authenticator.h (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/parallel_authenticator.h:135: // Mounts an public
but non-ephemeral cryptohome and notifies consumer on the
This comment is no longer accurate. There is an argument now that switches
between ephemeral and non-ephemeral.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/parallel_authenticator.h:138: const std::string&
app_user_id, bool force_ephemeral) OVERRIDE;
This violates the style guide. Either the entire declaration fits on one line or
each argument goes on a separate line.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/demo_app/manifest.json (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/resourc...
chrome/browser/resources/chromeos/demo_app/manifest.json:1: {
I find it strange that we ship the manifest with the Chrome source code and the
app separately. Is there any reason that the manifest cannot reside inside
/usr/share/chromeos-assets/demo_app so that it gets updated automatically
together with the app?

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/resourc...
chrome/browser/resources/chromeos/demo_app/manifest.json:2: //
chrome-extension://klimoghijjogocdbaikffefjfcfheiel
Comments are not allowed in JSON. This file is invalid.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator.cc:627:
chrome::AttemptUserExit();
Have you tested this? In my experience, Chrome is unable to shut down cleanly in
the first few seconds after launch because too many start-up tasks are still
running that do not expect to be torn down in mid-flight. I would expect
AttemptUserExit() to crash Chrome, probably leading to a crash loop.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:18: #include
"chrome/browser/chromeos/app_mode/kiosk_app_manager.h"
Nit: Not used.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:51: const int64
kDerelectDetectionTimeoutSeconds = 8 * 60 * 60; // 8 hours.
Why is this an int64? int would have been entirely sufficient, even on 16-bit
machines.

Nit: 2 spaces before trailing comment.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:52: const int64
kDerelectIdleTimeoutSeconds = 5 * 60; // 5 minutes.
Why is this an int64? int would have been entirely sufficient, even on 16-bit
machines.

Nit: 2 spaces before trailing comment.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:276:
LoginDisplayHost* host = LoginDisplayHostImpl::default_host();
Nit: #include "chrome/browser/chromeos/login/login_display_host.h"

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:289:
DCHECK(cmdline);
Nit: #include "base/logging.h"

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.h (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.h:111:
scoped_ptr<IdleDetector> detector_;
Nit 1: #include "base/memory/scoped_ptr.h"
Nit 2: |detector_| is very generic. What does it detect? Why not
|idle_detector_|?

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.h:115: // Timeout
before showing our demo up if the machine is in a derelict state.
Nit: s/up/app/.

https://codereview.chromium.org/156493004/diff/1050001/chromeos/chromeos_swit...
File chromeos/chromeos_switches.cc (right):

https://codereview.chromium.org/156493004/diff/1050001/chromeos/chromeos_swit...
chromeos/chromeos_switches.cc:27: // Time before a machine at OOBE is considered
derelict
Nit: End sentence with full stop.

[rkc, 2014-02-13 23:22:00]

Hey Bartosz,

Thanks for the extensive review. I've addressed all the comments here but the CL
with the changes is at,
https://codereview.chromium.org/165173002/ (since this code is already
committed).

I am currently working on the browser_tests for this. I do agree that they
should probably have gone in with the original CL, but I did want to get the
functionality in before our dev-channel to get the maximum amount of coverage I
could on the CL. The tests are being worked on and will be checked in soon.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/kiosk_profile_loader.h (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/kiosk_profile_loader.h:21: class
KioskAppManager;
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/kiosk_profile_loader.h:64: std::string
user_id_;
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: This can now be made const.

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/kiosk_profile_loader.h:65: bool
force_ephemeral_;
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: const.

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/idle_detector.cc (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/idle_detector.cc:9: #include "base/bind.h"
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: Not used.

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/idle_detector.cc:10: #include "base/logging.h"
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: Not used.

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/idle_detector.cc:19: if (ash::Shell::HasInstance() &&
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: Multi-line ifs should have curly braces.

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/idle_detector.cc:38: if (timer_.IsRunning()) {
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: No need for curly braces in single-line ifs.

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/idle_detector.cc:41: timer_.Start(FROM_HERE, timeout_,
idle_callback_);
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: #include "base/location.h"

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/idle_detector.h (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/idle_detector.h:15: class IdleDetector : public
ash::UserActivityObserver {
On 2014/02/13 19:51:53, bartfab wrote:
> The location and name of this file imply that it is something very generic.
> However, doing idle detection inside Chrome like this is a hack that should
not
> be used in the general case. The correct place for idle detection is powerd,
> which can be configured to take into account other signals, such as audio and
> video activity.
> 
> At the very least, the class should be renamed and a comment should be added
> that explains it is not meant to be used in the general case that idleness
needs
> to be determined.

UserActivityDetector is used multiple places in the code to detect idle
behavior. When I originally wrote the code for the idle logout detector, I did
use power manager to detect idle behavior but at that time derat@ had suggested
that we should use the UserActivityDetector instead.

If this is not the ideal way to detect user active/idle state anymore, I'll
discuss the new correct way with derat@ and get it in a separate CL. This code
currently works for our purposes (unless someone decides to play a video on
OOBE, which I am hoping won't happen in M34?) and it isn't a good idea to change
this code significantly before M34 branches on Monday.

I've created a bug and added a comment to address this in M35.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/idle_detector.h:17: IdleDetector(const base::Closure&
on_active_callback,
On 2014/02/13 19:51:53, bartfab wrote:
> What is |on_active_callback| meant for? Nobody ever uses it.

This code is entirely reusable for the idle_logout dialog code in retail mode,
provided we also have an on_active callback. I intend to use this class for that
code but wouldn't like to refactor this class again when I do.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/idle_detector.h:32: base::Closure active_callback_;
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: #include "base/callback.h"

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/idle_detector.h:35: base::TimeDelta timeout_;
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: #include "base/time.h"

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/app_launch_controller.cc (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/app_launch_controller.cc:124:
CHECK(KioskAppManager::Get() &&
On 2014/02/13 19:51:53, bartfab wrote:
> Nit 1: Better split this into two separate CHECKs. That way, if one of the two
> conditions fails, you will know which one.
> 
> Nit 2: #include "base/logging.h"

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/authenticator.h (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/authenticator.h:61: // |force_ephemeral| specifies
whether to force the session to be ephemeral.
On 2014/02/13 19:51:53, bartfab wrote:
> Here and elsewhere: Why is the term "force" part of this argument's name? It
is
> not like it forces a behavior, overriding some default. It switches between
> ephemeral and non-ephemeral. It should simply be called |ephemeral|.

Not really. If this flag is not specified, the ephemeral behavior will be
determined by enterprise policy. This flag specifically 'forces' us to use an
ephemeral mount, regardless of the enterprise policy. Hence, the name is
appropriate.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/authenticator.h:63: const std::string&
app_user_id, bool force_ephemeral) = 0;
On 2014/02/13 19:51:53, bartfab wrote:
> This violates the style guide. Either the entire declaration fits on one line
or
> each argument goes on a separate line.

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:26:
DemoAppLauncher::DemoAppLauncher() : profile_(NULL) {}
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: #include "chrome/browser/profiles/profile.h"

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:31: DVLOG(1) <<
"Launching demo app...";
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: #include "base/logging.h"

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:40: return user_id
== kDemoAppUserId ? true : false;
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: No need for a ternary operator. The result of |user_id == kDemoAppUserId|
> is a bool already.

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:52: std::string
extension_id = extension_service->component_loader()->Add(
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: const

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:54:
base::FilePath("/usr/share/chromeos-assets/demo_app"));
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: #include "base/files/file_path.h"

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:54:
base::FilePath("/usr/share/chromeos-assets/demo_app"));
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: #include "base/files/file_path.h"

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:56: const
extensions::Extension* extension =
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: #include "extensions/common/extension.h"

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:64: profile_,
extension, extensions::LAUNCH_CONTAINER_WINDOW, NEW_WINDOW));
On 2014/02/13 19:51:53, bartfab wrote:
> Nit 1: #include "chrome/common/extensions/extension_constants.h"
> Nit 2: #include "ui/base/window_open_disposition.h"

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:64: profile_,
extension, extensions::LAUNCH_CONTAINER_WINDOW, NEW_WINDOW));
On 2014/02/13 19:51:53, bartfab wrote:
> Nit 1: #include "chrome/common/extensions/extension_constants.h"
> Nit 2: #include "ui/base/window_open_disposition.h"

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:69:
LoginDisplayHostImpl::default_host()->Finalize();
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: #include "chrome/browser/chromeos/login/login_display_host.h"

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h:23: static bool
IsDemoAppSession(const std::string& user_id);
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: #include <string>

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h:27: virtual void
OnProfileLoaded(Profile* profile) OVERRIDE;
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: #include "base/compiler_specific.h"

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h:27: virtual void
OnProfileLoaded(Profile* profile) OVERRIDE;
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: #include "base/compiler_specific.h"

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h:30: Profile*
profile_;
On 2014/02/13 19:51:53, bartfab wrote:
> This member is unnecessary. |profile_| is only ever used in the one method
that
> sets it.

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h:33:
DISALLOW_COPY_AND_ASSIGN(DemoAppLauncher);
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: #include "base/basictypes.h"

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/login_display_host_impl.cc (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/login_display_host_impl.cc:664:
demo_app_launcher_.reset(new DemoAppLauncher());
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: No need for the ()

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/login_display_host_impl.h (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/login_display_host_impl.h:16: #include
"chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h"
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: Forward-declare |DemoAppLauncher| here and include the
demo_app_launcher.h
> in the implementation only.

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/login_performer.cc (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/login_performer.cc:300: const std::string&
app_user_id, bool force_ephemeral) {
On 2014/02/13 19:51:53, bartfab wrote:
> This violates the style guide. Either the entire declaration fits on one line
or
> each argument goes on a separate line.

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/login_performer.h (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/login_performer.h:86: bool force_ephemeral);
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: As before, I see no reason to call this argument |force_ephemeral|. Why
not
> just |ephemeral|?

Explained on the other comment.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/mock_authenticator.cc (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/mock_authenticator.cc:65: const std::string&
app_user_id, bool force_ephemeral) {
On 2014/02/13 19:51:53, bartfab wrote:
> This violates the style guide. Either the entire declaration fits on one line
or
> each argument goes on a separate line.

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/mock_authenticator.h (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/mock_authenticator.h:42: const std::string&
app_user_id, bool force_ephemeral) OVERRIDE;
On 2014/02/13 19:51:53, bartfab wrote:
> This violates the style guide. Either the entire declaration fits on one line
or
> each argument goes on a separate line.

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/parallel_authenticator.cc:349: const std::string&
app_user_id, bool force_ephemeral) {
On 2014/02/13 19:51:53, bartfab wrote:
> This violates the style guide. Either the entire declaration fits on one line
or
> each argument goes on a separate line.

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/parallel_authenticator.cc:360: int ephemeral_flag
= force_ephemeral ? cryptohome::ENSURE_EPHEMERAL : 0;
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: const

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/parallel_authenticator.h (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/parallel_authenticator.h:135: // Mounts an public
but non-ephemeral cryptohome and notifies consumer on the
On 2014/02/13 19:51:53, bartfab wrote:
> This comment is no longer accurate. There is an argument now that switches
> between ephemeral and non-ephemeral.

Fixed the comment and added details about the force_ephemeral behavior.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/login/parallel_authenticator.h:138: const std::string&
app_user_id, bool force_ephemeral) OVERRIDE;
On 2014/02/13 19:51:53, bartfab wrote:
> This violates the style guide. Either the entire declaration fits on one line
or
> each argument goes on a separate line.

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/demo_app/manifest.json (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/resourc...
chrome/browser/resources/chromeos/demo_app/manifest.json:1: {
On 2014/02/13 19:51:53, bartfab wrote:
> I find it strange that we ship the manifest with the Chrome source code and
the
> app separately. Is there any reason that the manifest cannot reside inside
> /usr/share/chromeos-assets/demo_app so that it gets updated automatically
> together with the app?

We're loading this as a component app, not a regular app. If we switch to
loading this just as a regular app, we can move this manifest out. I am
discussing the pros/cons of that with a few folks here before deciding what to
do with it in M35.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/resourc...
chrome/browser/resources/chromeos/demo_app/manifest.json:2: //
chrome-extension://klimoghijjogocdbaikffefjfcfheiel
On 2014/02/13 19:51:53, bartfab wrote:
> Comments are not allowed in JSON. This file is invalid.

Almost every component app manifest in the tree has that comment, so I am pretty
sure that the manifest parser handles comments fine.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator.cc:627:
chrome::AttemptUserExit();
On 2014/02/13 19:51:53, bartfab wrote:
> Have you tested this? In my experience, Chrome is unable to shut down cleanly
in
> the first few seconds after launch because too many start-up tasks are still
> running that do not expect to be torn down in mid-flight. I would expect
> AttemptUserExit() to crash Chrome, probably leading to a crash loop.

Yes, multiple times. Works fine.
If you also look at the code above for "chromeos::LaunchAppOrDie", if we are
unable to launch the app, we do AttemptUserExit there too.

Additionally, AttemptUserExit doesn't actually close the browser, but instead
notifies Session Manager that our session is over and requests that it terminate
us. In case Chrome 'does' crash due to any reason at this point, we won't have a
crash loop since SessionManager already knows that our session is over and will
bring us back to OOBE again.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:18: #include
"chrome/browser/chromeos/app_mode/kiosk_app_manager.h"
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: Not used.

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:51: const int64
kDerelectDetectionTimeoutSeconds = 8 * 60 * 60; // 8 hours.
On 2014/02/13 19:51:53, bartfab wrote:
> Why is this an int64? int would have been entirely sufficient, even on 16-bit
> machines.
> 
> Nit: 2 spaces before trailing comment.

To keep the type consistent with what base::Time::FromSeconds expects.
I am probably going to change this to milliseconds anyway once I check in the
tests since I don't really want us to have to wait 1s during tests.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:52: const int64
kDerelectIdleTimeoutSeconds = 5 * 60; // 5 minutes.
On 2014/02/13 19:51:53, bartfab wrote:
> Why is this an int64? int would have been entirely sufficient, even on 16-bit
> machines.
> 
> Nit: 2 spaces before trailing comment.

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:276:
LoginDisplayHost* host = LoginDisplayHostImpl::default_host();
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: #include "chrome/browser/chromeos/login/login_display_host.h"

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:289:
DCHECK(cmdline);
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: #include "base/logging.h"

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.h (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.h:111:
scoped_ptr<IdleDetector> detector_;
On 2014/02/13 19:51:53, bartfab wrote:
> Nit 1: #include "base/memory/scoped_ptr.h"
> Nit 2: |detector_| is very generic. What does it detect? Why not
> |idle_detector_|?

Done.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.h:115: // Timeout
before showing our demo up if the machine is in a derelict state.
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: s/up/app/.

Done.

https://codereview.chromium.org/156493004/diff/1050001/chromeos/chromeos_swit...
File chromeos/chromeos_switches.cc (right):

https://codereview.chromium.org/156493004/diff/1050001/chromeos/chromeos_swit...
chromeos/chromeos_switches.cc:27: // Time before a machine at OOBE is considered
derelict
On 2014/02/13 19:51:53, bartfab wrote:
> Nit: End sentence with full stop.

Done.

[bartfab (slow), 2014-02-17 15:53:45]

Thanks for uploading the clean-up CL. I left some comments there and replied to
a few others here.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
File chrome/browser/chromeos/idle_detector.h (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/chromeo...
chrome/browser/chromeos/idle_detector.h:17: IdleDetector(const base::Closure&
on_active_callback,
On 2014/02/13 23:22:01, Rahul Chaturvedi wrote:
> On 2014/02/13 19:51:53, bartfab wrote:
> > What is |on_active_callback| meant for? Nobody ever uses it.
> 
> This code is entirely reusable for the idle_logout dialog code in retail mode,
> provided we also have an on_active callback. I intend to use this class for
that
> code but wouldn't like to refactor this class again when I do.

As discussed offline, the retail mode idle logout dialog is obsolete and can be
removed very soon (yay!) rather than being refactored. Thus, my comments stand:
This class should be renamed into something that indicates it is a one-shot
implementation for demo mode only and the |on_active_callback| that nobody uses
should be removed.

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/demo_app/manifest.json (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/resourc...
chrome/browser/resources/chromeos/demo_app/manifest.json:1: {
On 2014/02/13 23:22:01, Rahul Chaturvedi wrote:
> On 2014/02/13 19:51:53, bartfab wrote:
> > I find it strange that we ship the manifest with the Chrome source code and
> the
> > app separately. Is there any reason that the manifest cannot reside inside
> > /usr/share/chromeos-assets/demo_app so that it gets updated automatically
> > together with the app?
> 
> We're loading this as a component app, not a regular app. If we switch to
> loading this just as a regular app, we can move this manifest out. I am
> discussing the pros/cons of that with a few folks here before deciding what to
> do with it in M35.

When loading the app, you specify the path to the manifest. Why would you be
unable to load it as a component app if the path pointed to somewhere inside
/usr/share/chromeos-assets/demo_app?

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/resourc...
chrome/browser/resources/chromeos/demo_app/manifest.json:2: //
chrome-extension://klimoghijjogocdbaikffefjfcfheiel
On 2014/02/13 23:22:01, Rahul Chaturvedi wrote:
> On 2014/02/13 19:51:53, bartfab wrote:
> > Comments are not allowed in JSON. This file is invalid.
> 
> Almost every component app manifest in the tree has that comment, so I am
pretty
> sure that the manifest parser handles comments fine.
> 

It's still invalid JSON :).

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (right):

https://codereview.chromium.org/156493004/diff/1050001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:51: const int64
kDerelectDetectionTimeoutSeconds = 8 * 60 * 60; // 8 hours.
On 2014/02/13 23:22:01, Rahul Chaturvedi wrote:
> On 2014/02/13 19:51:53, bartfab wrote:
> > Why is this an int64? int would have been entirely sufficient, even on
16-bit
> > machines.
> > 
> > Nit: 2 spaces before trailing comment.
> 
> To keep the type consistent with what base::Time::FromSeconds expects.
> I am probably going to change this to milliseconds anyway once I check in the
> tests since I don't really want us to have to wait 1s during tests.

1) C++ will implicitly up-cast an |int| to an |int64| when needed. There is no
need to have these constants be |int64|.
2) In tests, we do not want any waits, not even 1ms. For tests, you should test
the timeout to zero and at that point, it does not matter whether you specify
that as 0s or 0ms.