666927 - Cleanup nsBoxFrame::SetLayoutManager

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

Attachment: patch

Simple patch to remove some reference counting and use return values rather than outpointers.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 541674 [details] [diff] [review]
patch

Review of attachment 541674 [details] [diff] [review]:
-----------------------------------------------------------------

I guess instead of returning already_AddRefed you could create a method nsIBoxLayout::AsGridPart that returns an nsIGridPart* or null. Then you can effectively QI to nsIGridPart without addreffing.

::: layout/generic/nsIFrame.h
@@ +2590,5 @@
>    NS_IMETHOD GetBorder(nsMargin& aBorder)=0;
>    NS_IMETHOD GetPadding(nsMargin& aBorderAndPadding)=0;
>    NS_IMETHOD GetMargin(nsMargin& aMargin)=0;
> +  virtual void SetLayoutManager(nsIBoxLayout* aLayout) { }
> +  virtual nsIBoxLayout* GetLayoutManager() { return NULL; }

nsnull

::: layout/xul/base/src/grid/nsGrid.cpp
@@ +297,5 @@
>         NS_ASSERTION(scrolledFrame,"Error no scroll frame!!");
>         child = do_QueryFrame(scrolledFrame);
>      }
>  
> +    nsCOMPtr<nsIGridPart> monument(do_QueryInterface(child->GetLayoutManager()));

Shouldn't you call nsGrid::GetPartFromBox(child) here and in the other places you're cleaning up?

@@ +594,4 @@
>  {
> +  if (aBox) {
> +    nsCOMPtr<nsIGridPart> part(do_QueryInterface(aBox->GetLayoutManager()));
> +    return part;

since you have to take a reference anyway, and some callers might want to hold a reference, why not return already_AddRefed<nsIGridPart> and part.forget() here?

@@ +599,2 @@
>  
> +  return NULL;

nsnull

::: layout/xul/base/src/grid/nsGrid.h
@@ +113,5 @@
>                            nsGridRow*& aLastRow,
>                            PRBool aIsHorizontal);
>  
>  private:
> +  nsIGridPart* GetPartFromBox(nsIBox* aBox);

This should be static, right?

::: layout/xul/base/src/grid/nsGridRowLayout.cpp
@@ +80,5 @@
>    ChildAddedOrRemoved(aBox, aState);
>  }
>  
> +nsIGridPart*
> +nsGridRowLayout::GetParentGridPart(nsIBox* aBox, nsIBox** aParentBox)

Same here, return already_AddRefed<nsIGridPart>.

Comment 2

[Neil Deakin]

Attachment: patch v2

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 543151 [details] [diff] [review]
patch v2

Review of attachment 543151 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 4

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/3d6a355a8176

Comment 5

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336