[webkit-reviews] review granted: [Bug 93056] Reimplement RenderQuote placement algorithm : [Attachment 157495] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Elliott Sprehn <esprehn at gmail.com>'s
request for review:
Bug 93056: Reimplement RenderQuote placement algorithm
https://bugs.webkit.org/show_bug.cgi?id=93056

Attachment 157495: Patch
https://bugs.webkit.org/attachment.cgi?id=157495&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=157495&action=review


Looks OK.

> Source/WebCore/rendering/RenderQuote.cpp:130
> +    for (RenderObject* predecessor = previousInPreOrder(); predecessor;
predecessor = predecessor->previousInPreOrder()) {

Wow, so this will end up walking all the way back through the rendering tree.
That seems expensive. :)  Thankfully we only do this when we know there is
already a quote in the doc.

> Source/WebCore/rendering/RenderQuote.cpp:131
> +	   if (!predecessor->isQuote() ||
!toRenderQuote(predecessor)->m_attached)

Presumably a (private) accessor would be better than grabbing at a private
member here, but it doens't really matter.

I also would have added a small comment about *why* we skip un-attached quotes.


> Source/WebCore/rendering/RenderQuote.h:53
>      int m_depth;
>      RenderQuote* m_next;
>      RenderQuote* m_previous;
> -    void placeQuote();
> +    bool m_attached;

Do we care about the size of this object?  Should we be stealing a bit from
m_depth to use for m_attached?