Bug 109876

Summary: RenderQuote should not mark renderers as needing layout during layout
Product: WebKit Reporter: Elliott Sprehn <esprehn>
Component: New BugsAssignee: Elliott Sprehn <esprehn>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, ddkilzer, eric, inferno, jchaffraix, ojan.autocc, ojan, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Elliott Sprehn
Reported 2013-02-14 16:36:44 PST
RenderQuote should not mark renderers as needing layout during layout
Attachments
Patch (12.82 KB, patch)
2013-02-14 16:41 PST, Elliott Sprehn 		no flags
DetailsFormatted DiffDiff
Patch (12.87 KB, patch)
2013-02-14 18:01 PST, Elliott Sprehn 		no flags
DetailsFormatted DiffDiff
Patch (10.10 KB, patch)
2013-02-15 13:11 PST, Elliott Sprehn 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Elliott Sprehn
Comment 1 2013-02-14 16:41:24 PST
Created attachment 188451 [details] Patch
Elliott Sprehn
Comment 2 2013-02-14 18:01:57 PST
Created attachment 188459 [details] Patch
Ojan Vafai
Comment 3 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+. :)
Elliott Sprehn
Comment 4 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.
Elliott Sprehn
Comment 6 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.
Elliott Sprehn
Comment 7 2013-02-15 13:11:10 PST
Created attachment 188630 [details] Patch
Ojan Vafai
Comment 8 2013-02-15 13:16:57 PST
Comment on attachment 188630 [details] Patch <3
WebKit Review Bot
Comment 9 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>
WebKit Review Bot
Comment 10 2013-02-15 15:13:32 PST
All reviewed patches have been landed. Closing bug.
David Kilzer (:ddkilzer)
Comment 11 2014-01-05 15:30:48 PST
<rdar://problem/13666450>
Note You need to log in before you can comment on or make changes to this bug.