Bug 194397 – [iOS] [WK2] Modernize code for applying autocorrection

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

• Use BlockPtr instead of temporarily storing the completion handler.
• Change a LegacySync to Delayed.

[Wenson Hsieh]

Created attachment 361409 [details]
Patch

[Wenson Hsieh]

Created attachment 361459 [details]
Rebase on trunk

[WebKit Commit Bot]

Comment on attachment 361459 [details]
Rebase on trunk

Clearing flags on attachment: 361459

Committed r241180: <https://trac.webkit.org/changeset/241180>

[Daniel Bates]

Comment on attachment 361459 [details]
Rebase on trunk

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3387
> +    _page->applyAutocorrection(correction, input, [view = retainPtr(self), completion = makeBlockPtr(completionHandler)](auto& string, auto error) {

Why is it necessary to call this even if completionHandler be nil? Can you say what caller passes a nil completion handler? I guess it's not crazy to pass a nil completion handler for this function if you just want the correction to try to be applied :/

[Daniel Bates]

I'm importing this into radar because it trampled over my work.

[Radar WebKit Bug Importer]

<rdar://problem/47938347>

[Wenson Hsieh]

(In reply to Daniel Bates from comment #4)
> Comment on attachment 361459 [details]
> Rebase on trunk
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361459&action=review
> 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3387
> > +    _page->applyAutocorrection(correction, input, [view = retainPtr(self), completion = makeBlockPtr(completionHandler)](auto& string, auto error) {
> 
> Why is it necessary to call this even if completionHandler be nil? Can you
> say what caller passes a nil completion handler? I guess it's not crazy to
> pass a nil completion handler for this function if you just want the
> correction to try to be applied :/

It's just defensive programming. In practice, I don't think the completion handler should be nil. In the case where it is, the API contract is such that we ought to still apply the autocorrection, even if the caller (UIKit) doesn't care when the autocorrection is finished applying.

[Daniel Bates]

(In reply to Wenson Hsieh from comment #7)
> (In reply to Daniel Bates from comment #4)
> > Comment on attachment 361459 [details]
> > Rebase on trunk
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=361459&action=review
> > 
> > > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3387
> > > +    _page->applyAutocorrection(correction, input, [view = retainPtr(self), completion = makeBlockPtr(completionHandler)](auto& string, auto error) {
> > 
> > Why is it necessary to call this even if completionHandler be nil? Can you
> > say what caller passes a nil completion handler? I guess it's not crazy to
> > pass a nil completion handler for this function if you just want the
> > correction to try to be applied :/
> 
> It's just defensive programming. In practice, I don't think the completion
> handler should be nil. In the case where it is, the API contract is such
> that we ought to still apply the autocorrection, even if the caller (UIKit)
> doesn't care when the autocorrection is finished applying.

I think most of the nullity checks in this patch fall into the illogical and actively harmful category (read: not defensive). How do you figure it makes sense for **(void)** requestAutocorrection...: (<—- emphasis mine) to ever be passed a nil completion handler? I don’t even need to look at the return type to know this. It’s right there in the name, “request”. How are we going to give it to them? This is a violation of an invariant. We must not continue execution, assert or halt and catch fire.

[Daniel Bates]

Comment on attachment 361459 [details]
Rebase on trunk

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3220
> +        if (completionHandler)

Hell will freeze over before completionHandler is nil here in a sane world. Halt and catch fire.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3239
> +        if (completion)

Ditto.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3382
> +        if (completionHandler)

I think I could be convinced this could be nil.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3388
> +        if (completion)

I think I could be convinced this could be nil.

[Daniel Bates]

(In reply to Daniel Bates from comment #9)
> Comment on attachment 361459 [details]
> Rebase on trunk
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361459&action=review
> 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3220
> > +        if (completionHandler)
> 
> Hell will freeze over before completionHandler is nil here in a sane world.
> Halt and catch fire.

“sane world” is not the right phrase. I don’t know how to convey what world it would have to be. In logic, I would say a “well-formed world” (it may not be sound though!)

[Wenson Hsieh]

> I think most of the nullity checks in this patch fall into the illogical and
> actively harmful category (read: not defensive). How do you figure it makes
> sense for **(void)** requestAutocorrection...: (<—- emphasis mine) to ever
> be passed a nil completion handler? I don’t even need to look at the return
> type to know this. It’s right there in the name, “request”. How are we going
> to give it to them? This is a violation of an invariant. We must not
> continue execution, assert or halt and catch fire.

I'm not quite I understand the rationale here. It's true that it doesn't make sense for -requestAutocorrectionContext…: to take nil. However, I don't see why that would mean crashing is the optimal choice in this scenario.

That being said, I can see value in letting clients of this API know that they're passing nil here when they shouldn't. The canonical way of letting clients know that an API is being misused on Cocoa platforms is to raise an NSException with a description. Maybe this is preferable?

[Daniel Bates]

(In reply to Wenson Hsieh from comment #11)
> > I think most of the nullity checks in this patch fall into the illogical and
> > actively harmful category (read: not defensive). How do you figure it makes
> > sense for **(void)** requestAutocorrection...: (<—- emphasis mine) to ever
> > be passed a nil completion handler? I don’t even need to look at the return
> > type to know this. It’s right there in the name, “request”. How are we going
> > to give it to them? This is a violation of an invariant. We must not
> > continue execution, assert or halt and catch fire.
> 
> I'm not quite I understand the rationale here. It's true that it doesn't
> make sense for -requestAutocorrectionContext…: to take nil. However, I don't
> see why that would mean crashing is the optimal choice in this scenario.
> 

Do anything except what you’re doing now is what I was going for. Do anything that makes noise to be more precise.

> That being said, I can see value in letting clients of this API know that
> they're passing nil here when they shouldn't. The canonical way of letting
> clients know that an API is being misused on Cocoa platforms is to raise an
> NSException with a description. Maybe this is preferable?

Yes, this is preferable.

[Daniel Bates]

Do not go gentle into that good night.