Bug 37741 – Clean up RenderPart/RenderPartObject/RenderFrame/RenderEmbeddedObject

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

The class hierarchy for these renders is whacko:

           RenderPart
                |
       -------------------
      |                   |
      v                   V
RenderPartObject       RenderFrame
      |
      v
RenderEmbeddedObject

RenderEmbeddedObject is instantiated for plugins.
RenderFrame is instantiated for <frame>
RenderPartObject is instantiated for <iframe>

<frame> and <iframe> share some code in RenderPart, namely RenderPart::layoutWithFlattening(), but this does not apply to plug-ins. Also, most of the code in RenderPartObject only applies to iframes.

I think it should be:

           RenderPart
                |
       ----------------------
      |                      |
      v                      V
RenderEmbeddedObject    RenderFrameBase
                             |
                     ------------------
                    |                  |
                RenderFrame        RenderIFrame

[Simon Fraser (smfr)]

Created attachment 53752 [details]
Patch

[Darin Adler]

Comment on attachment 53752 [details]
Patch

> -// Renderer for embeds and objects.
> +// Renderer for embeds and objects (whose content may not necessarily be rendered via a plug-in, e.g. <embed src="foo.html>).

There's a missing close quote mark here.

I think this comment would be clearer as a second sentence. And I think it would be clearer if you could word it as a positive rather than a negative.

> + * Copyright (C) 2010 Apple Computer, Inc.  All rights reserved.

Should be "Copyright (C) 2010 Apple Inc. All rights reserved." with one space and no "Computer ,".

> +using namespace HTMLNames;
> +    

I noticed a trailing space here.

> +RenderFrameBase::~RenderFrameBase()
> +{
> +}

Can we just leave this out and let the compiler generate it?

> +void RenderFrameBase::layoutWithFlattening(bool fixedWidth, bool fixedHeight)

It would be nice to use "svn copy" so that we can see the history of this code instead of it seeming all new.

> +class RenderFrameBase : public RenderPart {
> +public:
> +    RenderFrameBase(Element*);

Constructor should be protected.

> +    virtual ~RenderFrameBase();

This destructor should just be omitted.

> +    void layoutWithFlattening(bool fixedWidth, bool fixedHeight);

This function should be protected rather than public, I think.

> +RenderIFrame::~RenderIFrame()
> +{
> +}

Same comment as above. Please omit this.

> +// Base class for RenderFrame and RenderIFrame
> +class RenderIFrame : public RenderFrameBase {

That comment is wrong.

> +public:
> +    RenderIFrame(Element*);
> +    virtual ~RenderIFrame();

You should omit this destructor.

> +    virtual void calcHeight();
> +    virtual void calcWidth();
> +
> +    virtual void layout();

Can these virtual functions all be private?

review+ but I'd like to see a version that preserves history and keeps things more private as mentioned above.

[Simon Fraser (smfr)]

Comment on attachment 53752 [details]
Patch

Landed in http://trac.webkit.org/changeset/57866 with svn moves, and comments addressed.

[Simon Fraser (smfr)]

Created attachment 53768 [details]
Patch

[Geoffrey Garen]

Comment on attachment 53768 [details]
Patch

r=me

[Simon Fraser (smfr)]

Comment on attachment 53768 [details]
Patch

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

[Simon Fraser (smfr)]

Created attachment 53770 [details]
Patch

Comment on attachment 53770 [details]
Patch

> +inline RenderFrameBase* toRenderFrameBase(RenderObject* object)
> +{
> +    ASSERT(!object || object->isRenderFrameBase());
> +    return static_cast<RenderFrameBase*>(object);
> +}

Typically there’s also a const version of these.

> +
> +// This will catch anyone doing an unnecessary cast.
> +void toRenderPart(const RenderFrameBase*);

This is should be toRenderFrameBase(const RenderFrameBase*)

r=me

[Simon Fraser (smfr)]

Comment on attachment 53770 [details]
Patch

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

[WebKit Review Bot]

http://trac.webkit.org/changeset/57892 might have broken Leopard Intel Debug (Tests)

[Simon Fraser (smfr)]

Comment on attachment 53770 [details]
Patch

This patch was backed out, since it causes an assertion. Frame::ownerRenderer() can be a RenderEmbeddedObject for an <object> containing HTML.

[Alejandro G. Castro]

The patch also caused 4 tests to launch an assertion in the gtk+ bots, the stacks are all like this one:

http://webkit-bots.igalia.com/amd64/svn_57893.core-when_1271781421-_-who_DumpRenderTree-_-why_11.10660.trace.html

[Simon Fraser (smfr)]

Created attachment 53850 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 53850 [details]
Patch

yay!

[Simon Fraser (smfr)]

Comment on attachment 53850 [details]
Patch

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

[Simon Fraser (smfr)]

I think this will do for now.