Bug 23453 – Devirtualize width/height/x/y on RenderObject and just make it non-virtual on RenderBox

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dave Hyatt]

This bug is covering taking width/height/x/y on RenderObject and removing it from that class completely.  The methods will become non-virtual on RenderBox instead.

This forces many derived classes to tighten up their type handling and will make people have to think about the special objects like RenderTexts that can consist of more than one rect.

[Dave Hyatt]

Created attachment 26891 [details]
Work in progress. All files compile except for RenderObject.cpp and bidi.cpp.

[Dave Hyatt]

Created attachment 26902 [details]
Compiles.  Crashes, but at least it compiles. :)

Just storing my work.  Not ready yet.

[Dave Hyatt]

Created attachment 26913 [details]
Patch that passes all layout tests

[Dave Hyatt]

Created attachment 26914 [details]
Patch that passes all layout tests

[Darin Adler]

Comment on attachment 26914 [details]
Patch that passes all layout tests

>              if (o->isText() && static_cast<RenderText *>(o)->firstTextBox()) {
> -                point.move(static_cast<RenderText *>(o)->minXPos(),
> +                point.move(static_cast<RenderText *>(o)->boundingBoxX(),
>                             static_cast<RenderText *>(o)->firstTextBox()->root()->topOverflow());
> -            } else
> -                point.move(o->xPos(), o->yPos());
> +            } else {
> +                RenderBox* box = static_cast<RenderBox*>(o);
> +                point.move(box->x(), box->y());
> +            }

This code is casting this to RenderBox* without checking that it's a RenderBox. For example, it could be a RenderText* with 0 for firstTextBox().

>      if (!o->isInline() || o->isReplaced())
>      {
> +        RenderBox* box = static_cast<RenderBox*>(o);

What guarantees this is a RenderBox? There are some strange classes like RenderPath, RenderSVGContainer, and RenderSVGGradientStop. Are we guaranteed it's not one of those? Do the isInline and isReplaced checks guarantee it's a RenderBox?

> +            if (o->isText()) {
> +                RenderText* text = static_cast<RenderText*>(o);
> +                point.move(text->boundingBoxX() + text->boundingBoxWidth(), text->boundingBoxHeight());
> +            } else {
> +                RenderBox* box = static_cast<RenderBox*>(o);
> +                point.move(box->x() + box->width(), box->y() + box->height());
> +            }

What prevents us from getting a RenderObject of one of the more exotic types here? Is it safe to assume it can only be RenderText or RenderBox?

> +    return m_renderer && m_renderer->isBox() ? static_cast<const RenderBox* const>(m_renderer) : 0;

You shouldn't need the "const" after the "*" in this expression.

>  IntRect Node::getRect() const
>  {
>      // FIXME: broken with transforms
> -    if (renderer()) {
> -        FloatPoint absPos = renderer()->localToAbsolute();
> -        return IntRect(roundedIntPoint(absPos),
> -                       IntSize(renderer()->width(), renderer()->height() + renderer()->borderTopExtra() + renderer()->borderBottomExtra()));
> -    }
> +    if (renderer())
> +        return renderer()->absoluteBoundingBoxRect();
>      return IntRect();
>  }

Could we do this fix first in another patch?

> +    // Use with caution. Does no type checking.  Mostly a convenience method for shadow nodes of form controls, where we know exactly
> +    // what kind of renderer we made.
> +    RenderBox* renderBox() const;

This comment is inaccurate. The function does indeed do type checking. I think what you mean to say is that this is illegal to call on something that has a renderer that is not a RenderBox.

> -    return node->renderer()->isReplaced() && canHaveChildrenForEditing(node) && node->renderer()->height() != 0 && !node->firstChild();
> +    return node->renderer()->isReplaced() && canHaveChildrenForEditing(node) && static_cast<RenderBox*>(node->renderer())->height() != 0 && !node->firstChild();

Should use renderBox() here. Is this renderer guaranteed to not have an exotic type of RenderObject?

>      if (!node()->hasTagName(htmlTag) && renderer->isBlockFlow() && !hasRenderedNonAnonymousDescendantsWithHeight(renderer) &&
> -       (renderer->height() || node()->hasTagName(bodyTag)))
> +       (static_cast<RenderBox*>(renderer)->height() || node()->hasTagName(bodyTag)))
>          return offset() == 0 && !nodeIsUserSelectNone(node());

Is isBlockFlow() enough to guarantee this is a RenderBox and not a RenderText or exotic type of RenderObject?

> -    if (renderer->height() == 0 || (renderer->isListItem() && renderer->isEmpty()))
> +    RenderBlock* block = static_cast<RenderBlock*>(renderer);
> +    if (block->height() == 0 || (block->isListItem() && block->isEmpty()))
>          return appendBlockPlaceholder(container);

What guarantees the renderer is a RenderBlock?

> -    return renderer()->width();
> +    return static_cast<RenderBox*>(renderer())->width();

Should use the renderBox() function here.

> -    return renderer()->height();
> +    return static_cast<RenderBox*>(renderer())->height();

Here too.

> -    RenderObject* renderer = m_highlightedNode->renderer();
> +    RenderBox* renderer = m_highlightedNode->renderBox();

This is bad in debug builds because renderBox() will assert if the renderer is a RenderText. Unless you somehow know this is a RenderBox?

> +    // Use with caution! The type is not checked!
> +    RenderBox* renderBox() const;

I think you should say this differently. Instead you should say this is only legal to call if you know the renderer is a RenderBox. I don't understand why you included the runtime check in Node, but not here.

I stopped reviewing at InlineFlowBox.h. I'll look again later.

I think this patch is really tricky because it mixes mechanical changes with real changes to the substance of things. As long as you're confident we have good enough regression test coverage, I think I can make myself comfortable with it.

[Robert O'Callahan]

All the Element scroll*/client*/offset* methods call renderBox(), which asserts the renderer is a RenderBox and will die if it isn't, but those methods can be called on any kind of element.

[Eric Seidel (no email)]

Comment on attachment 26914 [details]
Patch that passes all layout tests

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.

[Dave Hyatt]

Fixed in r40107.