270708 - Invalid comment in nsIFrame.h

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Philip K. Warren]

From nsIFrame.h:

819   /**
820    * This call is invoked when the value of a content objects's attribute
821    * is changed. 
822    * The first frame that maps that content is asked to deal
823    * with the change by doing whatever is appropriate.
824    *
825    * @param aChild the content object
826    * @param aAttribute the attribute whose value changed
827    * @param aHint the level of change that has already been dealt with
828    */
829   NS_IMETHOD  AttributeChanged(nsPresContext* aPresContext,
830                                nsIContent*     aChild,
831                                PRInt32         aNameSpaceID,
832                                nsIAtom*        aAttribute,
833                                PRInt32         aModType) = 0;

There is no aHint param, and several of the parameters are not documented.
According to bz, this function should be documented similarly to the
AttributeChanged function in nsIDocumentObserver.h.

Comment 1

[Boris Zbarsky [:bzbarsky]]

And as long as we're here we should remove aPresContext, since frames can get
their prescontext just fine via GetPresContext().

Comment 2

[Vidar Haarr (not reading bugmail)]

Attachment: removes nsPresContext from AttributeChanged and fixes comment in nsIFrame.h

How does this look?

bz: Did you mean to remove aPresContext wherever it is not really needed? I see
lots of methods taking nsPresContext* as parameter where they could just use
GetPresContext(). If so, I can search/replace more :)

Comment 3

[Boris Zbarsky [:bzbarsky]]

> bz: Did you mean to remove aPresContext wherever it is not really needed? 

In general, yes.  As part of this bug, probably not.  ;)

If you have time to clean up nsIFrame methods that take an nsIPresShell/nsIPresContext 
but don't really need it, that would be great!  File a bug on that, cc rbs, roc, dbaron, 
myself, bryner?

Comment 4

[Philip K. Warren]

Comment on attachment 169334 [details] [diff] [review]
removes nsPresContext from AttributeChanged and fixes comment in nsIFrame.h

>Index: generic/nsIFrame.h
>===================================================================
...
>+   * @param aModType Whether or not the attribute was added, changed, or removed.

Can you add the additional comment from nsIDocumentObserver.h in here? ("The
constants are defined in nsIDOMMutationEvent.h.")

>Index: svg/base/src/nsSVGTextFrame.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGTextFrame.cpp,v
>retrieving revision 1.9
>diff -w -u -p -7 -r1.9 nsSVGTextFrame.cpp
>--- svg/base/src/nsSVGTextFrame.cpp	5 Dec 2004 22:12:43 -0000	1.9
>+++ svg/base/src/nsSVGTextFrame.cpp	22 Dec 2004 00:36:16 -0000
>@@ -110,16 +110,15 @@ public:
>   
>   NS_IMETHOD Init(nsPresContext*  aPresContext,
>                   nsIContent*      aContent,
>                   nsIFrame*        aParent,
>                   nsStyleContext*  aContext,
>                   nsIFrame*        aPrevInFlow);
>@@ -273,32 +272,30 @@ NS_INTERFACE_MAP_BEGIN(nsSVGTextFrame)
> //----------------------------------------------------------------------
> // nsIFrame methods
> NS_IMETHODIMP
>-nsSVGTextFrame::Init(nsPresContext*  aPresContext,
>-                     nsIContent*      aContent,
>+nsSVGTextFrame::Init(nsIContent*      aContent,
>                      nsIFrame*        aParent,
>                      nsStyleContext*  aContext,
>                      nsIFrame*        aPrevInFlow)
> {
>   nsresult rv;
>-  rv = nsSVGTextFrameBase::Init(aPresContext, aContent, aParent,
>+  rv = nsSVGTextFrameBase::Init(GetPresContext(), aContent, aParent,
>                                 aContext, aPrevInFlow);
> 
>   Init();
>   
>   return rv;
> }

Did you mean to include this change in with the changes for AttributeChanged?
If so, you need to change the declaration in the header file above.

Everything else looks ok, but you should probably get someone else with more
layout experience to look it over.

r=pkw with those two changes (backing out change to Init and adding additional
comment in nsIFrame.h).

Comment 5

[Vidar Haarr (not reading bugmail)]

Comment on attachment 169334 [details] [diff] [review]
removes nsPresContext from AttributeChanged and fixes comment in nsIFrame.h

I've added the comment to nsIFrame.h and reverted my changes to
nsSVGTextFrame.cpp. I'll attach an updated patch after sr comments. Thanks for
the review, PKW!

Why did this build? I haven't enabled SVG - is that why?

bz: I'll open a new bug once this is checked in :)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Actually, is there a reason to keep the aContent argument?  It'll always be the mContent 
of the frame, I would hope....

Comment 7

[Vidar Haarr (not reading bugmail)]

I guess that if it isn't the mContent, something is wrong, or someone is
invoking methods out of context. I see lots of methods that check if (mContent)
{}, so I presume it has been/is null at some point.
In any case, I believe that should be a seperate bug - lets take a look at it
after aPresContext is gone.

bz: it seems bug 244581 covers some of the remaining aPresContexts. I'll do that
after this bug, and then open a new one if there are even more of them remaining.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I guess that if it isn't the mContent, something is wrong,

Yes.

>  I see lots of methods that check if (mContent) {}, so I presume it has been/is null at
> some point.

It's null in very rare cases (viewport and page frames).  It's non-null otherwise.

> In any case, I believe that should be a seperate bug - lets take a look at it
> after aPresContext is gone.

OK.  I'll treview this patch with that in mind.  Probably not happening till at least Tues 
night, though....

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 169334 [details] [diff] [review]
removes nsPresContext from AttributeChanged and fixes comment in nsIFrame.h

>Index: forms/nsFileControlFrame.cpp
>+nsFileControlFrame::AttributeChanged(nsIContent*     aChild,
>                                        PRInt32         aNameSpaceID,

I assume you fixed the indents in your tree, here and elsewhere?  It helps to
post a real patch in addition to the -w one....

>Index: generic/nsBlockFrame.cpp

>   if (nsHTMLAtoms::start == aAttribute) {

On this codepath you will end up calling GetPresContext() twice.  Call it once
and store the return value in a local nsIPresContext pointer here?

>       // Tell the enclosing block frame to renumber list items within
>       // itself

Same here.

>Index: mathml/base/src/nsMathMLmfracFrame.cpp

>+          mSlashChar->SetData(GetPresContext(), slashChar);
>+          ResolveMathMLCharStyle(GetPresContext(), mContent, mStyleContext, mSlashChar, PR_TRUE);

Same here.

>Index: svg/base/src/nsSVGTextFrame.cpp

>-nsSVGTextFrame::Init(nsPresContext*  aPresContext,
>-                     nsIContent*      aContent,
>+nsSVGTextFrame::Init(nsIContent*      aContent,

This won't compile, because you didn't change the prototype (why're you
changing Init() anyway?).

Please do build with all optional layout stuff enabled for treewide patches
like this...

>Index: xul/base/src/nsListBoxBodyFrame.cpp
>-      t2p = aPresContext->TwipsToPixels();
>+      t2p = GetPresContext()->TwipsToPixels();

Again, please cache the prescontext pointer here?

>Index: xul/base/src/nsTextBoxFrame.cpp
>+    UpdateAttributes(GetPresContext(), aAttribute, aResize, aRedraw);

And here.

The rest looks good.... please fix those issues, though.

Comment 10

[Vidar Haarr (not reading bugmail)]

Attachment: updated to review in comment #9 (diff -wup7)

"real patch" without -w will be attached shortly.

bz: I changed nsSVGTextFrame::Init because it was search/replace, and I didn't
catch it when I ran through manually or when I compiled (since I hadn't enabled
svg).

Comment 11

[Vidar Haarr (not reading bugmail)]

Attachment: patch without -w

without -w, for your pleasure

Comment 12

[Boris Zbarsky [:bzbarsky]]

Names starting with "a" are used for function arguments in Mozilla.... please
just name the locals "presContext".

I wouldn't bother with attaching the -w patch in this case -- it's not any
easier to review, and the without -w one is needed anyway for checkin.

Comment 13

[Vidar Haarr (not reading bugmail)]

Attachment: aPresContext -> presContext

Yes, I realized too late that adding both patches was just silly :)

If this passes sr, someone can just check it in at their leisure; I don't have
CVS access. You can credit me by full name and e-mail if you want to.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 169914 [details] [diff] [review]
aPresContext -> presContext

sr=bzbarsky

Comment 15

[Ian Neal]

Comment on attachment 169914 [details] [diff] [review]
aPresContext -> presContext

Checked in

Comment 16

[Vidar Haarr (not reading bugmail)]

Ian: Thanks!

bz: Care to file a new bug about aContent/mContent with some details? Feel free
to CC me, and I'll look into providing a patch!

I'll continue with bug 244581 now, and look into removing more nsPresContext
references after that one.

Checked in by Ian -> FIXED.

Comment 17

[Vidar Haarr (not reading bugmail)]

Attachment: bustage fix for mathml (checked in)

I didn't compile mathml earlier, even though bz told me to - I just forgot
about it, sorry.