[Webkit-unassigned] [Bug 27128] Make Widget Ref-Counted.

[bugzilla-daemon at webkit.org]

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


Dave Hyatt <hyatt at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #32530|review?                     |review-
               Flag|                            |




--- Comment #2 from Dave Hyatt <hyatt at apple.com>  2009-07-09 13:47:56 PDT ---
(From update of attachment 32530)

> -    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.

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