Bug 140523 - Removing an HTML element spends a lot of time in adjustDirectionalityIfNeededAfterChildrenChanged
Summary: Removing an HTML element spends a lot of time in adjustDirectionalityIfNeeded...
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
Keywords: InRadar
Depends on:
Reported: 2015-01-15 15:52 PST by Ryosuke Niwa
Modified: 2015-01-15 19:45 PST (History)
8 users (show)

See Also:

Fixes the bug (2.03 KB, patch)
2015-01-15 16:00 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated per Chris' comments (2.10 KB, patch)
2015-01-15 16:42 PST, Ryosuke Niwa
cdumez: 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 2015-01-15 15:52:17 PST
We're spending a lot of time inside adjustDirectionalityIfNeededAfterChildrenChanged while removing a HTML element.
Comment 1 Ryosuke Niwa 2015-01-15 16:00:14 PST
Created attachment 244721 [details]
Fixes the bug
Comment 2 Ryosuke Niwa 2015-01-15 16:06:21 PST
Comment 3 Chris Dumez 2015-01-15 16:29:55 PST
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.
Comment 4 Chris Dumez 2015-01-15 16:31:01 PST
and BTW, I removed this whole "if" block on Blink side a while back already.
Comment 5 Ryosuke Niwa 2015-01-15 16:42:11 PST
Created attachment 244725 [details]
Updated per Chris' comments
Comment 6 Chris Dumez 2015-01-15 16:48:47 PST
Comment on attachment 244725 [details]
Updated per Chris' comments

Comment 7 Chris Dumez 2015-01-15 16:50:13 PST
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.
Comment 8 Ryosuke Niwa 2015-01-15 19:45:21 PST
Landed in http://trac.webkit.org/changeset/178571.