Bug 88580 – RenderObject should not know about first-line element

• Component: Layout and Rendering
• Status: NEW

[Igor Trindade Oliveira]

RenderObject has some code related with first-line. This code should be in RenderBlockLineLayout or RenderBoxModelObject.

[Igor Trindade Oliveira]

Created attachment 146408 [details]
exploratory patch

[Levi Weintraub]

Comment on attachment 146408 [details]
exploratory patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146408&action=review

> Source/WebCore/rendering/InlineBox.h:278
> +    RenderStyle* styleForFirstLine(bool isStyleForFirstLine) const
> +    { 
> +        RenderStyle* style = m_renderer->style();
> +        if (isStyleForFirstLine) {
> +            if (m_renderer->isText())
> +                style = toRenderBoxModelObject(m_renderer->parent())->styleForFirstLine(isStyleForFirstLine);
> +            else
> +                style = boxModelObject()->styleForFirstLine(isStyleForFirstLine);
> +        }
> +        return style;
> +    }

This naming confuses me. If the function is called styleForFirstLine I don't expect to call it with a bool saying whether I actually want the first line.

Also, this is too much code to be written in the definition. I'd rather see something like what you see in RenderObject::preservesNewline() where you state it's inline, implement it in the header, but have the implementation at the end of the file.

> Source/WebCore/rendering/InlineBox.h:296
> +    EVerticalAlign verticalAlign() const
> +    {
> +        RenderBoxModelObject* object;
> +        if (m_renderer->isText())
> +            object = toRenderBoxModelObject(m_renderer->parent());
> +        else
> +            object = boxModelObject();
> +        return object->styleForFirstLine(m_bitfields.firstLine())->verticalAlign();
> +    }

Ditto.

[Julien Chaffraix]

Comment on attachment 146408 [details]
exploratory patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146408&action=review

>> Source/WebCore/rendering/InlineBox.h:278
>> +    }
> 
> This naming confuses me. If the function is called styleForFirstLine I don't expect to call it with a bool saying whether I actually want the first line.
> 
> Also, this is too much code to be written in the definition. I'd rather see something like what you see in RenderObject::preservesNewline() where you state it's inline, implement it in the header, but have the implementation at the end of the file.

For the boolean, this is unfortunately the current state of our code where we get a boolean telling us if we are on the first line. I would have preferred more differentiated code paths but that's likely too difficult to implement as part of this change.

The naming could indeed love some massaging. It's more a firstLineAwareStyle() than really a styleForFirstLine().

> Source/WebCore/rendering/InlineTextBox.cpp:922
> -    renderer()->getTextDecorationColors(deco, underline, overline, linethrough, true, isFirstLineStyle());
> +    toRenderBoxModelObject(renderer()->parent())->getTextDecorationColors(deco, underline, overline, linethrough, true, isFirstLineStyle());

Looks like an change in behavior.

> Source/WebCore/rendering/RenderBR.cpp:45
> +        RenderStyle* s = toRenderBoxModelObject(parent())->styleForFirstLine(firstLine);

There is a lot of toRenderBoxModelObject(someRenderer)->styleForFirstLine. Could it be possible to reduce the number of such casts? One idea that pops up is to change helper functions to take a RenderBoxModelObject instead of a RenderObject.

[Ryosuke Niwa]

(In reply to comment #3)
> For the boolean, this is unfortunately the current state of our code where we get a boolean telling us if we are on the first line. I would have preferred more differentiated code paths but that's likely too difficult to implement as part of this change.
> 
> The naming could indeed love some massaging. It's more a firstLineAwareStyle() than really a styleForFirstLine().

I like that!

[Levi Weintraub]

(In reply to comment #4)
> (In reply to comment #3)
> > For the boolean, this is unfortunately the current state of our code where we get a boolean telling us if we are on the first line. I would have preferred more differentiated code paths but that's likely too difficult to implement as part of this change.
> > 
> > The naming could indeed love some massaging. It's more a firstLineAwareStyle() than really a styleForFirstLine().
> 
> I like that!

Seconded! I wasn't saying we can't take a boolean, I was just saying that the name styleForFirstLine didn't actually seem to describe what the function did. firstLineAwareStyle is clearer.

[Eric Seidel (no email)]

(In reply to comment #0)
> RenderObject has some code related with first-line. This code should be in RenderBlockLineLayout or RenderBoxModelObject.

I support the idea of ripping subclass-specific code out of RenderObject, but it's not clear to me what you're trying to do here.

[Igor Trindade Oliveira]

I am just moving the methods uncachedFirstLineStyle and firstLineStyleSlowCase out of the RenderObject, however doing that i need to change the code in lot of places, specially because RenderObject::style(bool) is also called by RenderText object, so in some cases i need to do:

renderObject->isText() ? toRenderBoxModelObject(renderObject->parent())->firstLineStyle() : toRenderBoxModelObject(renderObject)->firstLineStyle();

This code is valid because firstLineStyleSlowCase() already does:

const RenderObject* renderer = isText() ? parent() : this;

In other parts of the code, i agree that probably the casts are not correctly(e.g. getTextDecorartion) but i am fixing case by case.

(In reply to comment #6)
> (In reply to comment #0)
> > RenderObject has some code related with first-line. This code should be in RenderBlockLineLayout or RenderBoxModelObject.
> 
> I support the idea of ripping subclass-specific code out of RenderObject, but it's not clear to me what you're trying to do here.

[Eric Seidel (no email)]

Comment on attachment 146408 [details]
exploratory patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146408&action=review

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:690
> +        RenderBoxModelObject* boxModelObject = renderer->isText() ? toRenderBoxModelObject(renderer->parent()) : toRenderBoxModelObject(renderer);

Isn't there a boxModelObject() function on a renderer which does this?

[Bruno Abinader (history only)]

Adding bug 93829 as it _might_ change its behavior.