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
Created attachment 53752 [details] Patch
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 on attachment 53752 [details] Patch Landed in http://trac.webkit.org/changeset/57866 with svn moves, and comments addressed.
Created attachment 53768 [details] Patch
Comment on attachment 53768 [details] Patch r=me
Comment on attachment 53768 [details] Patch http://trac.webkit.org/changeset/57867
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
Comment on attachment 53770 [details] Patch http://trac.webkit.org/changeset/57892
http://trac.webkit.org/changeset/57892 might have broken Leopard Intel Debug (Tests)
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.
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
Created attachment 53850 [details] Patch
Comment on attachment 53850 [details] Patch yay!
Comment on attachment 53850 [details] Patch http://trac.webkit.org/changeset/57900
I think this will do for now.