Switch to id-based affiliation determination

Fully switch to id-based determination of user affiliation status.
In this CL we do not compare user's e-mail with enterprise domain to understand if user is affiliated on the device or not. Instead we compare user-affiliation-ids and device-affiliation-ids. If there is an overlap - user is affiliated. This allows us to support cases when enterprise domain may be different from user e-mail (e.g. domain is "school.edu" and e-mail "someone@student.school.edu").
Because behavior was changed there was a need to fix the tests, that depend on way of affiliation determination.

BUG=491210
Committed: https://crrev.com/553ed81a99c6babadab6a0374570bf254983cde5
Cr-Commit-Position: refs/heads/master@{#360065}

[peletskyi, 2015-09-15 16:54:05]

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

[peletskyi, 2015-09-15 16:54:05]

Drew, PTAL

[peletskyi, 2015-09-15 16:54:08]

Drew, PTAL

[Andrew T Wilson (Slow), 2015-09-16 11:57:56]

Can you update the CL description to be more descriptive? "Fixed test" is kind
of a random comment to put in there that does not help anyone reading the
description understand what actually changed in your CL.

[peletskyi, 2015-09-16 13:14:33]

On 2015/09/16 11:57:56, Andrew T Wilson wrote:
> Can you update the CL description to be more descriptive? "Fixed test" is kind
> of a random comment to put in there that does not help anyone reading the
> description understand what actually changed in your CL.

Done

[Andrew T Wilson (Slow), 2015-09-16 15:02:06]

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_impl.cc
(right):

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_impl.cc:160:
// If the Profile belongs to a user who is not affiliated on the device,
nit: this is OK, but I think the old comment was more correct so consider
restoring it.

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/affiliation_test_helper.cc (right):

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/affiliation_test_helper.cc:128: ADD_FAILURE();
Should you add some information to this failure, like:

ADD_FAILURE() << user_context.GetUserID() << " was not added via PreLoginUser()"

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/affiliation_test_helper.h (right):

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/affiliation_test_helper.h:27: // todo peletskyi@:
Replace pointer with const reference and replace this
nit: wrong format for TODO. Also, this is a new API, so why not change it now?

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/affiliation_test_helper.h:34: // policies and
have device affiliation ID valid please use |test_helper|
How should the reader know what they should be doing here? Perhaps some high
level documentation explaining how to use these APIs in people's tests would be
valuable? Check out some of the examples in the .h files under base/ for a good
example on how to document API usage in header files.

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/affiliation_test_helper.h:54: void
PreLoginUser(const std::string& user_id);
Why isn't this part of LoginUser()? Why does it have to be called separately?
Maybe you can explain this in the documentation?

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/affiliation_test_helper.h:56: // Log in user with
|user_id|. User should be registered using RegisterUser().
What is RegisterUser()?

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/affiliation_test_helper.h:61: void
AppendCommandLineSwitches(base::CommandLine* command_line);
Maybe name this something like AppendCommandLineSwitchesForLoginManager() or
something clearer?

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_api.cc
(right):

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_api.cc:29:
return connector->IsEnterpriseManaged() && user->is_affiliated();
Why do we need to call IsEnterpriseManaged() here? Isn't calling is_affiliated()
sufficient?

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc
(right):

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc:159:
// function browser() to return non-NULL pointer. It is not our case and we
Not clear - why isn't browser() returning a valid pointer, when we clearly do
have a browser?

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc
(right):

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:198:
set_chromeos_user_ = false; // Command line should not be tweaked as if user
nit: move these comments so they start at the beginning of the line - having
them hang off the end is really only appropriate for one-line comments.

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:300:
chromeos::affiliation_test_helper::LoginUser(kEnterpriseUser);
What is this doing? And how do we know that LoggedInUsers contains
kEnterpriseUser?

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:373:
// function browser() to return non-NULL pointer. It is not our case and we
again, not clear how we have a browser here, but browser() is null.

[peletskyi, 2015-09-21 14:17:25]

PTAL

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_impl.cc
(right):

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_impl.cc:160:
// If the Profile belongs to a user who is not affiliated on the device,
On 2015/09/16 15:02:05, Andrew T Wilson wrote:
> nit: this is OK, but I think the old comment was more correct so consider
> restoring it.

I would say that the phrase "enrollment domain" in the old comment may confuse
because now we don't care about enrollment domain when determine affiliation.

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/affiliation_test_helper.cc (right):

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/affiliation_test_helper.cc:128: ADD_FAILURE();
On 2015/09/16 15:02:05, Andrew T Wilson wrote:
> Should you add some information to this failure, like:
> 
> ADD_FAILURE() << user_context.GetUserID() << " was not added via
PreLoginUser()"

Done.

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/affiliation_test_helper.h (right):

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/affiliation_test_helper.h:27: // todo peletskyi@:
Replace pointer with const reference and replace this
On 2015/09/16 15:02:06, Andrew T Wilson wrote:
> nit: wrong format for TODO. Also, this is a new API, so why not change it now?

Why not change it now? Because UserPolicyBuilder has method GetSigningKey() that
doesn't modify state but is not marked as const. If I add keyword "const" I will
need to modify many use cases of UserPolicyBuilder. So, better I'll do it in a
separate CL.
TODO - done

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/affiliation_test_helper.h:34: // policies and
have device affiliation ID valid please use |test_helper|
On 2015/09/16 15:02:05, Andrew T Wilson wrote:
> How should the reader know what they should be doing here? Perhaps some high
> level documentation explaining how to use these APIs in people's tests would
be
> valuable? Check out some of the examples in the .h files under base/ for a
good
> example on how to document API usage in header files.

Done.

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/affiliation_test_helper.h:54: void
PreLoginUser(const std::string& user_id);
On 2015/09/16 15:02:06, Andrew T Wilson wrote:
> Why isn't this part of LoginUser()? Why does it have to be called separately?
> Maybe you can explain this in the documentation?

It must be called in PRE_* functions.
Why we can't register user and login immediately without PRE_? The idea was
taken from this file: login_manager_test.h
In that file is a TODO mark before RegisterUser() to make it possible to login
in one step. Because the purpose of this CL is not devoted to solving this
problem, I decided to use a ready solution.

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/affiliation_test_helper.h:56: // Log in user with
|user_id|. User should be registered using RegisterUser().
On 2015/09/16 15:02:06, Andrew T Wilson wrote:
> What is RegisterUser()?

Done.

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/affiliation_test_helper.h:61: void
AppendCommandLineSwitches(base::CommandLine* command_line);
On 2015/09/16 15:02:06, Andrew T Wilson wrote:
> Maybe name this something like AppendCommandLineSwitchesForLoginManager() or
> something clearer?

Done.

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_api.cc
(right):

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_api.cc:29:
return connector->IsEnterpriseManaged() && user->is_affiliated();
On 2015/09/16 15:02:06, Andrew T Wilson wrote:
> Why do we need to call IsEnterpriseManaged() here? Isn't calling
is_affiliated()
> sufficient?

From my point of view connector->IsEnterpriseManaged() is not necessary here
even in old version. But my idea was to make as few changes as possible.
I think we need ask author for the review.
Done.

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc
(right):

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc:159:
// function browser() to return non-NULL pointer. It is not our case and we
On 2015/09/16 15:02:06, Andrew T Wilson wrote:
> Not clear - why isn't browser() returning a valid pointer, when we clearly do
> have a browser?

browser() is a function of InProcessBrowserTest and returns value of a private
field of type Browser*. This field is created in the constructor of
InProcessBrowserTest and is not modified any more. When we perform login we
obviously don't have browser. Thus browser() returns NULL.
I've modified the comment.

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc
(right):

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:198:
set_chromeos_user_ = false; // Command line should not be tweaked as if user
On 2015/09/16 15:02:06, Andrew T Wilson wrote:
> nit: move these comments so they start at the beginning of the line - having
> them hang off the end is really only appropriate for one-line comments.

Done.

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:300:
chromeos::affiliation_test_helper::LoginUser(kEnterpriseUser);
On 2015/09/16 15:02:06, Andrew T Wilson wrote:
> What is this doing? And how do we know that LoggedInUsers contains
> kEnterpriseUser?

I've added the comment.

https://codereview.chromium.org/1306183007/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:373:
// function browser() to return non-NULL pointer. It is not our case and we
On 2015/09/16 15:02:06, Andrew T Wilson wrote:
> again, not clear how we have a browser here, but browser() is null.

I've modified the comment.

[Thiemo Nagel, 2015-09-22 15:32:23]

tnagel@chromium.org changed reviewers:
+ tnagel@chromium.org

[Thiemo Nagel, 2015-09-22 15:32:24]

https://codereview.chromium.org/1306183007/diff/100001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_api.cc
(right):

https://codereview.chromium.org/1306183007/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_api.cc:16:

Please remove blank line.

[peletskyi, 2015-09-22 16:08:42]

https://codereview.chromium.org/1306183007/diff/100001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_api.cc
(right):

https://codereview.chromium.org/1306183007/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_api.cc:16:

On 2015/09/22 15:32:24, Thiemo Nagel wrote:
> Please remove blank line.

Done.

[peletskyi, 2015-09-23 09:41:23]

peletskyi@chromium.org changed reviewers:
+ pbond@chromium.org

[peletskyi, 2015-09-23 09:41:23]

pbond@chromium.org: Please review changes in 
chrome/browser/extensions/api/enterprise_device_attributes/

[Polina Bondarenko, 2015-09-23 14:36:04]

enterprise_device_attributes lgtm with small nits.

https://codereview.chromium.org/1306183007/diff/120001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc
(right):

https://codereview.chromium.org/1306183007/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc:4:

Do you use the PrefService anywhere? I don't see it.

https://codereview.chromium.org/1306183007/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc:167:
bool TestExtension(Browser* browser, const std::string& page_url) {
/s/<<"/<< "

https://codereview.chromium.org/1306183007/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc:188:

I'd add a comment that the both affiliated and non-affiliated cases are tested.

[peletskyi, 2015-09-25 12:15:58]

https://codereview.chromium.org/1306183007/diff/120001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc
(right):

https://codereview.chromium.org/1306183007/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc:4:

On 2015/09/23 14:36:04, Polina Bondarenko wrote:
> Do you use the PrefService anywhere? I don't see it.

local_state() call requires this include.

https://codereview.chromium.org/1306183007/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc:167:
bool TestExtension(Browser* browser, const std::string& page_url) {
On 2015/09/23 14:36:04, Polina Bondarenko wrote:
> /s/<<"/<< "

Done.

https://codereview.chromium.org/1306183007/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc:188:

On 2015/09/23 14:36:04, Polina Bondarenko wrote:
> I'd add a comment that the both affiliated and non-affiliated cases are
tested.

Done. Inserted the comment before INSTANTIATE_TEST_CASE_P.

[peletskyi, 2015-09-30 15:40:42]

On 2015/09/25 12:15:58, peletskyi wrote:
>
https://codereview.chromium.org/1306183007/diff/120001/chrome/browser/extensi...
> File
>
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc
> (right):
> 
>
https://codereview.chromium.org/1306183007/diff/120001/chrome/browser/extensi...
>
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc:4:
> 
> On 2015/09/23 14:36:04, Polina Bondarenko wrote:
> > Do you use the PrefService anywhere? I don't see it.
> 
> local_state() call requires this include.
> 
>
https://codereview.chromium.org/1306183007/diff/120001/chrome/browser/extensi...
>
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc:167:
> bool TestExtension(Browser* browser, const std::string& page_url) {
> On 2015/09/23 14:36:04, Polina Bondarenko wrote:
> > /s/<<"/<< "
> 
> Done.
> 
>
https://codereview.chromium.org/1306183007/diff/120001/chrome/browser/extensi...
>
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc:188:
> 
> On 2015/09/23 14:36:04, Polina Bondarenko wrote:
> > I'd add a comment that the both affiliated and non-affiliated cases are
> tested.
> 
> Done. Inserted the comment before INSTANTIATE_TEST_CASE_P.

Drew, friendly ping :)

[peletskyi, 2015-10-02 12:58:33]

On 2015/09/30 15:40:42, peletskyi wrote:
> On 2015/09/25 12:15:58, peletskyi wrote:
> >
>
https://codereview.chromium.org/1306183007/diff/120001/chrome/browser/extensi...
> > File
> >
>
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/1306183007/diff/120001/chrome/browser/extensi...
> >
>
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc:4:
> > 
> > On 2015/09/23 14:36:04, Polina Bondarenko wrote:
> > > Do you use the PrefService anywhere? I don't see it.
> > 
> > local_state() call requires this include.
> > 
> >
>
https://codereview.chromium.org/1306183007/diff/120001/chrome/browser/extensi...
> >
>
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc:167:
> > bool TestExtension(Browser* browser, const std::string& page_url) {
> > On 2015/09/23 14:36:04, Polina Bondarenko wrote:
> > > /s/<<"/<< "
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1306183007/diff/120001/chrome/browser/extensi...
> >
>
chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc:188:
> > 
> > On 2015/09/23 14:36:04, Polina Bondarenko wrote:
> > > I'd add a comment that the both affiliated and non-affiliated cases are
> > tested.
> > 
> > Done. Inserted the comment before INSTANTIATE_TEST_CASE_P.
> 
> Drew, friendly ping :)

Drew, ping ping ping! ;)

[Andrew T Wilson (Slow), 2015-10-09 12:35:46]

lgtm

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_impl_unittest.cc
(right):

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_impl_unittest.cc:239:
Profile* profile = LogInAndReturnProfile(kAffiliatedUserID1, true);
this is fine, but consider defining a constant:

const bool NOT_AFFILIATED = false;
const bool AFFILIATED = true;

Then the code is more readable:

LogInAndReturnProfile(kAffiliatedUserID1, AFFILIATED)

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/affiliation_test_helper.h (right):

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/affiliation_test_helper.h:28: // boilerplate in
other places.
Log a bug to track this and put bug number here.

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/affiliation_test_helper.h:47: // &test_helper,
fake_session_manager_client, device_affiliation_ids);
Properly indent your sample code.

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/affiliation_test_helper.h:49: // If it is used
together SetUserAffiliationIDs() (which is the most common
nit: "used together with"

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/affiliation_test_helper.h:51: //
SetUserAffiliationIDs() call.
Is this easy to enforce?

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/blocking_login_browsertest.cc (left):

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/blocking_login_browsertest.cc:247:
browser_policy_connector()->GetUserAffiliation(kUsername));
Why is it OK to remove this? Should we do a different test instead
(user->isAffiliated())?

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_affiliation_browsertest.cc (right):

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_affiliation_browsertest.cc:33:
UserAffiliationBrowserTest() { set_exit_when_last_browser_closes(false); }
Why is this flag needed?

[Thiemo Nagel, 2015-10-12 14:44:58]

tnagel@chromium.org changed reviewers:
- tnagel@chromium.org

[peletskyi, 2015-10-29 15:27:36]

peletskyi@chromium.org changed reviewers:
+ achuith@chromium.org, emaxx@chromium.org, erg@chromium.org

[peletskyi, 2015-10-29 15:27:37]

Hi everyone!
Please take a look to my changes.

erg@chromium.org: Please review changes in 
chrome/browser/profiles/profile_io_data.cc

emaxx@chromium.org: Please review changes in 
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc

achuith@chromium.org: Please review changes in 
chrome/browser/chromeos/login/saml/saml_browsertest.cc
chrome/browser/chromeos/login/session/user_session_manager.cc
chrome/browser/chromeos/proxy_config_service_impl.cc

Thanks a lot!

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_impl_unittest.cc
(right):

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_impl_unittest.cc:239:
Profile* profile = LogInAndReturnProfile(kAffiliatedUserID1, true);
On 2015/10/09 12:35:46, Andrew T Wilson (Slow) wrote:
> this is fine, but consider defining a constant:
> 
> const bool NOT_AFFILIATED = false;
> const bool AFFILIATED = true;
> 
> Then the code is more readable:
> 
> LogInAndReturnProfile(kAffiliatedUserID1, AFFILIATED)

Done.

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/affiliation_test_helper.h (right):

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/affiliation_test_helper.h:28: // boilerplate in
other places.
On 2015/10/09 12:35:46, Andrew T Wilson (Slow) wrote:
> Log a bug to track this and put bug number here.

Done.

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/affiliation_test_helper.h:47: // &test_helper,
fake_session_manager_client, device_affiliation_ids);
On 2015/10/09 12:35:46, Andrew T Wilson (Slow) wrote:
> Properly indent your sample code.

Done.

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/affiliation_test_helper.h:49: // If it is used
together SetUserAffiliationIDs() (which is the most common
On 2015/10/09 12:35:46, Andrew T Wilson (Slow) wrote:
> nit: "used together with"

Done.

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/affiliation_test_helper.h:51: //
SetUserAffiliationIDs() call.
On 2015/10/09 12:35:46, Andrew T Wilson (Slow) wrote:
> Is this easy to enforce?

One should just provide the same |fake_session_manager_client| into both
functions: SetDeviceAffiliationID() and SetUserAffiliationIDs().

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/blocking_login_browsertest.cc (left):

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/blocking_login_browsertest.cc:247:
browser_policy_connector()->GetUserAffiliation(kUsername));
On 2015/10/09 12:35:46, Andrew T Wilson (Slow) wrote:
> Why is it OK to remove this? Should we do a different test instead
> (user->isAffiliated())?

As far as I understood this line verifies domain, which was set in line 244 by
extracting it from kUsername. But currently we can not define domain judging by
user affiliation.

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_affiliation_browsertest.cc (right):

https://codereview.chromium.org/1306183007/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_affiliation_browsertest.cc:33:
UserAffiliationBrowserTest() { set_exit_when_last_browser_closes(false); }
On 2015/10/09 12:35:46, Andrew T Wilson (Slow) wrote:
> Why is this flag needed?

Because after login there is no open browser. Thus is the flag is not set the
test will quit immediately after login.

[Elliot Glaysher, 2015-10-29 17:12:50]

profiles change lgtm

[emaxx, 2015-10-30 16:10:17]

platform_keys LGTM with nits

https://codereview.chromium.org/1306183007/diff/180001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc
(right):

https://codereview.chromium.org/1306183007/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:162:
const char kGAIAHost[] = "accounts.google.com";
Can't we use GaiaUrls::GetInstance()->gaia_url().host() instead?

https://codereview.chromium.org/1306183007/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:387:
DCHECK(!page_url.empty()) <<"page_url cannot be empty";
Is this DCHECK message really useful?
And if yes, then there's a space missing after "<<".

[peletskyi, 2015-11-03 10:20:33]

https://codereview.chromium.org/1306183007/diff/180001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc
(right):

https://codereview.chromium.org/1306183007/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:162:
const char kGAIAHost[] = "accounts.google.com";
On 2015/10/30 16:10:17, emaxx wrote:
> Can't we use GaiaUrls::GetInstance()->gaia_url().host() instead?

Done.

https://codereview.chromium.org/1306183007/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:387:
DCHECK(!page_url.empty()) <<"page_url cannot be empty";
On 2015/10/30 16:10:17, emaxx wrote:
> Is this DCHECK message really useful?
> And if yes, then there's a space missing after "<<".

With empty page test will fail, this DCHECK will simplify developer to find an
error. Space done.

[peletskyi, 2015-11-09 12:54:45]

On 2015/11/03 10:20:38, peletskyi wrote:

achuithb, friendly ping

[peletskyi, 2015-11-13 16:35:38]

peletskyi@chromium.org changed reviewers:
- achuith@chromium.org

[peletskyi, 2015-11-13 16:37:12]

peletskyi@chromium.org changed reviewers:
+ oshima@chromium.org

[peletskyi, 2015-11-13 16:37:13]

Hi!
oshima@chromium.org: Please review changes in 
chrome/browser/chromeos/login/saml/saml_browsertest.cc
chrome/browser/chromeos/login/session/user_session_manager.cc
chrome/browser/chromeos/proxy_config_service_impl.cc
Thanks a lot :)

[oshima, 2015-11-13 19:53:05]

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_impl_unittest.cc
(right):

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_impl_unittest.cc:45:
const bool AFFILIATED = true;
optional suggestion. Please ignore if you prefer the way it is now.

I felt this is a bit odd. How about having two methods like:

LoginAndReturnAffiliatedProfile();
LoginAndReturnNonAffiliatedProfile();

?

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/affiliation_test_helper.h (right):

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/affiliation_test_helper.h:20: namespace chromeos
{
let's not add chromeos namespace under chrome/browser/chromeos. There are files
that has chromeos namespace for historical reasons, but it is recommended not to
have namespace for final project (chrome/) and "chromeos" namespace is for
src/chromeos now.

(I'll leave policy namespace for policy component owner though)

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/affiliation_test_helper.h:24: namespace
affiliation_test_helper {
this one is okay as this namespace is for a bag of global functions

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_affiliation_browsertest.cc (right):

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_affiliation_browsertest.cc:16: namespace
chromeos {
ditto

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_affiliation_browsertest.cc:16: namespace
chromeos {
ditto

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_affiliation_browsertest.cc:26: };
new line

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_affiliation_browsertest.cc:50:
scoped_ptr<SessionManagerClient>(fake_session_manager_client));
nit: make_scoped_ptr

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc
(right):

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:305:
}
nuke {}

[peletskyi, 2015-11-16 14:33:16]

Thanks for fast review :)
PTAL
BTW, linux_chromium_compile_dbg_32_ng seems to be a broken bot.

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_impl_unittest.cc
(right):

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_impl_unittest.cc:45:
const bool AFFILIATED = true;
On 2015/11/13 19:53:04, oshima wrote:
> optional suggestion. Please ignore if you prefer the way it is now.
> 
> I felt this is a bit odd. How about having two methods like:
> 
> LoginAndReturnAffiliatedProfile();
> LoginAndReturnNonAffiliatedProfile();
> 
> ?

Done.

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/affiliation_test_helper.h (right):

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/affiliation_test_helper.h:20: namespace chromeos
{
On 2015/11/13 19:53:04, oshima wrote:
> let's not add chromeos namespace under chrome/browser/chromeos. There are
files
> that has chromeos namespace for historical reasons, but it is recommended not
to
> have namespace for final project (chrome/) and "chromeos" namespace is for
> src/chromeos now.
> 
> (I'll leave policy namespace for policy component owner though)

Done.

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/affiliation_test_helper.h:24: namespace
affiliation_test_helper {
On 2015/11/13 19:53:04, oshima wrote:
> this one is okay as this namespace is for a bag of global functions

Done.

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_affiliation_browsertest.cc (right):

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_affiliation_browsertest.cc:16: namespace
chromeos {
On 2015/11/13 19:53:04, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_affiliation_browsertest.cc:16: namespace
chromeos {
On 2015/11/13 19:53:05, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_affiliation_browsertest.cc:26: };
On 2015/11/13 19:53:04, oshima wrote:
> new line

Done.

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_affiliation_browsertest.cc:50:
scoped_ptr<SessionManagerClient>(fake_session_manager_client));
On 2015/11/13 19:53:04, oshima wrote:
> nit: make_scoped_ptr

Done.

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc
(right):

https://codereview.chromium.org/1306183007/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:305:
}
On 2015/11/13 19:53:05, oshima wrote:
> nuke {}

Done.

[oshima, 2015-11-16 17:54:43]

lgtm with one nit

https://codereview.chromium.org/1306183007/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/affiliation_test_helper.h (right):

https://codereview.chromium.org/1306183007/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/affiliation_test_helper.h:10: #include
"components/policy/core/common/cloud/policy_builder.h"
nit: forward decl is enough?

[peletskyi, 2015-11-17 12:23:28]

The CQ bit was checked by peletskyi@chromium.org

[peletskyi, 2015-11-17 12:23:29]

The patchset sent to the CQ was uploaded after l-g-t-m from pbond@chromium.org,
atwilson@chromium.org, erg@chromium.org, emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/1306183007/#ps240001
(title: " ")

[commit-bot: I haz the power, 2015-11-17 12:23:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306183007/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306183007/240001

[peletskyi, 2015-11-17 12:24:37]

Thanks everyone for the review :)

https://codereview.chromium.org/1306183007/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/affiliation_test_helper.h (right):

https://codereview.chromium.org/1306183007/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/affiliation_test_helper.h:10: #include
"components/policy/core/common/cloud/policy_builder.h"
On 2015/11/16 17:54:42, oshima wrote:
> nit: forward decl is enough?

typedef redefinition with different types :(

[commit-bot: I haz the power, 2015-11-17 12:28:19]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-11-17 12:29:27]

Patchset 13 (id:??) landed as
https://crrev.com/553ed81a99c6babadab6a0374570bf254983cde5
Cr-Commit-Position: refs/heads/master@{#360065}