Bug 157763

Summary: Make handleAcceptedCandidate a public function
Product: WebKit Reporter: Beth Dakin <bdakin>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, cdumez, commit-queue, japhet
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch thorton: review+

Description Beth Dakin 2016-05-16 15:51:17 PDT
Make handleAcceptedCandidate a public function so that it can be called from another class. rdar://problem/26206397
Comment 1 Beth Dakin 2016-05-16 15:57:24 PDT
Created attachment 279057 [details]
Patch
Comment 2 Conrad Shultz 2016-05-17 12:43:13 PDT
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?
Comment 3 Beth Dakin 2016-05-17 15:57:43 PDT
(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.
Comment 4 Beth Dakin 2016-05-17 15:58:05 PDT
Created attachment 279176 [details]
Patch
Comment 5 Tim Horton 2016-05-17 16:15:50 PDT
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.
Comment 6 Beth Dakin 2016-05-17 16:24:29 PDT
Thanks Tim!