[webkit-reviews] review denied: [Bug 59157] ShadowContentElement should affect the order of renderer children : [Attachment 91701] Updated to ToT

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied MORITA Hajime
<morrita at google.com>'s request for review:
Bug 59157: ShadowContentElement should affect the order of renderer children
https://bugs.webkit.org/show_bug.cgi?id=59157

Attachment 91701: Updated to ToT
https://bugs.webkit.org/attachment.cgi?id=91701&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
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().