[webkit-reviews] review denied: [Bug 27128] Make Widget Ref-Counted. : [Attachment 32530] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Beth Dakin <bdakin at apple.com>'s request
for review:
Bug 27128: Make Widget Ref-Counted.
https://bugs.webkit.org/show_bug.cgi?id=27128

Attachment 32530: Patch
https://bugs.webkit.org/attachment.cgi?id=32530&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>

> -    const HashSet<Widget*>* viewChildren = children();
> -    HashSet<Widget*>::const_iterator end = viewChildren->end();
> -    for (HashSet<Widget*>::const_iterator current = viewChildren->begin();
current != end; ++current)
> -	   if ((*current)->isFrameView())
> -	       static_cast<FrameView*>(*current)->layoutIfNeededRecursive();
> +    const HashSet<RefPtr<Widget> >* viewChildren = children();
> +    HashSet<RefPtr<Widget> >::const_iterator end = viewChildren->end();
> +    for (HashSet<RefPtr<Widget> >::const_iterator current =
viewChildren->begin(); current != end; ++current) {
> +	   RefPtr<Widget> widget = (*current);
> +	   if (widget->isFrameView())
> +	      
static_cast<FrameView*>(widget.get())->layoutIfNeededRecursive();
> +    }
>  

I don't think the change to use a temporary variable here is helpful, since
you've created refcounting churn.  I think you should put it back the way it
was and just use |current|.


> Index: WebCore/rendering/RenderApplet.cpp
> ===================================================================
> --- WebCore/rendering/RenderApplet.cpp	(revision 45662)
> +++ WebCore/rendering/RenderApplet.cpp	(working copy)
> @@ -26,6 +26,7 @@
>  #include "HTMLAppletElement.h"
>  #include "HTMLNames.h"
>  #include "HTMLParamElement.h"
> +#include "Widget.h"
>  
>  namespace WebCore {
>  
> @@ -71,7 +72,8 @@ void RenderApplet::createWidgetIfNecessa
>  
>      Frame* frame = document()->frame();
>      ASSERT(frame);
> -    setWidget(frame->loader()->createJavaAppletWidget(IntSize(contentWidth,
contentHeight), element, m_args));
> +    RefPtr<Widget> widget =
frame->loader()->createJavaAppletWidget(IntSize(contentWidth, contentHeight),
element, m_args);
> +    setWidget(widget.get());
>  }
>  

I don't see any reason to use a temporary variable here?  You're at the end of
the function, so can't you just say:

setWidget(frame->loader()->createJavaAppletWidget(...));

?

Other comments:

Shouldn't setWidget take a PassRefPtr now, since RenderWidgets will be taking
ownership of the widget?

It seems like addChild should arguably take a PassRefPtr too, since the parent
will also be taking ownership of the added child.

I take back my objections to this patch.   The fact that Scrollbars and
FrameViews were already refcounted anyway makes this a really great cleanup.

Let me see one more round.  Thanks.