Add a basic demo mode browser test.

Add a basic demo mode browser test.
This CL adds a basic demo mode browser test that verifies that a machine with 0 derelict timeouts correctly brings up the demo app.
This CL also fixes a bunch of style issues pointed out in

R=bartfab@chromium.org, xiyuan@chromium.org
BUG=343574, 355134
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260255

[xiyuan, 2014-03-20 01:14:03]

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

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/idle...
chrome/browser/chromeos/idle_detector.h:10: #include "base/callback.h"
nit: callback_forward.h here and callback.h in cc

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

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:11: #include
"chrome/browser/chromeos/login/login_display_host.h"
nit: No need since login_display_host_impl.h is included below.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:36: demo_app_path_
=  new base::FilePath(kDefaultDemoAppPath);
nit: 1 space after '='

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:58: demo_app_path_
= new base::FilePath(path);
To be safe and avoid future leaks, we should delete previous |demo_app_path_| if
it is not NULL.

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

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:18: 
nit: nuke one empty line

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:56: }
nit: wrong indent

[xiyuan, 2014-03-20 04:59:54]

more nits.

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

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:5:
#include "apps/app_window_registry.h"
seems not used.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:9:
#include "chrome/browser/apps/app_browsertest_util.h"
is this needed?

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:12:
#include "chrome/browser/profiles/profile_manager.h"
no longer needed.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:16:
#include "extensions/common/extension.h"
seems not used.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:21:
char kDemoAppId[] = "klimoghijjogocdbaikffefjfcfheiel";
not used.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:23:
class DemoAppLauncherTest : public InProcessBrowserTest {
Let's still derive from ExtensionBrowserTest to be on the safe side.

[bartfab (slow), 2014-03-20 13:22:29]

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/app_...
File chrome/browser/chromeos/app_mode/kiosk_profile_loader.h (right):

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_profile_loader.h:37: bool force_guest,
Why is this being renamed from |ephemeral| to |guest|? It is not like you are
logging in as guest. You are still logging into a kiosk session, but using an
ephemeral mount.

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

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/idle...
chrome/browser/chromeos/idle_detector.h:10: #include "base/callback.h"
On 2014/03/20 01:14:03, xiyuan wrote:
> nit: callback_forward.h here and callback.h in cc

Actually, the full callback.h is required here because this file declares
base::Closure member variables. callback_forward.h is sufficient when you have
pointers or references.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/authenticator.h (right):

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/authenticator.h:61: // |force_guest| specifies
whether to force the session to use a guest mount.
No, the mount is an ephemeral mount. Guest happens to use it. As do public
sessions. As do regular users, if the appropriate policy is set.

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

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:11: #include
"chrome/browser/chromeos/login/login_display_host.h"
On 2014/03/20 01:14:03, xiyuan wrote:
> nit: No need since login_display_host_impl.h is included below.

But LoginDisplayHostImpl::default_host() returns a LoginDisplayHost, which the
code in this file does dereference and use. Hence, following IWYU,
login_display_host.h should be explicitly included.

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

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h:12: #include
"base/files/file_path.h"
Nit: A forward-declaration of FilePath would be sufficient.

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

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:15:
#include "content/public/test/test_utils.h"
Nit: Is this used?

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:23:
class DemoAppLauncherTest : public InProcessBrowserTest {
Nit: #include "chrome/test/base/in_process_browser_test.h"

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:28:
virtual ~DemoAppLauncherTest() {}
Nit: add blank line above.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:30:
virtual void SetUpCommandLine(CommandLine* cmd_line) OVERRIDE {
Nit 1: Avoid abbreviations: s/cmd_line/command_line/
Nit 2: #include "base/compiler_specific.h"

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:31:
cmd_line->AppendSwitch(chromeos::switches::kLoginManager);
Nit: Here and below: No need for the chromeos:: prefix.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:46:
base::FilePath GetTestDemoAppPath() {
Why is GetTestDemoAppPath() a separate method? These few lines could just be
part of SetUp().

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:46:
base::FilePath GetTestDemoAppPath() {
Nit: This should be a const method.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:48:
PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir);
Nit: PathService::Get() is normally wrapped in an ASSERT_TRUE(). You cannot do
it here because your method has a return value, but you can do the following
instead:

if (!PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir))
  FAIL();

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:53:
ExtensionTestMessageListener
Nit: You can do this with a temporary:

EXPECT_TRUE(ExtensionTestMessageListener("launchedDemoApp",
false).WaitUntilSatisfied());

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:55:
EXPECT_TRUE(launch_data_check_listener.WaitUntilSatisfied());
1: Overall, this is racy. What if the app manages to launch before the
ExtensionTestMessageListener has been instantiated? You should instantiate the
listener during setup.

2: Nit: #include "testing/gtest/include/gtest/gtest.h"

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:59:
DISALLOW_COPY_AND_ASSIGN(DemoAppLauncherTest);
Nit: #include "base/basictypes.h"

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:63:
VerifyDemoAppLaunch();
Nit: Move the body of your test into here. No need for a VerifyDemoAppLaunch()
method.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/parallel_authenticator.cc:349: const std::string&
app_user_id, bool force_guest) {
Nit: Put each argument on a separate line.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/parallel_authenticator.h (right):

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/parallel_authenticator.h:135: // Mounts an public
and notifies consumer on the success/failure. If
"Mounts an public" is missing a few words :).

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/parallel_authenticator.h:136: // |force_guest| is
true, instead we mount a guest mount (which is
"If [...], instead we [...], otherwise the mount [...]." is both grammatically
wrong (change of subject mid-sentence) and very hard to follow. Why not say
something like:

If |force_ephemeral| is true, the mount is always ephemeral. Otherwise, the
mount is ephemeral iff the |DeviceEphemeralUsersEnabled| policy is enabled.

[rkc, 2014-03-20 21:45:01]

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/app_...
File chrome/browser/chromeos/app_mode/kiosk_profile_loader.h (right):

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_profile_loader.h:37: bool force_guest,
On 2014/03/20 13:22:29, bartfab wrote:
> Why is this being renamed from |ephemeral| to |guest|? It is not like you are
> logging in as guest. You are still logging into a kiosk session, but using an
> ephemeral mount.

Logging in a Kiosk account with an ephemeral mount is very different from using
a guest mount. A guest mount follows a code path that is designed to be used
without an owner, the ephemeral mount code path isn't. Additionally, according
to Cryptohome, a guest user _is logging in, we just bait and switch in the
callback from the mount changing the user back to the demo user.

All of these reasons put together make it a good idea to make sure that the user
of this method knows that we aren't just loading a normal Kiosk Mount as
Ephemeral but rather a guest mount, hence the rename.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/authenticator.h (right):

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/authenticator.h:61: // |force_guest| specifies
whether to force the session to use a guest mount.
On 2014/03/20 13:22:29, bartfab wrote:
> No, the mount is an ephemeral mount. Guest happens to use it. As do public
> sessions. As do regular users, if the appropriate policy is set.

As explained in the other comment, this is not just an ephemeral mount. This is
specifically a guest mount; which is different from a Kiosk user mounted
ephemeral.

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

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:36: demo_app_path_
=  new base::FilePath(kDefaultDemoAppPath);
On 2014/03/20 01:14:03, xiyuan wrote:
> nit: 1 space after '='

Done.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:58: demo_app_path_
= new base::FilePath(path);
On 2014/03/20 01:14:03, xiyuan wrote:
> To be safe and avoid future leaks, we should delete previous |demo_app_path_|
if
> it is not NULL.

Done.

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

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h:12: #include
"base/files/file_path.h"
On 2014/03/20 13:22:29, bartfab wrote:
> Nit: A forward-declaration of FilePath would be sufficient.

Done.

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

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:5:
#include "apps/app_window_registry.h"
On 2014/03/20 04:59:54, xiyuan wrote:
> seems not used.

Done.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:9:
#include "chrome/browser/apps/app_browsertest_util.h"
On 2014/03/20 04:59:54, xiyuan wrote:
> is this needed?

Done.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:12:
#include "chrome/browser/profiles/profile_manager.h"
On 2014/03/20 04:59:54, xiyuan wrote:
> no longer needed.

Done.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:15:
#include "content/public/test/test_utils.h"
On 2014/03/20 13:22:29, bartfab wrote:
> Nit: Is this used?

Done.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:16:
#include "extensions/common/extension.h"
On 2014/03/20 04:59:54, xiyuan wrote:
> seems not used.

Done.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:18: 
On 2014/03/20 01:14:03, xiyuan wrote:
> nit: nuke one empty line

Done.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:21:
char kDemoAppId[] = "klimoghijjogocdbaikffefjfcfheiel";
On 2014/03/20 04:59:54, xiyuan wrote:
> not used.

Done.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:23:
class DemoAppLauncherTest : public InProcessBrowserTest {
On 2014/03/20 04:59:54, xiyuan wrote:
> Let's still derive from ExtensionBrowserTest to be on the safe side.

Done.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:23:
class DemoAppLauncherTest : public InProcessBrowserTest {
On 2014/03/20 13:22:29, bartfab wrote:
> Nit: #include "chrome/test/base/in_process_browser_test.h"

Deriving from ExtensionBrowserTest now, so not relevant anymore.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:28:
virtual ~DemoAppLauncherTest() {}
On 2014/03/20 13:22:29, bartfab wrote:
> Nit: add blank line above.

Done.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:30:
virtual void SetUpCommandLine(CommandLine* cmd_line) OVERRIDE {
On 2014/03/20 13:22:29, bartfab wrote:
> Nit 1: Avoid abbreviations: s/cmd_line/command_line/
> Nit 2: #include "base/compiler_specific.h"

Done.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:31:
cmd_line->AppendSwitch(chromeos::switches::kLoginManager);
On 2014/03/20 13:22:29, bartfab wrote:
> Nit: Here and below: No need for the chromeos:: prefix.

Done.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:46:
base::FilePath GetTestDemoAppPath() {
On 2014/03/20 13:22:29, bartfab wrote:
> Nit: This should be a const method.

Done.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:46:
base::FilePath GetTestDemoAppPath() {
On 2014/03/20 13:22:29, bartfab wrote:
> Why is GetTestDemoAppPath() a separate method? These few lines could just be
> part of SetUp().

Getting the path to the test demo app is an individual operation in itself,
which can later possibly be more complex. It doesn't really hurt to put this in
a separate method; IMHO it looks cleaner.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:48:
PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir);
On 2014/03/20 13:22:29, bartfab wrote:
> Nit: PathService::Get() is normally wrapped in an ASSERT_TRUE(). You cannot do
> it here because your method has a return value, but you can do the following
> instead:
> 
> if (!PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir))
>   FAIL();

ASSERT and FAIL have issues with returning a FilePath.
EXPECT_TRUE does the job.
Done.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:53:
ExtensionTestMessageListener
On 2014/03/20 13:22:29, bartfab wrote:
> Nit: You can do this with a temporary:
> 
> EXPECT_TRUE(ExtensionTestMessageListener("launchedDemoApp",
> false).WaitUntilSatisfied());

Done.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:55:
EXPECT_TRUE(launch_data_check_listener.WaitUntilSatisfied());
On 2014/03/20 13:22:29, bartfab wrote:
> 1: Overall, this is racy. What if the app manages to launch before the
> ExtensionTestMessageListener has been instantiated? You should instantiate the
> listener during setup.
> 
> 2: Nit: #include "testing/gtest/include/gtest/gtest.h"

1.) We don't actually run the message loop until we setup the listener - in
fact, WaitUntilSatisfied is what runs the loop. How would the app load?
2.) Done.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:56: }
On 2014/03/20 01:14:03, xiyuan wrote:
> nit: wrong indent

Done.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:59:
DISALLOW_COPY_AND_ASSIGN(DemoAppLauncherTest);
On 2014/03/20 13:22:29, bartfab wrote:
> Nit: #include "base/basictypes.h"

Done.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:63:
VerifyDemoAppLaunch();
On 2014/03/20 13:22:29, bartfab wrote:
> Nit: Move the body of your test into here. No need for a VerifyDemoAppLaunch()
> method.

I prefer this in a separate method. This will potentially be called from any
tests added here.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/parallel_authenticator.cc:349: const std::string&
app_user_id, bool force_guest) {
On 2014/03/20 13:22:29, bartfab wrote:
> Nit: Put each argument on a separate line.

Done.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/parallel_authenticator.h (right):

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/parallel_authenticator.h:135: // Mounts an public
and notifies consumer on the success/failure. If
On 2014/03/20 13:22:29, bartfab wrote:
> "Mounts an public" is missing a few words :).

Done.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/parallel_authenticator.h:136: // |force_guest| is
true, instead we mount a guest mount (which is
On 2014/03/20 13:22:29, bartfab wrote:
> "If [...], instead we [...], otherwise the mount [...]." is both grammatically
> wrong (change of subject mid-sentence) and very hard to follow. Why not say
> something like:
> 
> If |force_ephemeral| is true, the mount is always ephemeral. Otherwise, the
> mount is ephemeral iff the |DeviceEphemeralUsersEnabled| policy is enabled.
Changed the comment around to make it clearer.

[xiyuan, 2014-03-20 21:59:44]

lgtm

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

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:48:
PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir);
On 2014/03/20 21:45:01, Rahul Chaturvedi wrote:
> On 2014/03/20 13:22:29, bartfab wrote:
> > Nit: PathService::Get() is normally wrapped in an ASSERT_TRUE(). You cannot
do
> > it here because your method has a return value, but you can do the following
> > instead:
> > 
> > if (!PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir))
> >   FAIL();
> 
> ASSERT and FAIL have issues with returning a FilePath.
> EXPECT_TRUE does the job.
> Done.

We can also CHECK. Some tests do that when the enclosing function does not
return void.

https://codereview.chromium.org/205713002/diff/20001/chrome/test/data/chromeo...
File chrome/test/data/chromeos/demo_app/js/background.js (right):

https://codereview.chromium.org/205713002/diff/20001/chrome/test/data/chromeo...
chrome/test/data/chromeos/demo_app/js/background.js:8: console.log('Demo app
launched.');
nit: This might not be needed though since no one would read them. :)

[rkc, 2014-03-20 22:04:45]

https://codereview.chromium.org/205713002/diff/20001/chrome/test/data/chromeo...
File chrome/test/data/chromeos/demo_app/js/background.js (right):

https://codereview.chromium.org/205713002/diff/20001/chrome/test/data/chromeo...
chrome/test/data/chromeos/demo_app/js/background.js:8: console.log('Demo app
launched.');
On 2014/03/20 21:59:44, xiyuan wrote:
> nit: This might not be needed though since no one would read them. :)

Done.

[bartfab (slow), 2014-03-21 10:37:31]

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/app_...
File chrome/browser/chromeos/app_mode/kiosk_profile_loader.h (right):

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_profile_loader.h:37: bool force_guest,
On 2014/03/20 21:45:01, Rahul Chaturvedi wrote:
> On 2014/03/20 13:22:29, bartfab wrote:
> > Why is this being renamed from |ephemeral| to |guest|? It is not like you
are
> > logging in as guest. You are still logging into a kiosk session, but using
an
> > ephemeral mount.
> 
> Logging in a Kiosk account with an ephemeral mount is very different from
using
> a guest mount. A guest mount follows a code path that is designed to be used
> without an owner, the ephemeral mount code path isn't. Additionally, according
> to Cryptohome, a guest user _is logging in, we just bait and switch in the
> callback from the mount changing the user back to the demo user.
> 
> All of these reasons put together make it a good idea to make sure that the
user
> of this method knows that we aren't just loading a normal Kiosk Mount as
> Ephemeral but rather a guest mount, hence the rename.

Ugh. I had looked at ParallelAuthenticator::LoginAsKioskAccount(), saw that it
uses MountPublic() but missed the fact that it also uses MountGuest() sometimes.
This is a really awful hack. It should be fixed before any other follow-up fixes
are made. Guest is special on Chrome OS and it should be kept that way, instead
of mixing other features with it. Kiosk app sessions should use a different
mount command, which then takes an argument that tells cryptohomed whether to
mount as ephemeral or not.

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

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:48:
PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir);
On 2014/03/20 21:59:44, xiyuan wrote:
> On 2014/03/20 21:45:01, Rahul Chaturvedi wrote:
> > On 2014/03/20 13:22:29, bartfab wrote:
> > > Nit: PathService::Get() is normally wrapped in an ASSERT_TRUE(). You
cannot
> do
> > > it here because your method has a return value, but you can do the
following
> > > instead:
> > > 
> > > if (!PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir))
> > >   FAIL();
> > 
> > ASSERT and FAIL have issues with returning a FilePath.
> > EXPECT_TRUE does the job.
> > Done.
> 
> We can also CHECK. Some tests do that when the enclosing function does not
> return void.

I think CHECKs in tests are a bad idea. They crash the test while an EXPECT_TRUE
would allow the test to wind down properly, cleanly reporting a failure at the
end.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:55:
EXPECT_TRUE(launch_data_check_listener.WaitUntilSatisfied());
On 2014/03/20 21:45:01, Rahul Chaturvedi wrote:
> On 2014/03/20 13:22:29, bartfab wrote:
> > 1: Overall, this is racy. What if the app manages to launch before the
> > ExtensionTestMessageListener has been instantiated? You should instantiate
the
> > listener during setup.
> > 
> > 2: Nit: #include "testing/gtest/include/gtest/gtest.h"
> 
> 1.) We don't actually run the message loop until we setup the listener - in
> fact, WaitUntilSatisfied is what runs the loop. How would the app load?
> 2.) Done.

1.) InProcessBrowserTest spins the loop before we even get to the test body.
This is why setting up a listener in the test body is racy.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:63:
VerifyDemoAppLaunch();
On 2014/03/20 21:45:01, Rahul Chaturvedi wrote:
> On 2014/03/20 13:22:29, bartfab wrote:
> > Nit: Move the body of your test into here. No need for a
VerifyDemoAppLaunch()
> > method.
> 
> I prefer this in a separate method. This will potentially be called from any
> tests added here.

The problem is that the EXPECT* and ASSERT* macros print the line number when
things fail. If the macros are used in a helper method, any failures will point
at the helper, making it more difficult to figure out where in the main test
flow things went wrong. It's trivial here of course but the moment a test
happens to call VerifyDemoAppLaunch() twice, it will be impossible to tell which
of the calls failed. This is why putting EXPECT* and ASSERT* in helper methods
is a bad pattern that should be avoided in tests.

[xiyuan, 2014-03-21 16:38:16]

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/app_...
File chrome/browser/chromeos/app_mode/kiosk_profile_loader.h (right):

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_profile_loader.h:37: bool force_guest,
On 2014/03/21 10:37:32, bartfab wrote:
> On 2014/03/20 21:45:01, Rahul Chaturvedi wrote:
> > On 2014/03/20 13:22:29, bartfab wrote:
> > > Why is this being renamed from |ephemeral| to |guest|? It is not like you
> are
> > > logging in as guest. You are still logging into a kiosk session, but using
> an
> > > ephemeral mount.
> > 
> > Logging in a Kiosk account with an ephemeral mount is very different from
> using
> > a guest mount. A guest mount follows a code path that is designed to be used
> > without an owner, the ephemeral mount code path isn't. Additionally,
according
> > to Cryptohome, a guest user _is logging in, we just bait and switch in the
> > callback from the mount changing the user back to the demo user.
> > 
> > All of these reasons put together make it a good idea to make sure that the
> user
> > of this method knows that we aren't just loading a normal Kiosk Mount as
> > Ephemeral but rather a guest mount, hence the rename.
> 
> Ugh. I had looked at ParallelAuthenticator::LoginAsKioskAccount(), saw that it
> uses MountPublic() but missed the fact that it also uses MountGuest()
sometimes.
> This is a really awful hack. It should be fixed before any other follow-up
fixes
> are made. Guest is special on Chrome OS and it should be kept that way,
instead
> of mixing other features with it. Kiosk app sessions should use a different
> mount command, which then takes an argument that tells cryptohomed whether to
> mount as ephemeral or not.

From rkc@'s test, regular mount (i.e. Mount/MountPublic) does not work for demo
app because cryptohomed does not support doing regular mount before the machine
is owned. MountGuest is a workaround for that problem, given the time frame that
demo app is needed.

I am not sure what is the proper solution here though. We could add a call which
does MountGuest underhood but that does not make much difference.

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

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:55:
EXPECT_TRUE(launch_data_check_listener.WaitUntilSatisfied());
On 2014/03/21 10:37:32, bartfab wrote:
> On 2014/03/20 21:45:01, Rahul Chaturvedi wrote:
> > On 2014/03/20 13:22:29, bartfab wrote:
> > > 1: Overall, this is racy. What if the app manages to launch before the
> > > ExtensionTestMessageListener has been instantiated? You should instantiate
> the
> > > listener during setup.
> > > 
> > > 2: Nit: #include "testing/gtest/include/gtest/gtest.h"
> > 
> > 1.) We don't actually run the message loop until we setup the listener - in
> > fact, WaitUntilSatisfied is what runs the loop. How would the app load?
> > 2.) Done.
> 
> 1.) InProcessBrowserTest spins the loop before we even get to the test body.
> This is why setting up a listener in the test body is racy.

How about we make it a member of DemoAppLauncherTest to ensure it starts
listening before everything?

[rkc, 2014-03-21 23:31:18]

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/app_...
File chrome/browser/chromeos/app_mode/kiosk_profile_loader.h (right):

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_profile_loader.h:37: bool force_guest,
On 2014/03/21 10:37:32, bartfab wrote:
> On 2014/03/20 21:45:01, Rahul Chaturvedi wrote:
> > On 2014/03/20 13:22:29, bartfab wrote:
> > > Why is this being renamed from |ephemeral| to |guest|? It is not like you
> are
> > > logging in as guest. You are still logging into a kiosk session, but using
> an
> > > ephemeral mount.
> > 
> > Logging in a Kiosk account with an ephemeral mount is very different from
> using
> > a guest mount. A guest mount follows a code path that is designed to be used
> > without an owner, the ephemeral mount code path isn't. Additionally,
according
> > to Cryptohome, a guest user _is logging in, we just bait and switch in the
> > callback from the mount changing the user back to the demo user.
> > 
> > All of these reasons put together make it a good idea to make sure that the
> user
> > of this method knows that we aren't just loading a normal Kiosk Mount as
> > Ephemeral but rather a guest mount, hence the rename.
> 
> Ugh. I had looked at ParallelAuthenticator::LoginAsKioskAccount(), saw that it
> uses MountPublic() but missed the fact that it also uses MountGuest()
sometimes.
> This is a really awful hack. It should be fixed before any other follow-up
fixes
> are made. Guest is special on Chrome OS and it should be kept that way,
instead
> of mixing other features with it. Kiosk app sessions should use a different
> mount command, which then takes an argument that tells cryptohomed whether to
> mount as ephemeral or not.

What you described is what we used to do; but after speaking with drewry@, it
seems that the only way currently, or in the foreseeable future, for cryptohome
to be mounted without having an owner, is to use MountGuest. Once that changes,
I'd be happy to change the Chrome side code to match.

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

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:55:
EXPECT_TRUE(launch_data_check_listener.WaitUntilSatisfied());
On 2014/03/21 16:38:16, xiyuan wrote:
> On 2014/03/21 10:37:32, bartfab wrote:
> > On 2014/03/20 21:45:01, Rahul Chaturvedi wrote:
> > > On 2014/03/20 13:22:29, bartfab wrote:
> > > > 1: Overall, this is racy. What if the app manages to launch before the
> > > > ExtensionTestMessageListener has been instantiated? You should
instantiate
> > the
> > > > listener during setup.
> > > > 
> > > > 2: Nit: #include "testing/gtest/include/gtest/gtest.h"
> > > 
> > > 1.) We don't actually run the message loop until we setup the listener -
in
> > > fact, WaitUntilSatisfied is what runs the loop. How would the app load?
> > > 2.) Done.
> > 
> > 1.) InProcessBrowserTest spins the loop before we even get to the test body.
> > This is why setting up a listener in the test body is racy.
> 
> How about we make it a member of DemoAppLauncherTest to ensure it starts
> listening before everything?

Xiyuan and I discussed this offline. This test is unique in the way that we
don't really control when the test starts. It starts pretty much as soon as the
browser is launched. Hence controlling 'when' the app will launch is not really
possible. To fix the situation, I've changed the verification completely to
instead wait for the app window to come up. Using that as the verification
criteria, if the app has already launched, we're still fine since we can just
check if the app window exists already and know that our test passed, otherwise,
we simply wait for the window to show up.

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:63:
VerifyDemoAppLaunch();
On 2014/03/21 10:37:32, bartfab wrote:
> On 2014/03/20 21:45:01, Rahul Chaturvedi wrote:
> > On 2014/03/20 13:22:29, bartfab wrote:
> > > Nit: Move the body of your test into here. No need for a
> VerifyDemoAppLaunch()
> > > method.
> > 
> > I prefer this in a separate method. This will potentially be called from any
> > tests added here.
> 
> The problem is that the EXPECT* and ASSERT* macros print the line number when
> things fail. If the macros are used in a helper method, any failures will
point
> at the helper, making it more difficult to figure out where in the main test
> flow things went wrong. It's trivial here of course but the moment a test
> happens to call VerifyDemoAppLaunch() twice, it will be impossible to tell
which
> of the calls failed. This is why putting EXPECT* and ASSERT* in helper methods
> is a bad pattern that should be avoided in tests.

Changed this to have the EXPECT_ in the main test body instead.

[xiyuan, 2014-03-21 23:53:30]

Don't forget to upload latest patch.

[rkc, 2014-03-21 23:57:13]

On 2014/03/21 23:53:30, xiyuan wrote:
> Don't forget to upload latest patch.

Whoops, it was sitting on the confirmation line on git cl :)
Uploaded.

[xiyuan, 2014-03-22 04:02:42]

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

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_profile_loader.cc:165:
context.username_hash = context.username + "- demouserhashfakevalue";
nit: strip the space between "-" and the suffix.

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc
(right):

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:43:
base::Bind(&UserProfileWaiter::NotificationCallback,
Can we use UserManager::GetActiveUserProfile() after the wait instead of using
the callback?

If that does not work, how about wait on NOTIFICATION_SESSION_STARTED?

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:89:
virtual void SetUpOnMainThread() OVERRIDE {
Seems no need to override it.

[bartfab (slow), 2014-03-25 12:52:54]

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/app_...
File chrome/browser/chromeos/app_mode/kiosk_profile_loader.h (right):

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_profile_loader.h:37: bool force_guest,
On 2014/03/21 23:31:18, Rahul Chaturvedi wrote:
> On 2014/03/21 10:37:32, bartfab wrote:
> > On 2014/03/20 21:45:01, Rahul Chaturvedi wrote:
> > > On 2014/03/20 13:22:29, bartfab wrote:
> > > > Why is this being renamed from |ephemeral| to |guest|? It is not like
you
> > are
> > > > logging in as guest. You are still logging into a kiosk session, but
using
> > an
> > > > ephemeral mount.
> > > 
> > > Logging in a Kiosk account with an ephemeral mount is very different from
> > using
> > > a guest mount. A guest mount follows a code path that is designed to be
used
> > > without an owner, the ephemeral mount code path isn't. Additionally,
> according
> > > to Cryptohome, a guest user _is logging in, we just bait and switch in the
> > > callback from the mount changing the user back to the demo user.
> > > 
> > > All of these reasons put together make it a good idea to make sure that
the
> > user
> > > of this method knows that we aren't just loading a normal Kiosk Mount as
> > > Ephemeral but rather a guest mount, hence the rename.
> > 
> > Ugh. I had looked at ParallelAuthenticator::LoginAsKioskAccount(), saw that
it
> > uses MountPublic() but missed the fact that it also uses MountGuest()
> sometimes.
> > This is a really awful hack. It should be fixed before any other follow-up
> fixes
> > are made. Guest is special on Chrome OS and it should be kept that way,
> instead
> > of mixing other features with it. Kiosk app sessions should use a different
> > mount command, which then takes an argument that tells cryptohomed whether
to
> > mount as ephemeral or not.
> 
> What you described is what we used to do; but after speaking with drewry@, it
> seems that the only way currently, or in the foreseeable future, for
cryptohome
> to be mounted without having an owner, is to use MountGuest. Once that
changes,
> I'd be happy to change the Chrome side code to match.

In that case, |force_guest| is still the wrong term. It used to be that you
either forced ephemeral or got ephemeral/non-ephemeral depending on policy. Now
you switch between guest and ephemeral/non-ephemeral. So this argument does not
force anything, it switches from a regular mount to a guest mount. It should be
called something like |use_guest_mount| then (here and elsewhere throughout the
CL).

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

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_profile_loader.cc:165:
context.username_hash = context.username + "- demouserhashfakevalue";
On 2014/03/22 04:02:42, xiyuan wrote:
> nit: strip the space between "-" and the suffix.

1) Why not simply use "demouserhashfakevalue" as the hash, without any
concatenation? You only support a single user name here anyway.
2) Why is this a conditional? Can it ever happen that the hash *is* set already?

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

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/authenticator.h:61: // |force_guest| specifies
whether to force the session to use a guest mount.
Nit: Specify what happens when this is |false|.

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

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:59: delete
demo_app_path_;
Nit: No need for a conditional here. |delete| becomes a no-op if |demo_app_path|
happens to be NULL.

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc
(right):

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:5:
#include "apps/app_window.h"
Nit: Not used.

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:43:
base::Bind(&UserProfileWaiter::NotificationCallback,
Nit: #include "base/bind.h"

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:44:
base::Unretained(this))).Wait();
Nit: #include "base/bind_helpers.h"

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:64:
namespace chromeos {
Nit: Move this to the start of the file, right after the includes. The anonymous
namespace should also be part of "chromeos".

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:96:
return AppWindowWaiter(apps::AppWindowRegistry::Get(profile_),
Nit: Why cache the |profile_| in DemoAppLauncherTest if you could just call
UserProfileWaiter().Wait() on the fly here?

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:97:
kDemoAppId).Wait() != NULL;
Nit: AppWindowWaiter always returns a non-NULL value. And if something were to
go wrong, the EXPECT() inside AppWindowWaiter would trigger, making it
unnecessary to check here again.

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

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/parallel_authenticator.h:139: // force_guest is
false, a (non-guest) ephemeral cryptohome will be mounted.
This looks correct but a bit convoluted, in that it states that the mount
normally is non-ephemeral but then immediately contradicts that statement under
certain conditions. How about something along the lines of (with |force_guest|
replaced by |use_guest_mount|, as suggested elsewhere):

Mounts an ephemeral guest cryptohome if |use_guest_mount| is |true|. Otherwise,
mounts a public cryptohome, which will be ephemeral if the
|DeviceEphemeralUsersEnabled| policy is enabled and non-ephemeral otherwise.

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/test/app_window_waiter.cc (right):

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/test/app_window_waiter.cc:32:
EXPECT_TRUE(window_);
Nit: I think it would be better to let individual tests check for this
themselves. Putting gtest statements in helper classes is not a pattern I have
seen anywhere else and may surprise many test authors.

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/test/app_window_waiter.cc:43: running_ = false;
Nit: If you just did message_loop_runner_ = NULL here, you could do away with
the explicit |running_| variable because:

|running_| == |message_loop_runner_|

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/test/app_window_waiter.h (right):

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/test/app_window_waiter.h:32: //
AppWindowRegistry::Observer
Nit: Add a colon at the end.

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/test/app_window_waiter.h:40:
scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
1) New code should use base::RunLoop instead.
2) Nit: #include "base/memory/ref_counted.h"

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/test/app_window_waiter.h:44:
DISALLOW_COPY_AND_ASSIGN(AppWindowWaiter);
Nit: #include "base/basictypes.h"

https://codereview.chromium.org/205713002/diff/60001/chrome/test/data/chromeo...
File chrome/test/data/chromeos/demo_app/js/background.js (right):

https://codereview.chromium.org/205713002/diff/60001/chrome/test/data/chromeo...
chrome/test/data/chromeos/demo_app/js/background.js:8:
chrome.app.window.create('html/background.html', {'state': 'fullscreen'});
Nit: If this is a foreground window, why is the contents called background.html?

[rkc, 2014-03-26 21:30:40]

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/app_...
File chrome/browser/chromeos/app_mode/kiosk_profile_loader.h (right):

https://codereview.chromium.org/205713002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_profile_loader.h:37: bool force_guest,
On 2014/03/25 12:52:54, bartfab wrote:
> On 2014/03/21 23:31:18, Rahul Chaturvedi wrote:
> > On 2014/03/21 10:37:32, bartfab wrote:
> > > On 2014/03/20 21:45:01, Rahul Chaturvedi wrote:
> > > > On 2014/03/20 13:22:29, bartfab wrote:
> > > > > Why is this being renamed from |ephemeral| to |guest|? It is not like
> you
> > > are
> > > > > logging in as guest. You are still logging into a kiosk session, but
> using
> > > an
> > > > > ephemeral mount.
> > > > 
> > > > Logging in a Kiosk account with an ephemeral mount is very different
from
> > > using
> > > > a guest mount. A guest mount follows a code path that is designed to be
> used
> > > > without an owner, the ephemeral mount code path isn't. Additionally,
> > according
> > > > to Cryptohome, a guest user _is logging in, we just bait and switch in
the
> > > > callback from the mount changing the user back to the demo user.
> > > > 
> > > > All of these reasons put together make it a good idea to make sure that
> the
> > > user
> > > > of this method knows that we aren't just loading a normal Kiosk Mount as
> > > > Ephemeral but rather a guest mount, hence the rename.
> > > 
> > > Ugh. I had looked at ParallelAuthenticator::LoginAsKioskAccount(), saw
that
> it
> > > uses MountPublic() but missed the fact that it also uses MountGuest()
> > sometimes.
> > > This is a really awful hack. It should be fixed before any other follow-up
> > fixes
> > > are made. Guest is special on Chrome OS and it should be kept that way,
> > instead
> > > of mixing other features with it. Kiosk app sessions should use a
different
> > > mount command, which then takes an argument that tells cryptohomed whether
> to
> > > mount as ephemeral or not.
> > 
> > What you described is what we used to do; but after speaking with drewry@,
it
> > seems that the only way currently, or in the foreseeable future, for
> cryptohome
> > to be mounted without having an owner, is to use MountGuest. Once that
> changes,
> > I'd be happy to change the Chrome side code to match.
> 
> In that case, |force_guest| is still the wrong term. It used to be that you
> either forced ephemeral or got ephemeral/non-ephemeral depending on policy.
Now
> you switch between guest and ephemeral/non-ephemeral. So this argument does
not
> force anything, it switches from a regular mount to a guest mount. It should
be
> called something like |use_guest_mount| then (here and elsewhere throughout
the
> CL).

Done.

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

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_profile_loader.cc:165:
context.username_hash = context.username + "- demouserhashfakevalue";
On 2014/03/25 12:52:54, bartfab wrote:
> On 2014/03/22 04:02:42, xiyuan wrote:
> > nit: strip the space between "-" and the suffix.
> 
> 1) Why not simply use "demouserhashfakevalue" as the hash, without any
> concatenation? You only support a single user name here anyway.
> 2) Why is this a conditional? Can it ever happen that the hash *is* set
already?

So this wasn't really a good solution to go with. After reading the comments on
crbug.com/355134, it became apparent that using a fake hash is not the correct
thing to do. Instead I've changed the code to now actually fetch the real
username hash from Crypto home. This code has been removed.

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_profile_loader.cc:165:
context.username_hash = context.username + "- demouserhashfakevalue";
On 2014/03/22 04:02:42, xiyuan wrote:
> nit: strip the space between "-" and the suffix.

Code removed.

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

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/authenticator.h:61: // |force_guest| specifies
whether to force the session to use a guest mount.
On 2014/03/25 12:52:54, bartfab wrote:
> Nit: Specify what happens when this is |false|.

Done.

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

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:59: delete
demo_app_path_;
On 2014/03/25 12:52:54, bartfab wrote:
> Nit: No need for a conditional here. |delete| becomes a no-op if
|demo_app_path|
> happens to be NULL.

Done.

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc
(right):

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:5:
#include "apps/app_window.h"
On 2014/03/25 12:52:54, bartfab wrote:
> Nit: Not used.

Done.

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:43:
base::Bind(&UserProfileWaiter::NotificationCallback,
On 2014/03/25 12:52:54, bartfab wrote:
> Nit: #include "base/bind.h"

Done.

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:43:
base::Bind(&UserProfileWaiter::NotificationCallback,
On 2014/03/22 04:02:42, xiyuan wrote:
> Can we use UserManager::GetActiveUserProfile() after the wait instead of using
> the callback?
> 
> If that does not work, how about wait on NOTIFICATION_SESSION_STARTED?

base/bind not needed anymore. Changed this to no longer need a notification
callback.

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:44:
base::Unretained(this))).Wait();
On 2014/03/25 12:52:54, bartfab wrote:
> Nit: #include "base/bind_helpers.h"

Same as above. Code removed.

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:64:
namespace chromeos {
On 2014/03/25 12:52:54, bartfab wrote:
> Nit: Move this to the start of the file, right after the includes. The
anonymous
> namespace should also be part of "chromeos".

Done.

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:89:
virtual void SetUpOnMainThread() OVERRIDE {
On 2014/03/22 04:02:42, xiyuan wrote:
> Seems no need to override it.

Done.

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:96:
return AppWindowWaiter(apps::AppWindowRegistry::Get(profile_),
On 2014/03/25 12:52:54, bartfab wrote:
> Nit: Why cache the |profile_| in DemoAppLauncherTest if you could just call
> UserProfileWaiter().Wait() on the fly here?

Done.

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:97:
kDemoAppId).Wait() != NULL;
On 2014/03/25 12:52:54, bartfab wrote:
> Nit: AppWindowWaiter always returns a non-NULL value. And if something were to
> go wrong, the EXPECT() inside AppWindowWaiter would trigger, making it
> unnecessary to check here again.

Removed the EXPECT from AppWindowWaiter.
Done.

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

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/parallel_authenticator.h:139: // force_guest is
false, a (non-guest) ephemeral cryptohome will be mounted.
On 2014/03/25 12:52:54, bartfab wrote:
> This looks correct but a bit convoluted, in that it states that the mount
> normally is non-ephemeral but then immediately contradicts that statement
under
> certain conditions. How about something along the lines of (with |force_guest|
> replaced by |use_guest_mount|, as suggested elsewhere):
> 
> Mounts an ephemeral guest cryptohome if |use_guest_mount| is |true|.
Otherwise,
> mounts a public cryptohome, which will be ephemeral if the
> |DeviceEphemeralUsersEnabled| policy is enabled and non-ephemeral otherwise.

Done.

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/test/app_window_waiter.cc (right):

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/test/app_window_waiter.cc:32:
EXPECT_TRUE(window_);
On 2014/03/25 12:52:54, bartfab wrote:
> Nit: I think it would be better to let individual tests check for this
> themselves. Putting gtest statements in helper classes is not a pattern I have
> seen anywhere else and may surprise many test authors.

Done.

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/test/app_window_waiter.cc:43: running_ = false;
On 2014/03/25 12:52:54, bartfab wrote:
> Nit: If you just did message_loop_runner_ = NULL here, you could do away with
> the explicit |running_| variable because:
> 
> |running_| == |message_loop_runner_|
Using run_loop_->running() instead.

Done.

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/test/app_window_waiter.h (right):

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/test/app_window_waiter.h:32: //
AppWindowRegistry::Observer
On 2014/03/25 12:52:54, bartfab wrote:
> Nit: Add a colon at the end.

Done.

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/test/app_window_waiter.h:40:
scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
On 2014/03/25 12:52:54, bartfab wrote:
> 1) New code should use base::RunLoop instead.
> 2) Nit: #include "base/memory/ref_counted.h"

Done.

https://codereview.chromium.org/205713002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/test/app_window_waiter.h:44:
DISALLOW_COPY_AND_ASSIGN(AppWindowWaiter);
On 2014/03/25 12:52:54, bartfab wrote:
> Nit: #include "base/basictypes.h"

Done.

https://codereview.chromium.org/205713002/diff/60001/chrome/test/data/chromeo...
File chrome/test/data/chromeos/demo_app/js/background.js (right):

https://codereview.chromium.org/205713002/diff/60001/chrome/test/data/chromeo...
chrome/test/data/chromeos/demo_app/js/background.js:8:
chrome.app.window.create('html/background.html', {'state': 'fullscreen'});
On 2014/03/25 12:52:54, bartfab wrote:
> Nit: If this is a foreground window, why is the contents called
background.html?

Done.

[xiyuan, 2014-03-26 21:58:18]

lgtm

https://codereview.chromium.org/205713002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc
(right):

https://codereview.chromium.org/205713002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:36:
content::NotificationService::AllSources()).Wait();
nit: Wrap with enclosing {} since it is more than 1 line.

https://codereview.chromium.org/205713002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:66:
virtual void SetUpOnMainThread() OVERRIDE {
nit: no need to override this.

https://codereview.chromium.org/205713002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/test/app_window_waiter.h (right):

https://codereview.chromium.org/205713002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/test/app_window_waiter.h:19: class
MessageLoopRunner;
nit: This can be removed now.

[rkc, 2014-03-26 22:26:11]

https://codereview.chromium.org/205713002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc
(right):

https://codereview.chromium.org/205713002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:36:
content::NotificationService::AllSources()).Wait();
On 2014/03/26 21:58:19, xiyuan wrote:
> nit: Wrap with enclosing {} since it is more than 1 line.

Done.

https://codereview.chromium.org/205713002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:66:
virtual void SetUpOnMainThread() OVERRIDE {
On 2014/03/26 21:58:19, xiyuan wrote:
> nit: no need to override this.

Done.

https://codereview.chromium.org/205713002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/test/app_window_waiter.h (right):

https://codereview.chromium.org/205713002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/test/app_window_waiter.h:19: class
MessageLoopRunner;
On 2014/03/26 21:58:19, xiyuan wrote:
> nit: This can be removed now.

Done.

[bartfab (slow), 2014-03-27 14:13:28]

LGTM. Thanks for working on this. Great to see demo mode covered by a test now.

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc
(right):

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:67:
bool VerifyDemoAppLaunch() {
Nit: This does not access any |DemoAppLauncherTest| members. It can be moved to
a function in the anonymous namespace.

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:74:
base::FilePath GetTestDemoAppPath() const {
Nit: This does not access any |DemoAppLauncherTest| members. It can be moved to
a function in the anonymous namespace.

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
chrome/browser/chromeos/login/parallel_authenticator.cc:118:
cryptohome::AsyncMethodCaller::GetInstance()->AsyncMountGuest(
The AuthAttemptState constructor sets username_hash_obtained_ = true, meaning
that the user id hash is already known and there is no need to wait for it.
Since you want to retrieve the hash from cryptohome, you need to make sure that
ParallelAuthenticator waits for the result by invoking
attempt->UsernameHashRequested(); before making any cryptohome calls.

See line 85, where Mount() does exactly that.

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/test/app_window_waiter.cc (right):

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
chrome/browser/chromeos/login/test/app_window_waiter.cc:8: #include
"content/public/test/test_utils.h"
Nit: Not used.

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
chrome/browser/chromeos/login/test/app_window_waiter.cc:9: #include
"testing/gtest/include/gtest/gtest.h"
Nit: Not used.

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/test/app_window_waiter.h (right):

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
chrome/browser/chromeos/login/test/app_window_waiter.h:27: apps::AppWindow*
Wait();
Nit: RunLoop::Run() can only be called once per RunLoop lifetime. This implies
that your Wait() method can only be called once per AppWindowWaiter lifetime.
That should be documented.

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
chrome/browser/chromeos/login/test/app_window_waiter.h:30: virtual void
OnAppWindowAdded(apps::AppWindow* app_window) OVERRIDE;
Nit: #include "base/compiler_specific.h"

[rkc, 2014-03-27 21:43:10]

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc
(right):

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:67:
bool VerifyDemoAppLaunch() {
On 2014/03/27 14:13:29, bartfab wrote:
> Nit: This does not access any |DemoAppLauncherTest| members. It can be moved
to
> a function in the anonymous namespace.

Done.

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc:74:
base::FilePath GetTestDemoAppPath() const {
On 2014/03/27 14:13:29, bartfab wrote:
> Nit: This does not access any |DemoAppLauncherTest| members. It can be moved
to
> a function in the anonymous namespace.

Done.

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
chrome/browser/chromeos/login/parallel_authenticator.cc:118:
cryptohome::AsyncMethodCaller::GetInstance()->AsyncMountGuest(
On 2014/03/27 14:13:29, bartfab wrote:
> The AuthAttemptState constructor sets username_hash_obtained_ = true, meaning
> that the user id hash is already known and there is no need to wait for it.
> Since you want to retrieve the hash from cryptohome, you need to make sure
that
> ParallelAuthenticator waits for the result by invoking
> attempt->UsernameHashRequested(); before making any cryptohome calls.
> 
> See line 85, where Mount() does exactly that.

Done.

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/test/app_window_waiter.cc (right):

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
chrome/browser/chromeos/login/test/app_window_waiter.cc:8: #include
"content/public/test/test_utils.h"
On 2014/03/27 14:13:29, bartfab wrote:
> Nit: Not used.

Done.

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
chrome/browser/chromeos/login/test/app_window_waiter.cc:9: #include
"testing/gtest/include/gtest/gtest.h"
On 2014/03/27 14:13:29, bartfab wrote:
> Nit: Not used.

Done.

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/test/app_window_waiter.h (right):

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
chrome/browser/chromeos/login/test/app_window_waiter.h:27: apps::AppWindow*
Wait();
On 2014/03/27 14:13:29, bartfab wrote:
> Nit: RunLoop::Run() can only be called once per RunLoop lifetime. This implies
> that your Wait() method can only be called once per AppWindowWaiter lifetime.
> That should be documented.

Done.

https://codereview.chromium.org/205713002/diff/80002/chrome/browser/chromeos/...
chrome/browser/chromeos/login/test/app_window_waiter.h:30: virtual void
OnAppWindowAdded(apps::AppWindow* app_window) OVERRIDE;
On 2014/03/27 14:13:29, bartfab wrote:
> Nit: #include "base/compiler_specific.h"

Done.

[rkc, 2014-03-27 21:43:16]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-03-27 21:46:12]

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

[commit-bot: I haz the power, 2014-03-27 22:11:09]

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

[commit-bot: I haz the power, 2014-03-27 22:11:10]

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-03-27 22:19:41]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-03-27 22:22:02]

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

[rkc, 2014-03-28 20:16:58]

Committed patchset #8 manually as r260255 (presubmit successful).