Adding pref to store the user-selected proximity threshold.

Adding pref to store the user-selected proximity threshold.

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

Review-Url: https://codereview.chromium.org/2973243002
Cr-Commit-Position: refs/heads/master@{#487630}
Committed: https://chromium.googlesource.com/chromium/src/+/474f53bea3f44cba536a1c6f6fe8611d4d89f720

[sacomoto, 2017-07-07 16:16:14]

Description was changed from

==========
Adding a RSSI threshold selector EasyUnlock in the settings menu.

BUG=
==========

to

==========
Adding a RSSI threshold selector EasyUnlock in the settings menu.

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

[sacomoto, 2017-07-07 16:22:39]

Description was changed from

==========
Adding a RSSI threshold selector EasyUnlock in the settings menu.

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

to

==========
Adding a RSSI threshold selector EasyUnlock in the settings menu.

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

[sacomoto, 2017-07-07 16:22:39]

sacomoto@chromium.org changed reviewers:
+ tengs@chromium.org

[sacomoto, 2017-07-07 16:24:49]

Hey Tim,

I got the RSSI selector working, I still need to fix / add some tests. Please
take a first look.

BTW, I'm open to suggesting for the number RSSI options given to the user, and
their value.

Could also tell me what values work well for eve & cave devices (with a Nexus &
Pixel)?

[Tim Song, 2017-07-07 21:48:24]

Thanks for this. From my tests with eve and pixel xl, -70 seemed to work the
best. It seems to have RSSIs on the lower side.

https://codereview.chromium.org/2973243002/diff/20001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2973243002/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:1118: <message
name="IDS_SETTINGS_EASY_UNLOCK_PROXIMITY_THRESHOLD_FAR" desc="The text on the
Easy unlock settings page indicating the phone can far for the unlock to work.">
nit: can be far

https://codereview.chromium.org/2973243002/diff/20001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/2973243002/diff/20001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service.cc:266: registry->RegisterIntegerPref(
Although there are a lot of prefs here, I want to gradually move all of them to
ProximityAuthPrefManager. There's also a lot of prefs here that are only used by
the Chrome app.

Can you make a RegisterProfilePrefs() function in ProximityAuthPrefManager
instead?

https://codereview.chromium.org/2973243002/diff/20001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service.cc:266: registry->RegisterIntegerPref(
Note: We also need to sync this to the local state in
EasyUnlockServiceRegular::SyncProfilePrefsToLocalState().

https://codereview.chromium.org/2973243002/diff/20001/components/proximity_au...
File components/proximity_auth/proximity_monitor_impl.cc (right):

https://codereview.chromium.org/2973243002/diff/20001/components/proximity_au...
components/proximity_auth/proximity_monitor_impl.cc:223: switch (threshold) {
Add a default case as well.

https://codereview.chromium.org/2973243002/diff/20001/components/proximity_au...
components/proximity_auth/proximity_monitor_impl.cc:225: rssi_threshold_ = -55;
I would spread out these values out a bit more. Perhaps something like:

{ -30, -50, -70, -85 }

This way we can cover more combinations of devices.

[sacomoto, 2017-07-10 14:27:19]

Description was changed from

==========
Adding a RSSI threshold selector EasyUnlock in the settings menu.

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

to

==========
Adding a RSSI threshold selector for EasyUnlock in the settings menu.

EasyUnlock is disabled:
https://screenshot.googleplex.com/2WxZMn78UgM

EasyUnlock is enabled (the default value is selected):
https://screenshot.googleplex.com/PigQj6rzuCO

EasyUnlock is enabled, user is selecting the value:
https://screenshot.googleplex.com/qBwt6PkGhAw

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

[sacomoto, 2017-07-10 16:54:07]

PTAL.

https://codereview.chromium.org/2973243002/diff/20001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2973243002/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:1118: <message
name="IDS_SETTINGS_EASY_UNLOCK_PROXIMITY_THRESHOLD_FAR" desc="The text on the
Easy unlock settings page indicating the phone can far for the unlock to work.">
On 2017/07/07 21:48:24, Tim Song wrote:
> nit: can be far

Done.

https://codereview.chromium.org/2973243002/diff/20001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/2973243002/diff/20001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service.cc:266: registry->RegisterIntegerPref(
On 2017/07/07 21:48:24, Tim Song wrote:
> Note: We also need to sync this to the local state in
> EasyUnlockServiceRegular::SyncProfilePrefsToLocalState().

Done.

https://codereview.chromium.org/2973243002/diff/20001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service.cc:266: registry->RegisterIntegerPref(
On 2017/07/07 21:48:24, Tim Song wrote:
> Although there are a lot of prefs here, I want to gradually move all of them
to
> ProximityAuthPrefManager. There's also a lot of prefs here that are only used
by
> the Chrome app.
> 
> Can you make a RegisterProfilePrefs() function in ProximityAuthPrefManager
> instead?

I moved it to ProximityAuthPrefManager. There is already a
ProximityAuthPrefManager::RegisterPrefs(), which we are calling below.

However, I kept the definition of prefs::kEasyUnlockProximityThreshold in the
chrome/common/pref_names.h, because I need to whitelist them here:
webui/settings/md_settings_localized_strings_provider.cc.

https://codereview.chromium.org/2973243002/diff/20001/components/proximity_au...
File components/proximity_auth/proximity_monitor_impl.cc (right):

https://codereview.chromium.org/2973243002/diff/20001/components/proximity_au...
components/proximity_auth/proximity_monitor_impl.cc:223: switch (threshold) {
On 2017/07/07 21:48:24, Tim Song wrote:
> Add a default case as well.

Done.

https://codereview.chromium.org/2973243002/diff/20001/components/proximity_au...
components/proximity_auth/proximity_monitor_impl.cc:225: rssi_threshold_ = -55;
On 2017/07/07 21:48:24, Tim Song wrote:
> I would spread out these values out a bit more. Perhaps something like:
> 
> { -30, -50, -70, -85 }
> 
> This way we can cover more combinations of devices.
> 

I spread it a bit more, but I think -30 is really unrealistic.

[sacomoto, 2017-07-10 18:04:13]

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

[commit-bot: I haz the power, 2017-07-10 18:04:26]

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, 2017-07-10 18:14:24]

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

[commit-bot: I haz the power, 2017-07-10 18:14:25]

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

[Tim Song, 2017-07-10 19:27:56]

https://codereview.chromium.org/2973243002/diff/60001/components/proximity_au...
File components/proximity_auth/proximity_auth_pref_manager.h (right):

https://codereview.chromium.org/2973243002/diff/60001/components/proximity_au...
components/proximity_auth/proximity_auth_pref_manager.h:73: virtual void
SetProximityThreshold(int value);
This |value| should be an enum.

https://codereview.chromium.org/2973243002/diff/60001/components/proximity_au...
File components/proximity_auth/proximity_monitor_impl_unittest.cc (right):

https://codereview.chromium.org/2973243002/diff/60001/components/proximity_au...
components/proximity_auth/proximity_monitor_impl_unittest.cc:45: const int
kProximityThreshold = 2;
nit: rename to kProximityThresholdPrefValue

https://codereview.chromium.org/2973243002/diff/60001/components/proximity_au...
File components/proximity_auth/unlock_manager_impl.h (right):

https://codereview.chromium.org/2973243002/diff/60001/components/proximity_au...
components/proximity_auth/unlock_manager_impl.h:62: PrefService* pref_service);
You should take in the ProximityAuthPrefManager instead. We can pass in the
pref_manager_ in ProximityAuthSystem.

[sacomoto, 2017-07-11 09:56:06]

https://codereview.chromium.org/2973243002/diff/60001/components/proximity_au...
File components/proximity_auth/proximity_auth_pref_manager.h (right):

https://codereview.chromium.org/2973243002/diff/60001/components/proximity_au...
components/proximity_auth/proximity_auth_pref_manager.h:73: virtual void
SetProximityThreshold(int value);
On 2017/07/10 19:27:56, Tim Song wrote:
> This |value| should be an enum.

Done.

https://codereview.chromium.org/2973243002/diff/60001/components/proximity_au...
File components/proximity_auth/proximity_monitor_impl_unittest.cc (right):

https://codereview.chromium.org/2973243002/diff/60001/components/proximity_au...
components/proximity_auth/proximity_monitor_impl_unittest.cc:45: const int
kProximityThreshold = 2;
On 2017/07/10 19:27:56, Tim Song wrote:
> nit: rename to kProximityThresholdPrefValue

Done.

https://codereview.chromium.org/2973243002/diff/60001/components/proximity_au...
File components/proximity_auth/unlock_manager_impl.h (right):

https://codereview.chromium.org/2973243002/diff/60001/components/proximity_au...
components/proximity_auth/unlock_manager_impl.h:62: PrefService* pref_service);
On 2017/07/10 19:27:56, Tim Song wrote:
> You should take in the ProximityAuthPrefManager instead. We can pass in the
> pref_manager_ in ProximityAuthSystem.

Done.

[sacomoto, 2017-07-11 09:56:14]

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

[commit-bot: I haz the power, 2017-07-11 09:56:33]

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, 2017-07-11 11:25:44]

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

[commit-bot: I haz the power, 2017-07-11 11:25:46]

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

[Tim Song, 2017-07-11 18:34:46]

lgtm

[sacomoto, 2017-07-11 21:05:33]

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

[commit-bot: I haz the power, 2017-07-11 21:06:14]

Dry run: CQ is trying da patch.

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

[sacomoto, 2017-07-11 21:11:24]

sacomoto@chromium.org changed reviewers:
+ stevenjb@chromium.org

[sacomoto, 2017-07-11 21:12:47]

sacomoto@chromium.org changed reviewers:
+ battre@chromium.org

[sacomoto, 2017-07-11 21:15:16]

stevenjb@, PTAL at the resource and strings.

battre@, PTAL at the proximity_auth/DEPS.

[commit-bot: I haz the power, 2017-07-11 23:07:59]

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

[commit-bot: I haz the power, 2017-07-11 23:08:00]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has
not started yet; builder either lacks capacity or does not exist (misspelled?))
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))

[Tim Song, 2017-07-11 23:45:58]

Hey Gustavo, I just noticed a few issues with the dependencies. Please take a
look.

https://codereview.chromium.org/2973243002/diff/100001/components/proximity_a...
File components/proximity_auth/DEPS (right):

https://codereview.chromium.org/2973243002/diff/100001/components/proximity_a...
components/proximity_auth/DEPS:6: "+chrome/common/pref_names.h",
We shouldn't depend on chrome/

https://codereview.chromium.org/2973243002/diff/100001/components/proximity_a...
File components/proximity_auth/proximity_auth_pref_manager.cc (right):

https://codereview.chromium.org/2973243002/diff/100001/components/proximity_a...
components/proximity_auth/proximity_auth_pref_manager.cc:12: #include
"chrome/common/pref_names.h"
We shouldn't depend on anything from chrome/.

https://codereview.chromium.org/2973243002/diff/100001/components/proximity_a...
components/proximity_auth/proximity_auth_pref_manager.cc:28: void
ProximityAuthPrefManager::RegisterPrefs(PrefRegistrySimple* registry) {
We should take a PrefRegistrySyncable here to be more precise.

https://codereview.chromium.org/2973243002/diff/100001/components/proximity_a...
components/proximity_auth/proximity_auth_pref_manager.cc:33:
::prefs::kEasyUnlockProximityThreshold, 1,
Move this to proximity_auth_pref_names in the component.

[battre, 2017-07-12 07:18:47]

LGTM once the dependency issue pointed out by Tim is resolved.

https://codereview.chromium.org/2973243002/diff/100001/chrome/app/settings_st...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2973243002/diff/100001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:1113: very close
Drive by: I think that these values are pretty meaningless to me as a user.

https://codereview.chromium.org/2973243002/diff/100001/components/proximity_a...
File components/proximity_auth/proximity_monitor_impl.cc (right):

https://codereview.chromium.org/2973243002/diff/100001/components/proximity_a...
components/proximity_auth/proximity_monitor_impl.cc:223: default:
I would suggest to drop the default block. The compiler will warn you in case a
new threshold is being added without updating this.

https://codereview.chromium.org/2973243002/diff/100001/components/proximity_a...
File components/proximity_auth/proximity_monitor_impl.h (right):

https://codereview.chromium.org/2973243002/diff/100001/components/proximity_a...
components/proximity_auth/proximity_monitor_impl.h:110: int rssi_threshold_;
you should initialize this in the constructor.

[sacomoto, 2017-07-12 15:52:53]

https://codereview.chromium.org/2973243002/diff/100001/chrome/app/settings_st...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2973243002/diff/100001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:1113: very close
On 2017/07/12 07:18:47, battre wrote:
> Drive by: I think that these values are pretty meaningless to me as a user.

This is intentionally vague. Unfortunately, the mapping between RSSI value and
distance is not completely accurate, e.g. we cannot say that -70 RSSI
corresponds to 1m.

The signal strength is intrinsically noisy and the geometry (e.g. the location
of the BLE antenas) affects the signal strength. Moreover, the value of the RSSI
has a big dependency on the hardware combination (phone and the Chromebook).

https://codereview.chromium.org/2973243002/diff/100001/components/proximity_a...
File components/proximity_auth/DEPS (right):

https://codereview.chromium.org/2973243002/diff/100001/components/proximity_a...
components/proximity_auth/DEPS:6: "+chrome/common/pref_names.h",
On 2017/07/11 23:45:57, Tim Song wrote:
> We shouldn't depend on chrome/

Done.

https://codereview.chromium.org/2973243002/diff/100001/components/proximity_a...
File components/proximity_auth/proximity_auth_pref_manager.cc (right):

https://codereview.chromium.org/2973243002/diff/100001/components/proximity_a...
components/proximity_auth/proximity_auth_pref_manager.cc:12: #include
"chrome/common/pref_names.h"
On 2017/07/11 23:45:57, Tim Song wrote:
> We shouldn't depend on anything from chrome/.

Done.

https://codereview.chromium.org/2973243002/diff/100001/components/proximity_a...
components/proximity_auth/proximity_auth_pref_manager.cc:28: void
ProximityAuthPrefManager::RegisterPrefs(PrefRegistrySimple* registry) {
On 2017/07/11 23:45:57, Tim Song wrote:
> We should take a PrefRegistrySyncable here to be more precise.

Done.

https://codereview.chromium.org/2973243002/diff/100001/components/proximity_a...
components/proximity_auth/proximity_auth_pref_manager.cc:33:
::prefs::kEasyUnlockProximityThreshold, 1,
On 2017/07/11 23:45:57, Tim Song wrote:
> Move this to proximity_auth_pref_names in the component.

Done.

https://codereview.chromium.org/2973243002/diff/100001/components/proximity_a...
File components/proximity_auth/proximity_monitor_impl.cc (right):

https://codereview.chromium.org/2973243002/diff/100001/components/proximity_a...
components/proximity_auth/proximity_monitor_impl.cc:223: default:
On 2017/07/12 07:18:47, battre wrote:
> I would suggest to drop the default block. The compiler will warn you in case
a
> new threshold is being added without updating this.

Done.

https://codereview.chromium.org/2973243002/diff/100001/components/proximity_a...
File components/proximity_auth/proximity_monitor_impl.h (right):

https://codereview.chromium.org/2973243002/diff/100001/components/proximity_a...
components/proximity_auth/proximity_monitor_impl.h:110: int rssi_threshold_;
On 2017/07/12 07:18:47, battre wrote:
> you should initialize this in the constructor.

Done.

[sacomoto, 2017-07-12 15:53:02]

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

[commit-bot: I haz the power, 2017-07-12 15:53:17]

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, 2017-07-12 17:32:14]

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

[commit-bot: I haz the power, 2017-07-12 17:32:15]

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

[stevenjb, 2017-07-17 16:43:37]

Please also update easy_unlock_browsertest_chromeos.js

(You might want to exclude the settings changes from this CL and review / submit
those separately).

https://codereview.chromium.org/2973243002/diff/120001/chrome/app/settings_st...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2973243002/diff/120001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:1110: Only unlock this Chromebook when your
phone is
I'm concerned about how well this will localize. Generally a dropdown would have
a label and a list of self contained values (vs. completing a sentence, which
works in English but not as well in other languages). 

e.g. "Phone distance to unlock this Chromebook"

https://codereview.chromium.org/2973243002/diff/120001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:1113: very close
Usually we would capitalize these, e.g. 'Very close'

https://codereview.chromium.org/2973243002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/lock_screen.html (right):

https://codereview.chromium.org/2973243002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.html:140: <div
class="setting-box">
Instead of having a settings-box inside a settings-box like this, this should be
at the same level as #easyUnlock.

Also we generally use iron-collapse to hide the section when the setting is
disabled so that the subsection animates opened/closed when the controlling
setting is enabled/disabled.

See "Enable auto repeat" in the keyboard settings for an example.

https://codereview.chromium.org/2973243002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.html:146:
</settings-dropdown-menu>
The dropdown menu should be at the same level as the <div> so that it appears
right aligned.

https://codereview.chromium.org/2973243002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/lock_screen.js (right):

https://codereview.chromium.org/2973243002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.js:135: * threshold
selector dropdown menu.
* @type {DropdownMenuOptionList}

(You will need to include settings_dropdown_menu.js in compiled_resources2.gyp).

https://codereview.chromium.org/2973243002/diff/120001/components/proximity_a...
File components/proximity_auth/proximity_monitor_impl.cc (right):

https://codereview.chromium.org/2973243002/diff/120001/components/proximity_a...
components/proximity_auth/proximity_monitor_impl.cc:227: }
NOTREACHED();

[stevenjb, 2017-07-17 16:55:09]

https://codereview.chromium.org/2973243002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/lock_screen.html (right):

https://codereview.chromium.org/2973243002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.html:76:
class="list-item underbar">
'underbar' should be removed here (see next comment)

https://codereview.chromium.org/2973243002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.html:129: <div
id="easyUnlock" class="settings-box two-line continuation">
I also noticed that this really shouldn't be 'continuation' and we should remove
the final 'underbar' above.

[sacomoto, 2017-07-18 18:39:46]

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

[commit-bot: I haz the power, 2017-07-18 18:40:00]

Dry run: CQ is trying da patch.

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

[sacomoto, 2017-07-18 18:42:29]

Description was changed from

==========
Adding a RSSI threshold selector for EasyUnlock in the settings menu.

EasyUnlock is disabled:
https://screenshot.googleplex.com/2WxZMn78UgM

EasyUnlock is enabled (the default value is selected):
https://screenshot.googleplex.com/PigQj6rzuCO

EasyUnlock is enabled, user is selecting the value:
https://screenshot.googleplex.com/qBwt6PkGhAw

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

to

==========
Adding a pref to store the proximity threshold.

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

[sacomoto, 2017-07-18 18:46:19]

@stevenjb, I'm following your suggestion and removing all the settings change
from this CL. I'm keeping only the pref changes.

I'll address all your comments on a next containing only the settings changes.

https://codereview.chromium.org/2973243002/diff/120001/components/proximity_a...
File components/proximity_auth/proximity_monitor_impl.cc (right):

https://codereview.chromium.org/2973243002/diff/120001/components/proximity_a...
components/proximity_auth/proximity_monitor_impl.cc:227: }
On 2017/07/17 16:43:37, stevenjb wrote:
> NOTREACHED();

Done.

[sacomoto, 2017-07-18 21:30:12]

Description was changed from

==========
Adding a pref to store the proximity threshold.

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

to

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

[commit-bot: I haz the power, 2017-07-18 21:59:57]

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

[commit-bot: I haz the power, 2017-07-18 21:59:58]

Dry run: This issue passed the CQ dry run.

[sacomoto, 2017-07-18 22:06:21]

The CQ bit was checked by sacomoto@chromium.org

[sacomoto, 2017-07-18 22:06:22]

The patchset sent to the CQ was uploaded after l-g-t-m from tengs@chromium.org,
battre@chromium.org
Link to the patchset: https://codereview.chromium.org/2973243002/#ps200001
(title: "Fixing merge issues")

[commit-bot: I haz the power, 2017-07-18 22:06: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, 2017-07-18 22:19:04]

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

[commit-bot: I haz the power, 2017-07-18 22:19:06]

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...)

[sacomoto, 2017-07-18 22:21:00]

Description was changed from

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

to

==========
Adding pref to store the user-selected proximity threshold.

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

[sacomoto, 2017-07-18 22:21:12]

The CQ bit was checked by sacomoto@chromium.org

[commit-bot: I haz the power, 2017-07-18 22:21:26]

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, 2017-07-18 22:37:04]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1500416472888170,
"parent_rev": "628e9d0922332d94629b3fec38f1a1d274eff042", "commit_rev":
"2bcb2c8d7752b97f72bacdf45441fe86a23455ab"}

[commit-bot: I haz the power, 2017-07-18 22:37:21]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1500416472888170,
"parent_rev": "9ea3bab07797c644a1b0865924d4e203c838159c", "commit_rev":
"474f53bea3f44cba536a1c6f6fe8611d4d89f720"}

[commit-bot: I haz the power, 2017-07-18 22:37:35]

Description was changed from

==========
Adding pref to store the user-selected proximity threshold.

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

to

==========
Adding pref to store the user-selected proximity threshold.

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

Review-Url: https://codereview.chromium.org/2973243002
Cr-Commit-Position: refs/heads/master@{#487630}
Committed:
https://chromium.googlesource.com/chromium/src/+/474f53bea3f44cba536a1c6f6fe8...
==========

[commit-bot: I haz the power, 2017-07-18 22:37:37]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/474f53bea3f44cba536a1c6f6fe8...

[sacomoto, 2017-07-20 19:50:16]

Hey Steven,

I addressed your comments in this CL:
https://chromium-review.googlesource.com/c/580407

PTAL.

https://codereview.chromium.org/2973243002/diff/120001/chrome/app/settings_st...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2973243002/diff/120001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:1110: Only unlock this Chromebook when your
phone is
On 2017/07/17 16:43:36, stevenjb wrote:
> I'm concerned about how well this will localize. Generally a dropdown would
have
> a label and a list of self contained values (vs. completing a sentence, which
> works in English but not as well in other languages). 
> 
> e.g. "Phone distance to unlock this Chromebook"
> 

This was changed to "Distance needed for phone to unlock this Chromebook"

https://codereview.chromium.org/2973243002/diff/120001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:1113: very close
On 2017/07/17 16:43:36, stevenjb wrote:
> Usually we would capitalize these, e.g. 'Very close'

Done.

https://codereview.chromium.org/2973243002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/lock_screen.html (right):

https://codereview.chromium.org/2973243002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.html:76:
class="list-item underbar">
On 2017/07/17 16:55:08, stevenjb wrote:
> 'underbar' should be removed here (see next comment)

Done.

https://codereview.chromium.org/2973243002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.html:129: <div
id="easyUnlock" class="settings-box two-line continuation">
On 2017/07/17 16:55:08, stevenjb wrote:
> I also noticed that this really shouldn't be 'continuation' and we should
remove
> the final 'underbar' above.

Done.

https://codereview.chromium.org/2973243002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.html:140: <div
class="setting-box">
On 2017/07/17 16:43:36, stevenjb wrote:
> Instead of having a settings-box inside a settings-box like this, this should
be
> at the same level as #easyUnlock.
> 
> Also we generally use iron-collapse to hide the section when the setting is
> disabled so that the subsection animates opened/closed when the controlling
> setting is enabled/disabled.
> 
> See "Enable auto repeat" in the keyboard settings for an example.

Done.

https://codereview.chromium.org/2973243002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.html:146:
</settings-dropdown-menu>
On 2017/07/17 16:43:37, stevenjb wrote:
> The dropdown menu should be at the same level as the <div> so that it appears
> right aligned.

Done.

https://codereview.chromium.org/2973243002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/lock_screen.js (right):

https://codereview.chromium.org/2973243002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.js:135: * threshold
selector dropdown menu.
On 2017/07/17 16:43:37, stevenjb wrote:
> * @type {DropdownMenuOptionList}
> 
> (You will need to include settings_dropdown_menu.js in
compiled_resources2.gyp).

Done.