Bug 194397

Summary: [iOS] [WK2] Modernize code for applying autocorrection
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: HTML EditingAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dbates, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 194373    
Bug Blocks:    
Attachments:
Description Flags
Patch
thorton: review+
Rebase on trunk none

Description Wenson Hsieh 2019-02-07 10:09:53 PST
• Use BlockPtr instead of temporarily storing the completion handler.
• Change a LegacySync to Delayed.
Comment 1 Wenson Hsieh 2019-02-07 10:11:47 PST
Created attachment 361409 [details]
Patch
Comment 2 Wenson Hsieh 2019-02-07 15:13:05 PST
Created attachment 361459 [details]
Rebase on trunk
Comment 3 WebKit Commit Bot 2019-02-07 17:45:48 PST
Comment on attachment 361459 [details]
Rebase on trunk

Clearing flags on attachment: 361459

Committed r241180: <https://trac.webkit.org/changeset/241180>
Comment 4 Daniel Bates 2019-02-08 20:28:28 PST
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 :/
Comment 5 Daniel Bates 2019-02-08 20:33:00 PST
I'm importing this into radar because it trampled over my work.
Comment 6 Radar WebKit Bug Importer 2019-02-08 20:33:28 PST
<rdar://problem/47938347>
Comment 7 Wenson Hsieh 2019-02-08 20:39:02 PST
(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.
Comment 8 Daniel Bates 2019-02-08 22:24:28 PST
(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.
Comment 9 Daniel Bates 2019-02-08 22:35:21 PST
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.
Comment 10 Daniel Bates 2019-02-08 22:40:07 PST
(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!)
Comment 11 Wenson Hsieh 2019-02-08 23:43:52 PST
> 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?
Comment 12 Daniel Bates 2019-02-08 23:50:26 PST
(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.
Comment 13 Daniel Bates 2019-02-08 23:56:18 PST
Do not go gentle into that good night.