Bug 88580

Created attachment 146408 [details] exploratory patch

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.

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.

(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!

(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.

(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.

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.

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?

Adding bug 93829 as it _might_ change its behavior.