Bug 76262 – StyleRecalc should occur when shadow root exists and light children are changed.

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

[Shinya Kawanaka]

To support dynamic ShadowContentElement fallback, style recalculation should occur when children of node which has shadow tree and content element are changed.
To achieve it, maybe we should add some code into Element::childrenChanged.
We should measure the performance when adding code.

[Shinya Kawanaka]

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

[Shinya Kawanaka]

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

[Shinya Kawanaka]

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.

[Dimitri Glazkov (Google)]

Can haz patch? :)

[Shinya Kawanaka]

Created attachment 123113 [details]
Patch

[WebKit Review Bot]

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

[Dimitri Glazkov (Google)]

Why are details tests sad?

[Shinya Kawanaka]

(In reply to comment #7)
> Why are details tests sad?

Hmm...
Let me check it.

[Shinya Kawanaka]

(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...

[Hajime Morrita]

(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.

[Shinya Kawanaka]

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.

[Shinya Kawanaka]

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?

[Shinya Kawanaka]

(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.

[Dimitri Glazkov (Google)]

(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.

[Shinya Kawanaka]

(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.

[Shinya Kawanaka]

Created attachment 124647 [details]
Test Patch

[Shinya Kawanaka]

(In reply to comment #16)
> Created an attachment (id=124647) [details]
> Test Patch

Ah, actually this patch depends on Bug 76611...

[WebKit Review Bot]

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

[Shinya Kawanaka]

Created attachment 124891 [details]
Patch

[Shinya Kawanaka]

(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.

[Hajime Morrita]

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().

[Shinya Kawanaka]

Created attachment 125045 [details]
Patch

[Hajime Morrita]

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?

[Shinya Kawanaka]

Created attachment 125064 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 125064 [details]
Patch for landing

Clearing flags on attachment: 125064

Committed r106613: <http://trac.webkit.org/changeset/106613>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.