Bug 157763 – Make handleAcceptedCandidate a public function

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Beth Dakin]

Make handleAcceptedCandidate a public function so that it can be called from another class. rdar://problem/26206397

[Beth Dakin]

Created attachment 279057 [details]
Patch

[Conrad Shultz]

Comment on attachment 279057 [details]
Patch

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

Looks good to me, but I'm not a reviewer unfortunately.

> Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:1201
>      [m_webView showCandidates:candidates forString:m_paragraphContextForCandidateRequest.get() inRect:rectForSelectionCandidates forSelectedRange:m_rangeForCandidates view:m_webView completionHandler:[weakEditor](NSTextCheckingResult *acceptedCandidate) {

Seems like weakEditor is no longer used.

> Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:1202
> +        dispatch_async(dispatch_get_main_queue(), ^{ });

Why is this necessary? Can you pass nil as the completion handler (or, failing that, an empty block)?

> Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:1224
>                  [(WebHTMLView *)view _setSoftSpaceRange:softSpaceRange];

Looks like you could declare softSpaceRange only in the case where the replacement ends in space.

Does this properly handle a case where a replacement ends in multiple spaces?

[Beth Dakin]

(In reply to comment #2)
> Comment on attachment 279057 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279057&action=review
> 
> Looks good to me, but I'm not a reviewer unfortunately.
> 
> > Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:1201
> >      [m_webView showCandidates:candidates forString:m_paragraphContextForCandidateRequest.get() inRect:rectForSelectionCandidates forSelectedRange:m_rangeForCandidates view:m_webView completionHandler:[weakEditor](NSTextCheckingResult *acceptedCandidate) {
> 
> Seems like weakEditor is no longer used.

Removed it.

> 
> > Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:1202
> > +        dispatch_async(dispatch_get_main_queue(), ^{ });
> 
> Why is this necessary? Can you pass nil as the completion handler (or,
> failing that, an empty block)?
> 

Nil does work.

> > Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:1224
> >                  [(WebHTMLView *)view _setSoftSpaceRange:softSpaceRange];
> 
> Looks like you could declare softSpaceRange only in the case where the
> replacement ends in space.
> 
> Does this properly handle a case where a replacement ends in multiple spaces?

The current behavior matches NSTextView, so in that sense it does properly handle multiple spaces.

[Beth Dakin]

Created attachment 279176 [details]
Patch

[Tim Horton]

Comment on attachment 279176 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/mac/ViewWithEditableAreaLeak.mm:41
> +@implementation DoNotLeakWebView

This name is global to the project (because ObjC) so you might want something that includes at least part of the name of your test.

[Beth Dakin]

Thanks Tim!