[webkit-reviews] review granted: [Bug 135326] RenderSelectionInfoBase/RenderSelectionInfo/RenderBlockSelectionInfo should take renderer reference. : [Attachment 235581] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Zalan Bujtas <zalan at apple.com>'s
request for review:
Bug 135326:
RenderSelectionInfoBase/RenderSelectionInfo/RenderBlockSelectionInfo should
take renderer reference.
https://bugs.webkit.org/show_bug.cgi?id=135326

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=235581&action=review


Good refactoring.

> Source/WebCore/ChangeLog:3
> +	   RenderSelectionInfoBase/RenderSelectionInfo/RenderBlockSelectionInfo
should take renderer reference.

Bug title is a little more specific than the slightly broader refactoring this
patch does.

> Source/WebCore/rendering/RenderSelectionInfo.h:37
> +    explicit RenderSelectionInfoBase(RenderObject& renderer)

Too bad we can’t make the type tighter than RenderObject, but I don’t think we
can.

> Source/WebCore/rendering/RenderSelectionInfo.h:51
> +    void repaintRectangle(const LayoutRect& repaintRect)
> +    {
> +	   m_renderer.repaintUsingContainer(m_repaintContainer,
enclosingIntRect(repaintRect));
> +    }

I’m not sure this should be in the class definition. I think classes are easier
to read if we move multiple-line function definitions out of the class
definitions. I’ve been doing that when working on various header files.

> Source/WebCore/rendering/RenderSelectionInfo.h:84
> -    Vector<LayoutRect> m_rects;
> +    Vector<LayoutRect> m_rects; // relative to repaint container
>      LayoutRect m_rect; // relative to repaint container

Really unclear how these data members relate. Same is true of the two public
functions that expose them; not clear why we have both.