[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 #11 from Wenson Hsieh <wenson_hsieh at apple.com> ---
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

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

_WKFocusedElementInfo sounds much better. The purpose of this entity is to provide clients with information that can help customize the assistance behavior of a focused element -- this includes both a "snapshot" of the focused element, as well as some information about the action itself (e.g. isUserInitiated).

I made this a protocol because I was following the footsteps of WKFormInputSession, which also has a private protocol declaration and an implementation in WKContentViewInteraction.mm where it needs to be initialized.

>> Source/WebKit2/UIProcess/API/Cocoa/_WKElementFocusContext.h:69
>> + at property (nonatomic, readonly, copy) NSString *inputValue;
> 
> You may be able to drop the “input” suffix from these.

Changed to just type and value.

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

Got it. Renamed to isUserInitiated

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

Good point. Renamed shouldStartInputSession to focusShouldStartInputSession.

>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:285
>> +- (instancetype)initWithAssistedNodeInformation:(const AssistedNodeInformation *)information isInteracting:(BOOL)userIsInteracting;
> 
> Can this take a reference instead of a pointer?

Yes -- changed to use a const reference.

>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:290
>> +    NSString *_inputValue;
> 
> This should be a RetainPtr<NSString>.

Fixed.

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

Sounds good! Changed.

>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:350
>> +        default:
> 
> Why do we need a default case here?

Good point, it isn't necessary, since we have a 1-1 mapping. I was thinking of using default to catch input types we (for any reason) don't want to expose.

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

Good catch! Fixed.

-- 
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/ddfe5d0d/attachment-0001.html>