Bug 59157

Summary: ShadowContentElement should affect the order of renderer children
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: DOMAssignee: Hajime Morrita <morrita>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 59280    
Bug Blocks: 56967, 56973    
Attachments:
Description Flags
Patch
none
Patch
none
Updated to ToT
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-01
none
Mark new tests missing expectations dglazkov: review+

Description Hajime Morrita 2011-04-21 17:07:44 PDT
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.
Comment 1 Hajime Morrita 2011-04-22 17:51:36 PDT
Created attachment 90816 [details]
Patch
Comment 2 Dimitri Glazkov (Google) 2011-04-22 19:45:25 PDT
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 :)
Comment 3 Hajime Morrita 2011-04-28 15:27:03 PDT
Created attachment 91568 [details]
Patch
Comment 4 Hajime Morrita 2011-04-29 11:00:58 PDT
Created attachment 91701 [details]
Updated to ToT
Comment 5 Dimitri Glazkov (Google) 2011-04-29 13:14:25 PDT
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().
Comment 6 Dimitri Glazkov (Google) 2011-04-29 13:23:39 PDT
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.
Comment 7 Hajime Morrita 2011-05-12 23:16:57 PDT
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.
Comment 8 Hajime Morrita 2011-05-12 23:18:17 PDT
Created attachment 93404 [details]
Patch
Comment 9 WebKit Review Bot 2011-05-12 23:38:02 PDT
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
Comment 10 WebKit Review Bot 2011-05-12 23:38:07 PDT
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
Comment 11 Hajime Morrita 2011-05-13 00:10:40 PDT
Created attachment 93410 [details]
Mark new tests missing expectations
Comment 12 Dimitri Glazkov (Google) 2011-05-13 09:54:54 PDT
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?
Comment 13 Hajime Morrita 2011-05-15 18:40:28 PDT
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.
Comment 14 Hajime Morrita 2011-05-15 18:46:48 PDT
> 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.
Comment 15 Hajime Morrita 2011-05-15 19:55:36 PDT
Committed r86521: <http://trac.webkit.org/changeset/86521>