[Webkit-unassigned] [Bug 174394] Expose way to set values of classified form controls as {Legacy WebKit, WebKit} SPI

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=174394

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #318452|review?                     |review-
              Flags|                            |

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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170818/807ba8b5/attachment-0001.html>