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

[bugzilla-daemon at webkit.org]

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





--- Comment #12 from Darin Adler <darin at apple.com>  2013-09-13 12:05:24 PST ---
(From update of attachment 211575)
View in context: https://bugs.webkit.org/attachment.cgi?id=211575&action=review

> Source/WebCore/dom/Document.cpp:5847
> -                if (!curr->node() || !curr->node()->isElementNode() || curr->isText())
> +                if (!curr->node() || !curr->node()->isElementNode() || curr->isText() || curr->isBR())
>                      continue;

Wonder why this isText check is needed. I assume a text renderer would not have an element.

> Source/WebCore/dom/Position.cpp:63
> +static bool hasInlineBoxWrapper(RenderObject& renderer)
> +{
> +    if (renderer.isBox() && toRenderBox(renderer).inlineBoxWrapper())
> +        return true;
> +    if (renderer.isText() && toRenderText(renderer).firstTextBox())
> +        return true;
> +    if (renderer.isBR() && toRenderBR(renderer).inlineBoxWrapper())
> +        return true;
> +    return false;
> +}

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?

> Source/WebCore/editing/ApplyStyleCommand.cpp:1024
> +    if (node->renderer()->isBR() && node->renderer()->style()->isCollapsibleWhiteSpace('\n'))
> +        return;

I think there's another way to write this where we ask the style the question more directly without actually passing '\n', but on the other hand this should be pretty fast so maybe that's overkill.

> Source/WebCore/editing/Editor.cpp:3079
> +        if (node->renderer()->isBR())
> +            return node;

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

> Source/WebCore/rendering/InlineBox.cpp:161
> +    if (renderer().isBR() && !isText())
> +        return 0;

Why is the !isText() needed here?

> Source/WebCore/rendering/InlineBox.cpp:195
> +        if (m_renderer.isBox())
> +            toRenderBox(renderer()).setInlineBoxWrapper(0);
> +        else if (renderer().isBR())
> +            toRenderBR(renderer()).setInlineBoxWrapper(0);

Here’s that same box/br polymorphism I was wondering about earlier.

> Source/WebCore/rendering/InlineBox.h:72
> +    virtual bool isLineBreak() const { return renderer().isBR(); }

Add override? Or maybe this is not an override.

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

> Source/WebCore/rendering/RenderBR.h:3
> + * Copyright (C) 2013 Apple Computer, Inc.

Apple Inc.

> Source/WebCore/rendering/RenderBR.h:31
> +class RenderBR FINAL : public RenderBoxModelObject {

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.

> Source/WebCore/rendering/RenderBR.h:50
> +    virtual VisiblePosition positionForPoint(const LayoutPoint&) OVERRIDE;

We don't have to shout OVERRIDE any more. We can just say override these days.

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