Closed Bug 235335 Opened 20 years ago Closed 20 years ago

nsFrameManager should be a direct member of nsPresShell

Categories

(Core :: Layout, defect, P1)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: bryner, Assigned: roc)

Details

Attachments

(1 file, 2 obsolete files)

There's really no need for this to be an extra allocation.  Unfortunately, as
discussed in bug 233972, there are some hairy #include isseus with this due to
all the inline methods on nsIPresContext.

roc, feel free to take this bug if you want it... I'm not sure I'll have time to
get to this soon.
Okay, sure.

It's the extra dereferences that I want to get rid of, rather than the extra
allocation...
Okay, sure.

It's the extra dereferences that I want to get rid of, rather than the extra
allocation...
Assignee: nobody → roc
Attached patch patch to fix include problems (obsolete) — Splinter Review
This patch sets everything up so we can #include "nsFrameManager.h" from inside
nsIPresShell.h. The patch looks big but it's just moving a lot of stuff around
inside header files, making some inline functions be defined outside their
class, adding dummy struct definitions, and fixing up Makefiles to satisfy
include dependencies.

The general strategy is, when A.h includes B.h only because A.h contains inline
functions that need to see the definitions of types in B.h, we move the include
of B.h from the beginning of A.h to the end, and move the inline functions from
being defined in A.h's class to being defined outside the class after including
B.h. The idea is that if B.h triggers a chain includes that eventually includes
A.h recursively, it's OK because everything A.h needed to define has already
been defined.

In a few places that isn't enough so we use an additional strategy. We take the
definition of class nsFrameManager and pull it out into a separate header file
nsFrameManager_Class.h. Its inline functions are defined separately in
nsFrameManager.h. The idea here is that if you include nsFrameManager_Class.h,
then you are responsible for including nsFrameManager.h eventually. Now
nsIPresShell.h can pull in the definition of nsFrameManager and then define the
nsFrameManager inline methods after it defines class nsIPresShell. We do
something similar for nsRuleNode.

This is a rather large patch for a relatively simple issue, but I think we need
to do something like this. I think eventually we'll want to make nsPresShell a
member of nsPresContext, for example.
Attached patch oops (obsolete) — Splinter Review
This is the right patch.
Attachment #142205 - Attachment is obsolete: true
I guess I'm interested in what people think.
Attached patch better waySplinter Review
I guess people weren't too impressed with that try :-).

Here's a better way. We hoist the data members of nsFrameManager to a new class
nsFrameManagerBase. An nsFrameManagerBase member gets included in
nsIPresShell.h. nsIPresShell::FrameManager casts it down to nsFrameManager* so
we can actually use the nsFrameManager methods on it.

nsFrameManager doesn't appear to have any virtual methods but just to be on the
safe side, I use placement new trickery to ensure that the nsFrameManager
vtable	is installed correctly.

This patch actually works :-).
Attachment #142206 - Attachment is obsolete: true
Attachment #142809 - Flags: superreview?(bryner)
Attachment #142809 - Flags: review?(bryner)
Comment on attachment 142809 [details] [diff] [review]
better way


>-  nsFrameManager* FrameManager() { return mFrameManager; }
>+  nsFrameManager* FrameManager() const {
>+    return NS_REINTERPRET_CAST(nsFrameManager*,
>+      &NS_CONST_CAST(nsIPresShell*, this)->mFrameManager);
>+  }

Drive-by: Shouldn't you use NS_STATIC_CAST instead of NS_REINTERPRET_CAST?

/be
You can't, because at this point nsFrameManager is an incomplete type; we
haven't included nsFrameManager.h so we don't know that nsFrameManager is a
subclass of nsFrameManagerBase. And we can't include nsFrameManager.h to see
that, because avoiding that inclusion is the point of this patch.
Comment on attachment 142809 [details] [diff] [review]
better way

>--- content/shared/public/nsFrameManager.h	24 Feb 2004 00:36:35 -0000	1.2
>+++ content/shared/public/nsFrameManager.h	3 Mar 2004 04:38:00 -0000
>+class nsFrameManager : public nsFrameManagerBase
> {
> public:
>   nsFrameManager() NS_HIDDEN;
>   ~nsFrameManager() NS_HIDDEN;
> 
>+  void* operator new(size_t aSize, nsIPresShell* aHost) {
>+    NS_ASSERTION(aSize == sizeof(nsFrameManager), "Unexpected subclass");
>+    NS_ASSERTION(aSize == sizeof(nsFrameManagerBase),
>+                 "Superclass/subclass mismatch");
>+    return aHost->FrameManager();
>+  }
>+
>   NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW

Will the zeroing operator new actually get called if the object is not
allocated with new() ?	If not, we'll need to zero the memory some other way,
right?	I'm not clear on what's going on here.

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ content/shared/public/nsFrameManagerBase.h	3 Mar 2004 04:38:01 -0000
>+class nsFrameManagerBase
>+{
>+public:
>+  // force a vtable entry
>+  virtual ~nsFrameManagerBase() {}

This isn't necessary at all.  There is no vtable for nsFrameManager.

>--- layout/base/public/nsIPresShell.h	23 Feb 2004 21:29:04 -0000	3.139
>+++ layout/base/public/nsIPresShell.h	3 Mar 2004 04:38:25 -0000
>@@ -159,17 +160,20 @@ public:
> #ifdef _IMPL_NS_LAYOUT
>   nsStyleSet*  StyleSet() { return mStyleSet; }
> 
>   nsCSSFrameConstructor* FrameConstructor()
>   {
>     return mFrameConstructor;
>   }
> 
>-  nsFrameManager* FrameManager() { return mFrameManager; }
>+  nsFrameManager* FrameManager() const {
>+    return NS_REINTERPRET_CAST(nsFrameManager*,
>+      &NS_CONST_CAST(nsIPresShell*, this)->mFrameManager);
>+  }

why is this const_cast needed?
(In reply to comment #9)
> Will the zeroing operator new actually get called if the object is not
> allocated with new() ?	If not, we'll need to zero the memory some other
> way, right?	I'm not clear on what's going on here.

You're right. That ZEROING_OPERATOR_NEW won't get called, and we should remove
it. The frame manager member will still get zeroed, however, because PresShell
has a ZEROING_OPERATOR_NEW.

> This isn't necessary at all.  There is no vtable for nsFrameManager.

I know it's not necessary, but I don't want things to explode violently if
someone decides to add a virtual method to nsFrameManager. I can take it out if
you prefer.

> why is this const_cast needed?

The REINTERPRET_CAST complains if it casts away const. So I have to cast away
const separately.
> > This isn't necessary at all.  There is no vtable for nsFrameManager.
> 
> I know it's not necessary, but I don't want things to explode violently if
> someone decides to add a virtual method to nsFrameManager. I can take it out if
> you prefer.
> 

Yeah, I'd prefer if it was removed.  I went through quite a bit of work to make
nsFrameManager not have virtual functions, so I'll be complaining loudly if
someone adds one.

> The REINTERPRET_CAST complains if it casts away const. So I have to cast away
> const separately.

Ok.
Comment on attachment 142809 [details] [diff] [review]
better way

r+sr=bryner with the zeroing operator new and the virtual dtor removed.
Attachment #142809 - Flags: superreview?(bryner)
Attachment #142809 - Flags: superreview+
Attachment #142809 - Flags: review?(bryner)
Attachment #142809 - Flags: review+
Checked in.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
This has lit the AIX tinderbox. I can't look for a bustage fix right now, so I
reopen.
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Ports/1078572180.17122.gz
"/home/tbox/builds/tinderbox/AIX_5.1_Clobber/mozilla/layout/html/base/src/nsFrameManager.cpp",
line 2176.1: 1540-0062 (S) The incomplete class "PropertyList" must not be used
as a qualifier.
"/home/tbox/builds/tinderbox/AIX_5.1_Clobber/mozilla/layout/html/base/src/nsFrameManager.cpp",
line 1859.36: 1540-0251 (S) The "->" operator cannot be applied to the undefined
class "struct PropertyList".
"/home/tbox/builds/tinderbox/AIX_5.1_Clobber/mozilla/layout/html/base/src/nsFrameManager.cpp",
line 1860.10: 1540-0251 (S) The "->" operator cannot be applied to the undefined
class "struct PropertyList".

and a bunch more of those
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I fixed that bustage some time ago
Status: REOPENED → RESOLVED
Closed: 20 years ago20 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: