Bug 208406

Summary: Batch observations and completions of text manipulations
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, mifenton, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP
none
Patch
none
Fixed build wenson_hsieh: review+

Ryosuke Niwa
Reported 2020-02-29 00:28:20 PST
We should patch findings of paragraphs and completions of text manipulations. <rdar://problem/59643650>
Attachments
WIP (32.89 KB, patch)
2020-02-29 00:43 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Patch (68.27 KB, patch)
2020-03-02 23:13 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Fixed build (68.27 KB, patch)
2020-03-03 13:47 PST, Ryosuke Niwa 		wenson_hsieh: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2020-02-29 00:43:37 PST
Created attachment 392051 [details] WIP
Ryosuke Niwa
Comment 2 2020-03-02 23:13:18 PST
Created attachment 392250 [details] Patch
Ryosuke Niwa
Comment 3 2020-03-03 13:47:18 PST
Created attachment 392323 [details] Fixed build
Wenson Hsieh
Comment 4 2020-03-03 15:24:34 PST
Comment on attachment 392323 [details] Fixed build View in context: https://bugs.webkit.org/attachment.cgi?id=392323&action=review > Source/WebCore/editing/TextManipulationController.cpp:279 > + m_callback(*m_document, m_pendingItemsForCallback); > + m_pendingItemsForCallback.clear(); Nit - you could call flushPendingItemsForCallback() here instead of repeating the code. > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1658 > + // WebCore::TextManipulationController::ItemIdentifier itemID, const Vector<WebCore::TextManipulationController::ManipulationToken>& tokens Nit - stray comment? > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1694 > + [wkItems addObject:[[_WKTextManipulationItem alloc] initWithIdentifier:String::number(item.identifier.toUInt64()) tokens:wkTokens]]; This appears to be a leak — use RetainPtr/adoptNS or -autorelease? > Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:75 > + for (_WKTextManipulationItem* item in items) Nit - * on the other side.
Ryosuke Niwa
Comment 5 2020-03-03 21:00:39 PST
(In reply to Wenson Hsieh from comment #4) > Comment on attachment 392323 [details] > Fixed build > > View in context: > https://bugs.webkit.org/attachment.cgi?id=392323&action=review > > > Source/WebCore/editing/TextManipulationController.cpp:279 > > + m_callback(*m_document, m_pendingItemsForCallback); > > + m_pendingItemsForCallback.clear(); > > Nit - you could call flushPendingItemsForCallback() here instead of > repeating the code. Good idea. Done. > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1658 > > + // WebCore::TextManipulationController::ItemIdentifier itemID, const Vector<WebCore::TextManipulationController::ManipulationToken>& tokens > > Nit - stray comment? Oops, removed. > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1694 > > + [wkItems addObject:[[_WKTextManipulationItem alloc] initWithIdentifier:String::number(item.identifier.toUInt64()) tokens:wkTokens]]; > > This appears to be a leak — use RetainPtr/adoptNS or -autorelease? Oh oops, fixed it by adoptNS into a local variable first. > > Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:75 > > + for (_WKTextManipulationItem* item in items) > > Nit - * on the other side. Fixed.
Ryosuke Niwa
Comment 6 2020-03-03 21:22:16 PST
Committed r257830: <https://trac.webkit.org/changeset/257830>
Note You need to log in before you can comment on or make changes to this bug.