Settings People Revamp: Easy Unlock: Add basic UI

Settings People Revamp: Easy Unlock: Add basic UI

This patch
 1) Adds the Easy Unlock title and description to People UI.
 2) Adds the Easy Unlock setup and turnoff buttons. (Not hooked up)
 3) Updates the EasyUnlockHandler to support the enabled state.
 4) Fixes some issues with Easy Unlock strings.

BUG=563721
Committed: https://crrev.com/4949275f29f5ea6db05ef08b1336797ac3ae4ca8
Cr-Commit-Position: refs/heads/master@{#381509}

[tommycli, 2016-03-14 22:22:56]

Description was changed from

==========
Settings People Revamp: Add Easy Unlock setup and turnoff buttons.

This patch:
 1) Adds the Easy Unlock setup and turnoff buttons. (Not hooked up)
 2) Adds an EasyUnlockBrowserProxy which just communicates enabled
    state. (More functionality going to be added)
 3) Updates the EasyUnlockHandler to support the enabled state.
 4) Fixes some issues with Easy Unlock strings.

BUG=563721
==========

to

==========
Settings People Revamp: Add Easy Unlock text and buttons

This patch
 1) Adds the Easy Unlock description and help text to People UI.
 2) Adds the Easy Unlock setup and turnoff buttons. (Not hooked up)
 3) Updates the EasyUnlockHandler to support the enabled state.
 4) Fixes some issues with Easy Unlock strings.

BUG=563721
==========

[tommycli, 2016-03-14 22:24:09]

Description was changed from

==========
Settings People Revamp: Add Easy Unlock text and buttons

This patch
 1) Adds the Easy Unlock description and help text to People UI.
 2) Adds the Easy Unlock setup and turnoff buttons. (Not hooked up)
 3) Updates the EasyUnlockHandler to support the enabled state.
 4) Fixes some issues with Easy Unlock strings.

BUG=563721
==========

to

==========
Settings People Revamp: Easy Unlock: Add basic UI

This patch
 1) Adds the Easy Unlock title and description to People UI.
 2) Adds the Easy Unlock setup and turnoff buttons. (Not hooked up)
 3) Updates the EasyUnlockHandler to support the enabled state.
 4) Fixes some issues with Easy Unlock strings.

BUG=563721
==========

[tommycli, 2016-03-14 22:25:38]

tommycli@chromium.org changed reviewers:
+ dpapad@chromium.org, tengs@chromium.org

[tommycli, 2016-03-14 22:25:38]

dpapad: PTAL
tengs: PTAL easy_unlock_service.h small change

[dpapad, 2016-03-15 00:44:27]

Sending browser/resources/* comments. Have not reviewed ui/webui/settings/ yet.

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

https://codereview.chromium.org/1799913003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:64:
easyUnlockAllowed_: {
Nit: Does the value of this ever change? If not, should we also add "readonly:
true"?

https://codereview.chromium.org/1799913003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:91:
this.addWebUIListener(
IIUC, this is listener is not needed if easyUnlockAllowed_ is false, and that
value can never change. If that is true, let's wrap this

if (this.easyUnlockAllowed_) {
  this.addWebUIListener(...);
}

https://codereview.chromium.org/1799913003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:93:
this.handleEasyUnlockEnabledStatusChanged_.bind(this));
Nit (optional): Given that handleEasyUnlockEnabledStatusChanged_ is a single
line method, and that it is wrapped in <if>, how about simply inlining it here
as an anonymous function? This way there is no need for the 2nd <if> a few lines
below.

[dpapad, 2016-03-15 17:49:40]

https://codereview.chromium.org/1799913003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/easy_unlock_settings_handler.cc
(right):

https://codereview.chromium.org/1799913003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/easy_unlock_settings_handler.cc:30:
profile_pref_registrar_.Add(
Based on our WebUI initialization pitfalls discussion, registering this C++
observer in the constructor is not very safe. It might end-up calling to JS
before WebUI is ready.

The proposed solution (see
https://docs.google.com/document/d/1ubLoYnTSyjCMFCpy1tPy6g1RAm5FajmbuQygjqN3a...)
is to only register C++ observers after the page has communicated that it is
ready.

https://codereview.chromium.org/1799913003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/easy_unlock_settings_handler.cc:51:
html_source->AddString("easyUnlockLearnMoreURL",
Can we move "easyUnlockLearnMoreURL" registration in
md_settings_localized_strings_provider.cc? I think this is more aligned with the
pattern we follow for other pages, where string constants are registered there,
and only info that depends on this class is registered here (for example
booleans).

[tommycli, 2016-03-15 18:58:27]

dpapad: I think I've addressed your feedback now. Thanks!

Tommy

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

https://codereview.chromium.org/1799913003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:64:
easyUnlockAllowed_: {
On 2016/03/15 00:44:27, dpapad wrote:
> Nit: Does the value of this ever change? If not, should we also add "readonly:
> true"?

Done.

https://codereview.chromium.org/1799913003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:91:
this.addWebUIListener(
On 2016/03/15 00:44:27, dpapad wrote:
> IIUC, this is listener is not needed if easyUnlockAllowed_ is false, and that
> value can never change. If that is true, let's wrap this
> 
> if (this.easyUnlockAllowed_) {
>   this.addWebUIListener(...);
> }

Done.

https://codereview.chromium.org/1799913003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:93:
this.handleEasyUnlockEnabledStatusChanged_.bind(this));
On 2016/03/15 00:44:27, dpapad wrote:
> Nit (optional): Given that handleEasyUnlockEnabledStatusChanged_ is a single
> line method, and that it is wrapped in <if>, how about simply inlining it here
> as an anonymous function? This way there is no need for the 2nd <if> a few
lines
> below.

I should have mentioned: I will also be doing a cr.sendWithPromise with this as
a the callback fn in the next CL.

https://codereview.chromium.org/1799913003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/easy_unlock_settings_handler.cc
(right):

https://codereview.chromium.org/1799913003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/easy_unlock_settings_handler.cc:30:
profile_pref_registrar_.Add(
On 2016/03/15 17:49:40, dpapad wrote:
> Based on our WebUI initialization pitfalls discussion, registering this C++
> observer in the constructor is not very safe. It might end-up calling to JS
> before WebUI is ready.
> 
> The proposed solution (see
>
https://docs.google.com/document/d/1ubLoYnTSyjCMFCpy1tPy6g1RAm5FajmbuQygjqN3a...)
> is to only register C++ observers after the page has communicated that it is
> ready.

Done.

https://codereview.chromium.org/1799913003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/easy_unlock_settings_handler.cc:51:
html_source->AddString("easyUnlockLearnMoreURL",
On 2016/03/15 17:49:40, dpapad wrote:
> Can we move "easyUnlockLearnMoreURL" registration in
> md_settings_localized_strings_provider.cc? I think this is more aligned with
the
> pattern we follow for other pages, where string constants are registered
there,
> and only info that depends on this class is registered here (for example
> booleans).

Done.

[dpapad, 2016-03-15 20:21:47]

lgtm

[tommycli, 2016-03-15 20:40:02]

dpapad: thanks for the fast review!

tengs: ping for 1 line review plz :D

[tommycli, 2016-03-15 20:52:15]

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

[commit-bot: I haz the power, 2016-03-15 20:52:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1799913003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1799913003/80001

[commit-bot: I haz the power, 2016-03-15 20:55:22]

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

[commit-bot: I haz the power, 2016-03-15 20:55:23]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommycli, 2016-03-15 21:00:34]

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

[commit-bot: I haz the power, 2016-03-15 21:00:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1799913003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1799913003/100001

[commit-bot: I haz the power, 2016-03-15 21:31:53]

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

[commit-bot: I haz the power, 2016-03-15 21:31:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommycli, 2016-03-16 18:33:52]

tengs: ping for tiny review

[Tim Song, 2016-03-16 18:41:33]

easy_unlock_service.h LGTM

[tommycli, 2016-03-16 18:46:54]

On 2016/03/16 18:41:33, Tim Song wrote:
> easy_unlock_service.h LGTM

tengs: thanks!

[tommycli, 2016-03-16 18:47:00]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-03-16 18:47:00]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/1799913003/#ps120001
(title: "test fixes")

[commit-bot: I haz the power, 2016-03-16 18:47:23]

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

[commit-bot: I haz the power, 2016-03-16 19:16:43]

Description was changed from

==========
Settings People Revamp: Easy Unlock: Add basic UI

This patch
 1) Adds the Easy Unlock title and description to People UI.
 2) Adds the Easy Unlock setup and turnoff buttons. (Not hooked up)
 3) Updates the EasyUnlockHandler to support the enabled state.
 4) Fixes some issues with Easy Unlock strings.

BUG=563721
==========

to

==========
Settings People Revamp: Easy Unlock: Add basic UI

This patch
 1) Adds the Easy Unlock title and description to People UI.
 2) Adds the Easy Unlock setup and turnoff buttons. (Not hooked up)
 3) Updates the EasyUnlockHandler to support the enabled state.
 4) Fixes some issues with Easy Unlock strings.

BUG=563721
==========

[commit-bot: I haz the power, 2016-03-16 19:16:44]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-16 19:17:54]

Description was changed from

==========
Settings People Revamp: Easy Unlock: Add basic UI

This patch
 1) Adds the Easy Unlock title and description to People UI.
 2) Adds the Easy Unlock setup and turnoff buttons. (Not hooked up)
 3) Updates the EasyUnlockHandler to support the enabled state.
 4) Fixes some issues with Easy Unlock strings.

BUG=563721
==========

to

==========
Settings People Revamp: Easy Unlock: Add basic UI

This patch
 1) Adds the Easy Unlock title and description to People UI.
 2) Adds the Easy Unlock setup and turnoff buttons. (Not hooked up)
 3) Updates the EasyUnlockHandler to support the enabled state.
 4) Fixes some issues with Easy Unlock strings.

BUG=563721

Committed: https://crrev.com/4949275f29f5ea6db05ef08b1336797ac3ae4ca8
Cr-Commit-Position: refs/heads/master@{#381509}
==========

[commit-bot: I haz the power, 2016-03-16 19:17:55]

Patchset 7 (id:??) landed as
https://crrev.com/4949275f29f5ea6db05ef08b1336797ac3ae4ca8
Cr-Commit-Position: refs/heads/master@{#381509}