Bug 78532 - Last character display for passwords in Android.
Summary: Last character display for passwords in Android.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ramya Chandrasekaran
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-13 14:00 PST by Ramya Chandrasekaran
Modified: 2012-02-14 14:35 PST (History)
4 users (show)

See Also:


Attachments
Patch (5.82 KB, patch)
2012-02-13 14:02 PST, Ramya Chandrasekaran
no flags Details | Formatted Diff | Diff
Patch (5.82 KB, patch)
2012-02-13 14:16 PST, Ramya Chandrasekaran
no flags Details | Formatted Diff | Diff
Patch (5.03 KB, patch)
2012-02-14 13:43 PST, Ramya Chandrasekaran
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ramya Chandrasekaran 2012-02-13 14:00:51 PST
Last character display for passwords in Android.
Comment 1 Ramya Chandrasekaran 2012-02-13 14:02:24 PST
Created attachment 126829 [details]
Patch
Comment 2 WebKit Review Bot 2012-02-13 14:04:19 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Ramya Chandrasekaran 2012-02-13 14:16:34 PST
Created attachment 126835 [details]
Patch
Comment 4 Adam Barth 2012-02-13 14:39:19 PST
Comment on attachment 126835 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126835&action=review

The rest of the change looks fine.

> Source/WebCore/page/Settings.cpp:233
> +#if OS(ANDROID)
> +    , m_passwordEchoEnabled(true)
> +#else
>      , m_passwordEchoEnabled(false)
> +#endif

This isn't correct.  The Android should change this preference via the Chromium WebKit API.  We use the same initializations across all the ports.
Comment 5 Kent Tamura 2012-02-13 15:16:05 PST
Comment on attachment 126835 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126835&action=review

> Source/WebCore/ChangeLog:9
> +        No new tests. (OOPS!)
> +

This line should be removed or modified.

> Tools/DumpRenderTree/chromium/WebPreferences.cpp:248
> +#if OS(ANDROID)
> +    // By default, this is set to true for ANDROID in Settings.cpp.
> +    // This should be false for LayoutTests.
> +    settings->setPasswordEchoEnabled(false);
> +#endif

#if and #endif are not needed.
Comment 6 Darin Fisher (:fishd, Google) 2012-02-13 15:24:09 PST
Comment on attachment 126835 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126835&action=review

> Source/WebKit/chromium/public/WebSettings.h:135
> +    virtual void setPasswordEchoEnabled(bool) = 0;

API changes LGTM
Comment 7 Ramya Chandrasekaran 2012-02-14 13:43:49 PST
Created attachment 127035 [details]
Patch
Comment 8 Adam Barth 2012-02-14 14:10:00 PST
Comment on attachment 127035 [details]
Patch

Looks great.  Thanks!
Comment 9 WebKit Review Bot 2012-02-14 14:35:47 PST
Comment on attachment 127035 [details]
Patch

Clearing flags on attachment: 127035

Committed r107739: <http://trac.webkit.org/changeset/107739>
Comment 10 WebKit Review Bot 2012-02-14 14:35:52 PST
All reviewed patches have been landed.  Closing bug.