Add Easy Unlock options to chrome://settings behind a flag.

Add Easy Unlock options to chrome://settings behind a flag.

The URLs for this setting will need to be updated when they are finalized.

BUG=346048
TEST=manual

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258203

[Tim Song, 2014-03-14 22:16:38]

+xiyuan for chrome/browser/ui/webui/*
+rogerta for chrome/browser/signin

Please let me know if chrome/browser/signin is a good place to put code for this
feature.

[xiyuan, 2014-03-15 00:54:55]

https://codereview.chromium.org/197313007/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/197313007/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:583:
values->SetString("easyUnlockCheckboxLabel",
nit: wrong indent

https://codereview.chromium.org/197313007/diff/40001/chrome/common/url_consta...
File chrome/common/url_constants.cc (right):

https://codereview.chromium.org/197313007/diff/40001/chrome/common/url_consta...
chrome/common/url_constants.cc:707: 
nit: nuke one empty line

https://codereview.chromium.org/197313007/diff/40001/chromeos/chromeos_utils.cc
File chromeos/chromeos_utils.cc (right):

https://codereview.chromium.org/197313007/diff/40001/chromeos/chromeos_utils....
chromeos/chromeos_utils.cc:22: return "Chromebook";
These are UI facing strings. Shouldn't them be in a grd file? If using grd file,
/chromeos might not be a good place since it does not depend onl10n_util.

[Roger Tawa OOO till Jul 10th, 2014-03-17 17:15:00]

Tim: when I click on the mocks link in the crbug description, I get an empty
folder.  Can you add an updated link to mocks in the bug?  Thanks.

[Roger Tawa OOO till Jul 10th, 2014-03-17 17:23:10]

The code in the files under chrome/browser/signin looks fine, but I'm not sure
this is the best place for it.  Is this cros specific?  If so, does not make
more sense to place under chrome/browser/chromeos/login ?

https://codereview.chromium.org/197313007/diff/40001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_controller.cc (right):

https://codereview.chromium.org/197313007/diff/40001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_controller.cc:23: ::prefs::kEasyUnlockPairing,
No need for :: in front of prefs::

[xiyuan, 2014-03-17 17:28:37]

On 2014/03/17 17:23:10, Roger Tawa wrote:
> The code in the files under chrome/browser/signin looks fine, but I'm not sure
> this is the best place for it.  Is this cros specific?  If so, does not make
> more sense to place under chrome/browser/chromeos/login ?
> 

The plan is to make it work with Mirror unlock too. I am looking at the
bluetooth Windows stack now and would wire the easy unlock app on Windows when I
get it to work properly.

[Tim Song, 2014-03-17 19:12:07]

https://codereview.chromium.org/197313007/diff/40001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_controller.cc (right):

https://codereview.chromium.org/197313007/diff/40001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_controller.cc:23: ::prefs::kEasyUnlockPairing,
On 2014/03/17 17:23:10, Roger Tawa wrote:
> No need for :: in front of prefs::

Done.

https://codereview.chromium.org/197313007/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/197313007/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:583:
values->SetString("easyUnlockCheckboxLabel",
On 2014/03/15 00:54:56, xiyuan wrote:
> nit: wrong indent

Done.

https://codereview.chromium.org/197313007/diff/40001/chrome/common/url_consta...
File chrome/common/url_constants.cc (right):

https://codereview.chromium.org/197313007/diff/40001/chrome/common/url_consta...
chrome/common/url_constants.cc:707: 
On 2014/03/15 00:54:56, xiyuan wrote:
> nit: nuke one empty line

Done.

https://codereview.chromium.org/197313007/diff/40001/chromeos/chromeos_utils.cc
File chromeos/chromeos_utils.cc (right):

https://codereview.chromium.org/197313007/diff/40001/chromeos/chromeos_utils....
chromeos/chromeos_utils.cc:22: return "Chromebook";
On 2014/03/15 00:54:56, xiyuan wrote:
> These are UI facing strings. Shouldn't them be in a grd file? If using grd
file,
> /chromeos might not be a good place since it does not depend onl10n_util.

Hmm... there are a few other places that does a check like this (eg. Bluetooth,
Sync Session Name), and use the raw unlocalized string. As Panther is recently
released, each of these places need to be updated, so a common util function
would be helpful.

I also looked through some of the translated strings, and it doesn't look like
"Chromebook" or "Chromebox" are translated. There could be cases that these
names need localizing, so to be safe it makes sense to use a string resource.

However, the issue is that the components that currently does this check
(device/ and sync/) shouldn't really need to depend on ui/ either.

For now, I'll move this file to chrome/browser/chromeos, but we should probably
find a reusable solution for the other checks.

[xiyuan, 2014-03-17 20:04:50]

lgtm

https://codereview.chromium.org/197313007/diff/40001/chromeos/chromeos_utils.cc
File chromeos/chromeos_utils.cc (right):

https://codereview.chromium.org/197313007/diff/40001/chromeos/chromeos_utils....
chromeos/chromeos_utils.cc:22: return "Chromebook";
On 2014/03/17 19:12:07, Tim Song wrote:
> However, the issue is that the components that currently does this check
> (device/ and sync/) shouldn't really need to depend on ui/ either.
> 
> For now, I'll move this file to chrome/browser/chromeos, but we should
probably
> find a reusable solution for the other checks.

One possible solution is that GetChromeDeviceType in /chromeos component returns
an enum instead of a string and we have some util/helper function in /chrome to
translate the enum into a string.

This does not need to happen in this CL. I am okay with putting
GetChromeDeviceType in /c/b/chromeos for now.

[Roger Tawa OOO till Jul 10th, 2014-03-18 12:40:06]

lgtm c/b/s

[Tim Song, 2014-03-18 20:24:17]

+battre@ for chrome/browser/prefs/

[Tim Song, 2014-03-19 17:19:23]

-battre@
+mnissler@

[Mattias Nissler (ping if slow), 2014-03-19 18:15:19]

prefs LGTM once you address my comments. If you make substantial changes, please
have someone re-review though.

https://codereview.chromium.org/197313007/diff/80001/chrome/app/chromeos_stri...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/197313007/diff/80001/chrome/app/chromeos_stri...
chrome/app/chromeos_strings.grdp:7: <message name="IDS_CHROMEBOOK" desc="The
localized name for Chromebook.">
These look like candidates for chromium_strings.grd which has all the other
product names.

https://codereview.chromium.org/197313007/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/chromeos_utils.cc (right):

https://codereview.chromium.org/197313007/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/chromeos_utils.cc:14: const char* kChromeboxBoards[]  =
{ "stumpy", "panther" };
indentation should be zero spaces.

Also, are you seriously signing up for keeping this up-to-date?

https://codereview.chromium.org/197313007/diff/80001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_controller.h (right):

https://codereview.chromium.org/197313007/diff/80001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_controller.h:13: class EasyUnlockController {
No need to add a class here. Just declare a function named
RegisterEasyUnlockProfilePrefs. Might also want to rename the files to
easy_unlock_prefs.{h,cc} for clarity.

https://codereview.chromium.org/197313007/diff/80001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/197313007/diff/80001/chrome/common/pref_names...
chrome/common/pref_names.cc:1302: const char kGCMRegisteredAppIDs[] =
"gcm.register_app_ids";
profile prefs section ends here.

https://codereview.chromium.org/197313007/diff/80001/chrome/common/pref_names...
chrome/common/pref_names.cc:2676: // Whether Easy Unlock is enabled.
ditto regarding placement in file.

https://codereview.chromium.org/197313007/diff/80001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/197313007/diff/80001/chrome/common/pref_names...
chrome/common/pref_names.h:409: extern const char kGCMRegisteredAppIDs[];
Profile prefs section ends here.

https://codereview.chromium.org/197313007/diff/80001/chrome/common/pref_names...
chrome/common/pref_names.h:952: extern const char kEasyUnlockPairing[];
This should go at the end of the profile prefs section at the top of the file.

[Tim Song, 2014-03-19 23:41:36]

https://codereview.chromium.org/197313007/diff/80001/chrome/app/chromeos_stri...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/197313007/diff/80001/chrome/app/chromeos_stri...
chrome/app/chromeos_strings.grdp:7: <message name="IDS_CHROMEBOOK" desc="The
localized name for Chromebook.">
On 2014/03/19 18:15:19, Mattias Nissler wrote:
> These look like candidates for chromium_strings.grd which has all the other
> product names.

That file seems to be just for strings with "Chromium" in them as opposed to
"Chrome". I don't think "Chromiumbook" is actually a thing.

https://codereview.chromium.org/197313007/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/chromeos_utils.cc (right):

https://codereview.chromium.org/197313007/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/chromeos_utils.cc:14: const char* kChromeboxBoards[]  =
{ "stumpy", "panther" };
On 2014/03/19 18:15:19, Mattias Nissler wrote:
> indentation should be zero spaces.
> 
> Also, are you seriously signing up for keeping this up-to-date?

Done. It's better to keep this list up to date here than have every part of the
code do its own thing. I'll try my best to keep it up to date; it shouldn't be
too difficult unless we start releasing a different Chromebox every week.

https://codereview.chromium.org/197313007/diff/80001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_controller.h (right):

https://codereview.chromium.org/197313007/diff/80001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_controller.h:13: class EasyUnlockController {
On 2014/03/19 18:15:19, Mattias Nissler wrote:
> No need to add a class here. Just declare a function named
> RegisterEasyUnlockProfilePrefs. Might also want to rename the files to
> easy_unlock_prefs.{h,cc} for clarity.

This class will be less of a stub in the future once we move Easy Unlock logic
from our app's background page to native code.

https://codereview.chromium.org/197313007/diff/80001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/197313007/diff/80001/chrome/common/pref_names...
chrome/common/pref_names.cc:1302: const char kGCMRegisteredAppIDs[] =
"gcm.register_app_ids";
On 2014/03/19 18:15:19, Mattias Nissler wrote:
> profile prefs section ends here.

Done.

https://codereview.chromium.org/197313007/diff/80001/chrome/common/pref_names...
chrome/common/pref_names.cc:2676: // Whether Easy Unlock is enabled.
On 2014/03/19 18:15:19, Mattias Nissler wrote:
> ditto regarding placement in file.

Done.

https://codereview.chromium.org/197313007/diff/80001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/197313007/diff/80001/chrome/common/pref_names...
chrome/common/pref_names.h:409: extern const char kGCMRegisteredAppIDs[];
On 2014/03/19 18:15:19, Mattias Nissler wrote:
> Profile prefs section ends here.

Done.

https://codereview.chromium.org/197313007/diff/80001/chrome/common/pref_names...
chrome/common/pref_names.h:952: extern const char kEasyUnlockPairing[];
On 2014/03/19 18:15:19, Mattias Nissler wrote:
> This should go at the end of the profile prefs section at the top of the file.

Done.

[Tim Song, 2014-03-20 02:23:19]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2014-03-20 02:23:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/197313007/120001

[commit-bot: I haz the power, 2014-03-20 05:47:33]

Change committed as 258203