[Webkit-unassigned] [Bug 121221] RenderBR should not be RenderText

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=121221





--- Comment #13 from Antti Koivisto <koivisto at iki.fi>  2013-09-13 12:20:29 PST ---
> Wonder why this isText check is needed. I assume a text renderer would not have an element.

It is shouldn't be needed anymore actually. It was the former isBR() test.

> Looks like box and br have something in common. Maybe some day they can have a common base class to fix this sort of thing?

Yeah, there would room for a common base.

> WTF is "markable"? Any why does a text node that might be completely collapsed maybe or a <br> automatically qualify?

No idea.

> Why is the !isText() needed here?

inlineBox->isText() is not the same is inlineBox->renderer().isText(). It just has a confusing name.

> Add override? Or maybe this is not an override.

Nope, it is the base class.

> > Source/WebCore/rendering/InlineTextBox.cpp:368
> > -    return renderer().isBR() || (renderer().style()->preserveNewline() && len() == 1 && (*textRenderer().text())[start()] == '\n');
> > +    return renderer().style()->preserveNewline() && len() == 1 && (*textRenderer().text())[start()] == '\n';
> 
> Not sure why it's right to remove it here.

RenderBR now generates InlineBoxes not InlineTextBoxes.

> Seems like some of the functions in this class end up a little “copy and pastey”. Wonder if there are ways to share a little more of the code with RenderText. Just some inline functions maybe? Or maybe not worth it.

RenderText versions are generally much more complicated, I doubt there is much useful sharing.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.