Bug 199326 – Allow Clients to Add Fields to the AutoFillContext Dictionary

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Priyanka]

Allow Clients to Add Fields to the AutoFillContext Dictionary

[Priyanka]

Created attachment 373133 [details]
Patch

[Wenson Hsieh]

Comment on attachment 373133 [details]
Patch

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

Seems reasonable, with a few minor adjustments. It would also be nice to write an API test for this.

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

Nit - please include a radar link below the bugzilla URL.

> Source/WebKit/UIProcess/API/Cocoa/_WKInputDelegate.h:58
> +- (NSDictionary *)_webViewAdditionalContextForStrongPasswordAssistance:(WKWebView *)webView;

This should have an availability macro (i.e. `WK_API_AVAILABLE(ios(WK_IOS_TBA))`) at the end.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:352
> +    NSDictionary *_sfAutoFillAdditionalContext;

Nit - just _autoFillAdditionalContext, or _additionalContextForAutoFill? (It’s a bit weird for SF-prefixed things to leak into WebKit)

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6644
> +        return @{ @"_automaticPasswordKeyboard" : @YES, @"sfAutoFillAdditionalContext": _sfAutoFillAdditionalContext};

Nit - (same naming tweak for this key name)

[Wenson Hsieh]

Comment on attachment 373133 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:352
>> +    NSDictionary *_sfAutoFillAdditionalContext;
> 
> Nit - just _autoFillAdditionalContext, or _additionalContextForAutoFill? (It’s a bit weird for SF-prefixed things to leak into WebKit)

This should also be a RetainPtr<NSDictionary>. We should probably clean this up too in the same places where we currently reset _focusRequiresStrongPasswordAssistance to NO.

[Brian Weinstein]

Comment on attachment 373133 [details]
Patch

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

>>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:352
>>> +    NSDictionary *_sfAutoFillAdditionalContext;
>> 
>> Nit - just _autoFillAdditionalContext, or _additionalContextForAutoFill? (It’s a bit weird for SF-prefixed things to leak into WebKit)
> 
> This should also be a RetainPtr<NSDictionary>. We should probably clean this up too in the same places where we currently reset _focusRequiresStrongPasswordAssistance to NO.

I’d get rid of the sf prefix here, and just call this something like:

NSDictionary *_additionalContextForStrongPasswordAssistance;

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5218
> +        _sfAutoFillAdditionalContext = @{ };

Nit: Our style is @{} (no space).

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6644
>> +        return @{ @"_automaticPasswordKeyboard" : @YES, @"sfAutoFillAdditionalContext": _sfAutoFillAdditionalContext};
> 
> Nit - (same naming tweak for this key name)

I’d also get rid of the sf prefix in your string.

@“_strongPasswordAdditionalContext” seems more clear.

[Priyanka]

Created attachment 373157 [details]
Patch

[Priyanka]

Created attachment 373158 [details]
Patch

[Priyanka]

Adding API Tests to the Patch.

Comment on attachment 373158 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKInputDelegate.h:58
> +- (NSDictionary *)_webViewAdditionalContextForStrongPasswordAssistance:(WKWebView *)webView WK_API_AVAILABLE(ios(WK_IOS_TBA));

Can we use lightweight generics here to make the key and value types in the dictionary more specific?

[Priyanka]

Created attachment 373284 [details]
Patch

[Priyanka]

Created attachment 374748 [details]
Patch

[Priyanka]

Created attachment 374761 [details]
Patch

[Priyanka]

Created attachment 374763 [details]
Patch

[Wenson Hsieh]

Comment on attachment 374763 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:852
> +    _additionalContextForStrongPasswordAssistance = nullptr;

Nit - nil or { } instead of nullptr.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5317
> +    _additionalContextForStrongPasswordAssistance = nullptr;

(Ditto)

> Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:542
> +    if ([inputDelegate respondsToSelector:@selector(_webViewAdditionalContextForStrongPasswordAssistance:)])

Hmmm I might be missing something here, but shouldn’t TestInputDelegate implement -_webViewAdditionalContextForStrongPasswordAssistance: with something like:

- (NSDictionary<id, NSString *> *)_webViewAdditionalContextForStrongPasswordAssistance:(WKWebView *)webView
{
    if (_webViewAdditionalContextForStrongPasswordAssistance)
        return _webViewAdditionalContextForStrongPasswordAssistance(webView);
    return @{ };
}

…and then we would check the value of [[webView textInputContentView] _autofillContext] after focusing a password field?

You’ll probably also need to implement -_webView:_focusRequiresStrongPasswordAssistance: on TestInputDelegate, and return YES in this case.

> Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:547
> +    ASSERT([_additionalContextForStrongPasswordAssistance isEqualToDictionary:expected]);

This should be EXPECT_TRUE.

[Priyanka]

Created attachment 374800 [details]
Patch

[Wenson Hsieh]

Comment on attachment 374800 [details]
Patch

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

r=mews! (with the NSLog removed)

> Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:553
> +    NSLog( @"%@", [[webView textInputContentView] _autofillContext] );

Nit — it looks like you left a bit of logging here.

[Priyanka]

Created attachment 374804 [details]
Patch

[Wenson Hsieh]

(In reply to Priyanka from comment #16)
> Created attachment 374804 [details]
> Patch

It looks like you're missing a ChangeLog entry for Tools — you can generate this using `OpenSource/Tools/Scripts/prepare-ChangeLog Tools`.

[Priyanka]

Created attachment 374812 [details]
Patch

[Priyanka]

Added ChangeLog for Tools and removed the leftover logging.

[Wenson Hsieh]

Comment on attachment 374812 [details]
Patch

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

This patch looks great. Thanks again for working on the test! Just a couple more very minor nits.

> Tools/ChangeLog:4
> +

Nit - extra newline here.

> Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:532
> +    NSDictionary *expected =  @{ @"strongPasswordAdditionalContext" : @"testUUID" };

Nit - extra space here between = and @

[Priyanka]

Created attachment 374833 [details]
Patch

[Priyanka]

Fixed spacing between the = and @, removed the new line, and also removed some tabs in the beginning of blank lines I saw.

[Wenson Hsieh]

<rdar://problem/42816957>

[WebKit Commit Bot]

Comment on attachment 374833 [details]
Patch

Clearing flags on attachment: 374833

Committed r247804: <https://trac.webkit.org/changeset/247804>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.