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...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-01-15 15:52 PST by Ryosuke Niwa
Modified: 2015-01-15 19:45 PST (History)
8 users (show)

See Also:

Attachments
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
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

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 
<rdar://problem/19464329>
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

r=me
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.