[webkit-reviews] review denied: [Bug 30023] [Qt] Software input panel is not launched for password fields : [Attachment 40551] [Qt] patch to make the software input panel work for password fields

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Joseph Ligman
<joseph.ligman at nokia.com>'s request for review:
Bug 30023: [Qt] Software input panel is not launched for password fields
https://bugs.webkit.org/show_bug.cgi?id=30023

Attachment 40551: [Qt] patch to make the software input panel work for password
fields
https://bugs.webkit.org/attachment.cgi?id=40551&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>

> +	   case Qt::ImPasswordInput: {
> +	       if (frame->selection()->isContentEditable()) {
> +		   if (frame->document() && frame->document()->focusedNode()) {

> +		       if
(frame->document()->focusedNode()->hasTagName(HTMLNames::inputTag)) {
> +			   HTMLInputElement* inputElement =
static_cast<HTMLInputElement*>(frame->document()->focusedNode());
> +			   return QVariant(inputElement->isPasswordField());
> +		       }
> +		   }
> +	       }
> +	       return QVariant(false);

Given QVariant's implicit constructors it should be sufficient to simply
"return false;" here :)

> -    if (view)
> +    if (view) {
> +	   if (!active) {
> +	       Frame* frame =
m_page->d->page->focusController()->focusedOrMainFrame();
> +	       if (frame && frame->document() &&
frame->document()->focusedNode()) {
> +		   if
(frame->document()->focusedNode()->hasTagName(HTMLNames::inputTag)) {
> +		       HTMLInputElement* inputElement =
static_cast<HTMLInputElement*>(frame->document()->focusedNode());
> +		       if (inputElement->isPasswordField()) {
> +			   view->setAttribute(Qt::WA_InputMethodEnabled, true);

> +			   emit m_page->microFocusChanged();
> +			   return;

Wouldn't it be simpler to just replace the last three lines there with

    active = true;

Also, please add a comment explaining why we are diverging from the default
behaviour here. (the fact that in Qt we let the input method determine this and
that on the desktop
platforms the qt input method layer will not permit composition of these
fields, for security)

Given the code duplication I'm wondering if it would be worth to centralize the
above duplicated
code to determine if the focused node is a password protected input field into
a helper method?


Please also add a unit test for the behaviour of still enabling input methods
when
password fields are focused and that only in that case we are also returning
the ImPasswordInput
hint only for these fields.