Make sure GetAuthenticatedAccountId() returns a canonicalized id.

Make sure GetAuthenticatedAccountId() returns a canonicalized id.

This CL is a prerequisite for:
https://codereview.chromium.org/473153002/

BUG=341408
TBR=benwells@chromium.org

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

[msarda, 2014-10-01 15:57:25]

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

[msarda, 2014-10-01 15:57:26]

https://codereview.chromium.org/617183003/diff/1/components/signin/core/brows...
File components/signin/core/browser/signin_manager_base.cc (right):

https://codereview.chromium.org/617183003/diff/1/components/signin/core/brows...
components/signin/core/browser/signin_manager_base.cc:60: ?
gaia::CanonicalizeEmail(gaia::SanitizeEmail(GetAuthenticatedUsername()))
I think this is potentially an expensive operation (a lot of string parsing). I
think it would be better to keep this in a field and update it at the same time
as authentication_username_. And that avoid changing the API as well.

[Roger Tawa OOO till Jul 10th, 2014-10-02 03:18:22]

Thanks Mihai.  Please take another look.

https://codereview.chromium.org/617183003/diff/1/components/signin/core/brows...
File components/signin/core/browser/signin_manager_base.cc (right):

https://codereview.chromium.org/617183003/diff/1/components/signin/core/brows...
components/signin/core/browser/signin_manager_base.cc:60: ?
gaia::CanonicalizeEmail(gaia::SanitizeEmail(GetAuthenticatedUsername()))
On 2014/10/01 15:57:26, msarda wrote:
> I think this is potentially an expensive operation (a lot of string parsing).
I
> think it would be better to keep this in a field and update it at the same
time
> as authentication_username_. And that avoid changing the API as well.

Added new member, but left api as is.

[msarda, 2014-10-02 08:46:42]

lgtm

[msarda, 2014-10-02 08:48:06]

LGTM with nits.

https://codereview.chromium.org/617183003/diff/20001/components/signin/core/b...
File components/signin/core/browser/signin_manager_base.h (right):

https://codereview.chromium.org/617183003/diff/20001/components/signin/core/b...
components/signin/core/browser/signin_manager_base.h:81: std::string
GetAuthenticatedUsername() const;
I think this should still return const std::string&

https://codereview.chromium.org/617183003/diff/20001/components/signin/core/b...
components/signin/core/browser/signin_manager_base.h:97: std::string
GetAuthenticatedAccountId() const;
I think this should still return const std::string&

[Roger Tawa OOO till Jul 10th, 2014-10-02 13:04:24]

On 2014/10/02 08:48:06, msarda wrote:
> LGTM with nits.
> 
>
https://codereview.chromium.org/617183003/diff/20001/components/signin/core/b...
> File components/signin/core/browser/signin_manager_base.h (right):
> 
>
https://codereview.chromium.org/617183003/diff/20001/components/signin/core/b...
> components/signin/core/browser/signin_manager_base.h:81: std::string
> GetAuthenticatedUsername() const;
> I think this should still return const std::string&
> 
>
https://codereview.chromium.org/617183003/diff/20001/components/signin/core/b...
> components/signin/core/browser/signin_manager_base.h:97: std::string
> GetAuthenticatedAccountId() const;
> I think this should still return const std::string&
 
Thanks Mihai. I'll change it back to const ref, and I also need to add a test
for the upgrade.case in mo2ts.

[Roger Tawa OOO till Jul 10th, 2014-10-02 13:04:26]

On 2014/10/02 08:48:06, msarda wrote:
> LGTM with nits.
> 
>
https://codereview.chromium.org/617183003/diff/20001/components/signin/core/b...
> File components/signin/core/browser/signin_manager_base.h (right):
> 
>
https://codereview.chromium.org/617183003/diff/20001/components/signin/core/b...
> components/signin/core/browser/signin_manager_base.h:81: std::string
> GetAuthenticatedUsername() const;
> I think this should still return const std::string&
> 
>
https://codereview.chromium.org/617183003/diff/20001/components/signin/core/b...
> components/signin/core/browser/signin_manager_base.h:97: std::string
> GetAuthenticatedAccountId() const;
> I think this should still return const std::string&
 
Thanks Mihai. I'll change it back to const ref, and I also need to add a test
for the upgrade.case in mo2ts.

[Roger Tawa OOO till Jul 10th, 2014-10-02 20:31:16]

rogerta@chromium.org changed reviewers:
+ atwilson@chromium.org, yfriedman@chromium.org

[Roger Tawa OOO till Jul 10th, 2014-10-02 20:31:18]

Drew: can you owner review:
  user_policy_signin_service_mobile.cc
  supervised_user_signin_manager_wrapper.cc

Yaron: can you owner review:
  bookmark_server_service.cc

Thanks.

https://codereview.chromium.org/617183003/diff/20001/components/signin/core/b...
File components/signin/core/browser/signin_manager_base.h (right):

https://codereview.chromium.org/617183003/diff/20001/components/signin/core/b...
components/signin/core/browser/signin_manager_base.h:81: std::string
GetAuthenticatedUsername() const;
On 2014/10/02 08:48:06, msarda wrote:
> I think this should still return const std::string&

Done.

https://codereview.chromium.org/617183003/diff/20001/components/signin/core/b...
components/signin/core/browser/signin_manager_base.h:97: std::string
GetAuthenticatedAccountId() const;
On 2014/10/02 08:48:06, msarda wrote:
> I think this should still return const std::string&

Done.

[msarda, 2014-10-03 07:51:01]

LGTM with one nit.

https://codereview.chromium.org/617183003/diff/140001/components/signin/core/...
File components/signin/core/browser/mutable_profile_oauth2_token_service.cc
(right):

https://codereview.chromium.org/617183003/diff/140001/components/signin/core/...
components/signin/core/browser/mutable_profile_oauth2_token_service.cc:305: 
Please add a DCHECK that account_id is canonicalized here.

[Yaron, 2014-10-03 16:36:56]

lgtm

[Roger Tawa OOO till Jul 10th, 2014-10-06 00:45:13]

Patchset #9 (id:160001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-10-06 00:51:37]

Patchset #9 (id:180001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-10-06 01:12:48]

Hi Tommy,

Can you please review AccountManagerHelper.java.

Hui: can you take a look at the whole CL?

Thanks,

https://codereview.chromium.org/617183003/diff/140001/components/signin/core/...
File components/signin/core/browser/mutable_profile_oauth2_token_service.cc
(right):

https://codereview.chromium.org/617183003/diff/140001/components/signin/core/...
components/signin/core/browser/mutable_profile_oauth2_token_service.cc:305: 
On 2014/10/03 07:51:01, msarda wrote:
> Please add a DCHECK that account_id is canonicalized here.

Done.  Wrote helper function and called in a few different places  too.

[Roger Tawa OOO till Jul 10th, 2014-10-06 01:29:48]

rogerta@chromium.org changed reviewers:
+ guohui@chromium.org, nyquist@chromium.org

[nyquist, 2014-10-06 22:13:06]

https://codereview.chromium.org/617183003/diff/260001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java
(right):

https://codereview.chromium.org/617183003/diff/260001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:132:
private String CanonicalizeName(String name) {
canonicalizeName, also, you might want to make this static, given it has only
static dependencies.

https://codereview.chromium.org/617183003/diff/260001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:133:
String[] parts = name.split(AT_SYMBOL);
This will create a regexp behind the scenes every time this function is called.

How about having:
private static final Pattern AT_SYMBOL = Pattern.compile("@");
And using it like this:
String[] parts = AT_SYMBOL.split(name);

https://codereview.chromium.org/617183003/diff/260001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:135:
return name;
add braces or put on previous line.

Java rule is: Either all goes on one line (both conditional and statement), or
you add braces. Same below.

Usually though, early returns like this is written on the same line, but more
logic-y changes typically are graduated to full braces-style code :-)
So I'd use single-line here, and braces below.

I don't have a strong opinion about which one you choose though.

https://codereview.chromium.org/617183003/diff/260001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:137: if
(parts[1].equalsIgnoreCase(GOOGLEMAIL_COM))
Use the constant first, like this: GOOGLEMAIL_COM.equalsIgnoreCase(parts[1]).

Also, what about other google domains? Is the same thing possible for those
domains? If so, how do we identify them?

https://codereview.chromium.org/617183003/diff/260001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:142:
StringBuilder builder = new StringBuilder(name.length());
Would this be simpler to read as:
return (parts[0] + "@" + parts[1]).toLowerCase(Locale.US);
?

You are not doing anything in a loop to construct the String anyway.

[Roger Tawa OOO till Jul 10th, 2014-10-07 01:38:03]

Patchset #13 (id:280001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-10-07 01:40:30]

Thanks Tommy.  Comments addressed.

https://codereview.chromium.org/617183003/diff/260001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java
(right):

https://codereview.chromium.org/617183003/diff/260001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:132:
private String CanonicalizeName(String name) {
On 2014/10/06 22:13:05, nyquist wrote:
> canonicalizeName, also, you might want to make this static, given it has only
> static dependencies.

Done.

https://codereview.chromium.org/617183003/diff/260001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:133:
String[] parts = name.split(AT_SYMBOL);
On 2014/10/06 22:13:05, nyquist wrote:
> This will create a regexp behind the scenes every time this function is
called.
> 
> How about having:
> private static final Pattern AT_SYMBOL = Pattern.compile("@");
> And using it like this:
> String[] parts = AT_SYMBOL.split(name);

Done.

https://codereview.chromium.org/617183003/diff/260001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:135:
return name;
On 2014/10/06 22:13:05, nyquist wrote:
> add braces or put on previous line.
> 
> Java rule is: Either all goes on one line (both conditional and statement), or
> you add braces. Same below.
> 
> Usually though, early returns like this is written on the same line, but more
> logic-y changes typically are graduated to full braces-style code :-)
> So I'd use single-line here, and braces below.
> 
> I don't have a strong opinion about which one you choose though.

Done.

https://codereview.chromium.org/617183003/diff/260001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:137: if
(parts[1].equalsIgnoreCase(GOOGLEMAIL_COM))
On 2014/10/06 22:13:05, nyquist wrote:
> Use the constant first, like this: GOOGLEMAIL_COM.equalsIgnoreCase(parts[1]).
> 
> Also, what about other google domains? Is the same thing possible for those
> domains? If so, how do we identify them?

Only need to consider googlemail.com.

https://codereview.chromium.org/617183003/diff/260001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:142:
StringBuilder builder = new StringBuilder(name.length());
On 2014/10/06 22:13:06, nyquist wrote:
> Would this be simpler to read as:
> return (parts[0] + "@" + parts[1]).toLowerCase(Locale.US);
> ?
> 
> You are not doing anything in a loop to construct the String anyway.

Done.

[guohui, 2014-10-07 04:06:03]

lgtm

https://codereview.chromium.org/617183003/diff/300001/chrome/browser/android/...
File chrome/browser/android/profiles/profile_downloader_android.cc (right):

https://codereview.chromium.org/617183003/diff/300001/chrome/browser/android/...
chrome/browser/android/profiles/profile_downloader_android.cc:166:
image_side_pixels);
it may be better to add a DCHECK for AccountTrackerService::GetMigrationState,
so that later you wont forget to update this code.

https://codereview.chromium.org/617183003/diff/300001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/617183003/diff/300001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:134: //
TODO(rogerta): the java code will need to pass in the gaia-id
i guess you mean javascript?

Also it may be better to add a DCHECK for
AccountTrackerService::GetMigrationState, so that later you wont forget to
update this code.

https://codereview.chromium.org/617183003/diff/300001/components/signin/core/...
File components/signin/core/browser/signin_manager_base.cc (right):

https://codereview.chromium.org/617183003/diff/300001/components/signin/core/...
components/signin/core/browser/signin_manager_base.cc:90: ? username
nits: i think the chromium code style is to have operator at the end of the last
line.

[nyquist, 2014-10-07 15:59:40]

https://codereview.chromium.org/617183003/diff/320001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java
(right):

https://codereview.chromium.org/617183003/diff/320001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:141:
parts[0] = parts[0].replace(".", "");
I wonder, would this hit the CharSequence version of .replace? Could you see if
you could do the single-char replacement method using single-quotes instead? I
think this one might hit the CharSequence version which basically compiles a new
regexp every time.

https://codereview.chromium.org/617183003/diff/320001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:149:
public Account getAccountFromName(String accountName) {
One quick question, were you going to add a test for this?
I believe adding something to
//sync/android/javatests/src/org/chromium/sync/notifier/signin/ would be
beneficial.

I can help you offline to get that set up if you haven't done it before.

[Andrew T Wilson (Slow), 2014-10-07 16:31:22]

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

[Andrew T Wilson (Slow), 2014-10-07 16:31:23]

LGTM with one question.

Also added joaodasilva in case he wants to take a look.

https://codereview.chromium.org/617183003/diff/320001/chrome/browser/policy/c...
File chrome/browser/policy/cloud/user_policy_signin_service.cc (right):

https://codereview.chromium.org/617183003/diff/320001/chrome/browser/policy/c...
chrome/browser/policy/cloud/user_policy_signin_service.cc:45:
DCHECK(!signin_manager->IsAuthenticated() ||
Why is this required - does RefreshTokenIsAvailable() return an error if we
aren't yet authenticated?

[Roger Tawa OOO till Jul 10th, 2014-10-07 19:37:34]

Thanks Drew, Tommy.

Tommy: looking into the test now, thanks for the offline tips.

https://codereview.chromium.org/617183003/diff/320001/chrome/browser/policy/c...
File chrome/browser/policy/cloud/user_policy_signin_service.cc (right):

https://codereview.chromium.org/617183003/diff/320001/chrome/browser/policy/c...
chrome/browser/policy/cloud/user_policy_signin_service.cc:45:
DCHECK(!signin_manager->IsAuthenticated() ||
On 2014/10/07 16:31:23, Andrew T Wilson wrote:
> Why is this required - does RefreshTokenIsAvailable() return an error if we
> aren't yet authenticated?

Previously, you could call RefreshTokenIsAvailable() with an empty string and it
would return false.  However, in this CL I added checks to all public api of the
token service to validate the account_id.  One of the validation checks is that
the account_id is not empty.

This required a few tweaks to callers to fix tests, see ps9 and ps11.

You could argue that this validation should not be done in
RefreshTokenIsAvailable(), only the other public api calls of the token service.
 I think its good as is, but if you feel strongly that RefreshTokenIsAvailable()
should not validate the account_id, I can remove it.

https://codereview.chromium.org/617183003/diff/320001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java
(right):

https://codereview.chromium.org/617183003/diff/320001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:141:
parts[0] = parts[0].replace(".", "");
On 2014/10/07 15:59:40, nyquist wrote:
> I wonder, would this hit the CharSequence version of .replace? Could you see
if
> you could do the single-char replacement method using single-quotes instead? I
> think this one might hit the CharSequence version which basically compiles a
new
> regexp every time.

I can't use single-char version because you can't specify "nothing" or "empty"
with newChar.

[nyquist, 2014-10-07 20:05:37]

https://codereview.chromium.org/617183003/diff/320001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java
(right):

https://codereview.chromium.org/617183003/diff/320001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:141:
parts[0] = parts[0].replace(".", "");
On 2014/10/07 19:37:34, Roger Tawa wrote:
> On 2014/10/07 15:59:40, nyquist wrote:
> > I wonder, would this hit the CharSequence version of .replace? Could you see
> if
> > you could do the single-char replacement method using single-quotes instead?
I
> > think this one might hit the CharSequence version which basically compiles a
> new
> > regexp every time.
> 
> I can't use single-char version because you can't specify "nothing" or "empty"
> with newChar.

In that case I'd precompile the pattern for the "." like you did with AT_SYMBOL.

[Roger Tawa OOO till Jul 10th, 2014-10-07 21:38:30]

Thanks Hui.

https://codereview.chromium.org/617183003/diff/300001/chrome/browser/android/...
File chrome/browser/android/profiles/profile_downloader_android.cc (right):

https://codereview.chromium.org/617183003/diff/300001/chrome/browser/android/...
chrome/browser/android/profiles/profile_downloader_android.cc:166:
image_side_pixels);
On 2014/10/07 04:06:03, guohui wrote:
> it may be better to add a DCHECK for AccountTrackerService::GetMigrationState,
> so that later you wont forget to update this code.

Done.

https://codereview.chromium.org/617183003/diff/300001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/617183003/diff/300001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:134: //
TODO(rogerta): the java code will need to pass in the gaia-id
On 2014/10/07 04:06:03, guohui wrote:
> i guess you mean javascript?
> 
> Also it may be better to add a DCHECK for
> AccountTrackerService::GetMigrationState, so that later you wont forget to
> update this code.

Done.

https://codereview.chromium.org/617183003/diff/300001/components/signin/core/...
File components/signin/core/browser/signin_manager_base.cc (right):

https://codereview.chromium.org/617183003/diff/300001/components/signin/core/...
components/signin/core/browser/signin_manager_base.cc:90: ? username
On 2014/10/07 04:06:03, guohui wrote:
> nits: i think the chromium code style is to have operator at the end of the
last
> line.

Done.

[Joao da Silva, 2014-10-08 13:19:17]

policy lgtm

I just have a couple of questions, see inline.

https://codereview.chromium.org/617183003/diff/340001/chrome/browser/android/...
File chrome/browser/android/profiles/profile_downloader_android.cc (right):

https://codereview.chromium.org/617183003/diff/340001/chrome/browser/android/...
chrome/browser/android/profiles/profile_downloader_android.cc:29: explicit
AccountInfoRetriever(Profile* profile,
explicit isnt required here

https://codereview.chromium.org/617183003/diff/340001/chrome/browser/android/...
chrome/browser/android/profiles/profile_downloader_android.cc:170: new
AccountInfoRetriever(profile, gaia::CanonicalizeEmail(email), email,
signin_manager_base.cc calls SanitizeEmail too, should we do that here too?

https://codereview.chromium.org/617183003/diff/340001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/user_cloud_policy_token_forwarder.cc
(right):

https://codereview.chromium.org/617183003/diff/340001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_token_forwarder.cc:83: if
(signin_manager_->IsAuthenticated() &&
This is ChromeOS code, so AFAICT this is always true. Am I missing something?

https://codereview.chromium.org/617183003/diff/340001/components/signin/core/...
File components/signin/core/browser/signin_manager_base.cc (right):

https://codereview.chromium.org/617183003/diff/340001/components/signin/core/...
components/signin/core/browser/signin_manager_base.cc:90: username :
gaia::CanonicalizeEmail(gaia::SanitizeEmail(username));
SanitizeEmail seems redundant here, given that it's a nop if the string already
contains an @

[Roger Tawa OOO till Jul 10th, 2014-10-08 19:47:29]

Thanks Joao.  Changes uploaded.

https://codereview.chromium.org/617183003/diff/320001/chrome/browser/policy/c...
File chrome/browser/policy/cloud/user_policy_signin_service.cc (right):

https://codereview.chromium.org/617183003/diff/320001/chrome/browser/policy/c...
chrome/browser/policy/cloud/user_policy_signin_service.cc:45:
DCHECK(!signin_manager->IsAuthenticated() ||
On 2014/10/07 19:37:34, Roger Tawa wrote:
> On 2014/10/07 16:31:23, Andrew T Wilson wrote:
> > Why is this required - does RefreshTokenIsAvailable() return an error if we
> > aren't yet authenticated?
> 
> Previously, you could call RefreshTokenIsAvailable() with an empty string and
it
> would return false.  However, in this CL I added checks to all public api of
the
> token service to validate the account_id.  One of the validation checks is
that
> the account_id is not empty.
> 
> This required a few tweaks to callers to fix tests, see ps9 and ps11.
> 
> You could argue that this validation should not be done in
> RefreshTokenIsAvailable(), only the other public api calls of the token
service.
>  I think its good as is, but if you feel strongly that
RefreshTokenIsAvailable()
> should not validate the account_id, I can remove it.

I've decided to remove the validity check only from  RefreshTokenIsAvailable(). 
It was causing a lot of failures because there is code that passes an empty
string to RefreshTokenIsAvailable() as a test to see if the profile is signed
in.  I'll keep the check for all O2TS calls that would mutate the token service.
 As a result, I undid changes to user_cloud_policy_token_forwarder.cc,
user_policy_signin_service.cc, and auth_service.cc.

https://codereview.chromium.org/617183003/diff/340001/chrome/browser/android/...
File chrome/browser/android/profiles/profile_downloader_android.cc (right):

https://codereview.chromium.org/617183003/diff/340001/chrome/browser/android/...
chrome/browser/android/profiles/profile_downloader_android.cc:29: explicit
AccountInfoRetriever(Profile* profile,
On 2014/10/08 13:19:17, Joao da Silva wrote:
> explicit isnt required here

Done.

https://codereview.chromium.org/617183003/diff/340001/chrome/browser/android/...
chrome/browser/android/profiles/profile_downloader_android.cc:170: new
AccountInfoRetriever(profile, gaia::CanonicalizeEmail(email), email,
On 2014/10/08 13:19:17, Joao da Silva wrote:
> signin_manager_base.cc calls SanitizeEmail too, should we do that here too?

Good point.  Done.

https://codereview.chromium.org/617183003/diff/340001/components/signin/core/...
File components/signin/core/browser/signin_manager_base.cc (right):

https://codereview.chromium.org/617183003/diff/340001/components/signin/core/...
components/signin/core/browser/signin_manager_base.cc:90: username :
gaia::CanonicalizeEmail(gaia::SanitizeEmail(username));
On 2014/10/08 13:19:17, Joao da Silva wrote:
> SanitizeEmail seems redundant here, given that it's a nop if the string
already
> contains an @

Done.

[Roger Tawa OOO till Jul 10th, 2014-10-08 21:34:53]

Patchset #19 (id:420001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-10-08 21:35:42]

Hi Tommy,

Added test, please see ps19.  Thanks.

[nyquist, 2014-10-09 15:52:01]

sync/android lgtm

Up to you whether you wanted to address the .replace(".", "") comment about
replacing with a constant Pattern.compile(".") -->
DOT.matcher(parts[0]).replaceAll("");

To be honest, unless performance is extremely crucial here, I'd keep the code as
is, since I would consider it at least as readable.

[Roger Tawa OOO till Jul 10th, 2014-10-09 16:01:24]

On 2014/10/09 15:52:01, nyquist wrote:
> sync/android lgtm
> 
> Up to you whether you wanted to address the .replace(".", "") comment about
> replacing with a constant Pattern.compile(".") -->
> DOT.matcher(parts[0]).replaceAll("");
> 
> To be honest, unless performance is extremely crucial here, I'd keep the code
as
> is, since I would consider it at least as readable.

I think I'll keep it as is.  This is not a performance hot spot.

[Roger Tawa OOO till Jul 10th, 2014-10-09 16:51:28]

rogerta@chromium.org changed reviewers:
+ benwells@chromium.org, fgorski@chromium.org

[Roger Tawa OOO till Jul 10th, 2014-10-09 16:51:30]

Ben: you can review people_provider.cc
Filip: can you review gcm/fake_signin_manager.cc

Thanks.

[fgorski, 2014-10-09 18:29:46]

gcm change lgtm

[Roger Tawa OOO till Jul 10th, 2014-10-09 21:14:48]

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2014-10-09 21:16:40]

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

[benwells, 2014-10-09 22:15:02]

lgtm

[commit-bot: I haz the power, 2014-10-09 22:27:55]

Committed patchset #21 (id:510001)

[commit-bot: I haz the power, 2014-10-09 22:29:06]

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