[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