Bug 66052

Created attachment 103621 [details] fix patch

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.

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.

> 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?

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

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.

Created attachment 103769 [details] fix patch 2

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.

> > 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?

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

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

Created attachment 104240 [details] patch 3

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.

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

Comment on attachment 104240 [details] patch 3 Clearing flags on attachment: 104240 Committed r93291: <http://trac.webkit.org/changeset/93291>

All reviewed patches have been landed. Closing bug.

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.