[webkit-reviews] review denied: [Bug 174394] Expose way to set values of classified form controls as {Legacy WebKit, WebKit} SPI : [Attachment 318452] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Frederik Riedel <riedel at apple.com>'s
request for review:
Bug 174394: Expose way to set values of classified form controls as {Legacy
WebKit, WebKit} SPI
https://bugs.webkit.org/show_bug.cgi?id=174394

Attachment 318452: Patch

https://bugs.webkit.org/attachment.cgi?id=318452&action=review




--- Comment #24 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 318452
  --> https://bugs.webkit.org/attachment.cgi?id=318452
Patch

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

r- due to various issues. Please make sure this patch builds for iOS 10 as well
as Mac.

> Source/WebCore/html/HTMLInputElement.cpp:903
> +bool HTMLInputElement::acceptsAutofilledLoginCredentials()
> +{
> +    RefPtr<HTMLInputElement> usernameElement = nullptr;
> +    RefPtr<HTMLInputElement> passwordElement = nullptr;
> +    return computeUsernameAndPasswordElements(this, usernameElement,
passwordElement, this->document());
> +}
> +

I don't think we need to add this method to HTMLInputElement.
Whoever calls this function can just check the return value instead.

> Source/WebCore/html/HTMLInputElement.cpp:907
> +bool HTMLInputElement::isAssistableElement()
> +{
> +    return isTextField() || isDateField() || isDateTimeLocalField() ||
isMonthField() || isTimeField();
> +}

Given assisted node has a very specific meaning on iOS, I don't think we should
event new terminology like "assitable element".
Now, how does filling date time field with username help at all?

I'd suggest supportsAutofill or something. But I don't think there's any reason
this needs to be a method of HTMLInputElement.
It's a lot better for this function be just a static local function which takes
HTMLInputElement as an argument.

I don't think we should be checking that. If anything we should be checking for
isTextField() || isEmailField().

> Source/WebCore/html/HTMLInputElement.cpp:925
> +bool HTMLInputElement::computeUsernameAndPasswordElements(Element *start,
RefPtr<HTMLInputElement>& usernameElement, RefPtr<HTMLInputElement>&
passwordElement, const Document& document)

Instead of taking two out arguments. We should just create a struct, e.g.
AutofillElements, which contains two Ref: username & password
and then return std::optional<AutofillElements>.

Also why does this code needs to take Document in addition to the starting
element?
That doesn't make much sense since you can get the document out of the element
by start->document().

Finally, start should be Ref<Element>&& instead of Element*.
Also, in C++ code, you must put * immediately after Element without a space as
in: Element* start.
See https://webkit.org/code-style-guidelines/#pointers-and-references

> Source/WebCore/html/HTMLInputElement.cpp:927
> +    // Normal Login Workflow

I don't think this comment gives any context as to why this code exist or why
it's organized the way it is.
This is really a *what* comment, and we don't add those in WebKit.
Please remove it.

> Source/WebCore/html/HTMLInputElement.cpp:931
> +	       // Password element highlighted, now find a username element.

"highlight" has a very specific meaning in autofill / input element context.
Again, this is a *what* comment. Please remove.

> Source/WebCore/html/HTMLInputElement.cpp:942
> +	       // Username element highlighted, now find a password element.

Ditto.

> Source/WebCore/html/HTMLInputElement.cpp:955
> +    // Two-Step Login, Single Password Field on Page

Ditto.

> Source/WebCore/html/HTMLInputElement.h:79
> +    WEBCORE_EXPORT bool acceptsAutofilledLoginCredentials();
> +    WEBCORE_EXPORT void autofillLoginCredentials(String, String);
> +    WEBCORE_EXPORT bool isAssistableElement();
> +    static bool computeUsernameAndPasswordElements(Element *,
RefPtr<HTMLInputElement>&, RefPtr<HTMLInputElement>&, const Document&);

As I said in the person, we shouldn't be adding all these functions to
HTMLInputElement.

> Source/WebCore/page/FocusController.cpp:93
> +    if (is<HTMLSelectElement>(node))
> +	   return true;
> +    if (is<HTMLTextAreaElement>(node))
> +	   return true;

Why specifically select * textarea? Are we looking for any form control
element?
This is exactly the kind of case where a single line comment as to why we
consider these elements as "assistable" is useful.
Again, I don't think we should be using that terminology.

> Source/WebCore/page/FocusController.cpp:666
> +	   nextElement = isForward
> +	   ? nextFocusableElement(*nextElement)
> +	   : previousFocusableElement(*nextElement);

Nit: please put this into a single line.

> Source/WebCore/page/FocusController.h:81
> +    WEBCORE_EXPORT Element* nextAssistableElement(Node*, bool);
> +    WEBCORE_EXPORT bool hasNextAssistableElement(Node*, bool);
> +

This code doesn't really belong to the focus controller. FocusController
controls focus. It's a place to put code that does autofill.