Bug 109876 - RenderQuote should not mark renderers as needing layout during layout
Summary: RenderQuote should not mark renderers as needing layout during layout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Elliott Sprehn
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-02-14 16:36 PST by Elliott Sprehn
Modified: 2014-01-05 15:30 PST (History)
8 users (show)

See Also:


Attachments
Patch (12.82 KB, patch)
2013-02-14 16:41 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (12.87 KB, patch)
2013-02-14 18:01 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (10.10 KB, patch)
2013-02-15 13:11 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Elliott Sprehn 2013-02-14 16:36:44 PST
RenderQuote should not mark renderers as needing layout during layout
Comment 1 Elliott Sprehn 2013-02-14 16:41:24 PST
Created attachment 188451 [details]
Patch
Comment 2 Elliott Sprehn 2013-02-14 18:01:57 PST
Created attachment 188459 [details]
Patch
Comment 3 Ojan Vafai 2013-02-14 18:29:35 PST
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+. :)
Comment 4 Elliott Sprehn 2013-02-14 18:55:45 PST
(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.
Comment 6 Elliott Sprehn 2013-02-15 12:54:47 PST
(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.
Comment 7 Elliott Sprehn 2013-02-15 13:11:10 PST
Created attachment 188630 [details]
Patch
Comment 8 Ojan Vafai 2013-02-15 13:16:57 PST
Comment on attachment 188630 [details]
Patch

<3
Comment 9 WebKit Review Bot 2013-02-15 15:13:27 PST
Comment on attachment 188630 [details]
Patch

Clearing flags on attachment: 188630

Committed r143060: <http://trac.webkit.org/changeset/143060>
Comment 10 WebKit Review Bot 2013-02-15 15:13:32 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 David Kilzer (:ddkilzer) 2014-01-05 15:30:48 PST
<rdar://problem/13666450>