Summary: | Clean up RenderPart/RenderPartObject/RenderFrame/RenderEmbeddedObject | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, alex, cmarrin, eric, kenneth, tonikitoo, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Bug Depends on: | 37864 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Simon Fraser (smfr)
2010-04-16 18:24:12 PDT
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. |