[webkit-reviews] review denied: [Bug 101144] SimplifyMarkupCommand takes a disproportionally long time to run when there are many nodes to remove : [Attachment 172446] Lazy attach + avoid calling isContentEditable

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 101144: SimplifyMarkupCommand takes a disproportionally long time to run
when there are many nodes to remove
https://bugs.webkit.org/show_bug.cgi?id=101144

Attachment 172446: Lazy attach + avoid calling isContentEditable
https://bugs.webkit.org/attachment.cgi?id=172446&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=172446&action=review


Can you include a manual test-case? That way if anyone needs to work on this
code in the future, they will be able to make sure they don't regress it if
they need to change this significantly.

> Source/WebCore/ChangeLog:19
> +	   this specific case is not the worth the increase in the bot cycle
time. I'll note that the email
> +	   attached in the original radar bug (<rdar://problem/12179712>) took
100 seconds to open now only takes
> +	   7 seconds to open on my MacPro.

Do you still get the same numbers with this new patch?

> Source/WebCore/editing/ApplyStyleCommand.cpp:945
> +    node->document()->updateStyleIfNeeded();

Is this needed now because we lazy attach? If so, would be good to put a
comment to that effect.

> Source/WebCore/editing/SimplifyMarkupCommand.cpp:44
> +    Vector<RefPtr<Node> > nodesToRemove;

This looks like a behavior change to me. As in, it looks like you are fixing a
use-after-free bug here. Mind making a test-case for this?

> Source/WebCore/editing/SimplifyMarkupCommand.cpp:90
> +	   int numPrunedAncestors =
pruneSubsequentAncestorsToRemove(nodesToRemove, i);

It seems to me that we could do something much simpler here. In the loop above,
have two vectors:
nodesToRemove and nodeWithChildrenToPreserve (needs a better name).

Only add the highest ancestor to remove to nodesToRemove and the lowest
ancestor to remove to nodeWithChildrenToPreserve. Then, create a version of
removeNodePreservingChildren that takes a first argument of the highest
ancestor to remove and a second argument of the lowest ancestor to remove.

Then removeNodePreservingChildren just reparents the children of the lowest
ancestor and then removes the highest ancestor.

In addition to being simpler, I expect that would perform much better as well.
Also, then I don't think we'd need to do any of this lazyAttach or
AssumeContentIsAlwaysEditable stuff.