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
Beth Dakin
2016-05-16 15:51:17 PDT
Created attachment 279057 [details]
Patch
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? (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. Created attachment 279176 [details]
Patch
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. Thanks Tim! |