Closed Bug 673969 Opened 8 years ago Closed 8 years ago

System preference for "visible passwords" not followed

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: bzbarsky, Assigned: alexp)

Details

Attachments

(1 file, 2 obsolete files)

BUILD: Current fennec nightly

STEPS TO REPRODUCE:
1)  Get yourself an Android 3.1 device (in my case an eepad transformer)
2)  Go into the system Settings > Location and Security
3)  Uncheck the "Visible Passwords (show password as you type)" checkbox
4)  Start Nightly
5)  Go to type a password on some site

ACTUAL RESULTS: Last char of password is shown as I type

EXPECTED RESULTS: Given step 3 above, should probably not do that
OS: Mac OS X → Android
Hardware: x86 → ARM
Brad - Assign to a mobile platform dev?
Assignee: nobody → alexp
Also occurs in sync ui/chrome.
Attached patch Fix (obsolete) — Splinter Review
Added a check for Android system setting TEXT_SHOW_PASSWORD.

Note: that value is checked only once, on the first attempt to enter a password, so if the system setting is changed later while Firefox is running, the new value will not be read until the browser restarts. I believe this is fine, as I don't think the users are changing this setting back and forth. A simple alternative would be to get the setting from the system on every character when nsTextEditRules needs this flag, but this doesn't make sense. And a more complex implementation is not really needed.
Attachment #548671 - Flags: review?(blassey.bugs)
drive-by:

>+ContentParent::RecvGetShowPasswordSetting(PRInt32* showPassword)
>+{
>+#ifdef ANDROID
>+    if (!AndroidBridge::Bridge())
>+        return false;
>+
>+    *showPassword = AndroidBridge::Bridge()->GetShowPasswordSetting();
>+#endif
>+    return true;
>+}

Will this default to the right mobile setting for non-Android platforms? For example, on Maemo/Meego, we still want the "show the last character" behavior.
(In reply to comment #4)

> Will this default to the right mobile setting for non-Android platforms? For
> example, on Maemo/Meego, we still want the "show the last character"
> behavior.

The patch overrides nsILookAndFeel::GetEchoPassword() virtual method only for Android. The systems, where it's not overridden have this default implementation:
http://mxr.mozilla.org/mozilla-central/source/widget/public/nsILookAndFeel.h#368

So, the short answer: on Maemo/Meego the behavior will not change.
Comment on attachment 548671 [details] [diff] [review]
Fix

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

::: dom/ipc/ContentParent.cpp
@@ +595,5 @@
> +ContentParent::RecvGetShowPasswordSetting(PRInt32* showPassword)
> +{
> +#ifdef ANDROID
> +    if (!AndroidBridge::Bridge())
> +        return false;

return true here and set *showPassword to true, as I understand it returning false is near-fatal

@@ +598,5 @@
> +    if (!AndroidBridge::Bridge())
> +        return false;
> +
> +    *showPassword = AndroidBridge::Bridge()->GetShowPasswordSetting();
> +#endif

and a #else and set *showPassword to true for other platforms

::: embedding/android/GeckoAppShell.java
@@ +1315,5 @@
>  
>          return activityInfo.loadIcon(pm);
>      }
> +
> +    public static int getShowPasswordSetting() {

this method should return a boolean, its either on or off

::: widget/src/android/AndroidBridge.cpp
@@ +756,5 @@
>  }
>  
> +int
> +AndroidBridge::GetShowPasswordSetting()
> +{

make this a boolean as well

::: widget/src/android/AndroidBridge.h
@@ +211,5 @@
>      void GetSystemColors(AndroidSystemColors *aColors);
>  
>      void GetIconForExtension(const nsACString& aFileExt, PRUint32 aIconSize, PRUint8 * const aBuf);
>  
> +    int GetShowPasswordSetting();

bool
Attachment #548671 - Flags: review?(blassey.bugs) → review+
(In reply to comment #6)
> > +    public static int getShowPasswordSetting() {
> 
> this method should return a boolean, its either on or off

Frankly speaking - yes. I decided to have int here for two reasons: the setting itself is int (though it's either 0 or 1); it makes it easier to detect if the setting was obtained or not (initialized to -1 by default). But I didn't feel too strong about choosing this type. Boolean makes more sense - I agree.
Attached patch Fix v2 (obsolete) — Splinter Review
Addressed review comments.

r=blassey
Attachment #548671 - Attachment is obsolete: true
Attachment #548993 - Flags: review+
Attached patch Fix v2.1Splinter Review
Updated to the latest tree.
r=blassey
Attachment #548993 - Attachment is obsolete: true
Attachment #553335 - Flags: review?
Attachment #553335 - Flags: review? → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/c86cbbb7d0bf
Status: NEW → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Verified Fixed
Mozilla/5.0 (Android; Linux armv7l; rv:8.0a1) Gecko/20110816 Firefox/8.0a1 Fennec/8.0a1

Litmus:
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=25826
Status: RESOLVED → VERIFIED
Flags: in-litmus+
You need to log in before you can comment on or make changes to this bug.