[webkit-reviews] review granted: [Bug 44412] Teach InlineBox about IntPoint : [Attachment 65098] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Seidel <eric at webkit.org>'s
request for review:
Bug 44412: Teach InlineBox about IntPoint
https://bugs.webkit.org/show_bug.cgi?id=44412

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

------- Additional Comments from Darin Adler <darin at apple.com>
Great that you’re taking this on. Looks good!

I can't tell how you are choosing when to use x() and y() and when you are
choosing to use left() and top(). I think it would be better not to add top()
and left() or to remove x() and y() if you really want to add them. One of th
first curious cases I ran into was EllipsisBox::paintSelection, which seems to
mix the two in way that makes it non-obvious why one part has one and one part
has the other.

> +    // This is currently only used by ElipsesBox

You mean EllipisBox. Comment needs a period. This is a type of comment that can
easily become incorrect, and I’m not sure of its value. We might want to just
leave it out.

> -	   , m_x(x)
> -	   , m_y(y)
> +	   , m_topLeft(topLeft)

I think position might be a better name than topLeft.

> +    IntRect rect() const
> +    {
> +	   // FIXME: Will this want virtualHeight for the SVG case?
> +	   return IntRect(topLeft(), IntSize(width(), height()));
> +    }

Is there some way you could resolve this and so avoid adding the FIXME? I don’t
like adding confusion here.

I’m going to say review+, but I think this is not 100% improvement.