Bug 208286 – TextManipulationController should not generate a new item for content in manipulated paragraphs

• Component: WebKit API
• Status: RESOLVED FIXED

[Ryosuke Niwa]

TextManipulationController should ignore any renderer creation that happens within previously manipulated paragraphs.

<rdar://problem/58105815>

[Ryosuke Niwa]

Created attachment 391839 [details]
Patch

[Wenson Hsieh]

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?

[Wenson Hsieh]

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)

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

I guess I can add that to the chance log so that people would know what's up.

[Wenson Hsieh]

I see — makes sense!

[Ryosuke Niwa]

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"

[Ryosuke Niwa]

Committed r257613: <https://trac.webkit.org/changeset/257613>