Bug 78532

Summary: Last character display for passwords in Android.
Product: WebKit Reporter: Ramya Chandrasekaran <cramya>
Component: New BugsAssignee: Ramya Chandrasekaran <cramya>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, fishd, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

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.