[Smart Lock, Settings Reconciliation] Histograms for tracking initial and final pref values.

[Smart Lock, Settings Reconciliation] Histograms for tracking initial and final pref values.

In order to make sure that migration algorithm works correctly we want
to observe combined initial and final values for the preferences after
the migration has happened.

BUG=517087
Committed: https://crrev.com/ccc2bfbe9850ce2aaf4dd3e558baada0a17b4b3c
Cr-Commit-Position: refs/heads/master@{#352003}

[melandory, 2015-09-29 12:40:59]

melandory@chromium.org changed reviewers:
+ engedy@chromium.org

[melandory, 2015-09-29 12:40:59]

Hi Balazs,

please take a look.

Thanks!

[engedy, 2015-09-29 13:24:53]

LGTM % comments.

https://codereview.chromium.org/1373983005/diff/40001/chrome/browser/password...
File
chrome/browser/password_manager/password_manager_setting_migrator_service.cc
(right):

https://codereview.chromium.org/1373983005/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_manager_setting_migrator_service.cc:238:
bool final_new_pref_value =
nit: I would extract this into a separate method.

https://codereview.chromium.org/1373983005/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_manager_setting_migrator_service.cc:244:
const int kInitNewValueMask = 0x8;
nit: s/Init/Initial/, below too.

https://codereview.chromium.org/1373983005/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1373983005/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:30291: +    preference for controlling
Smart Lock on Android. Sample is recorded after
nit: A single sample ...

https://codereview.chromium.org/1373983005/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:30292: +    sync has merged last snaphot
and finished initialization for the sync users
nit: for sync users, and on profile initialization for non-sync users.

https://codereview.chromium.org/1373983005/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:67018: +      label="Initial state: new
pref is 'off', legacy pref is 'off' Final
nit: These strings are very long and will probably not fit in the metrics
dashboard UI. We should come up with a more concise, yet readable encoding (we
could describe it in the histogram definition).

[engedy, 2015-09-29 13:51:53]

https://codereview.chromium.org/1373983005/diff/40001/chrome/browser/password...
File
chrome/browser/password_manager/password_manager_setting_migrator_service.cc
(right):

https://codereview.chromium.org/1373983005/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_manager_setting_migrator_service.cc:208:
void PasswordManagerSettingMigratorService::MigrateOffState(
One more thing: the histograms.xml description mentions that the new histogram
would be recorded for non-sync users as well. Based on this and the unittest,
this is currently not the case, though. Which is the intended?

(Also, it would be nice to add unit test to verify this if we indeed wish to
record the value.)

[melandory, 2015-09-29 22:08:37]

melandory@chromium.org changed reviewers:
+ rkaplow@chromium.org

[melandory, 2015-09-29 22:08:38]

rkaplow@chromium.org: Please review changes in 

tools/metrics/histograms/histograms.xml

https://codereview.chromium.org/1373983005/diff/40001/chrome/browser/password...
File
chrome/browser/password_manager/password_manager_setting_migrator_service.cc
(right):

https://codereview.chromium.org/1373983005/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_manager_setting_migrator_service.cc:208:
void PasswordManagerSettingMigratorService::MigrateOffState(
On 2015/09/29 13:51:53, engedy wrote:
> One more thing: the histograms.xml description mentions that the new histogram
> would be recorded for non-sync users as well. Based on this and the unittest,
> this is currently not the case, though. Which is the intended?
> 
> (Also, it would be nice to add unit test to verify this if we indeed wish to
> record the value.)

It should be done for both, fixed. Done.

https://codereview.chromium.org/1373983005/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_manager_setting_migrator_service.cc:238:
bool final_new_pref_value =
On 2015/09/29 13:24:52, engedy wrote:
> nit: I would extract this into a separate method.

Done.

https://codereview.chromium.org/1373983005/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_manager_setting_migrator_service.cc:244:
const int kInitNewValueMask = 0x8;
On 2015/09/29 13:24:52, engedy wrote:
> nit: s/Init/Initial/, below too.

Done.

https://codereview.chromium.org/1373983005/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1373983005/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:30291: +    preference for controlling
Smart Lock on Android. Sample is recorded after
On 2015/09/29 13:24:52, engedy wrote:
> nit: A single sample ...

Done.

https://codereview.chromium.org/1373983005/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:30292: +    sync has merged last snaphot
and finished initialization for the sync users
On 2015/09/29 13:24:53, engedy wrote:
> nit: for sync users, and on profile initialization for non-sync users.

Done.

https://codereview.chromium.org/1373983005/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:67018: +      label="Initial state: new
pref is 'off', legacy pref is 'off' Final
On 2015/09/29 13:24:53, engedy wrote:
> nit: These strings are very long and will probably not fit in the metrics
> dashboard UI. We should come up with a more concise, yet readable encoding (we
> could describe it in the histogram definition).

Done.

[rkaplow, 2015-09-29 22:51:55]

lgtm

https://codereview.chromium.org/1373983005/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1373983005/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:67013: +    The pair initial values and
the pair of final values for the legacy
pair of

[melandory, 2015-10-02 08:21:37]

https://codereview.chromium.org/1373983005/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1373983005/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:67013: +    The pair initial values and
the pair of final values for the legacy
On 2015/09/29 22:51:55, rkaplow wrote:
> pair of

Done.

[melandory, 2015-10-02 08:21:46]

The CQ bit was checked by melandory@chromium.org

[melandory, 2015-10-02 08:21:46]

The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org,
rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/1373983005/#ps100001
(title: "Rebased on top of master.")

[commit-bot: I haz the power, 2015-10-02 08:21:59]

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

[commit-bot: I haz the power, 2015-10-02 11:35:02]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-02 11:36:36]

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