[Password Manager] Switch password manager code to use the Feature framework.

[Password Manager] Switch password manager code to use the Feature framework.

The FeatureList APIs replace the current practice for enabling/disabling
switches and offer expressing the default state of a feature in a structured
manner.

This CL does the refactoring for the password manager command-line switches to
use the FeatureList APIs.

BUG=561322
Committed: https://crrev.com/3b43e5a81c83605870ba5406143fcc5bb885890b
Cr-Commit-Position: refs/heads/master@{#380611}

[Pritam Nikam, 2016-02-08 11:53:33]

Description was changed from

==========
[Password Manager] Switch password manager code to use "base/feature_list.h".

Changes so far:
  A. 'allow-autofill-sync-credential' feature.
  B. 'disallow-autofill-sync-credential' feature.
  C. 'disallow-autofill-sync-credential-for-reauth' feature.

BUG=561322
==========

to

==========
[Password Manager] Switch password manager code to use "base/feature_list.h".

Changes so far:
  A. 'allow-autofill-sync-credential' feature.
  B. 'disallow-autofill-sync-credential' feature.
  C. 'disallow-autofill-sync-credential-for-reauth' feature.
      A. 'allow-autofill-sync-credential' feature.
      B. 'disallow-autofill-sync-credential' feature.
      C. 'disallow-autofill-sync-credential-for-reauth' feature.
      D. 'disable-affiliation-based-matching' feature.
      E. 'enable-affiliation-based-matching' feature.

BUG=561322
==========

[Pritam Nikam, 2016-02-08 11:54:12]

Description was changed from

==========
[Password Manager] Switch password manager code to use "base/feature_list.h".

Changes so far:
  A. 'allow-autofill-sync-credential' feature.
  B. 'disallow-autofill-sync-credential' feature.
  C. 'disallow-autofill-sync-credential-for-reauth' feature.
      A. 'allow-autofill-sync-credential' feature.
      B. 'disallow-autofill-sync-credential' feature.
      C. 'disallow-autofill-sync-credential-for-reauth' feature.
      D. 'disable-affiliation-based-matching' feature.
      E. 'enable-affiliation-based-matching' feature.

BUG=561322
==========

to

==========
[Password Manager] Switch password manager code to use "base/feature_list.h".

Changes so far:
  A. 'allow-autofill-sync-credential' feature.
  B. 'disallow-autofill-sync-credential' feature.
  C. 'disallow-autofill-sync-credential-for-reauth' feature.
  D. 'disable-affiliation-based-matching' feature.
  E. 'enable-affiliation-based-matching' feature.

BUG=561322
==========

[Pritam Nikam, 2016-02-08 13:46:13]

Description was changed from

==========
[Password Manager] Switch password manager code to use "base/feature_list.h".

Changes so far:
  A. 'allow-autofill-sync-credential' feature.
  B. 'disallow-autofill-sync-credential' feature.
  C. 'disallow-autofill-sync-credential-for-reauth' feature.
  D. 'disable-affiliation-based-matching' feature.
  E. 'enable-affiliation-based-matching' feature.

BUG=561322
==========

to

==========
[Password Manager] Switch password manager code to use "base/feature_list.h".

Changes so far:
  A. 'allow-autofill-sync-credential' feature.
  B. 'disallow-autofill-sync-credential' feature.
  C. 'disallow-autofill-sync-credential-for-reauth' feature.
  D. 'disable-affiliation-based-matching' feature.
  E. 'enable-affiliation-based-matching' feature.
  F. 'disable-drop-sync-credential' feature.
  G. 'enable-drop-sync-credential' feature.
  H. 'disable-manager-for-sync-signin' feature.
  I. 'enable-manager-for-sync-signin' feature.

BUG=561322
==========

[Pritam Nikam, 2016-02-08 13:46:13]

pritam.nikam@samsung.com changed reviewers:
+ vabr@chromium.org, vivekg@chromium.org

[Pritam Nikam, 2016-02-08 13:47:36]

Hi Vaclav,
This one is a long standing follow-up, PTAL.
Thanks!

[vabr (Chromium), 2016-02-08 14:16:49]

Very nice, LGTM!

Thanks for the clean-up and modernisation.

Cheers,
Vaclav

https://codereview.chromium.org/1668523002/diff/80001/components/password_man...
File
components/password_manager/sync/browser/sync_credentials_filter_unittest.cc
(right):

https://codereview.chromium.org/1668523002/diff/80001/components/password_man...
components/password_manager/sync/browser/sync_credentials_filter_unittest.cc:50:
void EnableFeature(const base::Feature& feature) {
optional: Instead of duplicating it here and in the affiliation utils test, you
could consider adding this function to
components/password_manager/core/browser/password_manager_test_utils.*

[Pritam Nikam, 2016-02-08 15:27:19]

Thanks Vaclav! New patch-set addresses the review inputs. PTAL.
Thanks!

https://codereview.chromium.org/1668523002/diff/80001/components/password_man...
File
components/password_manager/sync/browser/sync_credentials_filter_unittest.cc
(right):

https://codereview.chromium.org/1668523002/diff/80001/components/password_man...
components/password_manager/sync/browser/sync_credentials_filter_unittest.cc:50:
void EnableFeature(const base::Feature& feature) {
On 2016/02/08 14:16:49, vabr (Chromium) wrote:
> optional: Instead of duplicating it here and in the affiliation utils test,
you
> could consider adding this function to
> components/password_manager/core/browser/password_manager_test_utils.*

Done.

[Pritam Nikam, 2016-02-08 15:27:34]

The CQ bit was checked by pritam.nikam@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-08 15:28:01]

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

[vabr (Chromium), 2016-02-08 15:29:59]

lgtm

[commit-bot: I haz the power, 2016-02-08 15:45:26]

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

[commit-bot: I haz the power, 2016-02-08 15:45:27]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Pritam Nikam, 2016-02-09 06:55:07]

The CQ bit was checked by pritam.nikam@samsung.com

[Pritam Nikam, 2016-02-09 06:55:07]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/1668523002/#ps120001
(title: "Fixes mac bot breakages.")

[commit-bot: I haz the power, 2016-02-09 06:55:24]

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

[commit-bot: I haz the power, 2016-02-09 07:05:33]

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

[commit-bot: I haz the power, 2016-02-09 07:05:34]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Pritam Nikam, 2016-02-09 07:07:52]

The CQ bit was checked by pritam.nikam@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-09 07:08:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1668523002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1668523002/120001

[Pritam Nikam, 2016-02-09 07:24:24]

pritam.nikam@samsung.com changed reviewers:
+ bauerb@chromium.org

[Pritam Nikam, 2016-02-09 07:24:24]

Hi bauerb@chromium.org: Please review changes in 
chrome/browser/android/password_ui_view_android.cc, 
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc

Thanks!

[commit-bot: I haz the power, 2016-02-09 08:53:54]

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

[commit-bot: I haz the power, 2016-02-09 08:53:55]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-02-09 15:10:32]

bauerb@chromium.org changed reviewers:
+ asvitkine@chromium.org, isherman@chromium.org

[Bernhard Bauer, 2016-02-09 15:10:33]

Not LGTM -- this is not how features are supposed to be used. Features already
encapsulate all the logic about enabling or disabling, so that instead of
multiple command line switches, you can use a single feature (otherwise what
would be the point?). This does mean that command line switches don't just get
directly translated to features. Please take a look at how other features (for
example in the password manager) are implemented, or at base/feature_list.h.

kDisallowAutofillSyncCredentialForReauth is an interesting case, because
base::FeatureList at the moment only supports an Enabled/Disabled state, not
three states. Alexei, Ilya, what do you think would be the best thing to do
here?

https://codereview.chromium.org/1668523002/diff/120001/components/password_ma...
File components/password_manager/core/browser/password_manager_test_utils.cc
(right):

https://codereview.chromium.org/1668523002/diff/120001/components/password_ma...
components/password_manager/core/browser/password_manager_test_utils.cc:99: void
EnableFeature(const char* feature) {
Why does this take a char* instead of a Feature?

[Pritam Nikam, 2016-02-09 16:05:54]

Thanks Bernhard for review.

I was sceptical about adding below flags. I guess we can club these into 3 flags
instead:
  D. 'disable-affiliation-based-matching' feature.
  E. 'enable-affiliation-based-matching' feature.

  F. 'disable-drop-sync-credential' feature.
  G. 'enable-drop-sync-credential' feature.

  H. 'disable-manager-for-sync-signin' feature.
  I. 'enable-manager-for-sync-signin' feature.

Thanks!

https://codereview.chromium.org/1668523002/diff/120001/components/password_ma...
File components/password_manager/core/browser/password_manager_test_utils.cc
(right):

https://codereview.chromium.org/1668523002/diff/120001/components/password_ma...
components/password_manager/core/browser/password_manager_test_utils.cc:99: void
EnableFeature(const char* feature) {
On 2016/02/09 15:10:33, Bernhard Bauer wrote:
> Why does this take a char* instead of a Feature?

base::FeatureList::InitializeFromCommandLine() accepts 2 std::strings as
parameters:|enable_features| & |disable_features| [1].

--enable-features=Feature5,Feature7
--disable-features=Feature1,Feature2,Feature3

As plan was to enable only single feature for tests, I kept it as |char*| i.e. 
Feature.name [2].

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/feature_list....
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/base/feature_list....

[Bernhard Bauer, 2016-02-09 16:13:31]

On 2016/02/09 16:05:54, Pritam Nikam wrote:
> Thanks Bernhard for review.
> 
> I was sceptical about adding below flags. I guess we can club these into 3
flags
> instead:
>   D. 'disable-affiliation-based-matching' feature.
>   E. 'enable-affiliation-based-matching' feature.
> 
>   F. 'disable-drop-sync-credential' feature.
>   G. 'enable-drop-sync-credential' feature.
> 
>   H. 'disable-manager-for-sync-signin' feature.
>   I. 'enable-manager-for-sync-signin' feature.
> 
> Thanks!

You mean 3 *features*?
* affiliation-based-matching
* drop-sync-credential
* manager-for-sync-signin

(The other features in there shouldn't really start with "enable-" either I
think, so that's not a good example to follow. Sorry for the confusion.)

https://codereview.chromium.org/1668523002/diff/120001/components/password_ma...
File components/password_manager/core/browser/password_manager_test_utils.cc
(right):

https://codereview.chromium.org/1668523002/diff/120001/components/password_ma...
components/password_manager/core/browser/password_manager_test_utils.cc:99: void
EnableFeature(const char* feature) {
On 2016/02/09 16:05:54, Pritam Nikam wrote:
> On 2016/02/09 15:10:33, Bernhard Bauer wrote:
> > Why does this take a char* instead of a Feature?
> 
> base::FeatureList::InitializeFromCommandLine() accepts 2 std::strings as
> parameters:|enable_features| & |disable_features| [1].
> 
> --enable-features=Feature5,Feature7
> --disable-features=Feature1,Feature2,Feature3
> 
> As plan was to enable only single feature for tests, I kept it as |char*| i.e.

> Feature.name [2].
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/feature_list....
> [2]
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/feature_list....

Yes, but you could still take a Feature here and extract its name for
InitializeFromCommandLine() here.

[Alexei Svitkine (slow), 2016-02-09 16:30:33]

https://codereview.chromium.org/1668523002/diff/120001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1668523002/diff/120001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1061:
password_manager::features::kDisableAffiliationBasedMatching.name)},
Yeah, this doesn't make sense.

First, there should be 1 feature to replace a pair of enable/disable flags, not
2. But second, this code is specifying a command-line based control for this
about:flags entry - which wouldn't actually activate the feature. (i.e. if your
feature name is FooBar, passing --FooBar to Chrome doesn't activate it). You
need to use FEATURE_VALUE_TYPE() and pass in the feature struct.

[vabr (Chromium), 2016-02-10 10:52:17]

Thanks Alexei and Berhard for chiming in, I should have checked properly what
Features are before approving this change.

At this point, I can only confirm that indeed the flags should become 3 binary
and 1 ternary feature:

binary (enabled/disabled):
* affiliation-based-matching
* drop-sync-credential
* manager-for-sync-signin

ternary (never/always/not for reauth):
* fill-sync-credential

Because, as Bernhard noted, it looks like base::Feature has being binary
hard-coded into itself, I'd be fine with also splitting the last one into:
* fill-sync-credential (enabled/disabled)
* fill-sync-credential-on-reauth (enabled/disabled)
with the provision that the first one being "enabled" implies the second one
also "enabled" (the code could ignore disabling the second one and emit a
warning in that case).

Thanks,
Vaclav

[Bernhard Bauer, 2016-02-10 10:58:01]

On 2016/02/10 10:52:17, vabr (Chromium) wrote:
> Thanks Alexei and Berhard for chiming in, I should have checked properly what
> Features are before approving this change.
> 
> At this point, I can only confirm that indeed the flags should become 3 binary
> and 1 ternary feature:
> 
> binary (enabled/disabled):
> * affiliation-based-matching
> * drop-sync-credential
> * manager-for-sync-signin
> 
> ternary (never/always/not for reauth):
> * fill-sync-credential
> 
> Because, as Bernhard noted, it looks like base::Feature has being binary
> hard-coded into itself, I'd be fine with also splitting the last one into:
> * fill-sync-credential (enabled/disabled)
> * fill-sync-credential-on-reauth (enabled/disabled)
> with the provision that the first one being "enabled" implies the second one
> also "enabled" (the code could ignore disabling the second one and emit a
> warning in that case).

Just as a general point of advice, I would try to make it very explicit what
these two features do and how they interact. For example, one could interpret
fill-sync-credential as a master switch that turns the whole feature off, and
fill-sync-credential-on-reauth as a switch for just the reauth case, which is
different from what you described. Warning in case of an inconsistency is
definitely good though!

[vabr (Chromium), 2016-02-10 11:55:06]

On 2016/02/10 10:58:01, Bernhard Bauer wrote:
> On 2016/02/10 10:52:17, vabr (Chromium) wrote:
> > Thanks Alexei and Berhard for chiming in, I should have checked properly
what
> > Features are before approving this change.
> > 
> > At this point, I can only confirm that indeed the flags should become 3
binary
> > and 1 ternary feature:
> > 
> > binary (enabled/disabled):
> > * affiliation-based-matching
> > * drop-sync-credential
> > * manager-for-sync-signin
> > 
> > ternary (never/always/not for reauth):
> > * fill-sync-credential
> > 
> > Because, as Bernhard noted, it looks like base::Feature has being binary
> > hard-coded into itself, I'd be fine with also splitting the last one into:
> > * fill-sync-credential (enabled/disabled)
> > * fill-sync-credential-on-reauth (enabled/disabled)
> > with the provision that the first one being "enabled" implies the second one
> > also "enabled" (the code could ignore disabling the second one and emit a
> > warning in that case).
> 
> Just as a general point of advice, I would try to make it very explicit what
> these two features do and how they interact. For example, one could interpret
> fill-sync-credential as a master switch that turns the whole feature off, and
> fill-sync-credential-on-reauth as a switch for just the reauth case, which is
> different from what you described. Warning in case of an inconsistency is
> definitely good though!

Right, I believe I myself got a bit confused. As the purpose of the feature is
to
block filling sync, rather than enabling it (it singles out the sync
credential),
maybe we could rename the feature and invert the meaning. Then we would have:

* protect-sync-credential disabled & protect-sync-credential-on-reauth disabled
= fill sync credential everywhere
* protect-sync-credential disabled & protect-sync-credential-on-reauth enabled =
fill sync credential everywhere but on reauth
* protect-sync-credential enabled & protect-sync-credential-on-reauth enabled =
do not ever fill sync credential
* protect-sync-credential enabled & protect-sync-credential-on-reauth disabled =
this is "illegal", emit a warning and do not ever fill the sync credential

[Pritam Nikam, 2016-02-11 10:51:58]

Thanks all for review. New patch set incorporates the review inputs, PTAL.

Thanks!


On 2016/02/10 11:55:06, vabr (Chromium) wrote:
> On 2016/02/10 10:58:01, Bernhard Bauer wrote:
> > On 2016/02/10 10:52:17, vabr (Chromium) wrote:
> > > Thanks Alexei and Berhard for chiming in, I should have checked properly
> what
> > > Features are before approving this change.
> > > 
> > > At this point, I can only confirm that indeed the flags should become 3
> binary
> > > and 1 ternary feature:
> > > 
> > > binary (enabled/disabled):
> > > * affiliation-based-matching
> > > * drop-sync-credential
> > > * manager-for-sync-signin
> > > 
> > > ternary (never/always/not for reauth):
> > > * fill-sync-credential
> > > 
> > > Because, as Bernhard noted, it looks like base::Feature has being binary
> > > hard-coded into itself, I'd be fine with also splitting the last one into:
> > > * fill-sync-credential (enabled/disabled)
> > > * fill-sync-credential-on-reauth (enabled/disabled)
> > > with the provision that the first one being "enabled" implies the second
one
> > > also "enabled" (the code could ignore disabling the second one and emit a
> > > warning in that case).
> > 
> > Just as a general point of advice, I would try to make it very explicit what
> > these two features do and how they interact. For example, one could
interpret
> > fill-sync-credential as a master switch that turns the whole feature off,
and
> > fill-sync-credential-on-reauth as a switch for just the reauth case, which
is
> > different from what you described. Warning in case of an inconsistency is
> > definitely good though!
> 
> Right, I believe I myself got a bit confused. As the purpose of the feature is
> to
> block filling sync, rather than enabling it (it singles out the sync
> credential),
> maybe we could rename the feature and invert the meaning. Then we would have:
> 
> * protect-sync-credential disabled & protect-sync-credential-on-reauth
disabled
> = fill sync credential everywhere
> * protect-sync-credential disabled & protect-sync-credential-on-reauth enabled
=
> fill sync credential everywhere but on reauth
> * protect-sync-credential enabled & protect-sync-credential-on-reauth enabled
=
> do not ever fill sync credential
> * protect-sync-credential enabled & protect-sync-credential-on-reauth disabled
=
> this is "illegal", emit a warning and do not ever fill the sync credential

Done.

https://codereview.chromium.org/1668523002/diff/120001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1668523002/diff/120001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1061:
password_manager::features::kDisableAffiliationBasedMatching.name)},
On 2016/02/09 16:30:33, Alexei Svitkine wrote:
> Yeah, this doesn't make sense.
> 
> First, there should be 1 feature to replace a pair of enable/disable flags,
not
> 2. But second, this code is specifying a command-line based control for this
> about:flags entry - which wouldn't actually activate the feature. (i.e. if
your
> feature name is FooBar, passing --FooBar to Chrome doesn't activate it). You
> need to use FEATURE_VALUE_TYPE() and pass in the feature struct.

Done.

https://codereview.chromium.org/1668523002/diff/120001/components/password_ma...
File components/password_manager/core/browser/password_manager_test_utils.cc
(right):

https://codereview.chromium.org/1668523002/diff/120001/components/password_ma...
components/password_manager/core/browser/password_manager_test_utils.cc:99: void
EnableFeature(const char* feature) {
On 2016/02/09 16:13:31, Bernhard Bauer wrote:
> On 2016/02/09 16:05:54, Pritam Nikam wrote:
> > On 2016/02/09 15:10:33, Bernhard Bauer wrote:
> > > Why does this take a char* instead of a Feature?
> > 
> > base::FeatureList::InitializeFromCommandLine() accepts 2 std::strings as
> > parameters:|enable_features| & |disable_features| [1].
> > 
> > --enable-features=Feature5,Feature7
> > --disable-features=Feature1,Feature2,Feature3
> > 
> > As plan was to enable only single feature for tests, I kept it as |char*|
i.e.
> 
> > Feature.name [2].
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/feature_list....
> > [2]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/feature_list....
> 
> Yes, but you could still take a Feature here and extract its name for
> InitializeFromCommandLine() here.

Done.

[Bernhard Bauer, 2016-02-11 11:12:41]

https://codereview.chromium.org/1668523002/diff/180001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/1668523002/diff/180001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:575:
password_manager::features::kManagerForSyncSignin)) {
I don't think this is necessary; FeatureList will take care of that.

(This also applies to all the other calls to this method in this CL, so the
whole method can be removed.)

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
File components/password_manager/core/browser/affiliation_utils_unittest.cc
(right):

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
components/password_manager/core/browser/affiliation_utils_unittest.cc:206:
TEST(AffiliationUtilsTest, IsAffiliationBasedMatchingEnabled) {
TBH, I think these tests now need much fewer test cases, because FeatureList
itself is not under test here and there is now less logic in the password
manager code. Really, I think we only need to test that
IsAffiliationBasedMatchingEnabled() correctly reflects the state of the
kAffiliationBasedMatching feature, regardless of how that is set. I defer to
Václav though.

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
File components/password_manager/core/browser/password_manager_test_utils.cc
(right):

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
components/password_manager/core/browser/password_manager_test_utils.cc:102:
std::string enable_overrides, disable_overrides;
Nit: Declare each variable on a separate line.

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
File components/password_manager/core/browser/password_manager_test_utils.h
(right):

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
components/password_manager/core/browser/password_manager_test_utils.h:77: //
Helper function to set command-line |feature|. It |appends_to_list| of
Nit: The description of the |appends_to_list| parameter doesn't make much sense
-- it's a boolean that states *whether* the method is supposed to append to the
list, which implies that it won't do that if the value is false, but the comment
makes it appear as if it always would do that.

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
File components/password_manager/core/common/password_manager_features.cc
(right):

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
components/password_manager/core/common/password_manager_features.cc:28:
"enable-drop-sync-credential", base::FEATURE_DISABLED_BY_DEFAULT};
These two features are unused and redundant.

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
File components/password_manager/sync/browser/sync_credentials_filter.cc
(right):

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
components/password_manager/sync/browser/sync_credentials_filter.cc:109: } else
if (protect_sync_credential_on_reauth_disabled) {
You don't need an else if you return in the branch before.

[vabr (Chromium), 2016-02-11 12:30:21]

Hi Priam Nikam,

I've taken a deeper look at the features. We should try to make more use of the
Feature framework, in particular in the IsFeatureEnabled method, which can
handle command-line flag and Field trial and default settings for us. I'm not
completely sure, how to do the Field trial overrides, so figuring that out stays
with you as the kind volunteer to do this change. :)

(@asvitkine -- is there any documentation for how to tell the Feature what Field
trial groups control it? The header files in //base do not seem to mention this
much.)

I compiled an overview of what the features' default values and Field trial
dependencies should be, based on the current code:

affiliation-based-matching:
* default = on
* Field trial name = "AffiliationBasedMatching"
  - groups starting with "Disabled" turns the feature off
  - other groups turn it on

drop-sync-credential
* default = on
* Field trial name = "PasswordManagerDropSyncCredential"
  - group "Disabled" turns the feature off
  - other groups turn it on

manager-for-sync-signin
* obsolete: http://crbug.com/586107

protect-sync-credential
* default = off
* Field trial name = "AutofillSyncCredential"
  - group "DisallowSyncCredentials" turns the feature on
  - other groups turn it off

protect-sync-credential-on-reauth
* default = off
* Field trial name = "AutofillSyncCredential"
  - groups "DisallowSyncCredentials" and "DisallowSyncCredentialsForReauth" turn
the feature on
  - other groups turn it off

Cheers,
Vaclav

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
File components/password_manager/core/common/password_manager_features.cc
(right):

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
components/password_manager/core/common/password_manager_features.cc:17: //
Enables dropping the credential used to sync passwords.
nit: Reformulate for clarity:
// Drop the sync credential if captured for saving, do not offer it for saving.

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
components/password_manager/core/common/password_manager_features.cc:50: const
base::Feature kManagerForSyncSignin = {"manager-for-sync-signin",
This one is actually obsolete and ready for removal: http://crbug.com/586107.

However, that removal should be done in a separate CL.

[vabr (Chromium), 2016-02-11 12:33:40]

On 2016/02/11 12:30:21, vabr (Chromium) wrote:
> Hi Priam Nikam,
> 
> I've taken a deeper look at the features. We should try to make more use of
the
> Feature framework, in particular in the IsFeatureEnabled method, which can
> handle command-line flag and Field trial and default settings for us. I'm not
> completely sure, how to do the Field trial overrides, so figuring that out
stays
> with you as the kind volunteer to do this change. :)

Just to be clear, what I mean, an example:
SyncCredentialsFilter::GetAutofillForSyncCredentialsState, instead of today's
10+ lines should be able to just consist of calling IsEnabled on the two
features, protect-sync-credential-on-reauth and protect-sync-credential and
returning one of the 3 apropriate states.

[Bernhard Bauer, 2016-02-11 12:33:49]

On 2016/02/11 12:30:21, vabr (Chromium) wrote:
> Hi Priam Nikam,
> 
> I've taken a deeper look at the features. We should try to make more use of
the
> Feature framework, in particular in the IsFeatureEnabled method, which can
> handle command-line flag and Field trial and default settings for us. I'm not
> completely sure, how to do the Field trial overrides, so figuring that out
stays
> with you as the kind volunteer to do this change. :)
> 
> (@asvitkine -- is there any documentation for how to tell the Feature what
Field
> trial groups control it? The header files in //base do not seem to mention
this
> much.)

In case Alexei isn't up yet: FeatureList::RegisterFieldTrialOverride
[https://code.google.com/p/chromium/codesearch#chromium/src/base/feature_list.cc&l=81&gs=cpp:base::class-FeatureList::RegisterFieldTrialOverride(const%2520std::basic_string%253Cchar,%2520std::char_traits%253Cchar%253E,%2520std::allocator%253Cchar%253E%2520%253E%2520&,%2520base::FeatureList::OverrideState,%2520base::FieldTrial%2520*)@chromium/../../base/feature_list.cc%257Cdef&gsn=RegisterFieldTrialOverride&ct=xref_usages]
seems to do that.

> I compiled an overview of what the features' default values and Field trial
> dependencies should be, based on the current code:
> 
> affiliation-based-matching:
> * default = on
> * Field trial name = "AffiliationBasedMatching"
>   - groups starting with "Disabled" turns the feature off
>   - other groups turn it on
> 
> drop-sync-credential
> * default = on
> * Field trial name = "PasswordManagerDropSyncCredential"
>   - group "Disabled" turns the feature off
>   - other groups turn it on
> 
> manager-for-sync-signin
> * obsolete: http://crbug.com/586107
> 
> protect-sync-credential
> * default = off
> * Field trial name = "AutofillSyncCredential"
>   - group "DisallowSyncCredentials" turns the feature on
>   - other groups turn it off
> 
> protect-sync-credential-on-reauth
> * default = off
> * Field trial name = "AutofillSyncCredential"
>   - groups "DisallowSyncCredentials" and "DisallowSyncCredentialsForReauth"
turn
> the feature on
>   - other groups turn it off
> 
> Cheers,
> Vaclav
> 
>
https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
> File components/password_manager/core/common/password_manager_features.cc
> (right):
> 
>
https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
> components/password_manager/core/common/password_manager_features.cc:17: //
> Enables dropping the credential used to sync passwords.
> nit: Reformulate for clarity:
> // Drop the sync credential if captured for saving, do not offer it for
saving.
> 
>
https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
> components/password_manager/core/common/password_manager_features.cc:50: const
> base::Feature kManagerForSyncSignin = {"manager-for-sync-signin",
> This one is actually obsolete and ready for removal: http://crbug.com/586107.
> 
> However, that removal should be done in a separate CL.

[Bernhard Bauer, 2016-02-11 12:34:56]

On 2016/02/11 12:33:49, Bernhard Bauer wrote:
> On 2016/02/11 12:30:21, vabr (Chromium) wrote:
> > Hi Priam Nikam,
> > 
> > I've taken a deeper look at the features. We should try to make more use of
> the
> > Feature framework, in particular in the IsFeatureEnabled method, which can
> > handle command-line flag and Field trial and default settings for us. I'm
not
> > completely sure, how to do the Field trial overrides, so figuring that out
> stays
> > with you as the kind volunteer to do this change. :)
> > 
> > (@asvitkine -- is there any documentation for how to tell the Feature what
> Field
> > trial groups control it? The header files in //base do not seem to mention
> this
> > much.)
> 
> In case Alexei isn't up yet: FeatureList::RegisterFieldTrialOverride
>
[https://code.google.com/p/chromium/codesearch#chromium/src/base/feature_list.cc&l=81&gs=cpp:base::class-FeatureList::RegisterFieldTrialOverride(const%2520std::basic_string%253Cchar,%2520std::char_traits%253Cchar%253E,%2520std::allocator%253Cchar%253E%2520%253E%2520&,%2520base::FeatureList::OverrideState,%2520base::FieldTrial%2520*)@chromium/../../base/feature_list.cc%257Cdef&gsn=RegisterFieldTrialOverride&ct=xref_usages]
> seems to do that.

(That link in clickable: https://goo.gl/X3HDzk)

[Pritam Nikam, 2016-02-11 14:56:04]

On 2016/02/11 12:34:56, Bernhard Bauer wrote:
> On 2016/02/11 12:33:49, Bernhard Bauer wrote:
> > On 2016/02/11 12:30:21, vabr (Chromium) wrote:
> > > Hi Priam Nikam,
> > > 
> > > I've taken a deeper look at the features. We should try to make more use
of
> > the
> > > Feature framework, in particular in the IsFeatureEnabled method, which can
> > > handle command-line flag and Field trial and default settings for us. I'm
> not
> > > completely sure, how to do the Field trial overrides, so figuring that out
> > stays
> > > with you as the kind volunteer to do this change. :)
> > > 
> > > (@asvitkine -- is there any documentation for how to tell the Feature what
> > Field
> > > trial groups control it? The header files in //base do not seem to mention
> > this
> > > much.)
> > 
> > In case Alexei isn't up yet: FeatureList::RegisterFieldTrialOverride
> >
>
[https://code.google.com/p/chromium/codesearch#chromium/src/base/feature_list.cc&l=81&gs=cpp:base::class-FeatureList::RegisterFieldTrialOverride(const%2520std::basic_string%253Cchar,%2520std::char_traits%253Cchar%253E,%2520std::allocator%253Cchar%253E%2520%253E%2520&,%2520base::FeatureList::OverrideState,%2520base::FieldTrial%2520*)@chromium/../../base/feature_list.cc%257Cdef&gsn=RegisterFieldTrialOverride&ct=xref_usages]
> > seems to do that.
> 
> (That link in clickable: https://goo.gl/X3HDzk)

Thanks Bernhard for sharing, I'll go through it.
Thanks!

[Pritam Nikam, 2016-02-25 14:16:07]

Hi Vaclav & Bernhard,
Sorry for the delayed update, I was away for some time. I've addressed review
comments in new patch set. Please have a look.
Thanks!

https://codereview.chromium.org/1668523002/diff/180001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/1668523002/diff/180001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:575:
password_manager::features::kManagerForSyncSignin)) {
On 2016/02/11 11:12:40, Bernhard Bauer wrote:
> I don't think this is necessary; FeatureList will take care of that.
> 
> (This also applies to all the other calls to this method in this CL, so the
> whole method can be removed.)

Done.

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
File components/password_manager/core/browser/password_manager_test_utils.cc
(right):

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
components/password_manager/core/browser/password_manager_test_utils.cc:102:
std::string enable_overrides, disable_overrides;
On 2016/02/11 11:12:40, Bernhard Bauer wrote:
> Nit: Declare each variable on a separate line.

Done.

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
File components/password_manager/core/browser/password_manager_test_utils.h
(right):

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
components/password_manager/core/browser/password_manager_test_utils.h:77: //
Helper function to set command-line |feature|. It |appends_to_list| of
On 2016/02/11 11:12:40, Bernhard Bauer wrote:
> Nit: The description of the |appends_to_list| parameter doesn't make much
sense
> -- it's a boolean that states *whether* the method is supposed to append to
the
> list, which implies that it won't do that if the value is false, but the
comment
> makes it appear as if it always would do that. 

Done.

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
File components/password_manager/core/common/password_manager_features.cc
(right):

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
components/password_manager/core/common/password_manager_features.cc:17: //
Enables dropping the credential used to sync passwords.
On 2016/02/11 12:30:21, vabr (Chromium) wrote:
> nit: Reformulate for clarity:
> // Drop the sync credential if captured for saving, do not offer it for
saving.

Done.

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
components/password_manager/core/common/password_manager_features.cc:28:
"enable-drop-sync-credential", base::FEATURE_DISABLED_BY_DEFAULT};
On 2016/02/11 11:12:40, Bernhard Bauer wrote:
> These two features are unused and redundant.

Done.

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
components/password_manager/core/common/password_manager_features.cc:50: const
base::Feature kManagerForSyncSignin = {"manager-for-sync-signin",
On 2016/02/11 12:30:21, vabr (Chromium) wrote:
> This one is actually obsolete and ready for removal: http://crbug.com/586107.
> 
> However, that removal should be done in a separate CL.

Acknowledged.

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
File components/password_manager/sync/browser/sync_credentials_filter.cc
(right):

https://codereview.chromium.org/1668523002/diff/180001/components/password_ma...
components/password_manager/sync/browser/sync_credentials_filter.cc:109: } else
if (protect_sync_credential_on_reauth_disabled) {
On 2016/02/11 11:12:40, Bernhard Bauer wrote:
> You don't need an else if you return in the branch before.

Done.

[Pritam Nikam, 2016-02-25 16:39:26]

https://codereview.chromium.org/1668523002/diff/260001/components/password_ma...
File components/password_manager/core/browser/password_manager_util.cc (right):

https://codereview.chromium.org/1668523002/diff/260001/components/password_ma...
components/password_manager/core/browser/password_manager_util.cc:132: void
RegisterFieldTrials(base::FeatureList* feature_list) {
I guess I've to move these changes in "fieldtrial_testing_config_schema.json'
instead ?

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/vari...

[Pritam Nikam, 2016-02-25 16:49:12]

https://codereview.chromium.org/1668523002/diff/260001/components/password_ma...
File components/password_manager/core/browser/password_manager_util.cc (right):

https://codereview.chromium.org/1668523002/diff/260001/components/password_ma...
components/password_manager/core/browser/password_manager_util.cc:132: void
RegisterFieldTrials(base::FeatureList* feature_list) {
"fieldtrial_testing_config_{port}.json"

eg.
https://code.google.com/p/chromium/codesearch#chromium/src/testing/variations...

[vabr (Chromium), 2016-02-26 09:43:41]

Thanks, Pritam Nikam, for your changes!

I would really like to see http://crbug.com/586107 addressed separately in a CL
on which the current CL can then depend.

I also left some comments below.

If you address my comments and Bernhard is happy with the patch, feel free to
assume my LGTM. I'm on holiday next week, so won't be able to respond until
after that.

Thanks,
Vaclav

https://codereview.chromium.org/1668523002/diff/280001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1668523002/diff/280001/chrome/app/generated_r...
chrome/app/generated_resources.grd:5930: +        Autofill sync credential.
nit: Do not include the full-stop (.) in the name, only in description.

https://codereview.chromium.org/1668523002/diff/280001/chrome/app/generated_r...
chrome/app/generated_resources.grd:5936: +        Autofill sync credential only
for transactional reauth pages.
nit: Here as well: Do not include the full-stop (.) in the name, only in
description.

https://codereview.chromium.org/1668523002/diff/280001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1668523002/diff/280001/chrome/browser/about_f...
chrome/browser/about_flags.cc:924: {"enable-affiliation-based-matching",
I think this should exclude "enable-". Looking at
components/flags_ui/feature_entry.h, this will result in 3 values of the flag:
Default, Enabled, Disabled. Thus, the "enable-" prefix seems redundant.

Also on lines 1348, 1795 and 1798.

https://codereview.chromium.org/1668523002/diff/280001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.h (left):

https://codereview.chromium.org/1668523002/diff/280001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.h:103: static
bool EnabledForSyncSignin();
Please split this change (fixing http://crbug.com/586107) in a separate CL.

(If you have absolutely completely triple-convincing reasons to have all in this
one, please at least add 586107 to the BUG variable in the CL description.)

https://codereview.chromium.org/1668523002/diff/280001/components/password_ma...
File components/password_manager/core/browser/affiliation_utils.cc (right):

https://codereview.chromium.org/1668523002/diff/280001/components/password_ma...
components/password_manager/core/browser/affiliation_utils.cc:14: #include
"components/password_manager/core/browser/password_manager_util.h"
It looks like you can remove this header -- it was apparently not needed before,
and you only deleted code and headers which could not have transitively included
this one.

https://codereview.chromium.org/1668523002/diff/280001/components/password_ma...
File components/password_manager/core/browser/affiliation_utils_unittest.cc
(right):

https://codereview.chromium.org/1668523002/diff/280001/components/password_ma...
components/password_manager/core/browser/affiliation_utils_unittest.cc:268:
const char kVaraiationParam[] = "propagate_password_changes_to_web";
typo: Varaiaition -> Variation

https://codereview.chromium.org/1668523002/diff/280001/components/password_ma...
File components/password_manager/core/common/password_manager_features.h
(right):

https://codereview.chromium.org/1668523002/diff/280001/components/password_ma...
components/password_manager/core/common/password_manager_features.h:26: extern
const base::Feature kManagerForSyncSignin;
We should not need this, could we just drop it?

https://codereview.chromium.org/1668523002/diff/280001/testing/variations/fie...
File testing/variations/fieldtrial_testing_config_android.json (right):

https://codereview.chromium.org/1668523002/diff/280001/testing/variations/fie...
testing/variations/fieldtrial_testing_config_android.json:2:
"AffiliationBasedMatching": [
I don't think we want this feature turned off on trybots while it is on in
production by default.
(Same for the other platforms.)

https://codereview.chromium.org/1668523002/diff/280001/testing/variations/fie...
testing/variations/fieldtrial_testing_config_android.json:52:
"AutofillSyncCredential": [
I also do not see a point in having this override for testing. The default value
is fine, and we test the special cases in unit tests. So I suggest dropping this
change as well.
(Same for the other platforms.)

https://codereview.chromium.org/1668523002/diff/280001/testing/variations/fie...
testing/variations/fieldtrial_testing_config_android.json:227:
"PasswordManagerDropSyncCredential": [
Please drop this one as well, for the same reasons as commented on line 52.
(Same for the other platforms.)

[vabr (Chromium), 2016-02-26 09:52:22]

Oops, I missed an important part -- I believ patch set 14 was the right way to
register the overrides. But you may want to double-check with Alexei or other
owners of the features code.

https://codereview.chromium.org/1668523002/diff/260001/components/password_ma...
File components/password_manager/core/browser/password_manager_util.cc (right):

https://codereview.chromium.org/1668523002/diff/260001/components/password_ma...
components/password_manager/core/browser/password_manager_util.cc:44: if
(field_trial && base::StartsWith(update_enabled, "disabled",
nit: I believe calls to this function would be more readable if the "disabled"
was a parameter. Hardcoding it inside leaves no hint at the callsite, that it is
the value "disabled" which we look for.

https://codereview.chromium.org/1668523002/diff/260001/components/password_ma...
components/password_manager/core/browser/password_manager_util.cc:132: void
RegisterFieldTrials(base::FeatureList* feature_list) {
On 2016/02/25 16:39:26, Pritam (Away until Feb.17) wrote:
> I guess I've to move these changes in "fieldtrial_testing_config_schema.json'
> instead ?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/vari...

This does not sound like a good idea, actually. IIUC, the JSONs are there for
trybots, whereas this registering is needed for the official Chrome builds. I
would say you need this and not the JSONs, but please do confirm this with
Alexei or somebody knowledgeable.

https://codereview.chromium.org/1668523002/diff/260001/components/password_ma...
components/password_manager/core/browser/password_manager_util.cc:137: //   -
variation param value with "propagate_password_changes_to_web" turns
Not every value, just the "disabled" value, right?

https://codereview.chromium.org/1668523002/diff/260001/components/password_ma...
components/password_manager/core/browser/password_manager_util.cc:145:
RegisterFieldTrialOverrideByVariationParam(
nit: Maybe it would be more readable to inline this function? It only has one
call-site.

[Pritam Nikam, 2016-02-26 12:42:23]

Thanks Vaclav for review. Ptal at new patch set.
Thanks!

https://codereview.chromium.org/1668523002/diff/260001/components/password_ma...
File components/password_manager/core/browser/password_manager_util.cc (right):

https://codereview.chromium.org/1668523002/diff/260001/components/password_ma...
components/password_manager/core/browser/password_manager_util.cc:44: if
(field_trial && base::StartsWith(update_enabled, "disabled",
On 2016/02/26 09:52:22, vabr (Chromium) wrote:
> nit: I believe calls to this function would be more readable if the "disabled"
> was a parameter. Hardcoding it inside leaves no hint at the callsite, that it
is
> the value "disabled" which we look for.

Acknowledged.

Added a in-line implementation instead.

https://codereview.chromium.org/1668523002/diff/260001/components/password_ma...
components/password_manager/core/browser/password_manager_util.cc:137: //   -
variation param value with "propagate_password_changes_to_web" turns
On 2016/02/26 09:52:22, vabr (Chromium) wrote:
> Not every value, just the "disabled" value, right?

Done.

https://codereview.chromium.org/1668523002/diff/260001/components/password_ma...
components/password_manager/core/browser/password_manager_util.cc:145:
RegisterFieldTrialOverrideByVariationParam(
On 2016/02/26 09:52:22, vabr (Chromium) wrote:
> nit: Maybe it would be more readable to inline this function? It only has one
> call-site.

Done.

https://codereview.chromium.org/1668523002/diff/280001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1668523002/diff/280001/chrome/app/generated_r...
chrome/app/generated_resources.grd:5930: +        Autofill sync credential.
On 2016/02/26 09:43:41, vabr (Chromium) wrote:
> nit: Do not include the full-stop (.) in the name, only in description.

Done.

https://codereview.chromium.org/1668523002/diff/280001/chrome/app/generated_r...
chrome/app/generated_resources.grd:5936: +        Autofill sync credential only
for transactional reauth pages.
On 2016/02/26 09:43:41, vabr (Chromium) wrote:
> nit: Here as well: Do not include the full-stop (.) in the name, only in
> description.

Done.

https://codereview.chromium.org/1668523002/diff/280001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1668523002/diff/280001/chrome/browser/about_f...
chrome/browser/about_flags.cc:924: {"enable-affiliation-based-matching",
On 2016/02/26 09:43:41, vabr (Chromium) wrote:
> I think this should exclude "enable-". Looking at
> components/flags_ui/feature_entry.h, this will result in 3 values of the flag:
> Default, Enabled, Disabled. Thus, the "enable-" prefix seems redundant.
> 
> Also on lines 1348, 1795 and 1798.

Done.

https://codereview.chromium.org/1668523002/diff/280001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.h (left):

https://codereview.chromium.org/1668523002/diff/280001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.h:103: static
bool EnabledForSyncSignin();
On 2016/02/26 09:43:41, vabr (Chromium) wrote:
> Please split this change (fixing http://crbug.com/586107) in a separate CL.
> 
> (If you have absolutely completely triple-convincing reasons to have all in
this
> one, please at least add 586107 to the BUG variable in the CL description.)

Will address this in separate CL.

https://codereview.chromium.org/1668523002/diff/280001/components/password_ma...
File components/password_manager/core/browser/affiliation_utils.cc (right):

https://codereview.chromium.org/1668523002/diff/280001/components/password_ma...
components/password_manager/core/browser/affiliation_utils.cc:14: #include
"components/password_manager/core/browser/password_manager_util.h"
On 2016/02/26 09:43:41, vabr (Chromium) wrote:
> It looks like you can remove this header -- it was apparently not needed
before,
> and you only deleted code and headers which could not have transitively
included
> this one.

Done.

https://codereview.chromium.org/1668523002/diff/280001/components/password_ma...
File components/password_manager/core/browser/affiliation_utils_unittest.cc
(right):

https://codereview.chromium.org/1668523002/diff/280001/components/password_ma...
components/password_manager/core/browser/affiliation_utils_unittest.cc:268:
const char kVaraiationParam[] = "propagate_password_changes_to_web";
On 2016/02/26 09:43:41, vabr (Chromium) wrote:
> typo: Varaiaition -> Variation

Done.

https://codereview.chromium.org/1668523002/diff/280001/components/password_ma...
File components/password_manager/core/common/password_manager_features.h
(right):

https://codereview.chromium.org/1668523002/diff/280001/components/password_ma...
components/password_manager/core/common/password_manager_features.h:26: extern
const base::Feature kManagerForSyncSignin;
On 2016/02/26 09:43:41, vabr (Chromium) wrote:
> We should not need this, could we just drop it?

Acknowledged.

This will be resolved in separate CL.
BUG: http://crbug.com/586107

https://codereview.chromium.org/1668523002/diff/280001/testing/variations/fie...
File testing/variations/fieldtrial_testing_config_android.json (right):

https://codereview.chromium.org/1668523002/diff/280001/testing/variations/fie...
testing/variations/fieldtrial_testing_config_android.json:2:
"AffiliationBasedMatching": [
On 2016/02/26 09:43:41, vabr (Chromium) wrote:
> I don't think we want this feature turned off on trybots while it is on in
> production by default.
> (Same for the other platforms.)

Acknowledged.

https://codereview.chromium.org/1668523002/diff/280001/testing/variations/fie...
testing/variations/fieldtrial_testing_config_android.json:52:
"AutofillSyncCredential": [
On 2016/02/26 09:43:41, vabr (Chromium) wrote:
> I also do not see a point in having this override for testing. The default
value
> is fine, and we test the special cases in unit tests. So I suggest dropping
this
> change as well.
> (Same for the other platforms.)

Acknowledged.

https://codereview.chromium.org/1668523002/diff/280001/testing/variations/fie...
testing/variations/fieldtrial_testing_config_android.json:227:
"PasswordManagerDropSyncCredential": [
On 2016/02/26 09:43:41, vabr (Chromium) wrote:
> Please drop this one as well, for the same reasons as commented on line 52.
> (Same for the other platforms.)

Acknowledged.

[vabr (Chromium), 2016-02-26 12:54:42]

Thanks!

So far LGTM, but I have two more comments.

Cheers,
Vaclav

https://codereview.chromium.org/1668523002/diff/280001/components/password_ma...
File components/password_manager/core/common/password_manager_features.h
(right):

https://codereview.chromium.org/1668523002/diff/280001/components/password_ma...
components/password_manager/core/common/password_manager_features.h:26: extern
const base::Feature kManagerForSyncSignin;
On 2016/02/26 12:42:23, Pritam (Away until Feb.17) wrote:
> On 2016/02/26 09:43:41, vabr (Chromium) wrote:
> > We should not need this, could we just drop it?
> 
> Acknowledged.
> 
> This will be resolved in separate CL.
> BUG: http://crbug.com/586107

Thanks, that's great. Still, I would not even introduce this as a new feature,
if it is to be deleted. I see two options for this:

(1) Do the removal CL first, and rebase this one on that removal. That way you
get rid of the command flags and will not have to create this
feature-to-be-deleted either.

(2) Keep the manager-for-sync-signin command-line flag for now, and first remove
the command flags file once you get to remove manager-for-sync-signin in a
subsequent CL.

The reason is to avoid confusion. If someone will read the code and wonder why
we have the kManagerForSyncSignin feature, they would currently hit this CL as
adding it. Unless they read also the review discussion and do some further
git-archeology, it won't be clear to them, why this was added. So it's better to
just remove it as a flag, not as a feature.

https://codereview.chromium.org/1668523002/diff/300001/components/password_ma...
File components/password_manager/core/browser/password_manager_util.cc (right):

https://codereview.chromium.org/1668523002/diff/300001/components/password_ma...
components/password_manager/core/browser/password_manager_util.cc:127: const
char kDisabled[] = "Disabled";
nit: Just hard-code the string. kDisabled and "Disabled" is almost the same for
readability and length, and the meaning on lines 138 and all the others actually
differ slightly (most are a field trial group name, 138 is a variation parameter
value).

[Pritam Nikam, 2016-02-26 14:07:50]

Description was changed from

==========
[Password Manager] Switch password manager code to use "base/feature_list.h".

Changes so far:
  A. 'allow-autofill-sync-credential' feature.
  B. 'disallow-autofill-sync-credential' feature.
  C. 'disallow-autofill-sync-credential-for-reauth' feature.
  D. 'disable-affiliation-based-matching' feature.
  E. 'enable-affiliation-based-matching' feature.
  F. 'disable-drop-sync-credential' feature.
  G. 'enable-drop-sync-credential' feature.
  H. 'disable-manager-for-sync-signin' feature.
  I. 'enable-manager-for-sync-signin' feature.

BUG=561322
==========

to

==========
[Password Manager] Switch password manager code to use "base/feature_list.h".

Changes so far:
  A. 'allow-autofill-sync-credential' feature.
  B. 'disallow-autofill-sync-credential' feature.
  C. 'disallow-autofill-sync-credential-for-reauth' feature.
  D. 'disable-affiliation-based-matching' feature.
  E. 'enable-affiliation-based-matching' feature.
  F. 'disable-drop-sync-credential' feature.
  G. 'enable-drop-sync-credential' feature.

BUG=561322
==========

[Pritam Nikam, 2016-02-26 14:10:29]

Hi Vaclav,
Thanks for review! New patch set addresses nit, ptal.
Thanks!

https://codereview.chromium.org/1668523002/diff/280001/components/password_ma...
File components/password_manager/core/common/password_manager_features.h
(right):

https://codereview.chromium.org/1668523002/diff/280001/components/password_ma...
components/password_manager/core/common/password_manager_features.h:26: extern
const base::Feature kManagerForSyncSignin;
On 2016/02/26 12:54:41, vabr (away until 7 March) wrote:
> On 2016/02/26 12:42:23, Pritam (Away until Feb.17) wrote:
> > On 2016/02/26 09:43:41, vabr (Chromium) wrote:
> > > We should not need this, could we just drop it?
> > 
> > Acknowledged.
> > 
> > This will be resolved in separate CL.
> > BUG: http://crbug.com/586107
> 
> Thanks, that's great. Still, I would not even introduce this as a new feature,
> if it is to be deleted. I see two options for this:
> 
> (1) Do the removal CL first, and rebase this one on that removal. That way you
> get rid of the command flags and will not have to create this
> feature-to-be-deleted either.
> 
> (2) Keep the manager-for-sync-signin command-line flag for now, and first
remove
> the command flags file once you get to remove manager-for-sync-signin in a
> subsequent CL.
> 
> The reason is to avoid confusion. If someone will read the code and wonder why
> we have the kManagerForSyncSignin feature, they would currently hit this CL as
> adding it. Unless they read also the review discussion and do some further
> git-archeology, it won't be clear to them, why this was added. So it's better
to
> just remove it as a flag, not as a feature.

Done.

Submitted a CL to remove the feature flag.
Review URL: https://codereview.chromium.org/1744513002/

https://codereview.chromium.org/1668523002/diff/300001/components/password_ma...
File components/password_manager/core/browser/password_manager_util.cc (right):

https://codereview.chromium.org/1668523002/diff/300001/components/password_ma...
components/password_manager/core/browser/password_manager_util.cc:127: const
char kDisabled[] = "Disabled";
On 2016/02/26 12:54:42, vabr (away until 7 March) wrote:
> nit: Just hard-code the string. kDisabled and "Disabled" is almost the same
for
> readability and length, and the meaning on lines 138 and all the others
actually
> differ slightly (most are a field trial group name, 138 is a variation
parameter
> value).

Done.

[Bernhard Bauer, 2016-02-26 14:22:01]

Could you please update the description to reflect the contents of the CL?

[Pritam Nikam, 2016-02-26 14:44:14]

Description was changed from

==========
[Password Manager] Switch password manager code to use "base/feature_list.h".

Changes so far:
  A. 'allow-autofill-sync-credential' feature.
  B. 'disallow-autofill-sync-credential' feature.
  C. 'disallow-autofill-sync-credential-for-reauth' feature.
  D. 'disable-affiliation-based-matching' feature.
  E. 'enable-affiliation-based-matching' feature.
  F. 'disable-drop-sync-credential' feature.
  G. 'enable-drop-sync-credential' feature.

BUG=561322
==========

to

==========
[Password Manager] Switch password manager code to use the Feature framework.

The FeatureList APIs replace the current practice for enabling/disabling
switches and offer expressing the default state of a feature in a structured
manner.

This CL does the refactoring for the password manager command-line switches to
use the FeatureList APIs.

BUG=561322
==========

[vabr (Chromium), 2016-02-26 14:55:05]

Thanks, LGTM.
Vaclav

[Alexei Svitkine (slow), 2016-02-26 21:57:25]

https://codereview.chromium.org/1668523002/diff/320001/components/password_ma...
File components/password_manager/core/browser/affiliation_utils_unittest.cc
(right):

https://codereview.chromium.org/1668523002/diff/320001/components/password_ma...
components/password_manager/core/browser/affiliation_utils_unittest.cc:28: const
base::Feature kDummyFeature = {"FooBar",
Nit: No =

https://codereview.chromium.org/1668523002/diff/320001/components/password_ma...
File components/password_manager/core/browser/password_manager_util.cc (right):

https://codereview.chromium.org/1668523002/diff/320001/components/password_ma...
components/password_manager/core/browser/password_manager_util.cc:116: void
RegisterFieldTrials(base::FeatureList* feature_list) {
It doesn't make sense to have this client code. Instead, the server-side
experiment configs should be updated to specify these associations in the
configs. (Which would have to be done by a Googler - probably whoever
implemented these field trials in the first place.)

[Pritam Nikam, 2016-02-29 08:00:15]

Thanks Alexei for inputs. I've reverted field trials related changes, ptal.
Thanks!

https://codereview.chromium.org/1668523002/diff/320001/components/password_ma...
File components/password_manager/core/browser/affiliation_utils_unittest.cc
(right):

https://codereview.chromium.org/1668523002/diff/320001/components/password_ma...
components/password_manager/core/browser/affiliation_utils_unittest.cc:28: const
base::Feature kDummyFeature = {"FooBar",
On 2016/02/26 21:57:24, Alexei Svitkine (slow) wrote:
> Nit: No =

Done.

https://codereview.chromium.org/1668523002/diff/320001/components/password_ma...
File components/password_manager/core/browser/password_manager_util.cc (right):

https://codereview.chromium.org/1668523002/diff/320001/components/password_ma...
components/password_manager/core/browser/password_manager_util.cc:116: void
RegisterFieldTrials(base::FeatureList* feature_list) {
On 2016/02/26 21:57:24, Alexei Svitkine (slow) wrote:
> It doesn't make sense to have this client code. Instead, the server-side
> experiment configs should be updated to specify these associations in the
> configs. (Which would have to be done by a Googler - probably whoever
> implemented these field trials in the first place.)

Acknowledged.

[Bernhard Bauer, 2016-02-29 11:34:16]

https://codereview.chromium.org/1668523002/diff/340001/components/password_ma...
File components/password_manager/core/browser/affiliation_utils_unittest.cc
(right):

https://codereview.chromium.org/1668523002/diff/340001/components/password_ma...
components/password_manager/core/browser/affiliation_utils_unittest.cc:28: const
base::Feature No = {"FooBar", base::FEATURE_DISABLED_BY_DEFAULT};
If this is a constant (like other features), it should be called kSomething.

Actually, I think we can remove the tests for IsAffiliationBasedMatchingEnabled
and IsPropagatingPasswordChangesToWebCredentialsEnabled completely: the methods
themselves have been removed, and the test now only exercises code in
FeatureList, which is not even the class under test! 😃 And we can assume that it
is working correctly, because it has its own unit tests. (Yay decomposition!)

(Sorry for not catching this earlier -- sometimes things like that only jump out
once the previous cleanup has been done.)

[Pritam Nikam, 2016-02-29 12:54:50]

Thanks Bernhard for review. I've removed those tests, ptal.
Thanks!

https://codereview.chromium.org/1668523002/diff/340001/components/password_ma...
File components/password_manager/core/browser/affiliation_utils_unittest.cc
(right):

https://codereview.chromium.org/1668523002/diff/340001/components/password_ma...
components/password_manager/core/browser/affiliation_utils_unittest.cc:28: const
base::Feature No = {"FooBar", base::FEATURE_DISABLED_BY_DEFAULT};
On 2016/02/29 11:34:16, Bernhard Bauer wrote:
> If this is a constant (like other features), it should be called kSomething.
> 
> Actually, I think we can remove the tests for
IsAffiliationBasedMatchingEnabled
> and IsPropagatingPasswordChangesToWebCredentialsEnabled completely: the
methods
> themselves have been removed, and the test now only exercises code in
> FeatureList, which is not even the class under test! 😃 And we can assume that
it
> is working correctly, because it has its own unit tests. (Yay decomposition!)
> 
> (Sorry for not catching this earlier -- sometimes things like that only jump
out
> once the previous cleanup has been done.)

Done.

[Bernhard Bauer, 2016-02-29 13:03:07]

LGTM, thanks!

[Pritam Nikam, 2016-02-29 13:41:26]

https://codereview.chromium.org/1668523002/diff/320001/components/password_ma...
File components/password_manager/core/browser/password_manager_util.cc (right):

https://codereview.chromium.org/1668523002/diff/320001/components/password_ma...
components/password_manager/core/browser/password_manager_util.cc:116: void
RegisterFieldTrials(base::FeatureList* feature_list) {
On 2016/02/29 08:00:15, Pritam Nikam wrote:
> On 2016/02/26 21:57:24, Alexei Svitkine (slow) wrote:
> > It doesn't make sense to have this client code. Instead, the server-side
> > experiment configs should be updated to specify these associations in the
> > configs. (Which would have to be done by a Googler - probably whoever
> > implemented these field trials in the first place.)
> 
> Acknowledged.

Refer: http://crbug.com/590718

[Alexei Svitkine (slow), 2016-02-29 16:30:48]

https://codereview.chromium.org/1668523002/diff/320001/components/password_ma...
File components/password_manager/core/browser/password_manager_util.cc (right):

https://codereview.chromium.org/1668523002/diff/320001/components/password_ma...
components/password_manager/core/browser/password_manager_util.cc:116: void
RegisterFieldTrials(base::FeatureList* feature_list) {
On 2016/02/29 13:41:25, Pritam Nikam wrote:
> On 2016/02/29 08:00:15, Pritam Nikam wrote:
> > On 2016/02/26 21:57:24, Alexei Svitkine (slow) wrote:
> > > It doesn't make sense to have this client code. Instead, the server-side
> > > experiment configs should be updated to specify these associations in the
> > > configs. (Which would have to be done by a Googler - probably whoever
> > > implemented these field trials in the first place.)
> > 
> > Acknowledged.
> 
> Refer: http://crbug.com/590718

Thanks. I'll wait for that to be resolved and then this CL can be simplified
before submitting.

(not lgtm until then, sorry)

[vabr (Chromium), 2016-03-07 13:03:29]

On 2016/02/29 16:30:48, Alexei Svitkine (slow) wrote:
>
https://codereview.chromium.org/1668523002/diff/320001/components/password_ma...
> File components/password_manager/core/browser/password_manager_util.cc
(right):
> 
>
https://codereview.chromium.org/1668523002/diff/320001/components/password_ma...
> components/password_manager/core/browser/password_manager_util.cc:116: void
> RegisterFieldTrials(base::FeatureList* feature_list) {
> On 2016/02/29 13:41:25, Pritam Nikam wrote:
> > On 2016/02/29 08:00:15, Pritam Nikam wrote:
> > > On 2016/02/26 21:57:24, Alexei Svitkine (slow) wrote:
> > > > It doesn't make sense to have this client code. Instead, the server-side
> > > > experiment configs should be updated to specify these associations in
the
> > > > configs. (Which would have to be done by a Googler - probably whoever
> > > > implemented these field trials in the first place.)
> > > 
> > > Acknowledged.
> > 
> > Refer: http://crbug.com/590718
> 
> Thanks. I'll wait for that to be resolved and then this CL can be simplified
> before submitting.
> 
> (not lgtm until then, sorry)

Thanks for the work on this and reviews, everybody!

I'm happy to fix the Field trial configs. I just came back from holiday, so
expect slight delay. In the meantime I asked Alexei a question in
http://crbug.com/590718#c1.

Will update this CL once I fix the configs.

Thanks!
Vaclav

[Alexei Svitkine (slow), 2016-03-10 16:00:22]

lgtm

I'll let vabr confirm that this is safe - but looking closer it seems all of
these are enabled by default, so no actually no need for server side configs
(unless there's any active configs that turn these off currently).

[vabr (Chromium), 2016-03-10 18:22:10]

On 2016/03/10 16:00:22, Alexei Svitkine (very slow) wrote:
> lgtm
> 
> I'll let vabr confirm that this is safe - but looking closer it seems all of
> these are enabled by default, so no actually no need for server side configs
> (unless there's any active configs that turn these off currently).

I can confirm that the default settings for the features here are safe. The
affiliation based matching is enabled 100% since version 49, as Alexei noted,
and the other ones are also meant to be enabled by default (the tie to Finch is
to have a way of a kill switch, which is automatically given for features, as
opposed to command line flags.

LGTM, thanks all for help with this.

Cheers,
Vaclav

[Pritam Nikam, 2016-03-11 02:32:41]

The CQ bit was checked by pritam.nikam@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-11 02:33:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1668523002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1668523002/360001

[commit-bot: I haz the power, 2016-03-11 02:36:35]

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

[commit-bot: I haz the power, 2016-03-11 02:36:37]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[Pritam Nikam, 2016-03-11 09:31:38]

The CQ bit was checked by pritam.nikam@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-11 09:32:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1668523002/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1668523002/380001

[commit-bot: I haz the power, 2016-03-11 09:39:56]

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

[commit-bot: I haz the power, 2016-03-11 09:39:58]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Pritam Nikam, 2016-03-11 11:28:57]

The CQ bit was checked by pritam.nikam@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-11 11:29:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1668523002/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1668523002/400001

[commit-bot: I haz the power, 2016-03-11 12:36:15]

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

[commit-bot: I haz the power, 2016-03-11 12:36:17]

Dry run: This issue passed the CQ dry run.

[Pritam Nikam, 2016-03-11 12:36:41]

The CQ bit was checked by pritam.nikam@samsung.com

[Pritam Nikam, 2016-03-11 12:36:42]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
asvitkine@chromium.org, bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1668523002/#ps400001
(title: " ")

[commit-bot: I haz the power, 2016-03-11 12:37:07]

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

[commit-bot: I haz the power, 2016-03-11 12:42:55]

Description was changed from

==========
[Password Manager] Switch password manager code to use the Feature framework.

The FeatureList APIs replace the current practice for enabling/disabling
switches and offer expressing the default state of a feature in a structured
manner.

This CL does the refactoring for the password manager command-line switches to
use the FeatureList APIs.

BUG=561322
==========

to

==========
[Password Manager] Switch password manager code to use the Feature framework.

The FeatureList APIs replace the current practice for enabling/disabling
switches and offer expressing the default state of a feature in a structured
manner.

This CL does the refactoring for the password manager command-line switches to
use the FeatureList APIs.

BUG=561322
==========

[commit-bot: I haz the power, 2016-03-11 12:42:58]

Committed patchset #21 (id:400001)

[commit-bot: I haz the power, 2016-03-11 12:44:25]

Description was changed from

==========
[Password Manager] Switch password manager code to use the Feature framework.

The FeatureList APIs replace the current practice for enabling/disabling
switches and offer expressing the default state of a feature in a structured
manner.

This CL does the refactoring for the password manager command-line switches to
use the FeatureList APIs.

BUG=561322
==========

to

==========
[Password Manager] Switch password manager code to use the Feature framework.

The FeatureList APIs replace the current practice for enabling/disabling
switches and offer expressing the default state of a feature in a structured
manner.

This CL does the refactoring for the password manager command-line switches to
use the FeatureList APIs.

BUG=561322

Committed: https://crrev.com/3b43e5a81c83605870ba5406143fcc5bb885890b
Cr-Commit-Position: refs/heads/master@{#380611}
==========

[commit-bot: I haz the power, 2016-03-11 12:44:28]

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