233972 - deCOMtaminate nsFrameManager

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Brian Ryner (not reading)]

So, there's really no reason to have an abstract interface to the frame manager,
or for it to be refcounted.  Like the style set, it has the same lifetime as the
pres shell.

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

Eliminate nsIFrameManager; create content/shared/public nsFrameManager.h. 
Inlined some getters on nsFrameManager and removed some unused nsresult return
values.  Changed ComputeStyleChangeFor, HasAttributeDependentStyle, and
ReResolveStyleContext to return their result rather than using an out param.

Comment 2

[Brian Ryner (not reading)]

Attachment: diff -w of the patch

Comment 3

[Boris Zbarsky [:bzbarsky]]

Brian, I won't be able to review this any time soon (non-Mozilla stuff is taking
up all my time).

Comment 4

[Brian Ryner (not reading)]

Comment on attachment 141211 [details] [diff] [review]
diff -w of the patch

reassigning review request per boris's comments... roc, do you think you could
take a look?

Comment 5

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

yeah

Comment 6

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

+      nsFrameManager *frameMan = aPO->mParent->mPresShell->FrameManager();
       NS_ASSERTION(frameMan, "No Frame manager!");

Useless assertion. Remove it.

+    nsFrameManager *frameMan = aPO->mParent->mPresShell->FrameManager();
     NS_ASSERTION(frameMan, "No Frame manager!");

Ditto.

+    NS_ASSERTION(!mRootFrame, "aleady have a root frame");

"already"

+  // Notification that we were enable to render a replaced element

"unable"

+  NS_HIDDEN_(nsresult) GetFrameProperty(const nsIFrame* aFrame,
+                                        nsIAtom*        aPropertyName,
+                                        PRUint32        aOptions,
+                                        void**          aPropertyValue);

Seems like most callers don't check the nsresult and just want the value
or null. How about switching the nsresult to an out nsresult* parameter
(which can be null, to ignore the nsresult) (or a PRBool* aPresent) and return
aPropertyValue as the result? (It seems like even the callers that check the
nsresult mostly don't need to distinguish "property not set" and "property set
to null".)

I'm dubious about passing around the nsIPresContext* even in the nsFrameManager
private methods, but I'll let it slide.

+  nsFrameManager*           mFrameManager;  // [OWNS]

Why not declare the nsFrameManager as a direct member?

                        // won't use it, either, but we check it
                        // at the start of every function so that we'll
                        // be OK when nsIPresShell is converted to IDL.

That ain't happening :-). Just delete this comment I think.

   NS_PRECONDITION(aCanvasFrame, "aCanvasFrame argument cannot be null");

Strange ... how did this compile? This should be removed because aCanvasFrame
has gone away.

+VerifyStyleTree(nsIPresContext* aPresContext, FrameManager* aFrameManager)

You mean "nsFrameManager"... This couldn't have compiled either

+        FrameManager *fm1 = aFirstPresContext->FrameManager();

Ditto

+        FrameManager *fm2 = aSecondPresContext->FrameManager();

Ditto

The rest of the patch looks great.

Comment 7

[Brian Ryner (not reading)]

(In reply to comment #6)
> +  NS_HIDDEN_(nsresult) GetFrameProperty(const nsIFrame* aFrame,
> +                                        nsIAtom*        aPropertyName,
> +                                        PRUint32        aOptions,
> +                                        void**          aPropertyValue);
> 
> Seems like most callers don't check the nsresult and just want the value
> or null. How about switching the nsresult to an out nsresult* parameter
> (which can be null, to ignore the nsresult) (or a PRBool* aPresent) and return
> aPropertyValue as the result? (It seems like even the callers that check the
> nsresult mostly don't need to distinguish "property not set" and "property set
> to null".)

I spoke to dbaron about this and he was wary of losing the distinction between
"property not set" and "property set to null".  However, I agree that most
callers don't care.  I'll make the change you suggest.

> +  nsFrameManager*           mFrameManager;  // [OWNS]
> 
> Why not declare the nsFrameManager as a direct member?

You mean as a non-pointer member?  Seems like a good idea, given that the frame
manager doesn't rely on anything in its constructor.  My only concern would be
that it drags in more layout-private stuff when someone #includes
nsIPresShell.h, because I would have to #include nsFrameManager.h

> Strange ... how did this compile? This should be removed because aCanvasFrame
> has gone away.

(to answer this question and the remainder, turns out I didn't do a debug build
of this tree.  I thought I did.)

Comment 8

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

> My only concern would be that it drags in more layout-private stuff when someone
> #includes nsIPresShell.h, because I would have to #include nsFrameManager.h

Hmm. Is that manageable?

Maybe we really need layout to use nsPresShell.h and have the private stuff there

Comment 9

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

Another option which is a bit dirty would be to put the frame manager object
last in the presshell and wrap #ifdef _IMPL_NS_LAYOUT around it and the methods
that access it.

Comment 10

[Brian Ryner (not reading)]

(In reply to comment #8)
> Hmm. Is that manageable?

I'll try it and see what breaks.

> 
> Maybe we really need layout to use nsPresShell.h and have the private stuff there

That would be ideal... I've been wanting to move in that direction for some time
(using concrete class headers and pointer types within layout).

Comment 11

[Brian Ryner (not reading)]

(In reply to comment #10)
> (In reply to comment #8)
> > Hmm. Is that manageable?
> 
> I'll try it and see what breaks.
> 

This puts us into a real #include hell.  The main problem is that
nsFrameManager.h pulls in nsIFrame.h which pullls in nsStyleStruct.h, which has
inline methods that need nsIPresContext to be defined.  But if this whole chain
happened because you #included nsIPresContext.h which #included nsIPresShell.h,
then it's not yet defined.  The same thing happens with nsRuleNode.h.

Comment 12

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

Ick. C++ sucks.

It looks like nsIPresContext.h does not actually need to include nsIPresShell.h.

Comment 13

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

nsFrameManager.h doesn't need to include nsIFrame.h either.

Comment 14

[Brian Ryner (not reading)]

(In reply to comment #12)
> Ick. C++ sucks.
> 
> It looks like nsIPresContext.h does not actually need to include nsIPresShell.h.

Sure it does.  See the inline methods that call GetPresShell()->Foo().

> nsFrameManager.h doesn't need to include nsIFrame.h either.

Yes it does.  AppendFrames() and ReplaceFrame() are inline methods that call
methods on an nsIFrame, so they need the class definition.

Comment 15

[Brian Ryner (not reading)]

Attachment: patch v2

I incorporated all of roc's suggestesions except for making the frame manager a
non-pointer member (due to the above problems).  I also did test that this one
compiles with --enable-debug :)

Comment 16

[Brian Ryner (not reading)]

Attachment: diff -w of patch

Comment 17

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

Comment on attachment 141941 [details] [diff] [review]
diff -w of patch

Great.

I think ultimately I'd like callers of GetFrameProperty to be calling a
deCOMtaminated nsIFrame::GetProperty

void* GetProperty(nsIAtom* aProperty);
void* GetAndRemoveProperty(nsIAtom* aProperty);

That can wait.

Please file a bug about making nsFrameManage a direct member of the presshell.
There's got to be a way :-).

Maybe we can restructure selected includes like this?

// nsFoo.h
#ifndef _nsFoo_h
#define _nsFoo_h
#include "nsFoo_Type.h"
#include "nsBar.h"
void nsFoo::Foo { m->Bar(); }
#endif

// nsFoo_Type.h
#ifndef _nsFoo_Type_h
#define _nsFoo_Type_h
class nsBar;
class nsFoo {
  nsBar* m;
  inline void Foo();
};
#endif

// nsBar.h
#ifndef _nsBar_h
#define _nsBar_h
#include "nsFoo_Type.h"
class nsBar {
  nsFoo m;
  inline void Bar();
};
#include "nsFoo.h"
void nsBar::Bar { m->Foo(); }
#endif

Comment 18

[Brian Ryner (not reading)]

Checked in.  Filed bug 235335 for the pointer vs. direct member issue.

Comment 19

[Boris Zbarsky [:bzbarsky]]

This caused crash bug 236590 

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could somebody remind me why in the world I let you put this header file in the
completely ridiculous location of content/shared/public/ ?

Comment 21

[jag (Peter Annema)]

Comment on attachment 141940 [details] [diff] [review]
patch v2

>Index: layout/mathml/base/src/nsMathMLmtableFrame.cpp
>===================================================================
>+    nsFrameManager *fm = aPresContext->FrameManager();
>+    nsStyleChangeList changeList;
>+    nsChangeHint maxChange = fm->ComputeStyleChangeFor(aCellFrame, &changeList,
>+                                                       NS_STYLE_HINT_NONE);

Minor nit: maxChange is actually unused.