Bug 76262

This is a result before adding if-statement. http://dromaeo.com/?id=160729

This is after adding if-statement. About 5% performance degrade? http://dromaeo.com/?id=160730

I've tried a few times again. Before adding if-statement http://dromaeo.com/?id=160732 http://dromaeo.com/?id=160733 http://dromaeo.com/?id=160734 Then, I've added if (hasRareDate() && shadowRoot()) setNeedsStyleRecalc() in the last of Element::childrenChanged. After adding it, http://dromaeo.com/?id=160735 http://dromaeo.com/?id=160736 http://dromaeo.com/?id=160738 It seems adding if-statement is not harmful to performance.

Can haz patch? :)

Created attachment 123113 [details] Patch

Comment on attachment 123113 [details] Patch Attachment 123113 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11299091 New failing tests: fast/html/details-replace-text.html fast/html/details-remove-child-2.html fast/html/details-remove-child-1.html

Why are details tests sad?

(In reply to comment #7) > Why are details tests sad? Hmm... Let me check it.

(In reply to comment #8) > (In reply to comment #7) > > Why are details tests sad? > > Hmm... > Let me check it. Some unnecessary RenderBlock exists in DETAILS block. I'm thinking we should reconsider the attach order of light children when a shadow tree exists (Bug 76611) to fix it...

(In reply to comment #9) > Some unnecessary RenderBlock exists in DETAILS block. > I'm thinking we should reconsider the attach order of light children when a shadow tree exists (Bug 76611) to fix it... Yeah, let's fix Bug 76611 before this one.

I was investigating why anonymous RenderBlock was created today. Let's consider the following. <div>foo<div>bar</div></div> This will create renderer tree like the following: RenderBlock RenderBlock (anonymous) RenderText <-- (1) RenderBlock {DIV} <-- (2) RenderText When (1) is removed, the parent RenderBlock is not deleted. If (2) is removed, the previous anonymous RenderBlock is deleted. Also, let's consider the following. <div>foo<div>bar</div>baz</div> RenderBlock RenderBlock (anonymous) RenderText RenderBlock {DIV} RenderText RenderBlock (anonymous) RenderText When foo and baz are removed, these anonymous RenderBlock won't be removed. When DIV is deleted, those anonymous RenderBlocks are merged, then removed.

I'm now thinking left anonymous RenderBlock are not harmful at all... Can I change the test result (and create a bug to delete empty anonymous render block)? Or should I think another way?

(In reply to comment #12) > I'm now thinking left anonymous RenderBlock are not harmful at all... > > Can I change the test result (and create a bug to delete empty anonymous render block)? > Or should I think another way? Hmm... I'm now thinking when detaching SUMMARY, anonymous RenderBlock for DETAILS should be deleted. If SUMMARY and DETAILS are DIV, anonymous RenderBlock is deleted.

(In reply to comment #2) > This is after adding if-statement. About 5% performance degrade? > http://dromaeo.com/?id=160730 I think you need a bit more investigation here. I don't understand how DOM attributes test could be even impacted by this.

(In reply to comment #14) > (In reply to comment #2) > > This is after adding if-statement. About 5% performance degrade? > > http://dromaeo.com/?id=160730 > > I think you need a bit more investigation here. I don't understand how DOM attributes test could be even impacted by this. OKay... (In reply to comment #14) > (In reply to comment #2) > > This is after adding if-statement. About 5% performance degrade? > > http://dromaeo.com/?id=160730 > > I think you need a bit more investigation here. I don't understand how DOM attributes test could be even impacted by this. I agree that DOM attribute test is not relevant to this. But I think DOM modification test is relevant, because childrenChanged is called when children are changed. From the test result (especially from appendChild test and insertBefore test), I don't think my patch affect performance so much.

Created attachment 124647 [details] Test Patch

(In reply to comment #16) > Created an attachment (id=124647) [details] > Test Patch Ah, actually this patch depends on Bug 76611...

Comment on attachment 124647 [details] Test Patch Attachment 124647 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11150498 New failing tests: fast/html/details-replace-text.html fast/html/details-remove-child-2.html fast/html/details-remove-child-1.html

Created attachment 124891 [details] Patch

(In reply to comment #19) > Created an attachment (id=124891) [details] > Patch Now Bug 76611 was fixed, this patch won't make tests sad...hopefully.

Comment on attachment 124891 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124891&action=review > Source/WebCore/ChangeLog:8 > + Set style recalculation flag when a child element is changed. Could you elaborate why we need this change? > Source/WebCore/dom/Element.cpp:1371 > + setNeedsStyleRecalc(); This could be shadowRoot->hostChildrenChanged().

Created attachment 125045 [details] Patch

Comment on attachment 125045 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125045&action=review > Source/WebCore/ChangeLog:9 > + So we have to recalculate content element again. "recalculate content element" sounds strange. What we need to recompute is inclusion of that, right?

Created attachment 125064 [details] Patch for landing

Comment on attachment 125064 [details] Patch for landing Clearing flags on attachment: 125064 Committed r106613: <http://trac.webkit.org/changeset/106613>

All reviewed patches have been landed. Closing bug.