Bug 140523 – Removing an HTML element spends a lot of time in adjustDirectionalityIfNeededAfterChildrenChanged

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

We're spending a lot of time inside adjustDirectionalityIfNeededAfterChildrenChanged while removing a HTML element.

[Ryosuke Niwa]

Created attachment 244721 [details]
Fixes the bug

[Ryosuke Niwa]

<rdar://problem/19464329>

[Chris Dumez]

Comment on attachment 244721 [details]
Fixes the bug

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

> Source/WebCore/html/HTMLElement.cpp:950
>      if (document().renderView() && (changeType == ElementRemoved || changeType == TextRemoved)) {

I think we can just remove this whole if. This code was refactored by Antti in https://bugs.webkit.org/show_bug.cgi?id=120599 and his changeling says he tried to keep the previous behavior of this function. However, if you look at the for loop before the refactoring, it is clear that it was never iterating. if (childCountDelta < 0) { for() } and the for looped only until counter < childCountDelta. counter started at 0.

[Chris Dumez]

and BTW, I removed this whole "if" block on Blink side a while back already.

[Ryosuke Niwa]

Created attachment 244725 [details]
Updated per Chris' comments

[Chris Dumez]

Comment on attachment 244725 [details]
Updated per Chris' comments

r=me

[Chris Dumez]

Comment on attachment 244725 [details]
Updated per Chris' comments

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

> Source/WebCore/html/HTMLElement.cpp:946
>      // FIXME: This function looks suspicious.

And this comment is right. I think it is time we take a better look at this whole function and see if it makes sense.

[Ryosuke Niwa]

Landed in http://trac.webkit.org/changeset/178571.