Bug 208406 - Batch observations and completions of text manipulations
Summary: Batch observations and completions of text manipulations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-29 00:28 PST by Ryosuke Niwa
Modified: 2020-03-03 21:22 PST (History)
3 users (show)

See Also:

Attachments
WIP (32.89 KB, patch)
2020-02-29 00:43 PST, Ryosuke Niwa 		no flags Details | Formatted Diff | Diff
Patch (68.27 KB, patch)
2020-03-02 23:13 PST, Ryosuke Niwa 		no flags Details | Formatted Diff | Diff
Fixed build (68.27 KB, patch)
2020-03-03 13:47 PST, Ryosuke Niwa 		wenson_hsieh: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2020-02-29 00:28:20 PST 
We should patch findings of paragraphs and completions of text manipulations.

<rdar://problem/59643650>
Comment 1 Ryosuke Niwa 2020-02-29 00:43:37 PST 
Created attachment 392051 [details]
WIP
Comment 2 Ryosuke Niwa 2020-03-02 23:13:18 PST 
Created attachment 392250 [details]
Patch
Comment 3 Ryosuke Niwa 2020-03-03 13:47:18 PST 
Created attachment 392323 [details]
Fixed build
Comment 4 Wenson Hsieh 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.
Comment 5 Ryosuke Niwa 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.
Comment 6 Ryosuke Niwa 2020-03-03 21:22:16 PST 
Committed r257830: <https://trac.webkit.org/changeset/257830>