[webkit-reviews] review granted: [Bug 37741] Clean up RenderPart/RenderPartObject/RenderFrame/RenderEmbeddedObject : [Attachment 53752] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Apr 19 18:01:27 PDT 2010
Darin Adler <darin at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 37741: Clean up
RenderPart/RenderPartObject/RenderFrame/RenderEmbeddedObject
https://bugs.webkit.org/show_bug.cgi?id=37741
Attachment 53752: Patch
https://bugs.webkit.org/attachment.cgi?id=53752&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
> -// 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.
More information about the webkit-reviews
mailing list