Created attachment 388598 [details]
Patch

Comment on attachment 388598 [details]
Patch

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

r=me but I have to admit the logic of the change is not obvious to me

> Source/WebCore/rendering/RenderElement.cpp:1419
> +            while (currentElement && currentElement->hasDisplayContents())
> +                currentElement = currentElement->parentElement();

It’s not obvious *why* this is the right thing to do. So we need a comment. Why are we skipping over any element that "has display contents"?

(In reply to Darin Adler from comment #2)
> Comment on attachment 388598 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388598&action=review
> 
> r=me but I have to admit the logic of the change is not obvious to me
> 
> > Source/WebCore/rendering/RenderElement.cpp:1419
> > +            while (currentElement && currentElement->hasDisplayContents())
> > +                currentElement = currentElement->parentElement();
> 
> It’s not obvious *why* this is the right thing to do. So we need a comment.
> Why are we skipping over any element that "has display contents"?

Ryosuke?  You were the one that mentioned that we needed to find the first parent without "display: contents" set -- but it ultimately has something to do with the next line, where we need a valid renderer.  Is there something special about "display: contents" that would need this?

Created attachment 388888 [details]
Patch

I managed to speak with Ryosuke, and he confirmed that if an element is set to "display: contents" it will not have a renderer.  Hopefully the improved comment clarifies this; I must admit that I'm still not terribly familiar with its behaviors.

Comment on attachment 388888 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:1419
> +            // If an Element hasDisplayContents(), it will not have a renderer.
> +            // Instead, we must walk up the tree to find a parent element with a valid renderer.

This comment more or less repeats what the code says.
I'd try giving more of a why comment like this:
// When an element has display: contents, this element doesn't have a renderer
// and its children will render as children of the parent element.

There is no need to say that we're looking for the first element without display: contents
since that's pretty self explanatory from the code.

(In reply to Ryosuke Niwa from comment #6)
> Comment on attachment 388888 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388888&action=review
> 
> > Source/WebCore/rendering/RenderElement.cpp:1419
> > +            // If an Element hasDisplayContents(), it will not have a renderer.
> > +            // Instead, we must walk up the tree to find a parent element with a valid renderer.
> 
> This comment more or less repeats what the code says.
> I'd try giving more of a why comment like this:
> // When an element has display: contents, this element doesn't have a
> renderer
> // and its children will render as children of the parent element.
> 
> There is no need to say that we're looking for the first element without
> display: contents
> since that's pretty self explanatory from the code.

Agreed; I was trying to think of the best way to word it without adding anything which might be misleading.  This sounds much better. :)

Created attachment 388897 [details]
Patch

Comment on attachment 388897 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:1417
> +            Element* currentElement = element()->shadowHost();

auto* please

Created attachment 389262 [details]
Patch

The commit-queue encountered the following flaky tests while processing attachment 389262 [details]:

The commit-queue is continuing to process your patch.

The commit-queue encountered the following flaky tests while processing attachment 389262 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org)
The commit-queue is continuing to process your patch.

Comment on attachment 389262 [details]
Patch

Clearing flags on attachment: 389262

Committed r255448: <https://trac.webkit.org/changeset/255448>

All reviewed patches have been landed.  Closing bug.