New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 785953 link

Starred by 1 user
Status: Fixed
Owner:
█ nikhil.s...@samsung.com
Last visit > 30 days ago
Closed: Jun 2018
Cc:
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac , Fuchsia
Pri: 3
Type: Task



Sign in to add a comment

Fix names of password_manager features

Project Member Reported by vabr@chromium.org, Nov 16 2017 
components/password_manager/core/common/password_manager_features.* contains a lot of cases of features with "enabled" in their name (kEnable...)
All those "enables" are implicit and the name should be just the description of the feature itself (e.g., kViewPasswords).

We should clean that up, because it thrives on copy&paste.

Related complaint from the metrics team: http://g/chrome-password-manager-team/I3rUMcgCj0E/ZMJzqudNAQAJ

Comment 1 by vabr@chromium.org, Nov 16 2017

Labels: Hotlist-GoodFirstBug

Comment 2 by battre@chromium.org, Nov 16 2017 

Note that the complaint was not just about the "enabled":

---
components/password_manager/core/common/password_manager_features.cc

Use a number of different naming conventions for the feature name strings. Some use "enable-foo-bar", others "EnableFooBar", others "foo-bar" and one even "kEnableFooBar".

Please note that the recommended convention is: "FooBar". See:

https://cs.chromium.org/chromium/src/base/feature_list.h?rcl=1a7c25464ee925f9b644a7f861c311fdee70fdc3&l=39
---

I am not sure that we want to change any existing features. I think that we should add a comment about the right naming scheme and a warning not to copy&paste.

Comment 3 by vabr@chromium.org, Nov 17 2017 

Thanks for pointing out the other style problems.

As for leaving the old features as are and having a comment against copy&paste -- I doubt that will be effective, because I won't expect people to read the whole header file.

I don't think changing the name of the feature constants is a complicated process. Do you see a downside to it?

Comment 4 by nikhil.s...@samsung.com, Dec 1 2017 

Can I check this issue .
AFAIU we want to change kEnableHtmlBasedUsernameDetector to kHtmlBasedUsernameDetector and also strings like "EnableManualSaving" to "ManualSaving" ?

Please let me know if I can work on this?

Comment 5 by vabr@chromium.org, Dec 1 2017 

Indeed, both these need to change:
(1) No "enable" or "disable" in the name.
(2) Strings with feature names should use CamelCase, not names-of-hyphenated-style.

Please feel free to work on this. My two suggestions are:
(A) Do separate CLs for separate features -- if one has a problem, we won't have to revert the rest.
(B) Do watch for occurrences of the feature name even in places where components/password_manager/core/common/password_manager_features.h is not included -- e.g., chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java references the "view-passwords" feature.

I'm happy to review any CLs on this, and am available until 7 December 2017.

Thanks!

Comment 6 by nikhil.s...@samsung.com, Dec 6 2017

Owner: nikhil.s...@samsung.com

Comment 7 by nikhil.s...@samsung.com, Dec 6 2017

Status: Started (was: Available)
Project Member

Comment 8 by bugdroid1@chromium.org, Dec 8 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/378402ccc71bf446c20c8bf4c5919b9b2a5a7af2

commit 378402ccc71bf446c20c8bf4c5919b9b2a5a7af2
Author: nikhil <nikhil.sahni@samsung.com>
Date: Fri Dec 08 06:53:57 2017

Fixing names of password_manager ViewPassword feature.

Fixing  names of password_manager ViewPassword feature
as per the naming convention.

Bug:  785953 
Change-Id: I87b70742b52e6f393d51686c1c8322c062d0047e
Reviewed-on: https://chromium-review.googlesource.com/805177
Commit-Queue: NIKHIL SAHNI <nikhil.sahni@samsung.com>
Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522726}
[modify] https://crrev.com/378402ccc71bf446c20c8bf4c5919b9b2a5a7af2/chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java
[modify] https://crrev.com/378402ccc71bf446c20c8bf4c5919b9b2a5a7af2/chrome/browser/about_flags.cc
[modify] https://crrev.com/378402ccc71bf446c20c8bf4c5919b9b2a5a7af2/components/password_manager/core/common/password_manager_features.cc
[modify] https://crrev.com/378402ccc71bf446c20c8bf4c5919b9b2a5a7af2/tools/metrics/histograms/enums.xml
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 13 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/17ef14e27b2ec129ff03dda2698dc6b7cf96d2f2

commit 17ef14e27b2ec129ff03dda2698dc6b7cf96d2f2
Author: nikhil <nikhil.sahni@samsung.com>
Date: Wed Dec 13 07:35:31 2017

Fixing names of password_manager PasswordImport feature.

Fixing  names of password_manager PasswordImport feature
as per the naming convention.

Bug:  785953 
Change-Id: I7c7bbdd5852f40e6735ffd51e9c519e59d3a6b0c
Reviewed-on: https://chromium-review.googlesource.com/817458
Reviewed-by: srirama chandra sekhar <srirama.m@samsung.com>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Commit-Queue: srirama chandra sekhar <srirama.m@samsung.com>
Cr-Commit-Position: refs/heads/master@{#523712}
[modify] https://crrev.com/17ef14e27b2ec129ff03dda2698dc6b7cf96d2f2/chrome/browser/about_flags.cc
[modify] https://crrev.com/17ef14e27b2ec129ff03dda2698dc6b7cf96d2f2/components/password_manager/core/common/password_manager_features.cc
[modify] https://crrev.com/17ef14e27b2ec129ff03dda2698dc6b7cf96d2f2/tools/metrics/histograms/enums.xml
Project Member

Comment 10 by bugdroid1@chromium.org, Jan 8 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d70e7cb5de47a37ace52f747c247f43dc83ec59a

commit d70e7cb5de47a37ace52f747c247f43dc83ec59a
Author: nikhil <nikhil.sahni@samsung.com>
Date: Mon Jan 08 07:12:55 2018

Fixing names of password_manager PasswordExport feature.

Fixing  names of password_manager PasswordExport feature
as per the naming convention.

Bug:  785953 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I3bd4015af6b44f0b6b5ef9a9f145f9d1a8f2445c
Reviewed-on: https://chromium-review.googlesource.com/844009
Commit-Queue: NIKHIL SAHNI <nikhil.sahni@samsung.com>
Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527582}
[modify] https://crrev.com/d70e7cb5de47a37ace52f747c247f43dc83ec59a/chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java
[modify] https://crrev.com/d70e7cb5de47a37ace52f747c247f43dc83ec59a/chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java
[modify] https://crrev.com/d70e7cb5de47a37ace52f747c247f43dc83ec59a/chrome/browser/about_flags.cc
[modify] https://crrev.com/d70e7cb5de47a37ace52f747c247f43dc83ec59a/components/password_manager/core/common/password_manager_features.cc
[modify] https://crrev.com/d70e7cb5de47a37ace52f747c247f43dc83ec59a/ios/chrome/browser/about_flags.mm
[modify] https://crrev.com/d70e7cb5de47a37ace52f747c247f43dc83ec59a/tools/metrics/histograms/enums.xml
Project Member

Comment 11 by bugdroid1@chromium.org, Jan 8 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e50425bed92b9a78403abfceeefe269f44cb92c7

commit e50425bed92b9a78403abfceeefe269f44cb92c7
Author: nikhil <nikhil.sahni@samsung.com>
Date: Mon Jan 08 12:45:14 2018

Fixing names of password_manager AffiliationBasedMatching feature.

Fixing  names of password_manager AffiliationBasedMatching feature
as per the naming convention.

Bug:  785953 
Change-Id: I35ed9969a013ac7bf88acb26975f8d9fc16ea1b3
Reviewed-on: https://chromium-review.googlesource.com/851936
Commit-Queue: NIKHIL SAHNI <nikhil.sahni@samsung.com>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527616}
[modify] https://crrev.com/e50425bed92b9a78403abfceeefe269f44cb92c7/chrome/browser/about_flags.cc
[modify] https://crrev.com/e50425bed92b9a78403abfceeefe269f44cb92c7/components/password_manager/core/common/password_manager_features.cc
[modify] https://crrev.com/e50425bed92b9a78403abfceeefe269f44cb92c7/tools/metrics/histograms/enums.xml
Project Member

Comment 13 by bugdroid1@chromium.org, Jan 15 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/515e64a31d58f4a26a0dbe6cd38eaabfc396920a

commit 515e64a31d58f4a26a0dbe6cd38eaabfc396920a
Author: nikhil <nikhil.sahni@samsung.com>
Date: Mon Jan 15 08:35:52 2018

Fixing names of password_manager enable-password-force-saving feature.

Fixing  names of password_manager enable-password-force-saving feature
as per the naming convention.

Bug:  785953 
Change-Id: I9f7301b7b41fdb01a60d50b13056e7b1cff8a6f9
Reviewed-on: https://chromium-review.googlesource.com/853793
Commit-Queue: NIKHIL SAHNI <nikhil.sahni@samsung.com>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529227}
[modify] https://crrev.com/515e64a31d58f4a26a0dbe6cd38eaabfc396920a/chrome/browser/about_flags.cc
[modify] https://crrev.com/515e64a31d58f4a26a0dbe6cd38eaabfc396920a/components/password_manager/core/browser/password_manager_unittest.cc
[modify] https://crrev.com/515e64a31d58f4a26a0dbe6cd38eaabfc396920a/components/password_manager/core/common/experiments.cc
[modify] https://crrev.com/515e64a31d58f4a26a0dbe6cd38eaabfc396920a/components/password_manager/core/common/password_manager_features.cc
[modify] https://crrev.com/515e64a31d58f4a26a0dbe6cd38eaabfc396920a/components/password_manager/core/common/password_manager_features.h
[modify] https://crrev.com/515e64a31d58f4a26a0dbe6cd38eaabfc396920a/tools/metrics/histograms/enums.xml
Project Member

Comment 14 by bugdroid1@chromium.org, Jan 19 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a37fd2e1d6785d3d8d682eab2b3ae2c3bfe3bdc5

commit a37fd2e1d6785d3d8d682eab2b3ae2c3bfe3bdc5
Author: nikhil <nikhil.sahni@samsung.com>
Date: Fri Jan 19 07:29:04 2018

Fixing names of password_manager EnableManualSaving feature.

Fixing  names of password_manager EnableManualSaving feature
as per the naming convention.

Bug:  785953 
Change-Id: I1c056e5a6c3893274761a8ffc2cb974d54d5d4d9
Reviewed-on: https://chromium-review.googlesource.com/851519
Commit-Queue: NIKHIL SAHNI <nikhil.sahni@samsung.com>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530447}
[modify] https://crrev.com/a37fd2e1d6785d3d8d682eab2b3ae2c3bfe3bdc5/chrome/browser/about_flags.cc
[modify] https://crrev.com/a37fd2e1d6785d3d8d682eab2b3ae2c3bfe3bdc5/chrome/browser/password_manager/password_manager_interactive_uitest.cc
[modify] https://crrev.com/a37fd2e1d6785d3d8d682eab2b3ae2c3bfe3bdc5/components/autofill/content/renderer/password_autofill_agent.cc
[modify] https://crrev.com/a37fd2e1d6785d3d8d682eab2b3ae2c3bfe3bdc5/components/password_manager/core/common/password_manager_features.cc
[modify] https://crrev.com/a37fd2e1d6785d3d8d682eab2b3ae2c3bfe3bdc5/components/password_manager/core/common/password_manager_features.h
[modify] https://crrev.com/a37fd2e1d6785d3d8d682eab2b3ae2c3bfe3bdc5/tools/metrics/histograms/enums.xml
Project Member

Comment 15 by bugdroid1@chromium.org, Jan 29 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ee8c0f0ec6b8ec95c850883b5fbf66f25845615b

commit ee8c0f0ec6b8ec95c850883b5fbf66f25845615b
Author: nikhil <nikhil.sahni@samsung.com>
Date: Mon Jan 29 07:33:03 2018

Fixing names of password_manager kEnableHtmlBasedUsernameDetector feature.

Fixing  names of password_manager kEnableHtmlBasedUsernameDetector feature
as per the naming convention.

Bug:  785953 
Change-Id: I8818a2e2335d9a651d02ab9925bd7e4987a661a3
Reviewed-on: https://chromium-review.googlesource.com/878101
Commit-Queue: NIKHIL SAHNI <nikhil.sahni@samsung.com>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532343}
[modify] https://crrev.com/ee8c0f0ec6b8ec95c850883b5fbf66f25845615b/chrome/browser/about_flags.cc
[modify] https://crrev.com/ee8c0f0ec6b8ec95c850883b5fbf66f25845615b/components/autofill/content/renderer/password_form_conversion_utils.cc
[modify] https://crrev.com/ee8c0f0ec6b8ec95c850883b5fbf66f25845615b/components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc
[modify] https://crrev.com/ee8c0f0ec6b8ec95c850883b5fbf66f25845615b/components/password_manager/core/common/password_manager_features.cc
[modify] https://crrev.com/ee8c0f0ec6b8ec95c850883b5fbf66f25845615b/components/password_manager/core/common/password_manager_features.h
[modify] https://crrev.com/ee8c0f0ec6b8ec95c850883b5fbf66f25845615b/tools/metrics/histograms/enums.xml
Project Member

Comment 16 by bugdroid1@chromium.org, Feb 7 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/33c5e0a9db05dbd2f7793c23ac23b7aa6a556c05

commit 33c5e0a9db05dbd2f7793c23ac23b7aa6a556c05
Author: nikhil <nikhil.sahni@samsung.com>
Date: Wed Feb 07 05:49:40 2018

Fixing names of password_manager kEnableManualFallbacksFilling feature.

Fixing  names of password_manager kEnableManualFallbacksFilling feature
as per the naming convention.

Bug:  785953 
Change-Id: I4a4baa1649fe9f02c3783a5e4c40bc75e717cc03
Reviewed-on: https://chromium-review.googlesource.com/900566
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Commit-Queue: NIKHIL SAHNI <nikhil.sahni@samsung.com>
Cr-Commit-Position: refs/heads/master@{#534923}
[modify] https://crrev.com/33c5e0a9db05dbd2f7793c23ac23b7aa6a556c05/chrome/browser/about_flags.cc
[modify] https://crrev.com/33c5e0a9db05dbd2f7793c23ac23b7aa6a556c05/components/password_manager/core/browser/password_autofill_manager.cc
[modify] https://crrev.com/33c5e0a9db05dbd2f7793c23ac23b7aa6a556c05/components/password_manager/core/browser/password_autofill_manager_unittest.cc
[modify] https://crrev.com/33c5e0a9db05dbd2f7793c23ac23b7aa6a556c05/components/password_manager/core/common/password_manager_features.cc
[modify] https://crrev.com/33c5e0a9db05dbd2f7793c23ac23b7aa6a556c05/components/password_manager/core/common/password_manager_features.h
[modify] https://crrev.com/33c5e0a9db05dbd2f7793c23ac23b7aa6a556c05/tools/metrics/histograms/enums.xml

Comment 18 by vabr@chromium.org, Jun 20 2018

Description: Show this description
Project Member

Comment 20 by bugdroid1@chromium.org, Jun 26 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c0dfebd30665d9dc316700caacbb39aa18b11c14

commit c0dfebd30665d9dc316700caacbb39aa18b11c14
Author: Uladzimir Miniailau <miniailau@google.com>
Date: Tue Jun 26 11:44:49 2018

kEnableManualPasswordGeneration -> kManualPasswordGeneration

Remove unnecessary "Enable" from a password manager feature name.

Bug:  785953 
Change-Id: Icedb87e8f796c1ee58c0145de8f0bd1e3fb3e22e
Reviewed-on: https://chromium-review.googlesource.com/1114600
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Commit-Queue: Uladzimir Miniailau <miniailau@google.com>
Cr-Commit-Position: refs/heads/master@{#570375}
[modify] https://crrev.com/c0dfebd30665d9dc316700caacbb39aa18b11c14/components/password_manager/core/common/password_manager_features.cc
[modify] https://crrev.com/c0dfebd30665d9dc316700caacbb39aa18b11c14/components/password_manager/core/common/password_manager_features.h
Project Member

Comment 21 by bugdroid1@chromium.org, Jun 26 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2a4dd6f4dca335a48a9319cba30dd42ac0de691e

commit 2a4dd6f4dca335a48a9319cba30dd42ac0de691e
Author: Uladzimir Miniailau <miniailau@google.com>
Date: Tue Jun 26 14:08:35 2018

kEnableShowAllSavedPasswordsContextMen -> kShowAllSavedPasswordsContextMenu

Remove unnecessary "Enable" from a password manager feature name.

Bug:  785953 
Change-Id: I818488740dcdd450fa538c13957a9e09d1d9b017
Reviewed-on: https://chromium-review.googlesource.com/1114818
Commit-Queue: Uladzimir Miniailau <miniailau@google.com>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570396}
[modify] https://crrev.com/2a4dd6f4dca335a48a9319cba30dd42ac0de691e/components/password_manager/core/browser/password_manager_util.cc
[modify] https://crrev.com/2a4dd6f4dca335a48a9319cba30dd42ac0de691e/components/password_manager/core/common/password_manager_features.cc
[modify] https://crrev.com/2a4dd6f4dca335a48a9319cba30dd42ac0de691e/components/password_manager/core/common/password_manager_features.h

Comment 22 by miniailau@google.com, Jun 26 2018

Status: Fixed (was: Started)
►

Sign in to add a comment