190735 - deCOMtaminate nsIFrame

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

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

I have a patch and a strategy for deCOMifying nsIFrame. I will explain in a moment.

Comment 1

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

I'm going to attach a patch that whacks nsIFrame (and to some extent, nsFrame)
so that it supports both COM-style and non-COM-style interfaces for every
function that is actually used. We can land this patch immediately (well, once
1.4 opens) and start using the clean interfaces immediately. We can also
incrementally convert existing code from the COM-style to the non-COM-style ---
both callers of methods and implementors (overriders) of methods. This patch is
also hopefully of a reviewable size. I hope the subsequent conversion can be
done with a blanket r+sr. Once the conversion is complete we can remove the
legacy methods, and we can also remove the non-COM wrappers.

Here's an example of what I've done.
Existing code:
   NS_IMETHOD CanContinueTextRun(PRBool* aResult) = 0;
becomes
   virtual PRBool _CanContinueTextRun() {
     PRBool r;
     CanContinueTextRun(&r);
     return r;
   }
   NS_IMETHOD CanContinueTextRun(PRBool* aResult) {
     *aResult = _CanContinueTextRun();
     return NS_OK;
   }

This changes no behaviour at all. _CanContinueTextRun isn't called anywhere
(yet), and CanContinueTextRun is overridden in nsFrame. But going forward it
allows two things: We can convert calls to CanContinueTextRun to
_CanContinueTextRun, and they will work. Once all virtual call sites are
converted, we can convert the implementors (starting at the leaves of the class
hierarchy) to override the new method instead of the old one. Once we're done,
we can delete CanContinueTextRun, make _CanContinueTextRun() in nsIFrame pure
virtual, and use a script to rename _CanContinueTextRun to CanContinueTextRun
globally. This wasn't necessary everywhere, a lot of the new methods have been
given their final "real" name (methods that are only implemented in nsFrame, or
which I renamed to a better name anyway).

The reason we have to use _CanContinueTextRun instead of just overloading
CanContinueTextRun is because of a stupid feature of C++. The problem is that
subclasses which override CanContinueTextRun(PRBool*) would actually hide
CanContinueTextRun() from the base class, making it inaccessible in the
subclass. This also produces lots of warnings.

While whacking nsIFrame I have done the following things:
-- removed nsresult almost everywhere. In almost all cases the result was never
checked, or it was checked but nothing intelligent was or could be done about a
failure, or implementations always returned NS_OK, or the result was better
indicated with a special return value (typically null).
-- Replaced nsIPresShell references with nsIPresContext, with the idea of
helping an eventual merge of nsIPresShell into nsIPresContext
-- renamed methods where I thought the names were poor
-- converted out parameters to real results whenever possible
-- made all remaining out parameters be pointers, not references. bz and I agree
that having to put "&" in your parameter list at a call site helps show that
that parameter is an out parameter (C# does something similar with "ref" for its
out parameters).
-- removed methods that no-one was using or that should be somewhere else (e.g.,
moved down into nsTextFrame) (of course the COM-style legacy methods are still
there, for now)
-- made all references to nsISupports-derived objects be pointers, not C++
references --- especially nsIRenderingContext. This is for consistency.
-- made methods nonvirtual if they were only implemented in nsFrame

One thing I did not do was to stop nsIFrame inheriting from nsISupports. This
seems quite difficult because in various places frames are passed around as
nsISupports (e.g., frame tree iterators).

Comment 2

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

I had to add some 'virtual's back in to fix linking problems. Too bad editor
likes to monkey around with nsIFrame :-(.

Comment 3

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

Attachment: promised patch

Comment 4

[Boris Zbarsky [:bzbarsky]]

Quick comments from a totally partial skim:

1) already_AddRefed<nsIStyleContext> GetStyleContext() {}

That should prevent the need for that comment about it being already addrefed
too.  ;)

2) Any reasons not to use the MoveTo() and SizeTo() functions on mRect instead of
   setting its x, y, etc manually in the SetPosition and SetSize methods?

Comment 5

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

already_AddRefed... neat

OK, I'll use MoveTo/SizeTo.

Comment 6

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

*** Bug 187924 has been marked as a duplicate of this bug. ***

Comment 7

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

Attachment: improved patch

Takes account of bz's comments.

Makes some functions non-virtual and adds ...External versions for those
modules outside layout that can't link to them.

Comment 8

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

Attachment: another iteration

Changes SetFrameState/GetFrameState to
AddStateBits/RemoveStateBits/GetStateBits. adds nsresult to Init() because some
initializations really can fail and we need to detect them.

Comment 9

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

-   * Destroy() for each child)
+   * Destruct() for each child)

oops?

You could use a comment about the _ convention.  (That's the migration plan,
right?  I could certainly use the comment to remind me exactly how it's used.)

I think bryner may have beaten you to changing GetStyleContext.  Have you had a
chance to merge?

I'm not crazy about virtual functions defined inline.  Any better ideas?

More later...

Comment 10

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

> oops?

yep

> You could use a comment about the _ convention.  (That's the migration plan,
> right?  I could certainly use the comment to remind me exactly how it's used.)

Yep.

> I think bryner may have beaten you to changing GetStyleContext.  Have you had
> a chance to merge?

No. I don't carry patches in my tree. I'll merge when I iterate this patch for you.

> I'm not crazy about virtual functions defined inline.  Any better ideas?

Well, it's just a convenience. I could easily move them to nsFrame (leaving them
as nsIFrame::Blah), it would just be slightly more code to write. Anything in
particular make you disapprove of the practice?

Comment 11

[Boris Zbarsky [:bzbarsky]]

roc, is this a priority for 1.4a?  Or is it ok to start this in 1.4b?  (just
trying to prioritize reviews....)

Comment 12

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

This isn't going to make 1.4a. Either we will decide it's low risk enough for
1.4b, or we'll push it to 1.5.

The only review I want from you is for the transparent windows bug 113232 :-).

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 113155 [details] [diff] [review]
another iteration

>+typedef PRInt32 nsPaintResult;

Documentation?	What are possible values?  What do they mean?

>+/**
>+ */
>+struct nsContentRange {

Documentation?	What is this struct used for?

>+   * This method *can* fail. For example, frames for IFRAMEs and other complex
>+   * objects can fail to initialize due to resource limits.
>+   *
>    * @param   aContent the content object associated with the frame
>    * @param   aGeometricParent  the geometric parent frame
>    * @param   aContentParent  the content parent frame

There is no aContentParent an hasn't been in a while... Remove that line?

>-   * Destroy() for each child)
>+   * Destruct() for each child)

What's "Destruct()"?

>+  virtual void _SetInitialChildList(nsIPresContext* aPresContext,
>+                                    nsIAtom*        aListName,
>+                                    nsIFrame*       aChildList) {
>+    SetInitialChildList(aPresContext, aListName, aChildList);
>+  }

Hmmm... Are we sure we want to give no feedback on using an unrecognized
aListName?

(Not that anyone checks the return value of SetInitialChildList right now....)

>-  NS_IMETHOD AppendFrames(nsIPresContext* aPresContext,
>+   virtual void _AppendFrames(nsIPresContext* aPresContext,

Same here. And this is mis-indented.

>+  virtual void _InsertFrames(nsIPresContext* aPresContext,
>+  virtual void _RemoveFrame(nsIPresContext* aPresContext,
>+  virtual void _ReplaceFrame(nsIPresContext* aPresContext,

And here.

>+  already_AddRefed<nsIContent> GetContent() const { NS_IF_ADDREF(mContent); return mContent; >+  nsIContent* GetContentUnrefed() const { return mContent; }

Any reason to have both?  Why not just have |nsIContent* GetContent() { return
mContent; }|?

Same question for GetStyleContext/GetStyleContextUnrefed.

>+   * The indicies must be consecutive and implementations MUST return
>+   * nsnull if asked for an index that is out of range.

"indices".

>+  virtual already_AddRefed<nsIAtom> _GetAdditionalChildListName(PRInt32 aIndex) const {

Does this return nsnull if aIndex is out of some range?

>+   * @return an indication of the layers that need to be painted by this frame

I assume this will become clear in the future?	;)  It could use some
documentation explaining what one would use the return value for....

>+  virtual nsPaintResult _Paint(nsIPresContext*      aPresContext,

Will failures (can there be any?) be indicated via the nsPaintResult?

>+  virtual nsReflowStatus _Reflow(nsIPresContext*          aPresContext,

Same here....  Some things still do Init()-like stuff on initial reflow and
could fail.... Or do we want to enforce Reflow() not failing and move all such
stuff to Init() ?

>+  already_AddRefed<nsIAtom> GetType() const {
>+  virtual nsIAtom* GetTypeUnrefed() const {

Again, why have both?

>+  NS_IMETHOD Destroy(nsIPresContext* aPresContext) {
>+    _Destroy(aPresContext);
>+    return NS_OK;
>+  }

This bothers me.... _Destroy just calls Destroy(), no?	So you're depending on
all subclasses overriding this....  Is that a good thing to depend on?

Same applies to lots of the legacy stuff.



>Index: layout/base/public/nsIPresContext.h
>+  // deCOMtaminated methods
>+  nsIPresShell* GetShellUnrefed() { return mShell; }
>+  nsIPresShell* GetShell() { NS_IF_ADDREF(mShell); return mShell; }

already_AddRefed for that second one, please.  Again, why do we need both?

I guess we should add a comment saying that nsIPresContext is not a real
interface either?  Otherwise someone will try to "fix" the presence of members
in the interface....

>Index: layout/base/public/nsIPresShell.h
>+  nsIPresContext* GetPresContext() {

already_AddRefed.

>+  nsIPresContext* GetPresContextUnrefed();

And again, do we need both?


In nsFrame.cpp:

>-  NS_FRAME_TRACE_MSG(NS_FRAME_TRACE_CALLS,
>-                     ("WillReflow: oldState=%x", mState));

Why remove that?

>Index: layout/html/base/src/nsFrame.h
>+  NS_IMETHOD SetParent(const nsIFrame* aParent) {
>+    mParent = (nsIFrame*)aParent;

Can we make mParent const?  If not, imo SetParent should not take a const.

>+nsIPresContext*
>+PresShell::GetPresContextUnrefed()

Just call it GetPresContext; the fact that it's unrefed is clear from the
return type -- if it addrefed, it would return already_AddRefed

Looks pretty sweet, Robert. ;)

Comment 14

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

> Documentation?	What are possible values?  What do they mean?

OK

> Documentation?	What is this struct used for?

Used to get/set a range of content children. I'll add a comment.

> here is no aContentParent an hasn't been in a while... Remove that line?

Yeah

> What's "Destruct()"?

A mistake. Should be Destroy().

> Hmmm... Are we sure we want to give no feedback on using an unrecognized
> aListName?
>
> (Not that anyone checks the return value of SetInitialChildList right now....)

We should assert in the method. This is not something we really want to probe
for at runtime --- and if we really have to probe for some reason, we can
explicitly scan the child list names to see if the list we want is there. I'll
bet money we never want to do that.

As a general rule, I'm not returning a result if either a) no-one currently
checks it or b) it always currently returns NS_OK. Especially for these private
interfaces, I think we cry wolf when we pretend things can fail that really
can't (or where the failure is really just a coding bug that should be fixed and
therefore should be an assertion). It just means we end up ignoring results
willy-nilly and end up ignoring the ones that matter, too. (For public
interfaces the situation is a bit different because "can fail/cannot fail" may
be exposing too much of the implementation to clients.)

> Any reason to have both?  Why not just have |nsIContent* GetContent() { return
> mContent; }|?

Mainly because our getters usually do return an already-addrefed object. I'd
like to remain consistent with that.

> Does this return nsnull if aIndex is out of some range?

Yeah. I'll add a comment.

> I assume this will become clear in the future?	;)

Yeah. This is really a forward-looking comment anticipating a scheme for
dynamically discovering which layers need to be painted to reduce the number of
passes over the frame tree during painting. I should probably just remove it for
now.

> Will failures (can there be any?) be indicated via the nsPaintResult?

No, I don't think we'll be indicating Paint failure.

> Same here....  Some things still do Init()-like stuff on initial reflow and
> could fail.... Or do we want to enforce Reflow() not failing and move all such
> stuff to Init() ?

IIRC we don't currently handle Reflow failures anywhere. What would you do, anyway?

> This bothers me.... _Destroy just calls Destroy(), no?	So you're
> depending on all subclasses overriding this....  Is that a good thing to
> depend on?

Not in the long term, but this is a necessary part of the migration strategy.
The strategy is: change all virtual call sites of Destroy() to call _Destroy();
then change all implementations of Destroy() to override _Destroy() and call
ParentClass::_Destroy()*; then remove the Destroy() stub from nsIFrame; then
rename _Destroy to Destroy everywhere. The first two steps can be done
incrementally.

* This step has to be done to all children of a class C before it is done to C
itself.

Probably in practice we'll start by focusing on the virtual call sites and fix
most of them before we start fixing up the implementation signatures.

> already_AddRefed for that second one, please.

Sure.

> I guess we should add a comment saying that nsIPresContext is not a real
> interface either?

Yeah

> Why remove that?

Oops. I'll put it back.

> Can we make mParent const?  If not, imo SetParent should not take a const.

It probably should not take a const.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Mainly because our getters usually do return an already-addrefed object. I'd
> like to remain consistent with that.

Why? For out params that's a necessary convention, because you can't tell what
the callee did to you nsIFoo**.  But here, if you're assigning into an nsCOMPtr
you don't care what the method returns (because use of already_AddRefed will not
leak in that situation, and neither will a raw ptr return).  If you're assigning
into a raw ptr, the already_AddRefed version will simply fail to compile, and
you have to call .get() on it, indicating that you know you're taking over the
ref.  So the already_AddRefed annotation resolves the callee ambiguity that has
to be resolved by a convention in the out param case...

I'd just make all those methods return weak ptrs; if the caller wants to take a
strong ref by assigning into an nsCOMPtr, he will.  Just my $0.02.

Comment 16

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

OK, I'm convinced.

Comment 17

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

I also agree with bz about returning weak pointers where possible.

Comment 18

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

Comment on attachment 113155 [details] [diff] [review]
another iteration

I'm willing to say r=dbaron if you produce a patch with bz's comments
addressed.  I've looked at this a bit in the past and I think we should just
move forward rather than slowing everything down with too much review.

(And I also want this to land so I can combine the two templatized global
GetStyleData functions into one that has both parameters templatized so that we
don't need .get() when using it.)

Comment 19

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

Good ... For the next week at least I have to focus on cleaning up the fallout
from all my 1.4a landings so I'm not quite sure when I'll get back to this.

Comment 20

[Boris Zbarsky [:bzbarsky]]

OK, as I see it there is a possible problem with RemoveFrame/ReplaceFrame.

Consider the case when I have |nsIFrame *aFrame|.  This frame has a parent but
is _not_ in that parent's child list yet (it's in the middle of being constructed).

I try to get rid of that frame.  The problem is, I don't _know_ whether it's in
the parent's child list, since the same code can be hit from elsewhere.  So
after I call RemoveFrame or ReplaceFrame I have no way to tell whether I'm
supposed to be calling Destroy() on the frame...

We could say that RemoveFrame and ReplaceFrame should not be called on frames
that are not kids of that parent.  Then we need a way to find out whether a
frame is a kid of that parent....

(Fwiw, this is an issue for CantRenderReplacedElement, sorta.  I've hacked
around it for now, and I hope to rewrite CantRenderReplacedElement such that it
ceases being an issue, but it could come up in other contexts....  I'm not
saying those methods _need_ to return nsresult, but they should be asserting
otherwise and we should look at what places trigger those asserts.)

Comment 21

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

OK, I'll add results for Remove/ReplaceFrame.

I'm trying to decide how to handle prescontexts. Since this patch likely won't
land until 1.5 anyway, I think I'll try to put a prescontext pointer in
nsDocument, and let frames get it from there. We'll have something like this in
nsIFrame:

  virtual nsIPrescontext* GetPresContext() {
    GetContent()->GetDocument()->GetPresContext();
  }

which should turn into 3 loads if we do those helper functions right.

This will require extra code to make printing work right which resets the
prescontext on the document.

Comment 22

[Boris Zbarsky [:bzbarsky]]

As long as we're careful not to call that stuff as content is being removed from
the document (think removeChild)....

Comment 23

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

Comment on attachment 113155 [details] [diff] [review]
another iteration

Need another rev of this patch

Comment 24

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

Since this patch seems to be rotting, I'm tempted to help with this.  (I've
already done GetStyleData, at least to some extent.)  I'd be more inclined to
break it up into pieces by function (e.g., ContentStatesChanged,
GetView/HasView) rather than by class (as attachment 113155 [details] [diff] [review] seems to do).

Or were you (Robert) planning to revive this patch soon?

Comment 25

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

I'd like to get rid of the aPresContext parameters in this pass too. What do you
think is the best way to do that? Can we just stick an mPresContext in nsFrame
for now?

Comment 26

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

I think I'd rather not combine too many things into one patch, and I'm also a
bit hesitant about any strategy that involves lots of "temporary" code, having
seen temporary code that's still around from 1999.

I'd rather do the aPresContext removal somewhat separately and probably all at
once.  (If not all at once, then at least in groups of related methods that call
each other, such as "reflow methods", "painting methods", etc.  Otherwise we'd
change an |aPresContext| to a |GetPresContext()| in one pass and remove it in
the next pass, since many of the methods take pres context parameters only to
pass to something else that takes a pres context.)  However, initial steps could
be done to remove it from (1) things like nsFrameManager that can have an
mPresContext member variable independently of frames and (2) from all the
nsIFrame methods that then don't need it.  I'd rather not leave code in the tree
for a while that leaves us with the costs of both ways (the memory use of the
mPresContext on all frames plus the code to pass aPresContext from place to
place) and the benefits of neither.

Comment 27

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

OK, fine.

Comment 28

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

For reflow methods we can stick an mPresContext in the reflow state.

Comment 29

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

We can already do something just like comment 21 (and probably even more
null-safe), by getting the pres context from the rule node from the style
context from the frame.  (Same number of steps as the proposal in comment 21.) 
If that sounds reasonable, I'll do a patch (with a little deCOMification of the
necessary functions) and put it in a dependent bug...

Comment 30

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

Hey yeah! That rules! aPresContext BEGONE!

Comment 31

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

Attachment: checkin-able subset

Here's just a subset of the deCOMtamination that might be ready for checkin. It
just adds some non-COM accessor methods where that's easy, and removes
aPresContext parameters from a few of the methods that are only implemented in
nsFrame. Those methods get wrappers so no other client code has to change right
away.

If we check this in, then I would suggest just going mad and fixing all callers
to the obsolete methods, and removing those methods, without further review.

Comment 32

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

Hmm. Maybe methods like GetClosestView, GetParentWithView, and
AnyAncestorVisible should be moved to nsLayoutUtils. This would be a good time
to do it. Any thoughts?

Comment 33

[Boris Zbarsky [:bzbarsky]]

What would be the benefits of doing so?

Comment 34

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

It would make nsIFrame look more like "an interface to frames" rather than
"algorithms which involve frames". Maybe this is a distinction not worth making.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Yeah, ok.  That makes sense.

Comment 36

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

Comment on attachment 126101 [details] [diff] [review]
checkin-able subset

>+  nsRect GetRect() const { return mRect; }

How about:

const nsRect& GetRect() const { return mRect; }

(which shouldn't be slower for any uses (since clients can always copy into
their own nsRect, and could be faster for clients that assign to |const
nsRect&| or just use members directly) and maybe even a non-const version
instead of |SetRect|?

>+  nsFrameState GetFrameState() { return mState; }
>+  void SetFrameState(nsFrameState aNewState) { mState = aNewState; }

How about using what you had in the previous patch instead of this, which was:

+  nsFrameState GetStateBits() { return mState; }
+  void AddStateBits(nsFrameState aBits) { mState |= aBits; }
+  void RemoveStateBits(nsFrameState aBits) { mState &= ~aBits; }

I liked that better.

Could you also rename the new-style version of |GetParentWithView| to
|GetAncestorWithView|?

>-NS_IMETHODIMP nsFrame::GetParentWithView(nsIPresContext* aPresContext,
>-                                         nsIFrame**      aParent) const
>+nsIFrame* nsIFrame::GetParentWithView() const
> {
>-  NS_PRECONDITION(nsnull != aParent, "null OUT parameter pointer");
>-
>   nsIFrame* parent;
>-  for (parent = mParent; nsnull != parent; parent->GetParent(&parent)) {
>+  for (parent = mParent; nsnull != parent; parent = parent->GetParent()) {

Could you call the variable something other than |parent| (maybe |f|), and
declare it inside the for-loop expression?

> /* virtual */ PRBool
>-nsIFrame::AreAncestorViewsVisible(nsIPresContext* aPresContext)
>+nsIFrame::AreAncestorViewsVisible() const
> {
>-  for (nsIView* view = GetClosestView(aPresContext);
>+  for (nsIView* view = GetClosestView();
>        view; view->GetParent(view)) {

If these lines now fit under 80 chars, could you recombine?

>-NS_IMETHODIMP nsFrame::GetWindow(nsIPresContext* aPresContext,
>-                                 nsIWidget**     aWindow) const
>+nsIWidget* nsIFrame::GetWindow() const
> {
>-  NS_PRECONDITION(nsnull != aWindow, "null OUT parameter pointer");
>-  
>   nsIFrame*  frame;

Could you make this a |const nsIFrame*| and move it into the for-loop's
expression?

>   nsIWidget* window = nsnull;
>-  for (frame = (nsIFrame*)this; nsnull != frame; frame->GetParentWithView(aPresContext, &frame)) {
>+  for (frame = (nsIFrame*)this; nsnull != frame; frame = frame->GetParentWithView()) {

and skip the cast, and the "nsnull !=", and if that doesn't get it under 80
chars, break the line?

With those changes, r+sr=dbaron.

Comment 37

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

> (which shouldn't be slower for any uses (since clients can always copy into
> their own nsRect, and could be faster for clients that assign to |const
> nsRect&| or just use members directly)

I think it's slightly cleaner to return the *current* rect than to return a
reference to a rect that will change if the frame changes size or position, but OK.

> and maybe even a non-const version
> instead of |SetRect|?

I think I'll pass on this one. We might conceivably want to be able to track
actual changes to the rect.

> I liked that better.

Hmm, sure, so do I actually :-).

> Could you also rename the new-style version of |GetParentWithView| to
> |GetAncestorWithView|?

Sure

> Could you call the variable something other than |parent| (maybe |f|),

Sure

> and declare it inside the for-loop expression?

Isn't that deprecated because of different compiler interpretations of the
scope? I'll do it anyway since it won't matter here.

> If these lines now fit under 80 chars, could you recombine?

Sure

> Could you make this a |const nsIFrame*|

Sure

> and move it into the for-loop's expression?

OK

> and skip the cast, and the "nsnull !=", and if that doesn't get it under 80
> chars, break the line?

Sure.

Thanks!

Comment 38

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

> I think it's slightly cleaner to return the *current* rect than to return a
> reference to a rect that will change if the frame changes size or position,
but > OK.

But if that's what someone wants, wouldn't the |const nsRect&| syntax raise a
red flag?  (Maybe not, though, since it could be seen as binding a temporary to
a reference.)

(I never learned the rules for how many times the copy-constructor gets called
in the various use cases for your version...)

Comment 39

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

> But if that's what someone wants, wouldn't the |const nsRect&| syntax raise a
> red flag?

Probably someone would just copy manually with nsRect r = frame->GetRect(). I'm
just saying that the copy semantics is the cleaner default.

> I never learned the rules for how many times the copy-constructor gets called
> in the various use cases for your version...

Since it's inline, I don't think it really matters; the compiler should always
be able to get away with just one copy (or less; only the fields which are used
need to be copied).

Comment 40

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

After bug 206682, I have very little faith in the ability of compilers to
optimize code the way I think they ought to.

Comment 41

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

I asked scc about this a while ago and he claimed that

nsRect r = frame->GetRect(); (as I've defined it)
could be treated as equivalent to
frame->GetRect(&r);

so we should be OK.


After checking this in, shall I just go ahead and check in updates of callers to
the new methods? Or do you want review of those changes?

Comment 42

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

I checked in the patch with nsRect GetRect(). Changing it to const nsRect& in
the future will be easy if that's necessary.

Comment 43

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

After I check in a few more changes this afternoon, I'll have finished
layout/html/base/src. I'll move on to layout/html/forms/src.

A few observations:
-- Lots more interfaces should be deCOMtaminated...
-- Lots of nsCOMPtrs can be converted to weak pointers. I think this is
responsible for a lot of the code size savings.
-- The code size saving in gcc is often 2x the size saved in VC++ ... this
implies something very sad about gcc code generation.
-- We should document when methods can never return null (e.g.,
nsIPresContext::GetViewManager, nsIView::GetViewManager, nsIFrame::GetParent()
for non-viewport frames) and eliminate unncessary null checks. They uglify the
code and probably create unnecessary and costly alternative exit paths.
-- Some frames have an mPresContext member, which should be removed.

Comment 44

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

OK, now I've done everything under view/, layout/html/, and layout/base/. Next
up, layout/xul/, layout/svg/ and layout/mathml/.

Comment 45

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

Attachment: review "comments" on part of what I rubber-stamped

This fixes a few mistakes (with the last of them, I'm not sure whether it was
intended -- but I do want to look at the code some more) in the first third or
so of what roc's checked in under my rubber-stamp review, and also cleans up a
few things I noticed while reviewing.

Comment 46

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

Should I stop using the rubberstamp and go back to normal review?

Comment 47

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

Attachment: layout/xul changes

This contains all the changes to layout/xul, which are the last changes under
layout/.

Shall I check this in under rubber-stamp or do you want to review them first?

Comment 48

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

I reviewed it myself, caught a couple of errors, and checked it in.

Comment 49

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

I just checked in the changes for content/.

Comment 50

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

Attachment: Final patch in this series

Here's the final patch that cleans up the last uses of the deprecated methods
and removes the deprecated methods. This patch involves changes to
platform-specific code (and may reveal places that my build doesn't build where
I missed some changes), so it's the riskiest of the lot. However, there is
really nothing interesting going on here so unless I hear otherwise I'll check
it in under rubber-stamp and watch Tinderbox carefully.

Comment 51

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

Comment on attachment 127609 [details] [diff] [review]
review "comments" on part of what I rubber-stamped

oh, for some reason I thought we'd already checked this in.

I can check this in too if you like.

Comment 52

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

Checked in both of those patches and fixed bustage. woohoo.

Comment 53

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

FYI, I'm now working up a patch to convert GetFrameType to

NS_IMETHOD_(nsIAtom*) GetType();

Comment 54

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

Attachment: fix GetFrameType() -> GetType

As promised. This is a simple one; the signature changes in nsIFrame.h to
"virtual nsIAtom* GetType()" and everything else follows from that.

Comment 55

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

Comment on attachment 133872 [details] [diff] [review]
fix GetFrameType() -> GetType

Boris, another giant cleanup patch for you to review. Note: if you just
rubber-stamp it, I won't tell :-)

Comment 56

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 133872 [details] [diff] [review]
fix GetFrameType() -> GetType

>Index: layout/base/src/nsFrameTraversal.cpp
>+        nsIAtom* atom = grandParent->GetType();
>+        if ( atom ) 
>         {
>   #ifdef DEBUG_skamio

How about moving the if (atom) check inside the ifdef?	And hoisting the code
that runs in opt builds out of the body of this if?

>Index: layout/base/src/nsLayoutUtils.cpp
>   }
>-
>+  
>   if (aContent && aFrame->GetContent() != aContent) {

Let the whitespace be.	;)

>Index: layout/html/base/src/nsFrame.cpp
>@@ -3264,16 +3262,13 @@ nsFrame::GetNextPrevLineFromeBlockFrame(

>-          nsIAtom *resultFrameType;
>-          if(NS_SUCCEEDED(resultFrame->GetFrameType(&resultFrameType)) && resultFrameType)
>-          {
>-            if (resultFrameType ==  nsLayoutAtoms::tableOuterFrame)
>+            if (resultFrame->GetType() == nsLayoutAtoms::tableOuterFrame)

That code needs to get outdented, right?  Please do that.

>Index: layout/html/base/src/nsHTMLReflowState.cpp

>-      if (IS_TABLE_CELL(fType.get())) {
>+      if (IS_TABLE_CELL(parentReflowState->frame->GetType())) {

IS_TABLE_CELL is a macro that uses the type more than once... you may want to
store the type in a temporary here as before.

>@@ -1656,15 +1642,13 @@ static void
> CheckResetTableDerivedComputedWidth(nsHTMLReflowState& aState,
>+      if (!IS_TABLE_CELL(aState.parentReflowState->frame->GetType())) {

Again.

>Index: layout/html/style/src/nsCSSFrameConstructor.cpp

> GetOuterTableFrame(nsIFrame* aParentFrame) 
> {
>+  if (nsLayoutAtoms::tableOuterFrame == aParentFrame->GetType()) {
>+    return aParentFrame;
>   }
>   else {
>-    parent = aParentFrame->GetParent();
>+    return aParentFrame->GetParent();

else after return...

>@@ -10947,24 +10877,19 @@ nsCSSFrameConstructor::CreateContinuingF
>+        if (IS_TABLE_CELL(cellFrame->GetType())) {

IS_TABLE_CELL again.

>Index: layout/html/table/src/nsCellMap.cpp

>@@ -1342,23 +1342,19 @@ PRBool nsCellMap::CellsSpanOut(nsIPresCo
>+      if (IS_TABLE_CELL(cellFrame->GetType())) {

>@@ -1487,18 +1483,15 @@ nsCellMap::ExpandWithRows(nsIPresContext
>+      if (IS_TABLE_CELL(cFrame->GetType())) {

>@@ -1898,18 +1891,15 @@ nsCellMap::RebuildConsideringRows(nsIPre
>+        if (IS_TABLE_CELL(cFrame->GetType())) {

Again, all three times.

>Index: layout/html/table/src/nsTableCellFrame.cpp

>@@ -98,15 +98,13 @@ nsTableCellFrame::~nsTableCellFrame()
>+    if (IS_TABLE_CELL(childFrame->GetType())) {

And here.

>Index: layout/html/table/src/nsTableFrame.cpp

>@@ -473,15 +470,13 @@ nsTableFrame::IsPercentageBase(PRBool& a
>+  if (IS_TABLE_CELL(aFrame->GetType())) {

And here.

>@@ -1206,15 +1191,13 @@ void nsTableFrame::RemoveRows(nsIPresCon
>+    if (IS_TABLE_CELL(kidFrame->GetType())) {

Again.

>@@ -7051,15 +7005,13 @@ nsReflowTimer* GetFrameTimer(nsIFrame* a
>+    if (IS_TABLE_CELL(parentFrame->GetType())) {

And here.

> void nsTableFrame::DebugReflowDone(nsIFrame* aFrame)
>+    if (IS_TABLE_CELL(aFrame->GetType())) {

And here.

>Index: layout/html/table/src/nsTableRowFrame.cpp
>@@ -224,15 +224,13 @@ nsTableRowFrame::AppendFrames(nsIPresCon
>+    if (IS_TABLE_CELL(childFrame->GetType())) {

>@@ -259,15 +257,13 @@ nsTableRowFrame::InsertFrames(nsIPresCon
>+    if (IS_TABLE_CELL(childFrame->GetType())) {

>@@ -294,15 +290,13 @@ nsTableRowFrame::RemoveFrame(nsIPresCont
>+    if (IS_TABLE_CELL(aOldFrame->GetType())) {

> nsTableRowFrame::GetFirstCell() 
>+    if (IS_TABLE_CELL(childFrame->GetType())) {

>@@ -379,15 +369,13 @@ nsTableRowFrame::DidResize(nsIPresContex
>+    if (IS_TABLE_CELL(childFrame->GetType())) {

>@@ -515,15 +503,13 @@ nsTableRowFrame::CalcHeight(const nsHTML
>+    if (IS_TABLE_CELL(kidFrame->GetType())) {

>@@ -1502,15 +1487,13 @@ nsTableRowFrame::InsertCellFrame(nsTable
>+    if (!IS_TABLE_CELL(child->GetType())) {

And here, all seven times.

>Index: layout/mathml/base/src/nsMathMLmoFrame.cpp
>+nsMathMLmoFrame::GetType() const

Bunch of else-after-return stuff in here....

r+sr=bzbarsky with those nits picked (I could go either way on the
IS_TABLE_CELL thing, so it's your call).

Comment 57

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

Maybe IS_TABLE_CELL should be a function instead, with an nsIFrame* parameter? 
Then decisions on what to inline could be made without changing callers.

Comment 58

[Boris Zbarsky [:bzbarsky]]

Yeah, that's the other option. That'd be nice, in fact.

Comment 59

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

> How about moving the if (atom) check inside the ifdef?
> And hoisting the code that runs in opt builds out of the body of this if?

Good idea

> That code needs to get outdented, right?  Please do that.

OK

> else after return...

OK

> IS_TABLE_CELL is a macro that uses the type more than once... you may want to
> store the type in a temporary here as before.

I think the right thing to do is to make IS_TABLE_CELL an inline function. (Why
ever use a macro when an inline function will do?) However, I'll have it just
take the frame type as a parameter as it does now. That way, any call sites that
already have the type available can just pass it in.

Comment 60

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

Attachment: fix

Basically the same as before, but I added bz's comments, and see the final
change to nsLayoutAtoms.h. Making IS_TABLE_CELL be a simple static inline
function, no name change, is a bit cheezy but it should cause no problems.

Comment 61

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

Checked in the patch. Satisfying Tp decrease.

Comment 62

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

I'm working on converting FirstChild and GetAdditionalChildListName. They become

virtual nsIFrame* GetFirstChild(nsIAtom* aListName);
virtual nsIAtom* GetAdditionalChildListName(PRInt32 aIndex);

The existing FirstChild documents a special error code to distinguish between an
empty child list and a child list that the frame doesn't support ... but so far,
I haven't found anything that actually cares. One can always figure out if the
frame supports a particular child list by iterating with GetAdditionalChildListName.

Comment 63

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

Attachment: patch as described

Comment 64

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

Comment on attachment 134972 [details] [diff] [review]
patch as described

hey bz, another giant patch for you :-).

Comment 65

[Boris Zbarsky [:bzbarsky]]

It'll take me a few days; I have prior review commitments that I'm already way
behind on...

Comment 66

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 134972 [details] [diff] [review]
patch as described

>Index: layout/html/base/src/nsBlockFrame.cpp
>+nsBlockFrame::GetFirstChild(nsIAtom* aListName) const

This function has lots of else-after-returns now.. Is that necessary?

>Index: layout/html/base/src/nsContainerFrame.cpp
>+nsContainerFrame::GetFirstChild(nsIAtom* aListName) const

Same.

>+nsContainerFrame::GetAdditionalChildListName(PRInt32 aIndex) const

Same.

>Index: layout/html/base/src/nsFrame.cpp

> #define HUGE_DISTANCE 999999 //some HUGE number that will always fail first comparison

I know you didn't write this, but how about changing this to PR_INT32_MAX as
long as you're here?

>@@ -3000,22 +2990,18 @@ nsFrame::SetSelected(nsIPresContext* aPr

>+    nsIFrame* kid = GetFirstChild(nsnull);
>+    while (nsnull != kid) {

while (kid) {

>-                result = frame->FirstChild(aPresContext, nsnull,&frame);
>+                frame = frame->GetFirstChild(nsnull);
>                 if (frame && NS_SUCCEEDED(result))
>                 {
>                   result = frame->QueryInterface(NS_GET_IID(nsILineIteratorNavigator),
>                                                             getter_AddRefs(iter));

Again, not your code but how about |iter = do_QueryInterface(frame);| ?

>Index: layout/html/base/src/nsHTMLContainerFrame.cpp

>@@ -376,27 +374,24 @@ ReparentFrameViewTo(nsIPresContext* aPre
>+      nsIFrame* childFrame = aFrame->GetFirstChild(listName);
>       for (; childFrame; childFrame = childFrame->GetNextSibling()) {

Any reason to not move the childFrame initialization into the first clause of
the for() ?

>Index: layout/html/base/src/nsInlineFrame.cpp

>@@ -687,15 +687,13 @@ void MarkPercentAwareFrame(nsIPresContex
>         SetContainsPercentAwareChild(aInline); // if a child container is effected, so am I

Again not your code, but "affected"....

I've made it to the beginning of nsComboboxControlFrame.cpp; more later.

Comment 67

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

Sure, I can fix all those.

Comment 68

[Boris Zbarsky [:bzbarsky]]

I'm going to try to get this review done before freeze, but I'm not sure I can
pull it off.  :(

Comment 69

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

It won't kill me if you don't get to it before freeze.

Comment 70

[Aleksey Nogin]

Comment on attachment 133907 [details] [diff] [review]
fix

This have added two "might be used uninitialized" warnings on brad TBox:

+layout/html/base/src/nsHTMLReflowState.cpp:1709
+ `nsIAtom*fType' might be used uninitialized in this function
++layout/html/base/src/nsPresShell.cpp:4344
+ `nsIAtom*frameType' might be used uninitialized in this function

Comment 71

[Boris Zbarsky [:bzbarsky]]

> ++layout/html/base/src/nsPresShell.cpp:4344
> + `nsIAtom*frameType' might be used uninitialized in this function

This warning is completely bogus as a trivial analysis of the code shows
(frameType not initialized implies aFrame is null implies there are no accesses
to frameType at all).

The other warning is also bogus, but that's not nearly as obvious (same kind of
issue -- the access is protected by an if() condition that, if satisfied, would
have forced the variable being set earlier; just much further apart).  Here it
may make sense to init fType to null....

Comment 72

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 134972 [details] [diff] [review]
patch as described

>Index: layout/html/tests/TestInlineFrame.cpp

>-  for (FirstChild(f); nsnull != f; f->GetNextSibling(f)) {
>+  for (GetFirstChild(f); nsnull != f; f->GetNextSibling(f)) {

That's not the same FirstChild()...  Though I doubt this code builds either
way.

>Index: layout/xul/base/src/nsBoxObject.cpp

I assume we still need mPresShell in that file, even with this change?	But see
comment on mPresContext following.

>Index: layout/xul/base/src/nsMenuBarFrame.cpp

Similar here with mPresContext?

We should probably file a separate bug to move all mPresContext-using frames
over to getting it off the style context....

>Index: layout/xul/base/src/nsMenuFrame.cpp

>+nsMenuFrame::GetFirstChild(nsIAtom* aListName) const

else-after-return in this method.

>Index: layout/xul/base/src/nsPopupBoxObject.cpp

More mPresShell stuff here (separate bug, as above).

r+sr=bzbarsky with that.

Comment 73

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

Thanks for that. Certainly all frames can lose their mPresContext members. I'll
file a bug for that. I'll look into removing mPresShell and file a bug if necessary.

Comment 74

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

I checked this in (after fixing up considerable rot). It seems to have reduced
Tp a little bit. A bit surprising considering the trivialness of the change, but
perhaps it means more nsIFrame* locals can be held in registers.

Comment 75

[Aleksey Nogin]

Comment on attachment 134972 [details] [diff] [review]
patch as described

This checking have added two "may be uninitialized" warnings to brad TBox:

+layout/base/src/nsFrameTraversal.cpp:409
+ `nsIFrame*result' might be used uninitialized in this function
+
+layout/base/src/nsFrameTraversal.cpp:749
+ `nsIFrame*result' might be used uninitialized in this function
+

Comment 76

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

I checked in a fix for those warnings.

Comment 77

[Serge Gautherie (:sgautherie)]

This adding 3 other (actual !) warnings in
<http://tinderbox.mozilla.org/SeaMonkey/warn1073661540.19272.html>:

{
evaughan
1.	layout/xul/base/src/nsBox.cpp:806 (See build log excerpt)
	Unused variable `nsIPresContext*presContext'

804 nsBox::CollapseChild(nsBoxLayoutState& aState, nsIFrame* aFrame, PRBool aHide)
805 {
806       nsIPresContext* presContext = aState.GetPresContext();
807 
808     // shrink the view


karnaze
14.	layout/html/table/src/nsTableRowGroupFrame.cpp:1561 (See build log excerpt)
	Unused variable `nsresult rv'

1559 {
1560   nsTableFrame* tableFrame = nsnull;
1561   nsresult rv = nsTableFrame::GetTableFrame(this, tableFrame);
1562   if (!tableFrame) return 0;


roc+
2.	layout/xul/base/src/nsSplitterFrame.cpp:652 (See build log excerpt)
	Unused variable `nsIFrame*thumbFrame'

650 nsSplitterFrameInner::AddListener(nsIPresContext* aPresContext)
651 {
652   nsIFrame* thumbFrame = mOuter->GetFirstChild(nsnull);
653 
654   nsCOMPtr<nsIDOMEventReceiver>
}

The formers are old code, newly reported;
The later is new code.

Hints: line removal, (void) xxx, variable use...

Thanks.

Comment 78

[Bernd]

Robert, do you know any usage of ::ReplaceFrame other than to be called from ReplaceFrame?

Comment 79

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

I believe whatever LXR says.

Comment 80

[Zack Weinberg (:zwol)]

Should this bug still be open, five years later, with the new QueryFrame scheme that we have?

Comment 81

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

I'm going to claim this is fixed, more work can be done in followups.

Comment 82

[[e-mail]]

Comment on attachment 128431 [details] [diff] [review]
layout/xul changes

>   } else if (aAttribute == nsXULAtoms::acceltext) {
>     // someone reset the accelText attribute, so clear the bit that says *we* set it
>-    nsFrameState state;
>-    GetFrameState(&state);
>-    SetFrameState(state & ~NS_STATE_ACCELTEXT_IS_DERIVED);
>+    AddStateBits(NS_STATE_ACCELTEXT_IS_DERIVED);
This should be RemoveStateBits, no?

Comment 83

[Serge Gautherie (:sgautherie)]

(In reply to comment #82)
> >-    SetFrameState(state & ~NS_STATE_ACCELTEXT_IS_DERIVED);
> >+    AddStateBits(NS_STATE_ACCELTEXT_IS_DERIVED);
> This should be RemoveStateBits, no?

Obviously :-<

That was checked in in
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/layout/xul/base/src/nsMenuFrame.cpp&mark=1.251#1.252
(MOZILLA_1_5b_RELEASE, etc)

Then code was modified in
http://hg.mozilla.org/mozilla-central/rev/e4e653196488
Bug 583957, lazy menuframe update, r=enndeakin, sr=neil, a=dbaron
(FIREFOX_4_0b4_BUILD1)

That's bug 592424 ;->