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.