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

Status: RESOLVED FIXED

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

Description

[Chenxia Liu [:liuche]]

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).

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Yeap. Let's unify and refer to Logins where appropriate.

Comment 2

[Allison Naaktgeboren :ally]

so to confirm,  we want to use 'logins' every place we use the term 'passwords' and rename about:passwords to about:logins?

Comment 3

[Chenxia Liu [:liuche]]

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?

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Yep!

Comment 5

[Bill Maggs (bmaggs)]

Attachment: loggins2.png

Team mascot.

Comment 6

[Allison Naaktgeboren :ally]

Attachment: unifyPasswordAndLoginsNomenclature wip

IIIIIIII went to the danger zone!

tests need to be updated

Comment 7

[Allison Naaktgeboren :ally]

Attachment: unifyPasswordAndLoginsNomenclature wip 2

+ tests

Comment 8

[Allison Naaktgeboren :ally]

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

Comment 9

[Allison Naaktgeboren :ally]

hopefully this is the last of the test foo. https://treeherder.mozilla.org/#/jobs?repo=try&revision=71dc90b90b1a

Comment 10

[Allison Naaktgeboren :ally]

Attachment: unifyPasswordAndLoginsNomenclature wip 3

Comment 11

[Allison Naaktgeboren :ally]

Comment on attachment 8616259 [details] [diff] [review]
unifyPasswordAndLoginsNomenclature wip 3

all my rc1s have gone green, so lets have a look through

Comment 12

[Allison Naaktgeboren :ally]

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 13

[:Margaret Leibovic]

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?

Comment 14

[Allison Naaktgeboren :ally]

(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.

Comment 15

[:Margaret Leibovic]

(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.

Comment 16

[Allison Naaktgeboren :ally]

Attachment: unifyPasswordAndLoginsNomenclature wip 4

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. :)

Comment 17

[Allison Naaktgeboren :ally]

Attachment: unifyPasswordAndLoginsNomenclature wip 5

- aboutPasswords.properties did not get converted over in wip 4, so dialog was not popping up. 

- try still down.

Comment 18

[Allison Naaktgeboren :ally]

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?

Comment 19

[Chenxia Liu [:liuche]]

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"

Comment 20

[Allison Naaktgeboren :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.

Comment 21

[: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.

Comment 22

[Chenxia Liu [:liuche]]

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.

Comment 23

[Allison Naaktgeboren :ally]

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.

Comment 24

[Allison Naaktgeboren :ally]

 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.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a10852989627

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a10852989627

Comment 27

[Allison Naaktgeboren :ally]

Attachment: unificationSystemAndSyncSettings part 2/2

rc5 fails on clean trunk (so it shouldn't block), but otherwise try is clean, so time to review.

Comment 28

[Chenxia Liu [:liuche]]

Hm, do you have the try run? I don't see any changes to testAboutSettings, and that should definitely break with these changes.

Comment 29

[Allison Naaktgeboren :ally]

of course: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e422447664cc

Comment 30

[Chenxia Liu [:liuche]]

Yeah, the Settings tests run in rc5, so you definitely should make sure they run because they will break with this patch.

Comment 31

[Chenxia Liu [:liuche]]

/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.

Comment 32

[Chenxia Liu [:liuche]]

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.

Comment 33

[:Margaret Leibovic]

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 :)

Comment 34

[Chenxia Liu [: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.

Comment 35

[Allison Naaktgeboren :ally]

I have incorporated those strings relevant to this bug into StringHelper, and filed Bug 1174878 for the testMenuSettings.java file more generally.

Comment 36

[Chenxia Liu [:liuche]]

Great, I'm happy with that - can you make a try push when you flag me for review?

Comment 37

[Allison Naaktgeboren :ally]

Attachment: part 2/2

Please see above comment :)

Comment 38

[Allison Naaktgeboren :ally]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=974b65d16a2a for rc 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=718885d35ab5 for everything else

Comment 39

[Allison Naaktgeboren :ally]

Attachment: unificationSystemAndSyncSettings

Comment 40

[Chenxia Liu [:liuche]]

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.

Comment 41

[Allison Naaktgeboren :ally]

 https://hg.mozilla.org/integration/fx-team/rev/9a24aa4d7e69

Comment 42

[Allison Naaktgeboren :ally]

I'm a little confused why this hasnt been merged & resolved.

Comment 43

[Ryan VanderMeulen [: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