Add the Sync.PassphraseType histogram.

Add the Sync.PassphraseType histogram.

This logs the passphrase type on sync startup if encryption is enabled.

BUG=421623
Committed: https://crrev.com/5f86f1d17c205db9ab457d1efca053f95e5d010f
Cr-Commit-Position: refs/heads/master@{#307094}

[maxbogue, 2014-12-04 19:49:35]

maxbogue@chromium.org changed reviewers:
+ asvitkine@chromium.org, zea@chromium.org

[maxbogue, 2014-12-04 19:49:36]

PTAL! It feels weird to duplicate the enum in histograms.xml; am I missing a way
to prevent that?

[Alexei Svitkine (slow), 2014-12-04 19:57:06]

lgtm % comments

https://codereview.chromium.org/779093002/diff/1/sync/internal_api/public/syn...
File sync/internal_api/public/sync_encryption_handler.h (right):

https://codereview.chromium.org/779093002/diff/1/sync/internal_api/public/syn...
sync/internal_api/public/sync_encryption_handler.h:35: // the user should be
prompted for a decryption passphrase.
Nit: Add a note not to re-order or delete entries, since these are used in a
histogram.

https://codereview.chromium.org/779093002/diff/1/sync/internal_api/public/syn...
sync/internal_api/public/sync_encryption_handler.h:43: const int
PASSPHRASE_TYPE_SIZE = 4;
Usually, this is put inside the enum at the end, so it automatically grows as
new entries are added.

If there's a good reason why you'd rather not do that, I'm ok with that. If not,
please follow the convention.

[maxbogue, 2014-12-04 21:14:25]

https://codereview.chromium.org/779093002/diff/1/sync/internal_api/public/syn...
File sync/internal_api/public/sync_encryption_handler.h (right):

https://codereview.chromium.org/779093002/diff/1/sync/internal_api/public/syn...
sync/internal_api/public/sync_encryption_handler.h:35: // the user should be
prompted for a decryption passphrase.
On 2014/12/04 19:57:06, Alexei Svitkine wrote:
> Nit: Add a note not to re-order or delete entries, since these are used in a
> histogram.

Done.

https://codereview.chromium.org/779093002/diff/1/sync/internal_api/public/syn...
sync/internal_api/public/sync_encryption_handler.h:43: const int
PASSPHRASE_TYPE_SIZE = 4;
On 2014/12/04 19:57:06, Alexei Svitkine wrote:
> Usually, this is put inside the enum at the end, so it automatically grows as
> new entries are added.
> 
> If there's a good reason why you'd rather not do that, I'm ok with that. If
not,
> please follow the convention.

I thought about doing that, but it might affect code that depends on those being
the only possible values for that enum (thinking switch statements). Nicolas,
have any thoughts on this?

[Nicolas Zea, 2014-12-05 00:53:20]

https://codereview.chromium.org/779093002/diff/1/sync/internal_api/public/syn...
File sync/internal_api/public/sync_encryption_handler.h (right):

https://codereview.chromium.org/779093002/diff/1/sync/internal_api/public/syn...
sync/internal_api/public/sync_encryption_handler.h:43: const int
PASSPHRASE_TYPE_SIZE = 4;
On 2014/12/04 21:14:24, maxbogue wrote:
> On 2014/12/04 19:57:06, Alexei Svitkine wrote:
> > Usually, this is put inside the enum at the end, so it automatically grows
as
> > new entries are added.
> > 
> > If there's a good reason why you'd rather not do that, I'm ok with that. If
> not,
> > please follow the convention.
> 
> I thought about doing that, but it might affect code that depends on those
being
> the only possible values for that enum (thinking switch statements). Nicolas,
> have any thoughts on this?

I'm fine with adding a NUM_PASSPHRASE_TYPES enum, but not sure if you'll need to
update any switch statements.

https://codereview.chromium.org/779093002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/779093002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:36517: +    The active sync passphrase
type at sync startup, if encryption is active.
nit: encryption is always active, I'd just remove that clause.

[maxbogue, 2014-12-05 18:58:59]

PTAL

https://codereview.chromium.org/779093002/diff/1/sync/internal_api/public/syn...
File sync/internal_api/public/sync_encryption_handler.h (right):

https://codereview.chromium.org/779093002/diff/1/sync/internal_api/public/syn...
sync/internal_api/public/sync_encryption_handler.h:43: const int
PASSPHRASE_TYPE_SIZE = 4;
On 2014/12/05 00:53:19, Nicolas Zea wrote:
> On 2014/12/04 21:14:24, maxbogue wrote:
> > On 2014/12/04 19:57:06, Alexei Svitkine wrote:
> > > Usually, this is put inside the enum at the end, so it automatically grows
> as
> > > new entries are added.
> > > 
> > > If there's a good reason why you'd rather not do that, I'm ok with that.
If
> > not,
> > > please follow the convention.
> > 
> > I thought about doing that, but it might affect code that depends on those
> being
> > the only possible values for that enum (thinking switch statements).
Nicolas,
> > have any thoughts on this?
> 
> I'm fine with adding a NUM_PASSPHRASE_TYPES enum, but not sure if you'll need
to
> update any switch statements.

Done. It all compiled.

https://codereview.chromium.org/779093002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/779093002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:36517: +    The active sync passphrase
type at sync startup, if encryption is active.
On 2014/12/05 00:53:20, Nicolas Zea wrote:
> nit: encryption is always active, I'd just remove that clause.

Done. I forgot that KEYSTORE was for when the user hasn't entered their own
passphrase.

[Nicolas Zea, 2014-12-05 18:59:47]

LGTM

[maxbogue, 2014-12-05 21:26:14]

The CQ bit was checked by maxbogue@chromium.org

[commit-bot: I haz the power, 2014-12-05 21:26:37]

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

[commit-bot: I haz the power, 2014-12-05 22:16:08]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-05 22:17:19]

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