Closed Bug 1136477 Opened 5 years ago Closed 5 years ago

Unify terminology of Passwords/Logins for about:logins (nee about:passwords)

Categories

(Firefox for Android :: General, defect, P1)

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: liuche, Assigned: ally)

References

(Blocks 1 open bug)

Details

User Story

as a Fx user I want labels and buttons to refer to tell me Firefox is capturing and filling full login credentials, not just passwords.

Attachments

(3 files, 6 obsolete files)

We talked about using "Logins" instead of "Passwords" because it's a little more descriptive, so we should consider whether we want to update our strings to say Login instead of Passwords (in Settings, about:passwords, etc).
Yeap. Let's unify and refer to Logins where appropriate.
Blocks: 1167657
Rank: 15
Priority: -- → P1
User Story: (updated)
Assignee: nobody → ally
so to confirm,  we want to use 'logins' every place we use the term 'passwords' and rename about:passwords to about:logins?
Flags: needinfo?(alam)
We want to use "logins" where appropriate, i.e., when it refers to a username/password combination. So "Show password" should still stay the same, but about:passwords => about:logins.

So definitely changes in about:passwords, but I'm not sure about other places - antlam?
Yep!
Flags: needinfo?(alam)
Attached image loggins2.png
Team mascot.
IIIIIIII went to the danger zone!

tests need to be updated
+ tests
Attachment #8615096 - Attachment is obsolete: true
bit rotting got a bit funky when i moved it between trees. lets see what treeherder says

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b81a0dae5bc
hopefully this is the last of the test foo. https://treeherder.mozilla.org/#/jobs?repo=try&revision=71dc90b90b1a
Attachment #8615101 - Attachment is obsolete: true
Comment on attachment 8616259 [details] [diff] [review]
unifyPasswordAndLoginsNomenclature wip 3

all my rc1s have gone green, so lets have a look through
Attachment #8616259 - Flags: feedback?(margaret.leibovic)
Note that this is f? and not r? because the patch will rot with Chenxia's changes, and Margaret would like a new version with less churn in the file renaming. I'd like to know what else I'll need for an r+ so that I only have to do one more pass on the patch.
Comment on attachment 8616259 [details] [diff] [review]
unifyPasswordAndLoginsNomenclature wip 3

Review of attachment 8616259 [details] [diff] [review]:
-----------------------------------------------------------------

I mentioned this on IRC but it might have gone by unnoticed... can you please update this patch to use 'hg mv' to rename files, rather than removing/adding? That would make it much easier to review this patch, and it would also better preserve the history of these files.

::: mobile/android/base/AboutPages.java
@@ -18,5 @@
>      public static final String DOWNLOADS       = "about:downloads";
>      public static final String FIREFOX         = "about:firefox";
>      public static final String HEALTHREPORT    = "about:healthreport";
>      public static final String HOME            = "about:home";
> -    public static final String PASSWORDS       = "about:passwords";

Rename this to LOGINS?
Attachment #8616259 - Flags: feedback?(margaret.leibovic) → feedback-
(In reply to :Margaret Leibovic from comment #13)
> Comment on attachment 8616259 [details] [diff] [review]
> unifyPasswordAndLoginsNomenclature wip 3
> 
> Review of attachment 8616259 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I mentioned this on IRC but it might have gone by unnoticed... can you
> please update this patch to use 'hg mv' to rename files, rather than
> removing/adding? That would make it much easier to review this patch, and it
> would also better preserve the history of these files.

It went neither unnoticed nor undocumented, see comment 12.
(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #14)
> (In reply to :Margaret Leibovic from comment #13)
> > Comment on attachment 8616259 [details] [diff] [review]
> > unifyPasswordAndLoginsNomenclature wip 3
> > 
> > Review of attachment 8616259 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I mentioned this on IRC but it might have gone by unnoticed... can you
> > please update this patch to use 'hg mv' to rename files, rather than
> > removing/adding? That would make it much easier to review this patch, and it
> > would also better preserve the history of these files.
> 
> It went neither unnoticed nor undocumented, see comment 12.

Sorry about that! This patch generally seems fine, but it's hard to see what's actually changed with the removed/added files.
I couldn't hg mv over the existing patch (hg kept aborting the operation) so I made you a shiny new one! It should be much easier to read.

Since it's a brand new patch, I'd like to run it through try before bothering you for review in case I missed anything. Try is closed currently, but hopefully that'll re-open shortly. If try hasn't opened by tomorrow morning, I might send it to you for review anyway. :)
Attachment #8616259 - Attachment is obsolete: true
- aboutPasswords.properties did not get converted over in wip 4, so dialog was not popping up. 

- try still down.
Attachment #8616966 - Attachment is obsolete: true
Comment on attachment 8617049 [details] [diff] [review]
unifyPasswordAndLoginsNomenclature wip 5

Still no eta on when try & the rest of the trees will re-open, so I can haz review?
Attachment #8617049 - Flags: review?(margaret.leibovic)
Attachment #8617049 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8617049 [details] [diff] [review]
unifyPasswordAndLoginsNomenclature wip 5

Review of attachment 8617049 [details] [diff] [review]:
-----------------------------------------------------------------

If you're going to the level of changing variables (!) I found a few other places where we can be more consistent:

I'm clearing the review flag because I'd like to see these other easy places get changed as well, so we can be consistent.

Settings:
* "Control passwords, cookies, tracking data"
* "Remember passwords"
* "Saved passwords"
* "Sync your tabs, bookmarks, passwords, history"

Sync:
* "Sign in to sync your tabs, bookmarks, passwords..."
* "Passwords"
Attachment #8617049 - Flags: review+
Flags: needinfo?(ally)
Margaret? Do you want this in a followup patch, a followup bug, or rolled into this one? 

I deliberately did not touch sync or its strings.
Flags: needinfo?(margaret.leibovic)
To avoid too much scope creep, I think we should split those sync/settings changes into another bug, and keep this bug focused on about:logins.

But good catch, Chenxia, I think it's important to audit our app for consistency when building features that touch a bunch of different parts of the app. However, once we start touching things like sync preferences, we should also talk to the desktop/iOS teams to make sure we're consistent across our products.
Flags: needinfo?(margaret.leibovic)
Sure, I filed this bug with the intent of updating Settings too (comment #0) but we can split it into another bug - however, updating the Settings strings to be consistent should really the block about:passwords v1, and we should update this bug name to reflect this being about:logins only.
Flags: needinfo?(ally)
Summary: Unify terminology of Passwords/Logins → Unify terminology of Passwords/Logins for about:logins (nee about:passwords)
Then we can leave this open and land a follow up patch. The first one is already sufficiently large that I an hesitant to make it any larger and my initial attempts to enlarge it caused a bevy of test failures that Id rather tackle in its own patch.

Hg ate my aboutLogins.properties, so the rebased variant is https://treeherder.mozilla.org/#/jobs?repo=try&revision=e83338eccded  (since hg did not alert me to the missing file, I am concerned it what else might be missing so am double-checking to avoid the embarrassment of backout). Assuming that clears try Ill land the first patch.
Whiteboard: [leave-open]
 https://hg.mozilla.org/integration/fx-team/rev/a10852989627

rc5 sent me off on a wild goose chase as it failed on the above run.

However it turns our rc5 fails with the same messages on a clean copy of fx-team, so must be unrelated.
rc5 fails on clean trunk (so it shouldn't block), but otherwise try is clean, so time to review.
Attachment #8621667 - Flags: review?(margaret.leibovic)
Hm, do you have the try run? I don't see any changes to testAboutSettings, and that should definitely break with these changes.
Flags: needinfo?(ally)
Yeah, the Settings tests run in rc5, so you definitely should make sure they run because they will break with this patch.
/me flips tables
After talking to mcomella I realized that testSettingsMenuItems has been disabled on all platforms, instead of just 2.3 and 4.3 as it states in robocop.ini. Bug 1171789 is the breakage.
Depends on: 1144898
mcomella is testing a fix for bug 1171789 right now, which should fix this test on fx-team (and possibly allow us to re-enable this on 4.3?). Ally, you should pull in his patch when he's done with it, and then fix your strings on top of it.

Part of the problem is that a lot of the labels in testSettingsMenuItems are hardcoded in the test, instead of using StringHelper. Ally, while you're fixing this test for the changes you added, you could also go in and pull the hard-coded strings out to StringHelper, to make the test more robust against future problems.
Depends on: 1171789
Comment on attachment 8621667 [details] [diff] [review]
unificationSystemAndSyncSettings part 2/2

Review of attachment 8621667 [details] [diff] [review]:
-----------------------------------------------------------------

liuche seems to know more than me about what's going on here :)
Attachment #8621667 - Flags: review?(margaret.leibovic) → review?(liuche)
Comment on attachment 8621667 [details] [diff] [review]
unificationSystemAndSyncSettings part 2/2

Ok, mcomella landed bug 1171789 on fx-team, so Ally, you're good to go to fix the stuff in comment #32. Flag me for review when you've fixed the tests for this.
Flags: needinfo?(ally)
Attachment #8621667 - Flags: review?(liuche)
I have incorporated those strings relevant to this bug into StringHelper, and filed Bug 1174878 for the testMenuSettings.java file more generally.
Flags: needinfo?(ally)
Great, I'm happy with that - can you make a try push when you flag me for review?
Attached patch part 2/2 (obsolete) — Splinter Review
Please see above comment :)
Attachment #8621667 - Attachment is obsolete: true
Attachment #8622739 - Flags: review?(liuche)
Attachment #8622739 - Flags: review?(liuche)
Attachment #8622739 - Attachment is obsolete: true
Attachment #8622763 - Flags: review?(liuche)
Whiteboard: [leave-open]
Blocks: 1175279
Comment on attachment 8622763 [details] [diff] [review]
unificationSystemAndSyncSettings

Review of attachment 8622763 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments addressed.

::: mobile/android/tests/browser/robocop/StringHelper.java
@@ +149,4 @@
>      public final String MANAGE_LOGINS_LABEL;
>      public final String MASTER_PASSWORD_LABEL;
>      public final String CLEAR_PRIVATE_DATA_LABEL;
> +    public final String PRIVATE_DATA_PASSWORDS_LABEL;

Where is this used?

@@ +330,4 @@
>          MANAGE_LOGINS_LABEL = res.getString(R.string.pref_manage_logins);
>          MASTER_PASSWORD_LABEL = res.getString(R.string.pref_use_master_password);
>          CLEAR_PRIVATE_DATA_LABEL = res.getString(R.string.pref_clear_private_data);
> +        PRIVATE_DATA_PASSWORDS_LABEL = res.getString(R.string.pref_private_data_passwords);

If you need this, rename it to be *LOGINS_LABEL.
Attachment #8622763 - Flags: review?(liuche) → review+
I'm a little confused why this hasnt been merged & resolved.
Flags: needinfo?(ryanvm)
It was on Monday. Looks like the bug didn't get marked. Note for future reference that hg hashes are immutable, so you can verify that the same hash exists on mozilla-central to confirm a merge whether the bug was commented or not.

https://hg.mozilla.org/mozilla-central/rev/9a24aa4d7e69
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(ryanvm)
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.