Bug 109876 – RenderQuote should not mark renderers as needing layout during layout

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Elliott Sprehn]

RenderQuote should not mark renderers as needing layout during layout

[Elliott Sprehn]

Created attachment 188451 [details]
Patch

[Elliott Sprehn]

Created attachment 188459 [details]
Patch

[Ojan Vafai]

Comment on attachment 188459 [details]
Patch

I tried to make sense of this stuff and got lost. Can you explain it to me?

For example, why can't we just call attachQuote directly in the RenderQuote constructor and how do we get in a situation where there are multiple quotes needing attach?

Obviously, this change is an improvement. I just want to actually understand it before I r+. :)

[Elliott Sprehn]

(In reply to comment #3)
> (From update of attachment 188459 [details])
> I tried to make sense of this stuff and got lost. Can you explain it to me?
> 
> For example, why can't we just call attachQuote directly in the RenderQuote constructor and how do we get in a situation where there are multiple quotes needing attach?

Inside the render tree there's a linked list of RenderQuotes (m_next,m_prev) with the head of the list stored on the RenderView. Being attached means being inside that linked list so we can compute the nesting depth of the quotes. We can't be attached until the RenderQuote is inside the rooted render tree with the RenderView at the top so we can compute the depth properly and know which quote is really the head of the list.

We can end up in the situation where we need multiple for attach when you have:

content: open-quote open-quote open-quote; // or any variation of quotes.

In these cases PseudoElement::attach will create a bunch of RenderQuotes.

> 
> Obviously, this change is an improvement. I just want to actually understand it before I r+. :)

In thinking through this more we should be able to just use a static ListHashSet since we only need to keep track of the quotes for the duration of PseudoElement::attach.

[Abhishek Arya]

Have you tested if this fixes any of https://bugs.webkit.org/show_bug.cgi?id=109616, https://bugs.webkit.org/show_bug.cgi?id=108839, https://bugs.webkit.org/show_bug.cgi?id=109450 ?

[Elliott Sprehn]

(In reply to comment #5)
> Have you tested if this fixes any of https://bugs.webkit.org/show_bug.cgi?id=109616, https://bugs.webkit.org/show_bug.cgi?id=108839, https://bugs.webkit.org/show_bug.cgi?id=109450 ?

It fixes 109616, I couldn't see the other ones since I'm not on the security list (could you add me? :)). I had a realization last night that all this complexity isn't needed, we can just do it immediately after the addChild() in PseudoElement::attach.

Patch coming soon.

[Elliott Sprehn]

Created attachment 188630 [details]
Patch

[Ojan Vafai]

Comment on attachment 188630 [details]
Patch

<3

[WebKit Review Bot]

Comment on attachment 188630 [details]
Patch

Clearing flags on attachment: 188630

Committed r143060: <http://trac.webkit.org/changeset/143060>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

<rdar://problem/13666450>