<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#c10">Comment # 10</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:mitz@webkit.org" title="mitz@webkit.org <mitz@webkit.org>"> <span class="fn">mitz@webkit.org</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>
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.
<span class="quote">> Source/WebKit2/UIProcess/API/Cocoa/_WKElementFocusContext.h:63
> +@protocol _WKElementFocusContext <NSObject></span >
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 class="quote">> Source/WebKit2/UIProcess/API/Cocoa/_WKElementFocusContext.h:69
> +@property (nonatomic, readonly) WKInputType inputType;
> +
> +/* The value of the input at the time it is focused. */
> +@property (nonatomic, readonly, copy) NSString *inputValue;</span >
You may be able to drop the “input” suffix from these.
<span class="quote">> Source/WebKit2/UIProcess/API/Cocoa/_WKElementFocusContext.h:76
> +@property (nonatomic, readonly) BOOL userIsInteracting;</span >
If you followed in the footsteps of WKNavigationAction you’d call this property userInitiated (with a custom isUserInitiated getter).
<span class="quote">> Source/WebKit2/UIProcess/API/Cocoa/_WKInputDelegate.h:44
> +- (BOOL)_webView:(WKWebView *)webView shouldStartInputSession:(id <_WKElementFocusContext>)context;</span >
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 class="quote">> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:285
> +- (instancetype)initWithAssistedNodeInformation:(const AssistedNodeInformation *)information isInteracting:(BOOL)userIsInteracting;</span >
Can this take a reference instead of a pointer?
<span class="quote">> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:290
> + NSString *_inputValue;</span >
This should be a RetainPtr<NSString>.
<span class="quote">> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:296
> + if (self = [super init]) {</span >
Instead of having most of the method indented like this, you should reverse the condition and early-return nil.
<span class="quote">> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:350
> + default:</span >
Why do we need a default case here?
<span class="quote">> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3111
> + WKElementFocusContext *context = [[WKElementFocusContext alloc] initWithAssistedNodeInformation:&information isInteracting:userIsInteracting];</span >
This is leaking! You should use a RetainPtr to hold this object so that it doesn’t leak.</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>