Bug 59157 – ShadowContentElement should affect the order of renderer children

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

[Hajime Morrita]

Current implementation of ShadowContentElement does not effect the order of RenderObject children of the included DOM nodes.
RenderObjects for the included Nodes is added in order of attach() call.
But the order should be defined by its including ShadowContentElement.

[Hajime Morrita]

Created attachment 90816 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 90816 [details]
Patch

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

Overall, I dig it. I wonder if we should make splitting out NodeRendererFactory into its own patch. That should make actual changes easier to understand.

> Source/WebCore/dom/Node.cpp:1467
> +class NodeRenderCreator {

This is cool. NodeRendererFactory seems like a better name.

> Source/WebCore/dom/Node.cpp:1474
> +        AsContentChildOnContent

I feel like this enum is getting out of hand -- you'll need to draw a picture for me :)

[Hajime Morrita]

Created attachment 91568 [details]
Patch

[Hajime Morrita]

Created attachment 91701 [details]
Updated to ToT

[Dimitri Glazkov (Google)]

Comment on attachment 91701 [details]
Updated to ToT

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

I realize this code is complicated, but it's so obtuse now, I am having a difficult time feeling good about landing it as is. Let's give it another round:

> Source/WebCore/ChangeLog:7
> +        children for each ShadowContentElement. ShadowRoot collect child

collect -> collects

> Source/WebCore/ChangeLog:12
> +        Content children no longer creates its renderer during its normal

creates -> create

> Source/WebCore/ChangeLog:17
> +        as NodeRendererFactory::Type value AsContentChildOnLight and
> +        AsContentChildOnContent.

I think these names are confusing. It took me a few minutes to figure out what they mean, and I know what they do. We need to think of better ones.

> Source/WebCore/dom/Node.cpp:1484
> +    RenderObject* rendererBeforeChild() const;

before or after? nextRenderer seems to point toward the "after".

> Source/WebCore/dom/Node.cpp:1614
> +    // FIXME: This side effect should be visible from attach() code.

Indeed and that's the only reason you even need AsContentChildOnLight.

> Source/WebCore/dom/ShadowRoot.cpp:75
> +    s_currentInstance = this;

This is clever, but it is not immediately clear that you're doing this to avoid passing extra state params to attach(), which would definitely be an enormous change.

I wonder if we can somehow expose methods/checks that help the future WebKit engineers to understand what's going on.

> Source/WebCore/dom/ShadowRoot.cpp:100
> +        if (child->attached()) {
> +            child->detach();
> +            child->attach();
> +        }

Can you make a static inline helper for this, called forceReattach(Node*)?

> Source/WebCore/dom/ShadowRoot.cpp:161
> +        if (attached()) {
> +            detach();
> +            attach();
> +        }

use forceReattach.

> Source/WebCore/dom/ShadowRoot.cpp:171
> +ContainerNode* ShadowRoot::contentContainerFor(Node*)

Since this no longer needs an argument, perhaps we should remove it altogether? Also, the name can now be currentContentElement().

Overall, it seems weird that we're reaching into the ShadowRoot to query for this. Seems like this should be a static accessor on ShadowContentSelector, queried directly from Node.

> Source/WebCore/dom/ShadowRoot.cpp:183
> +    // This results re-attach() shadow tree. see ShadowRoot::recalcStyle().

Change this to:

// This results in forced detaching/attaching of the shadow render tree. See ShadowRoot::recalcStyle().

[Dimitri Glazkov (Google)]

Comment on attachment 91701 [details]
Updated to ToT

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

> Source/WebCore/dom/Node.cpp:1467
> +        AsContentChildOnLight,
> +        AsContentChildOnContent

Oh, I know -- let's not conflate this enum. Let's have another one indicating attachment phase.

Also, can we rename the opaque-sounding "Type" to "TreeLocation", with these values:

NotFound -> NotInTree
AsLightChild -> InTree
AsShadowChild -> InShadowTree
AsContentChild -> InShadowContentTree

> Source/WebCore/dom/Node.cpp:1483
>      ContainerNode* findVisualParent();

This is mis-named. We are doing more than just finding parent, and since it's only used from constructor, you could probably just put this code in constructor.

[Hajime Morrita]

Comment on attachment 91701 [details]
Updated to ToT

Hi Dimitri, thank you for review and sorry for my slow response.
I finally revised the patch...

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

> Source/WebCore/ChangeLog:8
> +        nodes of its host, and the descendant ShadowContentElement pulls

Fixed.

>> Source/WebCore/ChangeLog:12
>> +        Content children no longer creates its renderer during its normal
> 
> creates -> create

Fixed.

>> Source/WebCore/ChangeLog:17
>> +        AsContentChildOnContent.
> 
> I think these names are confusing. It took me a few minutes to figure out what they mean, and I know what they do. We need to think of better ones.

Hmm. I renamed two phase into AttachContentLight and AttachContentForwarded.
Splitting Type enum into TreeLocation and AttachPhase might help.

>> Source/WebCore/dom/Node.cpp:1467
>> +        AsContentChildOnContent
> 
> Oh, I know -- let's not conflate this enum. Let's have another one indicating attachment phase.
> 
> Also, can we rename the opaque-sounding "Type" to "TreeLocation", with these values:
> 
> NotFound -> NotInTree
> AsLightChild -> InTree
> AsShadowChild -> InShadowTree
> AsContentChild -> InShadowContentTree

Renamed:
NotFound -> LocationNotInTree,
AsLightChild -> LocationLightChild,
AsShadowChild  -> LocationShadowChild,
AsContentChild -> gone (there is no such position. content children are light children.)

Point here is that I don't choose InShadowTree because the value is used only for direct child of ShadowRoot, but not for its descendant.

>> Source/WebCore/dom/Node.cpp:1483
>>      ContainerNode* findVisualParent();
> 
> This is mis-named. We are doing more than just finding parent, and since it's only used from constructor, you could probably just put this code in constructor.

Merged it into the constructor.

>> Source/WebCore/dom/Node.cpp:1484
>> +    RenderObject* rendererBeforeChild() const;
> 
> before or after? nextRenderer seems to point toward the "after".

Went back to original name.

>> Source/WebCore/dom/ShadowRoot.cpp:75
>> +    s_currentInstance = this;
> 
> This is clever, but it is not immediately clear that you're doing this to avoid passing extra state params to attach(), which would definitely be an enormous change.
> 
> I wonder if we can somehow expose methods/checks that help the future WebKit engineers to understand what's going on.

Hmm. One idea is to change showTree() family to be content-aware.
I'd like to cleanup (like giving separate file for each these new classes before exploring ideas.

>> Source/WebCore/dom/ShadowRoot.cpp:100
>> +        }
> 
> Can you make a static inline helper for this, called forceReattach(Node*)?

Done.

>> Source/WebCore/dom/ShadowRoot.cpp:161
>> +        }
> 
> use forceReattach.

Done.

>> Source/WebCore/dom/ShadowRoot.cpp:171
>> +ContainerNode* ShadowRoot::contentContainerFor(Node*)
> 
> Since this no longer needs an argument, perhaps we should remove it altogether? Also, the name can now be currentContentElement().
> 
> Overall, it seems weird that we're reaching into the ShadowRoot to query for this. Seems like this should be a static accessor on ShadowContentSelector, queried directly from Node.

Removed argument.
I don't think make ShadowContentSelector visible from Node is a good idea.
In my feeling, it is a implementation detail of shadow tree and ShadowRoot is a facade of it.
What do you think?

>> Source/WebCore/dom/ShadowRoot.cpp:183
>> +    // This results re-attach() shadow tree. see ShadowRoot::recalcStyle().
> 
> Change this to:
> 
> // This results in forced detaching/attaching of the shadow render tree. See ShadowRoot::recalcStyle().

Done.

[Hajime Morrita]

Created attachment 93404 [details]
Patch

[WebKit Review Bot]

Comment on attachment 93404 [details]
Patch

Attachment 93404 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8693085

New failing tests:
fast/html/details-nested-2.html
fast/html/details-nested-1.html
fast/html/details-add-details-child-1.html
fast/html/details-add-details-child-2.html

[WebKit Review Bot]

Created attachment 93406 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Hajime Morrita]

Created attachment 93410 [details]
Mark new tests missing expectations

[Dimitri Glazkov (Google)]

Comment on attachment 93410 [details]
Mark new tests missing expectations

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

I am still not 100% happy with our TreeLocation/AttachPhase names. We need to make them clearer -- consider reading this code through the eyes of someone who doesn't understand shadow DOM or how it works. "Light", "Shadow", "Content" -- all these terms are only well-understood in the context of existing shadow DOM knowledge. We should work to eliminate this knowledge requirement.

> Source/WebCore/dom/Node.cpp:1470
> +        AttachStraight,

Straight seems awkward, but I can't think of a better name at the moment.

> Source/WebCore/dom/ShadowRoot.cpp:75
> +    // XXX: Make this lazy

Probably didn't mean to leave it this way?

[Hajime Morrita]

Comment on attachment 93410 [details]
Mark new tests missing expectations

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

Hi Dimitri, thank you for reviewing! I'll land this shortly and start some cleanup.

>> Source/WebCore/dom/ShadowRoot.cpp:75
>> +    // XXX: Make this lazy
> 
> Probably didn't mean to leave it this way?

Oops. Will remove this before landing.

[Hajime Morrita]

> I am still not 100% happy with our TreeLocation/AttachPhase names. We need to make them clearer -- consider reading this code through the eyes of someone who doesn't understand shadow DOM or how it works. "Light", "Shadow", "Content" -- all these terms are only well-understood in the context of existing shadow DOM knowledge. We should work to eliminate this knowledge requirement.
I agree with this.
On the other hand, what we are doing contains something different actually.
I think we need to make that something clear, explicit first citizen, instead of
eliminating its name behind.

Next I'll pull ShadowContentElement under dom/, which might clarify the relationship between
shadow family members.

[Hajime Morrita]

Committed r86521: <http://trac.webkit.org/changeset/86521>