Bug 199326

Created attachment 373133 [details] Patch

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)

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.

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.

Created attachment 373157 [details] Patch

Created attachment 373158 [details] Patch

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?

Created attachment 373284 [details] Patch

Created attachment 374748 [details] Patch

Created attachment 374761 [details] Patch

Created attachment 374763 [details] Patch

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.

Created attachment 374800 [details] Patch

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.

Created attachment 374804 [details] Patch

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

Created attachment 374812 [details] Patch

Added ChangeLog for Tools and removed the leftover logging.

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 @

Created attachment 374833 [details] Patch

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

<rdar://problem/42816957>

Comment on attachment 374833 [details] Patch Clearing flags on attachment: 374833 Committed r247804: <https://trac.webkit.org/changeset/247804>

All reviewed patches have been landed. Closing bug.