673969 - System preference for "visible passwords" not followed

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Brad - Assign to a mobile platform dev?

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Also occurs in sync ui/chrome.

Comment 3

[Alex Pakhotin (:alexp)]

Attachment: Fix

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.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 5

[Alex Pakhotin (:alexp)]

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

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 7

[Alex Pakhotin (:alexp)]

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

Comment 8

[Alex Pakhotin (:alexp)]

Attachment: Fix v2

Addressed review comments.

r=blassey

Comment 9

[Alex Pakhotin (:alexp)]

Attachment: Fix v2.1

Updated to the latest tree.
r=blassey

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

http://hg.mozilla.org/mozilla-central/rev/c86cbbb7d0bf

Comment 11

[Aaron Train [:aaronmt]]

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