TextManipulationController should ignore any renderer creation that happens within previously manipulated paragraphs. <rdar://problem/58105815>
Created attachment 391839 [details] Patch
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 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)
(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.
I guess I can add that to the chance log so that people would know what's up.
I see — makes sense!
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"
Committed r257613: <https://trac.webkit.org/changeset/257613>