[webkit-reviews] review granted: [Bug 23453] Devirtualize width/height/x/y on RenderObject and just make it non-virtual on RenderBox : [Attachment 26914] Patch that passes all layout tests

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Dave Hyatt <hyatt at apple.com>'s
request for review:
Bug 23453: Devirtualize width/height/x/y on RenderObject and just make it
non-virtual on RenderBox
https://bugs.webkit.org/show_bug.cgi?id=23453

Attachment 26914: Patch that passes all layout tests
https://bugs.webkit.org/attachment.cgi?id=26914&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
Seems this could use a FIXME for when you fix y():

 void RenderBlock::paint(PaintInfo& paintInfo, int tx, int ty)
14851485 {
1486	  tx += m_x;
1487	  ty += m_y;
 1486	  tx += x();
 1487	  ty += m_frameRect.y();

I guess you should just search for m_frameRect.y(), cause you seem to use that
in several places.

I doubt this is a win:
55	   // Optimize for the case where we don't move at all.
 56	    if (x == m_frameRect.x() && y == m_frameRect.y())
 57		return;

But if you found it to be in the PLT, I'll believe you.

The next lines:
58	   m_frameRect.setX(x);
 59	    m_frameRect.setY(y);
can be replaced by
m_frameRect.setOrigin(point) if you invert which setLocation calls which
setLocation

Seems odd this would be needed:
private:
 72 protected:
6973	 RenderObject* m_firstChild;
7074	 RenderObject* m_lastChild;

Why this cast is safe is unclear to me:

564		if (!c->isFloatingOrPositioned() && !c->isText() &&
!c->isInlineFlow())
 565		     bottom = max(bottom, static_cast<RenderBox*>(c)->y() +
c->lowestPosition(false));

If table cells are RenderBoxs, seems we should just cast directly to
RenderTableCell:
	     if (child->isTableCell()) {
 2233		      IntRect bbox =
static_cast<RenderBox*>(child)->borderBoxRect();


Maybe I'm missing context, but this seems unsafe:
@@ void RenderObject::updateHitTestResult(H
 2569		  RenderBox* block = static_cast<RenderBox*>(this);
27412570	     if (isInline())
27422571		 block = containingBlock();

I hope SVG never depended on this?
TransformationMatrix RenderObject::localTransform() const
32703012 {
3271	  return TransformationMatrix(1, 0, 0, 1, xPos(), yPos());
 3013	  return TransformationMatrix();
32723014 }
32733015 

Seems unintentional:
127	 bool m_widthChanged : 1;
 124	 bool m_widthChanged: 1;


Phew!  I read through it all.

I'm not sure I understand all of the SVG changes, but that's probably why the
SVG code got wrong in the first place. :)

Assuming all of the pixel tests pass (or are changing to better values), then
r=me.