Add finch control of user population in low reputation requests

Add Finch feature and parameters to control user population for low reputation requests and password reuse requests.

There are 3 experiment populations that corresponds to our launch plan:
(1) SBER && !incognito
(2) (SBER || Sync) && !incognito
(3) everyone

BUG=698899
Review-Url: https://codereview.chromium.org/2856033002
Cr-Commit-Position: refs/heads/master@{#470212}
Committed: https://chromium.googlesource.com/chromium/src/+/fe7f0c917485514e7ce80aac4ea56d4d4ffbb41b

[Jialiu Lin, 2017-05-03 00:14:27]

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

[commit-bot: I haz the power, 2017-05-03 00:14:51]

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, 2017-05-03 00:25:57]

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

[commit-bot: I haz the power, 2017-05-03 00:25:57]

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

[Jialiu Lin, 2017-05-03 19:05:40]

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

[commit-bot: I haz the power, 2017-05-03 19:06:23]

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, 2017-05-03 19:59:49]

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

[commit-bot: I haz the power, 2017-05-03 19:59:50]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2017-05-03 23:56:51]

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

[commit-bot: I haz the power, 2017-05-03 23:57:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jialiu Lin, 2017-05-04 00:07:31]

Description was changed from

==========
Add finch control of user population in low reputation requests and
password reuse requests.

BUG=698899
==========

to

==========
Add Finch feature and parameter to control user population for low reputation
requests and password reuse requests.

BUG=698899
==========

[Jialiu Lin, 2017-05-04 00:07:43]

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

[commit-bot: I haz the power, 2017-05-04 00:08:14]

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, 2017-05-04 01:23:11]

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

[commit-bot: I haz the power, 2017-05-04 01:23:12]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2017-05-04 18:36:55]

Description was changed from

==========
Add Finch feature and parameter to control user population for low reputation
requests and password reuse requests.

BUG=698899
==========

to

==========
Add Finch feature and parameters to control user population for low reputation
requests and password reuse requests.

There are 3 experiment populations that corresponds to our launch plan:
(1) SBER && !incognito
(2) (SBER || Sync) && !incognito
(3) everyone

BUG=698899
==========

[Jialiu Lin, 2017-05-04 18:36:55]

jialiul@chromium.org changed reviewers:
+ nparker@chromium.org

[Jialiu Lin, 2017-05-05 04:58:38]

Hi nparker@, 
Could you take a look at this CL? Thanks!

[Jialiu Lin, 2017-05-05 18:01:33]

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

[commit-bot: I haz the power, 2017-05-05 18:02:51]

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, 2017-05-05 20:36:46]

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

[commit-bot: I haz the power, 2017-05-05 20:36:47]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Nathan Parker, 2017-05-05 20:58:22]

https://codereview.chromium.org/2856033002/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/2856033002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:87:
MOCK_METHOD0(IsHistorySyncEnabled, bool());
Are these used in this file?  Or do you need them just to return their default
value (false)?

https://codereview.chromium.org/2856033002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_password_protection_service.cc (right):

https://codereview.chromium.org/2856033002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_password_protection_service.cc:100: return
allowed_incognito && allowed_extended_reporting &&
So if we wanted to launch for everyone but not on incognito, I think we'd need
another param "allow_everyone." Is that right? I realize that makes it more
complicated, but we wouldn't want to be stuck unable to launch w/o a code
change.  WDYT?

https://codereview.chromium.org/2856033002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_password_protection_service.cc:108:
sync->GetActiveDataTypes().Has(syncer::HISTORY_DELETE_DIRECTIVES);
What does the HISTORY_DELETE_DIRECTIVES requirement do?  Can you add a comment?

https://codereview.chromium.org/2856033002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_password_protection_service.h (right):

https://codereview.chromium.org/2856033002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_password_protection_service.h:27:
FRIEND_TEST_ALL_PREFIXES(
nit: put these at the end of the protected (or private?) section so they're not
so prominent.

https://codereview.chromium.org/2856033002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_password_protection_service.h:47: bool
IsPingingEnabled(const base::Feature& feature) override;
What is |feature| here?  Maybe add a comment or name it more explicitly.

https://codereview.chromium.org/2856033002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc
(right):

https://codereview.chromium.org/2856033002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc:21:
bool IsExtendedReporting() override { return is_extended_reporting_; }
nit: order these the same as how they're listed below, so it's less likely
you'll mix them up.

https://codereview.chromium.org/2856033002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc:50:
static std::set<std::string> features = {feature.name};
Why is this static? This could cause tests to have side effects.  If it needs to
persist, how about making it a member variable?

https://codereview.chromium.org/2856033002/diff/80001/components/safe_browsin...
File components/safe_browsing/csd.proto (right):

https://codereview.chromium.org/2856033002/diff/80001/components/safe_browsin...
components/safe_browsing/csd.proto:41: // The finch active group this user
belongs to (if any). Active group is
We should at some point list the possible values of this, or point to the
google3 finch config name that lists them. That way when people are dremeling,
they know what to look for.

https://codereview.chromium.org/2856033002/diff/80001/components/safe_browsin...
components/safe_browsing/csd.proto:44: optional string finch_active_group = 3;
This seems useful, but then what is a concrete use case? And would it tell us if
a use is enabled for both pings or just how they got enabled for this ping?

https://codereview.chromium.org/2856033002/diff/80001/components/safe_browsin...
File components/safe_browsing/password_protection/password_protection_request.h
(right):

https://codereview.chromium.org/2856033002/diff/80001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_request.h:52:
NO_EXTENDED_REPORTING = 6,  // Deprecated.
You could comment these out, or rename them _DEPRECATED_something to ensure they
don't get used.

https://codereview.chromium.org/2856033002/diff/80001/components/safe_browsin...
File components/safe_browsing/password_protection/password_protection_service.cc
(right):

https://codereview.chromium.org/2856033002/diff/80001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.cc:324:
user_population->set_finch_active_group(field_trial->trial_name() + "|" +
Would it be useful to know the group name for both features? (could do that w/ a
repeated field).

https://codereview.chromium.org/2856033002/diff/80001/components/safe_browsin...
File components/safe_browsing/password_protection/password_protection_service.h
(right):

https://codereview.chromium.org/2856033002/diff/80001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.h:141:
HostContentSettingsMap* content_settings() { return content_settings_; }
nit: content_settings() const

[Jialiu Lin, 2017-05-05 22:17:46]

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

[commit-bot: I haz the power, 2017-05-05 22:18:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jialiu Lin, 2017-05-05 22:19:33]

Thanks!

https://codereview.chromium.org/2856033002/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/2856033002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:87:
MOCK_METHOD0(IsHistorySyncEnabled, bool());
On 2017/05/05 at 20:58:21, Nathan Parker wrote:
> Are these used in this file?  Or do you need them just to return their default
value (false)?

They are pure virtual functions in PasswordProtectionService, need to declare
them as mock functions to create instance of MockPasswordProtectionService.

https://codereview.chromium.org/2856033002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_password_protection_service.cc (right):

https://codereview.chromium.org/2856033002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_password_protection_service.cc:100: return
allowed_incognito && allowed_extended_reporting &&
On 2017/05/05 at 20:58:21, Nathan Parker wrote:
> So if we wanted to launch for everyone but not on incognito, I think we'd need
another param "allow_everyone." Is that right? I realize that makes it more
complicated, but we wouldn't want to be stuck unable to launch w/o a code
change.  WDYT?

Yes, "allow_everyone" is needed for that case.
Code updated accordingly.

https://codereview.chromium.org/2856033002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_password_protection_service.cc:108:
sync->GetActiveDataTypes().Has(syncer::HISTORY_DELETE_DIRECTIVES);
On 2017/05/05 at 20:58:21, Nathan Parker wrote:
> What does the HISTORY_DELETE_DIRECTIVES requirement do?  Can you add a
comment?

You got me... I copied this function from web_history_service_factory.cc
https://cs.chromium.org/chromium/src/chrome/browser/history/web_history_servi...

My educated guess: it is a fancy way of saying "HISTORY"  (because it is the
only enum value mentioned "history" in its name in ModelType.)  And the comment
above HISTORY_DELETE_DIRECTIVES isn't very helpful. 
https://cs.chromium.org/chromium/src/components/sync/base/model_type.h?type=c...

https://codereview.chromium.org/2856033002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_password_protection_service.h (right):

https://codereview.chromium.org/2856033002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_password_protection_service.h:27:
FRIEND_TEST_ALL_PREFIXES(
On 2017/05/05 at 20:58:21, Nathan Parker wrote:
> nit: put these at the end of the protected (or private?) section so they're
not so prominent.

Done.

https://codereview.chromium.org/2856033002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_password_protection_service.h:47: bool
IsPingingEnabled(const base::Feature& feature) override;
On 2017/05/05 at 20:58:21, Nathan Parker wrote:
> What is |feature| here?  Maybe add a comment or name it more explicitly.

Feature can be either kLowReputationPinging or kProtectedPasswordEntryPinging.
Comment added.

https://codereview.chromium.org/2856033002/diff/80001/components/safe_browsin...
File components/safe_browsing/csd.proto (right):

https://codereview.chromium.org/2856033002/diff/80001/components/safe_browsin...
components/safe_browsing/csd.proto:41: // The finch active group this user
belongs to (if any). Active group is
On 2017/05/05 at 20:58:22, Nathan Parker wrote:
> We should at some point list the possible values of this, or point to the
google3 finch config name that lists them. That way when people are dremeling,
they know what to look for.

ChromeUserPopulation message is not only used by PhishGuard. I am concerned that
this list could potentially grow very long if we are going to list all the
possible trial experiment names here.

https://codereview.chromium.org/2856033002/diff/80001/components/safe_browsin...
components/safe_browsing/csd.proto:44: optional string finch_active_group = 3;
On 2017/05/05 at 20:58:22, Nathan Parker wrote:
> This seems useful, but then what is a concrete use case? And would it tell us
if a use is enabled for both pings or just how they got enabled for this ping?

My current implementation does the later (how they got enabled for this ping).
Changed to a repeated field and report all the PhishGuard related trials this
user is in.

https://codereview.chromium.org/2856033002/diff/80001/components/safe_browsin...
File components/safe_browsing/password_protection/password_protection_request.h
(right):

https://codereview.chromium.org/2856033002/diff/80001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_request.h:52:
NO_EXTENDED_REPORTING = 6,  // Deprecated.
On 2017/05/05 at 20:58:22, Nathan Parker wrote:
> You could comment these out, or rename them _DEPRECATED_something to ensure
they don't get used.

Done.

https://codereview.chromium.org/2856033002/diff/80001/components/safe_browsin...
File components/safe_browsing/password_protection/password_protection_service.cc
(right):

https://codereview.chromium.org/2856033002/diff/80001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.cc:324:
user_population->set_finch_active_group(field_trial->trial_name() + "|" +
On 2017/05/05 at 20:58:22, Nathan Parker wrote:
> Would it be useful to know the group name for both features? (could do that w/
a repeated field).

SG. Changed to a repeated field.

https://codereview.chromium.org/2856033002/diff/80001/components/safe_browsin...
File components/safe_browsing/password_protection/password_protection_service.h
(right):

https://codereview.chromium.org/2856033002/diff/80001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.h:141:
HostContentSettingsMap* content_settings() { return content_settings_; }
On 2017/05/05 at 20:58:22, Nathan Parker wrote:
> nit: content_settings() const

Done.

[Jialiu Lin, 2017-05-05 22:46:25]

jialiul@chromium.org changed reviewers:
+ dvadym@chromium.org, isherman@chromium.org

[Jialiu Lin, 2017-05-05 22:46:26]

+dvadym@ for chrome/browser/password_manager/*
+isherman@ for testing/variations/fieldtrial_testing_config.json

[Ilya Sherman, 2017-05-05 22:49:31]

https://codereview.chromium.org/2856033002/diff/120001/testing/variations/fie...
File testing/variations/fieldtrial_testing_config.json (right):

https://codereview.chromium.org/2856033002/diff/120001/testing/variations/fie...
testing/variations/fieldtrial_testing_config.json:1962: },
Only a single experiment per study will actually get test coverage, so there's
no need to list multiple experiments here.  Instead, please list just the
variation that is most likely to ship.  If/when that changes, you can update the
one that gets broad test coverage.

[Nathan Parker, 2017-05-05 22:51:41]

LGTM

https://codereview.chromium.org/2856033002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/chrome_password_protection_service.cc (right):

https://codereview.chromium.org/2856033002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/chrome_password_protection_service.cc:92: if
(!allowed_all_population) {
nit (or not, you decide which you like better):

if (allowed_all_poplation)
  return true;
...
...
return false.

https://codereview.chromium.org/2856033002/diff/100001/components/safe_browsi...
File components/safe_browsing/password_protection/password_protection_request.h
(right):

https://codereview.chromium.org/2856033002/diff/100001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_request.h:52:
DEPRECATED_NO_EXTENDED_REPORTING = 6,
Should change the names in histograms.xml too, to avoid confusion.

[Jialiu Lin, 2017-05-05 22:58:52]

@isherman, last patch also added some minor change in histograms.xml. PTAL

Thanks!

https://codereview.chromium.org/2856033002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/chrome_password_protection_service.cc (right):

https://codereview.chromium.org/2856033002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/chrome_password_protection_service.cc:92: if
(!allowed_all_population) {
On 2017/05/05 at 22:51:41, Nathan Parker wrote:
> nit (or not, you decide which you like better):
> 
> if (allowed_all_poplation)
>   return true;
> ...
> ...
> return false.

Acknowledged.

https://codereview.chromium.org/2856033002/diff/100001/components/safe_browsi...
File components/safe_browsing/password_protection/password_protection_request.h
(right):

https://codereview.chromium.org/2856033002/diff/100001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_request.h:52:
DEPRECATED_NO_EXTENDED_REPORTING = 6,
On 2017/05/05 at 22:51:41, Nathan Parker wrote:
> Should change the names in histograms.xml too, to avoid confusion.

histograms.xml updated

https://codereview.chromium.org/2856033002/diff/120001/testing/variations/fie...
File testing/variations/fieldtrial_testing_config.json (right):

https://codereview.chromium.org/2856033002/diff/120001/testing/variations/fie...
testing/variations/fieldtrial_testing_config.json:1962: },
On 2017/05/05 at 22:49:31, Ilya Sherman wrote:
> Only a single experiment per study will actually get test coverage, so there's
no need to list multiple experiments here.  Instead, please list just the
variation that is most likely to ship.  If/when that changes, you can update the
one that gets broad test coverage.

Good to know. in that case, I'll keep the first experiment for each study.
Thanks!

[Ilya Sherman, 2017-05-05 23:28:22]

Metrics and field trial changes lgtm, thanks

[dvadym, 2017-05-08 11:12:27]

chrome/browser/password_manager/* LGTM

[Jialiu Lin, 2017-05-08 17:19:24]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2017-05-08 17:19:24]

The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/2856033002/#ps140001
(title: "comments from isherman and nparker")

[commit-bot: I haz the power, 2017-05-08 17:19:53]

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, 2017-05-08 17:24:06]

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

[commit-bot: I haz the power, 2017-05-08 17:24:08]

Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Jialiu Lin, 2017-05-08 17:34:54]

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

[commit-bot: I haz the power, 2017-05-08 17:35:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jialiu Lin, 2017-05-08 18:13:05]

The CQ bit was unchecked by jialiul@chromium.org

[Jialiu Lin, 2017-05-08 18:13:08]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2017-05-08 18:13:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, dvadym@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2856033002/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-08 18:13:48]

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, 2017-05-09 04:36:58]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494267188388940,
"parent_rev": "dbf45973be46d9dd01f8d7d3dadddef0eeeaff06", "commit_rev":
"fe7f0c917485514e7ce80aac4ea56d4d4ffbb41b"}

[commit-bot: I haz the power, 2017-05-09 04:37:10]

Description was changed from

==========
Add Finch feature and parameters to control user population for low reputation
requests and password reuse requests.

There are 3 experiment populations that corresponds to our launch plan:
(1) SBER && !incognito
(2) (SBER || Sync) && !incognito
(3) everyone

BUG=698899
==========

to

==========
Add Finch feature and parameters to control user population for low reputation
requests and password reuse requests.

There are 3 experiment populations that corresponds to our launch plan:
(1) SBER && !incognito
(2) (SBER || Sync) && !incognito
(3) everyone

BUG=698899

Review-Url: https://codereview.chromium.org/2856033002
Cr-Commit-Position: refs/heads/master@{#470212}
Committed:
https://chromium.googlesource.com/chromium/src/+/fe7f0c917485514e7ce80aac4ea5...
==========

[commit-bot: I haz the power, 2017-05-09 04:37:12]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/fe7f0c917485514e7ce80aac4ea5...