Do not display lock for hosted domains

Do not display lock for hosted domains

BUG=413685
R=noms@chromium.org
TEST=Sign in to a profile with an @gmail.com or @google.com account,
lock should be available. Sign in with another other domain (such as
chromium.org) and lock will not be available.

Committed: https://crrev.com/f3a2e5bda7e28493a9c013722cf9cc77c0bfc52b
Cr-Commit-Position: refs/heads/master@{#297839}

[Mike Lerman, 2014-09-12 19:16:18]

Hey Monica,

Here's the review for disabling lock for hosted domain users (googlers exempt).
Please take a look!

Mike

[noms (inactive), 2014-09-12 19:18:12]

Oh man, before I even start reviewing this: this needs some tests :)

[noms (inactive), 2014-09-12 19:33:26]

Some initial comments

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/gaia...
File chrome/browser/profiles/gaia_info_update_service.cc (right):

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/gaia...
chrome/browser/profiles/gaia_info_update_service.cc:142:
profile_->GetPrefs()->SetInteger(prefs::kProfileIsLockableVersion,
You should add a comment that this only allows non hosted domains, i.e. google
or gmail

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/gaia...
chrome/browser/profiles/gaia_info_update_service.cc:147:
profile_->GetPrefs()->SetBoolean(prefs::kProfileIsLockable, false);
This could be 
profile_->GetPrefs()->SetBoolean(prefs::kProfileIsLockable, 
    hosted_domain.empty() || hosted_domain == base::UTF8ToUTF16("google.com"));

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:798: if
(switches::IsNewProfileManagement())
This will get triggered on more platforms than desktop. Do we care about this?

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profiles_state.cc:143: return;
Maybe you can just DCHECK that IsNewProfileManagement() instead of returning

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profiles_state.cc:158: // If the --google-profile-info
flag isn't used, then the
Since you've renamed the function, please update the comment as well

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profiles_state.h (right):

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profiles_state.h:71: // version of the Gaia profile
picture amd other profile info.
s/amd/and :)

https://codereview.chromium.org/566933005/diff/1/chrome/browser/ui/cocoa/prof...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/566933005/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1727:
prefs::kProfileIsLockable)) {
I don't think we should display the button at all if it's unusable (we don't for
local profiles). Maybe the caller of createOptionsViewWithRect should just pass
displayLock:NO

https://codereview.chromium.org/566933005/diff/1/chrome/browser/ui/views/prof...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/566933005/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:1281:
lock_button_->SetState(views::Button::STATE_DISABLED);
Same comment as above.

[Mike Lerman, 2014-09-15 14:08:56]

Tests coming soon - stay tuned!

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/gaia...
File chrome/browser/profiles/gaia_info_update_service.cc (right):

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/gaia...
chrome/browser/profiles/gaia_info_update_service.cc:142:
profile_->GetPrefs()->SetInteger(prefs::kProfileIsLockableVersion,
On 2014/09/12 19:33:25, Monica Dinculescu wrote:
> You should add a comment that this only allows non hosted domains, i.e. google
> or gmail

Added the comment by the definition of the version const in profile state.

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/gaia...
chrome/browser/profiles/gaia_info_update_service.cc:147:
profile_->GetPrefs()->SetBoolean(prefs::kProfileIsLockable, false);
On 2014/09/12 19:33:25, Monica Dinculescu wrote:
> This could be 
> profile_->GetPrefs()->SetBoolean(prefs::kProfileIsLockable, 
>     hosted_domain.empty() || hosted_domain ==
base::UTF8ToUTF16("google.com"));

Ugh ternaries, so unreadable! Done :P

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:798: if
(switches::IsNewProfileManagement())
On 2014/09/12 19:33:25, Monica Dinculescu wrote:
> This will get triggered on more platforms than desktop. Do we care about this?

Excellent point. added OS ifdefs.

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profiles_state.cc:143: return;
On 2014/09/12 19:33:25, Monica Dinculescu wrote:
> Maybe you can just DCHECK that IsNewProfileManagement() instead of returning

fo sho

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profiles_state.cc:158: // If the --google-profile-info
flag isn't used, then the
On 2014/09/12 19:33:25, Monica Dinculescu wrote:
> Since you've renamed the function, please update the comment as well

Done.

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profiles_state.h (right):

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profiles_state.h:71: // version of the Gaia profile
picture amd other profile info.
On 2014/09/12 19:33:25, Monica Dinculescu wrote:
> s/amd/and :)

What if I just really don't like NVidia?

https://codereview.chromium.org/566933005/diff/1/chrome/browser/ui/cocoa/prof...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/566933005/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1727:
prefs::kProfileIsLockable)) {
On 2014/09/12 19:33:26, Monica Dinculescu wrote:
> I don't think we should display the button at all if it's unusable (we don't
for
> local profiles). Maybe the caller of createOptionsViewWithRect should just
pass
> displayLock:NO

Chatted on one bug or another (I can find it if you like) about this with Alan
and Travis - we opted for showing it disabled. If something is disabled by
policy the standard is to show it disabled.

https://codereview.chromium.org/566933005/diff/1/chrome/browser/ui/views/prof...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/566933005/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:1281:
lock_button_->SetState(views::Button::STATE_DISABLED);
On 2014/09/12 19:33:26, Monica Dinculescu wrote:
> Same comment as above.

Same response as above.

[Mike Lerman, 2014-09-15 18:07:38]

Here are the tests!

[Mike Lerman, 2014-09-15 18:09:02]

mlerman@chromium.org changed reviewers:
+ msw@chromium.org

[Mike Lerman, 2014-09-15 18:09:02]

Hi Mike,

Disabling lock again, this time with different logic :) Can you review the
c/b/ui files for me, please? Thanks!

[msw, 2014-09-15 18:16:57]

Please get Monica's approval, then I'll take a look.

[noms (inactive), 2014-09-15 18:52:41]

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/gaia...
File chrome/browser/profiles/gaia_info_update_service.cc (right):

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/gaia...
chrome/browser/profiles/gaia_info_update_service.cc:142:
profile_->GetPrefs()->SetInteger(prefs::kProfileIsLockableVersion,
Hmm, but the comment would be useful here as well. Like, why allow only
"google.com" otherwise?

On 2014/09/15 14:08:55, Mike Lerman wrote:
> On 2014/09/12 19:33:25, Monica Dinculescu wrote:
> > You should add a comment that this only allows non hosted domains, i.e.
google
> > or gmail
> 
> Added the comment by the definition of the version const in profile state.

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/gaia_info_update_service.cc (right):

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/gaia_info_update_service.cc:141: base::string16
hosted_domain = downloader->GetProfileHostedDomain();
nit: const?

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/gaia_info_update_service.cc:146: hosted_domain ==
base::UTF8ToUTF16("google.com"));
nit: please put brackets around the == part to make it less scary.

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/gaia_info_update_service_unittest.cc (right):

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/gaia_info_update_service_unittest.cc:221:
profile()->GetPrefs()->GetInteger(
nit: can this fit in one line if profile()->GetPrefs() is indented 4, and not
aligned with the bracket? (here and below)

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_downloader.h (right):

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_downloader.h:55: virtual base::string16
GetProfileHostedDomain() const;
nit: const?

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_downloader_unittest.cc (right):

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_downloader_unittest.cc:145: true);
Maybe add a test for when the hosted domain is empty?

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:800:
profiles::UpdateIsProfileLockEnabledIfNeeded(profile);
Hmm, this worries me a little because it looks like you will download the same
Gaia info twice. Once wherever it's currently being downloaded, and once every
time you start Chrome. Or will this only happen if the algorithm version
changes? I'd add a comment explaining (if this is true) that this doesn't really
do anything most of the time

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profiles_state.cc:143:
DCHECK(switches::IsGoogleProfileInfo());
Maybe you should remove the IsGoogleProfileInfo DCHECK and just call
UpdateGaiaProfileInfo() so that you don't duplicate code.

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm
(right):

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:469:
browser()->profile()->GetPrefs()->SetBoolean(
This test confused me a lot because the kEmail is a gmail.com address, and I
assumed that should be lockable. Either you add a comment that it's only the
preference that matters in determining whether the profile is lockable, or you
make kEmail (or kEmail2, whaveter) be something like batman@gotham.com email,
and add a comment that only gmail and google is lockable.

[Mike Lerman, 2014-09-15 19:51:42]

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/gaia...
File chrome/browser/profiles/gaia_info_update_service.cc (right):

https://codereview.chromium.org/566933005/diff/1/chrome/browser/profiles/gaia...
chrome/browser/profiles/gaia_info_update_service.cc:142:
profile_->GetPrefs()->SetInteger(prefs::kProfileIsLockableVersion,
On 2014/09/15 18:52:40, Monica Dinculescu wrote:
> Hmm, but the comment would be useful here as well. Like, why allow only
> "google.com" otherwise?
> 
> On 2014/09/15 14:08:55, Mike Lerman wrote:
> > On 2014/09/12 19:33:25, Monica Dinculescu wrote:
> > > You should add a comment that this only allows non hosted domains, i.e.
> google
> > > or gmail
> > 
> > Added the comment by the definition of the version const in profile state.
> 

Hmm I wrote some wording I hope is good.

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/gaia_info_update_service.cc (right):

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/gaia_info_update_service.cc:141: base::string16
hosted_domain = downloader->GetProfileHostedDomain();
On 2014/09/15 18:52:40, Monica Dinculescu wrote:
> nit: const?

Sure?

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/gaia_info_update_service.cc:146: hosted_domain ==
base::UTF8ToUTF16("google.com"));
On 2014/09/15 18:52:40, Monica Dinculescu wrote:
> nit: please put brackets around the == part to make it less scary. 

Ternaries are always scary.

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/gaia_info_update_service_unittest.cc (right):

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/gaia_info_update_service_unittest.cc:221:
profile()->GetPrefs()->GetInteger(
On 2014/09/15 18:52:40, Monica Dinculescu wrote:
> nit: can this fit in one line if profile()->GetPrefs() is indented 4, and not
> aligned with the bracket? (here and below)

It does... but is that legal?

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_downloader.h (right):

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_downloader.h:55: virtual base::string16
GetProfileHostedDomain() const;
On 2014/09/15 18:52:40, Monica Dinculescu wrote:
> nit: const?

Discussed offline. none of the others return const.

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_downloader_unittest.cc (right):

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_downloader_unittest.cc:145: true);
On 2014/09/15 18:52:40, Monica Dinculescu wrote:
> Maybe add a test for when the hosted domain is empty?

Adding in trim functionality so I can achieve this moderately nicely.

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:800:
profiles::UpdateIsProfileLockEnabledIfNeeded(profile);
On 2014/09/15 18:52:40, Monica Dinculescu wrote:
> Hmm, this worries me a little because it looks like you will download the same
> Gaia info twice. Once wherever it's currently being downloaded, and once every
> time you start Chrome. Or will this only happen if the algorithm version
> changes? I'd add a comment explaining (if this is true) that this doesn't
really
> do anything most of the time

Only happens if the algorithm version changes. Comment written.

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profiles_state.cc:143:
DCHECK(switches::IsGoogleProfileInfo());
On 2014/09/15 18:52:40, Monica Dinculescu wrote:
> Maybe you should remove the IsGoogleProfileInfo DCHECK and just call
> UpdateGaiaProfileInfo() so that you don't duplicate code.

Done.

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm
(right):

https://codereview.chromium.org/566933005/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:469:
browser()->profile()->GetPrefs()->SetBoolean(
On 2014/09/15 18:52:40, Monica Dinculescu wrote:
> This test confused me a lot because the kEmail is a http://gmail.com address,
and I
> assumed that should be lockable. Either you add a comment that it's only the
> preference that matters in determining whether the profile is lockable, or you
> make kEmail (or kEmail2, whaveter) be something like mailto:batman@gotham.com
email,
> and add a comment that only gmail and google is lockable.

Added the former comment, I don't want a reader of the unit test to think that
the UserNameOfProfileAtIndex directly influences lock-ability.

[noms (inactive), 2014-09-15 20:24:39]

https://codereview.chromium.org/566933005/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/gaia_info_update_service.cc (right):

https://codereview.chromium.org/566933005/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/gaia_info_update_service.cc:141: const base::string16&
hosted_domain = downloader->GetProfileHostedDomain();
Oh, you don't need the &. Just const base::string16. Unless I'm missing
something, you don't assign to it, right?

https://codereview.chromium.org/566933005/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/gaia_info_update_service_unittest.cc (right):

https://codereview.chromium.org/566933005/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/gaia_info_update_service_unittest.cc:182:
prefs::kProfileIsLockableVersion));
nit: can this also be made to fit on one line, like L221?

https://codereview.chromium.org/566933005/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/profile_downloader_unittest.cc (right):

https://codereview.chromium.org/566933005/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_downloader_unittest.cc:46: stream << " }"; 
DLOG(ERROR) << "stream";
I spy with my little eye a leftover debug comment :)

https://codereview.chromium.org/566933005/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_downloader_unittest.cc:85: std::string
trimmed_hosted_domain;
Why do you need to trim this? Your code in gaia_info_... checks
hosted_domain.empty() without trimming, so your test should do the same. Pass a
std::string() on L161 rather than a space, and compare it here like the other
comparisons in L81-83. 

For that matter: what will your code do if hosted_domain == " " . Can that even
happen?

[Mike Lerman, 2014-09-15 20:41:09]

https://codereview.chromium.org/566933005/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/gaia_info_update_service.cc (right):

https://codereview.chromium.org/566933005/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/gaia_info_update_service.cc:141: const base::string16&
hosted_domain = downloader->GetProfileHostedDomain();
On 2014/09/15 20:24:39, Monica Dinculescu wrote:
> Oh, you don't need the &. Just const base::string16. Unless I'm missing
> something, you don't assign to it, right?
> 

Done.

https://codereview.chromium.org/566933005/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/gaia_info_update_service_unittest.cc (right):

https://codereview.chromium.org/566933005/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/gaia_info_update_service_unittest.cc:182:
prefs::kProfileIsLockableVersion));
On 2014/09/15 20:24:39, Monica Dinculescu wrote:
> nit: can this also be made to fit on one line, like L221?

Ah, missed one.

https://codereview.chromium.org/566933005/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/profile_downloader_unittest.cc (right):

https://codereview.chromium.org/566933005/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_downloader_unittest.cc:46: stream << " }"; 
DLOG(ERROR) << "stream";
On 2014/09/15 20:24:39, Monica Dinculescu wrote:
> I spy with my little eye a leftover debug comment :)

Hehe you Found It!!

https://codereview.chromium.org/566933005/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_downloader_unittest.cc:85: std::string
trimmed_hosted_domain;
On 2014/09/15 20:24:39, Monica Dinculescu wrote:
> Why do you need to trim this? Your code in gaia_info_... checks
> hosted_domain.empty() without trimming, so your test should do the same. Pass
a
> std::string() on L161 rather than a space, and compare it here like the other
> comparisons in L81-83. 
> 
> For that matter: what will your code do if hosted_domain == " " . Can that
even
> happen?

Done.

[Mike Lerman, 2014-09-16 18:54:57]

Updated this CL with the PM-sanctioned decision that Lock will be hidden rather
than disabled for hosted domain users.

[Mike Lerman, 2014-09-17 19:08:01]

mlerman@chromium.org changed reviewers:
+ rogerta@chromium.org

[Mike Lerman, 2014-09-17 19:08:02]

Hi Roger,

Monica asked me to add you as a reviewer. Can you please review this CL as a
whole? Thanks!

Mike

[noms (inactive), 2014-09-17 19:12:06]

I think this is lgtm with nits for me. I'd get a review from Roger just to
double check the preferences code is alright.

https://codereview.chromium.org/566933005/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/566933005/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1183:
browser_->profile()->GetPrefs()->GetBoolean(
nit: can think this can all fit in one line, if you indent by 4?

https://codereview.chromium.org/566933005/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1729:
prefs::kProfileIsLockable)) {
nit: this should only be indented by 4

https://codereview.chromium.org/566933005/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/566933005/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:1282:
prefs::kProfileIsLockable)) {
nit: wrong indent

https://codereview.chromium.org/566933005/diff/140001/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/566933005/diff/140001/chrome/common/pref_name...
chrome/common/pref_names.cc:44: // A boolean to determine whether lock is
available to this profile.
nit: whether this profile can be locked

[Mike Lerman, 2014-09-17 19:45:37]

https://codereview.chromium.org/566933005/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/566933005/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1183:
browser_->profile()->GetPrefs()->GetBoolean(
On 2014/09/17 19:12:05, Monica Dinculescu wrote:
> nit: can think this can all fit in one line, if you indent by 4?

I wish, but I'm one character over.

https://codereview.chromium.org/566933005/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1729:
prefs::kProfileIsLockable)) {
On 2014/09/17 19:12:05, Monica Dinculescu wrote:
> nit: this should only be indented by 4

Done.

https://codereview.chromium.org/566933005/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/566933005/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:1282:
prefs::kProfileIsLockable)) {
On 2014/09/17 19:12:06, Monica Dinculescu wrote:
> nit: wrong indent

I should just start using git cl format.

https://codereview.chromium.org/566933005/diff/140001/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/566933005/diff/140001/chrome/common/pref_name...
chrome/common/pref_names.cc:44: // A boolean to determine whether lock is
available to this profile.
On 2014/09/17 19:12:06, Monica Dinculescu wrote:
> nit: whether this profile can be locked

Done.

[Roger Tawa OOO till Jul 10th, 2014-09-17 21:11:01]

https://codereview.chromium.org/566933005/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile_downloader.cc (right):

https://codereview.chromium.org/566933005/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_downloader.cc:292: GURL(kUserEntryURL),
net::URLFetcher::GET, this));
Please use gaia::GaiaOAuthClient to fetch user info instead of fetching
kUserEntryURL directly.  See the following CL, that had done this but was
reverted, as a guide:

https://codereview.chromium.org/257773002/

You should be able to use most of this CL's changes to profile_downloader.cc,
except the changes to the kXXXPath[] constants.

[Mike Lerman, 2014-09-18 16:00:29]

Your yak, sir.

https://codereview.chromium.org/566933005/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile_downloader.cc (right):

https://codereview.chromium.org/566933005/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_downloader.cc:292: GURL(kUserEntryURL),
net::URLFetcher::GET, this));
On 2014/09/17 21:11:00, Roger Tawa wrote:
> Please use gaia::GaiaOAuthClient to fetch user info instead of fetching
> kUserEntryURL directly.  See the following CL, that had done this but was
> reverted, as a guide:
> 
> https://codereview.chromium.org/257773002/
> 
> You should be able to use most of this CL's changes to profile_downloader.cc,
> except the changes to the kXXXPath[] constants.

Done.

[Roger Tawa OOO till Jul 10th, 2014-09-18 20:59:22]

lgtm

Thanks Mike!

[Mike Lerman, 2014-09-19 15:40:44]

Updated this CL. The preference no longer tracks whether lock is available, but
rather what the hosted_domain is. Added a new constant to differentiate between
it being unset and being set-but-empty. The preference is now reset upon
signout.

Monica and Roger, and you review please? Once you're good I'll send it to msw
for the UI components. Thanks.

[Roger Tawa OOO till Jul 10th, 2014-09-19 16:03:43]

lgtm with comments below

https://codereview.chromium.org/566933005/diff/200001/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/566933005/diff/200001/chrome/common/pref_name...
chrome/common/pref_names.cc:45: const char kProfileHostedDomain[] =
"profile.hosted_domain";
Seems like this pref should be in signin_pref_names.cc and the value something
like:

const char kGoogleServicesHostedDomain[] =
    "google.services.hosted_domain";

Probably move kNoHostedDomainFound to here too.

[noms (inactive), 2014-09-19 17:23:57]

lgtm with nits

https://codereview.chromium.org/566933005/diff/200001/chrome/browser/profiles...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/566933005/diff/200001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:148: // Null value for the
kProfileHostedDomain preference.
nit: maybe rephrase it to "Value that represents an empty value for the
kProfileHostedDomain preference" ? It's not actually null.

[Mike Lerman, 2014-09-19 18:51:41]

https://codereview.chromium.org/566933005/diff/200001/chrome/browser/profiles...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/566933005/diff/200001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:148: // Null value for the
kProfileHostedDomain preference.
On 2014/09/19 17:23:57, Monica Dinculescu wrote:
> nit: maybe rephrase it to "Value that represents an empty value for the
> kProfileHostedDomain preference" ? It's not actually null.

Done.

https://codereview.chromium.org/566933005/diff/200001/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/566933005/diff/200001/chrome/common/pref_name...
chrome/common/pref_names.cc:45: const char kProfileHostedDomain[] =
"profile.hosted_domain";
On 2014/09/19 16:03:43, Roger Tawa wrote:
> Seems like this pref should be in signin_pref_names.cc and the value something
> like:
> 
> const char kGoogleServicesHostedDomain[] =
>     "google.services.hosted_domain";
> 
> Probably move kNoHostedDomainFound to here too.

Done.

[Mike Lerman, 2014-09-19 19:56:59]

Hi Mike,

Other reviews have provided their feedback. Over to you now - thanks!

Mike

[msw, 2014-09-19 21:47:40]

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:798: #if defined(OS_WIN) ||
defined(OS_MACOSX) || defined(OS_LINUX)
This will include Chrome OS, which defines both OS_LINUX and OS_CHROMEOS, is
that your intent? Why limit platforms in this way at all? Shouldn't checking
IsNewProfileManagement suffice here (or is that feature available on some
platform where this particular bit of functionality is not)?

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/profiles...
File chrome/browser/profiles/profile_window.cc (right):

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:299: hosted_domain == "google.com";
You CL description describes "google.com" and "gmail.com" being treated
differently than other hosted domains. Why does this check seem to treat
"google.com" and kNoHostedDomainFound similarly, without "gmail.com"?

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1183:
profiles::IsLockAvailable(browser_->profile());
This seems to hide the lock button, but your CL description says that this
disables the button. Clarify the intent by updating the CL or its description.

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm
(right):

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:473: //
Three profiles means we should have one active card, one separator, one
I see this comment is copied from elsewhere, but I don't understand why the
three profiles matters in this case (you don't list "x inactive profiles..."). I
don't think duplicating this comment, TODO, and subviews check everywhere makes
sense, anyway; consider removing lines 473-479?

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:500:
StartProfileChooserController();
nit: add a blank line above, or remove the corresponding blank line 467.

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:505: //
Three profiles means we should have one active card, one separator, one
Ditto.

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:890:
switches::IsNewProfileManagement() && item.signed_in &&
nit: should IsNewProfileManagement() be part of profiles::IsLockAvailable?

https://codereview.chromium.org/566933005/diff/240001/components/signin/core/...
File components/signin/core/browser/signin_manager.cc (right):

https://codereview.chromium.org/566933005/diff/240001/components/signin/core/...
components/signin/core/browser/signin_manager.cc:210:
client_->GetPrefs()->ClearPref(prefs::kGoogleServicesHostedDomain);
Should gcm::FakeSigninManager::SignOut or anywhere else also do this?

https://codereview.chromium.org/566933005/diff/240001/components/signin/core/...
File components/signin/core/common/signin_pref_names.cc (right):

https://codereview.chromium.org/566933005/diff/240001/components/signin/core/...
components/signin/core/common/signin_pref_names.cc:17: // A string of profile's
hosted domain; empty if there is no HD.
nit: "The profile's hosted domain; empty if there is none."

[Mike Lerman, 2014-09-21 00:19:36]

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:798: #if defined(OS_WIN) ||
defined(OS_MACOSX) || defined(OS_LINUX)
On 2014/09/19 21:47:40, msw wrote:
> This will include Chrome OS, which defines both OS_LINUX and OS_CHROMEOS, is
> that your intent? Why limit platforms in this way at all? Shouldn't checking
> IsNewProfileManagement suffice here (or is that feature available on some
> platform where this particular bit of functionality is not)?

That is not the intent - I want to target Linux Desktop but not ChromeOS. Is
there a better #ifdef for that? Checking IsNewProfileManagement() will
(intentionally) return true on ChromeOS (and Android), but should not be
considered a key for this particular functionality.

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/profiles...
File chrome/browser/profiles/profile_window.cc (right):

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:299: hosted_domain == "google.com";
On 2014/09/19 21:47:40, msw wrote:
> You CL description describes "google.com" and "gmail.com" being treated
> differently than other hosted domains. Why does this check seem to treat
> "google.com" and kNoHostedDomainFound similarly, without "gmail.com"?

gmail.com is not considered a hosted domain. Hosted domains are any Google For
Business or Enterprise accounts, which includes google.com. If a user has a
gmail.com account, the GAIA call will return no hosted domain.

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1183:
profiles::IsLockAvailable(browser_->profile());
On 2014/09/19 21:47:40, msw wrote:
> This seems to hide the lock button, but your CL description says that this
> disables the button. Clarify the intent by updating the CL or its description.

CL updated - thanks.

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm
(right):

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:473: //
Three profiles means we should have one active card, one separator, one
On 2014/09/19 21:47:40, msw wrote:
> I see this comment is copied from elsewhere, but I don't understand why the
> three profiles matters in this case (you don't list "x inactive profiles...").
I
> don't think duplicating this comment, TODO, and subviews check everywhere
makes
> sense, anyway; consider removing lines 473-479?

I can agree with that - a test should test its own thing, and this layout
assertion is tested above.

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:500:
StartProfileChooserController();
On 2014/09/19 21:47:40, msw wrote:
> nit: add a blank line above, or remove the corresponding blank line 467.

Done.

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:505: //
Three profiles means we should have one active card, one separator, one
On 2014/09/19 21:47:40, msw wrote:
> Ditto.

Done.

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:890:
switches::IsNewProfileManagement() && item.signed_in &&
On 2014/09/19 21:47:40, msw wrote:
> nit: should IsNewProfileManagement() be part of profiles::IsLockAvailable?

Done, here and in the controller.

https://codereview.chromium.org/566933005/diff/240001/components/signin/core/...
File components/signin/core/browser/signin_manager.cc (right):

https://codereview.chromium.org/566933005/diff/240001/components/signin/core/...
components/signin/core/browser/signin_manager.cc:210:
client_->GetPrefs()->ClearPref(prefs::kGoogleServicesHostedDomain);
On 2014/09/19 21:47:40, msw wrote:
> Should gcm::FakeSigninManager::SignOut or anywhere else also do this?

This isn't important for unit tests, so doesn't need to be in the
cgm::FakeSigninManager or the signin::FakeSigninManager. The code which sets
this won't ever be triggered in unit tests, so it never needs to be cleared.

https://codereview.chromium.org/566933005/diff/240001/components/signin/core/...
File components/signin/core/common/signin_pref_names.cc (right):

https://codereview.chromium.org/566933005/diff/240001/components/signin/core/...
components/signin/core/common/signin_pref_names.cc:17: // A string of profile's
hosted domain; empty if there is no HD.
On 2014/09/19 21:47:40, msw wrote:
> nit: "The profile's hosted domain; empty if there is none."

Done with a modification that explains this better.

[Mike Lerman, 2014-09-22 13:18:56]

Also, made a small patch to handle a null from unit tests.

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:798: #if defined(OS_WIN) ||
defined(OS_MACOSX) || defined(OS_LINUX)
I'll flip the OS defs around so I check iOS, Android, CrOS.

[msw, 2014-09-22 19:58:09]

lgtm

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:798: #if defined(OS_WIN) ||
defined(OS_MACOSX) || defined(OS_LINUX)
On 2014/09/22 13:18:56, Mike Lerman wrote:
> I'll flip the OS defs around so I check iOS, Android, CrOS.

Acknowledged.

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/profiles...
File chrome/browser/profiles/profile_window.cc (right):

https://codereview.chromium.org/566933005/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:299: hosted_domain == "google.com";
On 2014/09/21 00:19:36, Mike Lerman wrote:
> On 2014/09/19 21:47:40, msw wrote:
> > You CL description describes "google.com" and "gmail.com" being treated
> > differently than other hosted domains. Why does this check seem to treat
> > "google.com" and kNoHostedDomainFound similarly, without "gmail.com"?
> 
> http://gmail.com is not considered a hosted domain. Hosted domains are any
Google For
> Business or Enterprise accounts, which includes http://google.com. If a user
has a
> http://gmail.com account, the GAIA call will return no hosted domain.

Acknowledged.

[Mike Lerman, 2014-09-29 20:43:10]

mlerman@chromium.org changed reviewers:
+ nkostylev@chromium.org

[Mike Lerman, 2014-09-29 20:43:11]

Hi Nikita,

Can you take a look at my change to the unit test? Thanks!

Mike

[Nikita (slow), 2014-10-02 12:35:39]

lgtm

[Mike Lerman, 2014-10-02 15:26:37]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-10-02 15:27:58]

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

[commit-bot: I haz the power, 2014-10-02 15:33:29]

Committed patchset #17 (id:320001) as b6460455ea0501f8361dfddce885a0aeac0a3d68

[commit-bot: I haz the power, 2014-10-02 15:34:06]

Patchset 17 (id:??) landed as
https://crrev.com/f3a2e5bda7e28493a9c013722cf9cc77c0bfc52b
Cr-Commit-Position: refs/heads/master@{#297839}