Last Comment Bug 666927 - Cleanup nsBoxFrame::SetLayoutManager
: Cleanup nsBoxFrame::SetLayoutManager
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XP Toolkit/Widgets: XUL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Neil Deakin
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-24 07:23 PDT by Neil Deakin
Modified: 2011-07-04 07:44 PDT (History)
1 user (show)
enndeakin: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (27.69 KB, patch)
2011-06-24 07:23 PDT, Neil Deakin
no flags Details | Diff | Review
patch v2 (31.00 KB, patch)
2011-06-30 09:03 PDT, Neil Deakin
roc: review+
Details | Diff | Review

Description Neil Deakin 2011-06-24 07:23:25 PDT
Created attachment 541674 [details] [diff] [review]
patch

Simple patch to remove some reference counting and use return values rather than outpointers.
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-25 03:11:06 PDT
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 2011-06-30 09:03:57 PDT
Created attachment 543151 [details] [diff] [review]
patch v2
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-30 15:07:27 PDT
Comment on attachment 543151 [details] [diff] [review]
patch v2

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

Note You need to log in before you can comment on or make changes to this bug.