154199 - (prescontext) Combine PresShell and PresContext

Status: NEW

(Core :: Layout, defect)

Description

[Brian Ryner (not reading)]

As we've discussed... these objects basically represent the same thing, and if
we combine them we can avoid virtual function calls to go between the two all
the time.

Comment 1

[Brian Ryner (not reading)]

Attachment: patch, part 1 (.bz2)

Part 1 of 2, compressed with bzip2.

Comment 2

[Brian Ryner (not reading)]

Attachment: Patch, part 2 (.bz2)

Part 2 of 2 (compressed with bzip2).

Comment 3

[Brian Ryner (not reading)]

Much of this is fairly mundane search and replace.  Interesting files to look at:

content/base/src/nsDocumentViewer.cpp
layout/base/public/nsIPresContext.h
layout/base/src/nsPresContext.*

Comment 4

[John Keiser (jkeiser)]

I'm reviewing.

Shouldn't GetShellAt be GetContextAt?

Comment 5

[Brian Ryner (not reading)]

Attachment: updated patch, part 1 (gzipped)

updated to the trunk, incorporated a couple of suggestions from dbaron
(comments in nsIDocument.h, indenting in nsDocumentViewer.cpp). jkeiser, that's
a good change but one that could be done later.

Comment 6

[Brian Ryner (not reading)]

Attachment: updated patch, part 2 (gzipped)

Comment 7

[Brian Ryner (not reading)]

cc'ing hyatt, who hinted that he may be able to help out reviewing this

Comment 8

[John Keiser (jkeiser)]

OK, I have been through the entire patch.  I skimmed everything except
nsPresShell.h and the document stuff.  The document stuff looks OK except the
mPresContextHasShell makes me queasy.  The pres context stuff looks OK too,
though I admit I skimmed some there under the assumption that most stuff was
imported wholesale from PresShell.  This patch simplifies a lot of stuff, should
make us perform better.  r=jkeiser if we can iron out this:

As we talked about briefly in IRC, mPresContextHasShell stuff seems potentially
crash-prone if outsiders access PresContext methods when the shell part has not
been initialized yet.  Maybe it's enough to just put an assertion at the
beginning of every method you moved from PresShell--I saw you put assertions on
some of them.  I think I can live with that personally, though I'd like to hear
dbaron/hyatt/waterson's thoughts.  Is there any chance that the prescontext can
be used before InitShell() is called from the document?

I notice there are new dependencies on necko in some of the Makefile.ins.  Why?

Also, if you have other changes to make too, check your spacing.  There are
places throughout where you add tabs and make a few things mis-spaced.  However,
since you also fix spacing in other places, it all balances out and I'm not
going to hold up this patch for it :)

Comment 9

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

I'm hoping that the idea that you can have a pres context with the "pres shell
part" uninitialized can go away -- I thought that was only put in to satisfy
some unusual consumers (perhaps for printing or print preview).  I'm perfectly
happy, though, if that waits until after this patch.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

We need to drive this into the tree, and work through any issues.  Let's get
some formal reviews on this.

Targeting for 1.2, though I'm not sure yet if we'll take it.  If not we'll have
to find some what around the mlk issue with iframes in 160268

Comment 11

[John Keiser (jkeiser)]

The patch apparently needs to be updated.  I am satisfied and will give r= when
that occurs unless there are more than cosmetic differences (other than changing
callers from presshell -> prescontext).

Comment 12

[Brian Ryner (not reading)]

Attachment: new patch, part 1 (gzipped)

Merged to the trunk, did some work to try to eliminate the
InitShell/DestroyShell calls on PresContext.  It's still not as clean as I'd
like but I thought I'd post my patch here for comments.

Comment 13

[Brian Ryner (not reading)]

Attachment: new patch, part 2 (gzipped)

Comment 14

[John Keiser (jkeiser)]

I have broken it up into three and uploaded them to Patch Viewer for easier viewing:

http://landfill.bugzilla.org/bz176656/attachment.cgi?id=110&action=prettyview -
nsPresContext.cpp and nsPresShell.cpp

http://landfill.bugzilla.org/bz176656/attachment.cgi?id=111&action=prettyview -
other changes 1

http://landfill.bugzilla.org/bz176656/attachment.cgi?id=111&action=prettyview -
other changes 2

Comment 15

[John Keiser (jkeiser)]

Comment on attachment 108567 [details]
new patch, part 1 (gzipped)

All right, r=jkeiser, I see nothing glaring.  I didn't look over it with the
same detail as last time, but enough to see the main differences.  I see the
potential-double-Init() is kinda funky, but as discussed, we can deal with that
in a future bug.  I believe we will need a frozen tree for a day so that all
regressions are traceable to this checkin.

Comment 16

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

We should revive this old project and finish it this time. Here's a plan:

1) Synchronize the lifetimes of nsPresShell and nsPresContext (bug 229080)
2) Make nsPresContext inherit from nsPresShell
3) Embed nsStyleSet as a direct member of nsPresShell
4) Make nsCSSFrameConstructor inherit from nsFrameManager
5) Embed nsFrameManager as a direct member of nsPresShell
6) Incrementally convert references from presshell to prescontext and eliminate unnecessary getters

Comment 17

[Boris Zbarsky [:bzbarsky]]

So the plan would still be that nsIPresShell is the "external" API to this whole shebang, right?

Comment 18

[Boris Zbarsky [:bzbarsky]]

I should note that #3, #4, and #5 might be doable before doing #1/#2 if that's proving annoying.

Comment 19

[Zack Weinberg (:zwol)]

(6) gives me the impression that the end point is with nsPresContext as the primary external interface.

If we inherit nsPresContext from nsPresShell, we still have to take the virtual method call hit for everything that nsPresShell doesn't implement, right?  And likewise with nsCSSFrameConstructor/nsFrameManager.

From an abstract OO perspective, the "inheritance" relation doesn't feel right here -- there's never going to be an alternative subclass, ya?  Is the idea that after we're done incrementally converting references we do collapse the classes together, and this is just a way of avoiding having to convert everything at once?

Comment 20

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

(In reply to comment #17)
> So the plan would still be that nsIPresShell is the "external" API to this
> whole shebang, right?

Yes.

Another part of the plan is that we'd replace the mPresShell members of nsFrameManager and nsCSSFrameConstructor with PresContext() getters that just do an offsetof-based pointer adjustement. These getters could be defined inline in nsPresContext.h (which we'd be wanting to #include instead of nsCSSFrameConstructor.h/nsFrameManager.h). nsIPresShell::PresContext would be similarly declared inline and defined in nsPresContext.h as a static downcast.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> (6) gives me the impression that the end point is with nsPresContext as the
> primary external interface.

I assume roc was talking about internal places where we pass this object around.

Collapsing together once done makes sense to me, for what it's worth.

Comment 22

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

(In reply to comment #19)
> (6) gives me the impression that the end point is with nsPresContext as the
> primary external interface.

No, it would be the primary internal-to-content/layout interface.

> If we inherit nsPresContext from nsPresShell, we still have to take the virtual
> method call hit for everything that nsPresShell doesn't implement, right?  And
> likewise with nsCSSFrameConstructor/nsFrameManager.

Not sure what you mean. Most content and layout can have an nsPresContext*, #include <nsPresContext.h> and do direct nonvirtual calls with it.

> From an abstract OO perspective, the "inheritance" relation doesn't feel right
> here -- there's never going to be an alternative subclass, ya?

Correct. It is a small abuse of inheritance.

> Is the idea that after we're done incrementally converting references we do
> collapse the classes together, and this is just a way of avoiding having to
> convert everything at once?

We probably should collapse away the inherited classes, yeah. That would be step 7 I suppose.

Comment 23

[Jonathan Watt [:jwatt]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> 4) Make nsCSSFrameConstructor inherit from nsFrameManager

There's a patch that does that in bug 714839.