Status

()

task
ASSIGNED
15 years ago
6 days ago

People

(Reporter: bryner, Assigned: craig.topper)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(22 attachments, 5 obsolete attachments)

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
(Reporter)

Description

15 years ago
tracking bug for nsIPresShell deCOMtamination
(Reporter)

Comment 1

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

Updated

15 years ago
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+
(Reporter)

Comment 3

15 years ago
> +++ 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().
(Reporter)

Comment 4

15 years ago
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.
(Reporter)

Updated

15 years ago
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+
(Reporter)

Comment 6

15 years ago
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
(Reporter)

Updated

15 years ago
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+
(Reporter)

Comment 7

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

Updated

15 years ago
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+
(Reporter)

Comment 8

15 years ago
Same deal as GetRootFrame, except there are a ton more callers.
(Reporter)

Comment 9

15 years ago
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)
(Reporter)

Comment 10

15 years ago
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.
(Reporter)

Updated

15 years ago
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.
(Reporter)

Comment 15

15 years ago
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)
(Reporter)

Comment 19

15 years ago
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 22

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

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

Comment 23

13 years ago
go **** ya self
(Reporter)

Updated

13 years ago
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
(Assignee)

Comment 25

9 years ago
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.
(Assignee)

Comment 28

9 years ago
Attachment #433701 - Attachment is obsolete: true
(Assignee)

Comment 29

9 years ago
Assigning to myself.
Assignee: zweinberg → craig.topper
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/333a791d201e
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

9 years ago
Status: REOPENED → NEW
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 31

9 years ago
Made GetPresContext, GetViewManager, and IsSafeToFlush const.
Attachment #433824 - Flags: review?(roc)
(Assignee)

Comment 32

9 years ago
Posted patch GetCaret()Splinter Review
Changed GetCaret to return already_AddRefed<nsCaret>.
Attachment #433825 - Flags: review?(roc)
(Assignee)

Comment 33

9 years ago
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)
(Assignee)

Comment 34

9 years ago
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.
(Assignee)

Comment 37

9 years ago
(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.
(Assignee)

Comment 38

9 years ago
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)
(Assignee)

Comment 39

9 years ago
Attachment #433851 - Flags: review?(roc)
(Assignee)

Comment 40

9 years ago
Attachment #433862 - Flags: review?(roc)
Comment on attachment 433847 [details] [diff] [review]
GetEventTargetFrame() and GetEventTargetContent()

this is right
Attachment #433847 - Flags: review?(roc) → review+
(Assignee)

Comment 42

9 years ago
Attachment #433864 - Flags: review?(roc)
(Assignee)

Updated

9 years ago
Attachment #433864 - Flags: review?(roc)
(Assignee)

Updated

9 years ago
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+
(Assignee)

Comment 48

9 years ago
Here's the IID change to go with the other patches as well as some other cleanup.
Attachment #435520 - Flags: review?(roc)
(Assignee)

Comment 49

9 years ago
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?
(Assignee)

Comment 51

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

Comment 53

9 years ago
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?
(Assignee)

Comment 55

9 years ago
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)
(Assignee)

Updated

9 years ago
Attachment #435521 - Attachment is obsolete: true
Attachment #435521 - Flags: review?(roc)
(Assignee)

Comment 56

9 years ago
Made these 3 methods return the failure code from the functions they use instead of just returning NS_OK.
Attachment #435539 - Flags: review?(roc)

Comment 57

9 years ago
For this last patch: rv may be returned uninitialized, so give a default value.
(Assignee)

Comment 58

9 years ago
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+
(Assignee)

Comment 60

9 years ago
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.
(Assignee)

Updated

9 years ago
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
(Assignee)

Comment 65

9 years ago
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)
(Assignee)

Updated

9 years ago
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
(Assignee)

Comment 68

9 years ago
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)
(Assignee)

Comment 69

9 years ago
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?
(Assignee)

Comment 72

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

Updated

8 months ago
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
Type: defect → task
You need to log in before you can comment on or make changes to this bug.