Last Comment Bug 653649 - Kill nsIFrame::GetAdditionalChildListName
: Kill nsIFrame::GetAdditionalChildListName
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla9
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on: 684574 685069 685113
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-28 21:45 PDT by :Ehsan Akhgari
Modified: 2011-09-07 04:13 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1, implement new frame child list types (13.88 KB, patch)
2011-07-06 12:10 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 2, implement new GetChildList methods (28.55 KB, patch)
2011-07-06 12:10 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 3, fix GetAdditionalChildListName consumers (47.57 KB, patch)
2011-07-06 12:10 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 4, fix nsIAtom* child list consumers (408.77 KB, patch)
2011-07-06 12:10 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
the full patch in case you need it (490.29 KB, patch)
2011-07-06 12:15 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 1, implement new frame child list types (9.94 KB, patch)
2011-07-08 09:55 PDT, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
part 2, implement new GetChildList methods (30.71 KB, patch)
2011-07-08 09:58 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 3, fix GetAdditionalChildListName consumers (47.80 KB, patch)
2011-07-08 09:59 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 4, fix nsIAtom* child list consumers (408.73 KB, patch)
2011-07-08 10:00 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
the full patch in case you need it (488.70 KB, patch)
2011-07-08 10:01 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 1, implement new frame child list types (11.74 KB, patch)
2011-07-09 15:40 PDT, Mats Palmgren (:mats)
roc: review+
dbaron: superreview+
Details | Diff | Splinter Review
part 2, implement new GetChildList methods (31.03 KB, patch)
2011-07-09 15:40 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 3, fix GetAdditionalChildListName consumers (47.83 KB, patch)
2011-07-09 15:40 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 4, fix nsIAtom* child list consumers (408.96 KB, patch)
2011-07-09 15:40 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
the full patch in case you need it (489.94 KB, patch)
2011-07-09 15:40 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 2, implement new GetChildList methods (32.83 KB, patch)
2011-08-02 14:20 PDT, Mats Palmgren (:mats)
roc: review+
dbaron: superreview-
Details | Diff | Splinter Review
part 3, fix GetAdditionalChildListName consumers (46.98 KB, patch)
2011-08-02 14:35 PDT, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
part 4, fix nsIAtom* child list consumers (408.76 KB, patch)
2011-08-02 14:37 PDT, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
the full patch in case you need it (489.77 KB, patch)
2011-08-02 14:38 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 5, kPrincipalList alias in nsCSSFrameConstructor.cpp (31.95 KB, patch)
2011-08-09 17:17 PDT, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
the full patch in case you need it (488.92 KB, patch)
2011-08-09 17:18 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 1, implement new frame child list types (12.08 KB, patch)
2011-08-19 21:07 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 2, implement new GetChildList methods (33.14 KB, patch)
2011-08-19 21:09 PDT, Mats Palmgren (:mats)
dbaron: superreview+
Details | Diff | Splinter Review
part 3, fix GetAdditionalChildListName consumers (47.17 KB, patch)
2011-08-19 21:10 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 4, fix nsIAtom* child list consumers (408.15 KB, patch)
2011-08-19 21:11 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 5, kPrincipalList alias in nsCSSFrameConstructor.cpp (32.16 KB, patch)
2011-08-19 21:13 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
the full patch in case you need it (488.87 KB, patch)
2011-08-19 21:14 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review

Description :Ehsan Akhgari 2011-04-28 21:45:17 PDT
(In reply to bug 10209 comment #37)
> May I pause for a moment to express my discontent at the way
> GetAdditionalChildListName is designed?  The way that it works now, it's
> impossible to figure out what needs to happen in order to change something here
> without looking at all of the definitions of this function.  :(  Why can't each
> implementation just return an array of child list names it supports?

Yes.

Really I think the whole GetAdditionalChildListName dance should be replaced
with something simpler and faster. Right now traversing all the child frame
lists requires two virtual calls per child list the frame supports, which is
way more than necessary.

E.g. we could have
  struct ChildList {
    nsIAtom* mName;
    nsIFrame* mFirstChild;
  };
  /** Appends one ChildList element for every non-empty child list to aLists */
  virtual void GetChildLists(nsTArray<ChildList>* aLists);

then you could write
  nsAutoTArray<ChildList,4> childLists;
  frame->GetChildLists(&childLists);
  for (PRUint32 i = 0; i < childLists.Length(); ++i) {
    for (nsIFrame* f = childLists[i].mFirstChild; f = f->GetNextSibling()) {
      ...
    }
  }

The implementors could write
  void nsXYZFrame::GetChildLists(nsTArray<ChildList>* aLists)
  {
    nsSuperClassFrame::GetChildLists(aLists);
    if (!mMyExtraList.IsEmpty()) {
      aLists->AppendElement(ChildList(nsGkAtoms::myExtraList,
mMyExtraList.FirstFrame());
    }
  }
we could even add a helper function
nsFrameList::AppendIfNonEmpty(nsTArray<ChildList>* aLists, nsIAtom* aListName).
Comment 1 :Ehsan Akhgari 2011-04-28 21:46:47 PDT
We can even avoid the runtime cost of computing the array with some template magics and some helper macros to use them...
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-04-28 22:37:53 PDT
What do you mean by that?
Comment 3 :Ehsan Akhgari 2011-05-01 14:34:12 PDT
(In reply to comment #2)
> What do you mean by that?

I was fantasizing about a future world where we can use more advanced C++ features.  boost MPL has an implementation of what I'm talking about: http://www.boost.org/doc/libs/1_46_1/libs/mpl/doc/refmanual/list.html

But you can ignore me for now, I guess...  :-)
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-01 17:33:15 PDT
That's probably not even desirable, since most frames support some kinds of child lists that are usually empty, so it's better to not allocate space for those entries in the common case.
Comment 5 Mats Palmgren (:mats) 2011-05-29 18:42:31 PDT
I have a fix for this as follows:
* replace nsIAtom* list identifiers with a class with a single
  PRUint32 member, the list number (a unique bit)
* use a bitmask to represent a set of lists (we only have 15 lists)
* replace GetNextSibling() loops with nsFrameList/Enumerator

Enumeration is trivial and fast, no memory allocations involved
except for a couple of PRUint32's on the stack.
It's type safe - no more crashes due to passing an unrelated
nsIAtom* that isn't a list identifier ;-)

A typical child list loop looks likes this:

ChildListEnumerator lists(aFrame);
for (; lists.HasMore(); lists.Next()) {
  nsFrameList::Enumerator childFrames(lists.Current());
  for (; !childFrames.AtEnd(); childFrames.Next()) {
    nsIFrame* child = childFrames.get();
    // do stuff 
  }
}

Child list ids can be or'ed together, and removed from the
enumeration like so:

ChildListEnumerator iter(aFrame,
  aFrame->ChildLists().Exclude(kOverflowChildList |
                               kExcessOverflowContainersChildList |
                               kOverflowOutOfFlowChildList));

What do you think?
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-29 20:43:36 PDT
Sounds good except that you still have one virtual call per list with children, plus another one for aFrame->ChildLists(), right?

My suggestion in comment #0 would let you get all the children with just one virtual call. It would only require memory allocation if you had a single frame with more child lists that could fit in the array, which should almost never happen.
Comment 7 Mats Palmgren (:mats) 2011-05-29 21:12:09 PDT
I have a ChildLists(ChildListSelector::kExcludeEmptyLists) to avoid
that, it returns the set of lists that are non-empty.
The vast majority of call sites use that variant (so I should probably
make that the default).
Comment 8 Mats Palmgren (:mats) 2011-05-29 21:15:10 PDT
... but, yeah, it's still one virtual call per non-empty list
to extract the actual list.  I'll see what I can do about that...
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-29 21:20:56 PDT
(In reply to comment #7)
> I have a ChildLists(ChildListSelector::kExcludeEmptyLists) to avoid
> that, it returns the set of lists that are non-empty.
> The vast majority of call sites use that variant (so I should probably
> make that the default).

Are there any sites that want to see empty child lists? I couldn't think of any.
Comment 10 Mats Palmgren (:mats) 2011-06-06 15:17:39 PDT
Maybe CompareTrees() in nsPresShell.cpp needs it:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#8438
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-06 17:08:03 PDT
That is test-only code and would work fine if we only returned non-empty child lists.
Comment 12 Mats Palmgren (:mats) 2011-07-06 12:10:05 PDT
Created attachment 544299 [details] [diff] [review]
part 1, implement new frame child list types

Add new types in mozilla::layout namespace (files FrameChildList.h/cpp):
  * FrameChildListId to replace nsIAtom* child list names
  * FrameChildListIds - a set of FrameChildListId,
    this is basically a PRUint32 bit mask wrapped in a class
    (FrameChildListId is a sub-class)
  * FrameChildList - a FrameChildListId + nsFrameList
  * FrameChildListEnumerator for enumerating a nsTArray<FrameChildList>
  * AutoFrameChildListEnumerator - a subclass that takes a nsIFrame*
    and enumerates its child lists

These types are not expected to be used directly, instead they are all
typedef'ed in nsIFrame as:
typedef mozilla::layout::FrameChildListIds ChildListIds;
typedef mozilla::layout::FrameChildListEnumerator ChildListEnumerator;
typedef mozilla::layout::AutoFrameChildListEnumerator AutoChildListEnumerator;
typedef mozilla::layout::FrameChildListId ChildListId;
typedef mozilla::layout::FrameChildList ChildList;

and since they're typically used in nsIFrame sub-classes, there's
no need to use the nsIFrame:: prefix either.

(I placed these types outside nsIFrame to avoid bloating it too much)
Comment 13 Mats Palmgren (:mats) 2011-07-06 12:10:16 PDT
Created attachment 544300 [details] [diff] [review]
part 2, implement new GetChildList methods

* GetChildList(nsIAtom* aListName) => GetChildList(ChildListId aListId)
* Replace GetAdditionalChildListName with
  GetChildLists(nsTArray<ChildList>* aLists) that appends all non-empty
  child lists to aLists

The lists are:
  kPrincipalList
  kAbsoluteList
  kBulletList
  kCaptionList
  kColGroupList
  kExcessOverflowContainersList
  kFixedList
  kFloatList
  kOverflowContainersList
  kOverflowList
  kOverflowOutOfFlowList
  kPopupList
  kPushedFloatsList
  kSelectPopupList
  kNoReflowPrincipalList (an alias for kPrincipalList; formerly 'nextBidi')

which follows the nsGkAtoms:: names with an added 'k' prefix, except for
nsnull which becomes kPrincipalList and nsGkAtoms::nextBidi which
becomes kNoReflowPrincipalList.

I've added a convenience method nsIFrame::PrincipalChildList() for 
GetChildList(kPrincipalList) to avoid some typing.
Comment 14 Mats Palmgren (:mats) 2011-07-06 12:10:28 PDT
Created attachment 544301 [details] [diff] [review]
part 3, fix GetAdditionalChildListName consumers

Rewrite the GetAdditionalChildListName consumers.
It's fairly straight-forward.  I typical loop such as:

  PRInt32 listIndex = 0;
  nsIAtom* childList = nsnull;
  do {
    nsIFrame* child = GetFirstChild(childList);
    while (child) {
       r.UnionRect(r, child->ComputeTightBounds(aContext) + child->GetPosition());
       child = child->GetNextSibling();
    }
    childList = GetAdditionalChildListName(listIndex++);
  } while (childList);

becomes:

  AutoChildListEnumerator lists(this);
  for (; lists.HasMore(); lists.Next()) {
    nsFrameList::Enumerator childFrames(lists.Current());
    for (; !childFrames.AtEnd(); childFrames.Next()) {
      nsIFrame* child = childFrames.get();
      r.UnionRect(r, child->ComputeTightBounds(aContext) + child->GetPosition());
    }
  }


In some places we skip certain lists like this:
    ...
    do {
      childListName = aFrame->GetAdditionalChildListName(childListIndex++);
    } while (childListName == nsGkAtoms::popupList);
  } while (childListName);

now it can be done like this instead:
  for (; lists.HasMore(); lists.Next()) {
    if (lists.At() == kPopupList) {
      continue;

or if you have more than one list to skip:
  ChildListIds skip(kPrincipalList | kOverflowList);
  for (; lists.HasMore(); lists.Next()) {
    if (skip.Contains(lists.At())) {


There were a couple of tricky cases:
  in PresShell::FrameNeedsReflow:
        do {
          childListName = f->GetAdditionalChildListName(childListIndex++);
          for (nsIFrame *kid = f->GetFirstChild(childListName); kid;
               kid = kid->GetNextSibling()) {
            kid->MarkIntrinsicWidthsDirty();
            stack.AppendElement(kid);
          }
        } while (childListName);

starting with GetAdditionalChildListName means it'll skip the principal
list and process the other lists first and do the 'nsnull' list last.

and CompareTrees (in nsPresShell.cpp) as mentioned earlier.
Comment 15 Mats Palmgren (:mats) 2011-07-06 12:10:36 PDT
Created attachment 544302 [details] [diff] [review]
part 4, fix nsIAtom* child list consumers

Replace all nsIAtom* child list usage with ChildListId.
(mostly boring mechanical changes)

Remove the nsGkAtoms::*List atoms.
Comment 16 Mats Palmgren (:mats) 2011-07-06 12:15:58 PDT
Created attachment 544308 [details] [diff] [review]
the full patch in case you need it
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-06 15:19:16 PDT
Comment on attachment 544299 [details] [diff] [review]
part 1, implement new frame child list types

Review of attachment 544299 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/FrameChildList.h
@@ +96,5 @@
> +  static const FrameChildListId kOverflowList;
> +  static const FrameChildListId kOverflowOutOfFlowList;
> +  static const FrameChildListId kPopupList;
> +  static const FrameChildListId kPushedFloatsList;
> +  static const FrameChildListId kSelectPopupList;

Why not make the enum public here so callers can compile in the constant values?

In fact I think it might make sense so just expose the enum as FrameChildListId, and let FrameChildListIds be the only class.

@@ +121,5 @@
> +
> +/**
> + * A class for enumerating frame child lists.
> + */
> +class NS_STACK_CLASS FrameChildListEnumerator {

Let's follow FrameConstructionItemList::Iterator:

Call this FrameChildListIterator.

@@ +125,5 @@
> +class NS_STACK_CLASS FrameChildListEnumerator {
> + public:
> +  FrameChildListEnumerator(const nsTArray<FrameChildList>& aLists)
> +    : mLists(aLists), mCurrentIndex(0) {}
> +  bool HasMore() const { return mCurrentIndex < mLists.Length(); }

IsDone()

@@ +126,5 @@
> + public:
> +  FrameChildListEnumerator(const nsTArray<FrameChildList>& aLists)
> +    : mLists(aLists), mCurrentIndex(0) {}
> +  bool HasMore() const { return mCurrentIndex < mLists.Length(); }
> +  FrameChildListId At() const {

CurrentID()

@@ +130,5 @@
> +  FrameChildListId At() const {
> +    NS_ASSERTION(HasMore(), "At(): enumerator at end");
> +    return mLists[mCurrentIndex].mId;
> +  }
> +  const nsFrameList& Current() const {

CurrentList()

@@ +147,5 @@
> +
> +/**
> + * A class for retrieving a frame's child lists and enumerating them.
> + */
> +class NS_STACK_CLASS AutoFrameChildListEnumerator

How about AllFrameChildListIterator?
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-06 15:21:41 PDT
Comment on attachment 544300 [details] [diff] [review]
part 2, implement new GetChildList methods

Review of attachment 544300 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsIFrame.h
@@ +1060,5 @@
> +  static const ChildListId kSelectPopupList;
> +  // An alias for all the above lists.
> +  static const ChildListIds kAllChildLists;
> +  // A special alias for kPrincipalList that do not request reflow
> +  static const ChildListId kNoReflowPrincipalList;

You can redeclare this as another enum whose values are copied from the first enum.
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-06 15:23:27 PDT
Parts 1 and 2 will need sr from dbaron.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-06 17:25:05 PDT
Comment on attachment 544299 [details] [diff] [review]
part 1, implement new frame child list types

Review of attachment 544299 [details] [diff] [review]:
-----------------------------------------------------------------

er, didn't mean to r+ this
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-07 02:34:45 PDT
Comment on attachment 544299 [details] [diff] [review]
part 1, implement new frame child list types

Review of attachment 544299 [details] [diff] [review]:
-----------------------------------------------------------------

er, didn't mean to r+ this

::: layout/generic/FrameChildList.cpp
@@ +117,5 @@
> +} // namespace layout
> +} // namespace mozilla
> +
> +// Aliases provided by nsIFrame for convenience.
> +const mozilla::layout::FrameChildListId nsIFrame::kPrincipalList = mozilla::layout::FrameChildListId::kPrincipalList;

These could go inside the mozilla::layout namespace block, couldn't they?

::: layout/generic/FrameChildList.h
@@ +96,5 @@
> +  static const FrameChildListId kOverflowList;
> +  static const FrameChildListId kOverflowOutOfFlowList;
> +  static const FrameChildListId kPopupList;
> +  static const FrameChildListId kPushedFloatsList;
> +  static const FrameChildListId kSelectPopupList;

I thought about the type-safety argument again. I still think just using an enum is fine. Note that in C++ you can't just pass an integer where the enum is expected, so a call like frame->GetChildList(2) would not compile. "int x = kPrincipalList;" would compile, but I don't think that matters.
Comment 22 :Ms2ger (⌚ UTC+1/+2) 2011-07-07 03:27:42 PDT
Comment on attachment 544299 [details] [diff] [review]
part 1, implement new frame child list types

>--- a/layout/generic/Makefile.in
>+++ b/layout/generic/Makefile.in
> EXPORTS		= \
>+		FrameChildList.h \

EXPORTS_NAMESPACES = mozilla/layout

EXPORTS_mozilla/layout = \
  FrameChildList.h \
  $(NULL)
Comment 23 Mats Palmgren (:mats) 2011-07-08 09:55:18 PDT
Created attachment 544838 [details] [diff] [review]
part 1, implement new frame child list types

(In reply to comment #17)
> In fact I think it might make sense so just expose the enum as
> FrameChildListId, and let FrameChildListIds be the only class.

Ok.  I guess the type-safety enums have is good enough to catch most
mistakes.  (Hopefully we can use C++0x "enum class" soon.)

> How about AllFrameChildListIterator?

I named this one FrameChildListIterator, aka nsIFrame::ChildListIterator,
since it's used most and the other that takes a nsTArray& param
FrameChildListArrayIterator, aka nsIFrame::ChildListArrayIterator.

(In reply to comment #21)
> > +const mozilla::layout::FrameChildListId nsIFrame::kPrincipalList = mozilla::layout::FrameChildListId::kPrincipalList;
> 
> These could go inside the mozilla::layout namespace block, couldn't they?

The issue is moot, but FYI gcc rejected that - something about
nsIFrame:: not being in the enclosing scope.

(In reply to comment #22)
> EXPORTS_NAMESPACES = mozilla/layout

Fixed.
Comment 24 Mats Palmgren (:mats) 2011-07-08 09:58:45 PDT
Created attachment 544841 [details] [diff] [review]
part 2, implement new GetChildList methods

(In reply to comment #18)
> ::: layout/generic/nsIFrame.h
> > +  static const ChildListId kNoReflowPrincipalList;
> 
> You can redeclare this as another enum whose values are copied from the
> first enum.

No, because you can't use the nsIFrame enum values with FrameChildListIds
methods.  But, you can use "integral constant expressions" to initialize
a static const directly in the class declaration, so that's what I did.
I had to workaround a MSVC compiler bug though (it complained about
"multiply defined symbols") by #ifdef'ing out the definition of these
symbols in nsFrame.cpp.
Comment 25 Mats Palmgren (:mats) 2011-07-08 09:59:41 PDT
Created attachment 544842 [details] [diff] [review]
part 3, fix GetAdditionalChildListName consumers
Comment 26 Mats Palmgren (:mats) 2011-07-08 10:00:24 PDT
Created attachment 544843 [details] [diff] [review]
part 4, fix nsIAtom* child list consumers
Comment 27 Mats Palmgren (:mats) 2011-07-08 10:01:11 PDT
Created attachment 544844 [details] [diff] [review]
the full patch in case you need it
Comment 28 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-08 15:53:37 PDT
Comment on attachment 544838 [details] [diff] [review]
part 1, implement new frame child list types

Review of attachment 544838 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that

::: layout/generic/FrameChildList.h
@@ +117,5 @@
> +  FrameChildListArrayIterator(const nsTArray<FrameChildList>& aLists)
> +    : mLists(aLists), mCurrentIndex(0) {}
> +  bool IsDone() const { return mCurrentIndex < mLists.Length(); }
> +  FrameChildListId CurrentID() const {
> +    NS_ASSERTION(IsDone(), "CurrentID(): enumerator at end");

!IsDone(), surely? :-)

@@ +121,5 @@
> +    NS_ASSERTION(IsDone(), "CurrentID(): enumerator at end");
> +    return mLists[mCurrentIndex].mId;
> +  }
> +  const nsFrameList& CurrentList() const {
> +    NS_ASSERTION(IsDone(), "CurrentList(): enumerator at end");

!IsDone()

@@ +125,5 @@
> +    NS_ASSERTION(IsDone(), "CurrentList(): enumerator at end");
> +    return mLists[mCurrentIndex].mList;
> +  }
> +  void Next() {
> +    NS_ASSERTION(IsDone(), "Next(): enumerator at end");

!IsDone()

::: layout/generic/Makefile.in
@@ +74,5 @@
> +EXPORTS_NAMESPACES = mozilla/layout
> +
> +EXPORTS_mozilla/layout = \
> +  FrameChildList.h \
> +  $(NULL)

Align these with tabs like the other lists
Comment 29 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-08 16:00:50 PDT
Comment on attachment 544841 [details] [diff] [review]
part 2, implement new GetChildList methods

Review of attachment 544841 [details] [diff] [review]:
-----------------------------------------------------------------

I think a helper function nsFrameList::AppendIfNonempty(nsTArray<ChildList>* aLists, ChildListId aListName) would be very useful.

::: layout/generic/nsInlineFrame.cpp
@@ +1225,3 @@
>  {
> +  nsInlineFrame::GetChildLists(aLists);
> +  const nsFrameList& list = mAbsoluteContainer.GetChildList();

Add a method
mAbsoluteContainer.AppendChildList(nsTArray<ChildList>* aLists, ChildListId aListName) const
which appends the child list to the array if it's non-empty. Then four lines here (and in other users of nsAbosluteContainingBlock) get reduced to one line.
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-08 16:02:41 PDT
One more thing about part 1: Be consistent about Id vs ID. I strongly prefer ID. So ChildListID etc.
Comment 31 Mats Palmgren (:mats) 2011-07-09 15:40:05 PDT
Created attachment 545030 [details] [diff] [review]
part 1, implement new frame child list types

(In reply to comment #29)
> I think a helper function nsFrameList::AppendIfNonempty(nsTArray<ChildList>*
> aLists, ChildListId aListName) would be very useful.

I had to move the ChildListID enum to nsFrameList.h and implement the method
at the end of FrameChildList.h to solve the circular dependencies.
Comment 32 Mats Palmgren (:mats) 2011-07-09 15:40:20 PDT
Created attachment 545031 [details] [diff] [review]
part 2, implement new GetChildList methods
Comment 33 Mats Palmgren (:mats) 2011-07-09 15:40:28 PDT
Created attachment 545032 [details] [diff] [review]
part 3, fix GetAdditionalChildListName consumers
Comment 34 Mats Palmgren (:mats) 2011-07-09 15:40:38 PDT
Created attachment 545033 [details] [diff] [review]
part 4, fix nsIAtom* child list consumers
Comment 35 Mats Palmgren (:mats) 2011-07-09 15:40:49 PDT
Created attachment 545034 [details] [diff] [review]
the full patch in case you need it
Comment 36 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-10 03:30:49 PDT
Comment on attachment 545030 [details] [diff] [review]
part 1, implement new frame child list types

Review of attachment 545030 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 37 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-10 03:45:31 PDT
Comment on attachment 545031 [details] [diff] [review]
part 2, implement new GetChildList methods

Review of attachment 545031 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/forms/nsComboboxControlFrame.cpp
@@ +1249,5 @@
> +void
> +nsComboboxControlFrame::GetChildLists(nsTArray<ChildList>* aLists) const
> +{
> +  nsBlockFrame::GetChildLists(aLists);
> +  mPopupFrames.AppendIfNonempty(aLists, kSelectPopupList);

Should call the superclass method last if possible (as it is here), because that could be optimized as a tail call.

::: layout/generic/nsBlockFrame.cpp
@@ +587,2 @@
>  {
> +  if (aListID == kPrincipalList) {

We should use 'switch' now!

@@ +622,2 @@
>  {
> +  nsContainerFrame::GetChildLists(aLists);

As above, put the superclass call last.

@@ +624,5 @@
> +  nsLineList* overflowLines = GetOverflowLines();
> +  if (overflowLines && overflowLines->front()->mFirstChild) {
> +    nsFrameList overflowList(overflowLines->front()->mFirstChild,
> +                             overflowLines->back()->LastChild());
> +    overflowList.AppendIfNonempty(aLists, kOverflowList);

Isn't this guaranteed to be nonempty?

@@ +628,5 @@
> +    overflowList.AppendIfNonempty(aLists, kOverflowList);
> +  }
> +  const nsFrameList* list = GetOverflowOutOfFlows();
> +  if (list) {
> +    list->AppendIfNonempty(aLists, kOverflowOutOfFlowList);

And this?

@@ +633,5 @@
> +  }
> +  mFloats.AppendIfNonempty(aLists, kFloatList);
> +  if (HaveOutsideBullet()) {
> +    nsFrameList bullet(mBullet, mBullet);
> +    bullet.AppendIfNonempty(aLists, kBulletList);

And this?

@@ +638,5 @@
> +  }
> +  mAbsoluteContainer.AppendChildList(aLists, kAbsoluteList);
> +  list = GetPushedFloats();
> +  if (list) {
> +    list->AppendIfNonempty(aLists, kPushedFloatsList);

And this?

::: layout/generic/nsContainerFrame.cpp
@@ +298,4 @@
>  {
>    // We only know about the unnamed principal child list and the overflow
>    // lists
> +  if (aListID == kPrincipalList) {

switch

@@ +330,5 @@
> +{
> +  nsFrameList* list = static_cast<nsFrameList*>(
> +    aPropTable->Get(aFrame, aProperty));
> +  if (list) {
> +    list->AppendIfNonempty(aLists, aListID);

These are guaranteed to be nonempty, right?

::: layout/generic/nsFrame.cpp
@@ +947,3 @@
>  {
>    return nsFrameList::EmptyList();
>  }

Let's make this inline in nsFrame.h so it can be inlined by subclasses that call it.

@@ +950,5 @@
>  
> +void
> +nsFrame::GetChildLists(nsTArray<ChildList>* aLists) const
> +{
> +  // empty

Ditto.
Comment 38 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-10 04:24:40 PDT
Comment on attachment 545032 [details] [diff] [review]
part 3, fix GetAdditionalChildListName consumers

Review of attachment 545032 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/FrameLayerBuilder.cpp
@@ +1926,5 @@
> +      }
> +    } else {
> +      for (; !childFrames.AtEnd(); childFrames.Next()) {
> +        if (InternalInvalidateThebesLayersInSubtree(childFrames.get())) {
> +          foundContainerLayer = PR_TRUE;

Can't we avoid the code duplication here by overwriting childFrames? Or making it a pointer or something?

::: layout/base/nsPresShell.cpp
@@ +3602,5 @@
>            }
>          }
>  
> +        // Process all the supported non-principal child lists first,
> +        // then the principal child list last.

I don't think this order actually matters here. Do you?
Comment 39 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-10 04:25:47 PDT
(In reply to comment #37)
> ::: layout/forms/nsComboboxControlFrame.cpp
> @@ +1249,5 @@
> > +void
> > +nsComboboxControlFrame::GetChildLists(nsTArray<ChildList>* aLists) const
> > +{
> > +  nsBlockFrame::GetChildLists(aLists);
> > +  mPopupFrames.AppendIfNonempty(aLists, kSelectPopupList);
> 
> Should call the superclass method last if possible (as it is here), because
> that could be optimized as a tail call.

I guess that would change the list iteration order, which would be unnecessary risk, so don't bother.
Comment 40 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-10 04:30:23 PDT
I think in part 4 it's worth having a method nsIFrame::GetFirstPrincipalChild() (not 100% sure that's the best name).
Comment 41 Mats Palmgren (:mats) 2011-07-10 05:06:18 PDT
(In reply to comment #37)
> > +  if (overflowLines && overflowLines->front()->mFirstChild) {
> > +    nsFrameList overflowList(overflowLines->front()->mFirstChild,
> > +                             overflowLines->back()->LastChild());
> > +    overflowList.AppendIfNonempty(aLists, kOverflowList);
> 
> Isn't this guaranteed to be nonempty?

It is.  I intentionally wanted to use AppendIfNonempty everywhere
for consistency and simplicity.  In this case, the compiler can
probably see that the NotEmpty() is always true.

> > +  const nsFrameList* list = GetOverflowOutOfFlows();
> > +  if (list) {
> > +    list->AppendIfNonempty(aLists, kOverflowOutOfFlowList);
> 
> And this?

No, I'm pretty sure I've seen empty overflow lists recently.

> > +    nsFrameList bullet(mBullet, mBullet);
> > +    bullet.AppendIfNonempty(aLists, kBulletList);
> 
> And this?

We have asserts to that effect, yes.  Is it really worth it though?

> > +  nsFrameList* list = static_cast<nsFrameList*>(
> > +    aPropTable->Get(aFrame, aProperty));
> > +  if (list) {
> > +    list->AppendIfNonempty(aLists, aListID);
> 
> These are guaranteed to be nonempty, right?

I wouldn't bet on those, but I can investigate if you think
it's worth it.
Comment 42 Mats Palmgren (:mats) 2011-07-10 05:07:41 PDT
(In reply to comment #38)
> > +        // Process all the supported non-principal child lists first,
> > +        // then the principal child list last.
> 
> I don't think this order actually matters here. Do you?

I *know* order matters here.  It caused regression tests to fail
when I accidentally changed it.
Comment 43 Mats Palmgren (:mats) 2011-07-10 05:10:48 PDT
(In reply to comment #40)
> I think in part 4 it's worth having a method
> nsIFrame::GetFirstPrincipalChild() (not 100% sure that's the best name).

I can't think of a better name...  but it doesn't add much convenience
over GetFirstChild(kPrincipalList) in frame classes.
In non-frame classes it is shorter since you don't have to qualify
nsIFrame::kPrincipalList.  An alternative might be to add a static const
alias for kPrincipalList where it's motivated (nsCSSFrameConstructor
and nsLayoutUtils perhaps?) to avoid nsIFrame::
Comment 44 Mats Palmgren (:mats) 2011-07-10 05:14:25 PDT
An inline nsIFrame::GetFirstPrincipalChild() is pretty cheap though,
so let's do that...
Comment 45 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-10 05:34:59 PDT
(In reply to comment #41)
> I wouldn't bet on those, but I can investigate if you think
> it's worth it.

Don't worry about it.
Comment 46 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-10 05:41:21 PDT
(In reply to comment #42)
> I *know* order matters here.  It caused regression tests to fail
> when I accidentally changed it.

That seems surprising, since we're just marking stuff dirty. Do you know any more about why they failed?
Comment 47 Boris Zbarsky [:bz] 2011-07-10 08:33:40 PDT
Hmm.  So the do/while loop there ended up with an implicit iteration for the principal list at the end, huh?  I agree it's a little weird that the order there matters....
Comment 48 Mats Palmgren (:mats) 2011-08-02 14:20:56 PDT
Created attachment 550201 [details] [diff] [review]
part 2, implement new GetChildList methods

(In reply to comment #37)
> > +  if (aListID == kPrincipalList) {
> 
> We should use 'switch' now!

Fixed

> ::: layout/generic/nsFrame.cpp
> @@ +947,3 @@
> >  {
> >    return nsFrameList::EmptyList();
> >  }
> 
> Let's make this inline in nsFrame.h so it can be inlined by subclasses that
> call it.

The only subclass affected is nsContainerFrame.  It didn't call the
super-class at all, just returned nsFrameList::EmptyList() directly.
I changed that so that it does call the super-class and moved
nsFrame::GetChildList[s] to the header.  I think it will be inlined
even though there's a class in between (that doesn't implement it).


(In reply to comment #40)
> I think in part 4 it's worth having a method
> nsIFrame::GetFirstPrincipalChild() (not 100% sure that's the best name).

Fixed.
Comment 49 Mats Palmgren (:mats) 2011-08-02 14:35:02 PDT
Created attachment 550215 [details] [diff] [review]
part 3, fix GetAdditionalChildListName consumers

(In reply to comment #38)
> ::: layout/base/FrameLayerBuilder.cpp
> > +        if (InternalInvalidateThebesLayersInSubtree(childFrames.get())) {
> > +          foundContainerLayer = PR_TRUE;
> 
> Can't we avoid the code duplication here by overwriting childFrames? Or
> making it a pointer or something?

Yes, my code was bad, sorry.  It also contained a subtle bug
since the old code depends on doing something for an empty
principal list, which the new code would not see in the loop.
So I've added an outer loop like so:

  nsIFrame* frame = aFrame;
  while (frame) {
    nsIFrame::ChildListIterator lists(frame);
    for (; !lists.IsDone(); lists.Next()) {
      nsFrameList::Enumerator childFrames(lists.CurrentList());
      for (; !childFrames.AtEnd(); childFrames.Next()) {
        if (InternalInvalidateThebesLayersInSubtree(childFrames.get())) {
          foundContainerLayer = PR_TRUE;
        }
      }
    }
    if (frame == aFrame && !frame->GetFirstPrincipalChild()) {
      nsSubDocumentFrame* subdocumentFrame = do_QueryFrame(frame);
      if (subdocumentFrame) {
        // Descend into the subdocument
        frame = subdocumentFrame->GetSubdocumentRootFrame();
        continue;
      }
    }
    break;
  }

This changes the order slightly, but I don't think it matters for this
function.

So, I reviewed all consumers again for similar problems with
doing stuff with an empty child list, and found one more:
https://bugzilla.mozilla.org/attachment.cgi?id=545032&action=diff#a/layout/base/nsLayoutUtils.cpp_sec1
My suggested code would not do the "childList == nsnull && blockFrame"
part when the principal list is empty.
I've now moved that part outside the loop like so:

    nsIFrame::ChildListIDs skip(nsIFrame::kOverflowList |
                                nsIFrame::kExcessOverflowContainersList |
                                nsIFrame::kOverflowOutOfFlowList);
    nsBlockFrame* blockFrame = GetAsBlock(aFrame);
    if (blockFrame) {
      contentBottom =
        NS_MAX(contentBottom, CalculateBlockContentBottom(blockFrame));
      skip |= nsIFrame::kPrincipalList;
    }
    nsIFrame::ChildListIterator lists(aFrame);
    for (; !lists.IsDone(); lists.Next()) {
      if (!skip.Contains(lists.CurrentID())) {
        nsFrameList::Enumerator childFrames(lists.CurrentList()); 
        for (; !childFrames.AtEnd(); childFrames.Next()) {
          ... as before
        }
      }
    }


(In reply to comment #46 and comment #47)
> Hmm.  So the do/while loop there ended up with an implicit iteration for the
> principal list at the end, huh?

Correct.

> it's a little weird that the order there matters....

Mea culpa.  I introduced a bug in one of my early patches and then
subsequently misunderstood the nature of the problem...
There's *no* need to process the principal list last in
PresShell::FrameNeedsReflow - a straight forward iteration is fine.
Comment 50 Mats Palmgren (:mats) 2011-08-02 14:37:31 PDT
Created attachment 550217 [details] [diff] [review]
part 4, fix nsIAtom* child list consumers

(In reply to comment #40)
> I think in part 4 it's worth having a method
> nsIFrame::GetFirstPrincipalChild() (not 100% sure that's the best name).

Fixed.
Comment 51 Mats Palmgren (:mats) 2011-08-02 14:38:50 PDT
Created attachment 550218 [details] [diff] [review]
the full patch in case you need it
Comment 52 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-02 16:02:07 PDT
Comment on attachment 550201 [details] [diff] [review]
part 2, implement new GetChildList methods

Review of attachment 550201 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that fixed.

::: layout/generic/nsCanvasFrame.cpp
@@ +222,5 @@
>  {
>    if (CANVAS_ABS_POS_CHILD_LIST == aIndex)
>      return nsGkAtoms::absoluteList;
>  
>    return nsHTMLContainerFrame::GetAdditionalChildListName(aIndex);

Why aren't you removing GetAdditionalChildListName here? I don't know how this even compiles.
Comment 53 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-02 16:25:00 PDT
Comment on attachment 550215 [details] [diff] [review]
part 3, fix GetAdditionalChildListName consumers

Review of attachment 550215 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that

::: layout/base/FrameLayerBuilder.cpp
@@ +1918,5 @@
> +        }
> +      }
> +    }
> +    if (frame == aFrame && !frame->GetFirstPrincipalChild()) {
> +      nsSubDocumentFrame* subdocumentFrame = do_QueryFrame(frame);

I think we could get rid of the 'if' and just do the do_QueryFrame. The 'if' was just an optimization but now it's not really an optimization since the GetFirstPrincipalChild call is probably as expensive as the do_QueryFrame.

We could put the optimization back by having the loop through 'lists' record if we found a principal-list, but it's probably not worth it.
Comment 54 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-02 16:53:15 PDT
Comment on attachment 550217 [details] [diff] [review]
part 4, fix nsIAtom* child list consumers

Review of attachment 550217 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with or without that.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +10348,5 @@
>           textContent.get(), textFrame, newTextFrame);
>  #endif
>  
>    // Remove placeholder frame and the float
> +  aFrameManager->RemoveFrame(nsIFrame::kPrincipalList, placeholderFrame);

Might be worth importing kPrincipalList into nsCSSFrameConstructor so you don't need nsIFrame:: qualifiers all over the place.
Comment 55 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-02 16:54:49 PDT
(In reply to comment #52)
> Why aren't you removing GetAdditionalChildListName here? I don't know how
> this even compiles.

I see, you remove it in the other patch.
Comment 56 Mats Palmgren (:mats) 2011-08-09 17:17:21 PDT
Created attachment 551957 [details] [diff] [review]
part 5, kPrincipalList alias in nsCSSFrameConstructor.cpp

> Might be worth importing kPrincipalList into nsCSSFrameConstructor so you
> don't need nsIFrame:: qualifiers all over the place.

Ok, this patch replaces the 50 nsIFrame::kPrincipalList with a local
static kPrincipalList.  It might be a little confusing to see an unprefixed
kPrincipalList near other (prefixed) list IDs but it's probably worth the
convenience.
Comment 57 Mats Palmgren (:mats) 2011-08-09 17:18:12 PDT
Created attachment 551958 [details] [diff] [review]
the full patch in case you need it
Comment 58 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-08-16 10:50:10 PDT
Comment on attachment 545030 [details] [diff] [review]
part 1, implement new frame child list types

>+// Returns a literal string, do not free or modify it.

I'm not sure what "literal string" is; perhaps you mean "string literal", though that doesn't quite fit since a string literal is syntax rather than semantics.

That said, I think |const char*| is clear enough and you don't need the comment.

>+class NS_STACK_CLASS FrameChildListArrayIterator {

I tend to call things like this Enumerators rather than Iterators, but I think I've lost that battle.

sr=dbaron
Comment 59 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-08-16 12:12:37 PDT
Comment on attachment 545030 [details] [diff] [review]
part 1, implement new frame child list types

>Bug 653649 - Kill nsIFrame::GetAdditionalChildListName.  r=roc

Also, please use a commit message that describes what the patch does.  Five identical commit messages for five different patches is extremely bad.  I suggest:

>Add types to represent identifiers for frame child lists, sets of those identifiers, and iterators over the child lists of a frame.  (Bug 653649, patch 1)  r=roc  sr=dbaron
>
>Define a enumeration type FrameChildListID representing all of the types
>of child lists that frames have (each with a unique bit), a class
>FrameChildListIDs for representing any set of FrameChildListID, and a
>class FrameChildListIterator for iterating over all of the child lists
>of a frame.
Comment 60 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-08-16 12:15:42 PDT
Comment on attachment 550201 [details] [diff] [review]
part 2, implement new GetChildList methods

>Bug 653649 - Kill nsIFrame::GetAdditionalChildListName.  r=roc

Please don't request review or superreview without a commit message describing what the patch does.
Comment 61 Mats Palmgren (:mats) 2011-08-19 21:07:53 PDT
Created attachment 554605 [details] [diff] [review]
part 1, implement new frame child list types

Fixed the nit by removing the comment.  Updated the commit message.
Comment 62 Mats Palmgren (:mats) 2011-08-19 21:09:49 PDT
Created attachment 554606 [details] [diff] [review]
part 2, implement new GetChildList methods

Updated the commit message.
Comment 63 Mats Palmgren (:mats) 2011-08-19 21:10:55 PDT
Created attachment 554607 [details] [diff] [review]
part 3, fix GetAdditionalChildListName consumers

Updated the commit message.
Comment 64 Mats Palmgren (:mats) 2011-08-19 21:11:52 PDT
Created attachment 554608 [details] [diff] [review]
part 4, fix nsIAtom* child list consumers

Updated the commit message.
Comment 65 Mats Palmgren (:mats) 2011-08-19 21:13:07 PDT
Created attachment 554609 [details] [diff] [review]
part 5, kPrincipalList alias in nsCSSFrameConstructor.cpp

Updated the commit message.
Comment 66 Mats Palmgren (:mats) 2011-08-19 21:14:16 PDT
Created attachment 554610 [details] [diff] [review]
the full patch in case you need it
Comment 67 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-08-20 19:06:48 PDT
Comment on attachment 554606 [details] [diff] [review]
part 2, implement new GetChildList methods

>Bug 653649 - Add types to represent identifiers for frame child lists, sets of those identifiers, and iterators over the child lists of a frame.  (part 2/5) r=roc sr=dbaron

Why are you including the description of part 1 of the patch in the 
commit message for part 2?  (Same for parts 3, 4, and 5.)

>Implement GetChildList(ChildListID) and GetChildLists(nsTArray<ChildList>*)
>for various frame classes.  Remove GetAdditionalChildListName(PRInt32)
>methods and associated macros and list index constants.

Instead, you should lead with this (or a slightly shorter version of it)
on one line as the commit message.  This explains what the patch does.  (You could perhaps even prefix all of the messages with "new way of getting child lists from frames:" or something like that that describes the bug.)


It seems a little odd to me that the list constants are in both mozilla::layout:: and nsIFrame::.  Might it be better to have them only in nsIFrame?

In any case, sr=dbaron given an appropriate commit message.
Comment 68 Mats Palmgren (:mats) 2011-08-24 14:06:30 PDT
> Why are you including the description of part 1 of the patch ...

Sorry, I misunderstood what you said in comment 59 then.

> It seems a little odd to me that the list constants are in both
> mozilla::layout:: and nsIFrame::.  Might it be better to have them only in
> nsIFrame?

I tried that, but it didn't work out.  Moving the enum to nsIFrame required moving
FrameChildList and FrameChildListIDs classes as well since they depend on it
being defined.  With FrameChildList in nsIFrame.h there is a circular dependency
with nsFrameList.h, the nsTArray<FrameChildList> arg to AppendIfNonempty
requires FrameChildList to be defined...

Note You need to log in before you can comment on or make changes to this bug.