Expose way to set values of classified form controls as {Legacy WebKit, WebKit} SPI

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Frederik Riedel]

Expose way to set values of classified form controls as {Legacy WebKit, WebKit} SPI

[Frederik Riedel]

Created attachment 315179 [details]
Patch

[Build Bot]

Attachment 315179 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKContentView.mm:634:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit/mac/DOM/DOMHTMLInputElement.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 3 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 315179 [details]
Patch

r- for reasons discussed in email

[David Kilzer (:ddkilzer)]

The content of attachment 315179 [details] has been deleted for the following reason:

Accidental upload

[Frederik Riedel]

Created attachment 315354 [details]
Patch

[Build Bot]

Attachment 315354 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKContentView.mm:634:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit/mac/DOM/DOMHTMLInputElement.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 3 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Frederik Riedel]

I am not really sure about some comments of the style checker:

ERROR: Source/WebKit2/UIProcess/ios/WKContentView.mm:634:  This { should be at the end of the previous line  [whitespace/braces] [4]

Don't we put { in new lines in WebKit?
This is probably more about the missing space before (BOOL), right?
-(BOOL)acceptsAutofilledLoginCredentials


ERROR: Source/WebKit/mac/DOM/DOMHTMLInputElement.h:28:  Alphabetical sorting problem.  [build/include_order] [4]

Where do I need to put this import to make it alphabetically correctly? How are @class and import<> and import"" considered?

[Wenson Hsieh]

(In reply to Frederik Riedel from comment #7)
> I am not really sure about some comments of the style checker:
> 
> ERROR: Source/WebKit2/UIProcess/ios/WKContentView.mm:634:  This { should be
> at the end of the previous line  [whitespace/braces] [4]
> 
> Don't we put { in new lines in WebKit?
> This is probably more about the missing space before (BOOL), right?
> -(BOOL)acceptsAutofilledLoginCredentials

That's probably right -- I check-webkit-style is just getting confused here.

> 
> 
> ERROR: Source/WebKit/mac/DOM/DOMHTMLInputElement.h:28:  Alphabetical sorting
> problem.  [build/include_order] [4]
> 
> Where do I need to put this import to make it alphabetically correctly? How
> are @class and import<> and import"" considered?

This needs to be wrapped in a TARGET_OS_IPHONE anyways, I think if you do that the style queue won't complain about the ordering because platform specific headers go afterwards.

[Wenson Hsieh]

Comment on attachment 315354 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=174394

If there's a radar associated with this bug, please also include it below this bugzilla link on its own line, in the form of <rdar://problem/XXXXXXXX> (you can see https://trac.webkit.org/r219594 for an example of what this should look like).

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

It might not be too difficult to write an API test or LayoutTest for this. (e.g. UIPasteboardTests, which exercise some editing codepaths in TestWebKitAPI, or if real keyboard focus is required, then something like LayoutTests/fast/events/ios/input-value-after-oninput.html).

> Source/WebKit2/ChangeLog:34
> +        (WebKit::WebPage::setUsername): Sets the username on the HTMLInputElement.

Would it be more accurate to say something like "Sets the value of an input we've classified as a username field that is associated to the currently focused node."?

"Sets the username on the HTMLInputElement." alone doesn't really describe which HTMLInputElement this method is trying to modify.

> Source/WebCore/html/HTMLInputElement.cpp:904
> +    if (acceptsAutofilledLoginCredentials()) {

We generally prefer early returns first, so this should look like:

if (!acceptsAutofilledLoginCredentials())
    return;

m_passwordElement->setValue(password);
m_passwordElement->setAutoFilled();

> Source/WebCore/html/HTMLInputElement.cpp:912
> +    if (acceptsAutofilledLoginCredentials()) {

Ditto.

> Source/WebCore/html/HTMLInputElement.cpp:918
> +void HTMLInputElement::setPasswordElement(WebCore::HTMLInputElement *passwordElement)

You can drop the WebCore:: namespace prefix here, since we're already using namespace WebCore;

Also, the passwordElement here should be a RefPtr<HTMLInputElement> (WebKit's version of a ref counted pointer) instead of a raw pointer.

> Source/WebCore/html/HTMLInputElement.cpp:923
> +void HTMLInputElement::setUsernameElement(WebCore::HTMLInputElement *usernameElement)

Ditto.

> Source/WebCore/html/HTMLInputElement.h:446
> +    WebCore::HTMLInputElement *m_passwordElement;

If we're going to go with the "remember my associated input element" approach, these should be RefPtr<HTMLInputElement>s. we can also drop the WebCore::s here, since we're using namespace WebCore;

> Source/WebCore/html/HTMLInputElement.h:447
> +    WebCore::HTMLInputElement *m_usernameElement;

Ditto.

> Source/WebCore/page/FocusController.cpp:801
> +            // password field highlighted, now find a usernamefield

WebKit style guidelines (https://webkit.org/code-style-guidelines) recommend that comments should begin with a capital letter and end with a period, so this should be // Password field highlighted, now find a username field.

> Source/WebCore/page/FocusController.cpp:806
> +                    // we've now got a password field and a username field

Ditto regarding comment style, though I don't think this comment adds much here anyways (since the code already implies that we have a password field and username field)

> Source/WebCore/page/FocusController.cpp:807
> +                    inputElement->setPasswordElement(inputElement);

Can we merge setPasswordElement and setUsernameElement into a single method, like setPasswordAndUsernameElements(passwordElement, usernameElement)? It doesn't look like there's ever a case where we want to set only one, but not the other.

Also, when do we clear out an input element's associated password and username fields? It would be good to make sure the web process doesn't crash in cases where two input elements become associated, but then (for instance) JavaScript removes one of them, and then autofill happens. I think changing the raw HTMLInputElement pointers to RefPtrs will help here, but we should probably disassociate the username and password HTMLInputElements from each other if one of them is removed from the DOM tree.

> Source/WebKit/mac/DOM/DOMHTMLInputElement.h:36
> +@interface DOMHTMLInputElement : DOMHTMLElement<UITextInputTraits>

You'll need to guard <UITextInputTraits> conformance with TARGET_OS_IPHONE, but it's probably cleaner to define conformance in a separate category in DOMPrivate.h...

> Source/WebKit/mac/DOM/DOMHTMLInputElementPrivate.h:62
> +

Looks like there's a stray newline here.

> Source/WebKit2/UIProcess/WebPageProxy.h:562
> +    void setUsername(String);

I would make this take a const String&. Even though the only call site needs to convert an NSString * to a String anyways, it's nice to be consistent with the other functions in WebPageProxy. And if we ever call setUsername or setPassword from within WebKit with a String, the compiler can be smarter about it.

> Source/WebKit2/UIProcess/WebPageProxy.h:563
> +    void setPassword(String);

Ditto.

> Source/WebKit2/UIProcess/ios/WKContentView.h:86
> +- (void)setUsername:(NSString *)username;

I think these method names are too generic. It would be nicer to have something like setUsernameForAutofill: or maybe even autofillUsernameFieldIfPossible:, since we only have a setter and not a getter, and this call to autofill is also sort of 'best effort' (it  isn't guaranteed to effect any change).

Since this is an implementation of a UIKit protocol method, we can't change this for WKContentView, but all the names in WebPageProxy and below (in WebCore) can all be named something less generic.

> Source/WebKit2/UIProcess/ios/WKContentView.h:87
> +- (void)setPassword:(NSString *)password;

Ditto.

> Source/WebKit2/UIProcess/ios/WKContentView.mm:633
> +-(BOOL)acceptsAutofilledLoginCredentials

We should add a space between - and ( here.

> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:906
> +    m_acceptsAutofilledLoginCredentials = information.acceptsAutofilledLoginCredentials;

Should this be reset (set to false) in stopAssistingNode? Or is it okay that acceptsAutofilledLoginCredentials() can still return true even after the user loses focus? (even then, it would probably be nicer to ensure that this is reset for bookkeeping purposes :P)

> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:932
> +void WebPageProxy::setUsername(String username)

See earlier comment about renaming setUsername and setPassword for code internal to WebKit.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2832
> +        downcast<HTMLInputElement>(*m_assistedNode).setUsername(username);

Hm...instead of having the username and password fields store pointers to their associated fields, can we just compute the nextFocusableElement or previousFocusableElement on the fly when setting the username or password? I think this would make the code easier to follow (so we don't have to introduce more state to each input element which we have to then set and clean up in the right places), but has the con of an extra focusable element computation when actually setting the username or password. I don't think this is a bad tradeoff though, but it would be nice to get some more opinions about this.

Also, if we recompute the associated input elements here, then we don't have to worry about some corner cases, like the page changing a the type of an input element from a 'password' to something else (like just a 'text') after we've already classified it as a password input.

[Frederik Riedel]

Comment on attachment 315354 [details]
Patch

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

>> Source/WebCore/ChangeLog:4
>> +        https://bugs.webkit.org/show_bug.cgi?id=174394
> 
> If there's a radar associated with this bug, please also include it below this bugzilla link on its own line, in the form of <rdar://problem/XXXXXXXX> (you can see https://trac.webkit.org/r219594 for an example of what this should look like).

That's <rdar://problem/31159986>

>> Source/WebKit2/ChangeLog:34
>> +        (WebKit::WebPage::setUsername): Sets the username on the HTMLInputElement.
> 
> Would it be more accurate to say something like "Sets the value of an input we've classified as a username field that is associated to the currently focused node."?
> 
> "Sets the username on the HTMLInputElement." alone doesn't really describe which HTMLInputElement this method is trying to modify.

Yeah, that's a good point!

>> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2832
>> +        downcast<HTMLInputElement>(*m_assistedNode).setUsername(username);
> 
> Hm...instead of having the username and password fields store pointers to their associated fields, can we just compute the nextFocusableElement or previousFocusableElement on the fly when setting the username or password? I think this would make the code easier to follow (so we don't have to introduce more state to each input element which we have to then set and clean up in the right places), but has the con of an extra focusable element computation when actually setting the username or password. I don't think this is a bad tradeoff though, but it would be nice to get some more opinions about this.
> 
> Also, if we recompute the associated input elements here, then we don't have to worry about some corner cases, like the page changing a the type of an input element from a 'password' to something else (like just a 'text') after we've already classified it as a password input.

How can I compute these fields here? Right now I am using FocusController's method previousFocusableElement(*element); for this. Can I access this from here as well, or is there a different way to do it?

[Wenson Hsieh]

(In reply to Frederik Riedel from comment #10)
> Comment on attachment 315354 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315354&action=review
> 
> >> Source/WebCore/ChangeLog:4
> >> +        https://bugs.webkit.org/show_bug.cgi?id=174394
> > 
> > If there's a radar associated with this bug, please also include it below this bugzilla link on its own line, in the form of <rdar://problem/XXXXXXXX> (you can see https://trac.webkit.org/r219594 for an example of what this should look like).
> 
> That's <rdar://problem/31159986>
> 
> >> Source/WebKit2/ChangeLog:34
> >> +        (WebKit::WebPage::setUsername): Sets the username on the HTMLInputElement.
> > 
> > Would it be more accurate to say something like "Sets the value of an input we've classified as a username field that is associated to the currently focused node."?
> > 
> > "Sets the username on the HTMLInputElement." alone doesn't really describe which HTMLInputElement this method is trying to modify.
> 
> Yeah, that's a good point!
> 
> >> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2832
> >> +        downcast<HTMLInputElement>(*m_assistedNode).setUsername(username);
> > 
> > Hm...instead of having the username and password fields store pointers to their associated fields, can we just compute the nextFocusableElement or previousFocusableElement on the fly when setting the username or password? I think this would make the code easier to follow (so we don't have to introduce more state to each input element which we have to then set and clean up in the right places), but has the con of an extra focusable element computation when actually setting the username or password. I don't think this is a bad tradeoff though, but it would be nice to get some more opinions about this.
> > 
> > Also, if we recompute the associated input elements here, then we don't have to worry about some corner cases, like the page changing a the type of an input element from a 'password' to something else (like just a 'text') after we've already classified it as a password input.
> 
> How can I compute these fields here? Right now I am using FocusController's
> method previousFocusableElement(*element); for this. Can I access this from
> here as well, or is there a different way to do it?

Yes, I was just thinking of computing previousFocusableElement or nextFocusableElement again here. Maybe thorton has a better idea though :P

[Frederik Riedel]

Created attachment 316313 [details]
Patch

[Frederik Riedel]

Created attachment 316993 [details]
Patch

[Wenson Hsieh]

Comment on attachment 316993 [details]
Patch

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

> Source/WebCore/ChangeLog:3022
> [phone]  Frederik Riedel  <[e-mail]>

You should remove the previous ChangeLog entry here.

> Source/WebKit/ChangeLog:1613
> [phone]  Frederik Riedel  <[e-mail]>

Ditto.

> Source/WebKitLegacy/mac/ChangeLog:105
> [phone]  Frederik Riedel  <[e-mail]>

Ditto.

> Source/WebCore/dom/Document.cpp:6897
> +Element* Document::nextFocusableElement(Element& element)

The return type here should be a RefPtr<Element>. TBH, FocusController should probably not return a raw pointer either, but that is something we should address separately (additionally, I don't see why it's an Element& instead of a const Element&, but that's also a separate issue).

> Source/WebCore/dom/Document.cpp:6899
> +    return page()->focusController().nextFocusableElement(element);

Perhaps we should add a null check here too -- it looks like the frame may not necessarily exist, and so page() is not necessarily non-null. Something like:

if (auto* page = this->page())
    return page->focusController().nextFocusableElement(element);
return nullptr;

> Source/WebCore/dom/Document.cpp:6902
> +Element* Document::previousFocusableElement(Element& element)

Ditto with the RefPtr<Element>

> Source/WebCore/dom/Document.cpp:6904
> +    return page()->focusController().previousFocusableElement(element);

Ditto with the null check.

> Source/WebCore/html/HTMLInputElement.cpp:921
> +bool HTMLInputElement::computeUsernameAndPasswordElements(Element *start, HTMLInputElement*& usernameElement, HTMLInputElement*& passwordElement)

Instead of raw pointer references as arguments, I think taking a RefPtr<HTMLInputElement>& or std::optional<HTMLInputElement>& would be better.

> Source/WebCore/html/HTMLInputElement.cpp:924
> +        HTMLInputElement* inputElement = downcast<HTMLInputElement>(start);

In WebKit, we use auto in most places when explicitly specifying the type is redundant (as in this case). Something like:

auto& inputElement = downcast<HTMLInputElement>(*start);

...would be better. (we also know that the input element definitely exists here too, since is<HTMLInputElement>(start) would have been false otherwise).

> Source/WebCore/html/HTMLInputElement.cpp:928
> +            if (previousElement && is<HTMLInputElement>(previousElement)) {

The previousElement null check here isn't needed, since is<HTMLInputElement>(previousElement) will return false anyways if previousElement is null.

> Source/WebCore/html/HTMLInputElement.cpp:939
> +            if (nextElement && is<HTMLInputElement>(nextElement)) {

Ditto with the null check.

> Source/WebCore/html/HTMLInputElement.cpp:949
> +    return false;

Let's also set usernameElement and passwordElement to nullptr here before returning false, to ensure that they are null if username/password element computation failed.

> Source/WebCore/html/HTMLInputElement.h:77
> +    WEBCORE_EXPORT URL representingPageUrl();

Could we instead ask the Document directly for the page url instead of adding a new HTMLInputElement method to do this? It seems a bit odd to specifically teach the HTMLInputElement about the document URL.

> Source/WebKit/Platform/spi/ios/UIKitSPI.h:46
> +#import <UIKit/UIKeyboardLoginCredentialsSuggestion.h>

If this is a new header, this will need to be ifdef'ed out for this to compile on older iOSes.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2764
> +        information.representingPageUrl = element.representingPageUrl();

I think it would be better to ask the Page or Document directly here, instead of going through the HTMLInputElement.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2830
> +void WebPage::autofillLoginCredentials(String username, String password)

Let's change this to take const String&s here.

[Wenson Hsieh]

Comment on attachment 316993 [details]
Patch

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

> Source/WebCore/html/HTMLInputElement.cpp:922
> +{

Also, it looks like the only state on the HTMLInputElement that we use in this method is this->document(), so it's a bit strange to make this a method on HTMLInputElement. Perhaps this should be a static helper function in HTMLInputElement that takes a const Document&, or even a helper method on Document.

[Frederik Riedel]

Comment on attachment 316993 [details]
Patch

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

>> Source/WebCore/html/HTMLInputElement.h:77
>> +    WEBCORE_EXPORT URL representingPageUrl();
> 
> Could we instead ask the Document directly for the page url instead of adding a new HTMLInputElement method to do this? It seems a bit odd to specifically teach the HTMLInputElement about the document URL.

Yeah, that makes sense to access the Document directly. I added this variable here to able to access it from DOMHTMLInputElement. I could not figure out how to access the document/page from there directly.

>> Source/WebKit/Platform/spi/ios/UIKitSPI.h:46
>> +#import <UIKit/UIKeyboardLoginCredentialsSuggestion.h>
> 
> If this is a new header, this will need to be ifdef'ed out for this to compile on older iOSes.

Is this DYLD_IOS_VERSION_11_0?

[Wenson Hsieh]

Comment on attachment 316993 [details]
Patch

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

>>> Source/WebCore/html/HTMLInputElement.h:77
>>> +    WEBCORE_EXPORT URL representingPageUrl();
>> 
>> Could we instead ask the Document directly for the page url instead of adding a new HTMLInputElement method to do this? It seems a bit odd to specifically teach the HTMLInputElement about the document URL.
> 
> Yeah, that makes sense to access the Document directly. I added this variable here to able to access it from DOMHTMLInputElement. I could not figure out how to access the document/page from there directly.

I believe DOMHTMLInputElement has an ownerDocument property, which has an URL property. This calls into Document::urlForBindings()

>>> Source/WebKit/Platform/spi/ios/UIKitSPI.h:46
>>> +#import <UIKit/UIKeyboardLoginCredentialsSuggestion.h>
>> 
>> If this is a new header, this will need to be ifdef'ed out for this to compile on older iOSes.
> 
> Is this DYLD_IOS_VERSION_11_0?

Assuming this is in iOS 11, __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000 is what you want.

[Radar WebKit Bug Importer]

<rdar://problem/33837481>

[Ryosuke Niwa]

Comment on attachment 316993 [details]
Patch

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

r- given Wenson's comments and the fact EWS bots are all red.
I really don't think the code that assumes the username & password text fields are the immediately successive focusable element belongs in WebKit.

> Source/WebCore/html/HTMLInputElement.cpp:919
> +URL HTMLInputElement::representingPageUrl()
> +{
> +    return document().url();
> +}

We should just access input element's document() and then its url() in WebKit layer instead of WebCore.

> Source/WebCore/html/HTMLInputElement.cpp:927
> +            Element* previousElement = document().previousFocusableElement(*start);

I don't think we should assume that the username and password text fields are next to each other after start element.
Safari should just figure out which element is for username & which one is for password and call [inputElement setValue] instead.

[Frederik Riedel]

Comment on attachment 316993 [details]
Patch

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

> Source/WebCore/html/HTMLInputElement.cpp:909
> +        usernameElement->setValue(username);

We should use setValueForUser(username) instead. This simulates user input instead of just setting the value.

> Source/WebCore/html/HTMLInputElement.cpp:911
> +        passwordElement->setValue(password);

Ditto for setValueForUser(password).

>> Source/WebCore/html/HTMLInputElement.cpp:927
>> +            Element* previousElement = document().previousFocusableElement(*start);
> 
> I don't think we should assume that the username and password text fields are next to each other after start element.
> Safari should just figure out which element is for username & which one is for password and call [inputElement setValue] instead.

Isn't the previousFocusableElement/nextFocusableElement the same API that we use for the little blue arrow keys on top of iOS's keyboard? In this case I would expect that it works for most login forms (simulate pressing "tab").

[Frederik Riedel]

Comment on attachment 316993 [details]
Patch

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

>> Source/WebCore/ChangeLog:3022
>> [phone]  Frederik Riedel  <[e-mail]>
> 
> You should remove the previous ChangeLog entry here.

Done.

>> Source/WebKit/ChangeLog:1613
>> [phone]  Frederik Riedel  <[e-mail]>
> 
> Ditto.

Done.

>> Source/WebKitLegacy/mac/ChangeLog:105
>> [phone]  Frederik Riedel  <[e-mail]>
> 
> Ditto.

Done.

>> Source/WebCore/dom/Document.cpp:6897
>> +Element* Document::nextFocusableElement(Element& element)
> 
> The return type here should be a RefPtr<Element>. TBH, FocusController should probably not return a raw pointer either, but that is something we should address separately (additionally, I don't see why it's an Element& instead of a const Element&, but that's also a separate issue).

Done.

>> Source/WebCore/dom/Document.cpp:6899
>> +    return page()->focusController().nextFocusableElement(element);
> 
> Perhaps we should add a null check here too -- it looks like the frame may not necessarily exist, and so page() is not necessarily non-null. Something like:
> 
> if (auto* page = this->page())
>     return page->focusController().nextFocusableElement(element);
> return nullptr;

Done.

>> Source/WebCore/dom/Document.cpp:6902
>> +Element* Document::previousFocusableElement(Element& element)
> 
> Ditto with the RefPtr<Element>

Removed method in latest patch.

>> Source/WebCore/dom/Document.cpp:6904
>> +    return page()->focusController().previousFocusableElement(element);
> 
> Ditto with the null check.

Removed method in latest patch.

>> Source/WebCore/html/HTMLInputElement.cpp:909
>> +        usernameElement->setValue(username);
> 
> We should use setValueForUser(username) instead. This simulates user input instead of just setting the value.

Done.

>> Source/WebCore/html/HTMLInputElement.cpp:911
>> +        passwordElement->setValue(password);
> 
> Ditto for setValueForUser(password).

Done.

>> Source/WebCore/html/HTMLInputElement.cpp:919
>> +}
> 
> We should just access input element's document() and then its url() in WebKit layer instead of WebCore.

This is a good idea. I was able to get the url in WebKitLegacy using self.ownerDocument.URL. Do you know how I can something similar from WKContentView (this is the object I receive as the first responder for WebKit2)?

>> Source/WebCore/html/HTMLInputElement.cpp:921
>> +bool HTMLInputElement::computeUsernameAndPasswordElements(Element *start, HTMLInputElement*& usernameElement, HTMLInputElement*& passwordElement)
> 
> Instead of raw pointer references as arguments, I think taking a RefPtr<HTMLInputElement>& or std::optional<HTMLInputElement>& would be better.

Done.

>> Source/WebCore/html/HTMLInputElement.cpp:922
>> +{
> 
> Also, it looks like the only state on the HTMLInputElement that we use in this method is this->document(), so it's a bit strange to make this a method on HTMLInputElement. Perhaps this should be a static helper function in HTMLInputElement that takes a const Document&, or even a helper method on Document.

Is now static, done.

>> Source/WebCore/html/HTMLInputElement.cpp:924
>> +        HTMLInputElement* inputElement = downcast<HTMLInputElement>(start);
> 
> In WebKit, we use auto in most places when explicitly specifying the type is redundant (as in this case). Something like:
> 
> auto& inputElement = downcast<HTMLInputElement>(*start);
> 
> ...would be better. (we also know that the input element definitely exists here too, since is<HTMLInputElement>(start) would have been false otherwise).

I am now using RefPtr<HTMLInputElement> inputElement = &downcast<HTMLInputElement>(*start);
Can I still use auto for that?

>>> Source/WebCore/html/HTMLInputElement.cpp:927
>>> +            Element* previousElement = document().previousFocusableElement(*start);
>> 
>> I don't think we should assume that the username and password text fields are next to each other after start element.
>> Safari should just figure out which element is for username & which one is for password and call [inputElement setValue] instead.
> 
> Isn't the previousFocusableElement/nextFocusableElement the same API that we use for the little blue arrow keys on top of iOS's keyboard? In this case I would expect that it works for most login forms (simulate pressing "tab").

I am now using nextAssistableElement which is indeed the method that simulates the tab press that we use for the blue up/down arrows in iOS.

>> Source/WebCore/html/HTMLInputElement.cpp:928
>> +            if (previousElement && is<HTMLInputElement>(previousElement)) {
> 
> The previousElement null check here isn't needed, since is<HTMLInputElement>(previousElement) will return false anyways if previousElement is null.

Done.

>> Source/WebCore/html/HTMLInputElement.cpp:939
>> +            if (nextElement && is<HTMLInputElement>(nextElement)) {
> 
> Ditto with the null check.

Done.

>> Source/WebCore/html/HTMLInputElement.cpp:949
>> +    return false;
> 
> Let's also set usernameElement and passwordElement to nullptr here before returning false, to ensure that they are null if username/password element computation failed.

Done.

>>>> Source/WebKit/Platform/spi/ios/UIKitSPI.h:46
>>>> +#import <UIKit/UIKeyboardLoginCredentialsSuggestion.h>
>>> 
>>> If this is a new header, this will need to be ifdef'ed out for this to compile on older iOSes.
>> 
>> Is this DYLD_IOS_VERSION_11_0?
> 
> Assuming this is in iOS 11, __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000 is what you want.

I assume that #if __has_include(<UIKit/UIKeyboardLoginCredentialsSuggestion.h>) is even better for that?

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2764
>> +        information.representingPageUrl = element.representingPageUrl();
> 
> I think it would be better to ask the Page or Document directly here, instead of going through the HTMLInputElement.

Do you know how I can get the URL at this point from the m_page? I couldn't figure that out in the current patch.
This would also fix Ryosuke's comment.

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2830
>> +void WebPage::autofillLoginCredentials(String username, String password)
> 
> Let's change this to take const String&s here.

Done.

[Frederik Riedel]

Created attachment 318452 [details]
Patch

[Build Bot]

Attachment 318452 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLInputElement.cpp:899:  'usernameElement' is incorrectly named. It should be named 'protector' or 'protectedNullptr'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/html/HTMLInputElement.cpp:900:  'passwordElement' is incorrectly named. It should be named 'protector' or 'protectedNullptr'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/html/HTMLInputElement.cpp:911:  'usernameElement' is incorrectly named. It should be named 'protector' or 'protectedNullptr'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/html/HTMLInputElement.cpp:912:  'passwordElement' is incorrectly named. It should be named 'protector' or 'protectedNullptr'.  [readability/naming/protected] [4]
Total errors found: 4 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 318452 [details]
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.

[Frederik Riedel]

Comment on attachment 318452 [details]
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`?

[Ryosuke Niwa]

(In reply to Frederik Riedel from comment #25)
> Comment on attachment 318452 [details]
> 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?

Right.

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

I don't think hard coding the list of input types is what this FIXME is asking. I actually don't think we need to move this code to WebCore given the entire concept of whether an input element is assitable or not is specific to iOS.

Regardless, it doesn't make sense for your code to checking this since input with type=datetime is most certainly not a text field used for a username or a password.

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

Yes.

> We could also move the static function somewhere else. Any ideas for that?

We can move it to its own file. Maybe editing/EditorIOS.mm is be a good place too?

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

I don't think we should do that. The entire concept of assisted node is specific to iOS. It has not business being inside FocusController. Again, the focus controller should be concerned about controlling the focused node, and nothing else.

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

Why do we need a class for this? It seems like we just need a function which iterates over focused elements. A free standing function in EditorIOS.mm would suffice.

[Frederik Riedel]

Comment on attachment 318452 [details]
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?
> 
> Right.

Alright, done.

>>>> 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?
> 
> I don't think hard coding the list of input types is what this FIXME is asking. I actually don't think we need to move this code to WebCore given the entire concept of whether an input element is assitable or not is specific to iOS.
> 
> Regardless, it doesn't make sense for your code to checking this since input with type=datetime is most certainly not a text field used for a username or a password.

Ok, that makes sense to me. Done.

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

Done, implemented in EditorIOS.

>>>> 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.
> 
> I don't think we should do that. The entire concept of assisted node is specific to iOS. It has not business being inside FocusController. Again, the focus controller should be concerned about controlling the focused node, and nothing else.

Ok, 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`?
> 
> Why do we need a class for this? It seems like we just need a function which iterates over focused elements. A free standing function in EditorIOS.mm would suffice.

EditorIOS contains now the iOS-specific code for this feature. Done.

[Frederik Riedel]

Created attachment 319277 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 319277 [details]
Patch

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

Okay, this code is a lot better. You might still want to refactor it as a separate class. It's fine if you didn't but please address other review comments.

> Source/WebCore/editing/Editor.h:281
> +#if PLATFORM(IOS)
> +    WEBCORE_EXPORT static bool isAutofillable(RefPtr<Element>);
> +    WEBCORE_EXPORT static std::optional<AutofillElements> computeAutofillElements(RefPtr<Element>);
> +    WEBCORE_EXPORT static void autofillElements(String, String, AutofillElements);
> +#endif

Hm... now that I'm looking at this, it might be better to have a class named AutofillElements and make these functions members of that class.

> Source/WebCore/editing/ios/EditorIOS.mm:490
> +static inline RefPtr<Element> nextAutofillableElement(Node* startNode, Page& page, bool isForward)

I think it's better to have two functions one called nextAutofillableElement and previousAutofillableElement instead of taking isForward argument given how small the actual code is.

> Source/WebCore/editing/ios/EditorIOS.mm:495
> +    Element* nextElement = downcast<Element>(startNode);

Use RefPtr instead.

> Source/WebCore/editing/ios/EditorIOS.mm:539
> +                autofillElements.password = &inputElement;

We don't have username in this case?

> Source/WebCore/editing/ios/EditorIOS.mm:544
> +    return autofillElements;

We should return nullopt here instead.

> Source/WebCore/editing/ios/EditorIOS.mm:551
> +bool Editor::isAutofillable(RefPtr<Element> start)
> +{
> +    std::optional<AutofillElements> autofillElements = Editor::computeAutofillElements(start);
> +    return (autofillElements->username && autofillElements->password) || autofillElements->password;
> +}

This function is not needed once computeAutofillElements returns nullopt in the case neither username nor password is set.
The caller of this function can just check !Editor::computeAutofillElements(start) then.

> Source/WebCore/html/HTMLInputElement.cpp:900
> +bool HTMLInputElement::isAutofillableElement()
> +{
> +    return isTextField() || isEmailField();
> +}
> +

We're not using this function. Please remove.

[Frederik Riedel]

Comment on attachment 319277 [details]
Patch

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

>> Source/WebCore/editing/Editor.h:281
>> +#endif
> 
> Hm... now that I'm looking at this, it might be better to have a class named AutofillElements and make these functions members of that class.

Done.

>> Source/WebCore/editing/ios/EditorIOS.mm:490
>> +static inline RefPtr<Element> nextAutofillableElement(Node* startNode, Page& page, bool isForward)
> 
> I think it's better to have two functions one called nextAutofillableElement and previousAutofillableElement instead of taking isForward argument given how small the actual code is.

Done.

>> Source/WebCore/editing/ios/EditorIOS.mm:495
>> +    Element* nextElement = downcast<Element>(startNode);
> 
> Use RefPtr instead.

Done.

>> Source/WebCore/editing/ios/EditorIOS.mm:539
>> +                autofillElements.password = &inputElement;
> 
> We don't have username in this case?

Yes. This is the case where a standalone password field should be filled.

>> Source/WebCore/editing/ios/EditorIOS.mm:544
>> +    return autofillElements;
> 
> We should return nullopt here instead.

OK, done.

>> Source/WebCore/editing/ios/EditorIOS.mm:551
>> +}
> 
> This function is not needed once computeAutofillElements returns nullopt in the case neither username nor password is set.
> The caller of this function can just check !Editor::computeAutofillElements(start) then.

Makes sense, removed.

>> Source/WebCore/html/HTMLInputElement.cpp:900
>> +
> 
> We're not using this function. Please remove.

Removed.

[Frederik Riedel]

Created attachment 320266 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 320266 [details]
Patch

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

New patch looks really good! Just some stylistic issues. Please rebase it agains trunk.

> Source/WebCore/editing/ios/AutofillElements.cpp:34
> +static inline bool isAutofillableElement(Element* node)

This function should take Element& instead of Element*.

> Source/WebCore/editing/ios/AutofillElements.cpp:36
> +    if (is<HTMLInputElement>(node)) {

Nit: We refer an early return to nested structure.
so:
if (~)
  return false;
~
return true;

> Source/WebCore/editing/ios/AutofillElements.cpp:37
> +        RefPtr<HTMLInputElement> inputElement = &downcast<HTMLInputElement>(*node);

Use `auto inputElement`. There's no need to ref-churn here.

> Source/WebCore/editing/ios/AutofillElements.cpp:43
> +static inline RefPtr<Element> nextAutofillableElement(Node* startNode, Page& page)

This function should probably return RefPtr<HTMLInputElement>.

> Source/WebCore/editing/ios/AutofillElements.cpp:72
> +    if (is<HTMLInputElement>(start.get())) {

Please explicitly check whether start is null or not as in:
if (start && is<HTMLInputElement>(*start))

> Source/WebCore/editing/ios/AutofillElements.cpp:75
> +            RefPtr<Element> previousElement = previousAutofillableElement(&inputElement, *start->document().page());

We should check that page isn't null, and exit early when it is.
In fact, just store FocusController& as a local variable and use pass that to these helper functions instead.

> Source/WebCore/editing/ios/AutofillElements.cpp:76
> +            if (is<HTMLInputElement>(previousElement.get())) {

Ditto.

> Source/WebCore/editing/ios/AutofillElements.cpp:77
> +                RefPtr<HTMLInputElement> potentialUsernameField = &downcast<HTMLInputElement>(*previousElement);

There's no need to use RefPtr here since we know it can't be null.
Remove & before downcast and use Ref<HTMLInputElement>
But really, you should just use `auto` instead.

> Source/WebCore/editing/ios/AutofillElements.cpp:87
> +            RefPtr<Element> nextElement = nextAutofillableElement(&inputElement, *start->document().page());
> +            if (is<HTMLInputElement>(nextElement.get())) {
> +                RefPtr<HTMLInputElement> potentialPasswordField = &downcast<HTMLInputElement>(*nextElement);

Same issues explicitly checking the nullity and using `auto` (which makes RefPtr in line 87 for example) instead of RefPtr.

> Source/WebCore/editing/ios/AutofillElements.h:26
> +#include "Element.h"

There's no need to include Element since HTMLInputElement includes Element.

> Source/WebCore/editing/ios/AutofillElements.h:31
> +class AutofillElements {

We should probably use WTF_MAKE_FAST_ALLOCATED here.

> Source/WebCore/editing/ios/AutofillElements.h:36
> +    RefPtr<HTMLInputElement> username;
> +    RefPtr<HTMLInputElement> password;

It looks like these two member variables are never accessed outside AutofillElements class.
Make them private and prefix them with m_.
If not, please add an explicit getter & setter instead of exposing them as public member variables.

[Alex Christensen]

Comment on attachment 320266 [details]
Patch

See https://bugs.webkit.org/show_bug.cgi?id=176710 and https://bugs.webkit.org/show_bug.cgi?id=176139 for examples of tests that check input elements and autofill things.

[Frederik Riedel]

Comment on attachment 320266 [details]
Patch

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

>> Source/WebCore/editing/ios/AutofillElements.cpp:34
>> +static inline bool isAutofillableElement(Element* node)
> 
> This function should take Element& instead of Element*.

Done.

>> Source/WebCore/editing/ios/AutofillElements.cpp:36
>> +    if (is<HTMLInputElement>(node)) {
> 
> Nit: We refer an early return to nested structure.
> so:
> if (~)
>   return false;
> ~
> return true;

Done.

>> Source/WebCore/editing/ios/AutofillElements.cpp:37
>> +        RefPtr<HTMLInputElement> inputElement = &downcast<HTMLInputElement>(*node);
> 
> Use `auto inputElement`. There's no need to ref-churn here.

Done.

>> Source/WebCore/editing/ios/AutofillElements.cpp:43
>> +static inline RefPtr<Element> nextAutofillableElement(Node* startNode, Page& page)
> 
> This function should probably return RefPtr<HTMLInputElement>.

Done. Also for previousAutofillableElement.

>> Source/WebCore/editing/ios/AutofillElements.cpp:72
>> +    if (is<HTMLInputElement>(start.get())) {
> 
> Please explicitly check whether start is null or not as in:
> if (start && is<HTMLInputElement>(*start))

Done.

>> Source/WebCore/editing/ios/AutofillElements.cpp:75
>> +            RefPtr<Element> previousElement = previousAutofillableElement(&inputElement, *start->document().page());
> 
> We should check that page isn't null, and exit early when it is.
> In fact, just store FocusController& as a local variable and use pass that to these helper functions instead.

Great idea! We don't use the page variable anyway, only its focusController(). Passing it directly now. Done.

>> Source/WebCore/editing/ios/AutofillElements.cpp:76
>> +            if (is<HTMLInputElement>(previousElement.get())) {
> 
> Ditto.

Done.

>> Source/WebCore/editing/ios/AutofillElements.cpp:77
>> +                RefPtr<HTMLInputElement> potentialUsernameField = &downcast<HTMLInputElement>(*previousElement);
> 
> There's no need to use RefPtr here since we know it can't be null.
> Remove & before downcast and use Ref<HTMLInputElement>
> But really, you should just use `auto` instead.

Done.

>> Source/WebCore/editing/ios/AutofillElements.cpp:87
>> +                RefPtr<HTMLInputElement> potentialPasswordField = &downcast<HTMLInputElement>(*nextElement);
> 
> Same issues explicitly checking the nullity and using `auto` (which makes RefPtr in line 87 for example) instead of RefPtr.

Done.

>> Source/WebCore/editing/ios/AutofillElements.h:26
>> +#include "Element.h"
> 
> There's no need to include Element since HTMLInputElement includes Element.

True, removed.

>> Source/WebCore/editing/ios/AutofillElements.h:31
>> +class AutofillElements {
> 
> We should probably use WTF_MAKE_FAST_ALLOCATED here.

Ok. Added.

>> Source/WebCore/editing/ios/AutofillElements.h:36
>> +    RefPtr<HTMLInputElement> password;
> 
> It looks like these two member variables are never accessed outside AutofillElements class.
> Make them private and prefix them with m_.
> If not, please add an explicit getter & setter instead of exposing them as public member variables.

They can be private indeed. Added m_ as well. Done.

[Ryosuke Niwa]

Looks like the patch no longer applies on trunk? Could you re-upload it after rebating against ToT?

[Frederik Riedel]

(In reply to Ryosuke Niwa from comment #35)
> Looks like the patch no longer applies on trunk? Could you re-upload it
> after rebating against ToT?

Yes, rebased on master.

[Frederik Riedel]

Created attachment 321243 [details]
Patch

[Ryosuke Niwa]

Looks like it still doesn't apply?

[Frederik Riedel]

Created attachment 321328 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 321328 [details]
Patch

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

> Source/WebKitLegacy/mac/DOM/DOMHTMLInputElement.h:39
> +#elif

elif what? This is causing the build failure.

[Ryosuke Niwa]

Comment on attachment 321328 [details]
Patch

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

> Source/WebKitLegacy/mac/DOM/DOMHTMLInputElement.mm:46
>  #import "ExceptionHandlers.h"
> +#if __has_include(<UIKit/UIKeyboardLoginCredentialsSuggestion.h>)
> +#import <UIKit/UIKeyboardLoginCredentialsSuggestion.h>
> +#else
> +#import <UIKit/UITextInput_Private.h>
> +@interface UIKeyboardLoginCredentialsSuggestion : UITextSuggestion
> +
> +@property (nonatomic, assign) NSString *username;
> +@property (nonatomic, assign) NSString *password;
> +
> +@end
> +#endif

You need to wrap this whole thing in #if PLATFORM(IOS) or #if TARGET_OS_IPHONE

[Frederik Riedel]

Comment on attachment 321328 [details]
Patch

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

>> Source/WebKitLegacy/mac/DOM/DOMHTMLInputElement.h:39
>> +#elif
> 
> elif what? This is causing the build failure.

Yes, this should be #else.

>> Source/WebKitLegacy/mac/DOM/DOMHTMLInputElement.mm:46
>> +#endif
> 
> You need to wrap this whole thing in #if PLATFORM(IOS) or #if TARGET_OS_IPHONE

Right. Even if the UIKit include is not available we would try to #import it in #else. This breaks the build on Mac. 
I wrapped the whole thing in an additional check for #if TARGET_OS_IPHONE.

[Frederik Riedel]

Created attachment 321513 [details]
Patch

[Ryosuke Niwa]

It looks like Source/WebCore/WebCore.xcodeproj/project.pbxproj and Source/WebCore/WebCore.xcodeproj/project.pbxproj are conflicting again?

In general, adding a new file to WebKit typically requires you to be always syncing to tip of source tree every day. It looks like your checkout was more than a day old when rebased so it's likely too old.

[Frederik Riedel]

Yep, just noticed. Rebasing now and then going to upload the new patch.

(In reply to Ryosuke Niwa from comment #44)
> It looks like Source/WebCore/WebCore.xcodeproj/project.pbxproj and
> Source/WebCore/WebCore.xcodeproj/project.pbxproj are conflicting again?
> 
> In general, adding a new file to WebKit typically requires you to be always
> syncing to tip of source tree every day. It looks like your checkout was
> more than a day old when rebased so it's likely too old.

[Frederik Riedel]

Created attachment 321517 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 321517 [details]
Patch

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

> Source/WebCore/editing/ios/AutofillElements.cpp:94
> +                    autofillElements.m_password = &inputElement;
> +                    autofillElements.m_username = previousElement;
> +                    return autofillElements;

Can we just add a private constructor which takes username & password elements,
and do AutofillElements(previousElement, &inputElement) here?

> Source/WebCore/editing/ios/AutofillElements.cpp:108
> +                    autofillElements.m_password = nextElement;
> +                    autofillElements.m_username = &inputElement;
> +                    return autofillElements;

Ditto.

> Source/WebCore/editing/ios/AutofillElements.cpp:122
> +            if (!previousElement && !nextElement) {
> +                autofillElements.m_password = &inputElement;
> +                return autofillElements;
> +            }

Ditto.

> Source/WebCore/editing/ios/AutofillElements.cpp:137
> +    if (this->m_username) {
> +        this->m_username->setValueForUser(username);
> +        this->m_username->setAutoFilled();
> +    }
> +    if (this->m_password) {
> +        this->m_password->setValueForUser(password);
> +        this->m_password->setAutoFilled();
> +    }

Nit: We don't use "this->" in WebKit. Please remove.

> Source/WebKit/Shared/AssistedNodeInformation.h:113
> +    bool acceptsAutofilledLoginCredentials;

Please add an initializer: { false }. r- because of this and iOS build failures.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2814
> +    if (is<HTMLInputElement>(m_assistedNode.get())) {

Use is<HTMLInputElement>(*m_assistedNode) instead.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2815
> +        RefPtr<Element> assistendElement = &downcast<Element>(*m_assistedNode);

Use Ref<Element> instead since we know m_assistedNode is not null at this point.

[Frederik Riedel]

Comment on attachment 321517 [details]
Patch

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

>> Source/WebCore/editing/ios/AutofillElements.cpp:94
>> +                    return autofillElements;
> 
> Can we just add a private constructor which takes username & password elements,
> and do AutofillElements(previousElement, &inputElement) here?

Yes, good idea. Added.

>> Source/WebCore/editing/ios/AutofillElements.cpp:108
>> +                    return autofillElements;
> 
> Ditto.

Added new constructor call.

>> Source/WebCore/editing/ios/AutofillElements.cpp:122
>> +            }
> 
> Ditto.

Added new constructor call.

>> Source/WebCore/editing/ios/AutofillElements.cpp:137
>> +    }
> 
> Nit: We don't use "this->" in WebKit. Please remove.

Removed "this->".

>> Source/WebKit/Shared/AssistedNodeInformation.h:113
>> +    bool acceptsAutofilledLoginCredentials;
> 
> Please add an initializer: { false }. r- because of this and iOS build failures.

Added initializer.

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2814
>> +    if (is<HTMLInputElement>(m_assistedNode.get())) {
> 
> Use is<HTMLInputElement>(*m_assistedNode) instead.

Done.

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2815
>> +        RefPtr<Element> assistendElement = &downcast<Element>(*m_assistedNode);
> 
> Use Ref<Element> instead since we know m_assistedNode is not null at this point.

The method computeAutofillElements is expecting a RefPtr. Would this work if we use a Ref here instead?

[Wenson Hsieh]

Comment on attachment 321517 [details]
Patch

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

>>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2815
>>> +        RefPtr<Element> assistendElement = &downcast<Element>(*m_assistedNode);
>> 
>> Use Ref<Element> instead since we know m_assistedNode is not null at this point.
> 
> The method computeAutofillElements is expecting a RefPtr. Would this work if we use a Ref here instead?

It looks like computeAutofillElements assumes that the start node must exist, since the first thing it does is dereference start. That should probably take a Ref<> or const & instead.

[Wenson Hsieh]

Comment on attachment 321517 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/ios/WKWebViewAutofillTests.mm:49
> +static UIKeyboardLoginCredentialsSuggestion *createUIKeyboardLoginCredentialsSuggestion = [NSClassFromString(@"UIKeyboardLoginCredentialsSuggestion") new];

I think you meant for this to be a static function that returns a new credentials suggestion?

> Tools/TestWebKitAPI/Tests/ios/WKWebViewAutofillTests.mm:66
> +    TestWKWebView *webView = [[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)];

This leaks webView. Do probably want to make this something like:

auto webView = adoptNS(~)

...or alternately, just make sure you -release the webView at the end.

> Tools/TestWebKitAPI/Tests/ios/WKWebViewAutofillTests.mm:87
> +    TestWKWebView *webView = [[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)];

Ditto.

> Tools/TestWebKitAPI/Tests/ios/WKWebViewAutofillTests.mm:99
> +    TestWKWebView *webView = [[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)];

Ditto.

> Tools/TestWebKitAPI/Tests/ios/WKWebViewAutofillTests.mm:125
> +    TestWKWebView *webView = [[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)];

Ditto.

[Frederik Riedel]

Comment on attachment 321517 [details]
Patch

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

>>>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2815
>>>> +        RefPtr<Element> assistendElement = &downcast<Element>(*m_assistedNode);
>>> 
>>> Use Ref<Element> instead since we know m_assistedNode is not null at this point.
>> 
>> The method computeAutofillElements is expecting a RefPtr. Would this work if we use a Ref here instead?
> 
> It looks like computeAutofillElements assumes that the start node must exist, since the first thing it does is dereference start. That should probably take a Ref<> or const & instead.

Using a Ref here now.

>> Tools/TestWebKitAPI/Tests/ios/WKWebViewAutofillTests.mm:49
>> +static UIKeyboardLoginCredentialsSuggestion *createUIKeyboardLoginCredentialsSuggestion = [NSClassFromString(@"UIKeyboardLoginCredentialsSuggestion") new];
> 
> I think you meant for this to be a static function that returns a new credentials suggestion?

Yep, I updated that to be a static method which returns a new UIKeyboardLoginCredentialsSuggestion object.

>> Tools/TestWebKitAPI/Tests/ios/WKWebViewAutofillTests.mm:66
>> +    TestWKWebView *webView = [[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)];
> 
> This leaks webView. Do probably want to make this something like:
> 
> auto webView = adoptNS(~)
> 
> ...or alternately, just make sure you -release the webView at the end.

Added [webView release];

>> Tools/TestWebKitAPI/Tests/ios/WKWebViewAutofillTests.mm:87
>> +    TestWKWebView *webView = [[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)];
> 
> Ditto.

Done.

>> Tools/TestWebKitAPI/Tests/ios/WKWebViewAutofillTests.mm:99
>> +    TestWKWebView *webView = [[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)];
> 
> Ditto.

Done.

>> Tools/TestWebKitAPI/Tests/ios/WKWebViewAutofillTests.mm:125
>> +    TestWKWebView *webView = [[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)];
> 
> Ditto.

Done.

[Frederik Riedel]

Created attachment 321595 [details]
Patch

[Frederik Riedel]

Comment on attachment 321595 [details]
Patch

WebKit is compiling without the internal SDK, putting the related code behind USE_APPLE_INTERNAL_SDK.

[Frederik Riedel]

Created attachment 321600 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 321600 [details]
Patch

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

> Source/WebCore/editing/ios/AutofillElements.h:36
> +    AutofillElements(RefPtr<HTMLInputElement>, RefPtr<HTMLInputElement>);

Use RefPtr<HTMLInputElement>&& instead to avoid ref churn.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2816
> +        Ref<HTMLInputElement> assistendElement = downcast<HTMLInputElement>(*m_assistedNode);
> +        auto autofillElements = AutofillElements::computeAutofillElements(assistendElement.get());

It looks like we don't need this local variable?
Why not just do this?:
AutofillElements::computeAutofillElements(downcast<HTMLInputElement>(*m_assistedNode));

> Tools/TestWebKitAPI/Tests/ios/WKWebViewAutofillTests.mm:37
> +// FIXME 34583628: Simplyfy this once the UIKit work is available in the build.

Nit: "FIXME <radar:34583628>: ~"

[Ryosuke Niwa]

In order to commit this change, either fix the problems I've pointed out and say that you want to land as is, and run "./Tools/Scripts/webkit-patch land-safely" (or open "Details" and set "cq?").

Note: Don't use "webkit-patch upload" because that'd clear my r+ bit. You'd need to ask for another review if you've done that.

[Frederik Riedel]

Comment on attachment 321600 [details]
Patch

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

>> Source/WebCore/editing/ios/AutofillElements.h:36
>> +    AutofillElements(RefPtr<HTMLInputElement>, RefPtr<HTMLInputElement>);
> 
> Use RefPtr<HTMLInputElement>&& instead to avoid ref churn.

What is 'ref churn'?

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2816
>> +        auto autofillElements = AutofillElements::computeAutofillElements(assistendElement.get());
> 
> It looks like we don't need this local variable?
> Why not just do this?:
> AutofillElements::computeAutofillElements(downcast<HTMLInputElement>(*m_assistedNode));

Done.

>> Tools/TestWebKitAPI/Tests/ios/WKWebViewAutofillTests.mm:37
>> +// FIXME 34583628: Simplyfy this once the UIKit work is available in the build.
> 
> Nit: "FIXME <radar:34583628>: ~"

Done.



Thanks for reviewing, Ryosuke!

[Wenson Hsieh]

Comment on attachment 321600 [details]
Patch

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

>>> Source/WebCore/editing/ios/AutofillElements.h:36
>>> +    AutofillElements(RefPtr<HTMLInputElement>, RefPtr<HTMLInputElement>);
>> 
>> Use RefPtr<HTMLInputElement>&& instead to avoid ref churn.
> 
> What is 'ref churn'?

This is when objects that keep track of reference counts (e.g. RefPtr, Ref, RetainPtr) are unnecessarily destroyed and recreated. In this case, we can turn this into RefPtr<HTMLInputElement>&&s and WTFMove() at the call site to avoid this extra dance.

[Frederik Riedel]

Created attachment 321709 [details]
Patch for landing

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #58)
> Comment on attachment 321600 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=321600&action=review
> 
> >>> Source/WebCore/editing/ios/AutofillElements.h:36
> >>> +    AutofillElements(RefPtr<HTMLInputElement>, RefPtr<HTMLInputElement>);
> >> 
> >> Use RefPtr<HTMLInputElement>&& instead to avoid ref churn.
> > 
> > What is 'ref churn'?
> 
> This is when objects that keep track of reference counts (e.g. RefPtr, Ref,
> RetainPtr) are unnecessarily destroyed and recreated. In this case, we can
> turn this into RefPtr<HTMLInputElement>&&s and WTFMove() at the call site to
> avoid this extra dance.

Well, more precisely about calling ref() and deref() multiple times. This is an expensive operation because they're dependent operations in CPU, and must be executed consecutively. In a CPU which doesn't do an out-of-order execution, it can block the code execution or, worse, cause a pipeline hazard.

[Ryosuke Niwa]

Comment on attachment 321709 [details]
Patch for landing

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2816
> +        auto autofillElements = AutofillElements::computeAutofillElements(downcast<HTMLInputElement>(*m_assistedNode));
> +        autofillElements->autofill(username, password);

Oh wait, you're missing the check to null opt. It's possible that computeAutofillElements don't find an element, right?

[Ryosuke Niwa]

Comment on attachment 321709 [details]
Patch for landing

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

> Source/WebKitLegacy/mac/DOM/DOMHTMLInputElement.mm:704
> +        std::optional<WebCore::AutofillElements> autofillElements = WebCore::AutofillElements::computeAutofillElements(*IMPL);
> +        autofillElements->autofill(credentialsSuggestion.username, credentialsSuggestion.password);

use auto here as in auto autofillElements.
You should check for nullopt. Otherwise, the program will crash.

[Wenson Hsieh]

(In reply to Ryosuke Niwa from comment #62)
> Comment on attachment 321709 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=321709&action=review
> 
> > Source/WebKitLegacy/mac/DOM/DOMHTMLInputElement.mm:704
> > +        std::optional<WebCore::AutofillElements> autofillElements = WebCore::AutofillElements::computeAutofillElements(*IMPL);
> > +        autofillElements->autofill(credentialsSuggestion.username, credentialsSuggestion.password);
> 
> use auto here as in auto autofillElements.
> You should check for nullopt. Otherwise, the program will crash.

A clean way to write this would be:

    if (auto elements = WebCore::AutofillElements::computeAutofillElements(*IMPL))
        elements->autofill(credentialsSuggestion.username, credentialsSuggestion.password);

[Frederik Riedel]

Comment on attachment 321709 [details]
Patch for landing

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

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2816
>> +        autofillElements->autofill(username, password);
> 
> Oh wait, you're missing the check to null opt. It's possible that computeAutofillElements don't find an element, right?

Done.

>>> Source/WebKitLegacy/mac/DOM/DOMHTMLInputElement.mm:704
>>> +        autofillElements->autofill(credentialsSuggestion.username, credentialsSuggestion.password);
>> 
>> use auto here as in auto autofillElements.
>> You should check for nullopt. Otherwise, the program will crash.
> 
> A clean way to write this would be:
> 
>     if (auto elements = WebCore::AutofillElements::computeAutofillElements(*IMPL))
>         elements->autofill(credentialsSuggestion.username, credentialsSuggestion.password);

Done.

[Frederik Riedel]

Created attachment 321762 [details]
Patch for landing

[Ryosuke Niwa]

Let's wait for EWS results.

[Build Bot]

Attachment 321762 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2816:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 321762 [details]
Patch for landing

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2815
> +        if(auto autofillElements = AutofillElements::computeAutofillElements(downcast<HTMLInputElement>(*m_assistedNode)))

You need a space between if and (.

[Frederik Riedel]

Created attachment 321779 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 321779 [details]
Patch for landing

Clearing flags on attachment: 321779

Committed r222487: <http://trac.webkit.org/changeset/222487>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

It seems like after this patch, AutofillElements is no longer iOS-specific --- should we move it out of the iOS subdirectory?

[Ryosuke Niwa]

(In reply to Tim Horton from comment #72)
> It seems like after this patch, AutofillElements is no longer iOS-specific
> --- should we move it out of the iOS subdirectory?

Oh oops, I didn't see that. We should discuss that offline somewhere.