Only report PasswordState in Sync for UMA+non-custom passphrase users.

Only report PasswordState in Sync for UMA+non-custom passphrase users.

BUG=641421, 643393
Committed: https://crrev.com/8548bedfaf6c4a54b37de8f01821dff63544b85c
Cr-Commit-Position: refs/heads/master@{#416659}

[Nathan Parker, 2016-09-01 21:41:39]

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

[commit-bot: I haz the power, 2016-09-01 21:42:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nathan Parker, 2016-09-01 21:47:07]

nparker@chromium.org changed reviewers:
+ asvitkine@chromium.org, dvadym@chromium.org

[Nathan Parker, 2016-09-01 21:47:07]

dvadym: Please review password_manager/
asvitkine: Please review metrics/

Thanks!

[commit-bot: I haz the power, 2016-09-01 22:45:03]

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

[commit-bot: I haz the power, 2016-09-01 22:45:04]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-09-02 09:12:19]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2016-09-02 09:12:20]

Thanks, I have some comments for the password_manager side.

Cheers,
Vaclav

https://codereview.chromium.org/2302823003/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/2302823003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/chrome_password_manager_client.h:168: // Returns
true if this profile has metrics reporting and doesn't have
nit: The current phrasing is unclear about the necessity of sync being set up.
What about:

// Returns true if this profile has metrics reporting and active sync without a
custom sync passphrase.

https://codereview.chromium.org/2302823003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/chrome_password_manager_client.h:202:
FRIEND_TEST_ALL_PREFIXES(ChromePasswordManagerClientTest,
Please do not friend the test. We try hard to eliminate that from our code.
Tests should use public API to interact with the tested class.

In this case, the nearest public API to call in the test is
ChromePasswordManagerClient::AnnotateNavigationEntry. When the annotation is not
cancelled because of ShouldAnnotateNavigationEntry, you should be able to check
the last committed entry for the annotation.

https://codereview.chromium.org/2302823003/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/2302823003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:379:
// Metrics enabled w/ non-custom passphrase: do annotate
Please split the 4 blocks at lines 373, 379, 386 and 393 up into 4 different
test cases.
(See go/unit-test-practices: "DO: Write a separate test for each behavior
defined by a method" vs. "DON'T: Use a single test to validate a method that has
multiple effects"

[Alexei Svitkine (slow), 2016-09-02 18:05:46]

Thanks! Comments below.

https://codereview.chromium.org/2302823003/diff/1/chrome/browser/metrics/chro...
File chrome/browser/metrics/chrome_metrics_service_accessor.cc (right):

https://codereview.chromium.org/2302823003/diff/1/chrome/browser/metrics/chro...
chrome/browser/metrics/chrome_metrics_service_accessor.cc:20: const bool*
gMetricsAndCrashReportingForTesting = nullptr;
Nit: Incorrect naming convention.

We only use kFooBar for constants. Everything else should be hacker_style - so
g_metrics_and_...

I'd actually name it:

g_metrics_consent_for_testing

https://codereview.chromium.org/2302823003/diff/1/chrome/browser/metrics/chro...
File chrome/browser/metrics/chrome_metrics_service_accessor.h (right):

https://codereview.chromium.org/2302823003/diff/1/chrome/browser/metrics/chro...
chrome/browser/metrics/chrome_metrics_service_accessor.h:135: static void
SetMetricsAndCrashReportingForTesting(const bool* value);
I'm OK with this being public so that further tests don't need to be added as
friends. The ForTesting suffix should make sure it doesn't make it into
production code.

[Nathan Parker, 2016-09-02 22:42:03]

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

[commit-bot: I haz the power, 2016-09-02 22:42:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nathan Parker, 2016-09-02 22:42:36]

Thanks, all comments addressed.

vabr and asvitkine: PTAL

https://codereview.chromium.org/2302823003/diff/1/chrome/browser/metrics/chro...
File chrome/browser/metrics/chrome_metrics_service_accessor.cc (right):

https://codereview.chromium.org/2302823003/diff/1/chrome/browser/metrics/chro...
chrome/browser/metrics/chrome_metrics_service_accessor.cc:20: const bool*
gMetricsAndCrashReportingForTesting = nullptr;
On 2016/09/02 18:05:45, Alexei Svitkine wrote:
> Nit: Incorrect naming convention.
> 
> We only use kFooBar for constants. Everything else should be hacker_style - so
> g_metrics_and_...
> 
> I'd actually name it:
> 
> g_metrics_consent_for_testing

Done.

https://codereview.chromium.org/2302823003/diff/1/chrome/browser/metrics/chro...
File chrome/browser/metrics/chrome_metrics_service_accessor.h (right):

https://codereview.chromium.org/2302823003/diff/1/chrome/browser/metrics/chro...
chrome/browser/metrics/chrome_metrics_service_accessor.h:135: static void
SetMetricsAndCrashReportingForTesting(const bool* value);
On 2016/09/02 18:05:45, Alexei Svitkine wrote:
> I'm OK with this being public so that further tests don't need to be added as
> friends. The ForTesting suffix should make sure it doesn't make it into
> production code.

Done.

https://codereview.chromium.org/2302823003/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/2302823003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/chrome_password_manager_client.h:168: // Returns
true if this profile has metrics reporting and doesn't have
On 2016/09/02 09:12:20, vabr (Chromium) wrote:
> nit: The current phrasing is unclear about the necessity of sync being set up.
> What about:
> 
> // Returns true if this profile has metrics reporting and active sync without
a
> custom sync passphrase.

Done.

https://codereview.chromium.org/2302823003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/chrome_password_manager_client.h:202:
FRIEND_TEST_ALL_PREFIXES(ChromePasswordManagerClientTest,
On 2016/09/02 09:12:20, vabr (Chromium) wrote:
> Please do not friend the test. We try hard to eliminate that from our code.
> Tests should use public API to interact with the tested class.
> 
> In this case, the nearest public API to call in the test is
> ChromePasswordManagerClient::AnnotateNavigationEntry. When the annotation is
not
> cancelled because of ShouldAnnotateNavigationEntry, you should be able to
check
> the last committed entry for the annotation.

Interesting, good idea in general.  Done.  And I tested more functionality.

https://codereview.chromium.org/2302823003/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/2302823003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:379:
// Metrics enabled w/ non-custom passphrase: do annotate
On 2016/09/02 09:12:20, vabr (Chromium) wrote:
> Please split the 4 blocks at lines 373, 379, 386 and 393 up into 4 different
> test cases.
> (See go/unit-test-practices: "DO: Write a separate test for each behavior
> defined by a method" vs. "DON'T: Use a single test to validate a method that
has
> multiple effects"

Done.  And I split apart the new test I added.

[commit-bot: I haz the power, 2016-09-02 23:27:45]

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

[commit-bot: I haz the power, 2016-09-02 23:27:46]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-09-05 08:47:49]

Great stuff, Nathan, thank you!

All in password_manager LGTM.

Cheers,
Vaclav

[Alexei Svitkine (slow), 2016-09-06 16:53:45]

lgtm

[Nathan Parker, 2016-09-06 17:02:28]

The CQ bit was checked by nparker@chromium.org

[commit-bot: I haz the power, 2016-09-06 17:02:45]

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-09-06 17:53:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-06 17:56:47]

Description was changed from

==========
Only report PasswordState in Sync for UMA+non-custom passphrase users.

BUG=641421,643393
==========

to

==========
Only report PasswordState in Sync for UMA+non-custom passphrase users.

BUG=641421,643393

Committed: https://crrev.com/8548bedfaf6c4a54b37de8f01821dff63544b85c
Cr-Commit-Position: refs/heads/master@{#416659}
==========

[commit-bot: I haz the power, 2016-09-06 17:56:48]

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