[MD Settings][People] Updates positions of username and sync status message

[MD Settings][People] Updates positions of username and sync status message

before: https://screenshot.googleplex.com/q9gMEb84CDy
after: https://screenshot.googleplex.com/oobv4yKECg5

This CL also updates sync status messages.

BUG=638464
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/9d08a7d8a72909a0c5bfe0ed5cb0b590876b7dcf
Cr-Commit-Position: refs/heads/master@{#425991}

[Moe, 2016-10-04 21:58:22]

Description was changed from

==========
[MD Settings][People] Updates positions of username and sync status message

BUG=638464
==========

to

==========
[MD Settings][People] Updates positions of username and sync status message

BUG=638464
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Moe, 2016-10-04 21:58:27]

The CQ bit was checked by mahmadi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 21:58:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 00:29:24]

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

[commit-bot: I haz the power, 2016-10-05 00:29:24]

Dry run: This issue passed the CQ dry run.

[Moe, 2016-10-05 13:52:47]

mahmadi@chromium.org changed reviewers:
+ dbeam@chromium.org, maxbogue@chromium.org

[Moe, 2016-10-05 13:52:47]

Hi Please review this CL.

dbeam@ webui
maxbogue@ c/b/sync

[Moe, 2016-10-05 13:54:20]

Description was changed from

==========
[MD Settings][People] Updates positions of username and sync status message

BUG=638464
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Settings][People] Updates positions of username and sync status message

This CL also updates some sync error messages.

BUG=638464
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Moe, 2016-10-05 13:57:15]

Description was changed from

==========
[MD Settings][People] Updates positions of username and sync status message

This CL also updates some sync error messages.

BUG=638464
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Settings][People] Updates positions of username and sync status message

before: https://screenshot.googleplex.com/q9gMEb84CDy
after: https://screenshot.googleplex.com/oobv4yKECg5

This CL also updates some sync error messages.

BUG=638464
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Moe, 2016-10-05 14:02:08]

Description was changed from

==========
[MD Settings][People] Updates positions of username and sync status message

before: https://screenshot.googleplex.com/q9gMEb84CDy
after: https://screenshot.googleplex.com/oobv4yKECg5

This CL also updates some sync error messages.

BUG=638464
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Settings][People] Updates positions of username and sync status message

before: https://screenshot.googleplex.com/q9gMEb84CDy
after: https://screenshot.googleplex.com/oobv4yKECg5

This CL also updates sync status messages.

BUG=638464
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[maxbogue, 2016-10-05 15:56:23]

https://codereview.chromium.org/2393643004/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/2393643004/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:20: *           
syncAccountInfo: (string|undefined),
Why syncAccountInfo and not just accountInfo?

https://codereview.chromium.org/2393643004/diff/1/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/2393643004/diff/1/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_android.cc:445:
l10n_util::GetStringUTF16(IDS_SYNC_ACCOUNT_SYNCING));
I believe that since this no longer needs to insert a value from native, this
string can be used directly from Java. Would you either remove
GetCurrentSignedInAccountText() entirely from ProfileSyncServiceAndroid and
ProfileSyncService.java, or if you're uncomfortable with Android just write a
TODO(maxbogue) for it? 

https://cs.chromium.org/search/?q=GetCurrentSignedInAccountText&sq=package:ch...

https://codereview.chromium.org/2393643004/diff/1/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util.h (right):

https://codereview.chromium.org/2393643004/diff/1/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.h:61: base::string16
GetAuthenticatedUsername(const SigninManagerBase* signin);
This function no longer has anything to do with sync, really. Is there somewhere
else it could live? Perhaps chrome/browser/signin/signin_ui_util.h?

[Moe, 2016-10-05 18:47:13]

The CQ bit was checked by mahmadi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 18:47:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Moe, 2016-10-05 18:52:44]

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

[Moe, 2016-10-05 18:52:45]

+rogerta@ c/b/signin

https://codereview.chromium.org/2393643004/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/2393643004/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:20: *           
syncAccountInfo: (string|undefined),
On 2016/10/05 15:56:23, maxbogue wrote:
> Why syncAccountInfo and not just accountInfo?

Done.

https://codereview.chromium.org/2393643004/diff/1/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/2393643004/diff/1/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_android.cc:445:
l10n_util::GetStringUTF16(IDS_SYNC_ACCOUNT_SYNCING));
On 2016/10/05 15:56:23, maxbogue wrote:
> I believe that since this no longer needs to insert a value from native, this
> string can be used directly from Java. Would you either remove
> GetCurrentSignedInAccountText() entirely from ProfileSyncServiceAndroid and
> ProfileSyncService.java, or if you're uncomfortable with Android just write a
> TODO(maxbogue) for it? 
> 
>
https://cs.chromium.org/search/?q=GetCurrentSignedInAccountText&sq=package:ch...

I don't have the setup to build and test android. I'll leave a TODO. Thanks!

https://codereview.chromium.org/2393643004/diff/1/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util.h (right):

https://codereview.chromium.org/2393643004/diff/1/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.h:61: base::string16
GetAuthenticatedUsername(const SigninManagerBase* signin);
On 2016/10/05 15:56:23, maxbogue wrote:
> This function no longer has anything to do with sync, really. Is there
somewhere
> else it could live? Perhaps chrome/browser/signin/signin_ui_util.h?

Done.

[maxbogue, 2016-10-05 18:58:37]

//chrome/browser/sync lgtm, thanks!

[Dan Beam, 2016-10-05 19:00:19]

dbeam@chromium.org changed reviewers:
+ tommycli@chromium.org
- dbeam@chromium.org

[Dan Beam, 2016-10-05 19:00:31]

+tommycli@ for first pass

[tommycli, 2016-10-05 19:37:38]

webui, html, and js lgtm with me except:

https://codereview.chromium.org/2393643004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/2393643004/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:20: *           
accountInfo: (string|undefined),
alphabetize?

https://codereview.chromium.org/2393643004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/sync_browser_proxy.js
(right):

https://codereview.chromium.org/2393643004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/sync_browser_proxy.js:26: *       
    username: (string|undefined)}}
alphabetize?

https://codereview.chromium.org/2393643004/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/people_handler.cc (right):

https://codereview.chromium.org/2393643004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/people_handler.cc:744:
sync_status->SetString("username",
can we make this variable name something else? Like maybe authenticatedUsername?

[commit-bot: I haz the power, 2016-10-05 19:48:45]

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

[commit-bot: I haz the power, 2016-10-05 19:48:45]

Dry run: This issue passed the CQ dry run.

[Moe, 2016-10-05 21:04:56]

The CQ bit was checked by mahmadi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 21:05:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Moe, 2016-10-05 21:08:24]

https://codereview.chromium.org/2393643004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/2393643004/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:20: *           
accountInfo: (string|undefined),
On 2016/10/05 19:37:37, tommycli wrote:
> alphabetize?

Done.

https://codereview.chromium.org/2393643004/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/people_handler.cc (right):

https://codereview.chromium.org/2393643004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/people_handler.cc:744:
sync_status->SetString("username",
On 2016/10/05 19:37:37, tommycli wrote:
> can we make this variable name something else? Like maybe
authenticatedUsername?

Done.

[commit-bot: I haz the power, 2016-10-05 22:16:46]

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

[commit-bot: I haz the power, 2016-10-05 22:16:47]

Dry run: This issue passed the CQ dry run.

[Roger Tawa OOO till Jul 10th, 2016-10-06 13:38:50]

lgtm

https://codereview.chromium.org/2393643004/diff/40001/chrome/browser/signin/s...
File chrome/browser/signin/signin_ui_util.cc (right):

https://codereview.chromium.org/2393643004/diff/40001/chrome/browser/signin/s...
chrome/browser/signin/signin_ui_util.cc:89: // Original email (containing dots)
is stored as "display email".
As of m48 (or around there), signin->GetAuthenticatedAccountInfo().email should
not be sanitized on any platform.  So line 84 should work everywhere.  However
is this #ifdef needed because cros has not yet finished the gaia id migration?

[Alexander Alekseev, 2016-10-06 18:25:58]

https://codereview.chromium.org/2393643004/diff/40001/chrome/browser/signin/s...
File chrome/browser/signin/signin_ui_util.cc (right):

https://codereview.chromium.org/2393643004/diff/40001/chrome/browser/signin/s...
chrome/browser/signin/signin_ui_util.cc:89: // Original email (containing dots)
is stored as "display email".
On 2016/10/06 13:38:50, Roger Tawa wrote:
> As of m48 (or around there), signin->GetAuthenticatedAccountInfo().email
should
> not be sanitized on any platform.  So line 84 should work everywhere.  However
> is this #ifdef needed because cros has not yet finished the gaia id migration?

Yes, cros has not finished migration.
It is not safe to remove this block.

[Moe, 2016-10-07 13:25:18]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2016-10-07 13:25:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
maxbogue@chromium.org, tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/2393643004/#ps40001
(title: "Addressed comments")

[commit-bot: I haz the power, 2016-10-07 13:25:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 13:34:00]

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

[commit-bot: I haz the power, 2016-10-07 13:34:01]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Moe, 2016-10-07 19:10:27]

mahmadi@chromium.org changed reviewers:
+ dbeam@chromium.org

[Moe, 2016-10-07 19:10:28]

Dan, please owner's lgtm for webui and options. Thank you.

[Dan Beam, 2016-10-13 05:06:20]

lgtm

[Moe, 2016-10-18 14:22:14]

The CQ bit was checked by mahmadi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 14:22:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 16:19:16]

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

[commit-bot: I haz the power, 2016-10-18 16:19:19]

Dry run: This issue passed the CQ dry run.

[Moe, 2016-10-18 16:51:19]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2016-10-18 16:51:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
maxbogue@chromium.org, rogerta@chromium.org, dbeam@chromium.org,
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/2393643004/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-18 16:51:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 16:56:38]

Description was changed from

==========
[MD Settings][People] Updates positions of username and sync status message

before: https://screenshot.googleplex.com/q9gMEb84CDy
after: https://screenshot.googleplex.com/oobv4yKECg5

This CL also updates sync status messages.

BUG=638464
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Settings][People] Updates positions of username and sync status message

before: https://screenshot.googleplex.com/q9gMEb84CDy
after: https://screenshot.googleplex.com/oobv4yKECg5

This CL also updates sync status messages.

BUG=638464
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-10-18 16:56:40]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-18 16:59:26]

Description was changed from

==========
[MD Settings][People] Updates positions of username and sync status message

before: https://screenshot.googleplex.com/q9gMEb84CDy
after: https://screenshot.googleplex.com/oobv4yKECg5

This CL also updates sync status messages.

BUG=638464
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Settings][People] Updates positions of username and sync status message

before: https://screenshot.googleplex.com/q9gMEb84CDy
after: https://screenshot.googleplex.com/oobv4yKECg5

This CL also updates sync status messages.

BUG=638464
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/9d08a7d8a72909a0c5bfe0ed5cb0b590876b7dcf
Cr-Commit-Position: refs/heads/master@{#425991}
==========

[commit-bot: I haz the power, 2016-10-18 16:59:27]

Patchset 4 (id:??) landed as
https://crrev.com/9d08a7d8a72909a0c5bfe0ed5cb0b590876b7dcf
Cr-Commit-Position: refs/heads/master@{#425991}