Bug 221429 – Allow Password AutoFill in more text field configurations

• Component: Forms
• Status: RESOLVED FIXED

[Ricky Mondello]

Allow Password AutoFill in more text field configurations

[Ricky Mondello]

Created attachment 419321 [details]
First attempt at a patch

[Radar WebKit Bug Importer]

<rdar://problem/73997299>

[Wenson Hsieh]

Comment on attachment 419321 [details]
First attempt at a patch

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

Looks good! Just a couple of minor comments. We should also write an API test for this (similar to the existing ones in WKWebViewAutoFillTests).

> Source/WebCore/editing/cocoa/AutofillElements.cpp:99
> +        return AutofillElements(
> +            previousFieldIsTextField ? WTFMove(previousElement) : nullptr,
> +            WTFMove(start),
> +            hasSecondPasswordFieldToFill ? WTFMove(nextElement) : nullptr
> +        );

We could probably put this on one line, as `return {{ previousFieldIsTextField ? WTFMove(previousElement) : nullptr, WTFMove(start), hasSecondPasswordFieldToFill ? WTFMove(nextElement) : nullptr }};`

> Source/WebCore/editing/cocoa/AutofillElements.cpp:104
> +                RefPtr<HTMLInputElement> elementAfterNextElement = nextAutofillableElement(nextElement.get(), focusController);

Nit - `auto elementAfterNextElement = nextAutofillableElement(nextElement.get(), focusController);`

> Source/WebCore/editing/cocoa/AutofillElements.cpp:108
> +                return AutofillElements(WTFMove(start), WTFMove(nextElement),
> +                    hasSecondPasswordFieldToFill ? WTFMove(elementAfterNextElement) : nullptr);

Nit - I think this should could just be a single line as `return {{ WTFMove(start), WTFMove(nextElement), hasSecondPasswordFieldToFill ? WTFMove(elementAfterNextElement) : nullptr }};`

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #3)
> Comment on attachment 419321 [details]
> First attempt at a patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419321&action=review
> 
> Looks good! Just a couple of minor comments. We should also write an API
> test for this (similar to the existing ones in WKWebViewAutoFillTests).

Actually, it looks like the existing test WKWebViewAutoFillTests.AccountCreationPage already exercises this scenario, and just needs to be rebaselined.

> 
> > Source/WebCore/editing/cocoa/AutofillElements.cpp:99
> > +        return AutofillElements(
> > +            previousFieldIsTextField ? WTFMove(previousElement) : nullptr,
> > +            WTFMove(start),
> > +            hasSecondPasswordFieldToFill ? WTFMove(nextElement) : nullptr
> > +        );
> 
> We could probably put this on one line, as `return {{
> previousFieldIsTextField ? WTFMove(previousElement) : nullptr,
> WTFMove(start), hasSecondPasswordFieldToFill ? WTFMove(nextElement) :
> nullptr }};`
> 
> > Source/WebCore/editing/cocoa/AutofillElements.cpp:104
> > +                RefPtr<HTMLInputElement> elementAfterNextElement = nextAutofillableElement(nextElement.get(), focusController);
> 
> Nit - `auto elementAfterNextElement =
> nextAutofillableElement(nextElement.get(), focusController);`
> 
> > Source/WebCore/editing/cocoa/AutofillElements.cpp:108
> > +                return AutofillElements(WTFMove(start), WTFMove(nextElement),
> > +                    hasSecondPasswordFieldToFill ? WTFMove(elementAfterNextElement) : nullptr);
> 
> Nit - I think this should could just be a single line as `return {{
> WTFMove(start), WTFMove(nextElement), hasSecondPasswordFieldToFill ?
> WTFMove(elementAfterNextElement) : nullptr }};`

[Ricky Mondello]

Created attachment 419433 [details]
Second try

[Wenson Hsieh]

Comment on attachment 419433 [details]
Second try

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

r=mews

> Source/WebCore/editing/cocoa/AutofillElements.cpp:78
>      : m_username(username)
>      , m_password(password)
> +    , m_secondPassword(secondPassword)

(Not exactly new to this patch, but these could all be `WTFMove`d into the class members)

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #6)
> Comment on attachment 419433 [details]
> Second try
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419433&action=review
> 
> r=mews
> 
> > Source/WebCore/editing/cocoa/AutofillElements.cpp:78
> >      : m_username(username)
> >      , m_password(password)
> > +    , m_secondPassword(secondPassword)
> 
> (Not exactly new to this patch, but these could all be `WTFMove`d into the
> class members)

Perhaps we can just do this as a followup.

[Wenson Hsieh]

Comment on attachment 419433 [details]
Second try

C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorDOMAgent.cpp(1761,99): error C2514: 'WTF::Optional': class template cannot be constructed (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-5.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorDOMAgent.cpp(1761,99): error C2451: conditional expression of type 'WTF::Optional' is illegal (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-5.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorDOMAgent.cpp(1762,36): error C2039: 'setLayoutContextType': is not a member of 'Inspector::Protocol::DOM::Node' (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-5.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorDOMAgent.cpp(1762,54): error C2663: 'WTF::Optional<T>::value': 3 overloads have no legal conversion for 'this' pointer (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-5.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]

Windows build failure does not appear to be related.

[EWS]

Committed r272448: <https://trac.webkit.org/changeset/272448>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419433 [details].