Bug 208286 - TextManipulationController should not generate a new item for content in manipulated paragraphs
Summary: TextManipulationController should not generate a new item for content in mani...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-27 00:01 PST by Ryosuke Niwa
Modified: 2020-02-27 19:37 PST (History)
4 users (show)

See Also:


Attachments
Patch (10.72 KB, patch)
2020-02-27 00:18 PST, Ryosuke Niwa
wenson_hsieh: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2020-02-27 00:01:54 PST
TextManipulationController should ignore any renderer creation that happens within previously manipulated paragraphs.

<rdar://problem/58105815>
Comment 1 Ryosuke Niwa 2020-02-27 00:18:51 PST
Created attachment 391839 [details]
Patch
Comment 2 Wenson Hsieh 2020-02-27 07:40:54 PST
Comment on attachment 391839 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Added an early exist to observeParagraphs when the observed content has an element that has already been manipulated

"early exist” => "early exit"

> Source/WebCore/ChangeLog:16
> +        forever so that we can track any element that has already been manipulated. Als renamed m_mutatedElements

“Als” => “Also"

> Source/WebCore/editing/TextManipulationController.cpp:117
> +    if (!m_manipulatedElements.capacity())

Can we just check isEmpty() here?
Comment 3 Wenson Hsieh 2020-02-27 09:14:28 PST
Comment on attachment 391839 [details]
Patch

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

>> Source/WebCore/editing/TextManipulationController.cpp:117
>> +    if (!m_manipulatedElements.capacity())
> 
> Can we just check isEmpty() here?

(Sorry, I meant computesEmpty() since it’s a WeakHashSet)
Comment 4 Ryosuke Niwa 2020-02-27 11:27:59 PST
(In reply to Wenson Hsieh from comment #3)
> Comment on attachment 391839 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391839&action=review
> 
> >> Source/WebCore/editing/TextManipulationController.cpp:117
> >> +    if (!m_manipulatedElements.capacity())
> > 
> > Can we just check isEmpty() here?
> 
> (Sorry, I meant computesEmpty() since it’s a WeakHashSet)

No, we don't want to do that since computesEmpty() would iterate over all stale WeakPre entries each time it's called. Since m_manipulatedElements is expected to have a lot of stale elements in it (it basically contains all the elements ever inserted into the document and got a renderer), I don't think we want to be doing that. This early check exists for the very first observation where we're going through thousands of paragraphs. We want to skip this check in that particular case. But after that, this hash map will never be empty so we want the check to be fast & fail immediately.

Note that WeakHashMap will eventually clean itself up by removing stale WeakPtr in every rehash, and it won't retain any elements so it's not a leak.
Comment 5 Ryosuke Niwa 2020-02-27 11:28:15 PST
I guess I can add that to the chance log so that people would know what's up.
Comment 6 Wenson Hsieh 2020-02-27 11:44:59 PST
I see — makes sense!
Comment 7 Ryosuke Niwa 2020-02-27 19:23:12 PST
Comment on attachment 391839 [details]
Patch

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

> Source/WebCore/editing/TextManipulationController.cpp:118
> +        return false; // Happens inside startObservingParagraphs.

I've rephrased this as "fast path for startObservingParagraphs"
Comment 8 Ryosuke Niwa 2020-02-27 19:37:24 PST
Committed r257613: <https://trac.webkit.org/changeset/257613>