Bug 37741 - Clean up RenderPart/RenderPartObject/RenderFrame/RenderEmbeddedObject
Summary: Clean up RenderPart/RenderPartObject/RenderFrame/RenderEmbeddedObject
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 37864
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-16 18:24 PDT by Simon Fraser (smfr)
Modified: 2010-04-20 11:24 PDT (History)
7 users (show)

See Also:


Attachments
Patch (31.19 KB, patch)
2010-04-19 17:54 PDT, Simon Fraser (smfr)
darin: review+
Details | Formatted Diff | Diff
Patch (3.20 KB, patch)
2010-04-19 21:19 PDT, Simon Fraser (smfr)
ggaren: review+
Details | Formatted Diff | Diff
Patch (11.41 KB, patch)
2010-04-19 21:55 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (16.98 KB, patch)
2010-04-20 11:15 PDT, Simon Fraser (smfr)
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2010-04-16 18:24:12 PDT
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
Comment 1 Simon Fraser (smfr) 2010-04-19 17:54:05 PDT
Created attachment 53752 [details]
Patch
Comment 2 Darin Adler 2010-04-19 18:01:27 PDT
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.
Comment 3 Simon Fraser (smfr) 2010-04-19 21:11:10 PDT
Comment on attachment 53752 [details]
Patch

Landed in http://trac.webkit.org/changeset/57866 with svn moves, and comments addressed.
Comment 4 Simon Fraser (smfr) 2010-04-19 21:19:57 PDT
Created attachment 53768 [details]
Patch
Comment 5 Geoffrey Garen 2010-04-19 21:21:16 PDT
Comment on attachment 53768 [details]
Patch

r=me
Comment 6 Simon Fraser (smfr) 2010-04-19 21:23:31 PDT
Comment on attachment 53768 [details]
Patch

http://trac.webkit.org/changeset/57867
Comment 7 Simon Fraser (smfr) 2010-04-19 21:55:19 PDT
Created attachment 53770 [details]
Patch
Comment 8 mitz 2010-04-19 22:03:32 PDT
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
Comment 9 Simon Fraser (smfr) 2010-04-20 08:06:53 PDT
Comment on attachment 53770 [details]
Patch

http://trac.webkit.org/changeset/57892
Comment 10 WebKit Review Bot 2010-04-20 08:56:14 PDT
http://trac.webkit.org/changeset/57892 might have broken Leopard Intel Debug (Tests)
Comment 11 Simon Fraser (smfr) 2010-04-20 10:27:42 PDT
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.
Comment 12 Alejandro G. Castro 2010-04-20 10:51:19 PDT
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
Comment 13 Simon Fraser (smfr) 2010-04-20 11:15:41 PDT
Created attachment 53850 [details]
Patch
Comment 14 Dimitri Glazkov (Google) 2010-04-20 11:18:58 PDT
Comment on attachment 53850 [details]
Patch

yay!
Comment 15 Simon Fraser (smfr) 2010-04-20 11:23:48 PDT
Comment on attachment 53850 [details]
Patch

http://trac.webkit.org/changeset/57900
Comment 16 Simon Fraser (smfr) 2010-04-20 11:24:05 PDT
I think this will do for now.