[MD Settings][People] Visual updates to the sync setup page.

[MD Settings][People] Visual updates to the sync setup page.

screenshots:
https://screenshot.googleplex.com/gnDB2qGYufo
https://screenshot.googleplex.com/yXPegOqyNnK
https://screenshot.googleplex.com/LBQZ1uzYLjt
https://screenshot.googleplex.com/mooGsad6fjJ
https://screenshot.googleplex.com/0SGpgoeW6Bg

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

Committed: https://crrev.com/23efaf6aa7ffc0c186772321cce481f90160a9a6
Cr-Commit-Position: refs/heads/master@{#427833}

[Moe, 2016-10-19 22:09:19]

Description was changed from

==========
[MD Settings][People] Visual updates to the sync setup page.

BUG=621683
==========

to

==========
[MD Settings][People] Visual updates to the sync setup page.

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

[Moe, 2016-10-19 22:09:26]

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

[commit-bot: I haz the power, 2016-10-19 22:10:23]

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-19 23:05:09]

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

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

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Moe, 2016-10-20 14:06:28]

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

[Moe, 2016-10-20 14:06:43]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-10-20 14:06:53]

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-20 14:08:31]

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

[Moe, 2016-10-20 14:08:31]

Hi Tommy, please review this CL.

[Moe, 2016-10-20 15:02:02]

Description was changed from

==========
[MD Settings][People] Visual updates to the sync setup page.

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

to

==========
[MD Settings][People] Visual updates to the sync setup page.

screenshots:
https://screenshot.googleplex.com/gnDB2qGYufo
https://screenshot.googleplex.com/yXPegOqyNnK
https://screenshot.googleplex.com/LBQZ1uzYLjt
https://screenshot.googleplex.com/mooGsad6fjJ
https://screenshot.googleplex.com/0SGpgoeW6Bg

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

[commit-bot: I haz the power, 2016-10-20 15:19:39]

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

[commit-bot: I haz the power, 2016-10-20 15:19:40]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-10-25 18:28:46]

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

https://codereview.chromium.org/2441503002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/sync_page.js:115: confirmation_: {
since you are now binding the values of these input fields to properties, I
suggest using these everywhere. Do a Ctrl+F on this file for ".value".

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

https://codereview.chromium.org/2441503002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/people_handler.cc:872:
args.SetString("fullEncryptionBody",
Why was this removed? Is this string never used?

https://codereview.chromium.org/2441503002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_sync_page_test.js (right):

https://codereview.chromium.org/2441503002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_sync_page_test.js:358:
test('ExistingPassphraseSubmitButtonHiddenIfPassphraseEmpty', function() {
The UI specs really call for the button to be HIDDEN if there is nothing in the
input field? That sounds kind of counter intuitive. Maybe push back if you
agree?

[Moe, 2016-10-25 22:28:39]

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

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

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-25 22:30:18]

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

https://codereview.chromium.org/2441503002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/sync_page.js:115: confirmation_: {
On 2016/10/25 18:28:46, tommycli wrote:
> since you are now binding the values of these input fields to properties, I
> suggest using these everywhere. Do a Ctrl+F on this file for ".value".

Done.

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

https://codereview.chromium.org/2441503002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/people_handler.cc:873:
GetStringUTF16(IDS_SYNC_FULL_ENCRYPTION_DATA));

I don't think this was ever necessary. This string is identical to
'encryptWithSyncPassphraseLabel' which is used in
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/people...
if fullEncryptionBody isn't set.
Since the string in the mocks seems to have a hyperlink, in order to avoid the
inner-h-t-m-l hack, I changed the logic by not setting the fullEncryptionBody
here and instead use encryptWithSyncPassphraseLabel directly in sync_page.html

https://codereview.chromium.org/2441503002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_sync_page_test.js (right):

https://codereview.chromium.org/2441503002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_sync_page_test.js:358:
test('ExistingPassphraseSubmitButtonHiddenIfPassphraseEmpty', function() {
On 2016/10/25 18:28:46, tommycli wrote:
> The UI specs really call for the button to be HIDDEN if there is nothing in
the
> input field? That sounds kind of counter intuitive. Maybe push back if you
> agree?

The mocks seem to suggest that. I pinged Alan to know whether that is by design.
I'll further update the CL if he suggests a disabled state.

[tommycli, 2016-10-25 22:39:09]

lgtm

https://codereview.chromium.org/2441503002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_sync_page_test.js (right):

https://codereview.chromium.org/2441503002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_sync_page_test.js:358:
test('ExistingPassphraseSubmitButtonHiddenIfPassphraseEmpty', function() {
On 2016/10/25 22:30:18, moe wrote:
> On 2016/10/25 18:28:46, tommycli wrote:
> > The UI specs really call for the button to be HIDDEN if there is nothing in
> the
> > input field? That sounds kind of counter intuitive. Maybe push back if you
> > agree?
> 
> The mocks seem to suggest that. I pinged Alan to know whether that is by
design.
> I'll further update the CL if he suggests a disabled state.

Okay great. I leave it up to you if you want to handle it in this CL or a
followup.

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

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

[commit-bot: I haz the power, 2016-10-26 00:05:01]

Dry run: This issue passed the CQ dry run.

[Moe, 2016-10-26 18:26:02]

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

[Moe, 2016-10-26 18:26:13]

https://codereview.chromium.org/2441503002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_sync_page_test.js (right):

https://codereview.chromium.org/2441503002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_sync_page_test.js:358:
test('ExistingPassphraseSubmitButtonHiddenIfPassphraseEmpty', function() {
On 2016/10/25 22:39:09, tommycli wrote:
> On 2016/10/25 22:30:18, moe wrote:
> > On 2016/10/25 18:28:46, tommycli wrote:
> > > The UI specs really call for the button to be HIDDEN if there is nothing
in
> > the
> > > input field? That sounds kind of counter intuitive. Maybe push back if you
> > > agree?
> > 
> > The mocks seem to suggest that. I pinged Alan to know whether that is by
> design.
> > I'll further update the CL if he suggests a disabled state.
> 
> Okay great. I leave it up to you if you want to handle it in this CL or a
> followup.

Alan confirmed that the button should be disabled rather than hidden. I made
changes in the last patch.

[commit-bot: I haz the power, 2016-10-26 18:27:02]

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

[tommycli, 2016-10-26 18:27:30]

latest changes lgtm with one question:

https://codereview.chromium.org/2441503002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_sync_page_test.js (right):

https://codereview.chromium.org/2441503002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_sync_page_test.js:371:
assertTrue(!!submitExistingPassphrase.disabled);
nit: Why the double bang? Is it undefined sometimes?

[Moe, 2016-10-26 19:01:31]

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

[commit-bot: I haz the power, 2016-10-26 19:02: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-26 21:07:41]

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

[commit-bot: I haz the power, 2016-10-26 21:07:42]

Dry run: This issue passed the CQ dry run.

[Moe, 2016-10-26 21:42:40]

https://codereview.chromium.org/2441503002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_sync_page_test.js (right):

https://codereview.chromium.org/2441503002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_sync_page_test.js:371:
assertTrue(!!submitExistingPassphrase.disabled);
On 2016/10/26 18:27:30, tommycli wrote:
> nit: Why the double bang? Is it undefined sometimes?

Done.

[Moe, 2016-10-26 21:42:46]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2016-10-26 21:42:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/2441503002/#ps80001
(title: "Addressed comment")

[commit-bot: I haz the power, 2016-10-26 21:43:27]

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-26 21:57:20]

Description was changed from

==========
[MD Settings][People] Visual updates to the sync setup page.

screenshots:
https://screenshot.googleplex.com/gnDB2qGYufo
https://screenshot.googleplex.com/yXPegOqyNnK
https://screenshot.googleplex.com/LBQZ1uzYLjt
https://screenshot.googleplex.com/mooGsad6fjJ
https://screenshot.googleplex.com/0SGpgoeW6Bg

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

to

==========
[MD Settings][People] Visual updates to the sync setup page.

screenshots:
https://screenshot.googleplex.com/gnDB2qGYufo
https://screenshot.googleplex.com/yXPegOqyNnK
https://screenshot.googleplex.com/LBQZ1uzYLjt
https://screenshot.googleplex.com/mooGsad6fjJ
https://screenshot.googleplex.com/0SGpgoeW6Bg

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

[commit-bot: I haz the power, 2016-10-26 21:57:21]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-10-26 22:00:09]

Description was changed from

==========
[MD Settings][People] Visual updates to the sync setup page.

screenshots:
https://screenshot.googleplex.com/gnDB2qGYufo
https://screenshot.googleplex.com/yXPegOqyNnK
https://screenshot.googleplex.com/LBQZ1uzYLjt
https://screenshot.googleplex.com/mooGsad6fjJ
https://screenshot.googleplex.com/0SGpgoeW6Bg

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

to

==========
[MD Settings][People] Visual updates to the sync setup page.

screenshots:
https://screenshot.googleplex.com/gnDB2qGYufo
https://screenshot.googleplex.com/yXPegOqyNnK
https://screenshot.googleplex.com/LBQZ1uzYLjt
https://screenshot.googleplex.com/mooGsad6fjJ
https://screenshot.googleplex.com/0SGpgoeW6Bg

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

Committed: https://crrev.com/23efaf6aa7ffc0c186772321cce481f90160a9a6
Cr-Commit-Position: refs/heads/master@{#427833}
==========

[commit-bot: I haz the power, 2016-10-26 22:00:10]

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