Settings People Revamp: Easy Unlock: Add browser proxy and setup flow.

Settings People Revamp: Easy Unlock: Add browser proxy and setup flow.

Adds a browser proxy object and implements the setup flow. Tests in a
separate CL.

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

[tommycli, 2016-03-16 21:57:16]

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

[commit-bot: I haz the power, 2016-03-16 21:57:31]

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

[tommycli, 2016-03-16 21:58:18]

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

[tommycli, 2016-03-16 21:58:19]

dpapad: PTAL, thanks!

[dpapad, 2016-03-16 22:17:01]

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

https://codereview.chromium.org/1806123002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:61:
easyUnlockBrowserProxy_: {
Nit: How about renaming this simply to "browserProxy_"? Since there is no other
browser proxy this component talks to, I think it is more readable.

https://codereview.chromium.org/1806123002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:64: return
settings.EasyUnlockBrowserProxyImpl.getInstance();
Do you think it would be worth adding unit tests for <settings-people-page>? It
could test that the correct proxy methods are called when this element is
attached and also when each of the buttons is tapped.

If so, you might have to move this assignment within the ready() lifecycle
method instead (otherwise you will not have an opportunity to swap with the fake
proxy).

[commit-bot: I haz the power, 2016-03-16 23:13:37]

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

[commit-bot: I haz the power, 2016-03-16 23:13:38]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-03-17 00:33:26]

dpapad: thanks!

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

https://codereview.chromium.org/1806123002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:61:
easyUnlockBrowserProxy_: {
On 2016/03/16 22:17:01, dpapad wrote:
> Nit: How about renaming this simply to "browserProxy_"? Since there is no
other
> browser proxy this component talks to, I think it is more readable.

Done.

https://codereview.chromium.org/1806123002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:64: return
settings.EasyUnlockBrowserProxyImpl.getInstance();
On 2016/03/16 22:17:01, dpapad wrote:
> Do you think it would be worth adding unit tests for <settings-people-page>?
It
> could test that the correct proxy methods are called when this element is
> attached and also when each of the buttons is tapped.
> 
> If so, you might have to move this assignment within the ready() lifecycle
> method instead (otherwise you will not have an opportunity to swap with the
fake
> proxy).

Yes, the unit tests are in the very next patch I'm sending you. You may preview
it at https://codereview.chromium.org/1811813002, however, I have not
self-reviewed this yet, so it's not ready for a full review. I can combine the
two if you prefer.

In the unit tests, I modify the loadTimeData before I recreate the
settings-page-element for the test. Is that also acceptable?

[dpapad, 2016-03-17 00:52:23]

lgtm

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

https://codereview.chromium.org/1806123002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:64: return
settings.EasyUnlockBrowserProxyImpl.getInstance();
No need to merge them. I'll review the follow up once ready.

Regarding setting loadTimeData during testing, it seems fine, unless we have a
better solution.

The only alternative that comes to mind is to at least move any information that
does not need to be available in a synchronous way (if any) from loadTimeData to
a cr.sendWithPromise() call via the proxy. But I am guessing that what you have
in loadTimeData() is there because it is needed synchronously (otherwise
probably UI flicker would occur).

[tommycli, 2016-03-17 18:38:07]

dpapad: thanks!

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

https://codereview.chromium.org/1806123002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:64: return
settings.EasyUnlockBrowserProxyImpl.getInstance();
On 2016/03/17 00:52:23, dpapad wrote:
> No need to merge them. I'll review the follow up once ready.
> 
> Regarding setting loadTimeData during testing, it seems fine, unless we have a
> better solution.
> 
> The only alternative that comes to mind is to at least move any information
that
> does not need to be available in a synchronous way (if any) from loadTimeData
to
> a cr.sendWithPromise() call via the proxy. But I am guessing that what you
have
> in loadTimeData() is there because it is needed synchronously (otherwise
> probably UI flicker would occur).

Exactly. The data is retrieved via cr.sendWithPromise anyways, but making it
available within loadTimeData prevents the UI flicker.

[tommycli, 2016-03-17 18:38:10]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-03-17 18:38:37]

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

[commit-bot: I haz the power, 2016-03-17 20:17:47]

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

[commit-bot: I haz the power, 2016-03-17 20:17:49]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tommycli, 2016-03-17 20:20:20]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-03-17 20:20:43]

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

[commit-bot: I haz the power, 2016-03-17 22:16:26]

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

[commit-bot: I haz the power, 2016-03-17 22:16:27]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tommycli, 2016-03-17 23:38:50]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-03-17 23:39:22]

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

[commit-bot: I haz the power, 2016-03-18 00:21:10]

Committed patchset #3 (id:40001)

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

Description was changed from

==========
Settings People Revamp: Easy Unlock: Add browser proxy and setup flow.

Adds a browser proxy object and implements the setup flow. Tests in a
separate CL.

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

to

==========
Settings People Revamp: Easy Unlock: Add browser proxy and setup flow.

Adds a browser proxy object and implements the setup flow. Tests in a
separate CL.

BUG=563721

Committed: https://crrev.com/ebfb928267b08b8b2552b6ece3cb705208cbd4ca
Cr-Commit-Position: refs/heads/master@{#381845}
==========

[commit-bot: I haz the power, 2016-03-18 00:23:01]

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