[webkit-reviews] review denied: [Bug 44496] [EFL] Password manager for WebKit-Efl : [Attachment 65713] Password manager for WebKit-Efl

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Grzegorz <g.czajkowski at samsung.com>'s
request for review:
Bug 44496: [EFL] Password manager for WebKit-Efl
https://bugs.webkit.org/show_bug.cgi?id=44496

Attachment 65713: Password manager for WebKit-Efl
https://bugs.webkit.org/attachment.cgi?id=65713&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=65713&action=prettypatch

> WebKit/efl/ewk/ewk_view.cpp:4046
> +    for (unsigned i = 0; i < forms->length(); i++) {
> +	   WebCore::HTMLFormElement *formElement =
static_cast<WebCore::HTMLFormElement *> (forms->item(i));
> +	   for (unsigned j = 0; j < formElement->associatedElements().size();
j++) {	 
> +	       WebCore::HTMLFormControlElement *element =
formElement->associatedElements()[j];
> +	       if (!element->hasLocalName(WebCore::HTMLNames::inputTag))
Why just the localName?  Seems like you'd want to match the namespace too.

> WebKit/efl/ewk/ewk_view.cpp:4051
> +	       WebCore::HTMLInputElement *inputElement =
static_cast<WebCore::HTMLInputElement *>(element);
> +	       if (!inputElement->isEnabledFormControl())
> +		   continue;
This not memory safe.  Something can have a localName of "input" without being
an HTMLInputElement.

> WebKit/efl/ewk/ewk_view.cpp:4069
> +		       if ((inputElement->inputType() ==
WebCore::HTMLInputElement::TEXT)) {
> +			   inputElement->setValue(username);
> +			   return true;
> +		       }
This whole thing looks like overly much poking at WebCore from the WebKit
layer.	If this logic is right, does every port need to copy/paste this method?
 Does this code exist in other ports?  Should we move the correct
implementation to WebCore proper?