Bug 107041 – Tighten RenderLayerModelObject subclass constructors to operate on ContainerNodes.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Antti Koivisto]

Knowing you have a ContainerNode avoids some branches you get by calling plain Node functions. Tighter typing is better in general.

[Antti Koivisto]

With some more refactoring (involving RenderView) most of the rendering tree could use Elements.

[Antti Koivisto]

Created attachment 183023 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 183023 [details]
patch

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

> Source/WebCore/ChangeLog:8
> +        The only non-ContainerNodes show up in the text rendering code. Tighter typing is better in general. It also enables better code

I can't parse "The only non-ContainerNodes show up in the text rendering code"

> Source/WebCore/ChangeLog:28
> +        (RenderBlock):

Remove useless lines like this.

> Source/WebCore/rendering/RenderFullScreen.cpp:59
> +RenderFullScreen::RenderFullScreen(Document* document)
> +    : RenderDeprecatedFlexibleBox(document)

Is this always the Document, even if some non-document element is taken fullscreen?

> Source/WebCore/rendering/RenderLayerModelObject.h:51
> +    ContainerNode* node() const { return toContainerNode(RenderObject::node()); }

A comment noting whether this can be null might be helpful.

> Source/WebCore/rendering/RenderObject.cpp:148
> -    if (contentData && !contentData->next() && contentData->isImage() && doc != node && !node->isPseudoElement()) {
> -        RenderImage* image = new (arena) RenderImage(node);
> +    if (contentData && !contentData->next() && contentData->isImage() && !element->isPseudoElement()) {
> +        RenderImage* image = new (arena) RenderImage(element);

So the element can never be the Document? Would be nice for the Changelog to note this.

> Source/WebCore/rendering/RenderTextTrackCue.cpp:37
>  RenderTextTrackCue::RenderTextTrackCue(TextTrackCueBox* node)
> -    : RenderBlock(static_cast<Node*>(node))
> +    : RenderBlock(node)

Ugh, so confusing for something called TextTrackCueBox to be en Element (not a RenderBox*)..

[Eric Seidel (no email)]

Fantastic!

[Antti Koivisto]

(In reply to comment #3)
> > Source/WebCore/rendering/RenderFullScreen.cpp:59
> > +RenderFullScreen::RenderFullScreen(Document* document)
> > +    : RenderDeprecatedFlexibleBox(document)
> 
> Is this always the Document, even if some non-document element is taken fullscreen?

Giving a Document as the node parameter just makes the renderer anonymous. Looks like the actual element that is going to full screen is provided as a child renderer for RenderFullScreen.

The code for making anonymous renderers could be clearer but that is outside of the scope here.

> > Source/WebCore/rendering/RenderObject.cpp:148
> > -    if (contentData && !contentData->next() && contentData->isImage() && doc != node && !node->isPseudoElement()) {
> > -        RenderImage* image = new (arena) RenderImage(node);
> > +    if (contentData && !contentData->next() && contentData->isImage() && !element->isPseudoElement()) {
> > +        RenderImage* image = new (arena) RenderImage(element);
> 
> So the element can never be the Document? Would be nice for the Changelog to note this.

Documents are not Elements, they are ContainerNodes. Compiler told that the test was unnecessary (showing why tight typing is good!).

[Antti Koivisto]

http://trac.webkit.org/changeset/139920

[Darin Adler]

Comment on attachment 183023 [details]
patch

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

> Source/WebCore/ChangeLog:9
> +        generation (especially with Document* moving from Node to ContainerNode).

Antti, when do ContainerNodes that are not Elements show up? As far as I know, that’s usually just Document, which I don’t think has a renderer.

> Source/WebCore/ChangeLog:11
> +        This patch tightens constuctors for better static type checking. It also overrides node() with a covariant ContainerNode* return type

Typo: constuctors

[Antti Koivisto]

It's always the Document. Moving to Elements is blocked by

- We currently use passing Document* as the node to construct anonymous renderers (renderers without associated node). This is simple to change.
- We also have a few cases of non-anonymous renderers that have Document as their node. The most difficult one to deal with is the RenderView. It inherits RenderBlock forcing large chunk of the rendering tree use ContainerNodes. There are number of ways this could be fixed (making it anonymous, switching it to other base class, making it use documentElement instead, etc).
- RenderFlowThreads are also non-anonymous Document renderers. I suspect that code is just confused and could be fixed easily.