253889 - deCOMtaminate nsIPresShell

Status: RESOLVED FIXED

(Core :: Layout, task)

Description

[Brian Ryner (not reading)]

tracking bug for nsIPresShell deCOMtamination

Comment 1

[Brian Ryner (not reading)]

Attachment: remove non-inlined nsIPresShell::GetDocument()

This converts all callers to use the inlined version of
nsIPresShell::GetDocument() and removes the virtual, addref'ing version.

Comment 2

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

Comment on attachment 154903 [details] [diff] [review]
remove non-inlined nsIPresShell::GetDocument()

+++ accessible/src/base/nsCaretAccessible.cpp	1 Aug 2004 04:03:18 -0000
@@ -95,8 +95,7 @@ NS_IMETHODIMP nsCaretAccessible::AttachN
   if (!presShell)
     return NS_ERROR_FAILURE;

-  nsCOMPtr<nsIDocument> doc;
-  presShell->GetDocument(getter_AddRefs(doc));
+  nsCOMPtr<nsIDocument> doc = presShell->GetDocument();

Can't you make this weak?

@@ -863,7 +860,7 @@ nsPresContext::SetImageAnimationModeInte
   // on all the images
   nsCOMPtr<nsIDocument> doc;

You forgot to remove this declaration of doc

@@ -226,12 +225,13 @@ nsMenuBarFrame::SetActive(PRBool aActive
       break;

     nsCOMPtr<nsIDOMDocument> domDoc;
+    nsCOMPtr<nsIDocument> focusedDoc;

You can make this weak too, can't you?

Comment 3

[Brian Ryner (not reading)]

> +++ accessible/src/base/nsCaretAccessible.cpp	1 Aug 2004 04:03:18 -0000
> @@ -95,8 +95,7 @@ NS_IMETHODIMP nsCaretAccessible::AttachN
>    if (!presShell)
>      return NS_ERROR_FAILURE;
> 
> -  nsCOMPtr<nsIDocument> doc;
> -  presShell->GetDocument(getter_AddRefs(doc));
> +  nsCOMPtr<nsIDocument> doc = presShell->GetDocument();
> 
> Can't you make this weak?

I left it a COMPtr for compactness; note that it's conditionally used to hold
the result of a do_QueryInterface().

> @@ -863,7 +860,7 @@ nsPresContext::SetImageAnimationModeInte
>    // on all the images
>    nsCOMPtr<nsIDocument> doc;
> 
> You forgot to remove this declaration of doc
> 

Fixed.

> @@ -226,12 +225,13 @@ nsMenuBarFrame::SetActive(PRBool aActive
>        break;
> 
>      nsCOMPtr<nsIDOMDocument> domDoc;
> +    nsCOMPtr<nsIDocument> focusedDoc;
> 
> You can make this weak too, can't you?
> 

No, this holds the result of a do_QueryInterface().

Comment 4

[Brian Ryner (not reading)]

Attachment: remove non-inlined nsIPresShell::GetPresContext()

I kept strong references here unless I knew for sure that the call would not
dispatch any sort of event (since event handlers can do all sorts of teardown).
 That includes around any calls to nsIContent::HandleDOMEvent,
nsIEventStateManager::DispatchNewEvent, or nsIContent::SetFocus.

Comment 5

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

Comment on attachment 155747 [details] [diff] [review]
remove non-inlined nsIPresShell::GetPresContext()

yum

Comment 6

[Brian Ryner (not reading)]

Attachment: next round of changes

Made the following methods nonvirtual, with external virtual implementations
where needed:

Get/SetAuthorStyleDisabled
ReconstructStyleData

inlined GetFrameSelection and renamed to FrameSelection()  (failure to create
the FrameSelection causes Init() to fail and this is sufficient for callers to
assume that it will never be null)

removed noninlined GetViewManager

Removed EnablePrefStyleRules and ArePrefStyleRulesEnabled -- no one uses these

Comment 7

[Brian Ryner (not reading)]

Attachment: fix up nsIPresShell::GetRootFrame

Got rid of GetRootFrame's useless nsresult return, changed callers within
layout to use the frame manager so that the whole thing is inlined.

Comment 8

[Brian Ryner (not reading)]

Attachment: nsIPresShell::GetPrimaryFrameFor()

Same deal as GetRootFrame, except there are a ton more callers.

Comment 9

[Brian Ryner (not reading)]

Comment on attachment 157749 [details] [diff] [review]
nsIPresShell::GetPrimaryFrameFor()

brendan offered to help out here, so I'll give roc a break.

Comment 10

[Brian Ryner (not reading)]

brendan pointed out that it would be cleaner if we didnt need to explicitly use
FrameManager() when calling from within layout.  The obvious solution of
inlining GetPrimaryFrameFor is currently not possible because it would create a
circular #include cycle where several classes all define inline methods that
call each other.

So, there are a couple of alternate solutions:
1. use a macro instead of an inline, and depend on the callers to also #include
nsFrameManager.h before using it.
2. restructure the #includes such that it can be an inline method

The second option turns out to be pretty tricky.  However, I may have come up
with a way to fix it.  The basic idea is to #include the full definitions of all
of the classes involved, then pull in their inline methods (outside of the class
definition).  To accomplish this, I made it so #including any header in this
circular dependency group falls out to a master header which does the
appropriate #define magic to pull in only what is wanted from each header.

dbaron thinks there may be problems with referencing an inline method from
another inline method before the first one is defined, where it would emit an
out-of-line function call.  I can't find any evidence that this ever happens,
with gcc anyway.

Anyway, I'll post a patch for what I've got done to fix up the #includes.  If it
looks ok, I can trivially change the GetPrimaryFrameFor patch to inline it for
callers inside of gklayout.

Comment 11

[Brian Ryner (not reading)]

Attachment: possible fix for inlining problems

Comment 12

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

> dbaron thinks there may be problems with referencing an inline method from
> another inline method before the first one is defined, where it would emit an
> out-of-line function call.  I can't find any evidence that this ever happens,
> with gcc anyway.

I also thought this could happen. gcc has this new unit-at-a-time mode where it
processes the entire compilation unit before emitting any code, so you may only
see problems with older gccs.

Comment 13

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

But likewise I can't seem to make it happen.

Comment 14

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

I think the basic idea is good, but we'll have to see what various compilers do
with it.

Comment 15

[Brian Ryner (not reading)]

I tested this on what I could get my hands on, which is gcc 3.3.2, msvc6, and
HPUX aCC, none of which had a problem with it.

Comment 16

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

I think this is a fine idea, but dbaron and I don't both need to review it.
Choose one of us :-)

Comment 17

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

Remind me why we can't just move all necessary code for GetPrimaryFrameFor into
nsPresContext?

Comment 18

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

Comment on attachment 157948 [details] [diff] [review]
possible fix for inlining problems

waiting for an answer to the question in my last comment

Comment 19

[Brian Ryner (not reading)]

We could try.  It would change the FrameManager's lifetime so that it is tied to
the PresContext instead of the PresShell.  I think the PresShell is normally
destroyed first (should double-check), so I don't think that will cause any
problems with it going away too early...

Comment 20

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

Comment on attachment 157948 [details] [diff] [review]
possible fix for inlining problems

When declaring an inline member function that's defined later, please use
|inline| so it's clear what's going on.  I'm not sure if it actually makes a
difference on any compilers, but it might.

Sorry for the delay in getting to this.

r=dbaron, or r+sr if you want

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 157749 [details] [diff] [review]
nsIPresShell::GetPrimaryFrameFor()

Bryner said he wasn't sure what he wants to do with this patch, so removing
from my queue for now.

/be

Comment 22

[Bastiaan Jacques]

Comment on attachment 157749 [details] [diff] [review]
nsIPresShell::GetPrimaryFrameFor()

I guess the same was done in bug 303779. :(

Comment 23

[dan]

go **** ya self

Comment 24

[Zack Weinberg (:zwol)]

I'm not promising to do this, but I might, so I'm assigning to myself so I don't lose it.

Comment 25

[Craig Topper]

Attachment: DeCOMtamination of more methods

This includes deCOMtamination of various methods. I can split this patch up if that would be easier to review.

More to come.

Comment 26

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

Comment on attachment 433701 [details] [diff] [review]
DeCOMtamination of more methods

Welcome back, Craig! Thanks!!!

Comment 27

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

Oh, you need to rev the nsIPresShell IID.

Comment 28

[Craig Topper]

Attachment: Changed IID and updated to tip

Comment 29

[Craig Topper]

Assigning to myself.

Comment 30

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/333a791d201e

Comment 31

[Craig Topper]

Attachment: Add const to a couple methods

Made GetPresContext, GetViewManager, and IsSafeToFlush const.

Comment 32

[Craig Topper]

Attachment: GetCaret()

Changed GetCaret to return already_AddRefed<nsCaret>.

Comment 33

[Craig Topper]

Attachment: Replace NS_IMETHOD with nsresult or void on routines with no outside impact

Bunch of simple conversion of NS_IMETHOD to nsresult or void(if always returned NS_OK). No impact to other files because all outside callers were already ignoring the return value on the ones that became void.

Comment 34

[Craig Topper]

Attachment: Replace NS_IMETHOD with nsresult or void on routines with no outside impact

Rebased from GetCaret() patch to avoid a conflict.

Bunch of simple conversion of NS_IMETHOD to nsresult or void(if always returned
NS_OK). No impact to other files because all outside callers were already
ignoring the return value on the ones that became void.

Comment 35

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

Most of the time, there's probably not much point making methods 'const'. I wouldn't bother unless there's a clear need.

Comment 36

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

Comment on attachment 433830 [details] [diff] [review]
Replace NS_IMETHOD with nsresult or void on routines with no outside impact

I'm not sure if having NS_HIDDEN on virtual methods actually makes a difference. I guess it doesn't hurt.

Comment 37

[Craig Topper]

(In reply to comment #36)
> (From update of attachment 433830 [details] [diff] [review])
> I'm not sure if having NS_HIDDEN on virtual methods actually makes a
> difference. I guess it doesn't hurt.

I just did it cause some of the other patches in this bug were doing it. I didn't really know what the right thing to do was.

Comment 38

[Craig Topper]

Attachment: GetEventTargetFrame() and GetEventTargetContent()

GetEventTargetContent was changed to return an already_AddRefed<nsIContent>, but the result needs to be put in an outparam in one place. So I used get() on the already_AddRefed result. Is this the right way to do this?

*aContent = presShell->GetEventTargetContent(aEvent).get();

Comment 39

[Craig Topper]

Attachment: IsReflowLocked()

Comment 40

[Craig Topper]

Attachment: IsPaintingSuppressed()

Comment 41

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

Comment on attachment 433847 [details] [diff] [review]
GetEventTargetFrame() and GetEventTargetContent()

this is right

Comment 42

[Craig Topper]

Attachment: New IID to go with my previous set of patches

Comment 43

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Checked in attachments 433824, 433825, 433830, 433847, 433851, 433862, and 433864 as http://hg.mozilla.org/mozilla-central/rev/4b8936ac4a31

Comment 44

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Also pushed http://hg.mozilla.org/mozilla-central/rev/90a30ea68797 to fix windows bustage and clean up a comment.

Comment 45

[Craig Topper]

Attachment: Moved some variables from PresShell to nsIPresShell and inlined their getters

Comment 46

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

Comment on attachment 435518 [details] [diff] [review]
Moved some variables from PresShell to nsIPresShell and inlined their getters

Will need an IID rev.

Comment 47

[Craig Topper]

Attachment: Removed NS_IMETHOD macros and fixed returns types on several methods

Comment 48

[Craig Topper]

Attachment: New IID, removed QI form NS_NewPresShell, and added const on several routines

Here's the IID change to go with the other patches as well as some other cleanup.

Comment 49

[Craig Topper]

Attachment: Fixed some calls to nsIPresShell::GetRootFrame() to call nsIPresShell::FrameManager()->GetRootFrame() to avoid the virtual call

Some of the calls in this file were already doing this and I happened to see a few that weren't.

Comment 50

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

Maybe we could get rid of the need for this using #ifdef _IMPL_NS_LAYOUT the way we do for some stuff in nsIFrame?

Comment 51

[Craig Topper]

You mean something like making nsIPresShell::GetRootFrame() behave differently inside and outside gklayout like the Internal/External stuff already being done for some things?

Comment 52

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

Yes.

Comment 53

[Craig Topper]

I think this would require nsFrameManager.h to be included in nsIPresShell.h which seems to have severe import ordering issues that already being hacked around.

Comment 54

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

Hmm. Maybe we should move mRootFrame into nsIPresShell?

Comment 55

[Craig Topper]

Attachment: Made nsIPresShell::GetRootFrame() a non-virtual call inside gklayout

Moved GetRootFrame up to FrameManagerBase to fix an include problem. Then made nsIPresShell::GetRootFrame an inlined call to FrameManagerBase::GetRootFrame(itself inlined) inside of gklayout and preserved the virtual call outside of gklayout.

This removes the need for nsIPressShell::FrameManager()->GetRootFrame() inside gklayout. Will write a future patch to go back to clean up everywhere this is currently done.

Comment 56

[Craig Topper]

Attachment: HandleEventWithTarget and HandleDOMEventWithTarget

Made these 3 methods return the failure code from the functions they use instead of just returning NS_OK.

Comment 57

[Alfred Kayser]

For this last patch: rv may be returned uninitialized, so give a default value.

Comment 58

[Craig Topper]

Attachment: HandleEventWithTarget and HandleDOMEventWithTarget fixed to init return value NS_OK

Thanks for catching that.

Comment 59

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

Comment on attachment 435543 [details] [diff] [review]
HandleEventWithTarget and HandleDOMEventWithTarget fixed to init return value NS_OK

+  nsresult rv = NS_OK;
 
   PushCurrentEventInfo(aFrame, aContent);
-  ret = HandleEventInternal(aEvent, nsnull, aStatus);
+  rv = HandleEventInternal(aEvent, nsnull, aStatus);
   PopCurrentEventInfo();
-  return NS_OK;
+  return rv;

It's not used uninitialized here.

Just changing NS_IMETHOD to virtual nsresult isn't much of a win, AFAIK.

Comment 60

[Craig Topper]

The removal of NS_IMETHOD probably only results in the removal of _stdcall on windows. But mainly I was just removing them to keep track of what functions were left to DeCOM.

Comment 61

[Craig Topper]

Attachment: Removed unnecessary initialization in HandleEventWithTarget as roc pointed out

Comment 62

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

We need to back out 4b8936ac4a31 to fix VC9 builds (bug 555727)

Comment 63

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #62)
> We need to back out 4b8936ac4a31 to fix VC9 builds (bug 555727)

Done: http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=9ba741f58e4f

Comment 64

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Relanded along with the newer patches in this bug.  I landed them separately this time to make bizarre regression hunting easier.

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=e4f8663e3257

Comment 65

[Craig Topper]

Attachment: Inlined a few more routines, removed unnecessary forward declarations

This inlines two more trivial routines by moving them from PresShell to nsIPresShell.

Removes a bunch of forward declarations from nsIPresShell.h that aren't used. My Mac didn't reveal any dependencies in other files on these forward declarations, but it's possible some other platform could have a dependency here.

Also cleaned up a block nsPresContext that I seem to have accidentally left when I deCOMed IsReflowLocked.

Comment 66

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

According to IRC dicussion, we shouldn't add NS_HIDDEN anywhere because it doesn't make any difference to libxul builds (which are the only builds we ship).

Comment 67

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/f219b151fc4a

Comment 68

[Craig Topper]

Attachment: Fix permissions on several methods and reorder member variables by type/size

Made the Internal/External methods protected instead of public. Converted protected in PresShell to private since there are no derived classes.

Also reordered some members in nsIPresShell so that variables of like type/size are together. Theoretically making the class ever so slightly smaller. Or more readable since there's no longer a method declaration in the middle of the members.

Comment 69

[Craig Topper]

Attachment: Moved more methods to nsIPresShell and de-virtualize as many as possible

I de-virtualized as many as I could, but some are referenced outside of layout. Could go back do the Internal/External trick if there is a desire.

I built this on my Macbook with disable-libxul and enable-accessibility to hopefully catch all the methods that need to stay virtual. Any other build flags I need to be safe?

Comment 70

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

Comment on attachment 469790 [details] [diff] [review]
Moved more methods to nsIPresShell and de-virtualize as many as possible

Need to rev IID again

Comment 71

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Craig, anything left to land here?

Comment 72

[Craig Topper]

I don't think all of these patches landed, but they've probably bitrotted by now. I'll get them cleaned up.

Comment 73

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 253889 - part 1: Get rid of nsIPresShell from gfx

gfxUtils::WriteAsPNG() is the only user of nsIPresShell in gfx, but
nobody calls it. So, we can get rid of it with nsIPresShell reference.

Comment 74

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 253889 - part 2: Move all methods and public structs of nsIPresShell into mozilla::PresShell

(Sorry for big patch. It requires me to take long time if I try to move method separately due to dependencies each other.)

Comment 75

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 253889 - part 3: Move all remaining members of nsIPresShell to mozilla::PresShell

Additionally, this sorts out the order of member variables for minimizing the
instance size.

And also this changes enum RenderFlags to enum class RenderingStateFlags.

Comment 76

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 253889 - part 4: Finally, get rid of nsIPresShell

And also this cleans up some legacy comments of PresShell users.

Comment 77

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ea9cc51a202
part 1: Get rid of nsIPresShell from gfx r=jwatt
https://hg.mozilla.org/integration/autoland/rev/ad0568bbf0d1
part 2: Move all methods and public structs of nsIPresShell into mozilla::PresShell r=emilio
https://hg.mozilla.org/integration/autoland/rev/cbf6babb97cd
part 3: Move all remaining members of nsIPresShell to mozilla::PresShell r=emilio
https://hg.mozilla.org/integration/autoland/rev/d1f9cf74a077
part 4: Finally, get rid of nsIPresShell r=emilio

Comment 78

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/9ea9cc51a202
https://hg.mozilla.org/mozilla-central/rev/ad0568bbf0d1
https://hg.mozilla.org/mozilla-central/rev/cbf6babb97cd
https://hg.mozilla.org/mozilla-central/rev/d1f9cf74a077