328926 - Remove aPresContext parameters from Frame methods

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Marc Liddell]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.1) Gecko/20060124 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.1) Gecko/20060124 Firefox/1.5.0.1

Lots of Frame methods take an aPresContext. This is unnecessary.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Marc Liddell]

Attachment: Remove aPresContext from Frame::Init and fix all callers

Comment 2

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

Your nsListControlFrame.h patch is mixed up in there. If you bring your CVS up to date, then post a new patch, that would be great.

Comment 3

[Marc Liddell]

Attachment: Remove aPresContext from Frame::Init and fix all callers

Oops, sorry about that...

Comment 4

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

I think it would be a good idea to remove aPresContext from DidSetStyleContext right now. Otherwise there's a chance of a slight performance regression from having to call GetPresContext() in every Init (via SetStyleContext).

Comment 5

[Marc Liddell]

Attachment: Remove aPresContext from Frame::Init and fix all callers + DidSetStyleContext

As I do more I'll be able to remove some other GetPresContext()s in Init and other places.

Comment 6

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

+  nsPresContext *aPresContext = GetPresContext();

You shouldn't call local variables 'aPresContext'. I know, it's a pain to rename, but that's our convention. However, since it happens pretty often and matters minimally, I'll let this one slide :-).

+      newSC = GetPresContext()->StyleSet()->

You can't call GetPresContext() before calling superclass's ::Init ... this will crash. Use aContext->GetRuleNode()->GetPresContext() instead.

+  MapAttributesIntoCSS(GetPresContext(), aContent);

Same here (happens twice). Or you could just remove aPresContext from MapAttributesIntoCSS.

+        newStyleContext = GetPresContext()->StyleSet()->

Same here.

-  mPresContext = aPresContext;

You need to also remove mPresContext completely from nsMathMLmactionFrame.

Comment 7

[Marc Liddell]

Attachment: Remove aPresContext from Frame::Init and fix all callers + DidSetStyleContext

Comment 8

[Marc Liddell]

Attachment: Remove aPresContext from Frame::Init and fix all callers + DidSetStyleContext

Keeping it up to date...

Comment 9

[Marc Liddell]

Attachment: Remove aPresContext from Frame::Init and fix all callers + DidSetStyleContext

I noticed another time I called GetPresContext before Init

Comment 10

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

Comment on attachment 214103 [details] [diff] [review]
Remove aPresContext from Frame::Init and fix all callers + DidSetStyleContext

great!

Comment 11

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

BTW we should remove mEventQueueService from nsComboboxControlFrame, use nsContentUtils::EventQueueService instead

Comment 12

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

checked in attachment 214103 [details] [diff] [review]

Comment 13

[Marc Liddell]

Attachment: Destroy and SetInitialChildList

This removes the PresContext params from Destroy and SetInitialChildList and changes the return type of Destroy to void since no-one was checking it.

I also went through and changed the callers and callees and removed aPresContext params from their signatures too.

Comment 14

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

-    frames.DestroyFrames(NS_STATIC_CAST(nsPresContext*, aDtorData));

We probably don't need the dtorData anymore, which is set in SetProperty I think, so you may want to remove it from there.

You should be able to remove aPresContext from mAbsoluteContainer.SetInitialChildList ... get it from aFrame if you need to.

Other than that, it looks great.

After this I think we should take a break from changing nsIFrame for a while. I'm afraid we'll start to interfere with patch merging between trunk to branch. There's plenty of other work to do :-).

One thing I need is renaming of nsDisplayListBuilder::IsForEventDelivery to IsForHandleEvent...

Comment 15

[Marc Liddell]

Attachment: Destroy and SetInitialChildList v2

I'm not sure exactly what you meant by removing the dtorData so I just used nsnull instead of passing aPresContext. I couldn't find a SetProperty which didn't need the extra argument.

SetInitialChildList on nsAbsoluteContainingBlock didn't even use the PresContext so I didn't need to do anything but change the signature.

Comment 16

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

Comment on attachment 214688 [details] [diff] [review]
Destroy and SetInitialChildList v2

Passing null there is right.

As we push this deeper we'll find more and more situations where aPresContxt is unused and we can remove some of the GetPresContext()s we've added.

Comment 17

[Stuart Parmenter]

Comment on attachment 214688 [details] [diff] [review]
Destroy and SetInitialChildList v2

backed out.  this causes crashes while running trender.

Comment 18

[Vidar Haarr (not reading bugmail)]

bug 308463 ... ?

Comment 19

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

ha hmm. Forgot about that one.

Comment 20

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

Attachment: updated patch

This fixes the Trender issue by allowing Bidi directional frames to be safely destroyed.

But we should address the issues raised in bug 308463, summarized in
https://bugzilla.mozilla.org/show_bug.cgi?id=308463#c14

Comment 21

[Bernard Alleysson]

I'm pretty sure I've crashed here today because of this bug:
http://www.microsoft.com/windowsvista/features/foreveryone/sidebar.mspx
(It crashed while displaying the picture)

Comment 22

[Marc Liddell]

Attachment: updated again

Bah. I'm sorry I missed that bug. I guess I must've looked for "aPresContext"...

I tested roc's patch and it passes the whole trender test (which I'll be trying every single one of my patches on from now on) as well the vista page. A few bits didn't apply so I updated it.

Comment 23

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

In bug 308463, bz is worried that we'll destroy frames without a style context set yet and crash. The suggestion is to move the style context out of ::Init and into the constructor, which means callers will pass it when they call new (presShell) nsFooFrame(aStyleContext). In contrast to my comment #15 I think we should pass the style context as an extra parameter to NS_New*Frame.

Can you make this happen in a separate patch before we try landing this patch again?

Comment 24

[Marc Liddell]

Attachment: Updated patch

I attached the wrong patch by mistake.

Comment 25

[Marc Liddell]

Attachment: Remove aPresContext from lots of frame methods and lots of nsCSSFrameConstructor methods

This is an update of the patch since bug 330934 landed.

I didn't include the change to nsDirectionalFrame s since the reason they crashed was fixed by bug 330934. Did you want me to keep the change you added? Without it this patch still passes all of Trender and works fine on the other page mentioned in this bug.

Comment 26

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

Comment on attachment 216365 [details] [diff] [review]
Remove aPresContext from lots of frame methods and lots of nsCSSFrameConstructor methods

No, this is good and we don't need the directionalFrame changes anymore. I'll land this.

Comment 27

[Marc Liddell]

Attachment: Remove aPresContext from lots of frame methods and lots of nsCSSFrameConstructor methods

Keeping it up to date...

Comment 28

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

checked in