Make the policy fetch for first time login blocking

Make the policy fetch for first time login blocking

The CL makes policy fetching for first time login blocking for all users, except the ones that are known to be non-enterprise users.

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

[kaliamoorthi, 2014-06-13 16:05:08]

This is a fix for 334584.

PTAL.

Bartosz says the new behavior could cause a regression for some users. Lets
discuss this Monday.

[bartfab (slow), 2014-06-16 09:37:27]

To clarify, my concerns are as follows:
* I see no problem with bumping the policy fetch timeout on first login from 10s
to 60s.

* I see no problem with expanding the blocking policy fetch to all enterprise
users.

* I do see a problem with removing the blocking policy fetch on subsequent
logins. According to Prabhu's own investiation, we currently do a blocking
policy fetch on every login if the user's domain matches the device's enrollment
domain. Losing that will mean that policies which do not have dynamic_refresh no
longer kick in on the first login after a policy change.

https://codereview.chromium.org/330843002/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/330843002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:150:
DCHECK(!BrowserPolicyConnector::IsNonEnterpriseUser(username));
Nit: Instead of a DCHECK here, better add a comment in line 135 clarifying that
we bail out there if the user is known to be a consumer. It took me a while to
spot the check there.

[Andrew T Wilson (Slow), 2014-06-16 12:34:58]

We need a test for this.

I'll follow up with an email thread with the various stakeholders to validate
the general approach.

https://codereview.chromium.org/330843002/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/330843002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:150:
DCHECK(!BrowserPolicyConnector::IsNonEnterpriseUser(username));
On 2014/06/16 09:37:27, bartfab wrote:
> Nit: Instead of a DCHECK here, better add a comment in line 135 clarifying
that
> we bail out there if the user is known to be a consumer. It took me a while to
> spot the check there.

+1 to this. I didn't understand this check either.

[kaliamoorthi, 2014-07-07 14:29:10]

PTAL

[kaliamoorthi, 2014-07-07 14:32:26]

Sinci sky@ is OOO adding jcivelli@ as a reviewer for InProcessBrowserTest
changes.

[Jay Civelli, 2014-07-07 16:28:40]

lgtm

[bartfab (slow), 2014-07-07 17:40:13]

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc
(right):

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:1:
// Copyright (c) 2013 The Chromium Authors. All rights reserved.
Nit: It's 2014 :).

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:42:
virtual void SetUp() OVERRIDE {
Nit: #include "base/compiler_specific.h"

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:43:
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
#include "testing/gtest/include/gtest/gtest.h"

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:54:
command_line->AppendSwitchASCII(policy::switches::kDeviceManagementUrl,
Nit: #include "base/command_line.h"

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:55:
test_server_->GetServiceURL().spec());
#include "url/gurl.h"

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:57:
SetupGaiaServerWithAccessTokens();
Why are these two calls in SetUpCommandLine()? They do not appear to have any
connection with the command line.

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:61:
FakeGaia::AccessTokenInfo token_info;
#include "google_apis/gaia/fake_gaia.h"

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:88:
CHECK(wizard_controller);
Nit: s/CHECK/ASSERT_TRUE/

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:89:
wizard_controller->SkipToLoginForTesting(chromeos::LoginScreenContext());
Nit: #include "chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h"

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:91:
content::WindowedNotificationObserver(
#include "content/public/test/test_utils.h"

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:96:
void Login(const std::string username, const std::string password) {
Nit 1: s/Login/LogIn/ (verb in the imperative form, not noun)
Nit 2: s/username/user_id/ ("username" is legacy; all new code should use
"user_id")
Nit 3: s/std::string/std::string&/

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:100:
chrome::NOTIFICATION_SESSION_STARTED,
#include "chrome/browser/chrome_notification_types.h"

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:101:
content::NotificationService::AllSources()).Wait();
#include "content/public/browser/notification_service.h"

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:105:
const char kEmptyPolicy[] =
Nit: Rename the constant. This policy is not empty at all.

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:109:
"      \"ShowHomeButton\": true,"
Nit: Is this necessary?

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:123:
std::string policy = base::StringPrintf(
Nit 1: const.
Nit 2: #include <string>

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:124:
kEmptyPolicy, dm_protocol::kChromeUserPolicyType, kAccountId);
#include "components/policy/core/common/cloud/cloud_policy_constants.h"

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:126:
int result =
Nit 1: const.
Nit 2: s/result/bytes_written/

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:127:
base::WriteFile(policy_file_path(), policy.data(), policy.size());
Nit: #include "base/file_util.h"

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:131:
base::FilePath policy_file_path() const {
Nit: #include "base/files/file_path.h"

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:135:
protected:
Nit: You are in a protected block already.

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:136:
scoped_ptr<LocalPolicyTestServer> test_server_;
Nit: #include "base/memory/scoped_ptr.h"

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:138:
base::ScopedTempDir temp_dir_;
Nit: #include "base/files/scoped_temp_dir.h"

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:141:
DISALLOW_COPY_AND_ASSIGN(UserCloudPolicyManagerTest);
Nit: #include "base/macros.h"

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:151:
BrowserList::GetInstance(chrome::HOST_DESKTOP_TYPE_ASH);
Nit: #include "chrome/browser/ui/host_desktop.h"

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:153:
Browser* browser = browser_list->get(0);
Nit: #include "chrome/browser/ui/browser.h"

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:158:
int expected_tab_count = static_cast<int>(arraysize(kStartupURLs));
Nit: const.

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:142:
DCHECK(!BrowserPolicyConnector::IsNonEnterpriseUser(username));
Nit: #include "components/policy/core/browser/browser_policy_connector.h"

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:149:
const bool wait_for_initial_policy = !is_browser_restart;
This changes the logic more than you intended - it makes the policy fetch
blocking even for subsequent logins of non-affiliated users.

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:151:
base::TimeDelta initial_policy_fetch_timeout =
Nit 1: const.
Nit 2: s/initial_policy_fetch_timeout/policy_fetch_timeout/

https://codereview.chromium.org/330843002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:152:
chromeos::UserManager::Get()->IsCurrentUserNew()
1: Nit: #include "chrome/browser/chromeos/login/users/user_manager.h"
2: The resulting code would still allow an enterprise user to evade policy:

* Enterprise user starts first login.
* After authenticating against GAIA, user immediately yanks the network cable.
* User reboots device.
* User logs in as existing user.

At this point, the |kInitialPolicyFetchTimeoutSeconds| timeout is in force and
the user can skip the policy fetch by keeping the network cable disconnected
until the timeout hits.

[kaliamoorthi, 2014-07-08 14:43:08]

PTAL

[kaliamoorthi, 2014-07-08 14:49:39]

Adding nkostylev@ for LoginUtilsTest changes.

[Nikita (slow), 2014-07-08 15:08:30]

https://codereview.chromium.org/330843002/diff/110001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/330843002/diff/110001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils_browsertest.cc:546:
EXPECT_FALSE(prepared_profile_);
Why profile will be NULL here?

[kaliamoorthi, 2014-07-08 15:44:34]

https://codereview.chromium.org/330843002/diff/110001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/330843002/diff/110001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils_browsertest.cc:546:
EXPECT_FALSE(prepared_profile_);
On 2014/07/08 15:08:30, Nikita Kostylev wrote:
> Why profile will be NULL here?

I have made the first login block on policy fetch for all users. So
PrepareProfile returns immediately without OnProfilePrepared being called. The
profile is set to a non NULL value by OnProfilePrepared, which will eventually
get called after blocking policy fetch completes. It the mean time it remains
null.

[Nikita (slow), 2014-07-08 15:49:18]

Please add comment in the test about that.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kaliamoorthi, 2014-07-08 16:37:17]

On 2014/07/08 15:49:18, Nikita Kostylev wrote:
> Please add comment in the test about that.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

PTAL

[bartfab (slow), 2014-07-08 17:40:33]

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils_browsertest.cc:543: // First login in
a unmanaged device would still block to verify if there is
Nit: Use indicative, not subjunctive.

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils_browsertest.cc:568:
PrepareProfile(kUsernameOtherDomain);
This is not necessarily a non-enterprise user. The difference between this and
the previous one is whether the device is enrolled or not. So the tests should
be named something like:

LoginBlocksForConsumerUser
LoginBlocksForUnaffiliatedEnterpriseUser

and then EnterpriseLoginBlocksForEnterpriseUser should be something like:

LoginBlocksForAffiliatedEnterpriseUser

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils_browsertest.cc:619:
EXPECT_FALSE(prepared_profile_);
This test actually verifies that the blocking policy fetch works. The
NormalLoginDoesBlock and EnterpriseLoginDoesBlockForNormalUser tests should do
the same.

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc
(right):

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:43:
const char kTestAuthLogInAccessToken[] = "fake-access-token";
Nit: This one should be "Login" - Here, it is a token for the "login" (noun). In
the method name that I flagged earlier, it was "log in" (verb).

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:180:
static_cast<const int>(arraysize(kStartupURLs));
Nit (nothing to fix, just an observation): The |const| should be unnecessary
here.

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:149:
const bool is_managed_user = affiliation == USER_AFFILIATION_MANAGED;
Nit: Better s/is_managed_user/is_affiliated_user/. A user whose domain does not
match the enrollment domain can still be managed. The user is simply not
affiliated.

[Joao da Silva, 2014-07-09 12:31:16]

The core change looks good to me.

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:144:
DCHECK(!BrowserPolicyConnector::IsNonEnterpriseUser(username));
This is guaranteed by the statement immediately before, so I don't see the value
of DCHECKing...

https://codereview.chromium.org/330843002/diff/130001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/330843002/diff/130001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:1017:
'browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc',
nit: order

[Andrew T Wilson (Slow), 2014-07-09 13:37:59]

drive-by nits. Deferring actual review to Joao/Bartosz.

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:153:
(chromeos::UserManager::Get()->IsCurrentUserNew() ? true
Nit: I think:

IsCurrentUserNew() || is_managed_user

Is easier to read than

IsCurrentUserNew() : true ? is_managed_user

https://codereview.chromium.org/330843002/diff/130001/chrome/test/base/in_pro...
File chrome/test/base/in_process_browser_test.h (right):

https://codereview.chromium.org/330843002/diff/130001/chrome/test/base/in_pro...
chrome/test/base/in_process_browser_test.h:205: // True if the about:blank
should be opened when the browser is launched.
nit: "the about:blank tab" or just "about:blank" - "the about:blank" is weird.

[kaliamoorthi, 2014-07-11 10:05:20]

PTAL

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils_browsertest.cc:543: // First login in
a unmanaged device would still block to verify if there is
On 2014/07/08 17:40:33, bartfab wrote:
> Nit: Use indicative, not subjunctive.

Removed the whole test

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils_browsertest.cc:568:
PrepareProfile(kUsernameOtherDomain);
On 2014/07/08 17:40:33, bartfab wrote:
> This is not necessarily a non-enterprise user. The difference between this and
> the previous one is whether the device is enrolled or not. So the tests should
> be named something like:
> 
> LoginBlocksForConsumerUser
> LoginBlocksForUnaffiliatedEnterpriseUser
> 
> and then EnterpriseLoginBlocksForEnterpriseUser should be something like:
> 
> LoginBlocksForAffiliatedEnterpriseUser

Removed these and folded them into the existing parametrized test

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils_browsertest.cc:619:
EXPECT_FALSE(prepared_profile_);
On 2014/07/08 17:40:33, bartfab wrote:
> This test actually verifies that the blocking policy fetch works. The
> NormalLoginDoesBlock and EnterpriseLoginDoesBlockForNormalUser tests should do
> the same.

Done.

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc
(right):

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:43:
const char kTestAuthLogInAccessToken[] = "fake-access-token";
On 2014/07/08 17:40:33, bartfab wrote:
> Nit: This one should be "Login" - Here, it is a token for the "login" (noun).
In
> the method name that I flagged earlier, it was "log in" (verb).

Done.

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc:180:
static_cast<const int>(arraysize(kStartupURLs));
On 2014/07/08 17:40:33, bartfab wrote:
> Nit (nothing to fix, just an observation): The |const| should be unnecessary
> here.

Removed it

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:144:
DCHECK(!BrowserPolicyConnector::IsNonEnterpriseUser(username));
On 2014/07/09 12:31:16, Joao da Silva wrote:
> This is guaranteed by the statement immediately before, so I don't see the
value
> of DCHECKing...

Done.

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:149:
const bool is_managed_user = affiliation == USER_AFFILIATION_MANAGED;
On 2014/07/08 17:40:33, bartfab wrote:
> Nit: Better s/is_managed_user/is_affiliated_user/. A user whose domain does
not
> match the enrollment domain can still be managed. The user is simply not
> affiliated.

Done.

https://codereview.chromium.org/330843002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:153:
(chromeos::UserManager::Get()->IsCurrentUserNew() ? true
On 2014/07/09 13:37:59, Andrew T Wilson wrote:
> Nit: I think:
> 
> IsCurrentUserNew() || is_managed_user
> 
> Is easier to read than
> 
> IsCurrentUserNew() : true ? is_managed_user
> 

Done.

https://codereview.chromium.org/330843002/diff/130001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/330843002/diff/130001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:1017:
'browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc',
On 2014/07/09 12:31:16, Joao da Silva wrote:
> nit: order

Done.

https://codereview.chromium.org/330843002/diff/130001/chrome/test/base/in_pro...
File chrome/test/base/in_process_browser_test.h (right):

https://codereview.chromium.org/330843002/diff/130001/chrome/test/base/in_pro...
chrome/test/base/in_process_browser_test.h:205: // True if the about:blank
should be opened when the browser is launched.
On 2014/07/09 13:37:59, Andrew T Wilson wrote:
> nit: "the about:blank tab" or just "about:blank" - "the about:blank" is weird.

Done.

[bartfab (slow), 2014-07-11 11:16:45]

Please add a TEST= line to the CL description.

LGTM

https://codereview.chromium.org/330843002/diff/150001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/330843002/diff/150001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils_browsertest.cc:586: // Login with a
user of the enterprise domain waits for policy.
Nit: Remove this comment. It is no longer applicable.

https://codereview.chromium.org/330843002/diff/150001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils_browsertest.cc:674: const
LoginUtilsBlockingLoginTestParam kTestData[] = {
Nit: s/kTestData/kBlockinLoginTestCases/ (or something similar - kTestData is
too generic)

[kaliamoorthi, 2014-07-11 11:27:57]

Hi Nikita,

I need an LGTM from you to land the CL.

https://codereview.chromium.org/330843002/diff/150001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/330843002/diff/150001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils_browsertest.cc:586: // Login with a
user of the enterprise domain waits for policy.
On 2014/07/11 11:16:45, bartfab wrote:
> Nit: Remove this comment. It is no longer applicable.

Done.

https://codereview.chromium.org/330843002/diff/150001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils_browsertest.cc:674: const
LoginUtilsBlockingLoginTestParam kTestData[] = {
On 2014/07/11 11:16:45, bartfab wrote:
> Nit: s/kTestData/kBlockinLoginTestCases/ (or something similar - kTestData is
> too generic)

Done.

[Nikita (slow), 2014-07-14 09:19:42]

chrome/browser/chromeos/login/login_utils_browsertest.cc lgtm

[kaliamoorthi, 2014-07-14 09:21:22]

The CQ bit was checked by kaliamoorthi@chromium.org

[commit-bot: I haz the power, 2014-07-14 09:21:35]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/kaliamoorthi@chromium.org/330843002/17...

[commit-bot: I haz the power, 2014-07-14 09:56:54]

Change committed as 282925