<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Add a WKWebView input delegate SPI"
href="https://bugs.webkit.org/show_bug.cgi?id=149646#c11">Comment # 11</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Add a WKWebView input delegate SPI"
href="https://bugs.webkit.org/show_bug.cgi?id=149646">bug 149646</a>
from <span class="vcard"><a class="email" href="mailto:wenson_hsieh@apple.com" title="Wenson Hsieh <wenson_hsieh@apple.com>"> <span class="fn">Wenson Hsieh</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=262273&action=diff" name="attach_262273" title="Patch">attachment 262273</a> <a href="attachment.cgi?id=262273&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=262273&action=review">https://bugs.webkit.org/attachment.cgi?id=262273&action=review</a>
<span class="quote">>> Source/WebKit2/UIProcess/API/Cocoa/_WKElementFocusContext.h:63
>> +@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?</span >
_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.
<span class="quote">>> Source/WebKit2/UIProcess/API/Cocoa/_WKElementFocusContext.h:69
>> +@property (nonatomic, readonly, copy) NSString *inputValue;
>
> You may be able to drop the “input” suffix from these.</span >
Changed to just type and value.
<span class="quote">>> Source/WebKit2/UIProcess/API/Cocoa/_WKElementFocusContext.h:76
>> +@property (nonatomic, readonly) BOOL userIsInteracting;
>
> If you followed in the footsteps of WKNavigationAction you’d call this property userInitiated (with a custom isUserInitiated getter).</span >
Got it. Renamed to isUserInitiated
<span class="quote">>> 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.</span >
Good point. Renamed shouldStartInputSession to focusShouldStartInputSession.
<span class="quote">>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:285
>> +- (instancetype)initWithAssistedNodeInformation:(const AssistedNodeInformation *)information isInteracting:(BOOL)userIsInteracting;
>
> Can this take a reference instead of a pointer?</span >
Yes -- changed to use a const reference.
<span class="quote">>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:290
>> + NSString *_inputValue;
>
> This should be a RetainPtr<NSString>.</span >
Fixed.
<span class="quote">>> 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.</span >
Sounds good! Changed.
<span class="quote">>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:350
>> + default:
>
> Why do we need a default case here?</span >
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.
<span class="quote">>> 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.</span >
Good catch! Fixed.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>