Closed Bug 666927 Opened 13 years ago Closed 13 years ago

Cleanup nsBoxFrame::SetLayoutManager

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: enndeakin, Assigned: enndeakin)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
Simple patch to remove some reference counting and use return values rather than outpointers.
Attachment #541674 - Flags: review?(roc)
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>.
Attached patch patch v2Splinter Review
Attachment #541674 - Attachment is obsolete: true
Attachment #543151 - Flags: review?(roc)
Attachment #541674 - Flags: review?(roc)
Comment on attachment 543151 [details] [diff] [review]
patch v2

Review of attachment 543151 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #543151 - Flags: review?(roc) → review+
http://hg.mozilla.org/mozilla-central/rev/3d6a355a8176
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
You need to log in before you can comment on or make changes to this bug.