Bug 66052 – Add support of setPasswordEchoEnabled and setPasswordEchoDuration for password echo feature

• Component: Forms
• Status: RESOLVED FIXED

[Chang Shu]

Add the above support in page/Settings and LayoutTestController so user can enable/disable password echo feature and control the echo duration.

[Chang Shu]

Created attachment 103621 [details]
fix patch

[Alexey Proskuryakov]

Comment on attachment 103621 [details]
fix patch

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

Most of the comments are not mandatory to address, but I think that you'll want to use either overridePreference or window.internals.

> Source/WebCore/page/Settings.cpp:111
> +    , m_passwordEchoDuration(1.0)

I think that it's just "1" per coding style.

I like to have "Seconds" in variables that measure time in seconds to avoid potential confusion.

> Source/WebCore/page/Settings.cpp:213
> +    , m_isPasswordEchoEnabled(false)

There is lots of precedent in this class already, but it is grammatically incorrect to say "if is password data enabled". We try to name boolean flags in a way that would make such checks more readable.

> Source/WebKit/mac/WebView/WebPreferences.mm:383
> +        [NSNumber numberWithDouble:2.0],   WebKitPasswordEchoDurationPreferenceKey,

Why is this different from WebCore default?

> Tools/DumpRenderTree/LayoutTestController.cpp:2442
> +        { "setPasswordEchoEnabled", setPasswordEchoEnabledCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },
> +        { "setPasswordEchoDuration", setPasswordEchoDurationCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },

There is a bunch of considerations here, and I'm not sure what the best practice is now:
1. As there is a preference key for this, you could/should use overridePreference instead of implementing a custom method.
2. But I'm not sure if preferences set with overridePreference are correctly reset between tests.
3. Hacking into WebCore is now done via window.internals, not layoutTestController. Layout test controller is better in that it goes through WebKit APIs and thus tests those, but this API is trivial, and supporting internals on all platforms is much easier.

> LayoutTests/editing/input/password-echo-settings.html:10
> +<p> Test enable password echo and set echo duration.

This does not really test any functionality. If you intend to a real test, why not do it now?

> LayoutTests/platform/wk2/Skipped:1870
> +# WebKitTestRunner needs layoutTestController.setPasswordEchoEnabled
> +# WebKitTestRunner needs layoutTestController.setPasswordEchoDuration
> +editing/input/password-echo-settings.html

WebKit2 is the main WebKit, I think that it's important to support it.

[Chang Shu]

Thanks for the review, ap. I added my comments below.

> > Source/WebKit/mac/WebView/WebPreferences.mm:383
> > +        [NSNumber numberWithDouble:2.0],   WebKitPasswordEchoDurationPreferenceKey,
> 
> Why is this different from WebCore default?

I noticed that the default value on Nokia phones has been 1 second but on iOS it's 2 seconds.
> 
> > Tools/DumpRenderTree/LayoutTestController.cpp:2442
> > +        { "setPasswordEchoEnabled", setPasswordEchoEnabledCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },
> > +        { "setPasswordEchoDuration", setPasswordEchoDurationCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },
> 
> There is a bunch of considerations here, and I'm not sure what the best practice is now:
> 1. As there is a preference key for this, you could/should use overridePreference instead of implementing a custom method.
> 2. But I'm not sure if preferences set with overridePreference are correctly reset between tests.
> 3. Hacking into WebCore is now done via window.internals, not layoutTestController. Layout test controller is better in that it goes through WebKit APIs and thus tests those, but this API is trivial, and supporting internals on all platforms is much easier.

It's great to have window.internals. It's easy to implement and it's cross-platform. But on the second thought, I think we need WebKit API for this. The user needs to enable the feature in their cpp code, right?

> 
> > LayoutTests/editing/input/password-echo-settings.html:10
> > +<p> Test enable password echo and set echo duration.
> 
> This does not really test any functionality. If you intend to a real test, why not do it now?

Do you suggest I move my original password echo tests in 32509 here and leave them failed? Sounds good to me.
> 
> > LayoutTests/platform/wk2/Skipped:1870
> > +# WebKitTestRunner needs layoutTestController.setPasswordEchoEnabled
> > +# WebKitTestRunner needs layoutTestController.setPasswordEchoDuration
> > +editing/input/password-echo-settings.html
> 
> WebKit2 is the main WebKit, I think that it's important to support it.

I will do this in a separate patch.

[Alexey Proskuryakov]

> It's great to have window.internals. It's easy to implement and it's cross-platform. 
> But on the second thought, I think we need WebKit API for this. The user needs to 
> enable the feature in their cpp code, right?

Perhaps I'm confused, but I thought that platform specific code in WebKit would do that (e.g. always on on iOS). Do applications embedding WebKit need control over this?

[Chang Shu]

> Perhaps I'm confused, but I thought that platform specific code in WebKit would do that (e.g. always on on iOS). Do applications embedding WebKit need control over this?

The platform itself does not necessarily determine if password echo should be enabled or not. E.g., the qt port may work on desktops and mobile devices with various hardware configurations. This is why I was thinking leaving the choice to the application code.

[Alexey Proskuryakov]

I understand that Qt port works on multiple platforms, but it seems that it's still in a good position to make the choice. It's a platform standard, so applications shouldn't be able to change it.

[Chang Shu]

Created attachment 103769 [details]
fix patch 2

[Alexey Proskuryakov]

Comment on attachment 103769 [details]
fix patch 2

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

I'm not saying r=me because I don't understand what will reset the setting back to default after a test is finished. Is there some code that just resets all settings?

> Source/WebCore/page/Settings.h:464
> +        void setPasswordEchoEnabled(bool flag) { m_passwordEchoEnabled = flag; };

Unneeded semicolon at the end.

> Source/WebCore/page/Settings.h:467
> +        void setPasswordEchoDuration(double durationInSeconds) { m_passwordEchoDurationInSeconds = durationInSeconds; };

Ditto.

> Source/WebCore/testing/Internals.cpp:207
> +    if (!document || !document->settings()) {
> +        ec = INVALID_ACCESS_ERR;
> +        return;
> +    }

I don't think that there is a point in making these checks and raising an exception. But that seems to match common pattern in this file.

> Source/WebCore/testing/Internals.idl:49
> +        void setPasswordEchoEnabled(in Document document, in boolean enabled) raises(DOMException);
> +        void setPasswordEchoDuration(in Document document, in double durationInSeconds) raises(DOMException);

What's the reason to pass in a document? Can't we just get a page pointer from the window object this Internals instance lives on?

> Source/WebKit/qt/Api/qwebsettings.cpp:290
> +        settings->setPasswordEchoDuration(1);

You didn't put "InSeconds" in function names, so call sites like this one are less obvious than they could be.

> LayoutTests/editing/input/resources/password-echo.js:83
> +            window.internals.setPasswordEchoDuration(document, 0.1);

Again, not having "InSeconds" in API name makes this slightly more confusing than it could be.

[Chang Shu]

> > Source/WebCore/testing/Internals.cpp:207
> > +    if (!document || !document->settings()) {
> > +        ec = INVALID_ACCESS_ERR;
> > +        return;
> > +    }
> 
> I don't think that there is a point in making these checks and raising an exception. But that seems to match common pattern in this file.

Yeah, it seems all functions do this. It's a bit over-engineered but it's probably good to keep the pattern.

> > +        void setPasswordEchoDuration(in Document document, in double durationInSeconds) raises(DOMException);
> 
> What's the reason to pass in a document? Can't we just get a page pointer from the window object this Internals instance lives on?

I don't see the current implementation passes window object to Internals. So from Internals, no way we can figure out which page it belongs to. Again, it seems all functions are doing this.

WRT the resetting issue we discussed on IRC, I think it's better to let Internals automatically backs up values during the 1st set call than resetting the values to a hardcoded defaults in the reset function. The original value could be set by WebCore, WebKit, or even application (DRT). What do you think?

[Alexey Proskuryakov]

JSInternals inherits from JSObjectWithGlobalObject, so it has a pointer to window.

[Dimitri Glazkov (Google)]

> WRT the resetting issue we discussed on IRC, I think it's better to let Internals automatically backs up values during the 1st set call than resetting the values to a hardcoded defaults in the reset function. The original value could be set by WebCore, WebKit, or even application (DRT). What do you think?

Sounds good.

[Chang Shu]

Created attachment 104240 [details]
patch 3

[Alexey Proskuryakov]

Comment on attachment 104240 [details]
patch 3

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

> LayoutTests/platform/wk2/Skipped:1854
> +# WebKitTestRunner needs layoutTestController.setPasswordEchoEnabled
> +# WebKitTestRunner needs layoutTestController.setPasswordEchoDuration

s/layoutTestController/internals/, and I don't understand why it doesn't just work.

[Chang Shu]

> > LayoutTests/platform/wk2/Skipped:1854
> > +# WebKitTestRunner needs layoutTestController.setPasswordEchoEnabled
> > +# WebKitTestRunner needs layoutTestController.setPasswordEchoDuration
> 
> s/layoutTestController/internals/, and I don't understand why it doesn't just work.

The reset support hasn't been implemented yet. I will take care of wk2 later.

[WebKit Review Bot]

Comment on attachment 104240 [details]
patch 3

Clearing flags on attachment: 104240

Committed r93291: <http://trac.webkit.org/changeset/93291>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chang Shu]

A note for people have cherry-picked the previous passwordecho patch:
The feature is now under a build flag on Qt. It is no longer tied automatically with composite input mode.
On other platforms, the feature is always disabled since no code is implemented in WebKit.