[Webkit-unassigned] [Bug 149646] Add a WKWebView input delegate SPI

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=149646

--- Comment #10 from mitz at webkit.org <mitz at webkit.org> ---
Comment on attachment 262273
  --> https://bugs.webkit.org/attachment.cgi?id=262273
Patch

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

When making changes that affect API, binary- and source-compatiblity need to be considered. Some of these apply even when changing private API. We need to keep TOT WebKit working with the shipping version of iOS Safari, so we can’t just remove the _formDelegate property’s accessor methods, because shipping Safari calls them (at least the setter). We also can’t remove a header like _WFormDelegate.h or remove other declarations from the header files, because it breaks compatibility with some source code internally at Apple, so the changes will need to be staged over a short period of time until that source can be updated.

> Source/WebKit2/UIProcess/API/Cocoa/_WKElementFocusContext.h:63
> + at protocol _WKElementFocusContext <NSObject>

I am not sure what “focus context” means. We have been using the suffix “info” for immutable snapshots of objects, such as WKFrameInfo, _WKActivatedElementInfo. Perhaps a name similar to _WKFocusedElementInfo would be more in line with those. Or perhaps this is more about the focusing action and not just the element, in which case maybe _WKElementFocusAction (similar to WKNavigationAction) might work?

Why is this a protocol and not a class?

> Source/WebKit2/UIProcess/API/Cocoa/_WKElementFocusContext.h:69
> + at property (nonatomic, readonly) WKInputType inputType;
> +
> +/* The value of the input at the time it is focused. */
> + at property (nonatomic, readonly, copy) NSString *inputValue;

You may be able to drop the “input” suffix from these.

> Source/WebKit2/UIProcess/API/Cocoa/_WKElementFocusContext.h:76
> + at property (nonatomic, readonly) BOOL userIsInteracting;

If you followed in the footsteps of WKNavigationAction you’d call this property userInitiated (with a custom isUserInitiated getter).

> Source/WebKit2/UIProcess/API/Cocoa/_WKInputDelegate.h:44
> +- (BOOL)_webView:(WKWebView *)webView shouldStartInputSession:(id <_WKElementFocusContext>)context;

It’s a little confusing that didStartInputSession: takes an id <_WKFormInputSession> and shouldStartInputSession: takes a different type. Perhaps this method should be called -_webView:focusShouldStartInputSession: or even -_webView:focusActionShouldStartInputSession: depending on how you name the protocol.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:285
> +- (instancetype)initWithAssistedNodeInformation:(const AssistedNodeInformation *)information isInteracting:(BOOL)userIsInteracting;

Can this take a reference instead of a pointer?

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:290
> +    NSString *_inputValue;

This should be a RetainPtr<NSString>.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:296
> +    if (self = [super init]) {

Instead of having most of the method indented like this, you should reverse the condition and early-return nil.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:350
> +        default:

Why do we need a default case here?

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3111
> +        WKElementFocusContext *context = [[WKElementFocusContext alloc] initWithAssistedNodeInformation:&information isInteracting:userIsInteracting];

This is leaking! You should use a RetainPtr to hold this object so that it doesn’t leak.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151007/c773b781/attachment.html>