tracking bug for nsIPresShell deCOMtamination

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

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?

> +++ 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().

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 on attachment 155747 [details] [diff] [review] remove non-inlined nsIPresShell::GetPresContext() yum

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

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

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

Comment on attachment 157749 [details] [diff] [review] nsIPresShell::GetPrimaryFrameFor() brendan offered to help out here, so I'll give roc a break.

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.

> 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.

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

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

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.

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

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

Comment on attachment 157948 [details] [diff] [review] possible fix for inlining problems waiting for an answer to the question in my last comment

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 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 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 on attachment 157749 [details] [diff] [review] nsIPresShell::GetPrimaryFrameFor() I guess the same was done in bug 303779. :(

go **** ya self

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

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

Comment on attachment 433701 [details] [diff] [review] DeCOMtamination of more methods Welcome back, Craig! Thanks!!!

Oh, you need to rev the nsIPresShell IID.

Assigning to myself.

Made GetPresContext, GetViewManager, and IsSafeToFlush const.

Changed GetCaret to return already_AddRefed<nsCaret>.

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.

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.

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

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.

(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.

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 on attachment 433847 [details] [diff] [review] GetEventTargetFrame() and GetEventTargetContent() this is right

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

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

Comment on attachment 435518 [details] [diff] [review] Moved some variables from PresShell to nsIPresShell and inlined their getters Will need an IID rev.

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

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

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

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

Yes.

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.

Hmm. Maybe we should move mRootFrame into nsIPresShell?

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.

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

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

Thanks for catching that.

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.

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.

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

(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

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

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.

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).

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.

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 on attachment 469790 [details] [diff] [review] Moved more methods to nsIPresShell and de-virtualize as many as possible Need to rev IID again

Craig, anything left to land here?

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

Pushed by masayuki@d-toybox.com: 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