307419 - Remove mPresContext in /layout/xul/

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Vidar Haarr (not reading bugmail)]

In the spirit of bug 301313 and bug 264453, I removed some mPresContext
variables in layout/xul.

Patch coming.

Comment 1

[Vidar Haarr (not reading bugmail)]

Attachment: version 0.1

First stab at layout/xul mPresContext removal.
I'm not even sure we want to do this - ref. dbarons bug 264453 comment 2 - but
I was bored, so I just patched it.

Comment 2

[Boris Zbarsky [:bzbarsky]]

The difference is that frames are not stack-allocated -- they're always around.

I'm not going to be able to get to this patch for a bit, though.

Comment 3

[Vidar Haarr (not reading bugmail)]

Ah, right.
Well, this isn't exactly an urgent patch, so take your time :-)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 195180 [details] [diff] [review]
version 0.1

>Index: xul/base/src/nsBoxFrame.cpp
> nsBoxFrame::FireDOMEvent(const nsAString& aDOMEventName, nsIContent *aContent)
>+  if (content && GetPresContext()) {

Please store GetPresContext() in a local here -- it's used 3 times in a row.

>Index: xul/base/src/nsListBoxBodyFrame.cpp
> nsListBoxBodyFrame::InternalPositionChanged(PRBool aUp, PRInt32 aDelta)

Same in this function -- either cache it, or get it off the state (with state.PresContext()).

>Index: xul/base/src/nsMenuPopupFrame.cpp
>@@ -252,23 +249,23 @@ nsMenuPopupFrame::MarkStyleChange(nsBoxL
>+    nsIPopupSetFrame* popupSet = GetPopupSetFrame(GetPresContext());
....
>+        nsBoxLayoutState state(GetPresContext());

Local variable, please.

>@@ -303,23 +300,23 @@ nsMenuPopupFrame::MarkDirty(nsBoxLayoutS

Same here.

>@@ -344,23 +341,23 @@ nsMenuPopupFrame::RelayoutDirtyChild(nsB

And here.

>Index: xul/base/src/nsPopupSetFrame.cpp
>@@ -748,17 +746,17 @@ nsPopupSetFrame::RemovePopupFrame(nsIFra

And here -- put the temporary outside the loop!

>Index: xul/base/src/nsSplitterFrame.cpp
> nsSplitterFrameInner::MouseDown(nsIDOMEvent* aMouseEvent)

Again, local, please.

>Index: xul/base/src/tree/src/nsTreeBodyFrame.cpp
> nsTreeBodyFrame::EnsureScrollable(PRBool ensureboth)

Local here, please.

>@@ -826,30 +825,30 @@ nsTreeBodyFrame::CheckOverflow()

And here.

>nsTreeBodyFrame::AdjustClientCoordsToBoxCoordSpace(PRInt32 aX, PRInt32 aY,

And in this method.

>@@ -1907,50 +1906,50 @@ PRInt32 nsTreeBodyFrame::GetRowHeight()

And here.

>@@ -3228,27 +3227,27 @@ nsTreeBodyFrame::PaintDropFeedback(const

And here.

>@@ -3447,27 +3446,27 @@ nsTreeBodyFrame::ScrollInternal(PRInt32 

And here.

> nsTreeBodyFrame::ScrollHorzInternal(PRInt32 aPosition)

And here.

>@@ -3560,44 +3559,44 @@ nsTreeBodyFrame::PositionChanged(nsISupp

And here.

Comment 5

[Vidar Haarr (not reading bugmail)]

Attachment: version 0.2

Thanks for pointing out all those silly mistakes. I should've done those before submitting the initial patch.

I think I got them all now.

Comment 6

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

Why does listboxbodyframe hold a pointer to the frame constructor? That seems mad. Maybe file a followup bug.

Comment 7

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

Comment on attachment 205864 [details] [diff] [review]
version 0.2

much better! I'll check in.

Comment 8

[Vidar Haarr (not reading bugmail)]

(In reply to comment #6)
> Why does listboxbodyframe hold a pointer to the frame constructor? That seems
> mad. Maybe file a followup bug.

Yes, that is weird. I'll make it get a nsCSSFrameConstructor from the PresShell instead in bug 320337.

Comment 9

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

checked in.

Comment 10

[Vidar Haarr (not reading bugmail)]

-> FIXED.

Thanks for the reviews and checkin.