[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

--- Comment #25 from Frederik Riedel <riedel at apple.com> ---
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

>> Source/WebCore/html/HTMLInputElement.cpp:903
>> +
> 
> I don't think we need to add this method to HTMLInputElement.
> Whoever calls this function can just check the return value instead.

So this means that we expose the computeUsernameAndPasswordElements method and call it directly from outside instead of wrapping it into acceptsAutofilledLoginCredentials?

>> Source/WebCore/html/HTMLInputElement.cpp:907
>> +}
> 
> 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().

I implemented this because there was a comment in WebPageIOS about this:
        // FIXME: This laundry list of types is not a good way to factor this. Need a suitable function on HTMLInputElement itself.
Wouldn't the method that I implemented be a suitable function for this?

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

That makes sense. Done.

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

Removed.

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

Removed.

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

Removed.

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

Removed.

>> Source/WebCore/html/HTMLInputElement.h:79
>> +    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.

If we expose the computeUsernameAndPasswordElements with WEBCORE_EXPORT, we can remove acceptsAutofilledLoginCredentials.
Then we also don't need autofillLoginCredentials because we can call it on the HTMLInputElements directly that we get back from the new AutofillElements struct.
Does that sound reasonable to you?
We could also move the static function somewhere else. Any ideas for that?

>> Source/WebCore/page/FocusController.cpp:93
>> +        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.

This is not a specific function for the autofill feature, but supports the nextAssistableElement function that I moved from WebKit2 to WebCore.

>> Source/WebCore/page/FocusController.cpp:666
>> +        : previousFocusableElement(*nextElement);
> 
> Nit: please put this into a single line.

Done.

>> Source/WebCore/page/FocusController.h:81
>> +
> 
> This code doesn't really belong to the focus controller. FocusController controls focus. It's a place to put code that does autofill.

So what place would you suggest for this?
A new `AssistableController`?

-- 
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/54c914ca/attachment.html>