[webkit-reviews] review denied: [Bug 66052] Add support of setPasswordEchoEnabled and setPasswordEchoDuration for password echo feature : [Attachment 103621] fix patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Chang Shu <cshu at webkit.org>'s
request for review:
Bug 66052: Add support of setPasswordEchoEnabled and setPasswordEchoDuration
for password echo feature
https://bugs.webkit.org/show_bug.cgi?id=66052

Attachment 103621: fix patch
https://bugs.webkit.org/attachment.cgi?id=103621&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.