Added uma for pin unlock set up.

Added uma for pin unlock set up.

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

Committed: https://crrev.com/e5a389c22acbf08f7b7ed89e7242290a2f20d9fc
Cr-Commit-Position: refs/heads/master@{#439836}

[sammiequon, 2016-09-06 22:17:48]

Description was changed from

==========
Added uma for pin unlock set up.

BUG=621548
==========

to

==========
Added uma for pin unlock set up.

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

[sammiequon, 2016-09-07 15:56:13]

Description was changed from

==========
Added uma for pin unlock set up.

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

to

==========
Added uma for pin unlock set up.

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

[sammiequon, 2016-09-07 15:56:13]

sammiequon@chromium.org changed reviewers:
+ jdufault@chromium.org

[sammiequon, 2016-09-07 15:56:34]

jdufault@ - Please take a look. Thanks!

[jdufault, 2016-09-13 18:36:06]

https://codereview.chromium.org/2313103002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/2313103002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:440:
PageManager.showPageByName('quickUnlockConfigureOverlay');
nit: indentation

https://codereview.chromium.org/2313103002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/lock_screen_constants.js
(right):

https://codereview.chromium.org/2313103002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/lock_screen_constants.js:13: var
PinUnlockUmaHistogramName = 'Settings.PinUnlockSetup';
Since you're already introducing a new file, what about adding a helper method

  recordLockScreenProgress(LockScreenProgress.START_SCREEN_LOCK);

https://codereview.chromium.org/2313103002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/lock_screen_constants.js:19: var
PinUnlockSetupType = {
Rename to LockScreenProgress?

https://codereview.chromium.org/2313103002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/setup_pin_dialog.js (right):

https://codereview.chromium.org/2313103002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/setup_pin_dialog.js:232:
chrome.send('metricsHandler:recordInHistogram',
fix indentation

https://codereview.chromium.org/2313103002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2313103002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:54713: +    The users progress throught
the pin unlock setup wizard. Each type
throught => through

[sammiequon, 2016-09-13 19:18:58]

Patchset #2 (id:20001) has been deleted

[sammiequon, 2016-09-13 19:21:41]

https://codereview.chromium.org/2313103002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/2313103002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:440:
PageManager.showPageByName('quickUnlockConfigureOverlay');
On 2016/09/13 18:36:05, jdufault wrote:
> nit: indentation

Done.

https://codereview.chromium.org/2313103002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/lock_screen_constants.js
(right):

https://codereview.chromium.org/2313103002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/lock_screen_constants.js:13: var
PinUnlockUmaHistogramName = 'Settings.PinUnlockSetup';
On 2016/09/13 18:36:05, jdufault wrote:
> Since you're already introducing a new file, what about adding a helper method
> 
>   recordLockScreenProgress(LockScreenProgress.START_SCREEN_LOCK);

Done.

https://codereview.chromium.org/2313103002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/lock_screen_constants.js:19: var
PinUnlockSetupType = {
On 2016/09/13 18:36:05, jdufault wrote:
> Rename to LockScreenProgress?

Done.

https://codereview.chromium.org/2313103002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/setup_pin_dialog.js (right):

https://codereview.chromium.org/2313103002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/setup_pin_dialog.js:232:
chrome.send('metricsHandler:recordInHistogram',
On 2016/09/13 18:36:05, jdufault wrote:
> fix indentation

Done.

https://codereview.chromium.org/2313103002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2313103002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:54713: +    The users progress throught
the pin unlock setup wizard. Each type
On 2016/09/13 18:36:05, jdufault wrote:
> throught => through

Done.

[jdufault, 2016-09-13 19:36:35]

https://codereview.chromium.org/2313103002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/options_resources.grd (right):

https://codereview.chromium.org/2313103002/diff/40001/chrome/browser/resource...
chrome/browser/resources/options_resources.grd:38: <structure
name="IDR_OPTIONS_LOCK_SCREEN_CONSTANTS_JS"
These files also need to get added to compiled_resources2.gyp so they will get
closure compiled.

https://codereview.chromium.org/2313103002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/lock_screen_constants.js
(right):

https://codereview.chromium.org/2313103002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/lock_screen_constants.js:31: *
@param {number} currentProgress
@param {LockScreenProgress} currentProgress

https://codereview.chromium.org/2313103002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/lock_screen_constants.js:34: if
(currentProgress >= LockScreenProgress.MAX_BUCKET)
Log an error?

https://codereview.chromium.org/2313103002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/setup_pin_dialog.js (right):

https://codereview.chromium.org/2313103002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/setup_pin_dialog.js:230:
recordLockScreenProgress(LockScreenProgress.CONFIRM_PIN);
Fix indentation

https://codereview.chromium.org/2313103002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/people_handler.cc (right):

https://codereview.chromium.org/2313103002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/people_handler.cc:215:
web_ui()->AddMessageHandler(new MetricsHandler());
This should probably be added inside of md_settings_ui since it is not specific
to the people page.

[sammiequon, 2016-09-13 21:33:52]

Patchset #3 (id:60001) has been deleted

[sammiequon, 2016-09-13 21:34:58]

https://codereview.chromium.org/2313103002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/options_resources.grd (right):

https://codereview.chromium.org/2313103002/diff/40001/chrome/browser/resource...
chrome/browser/resources/options_resources.grd:38: <structure
name="IDR_OPTIONS_LOCK_SCREEN_CONSTANTS_JS"
On 2016/09/13 19:36:35, jdufault wrote:
> These files also need to get added to compiled_resources2.gyp so they will get
> closure compiled.

Done.

https://codereview.chromium.org/2313103002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/lock_screen_constants.js
(right):

https://codereview.chromium.org/2313103002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/lock_screen_constants.js:31: *
@param {number} currentProgress
On 2016/09/13 19:36:35, jdufault wrote:
> @param {LockScreenProgress} currentProgress

Done.

https://codereview.chromium.org/2313103002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/lock_screen_constants.js:34: if
(currentProgress >= LockScreenProgress.MAX_BUCKET)
On 2016/09/13 19:36:35, jdufault wrote:
> Log an error?

Done.

https://codereview.chromium.org/2313103002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/setup_pin_dialog.js (right):

https://codereview.chromium.org/2313103002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/setup_pin_dialog.js:230:
recordLockScreenProgress(LockScreenProgress.CONFIRM_PIN);
On 2016/09/13 19:36:35, jdufault wrote:
> Fix indentation

Done.

https://codereview.chromium.org/2313103002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/people_handler.cc (right):

https://codereview.chromium.org/2313103002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/people_handler.cc:215:
web_ui()->AddMessageHandler(new MetricsHandler());
On 2016/09/13 19:36:35, jdufault wrote:
> This should probably be added inside of md_settings_ui since it is not
specific
> to the people page.

Done.

[jdufault, 2016-09-13 22:28:35]

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

https://codereview.chromium.org/2313103002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:267:
recordLockScreenProgress(LockScreenProgress.START_SCREEN_LOCK);
I don't think this catches users who navigate directly to the page by typing it
in their address bar: chrome://md-settings/lockScreen.

This stat should probably be recorded by the password prompt dialog show method.

https://codereview.chromium.org/2313103002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_ui.cc (right):

https://codereview.chromium.org/2313103002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_ui.cc:84:
web_ui->AddMessageHandler(new MetricsHandler());
Add this after all of the AddSettingsPageUIHandlers have been added.

https://codereview.chromium.org/2313103002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/people_handler.cc (right):

https://codereview.chromium.org/2313103002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/people_handler.cc:199: 
Revert

[sammiequon, 2016-09-14 02:00:57]

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

https://codereview.chromium.org/2313103002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:267:
recordLockScreenProgress(LockScreenProgress.START_SCREEN_LOCK);
On 2016/09/13 22:28:35, jdufault wrote:
> I don't think this catches users who navigate directly to the page by typing
it
> in their address bar: chrome://md-settings/lockScreen.
> 
> This stat should probably be recorded by the password prompt dialog show
method.

Done.

https://codereview.chromium.org/2313103002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_ui.cc (right):

https://codereview.chromium.org/2313103002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_ui.cc:84:
web_ui->AddMessageHandler(new MetricsHandler());
On 2016/09/13 22:28:35, jdufault wrote:
> Add this after all of the AddSettingsPageUIHandlers have been added.

Done.

https://codereview.chromium.org/2313103002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/people_handler.cc (right):

https://codereview.chromium.org/2313103002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/people_handler.cc:199: 
On 2016/09/13 22:28:35, jdufault wrote:
> Revert

Done.

[jdufault, 2016-09-14 18:27:06]

lgtm

[sammiequon, 2016-09-14 21:45:06]

sammiequon@chromium.org changed reviewers:
+ dbeam@chromium.org

[sammiequon, 2016-09-14 21:45:26]

dbeam@ - Please take a look. Thanks!

[Dan Beam, 2016-09-16 21:43:15]

https://codereview.chromium.org/2313103002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/2313103002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/browser_options.js:441:
recordLockScreenProgress(LockScreenProgress.START_SCREEN_LOCK);
where is this method included from?  i don't see an addition of your new
lock_screen_constants.js to /options/

[sammiequon, 2016-09-17 00:08:16]

https://codereview.chromium.org/2313103002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/2313103002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/browser_options.js:441:
recordLockScreenProgress(LockScreenProgress.START_SCREEN_LOCK);
On 2016/09/16 21:43:15, Dan Beam wrote:
> where is this method included from?  i don't see an addition of your new
> lock_screen_constants.js to /options/

I'm not quite sure where to add this file, is options_bundle.js the place to add
it?

[Dan Beam, 2016-09-17 00:36:03]

please add a test for both options and settings

https://codereview.chromium.org/2313103002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/2313103002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/browser_options.js:441:
recordLockScreenProgress(LockScreenProgress.START_SCREEN_LOCK);
On 2016/09/17 00:08:15, sammiequon wrote:
> On 2016/09/16 21:43:15, Dan Beam wrote:
> > where is this method included from?  i don't see an addition of your new
> > lock_screen_constants.js to /options/
> 
> I'm not quite sure where to add this file, is options_bundle.js the place to
add
> it?

how about a <script> or <link rel=import> in browser_options.html

[sammiequon, 2016-09-21 18:22:32]

On 2016/09/17 00:36:03, Dan Beam wrote:
> please add a test for both options and settings
> 
>
https://codereview.chromium.org/2313103002/diff/100001/chrome/browser/resourc...
> File chrome/browser/resources/options/browser_options.js (right):
> 
>
https://codereview.chromium.org/2313103002/diff/100001/chrome/browser/resourc...
> chrome/browser/resources/options/browser_options.js:441:
> recordLockScreenProgress(LockScreenProgress.START_SCREEN_LOCK);
> On 2016/09/17 00:08:15, sammiequon wrote:
> > On 2016/09/16 21:43:15, Dan Beam wrote:
> > > where is this method included from?  i don't see an addition of your new
> > > lock_screen_constants.js to /options/
> > 
> > I'm not quite sure where to add this file, is options_bundle.js the place to
> add
> > it?
> 
> how about a <script> or <link rel=import> in browser_options.html

jdufault@ is working on test in a seperate CL

[sammiequon, 2016-09-21 18:22:43]

https://codereview.chromium.org/2313103002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/2313103002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/browser_options.js:441:
recordLockScreenProgress(LockScreenProgress.START_SCREEN_LOCK);
On 2016/09/17 00:36:03, Dan Beam wrote:
> On 2016/09/17 00:08:15, sammiequon wrote:
> > On 2016/09/16 21:43:15, Dan Beam wrote:
> > > where is this method included from?  i don't see an addition of your new
> > > lock_screen_constants.js to /options/
> > 
> > I'm not quite sure where to add this file, is options_bundle.js the place to
> add
> > it?
> 
> how about a <script> or <link rel=import> in browser_options.html

Done.

[jdufault, 2016-09-21 19:01:47]

https://codereview.chromium.org/2313103002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/2313103002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/options/browser_options.html:2: <link rel="import"
href="../settings/people_page/lock_screen_constants.html">
no indent

https://codereview.chromium.org/2313103002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/options/browser_options.html:2: <link rel="import"
href="../settings/people_page/lock_screen_constants.html">
The href doesn't look like a valid URL?

Does this work?

  <link rel="import" href="/people_page/lock_screen_constants.html">

https://codereview.chromium.org/2313103002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/options_ui.cc (right):

https://codereview.chromium.org/2313103002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/options_ui.cc:131: constexpr char
kLockScreenConstantsHTMLPath[] =
These should go before the kLockStateBehavior entries.

https://codereview.chromium.org/2313103002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/options_ui.cc:268:
path_to_idr_map_[kLockScreenConstantsHTMLPath] =
These should go before kLockStateBehavior entries

[sammiequon, 2016-09-22 00:55:59]

https://codereview.chromium.org/2313103002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/2313103002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/options/browser_options.html:2: <link rel="import"
href="../settings/people_page/lock_screen_constants.html">
On 2016/09/21 19:01:46, jdufault wrote:
> no indent

Done.

https://codereview.chromium.org/2313103002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/options/browser_options.html:2: <link rel="import"
href="../settings/people_page/lock_screen_constants.html">
On 2016/09/21 19:01:46, jdufault wrote:
> The href doesn't look like a valid URL?
> 
> Does this work?
> 
>   <link rel="import" href="/people_page/lock_screen_constants.html">

Done.

https://codereview.chromium.org/2313103002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/options_ui.cc (right):

https://codereview.chromium.org/2313103002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/options_ui.cc:131: constexpr char
kLockScreenConstantsHTMLPath[] =
On 2016/09/21 19:01:46, jdufault wrote:
> These should go before the kLockStateBehavior entries.

Done.

https://codereview.chromium.org/2313103002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/options_ui.cc:268:
path_to_idr_map_[kLockScreenConstantsHTMLPath] =
On 2016/09/21 19:01:46, jdufault wrote:
> These should go before kLockStateBehavior entries

Done.

[jdufault, 2016-09-22 17:32:54]

https://codereview.chromium.org/2313103002/diff/160001/extensions/browser/api...
File extensions/browser/api/printer_provider_internal/BUILD.gn (right):

https://codereview.chromium.org/2313103002/diff/160001/extensions/browser/api...
extensions/browser/api/printer_provider_internal/BUILD.gn:12: configs += [
Revert? Rebase change?

[sammiequon, 2016-09-22 23:11:44]

https://codereview.chromium.org/2313103002/diff/160001/extensions/browser/api...
File extensions/browser/api/printer_provider_internal/BUILD.gn (right):

https://codereview.chromium.org/2313103002/diff/160001/extensions/browser/api...
extensions/browser/api/printer_provider_internal/BUILD.gn:12: configs += [
On 2016/09/22 17:32:54, jdufault wrote:
> Revert? Rebase change?

Done.

[jdufault, 2016-09-23 23:21:18]

lgtm

[sammiequon, 2016-09-23 23:57:35]

On 2016/09/23 23:21:18, jdufault wrote:
> lgtm

dbeam@ - Please take a look. Thanks!

[Dan Beam, 2016-09-25 00:53:49]

On 2016/09/21 18:22:32, sammiequon wrote:
> On 2016/09/17 00:36:03, Dan Beam wrote:
> > please add a test for both options and settings
> > 
> >
>
https://codereview.chromium.org/2313103002/diff/100001/chrome/browser/resourc...
> > File chrome/browser/resources/options/browser_options.js (right):
> > 
> >
>
https://codereview.chromium.org/2313103002/diff/100001/chrome/browser/resourc...
> > chrome/browser/resources/options/browser_options.js:441:
> > recordLockScreenProgress(LockScreenProgress.START_SCREEN_LOCK);
> > On 2016/09/17 00:08:15, sammiequon wrote:
> > > On 2016/09/16 21:43:15, Dan Beam wrote:
> > > > where is this method included from?  i don't see an addition of your new
> > > > lock_screen_constants.js to /options/
> > > 
> > > I'm not quite sure where to add this file, is options_bundle.js the place
to
> > add
> > > it?
> > 
> > how about a <script> or <link rel=import> in browser_options.html
> 
> jdufault@ is working on test in a seperate CL

can we land that first?

[jdufault, 2016-10-14 18:06:07]

On 2016/09/25 00:53:49, Dan Beam wrote:
> On 2016/09/21 18:22:32, sammiequon wrote:
> > On 2016/09/17 00:36:03, Dan Beam wrote:
> > > please add a test for both options and settings
> > > 
> > >
> >
>
https://codereview.chromium.org/2313103002/diff/100001/chrome/browser/resourc...
> > > File chrome/browser/resources/options/browser_options.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2313103002/diff/100001/chrome/browser/resourc...
> > > chrome/browser/resources/options/browser_options.js:441:
> > > recordLockScreenProgress(LockScreenProgress.START_SCREEN_LOCK);
> > > On 2016/09/17 00:08:15, sammiequon wrote:
> > > > On 2016/09/16 21:43:15, Dan Beam wrote:
> > > > > where is this method included from?  i don't see an addition of your
new
> > > > > lock_screen_constants.js to /options/
> > > > 
> > > > I'm not quite sure where to add this file, is options_bundle.js the
place
> to
> > > add
> > > > it?
> > > 
> > > how about a <script> or <link rel=import> in browser_options.html
> > 
> > jdufault@ is working on test in a seperate CL
> 
> can we land that first?

FYI, the tests have landed (https://codereview.chromium.org/2157673002/).

[sammiequon, 2016-10-28 17:55:46]

On 2016/10/14 18:06:07, jdufault wrote:
> On 2016/09/25 00:53:49, Dan Beam wrote:
> > On 2016/09/21 18:22:32, sammiequon wrote:
> > > On 2016/09/17 00:36:03, Dan Beam wrote:
> > > > please add a test for both options and settings
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2313103002/diff/100001/chrome/browser/resourc...
> > > > File chrome/browser/resources/options/browser_options.js (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2313103002/diff/100001/chrome/browser/resourc...
> > > > chrome/browser/resources/options/browser_options.js:441:
> > > > recordLockScreenProgress(LockScreenProgress.START_SCREEN_LOCK);
> > > > On 2016/09/17 00:08:15, sammiequon wrote:
> > > > > On 2016/09/16 21:43:15, Dan Beam wrote:
> > > > > > where is this method included from?  i don't see an addition of your
> new
> > > > > > lock_screen_constants.js to /options/
> > > > > 
> > > > > I'm not quite sure where to add this file, is options_bundle.js the
> place
> > to
> > > > add
> > > > > it?
> > > > 
> > > > how about a <script> or <link rel=import> in browser_options.html
> > > 
> > > jdufault@ is working on test in a seperate CL
> > 
> > can we land that first?
> 
> FYI, the tests have landed (https://codereview.chromium.org/2157673002/).

dbeam@ - Please take a look. Thanks!

[jdufault, 2016-10-28 18:13:44]

> dbeam@ - Please take a look. Thanks!

Did you add tests?

[sammiequon, 2016-11-18 22:57:35]

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

[commit-bot: I haz the power, 2016-11-18 22:58:12]

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-11-19 00:12:15]

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

[commit-bot: I haz the power, 2016-11-19 00:12:16]

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

[sammiequon, 2016-11-20 21:45:59]

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

[commit-bot: I haz the power, 2016-11-20 21:46:02]

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-11-20 22:49:24]

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

[commit-bot: I haz the power, 2016-11-20 22:49:25]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-11-22 02:39:19]

commenting to get out of my queue

please ping this CL when you need a review

[sammiequon, 2016-11-29 19:41:09]

Patchset #11 (id:240001) has been deleted

[sammiequon, 2016-11-29 19:41:16]

Patchset #10 (id:220001) has been deleted

[sammiequon, 2016-11-29 20:26:48]

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

[commit-bot: I haz the power, 2016-11-29 20:27:36]

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-11-29 22:19:28]

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

[commit-bot: I haz the power, 2016-11-29 22:19:29]

Dry run: This issue passed the CQ dry run.

[sammiequon, 2016-11-30 00:13:40]

On 2016/11/22 02:39:19, Dan Beam wrote:
> commenting to get out of my queue
> 
> please ping this CL when you need a review

dbeam@ - Added tests. Please take a look. Thanks!

[Dan Beam, 2016-12-16 08:31:01]

usually i'd say you should make an @interface and make a normal (concrete)
version of it in "production" code, and a test double.  this is basically what
we do throughout settings.  as well as you shouldn't really set or access
private members from a test.

but your solutions is really simple and straightforward, and probably less code
than an @interface, so it's fine.

thanks for waiting on the tests.  lgtm

https://codereview.chromium.org/2313103002/diff/260001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/lock_screen_constants.js
(right):

https://codereview.chromium.org/2313103002/diff/260001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen_constants.js:33: var
recordLockScreenProgress = function(currentProgress) {
maybe put this in a namespace?  i.e. settings.recordLockScreenProgress?

[sammiequon, 2016-12-16 23:27:07]

sammiequon@chromium.org changed reviewers:
+ isherman@chromium.org

[sammiequon, 2016-12-16 23:28:19]

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

[commit-bot: I haz the power, 2016-12-16 23:29:12]

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-12-17 01:30:52]

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

[commit-bot: I haz the power, 2016-12-17 01:30:53]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[sammiequon, 2016-12-17 02:15:49]

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

[commit-bot: I haz the power, 2016-12-17 02:16:31]

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-12-17 04:17:22]

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

[commit-bot: I haz the power, 2016-12-17 04:17:24]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[sammiequon, 2016-12-17 04:41:08]

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

[commit-bot: I haz the power, 2016-12-17 04:41:24]

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-12-17 05:48:15]

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

[commit-bot: I haz the power, 2016-12-17 05:48:16]

Dry run: This issue passed the CQ dry run.

[sammiequon, 2016-12-17 18:17:14]

isherman@ - Please take a look at histograms.xml. Thanks!

https://codereview.chromium.org/2313103002/diff/260001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/lock_screen_constants.js
(right):

https://codereview.chromium.org/2313103002/diff/260001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen_constants.js:33: var
recordLockScreenProgress = function(currentProgress) {
On 2016/12/16 08:31:01, Dan Beam wrote:
> maybe put this in a namespace?  i.e. settings.recordLockScreenProgress?

Done.

[Ilya Sherman, 2016-12-20 01:26:30]

Metrics LGTM

[sammiequon, 2016-12-20 16:33:52]

The CQ bit was checked by sammiequon@chromium.org

[sammiequon, 2016-12-20 16:33:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
jdufault@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2313103002/#ps320001
(title: "Closure compiler fix.")

[commit-bot: I haz the power, 2016-12-20 16:34:08]

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-12-20 17:31:12]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1482251632511410,
"parent_rev": "bb3a8f88f9b552889c2ab7ef6a1ba6a73c3be12f", "commit_rev":
"aa5772f0f6538d08e031bc7eaf45e2deab94f5bd"}

[commit-bot: I haz the power, 2016-12-20 17:31:40]

Description was changed from

==========
Added uma for pin unlock set up.

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

to

==========
Added uma for pin unlock set up.

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

Review-Url: https://codereview.chromium.org/2313103002
==========

[commit-bot: I haz the power, 2016-12-20 17:31:42]

Committed patchset #13 (id:320001)

[commit-bot: I haz the power, 2016-12-20 17:35:13]

Description was changed from

==========
Added uma for pin unlock set up.

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

Review-Url: https://codereview.chromium.org/2313103002
==========

to

==========
Added uma for pin unlock set up.

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

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

[commit-bot: I haz the power, 2016-12-20 17:35:15]

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