[Webkit-unassigned] [Bug 37741] Clean up RenderPart/RenderPartObject/RenderFrame/RenderEmbeddedObject

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=37741


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #53752|review?                     |review+
               Flag|                            |




--- Comment #2 from Darin Adler <darin at apple.com>  2010-04-19 18:01:27 PST ---
(From update of attachment 53752)
> -// 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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.