Status

()

defect
P3
normal
RESOLVED FIXED
17 years ago
5 years ago

People

(Reporter: roc, Unassigned)

Tracking

(Blocks 3 bugs, {perf})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fix])

Attachments

(7 attachments, 3 obsolete attachments)

80.62 KB, patch
Details | Diff | Splinter Review
17.52 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
5.85 KB, patch
roc
: review+
Details | Diff | Splinter Review
166.65 KB, patch
Details | Diff | Splinter Review
104.93 KB, patch
Details | Diff | Splinter Review
fix
276.47 KB, patch
Details | Diff | Splinter Review
194.61 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
I have a patch and a strategy for deCOMifying nsIFrame. I will explain in a moment.
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).
I had to add some 'virtual's back in to fix linking problems. Too bad editor
likes to monkey around with nsIFrame :-(.
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?
already_AddRefed... neat

OK, I'll use MoveTo/SizeTo.
*** Bug 187924 has been marked as a duplicate of this bug. ***
Posted patch improved patch (obsolete) — Splinter Review
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.
Attachment #112715 - Attachment is obsolete: true
Changes SetFrameState/GetFrameState to
AddStateBits/RemoveStateBits/GetStateBits. adds nsresult to Init() because some
initializations really can fail and we need to detect them.
Attachment #113102 - Attachment is obsolete: true
Attachment #113155 - Flags: superreview?(bzbarsky)
Attachment #113155 - Flags: review?(dbaron)
-   * 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...
> 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?
roc, is this a priority for 1.4a?  Or is it ok to start this in 1.4b?  (just
trying to prioritize reviews....)
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 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. ;)
> 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.
> 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.

I also agree with bz about returning weak pointers where possible.
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.)
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.
Attachment #113155 - Flags: superreview?(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.)
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.
As long as we're careful not to call that stuff as content is being removed from
the document (think removeChild)....
Comment on attachment 113155 [details] [diff] [review]
another iteration

Need another rev of this patch
Attachment #113155 - Flags: review?(dbaron)
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?
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?
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.
For reflow methods we can stick an mPresContext in the reflow state.

Updated

16 years ago
Keywords: perf

Updated

16 years ago
Blocks: 203448
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...
Hey yeah! That rules! aPresContext BEGONE!
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.
Attachment #126101 - Flags: superreview?(dbaron)
Attachment #126101 - Flags: review?(dbaron)
Hmm. Maybe methods like GetClosestView, GetParentWithView, and
AnyAncestorVisible should be moved to nsLayoutUtils. This would be a good time
to do it. Any thoughts?
What would be the benefits of doing so?
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.
Yeah, ok.  That makes sense.
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.
Attachment #126101 - Flags: superreview?(dbaron)
Attachment #126101 - Flags: superreview+
Attachment #126101 - Flags: review?(dbaron)
Attachment #126101 - Flags: review+
> (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!
> 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...)
> 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).
After bug 206682, I have very little faith in the ability of compilers to
optimize code the way I think they ought to.
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?
I checked in the patch with nsRect GetRect(). Changing it to const nsRect& in
the future will be easy if that's 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.
OK, now I've done everything under view/, layout/html/, and layout/base/. Next
up, layout/xul/, layout/svg/ and layout/mathml/.
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.
Should I stop using the rubberstamp and go back to normal review?
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?
I reviewed it myself, caught a couple of errors, and checked it in.
I just checked in the changes for content/.
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.
Attachment #127609 - Flags: superreview?(roc)
Attachment #127609 - Flags: review?(roc)
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.
Attachment #127609 - Flags: superreview?(roc)
Attachment #127609 - Flags: superreview+
Attachment #127609 - Flags: review?(roc)
Attachment #127609 - Flags: review+
Checked in both of those patches and fixed bustage. woohoo.
FYI, I'm now working up a patch to convert GetFrameType to

NS_IMETHOD_(nsIAtom*) GetType();
Posted patch fix GetFrameType() -> GetType (obsolete) — Splinter Review
As promised. This is a simple one; the signature changes in nsIFrame.h to
"virtual nsIAtom* GetType()" and everything else follows from that.
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 :-)
Attachment #133872 - Flags: superreview?(bzbarsky)
Attachment #133872 - Flags: review?(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).
Attachment #133872 - Flags: superreview?(bzbarsky)
Attachment #133872 - Flags: superreview+
Attachment #133872 - Flags: review?(bzbarsky)
Attachment #133872 - Flags: review+
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.
Yeah, that's the other option. That'd be nice, in fact.
> 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.
Posted patch fixSplinter Review
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.
Attachment #133872 - Attachment is obsolete: true
Checked in the patch. Satisfying Tp decrease.
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 on attachment 134972 [details] [diff] [review]
patch as described

hey bz, another giant patch for you :-).
Attachment #134972 - Flags: superreview?(bz-vacation)
Attachment #134972 - Flags: review?(bz-vacation)
It'll take me a few days; I have prior review commitments that I'm already way
behind on...
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.
Sure, I can fix all those.
I'm going to try to get this review done before freeze, but I'm not sure I can
pull it off.  :(
It won't kill me if you don't get to it before freeze.

Comment 70

16 years ago
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
> ++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 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.
Attachment #134972 - Flags: superreview?(bz-vacation)
Attachment #134972 - Flags: superreview+
Attachment #134972 - Flags: review?(bz-vacation)
Attachment #134972 - Flags: review+
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.
Priority: -- → P1
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

16 years ago
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
+
I checked in a fix for those warnings.
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.
Priority: P1 → P3

Comment 78

14 years ago
Robert, do you know any usage of ::ReplaceFrame other than to be called from ReplaceFrame?
I believe whatever LXR says.

Updated

12 years ago
Depends on: 396185
QA Contact: ian → layout
Blocks: deCOM
Should this bug still be open, five years later, with the new QueryFrame scheme that we have?
Assignee: roc → nobody
I'm going to claim this is fixed, more work can be done in followups.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
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?
(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 ;->
You need to log in before you can comment on or make changes to this bug.