Closed Bug 253889 Opened 16 years ago Closed 1 year ago

deCOMtaminate nsIPresShell

Categories

(Core :: Layout, task)

task
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: bryner, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(26 files, 5 obsolete files)

51.89 KB, patch
roc
: review+
Details | Diff | Splinter Review
105.32 KB, patch
roc
: review+
Details | Diff | Splinter Review
47.96 KB, patch
roc
: review+
Details | Diff | Splinter Review
19.79 KB, patch
roc
: review+
Details | Diff | Splinter Review
101.64 KB, patch
Details | Diff | Splinter Review
41.51 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
59.49 KB, patch
Details | Diff | Splinter Review
3.02 KB, patch
roc
: review+
Details | Diff | Splinter Review
21.36 KB, patch
roc
: review+
Details | Diff | Splinter Review
17.76 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.73 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.65 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.56 KB, patch
roc
: review+
Details | Diff | Splinter Review
854 bytes, patch
Details | Diff | Splinter Review
9.16 KB, patch
roc
: review+
Details | Diff | Splinter Review
11.64 KB, patch
roc
: review+
Details | Diff | Splinter Review
10.41 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.53 KB, patch
roc
: review+
Details | Diff | Splinter Review
8.96 KB, patch
Details | Diff | Splinter Review
7.52 KB, patch
roc
: review+
Details | Diff | Splinter Review
8.12 KB, patch
roc
: review+
Details | Diff | Splinter Review
49.18 KB, patch
roc
: review+
Details | Diff | Splinter Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
tracking bug for nsIPresShell deCOMtamination
This converts all callers to use the inlined version of
nsIPresShell::GetDocument() and removes the virtual, addref'ing version.
Attachment #154903 - Flags: superreview?(roc)
Attachment #154903 - Flags: review?(roc)
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?
Attachment #154903 - Flags: superreview?(roc)
Attachment #154903 - Flags: superreview+
Attachment #154903 - Flags: review?(roc)
Attachment #154903 - Flags: review+
> +++ 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.
Attachment #155747 - Flags: superreview?(roc)
Attachment #155747 - Flags: review?(roc)
Comment on attachment 155747 [details] [diff] [review]
remove non-inlined nsIPresShell::GetPresContext()

yum
Attachment #155747 - Flags: superreview?(roc)
Attachment #155747 - Flags: superreview+
Attachment #155747 - Flags: review?(roc)
Attachment #155747 - Flags: review+
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
Attachment #157096 - Flags: superreview?(roc)
Attachment #157096 - Flags: review?(roc)
Attachment #157096 - Flags: superreview?(roc)
Attachment #157096 - Flags: superreview+
Attachment #157096 - Flags: review?(roc)
Attachment #157096 - Flags: review+
Got rid of GetRootFrame's useless nsresult return, changed callers within
layout to use the frame manager so that the whole thing is inlined.
Attachment #157665 - Flags: superreview?(roc)
Attachment #157665 - Flags: review?(roc)
Attachment #157665 - Flags: superreview?(roc)
Attachment #157665 - Flags: superreview+
Attachment #157665 - Flags: review?(roc)
Attachment #157665 - Flags: review+
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.
Attachment #157749 - Flags: superreview?(brendan)
Attachment #157749 - Flags: review?(brendan)
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.
Attachment #157948 - Flags: superreview?(roc)
Attachment #157948 - Flags: review?(dbaron)
> 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
Attachment #157948 - Flags: superreview?(roc)
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
Attachment #157948 - Flags: review?(dbaron) → review+
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
Attachment #157749 - Flags: superreview?(brendan)
Attachment #157749 - Flags: review?(brendan)
Comment on attachment 157749 [details] [diff] [review]
nsIPresShell::GetPrimaryFrameFor()

I guess the same was done in bug 303779. :(
go **** ya self
Assignee: bryner → nobody
Blocks: deCOM
I'm not promising to do this, but I might, so I'm assigning to myself so I don't lose it.
Assignee: nobody → zweinberg
Attached patch DeCOMtamination of more methods (obsolete) — Splinter Review
This includes deCOMtamination of various methods. I can split this patch up if that would be easier to review.

More to come.
Attachment #433701 - Flags: review?(roc)
Comment on attachment 433701 [details] [diff] [review]
DeCOMtamination of more methods

Welcome back, Craig! Thanks!!!
Attachment #433701 - Flags: review?(roc) → review+
Oh, you need to rev the nsIPresShell IID.
Attachment #433701 - Attachment is obsolete: true
Assigning to myself.
Assignee: zweinberg → craig.topper
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/333a791d201e
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → NEW
Status: NEW → ASSIGNED
Made GetPresContext, GetViewManager, and IsSafeToFlush const.
Attachment #433824 - Flags: review?(roc)
Attached patch GetCaret()Splinter Review
Changed GetCaret to return already_AddRefed<nsCaret>.
Attachment #433825 - Flags: review?(roc)
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.
Attachment #433828 - Flags: review?(roc)
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.
Attachment #433828 - Attachment is obsolete: true
Attachment #433830 - Flags: review?(roc)
Attachment #433828 - Flags: review?(roc)
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();
Attachment #433847 - Flags: review?(roc)
Attached patch IsReflowLocked()Splinter Review
Attachment #433851 - Flags: review?(roc)
Attachment #433862 - Flags: review?(roc)
Comment on attachment 433847 [details] [diff] [review]
GetEventTargetFrame() and GetEventTargetContent()

this is right
Attachment #433847 - Flags: review?(roc) → review+
Attachment #433864 - Flags: review?(roc)
Keywords: checkin-needed
Checked in attachments 433824, 433825, 433830, 433847, 433851, 433862, and 433864 as http://hg.mozilla.org/mozilla-central/rev/4b8936ac4a31
Comment on attachment 435518 [details] [diff] [review]
Moved some variables from PresShell to nsIPresShell and inlined their getters

Will need an IID rev.
Attachment #435518 - Flags: review?(roc) → review+
Here's the IID change to go with the other patches as well as some other cleanup.
Attachment #435520 - Flags: review?(roc)
Some of the calls in this file were already doing this and I happened to see a few that weren't.
Attachment #435521 - Flags: review?(roc)
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?
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.
Attachment #435528 - Flags: review?(roc)
Attachment #435521 - Attachment is obsolete: true
Attachment #435521 - Flags: review?(roc)
Made these 3 methods return the failure code from the functions they use instead of just returning NS_OK.
Attachment #435539 - Flags: review?(roc)
For this last patch: rv may be returned uninitialized, so give a default value.
Thanks for catching that.
Attachment #435539 - Attachment is obsolete: true
Attachment #435543 - Flags: review?(roc)
Attachment #435539 - Flags: review?(roc)
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.
Attachment #435543 - Flags: review?(roc) → review+
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.
Keywords: checkin-needed
No longer blocks: 555727
Depends on: 555727
We need to back out 4b8936ac4a31 to fix VC9 builds (bug 555727)
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
Keywords: checkin-needed
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.
Attachment #436987 - Flags: review?(roc)
Keywords: checkin-needed
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).
Depends on: 557690
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.
Attachment #469789 - Flags: review?(roc)
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?
Attachment #469790 - Flags: review?(roc)
Comment on attachment 469790 [details] [diff] [review]
Moved more methods to nsIPresShell and de-virtualize as many as possible

Need to rev IID again
Attachment #469790 - Flags: review?(roc) → review+
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.
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
Type: defect → task
Assignee: craig.topper → masayuki

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

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

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

And also this changes enum RenderFlags to enum class RenderingStateFlags.

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

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
You need to log in before you can comment on or make changes to this bug.