Closed Bug 653649 Opened 13 years ago Closed 13 years ago

Kill nsIFrame::GetAdditionalChildListName

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: ehsan.akhgari, Assigned: MatsPalmgren_bugz)

References

Details

Attachments

(6 files, 21 obsolete files)

12.08 KB, patch
Details | Diff | Splinter Review
33.14 KB, patch
dbaron
: superreview+
Details | Diff | Splinter Review
47.17 KB, patch
Details | Diff | Splinter Review
408.15 KB, patch
Details | Diff | Splinter Review
32.16 KB, patch
Details | Diff | Splinter Review
488.87 KB, patch
Details | Diff | Splinter Review
(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).
We can even avoid the runtime cost of computing the array with some template magics and some helper macros to use them...
(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...  :-)
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.
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?
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.
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).
... 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...
(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.
Maybe CompareTrees() in nsPresShell.cpp needs it:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#8438
Assignee: nobody → matspal
OS: Mac OS X → All
Hardware: x86 → All
That is test-only code and would work fine if we only returned non-empty child lists.
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)
Attachment #544299 - Flags: review?(roc)
* 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.
Attachment #544300 - Flags: review?(roc)
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.
Attachment #544301 - Flags: review?(roc)
Replace all nsIAtom* child list usage with ChildListId.
(mostly boring mechanical changes)

Remove the nsGkAtoms::*List atoms.
Attachment #544302 - Flags: review?(roc)
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?
Attachment #544299 - Flags: review?(roc) → review+
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.
Parts 1 and 2 will need sr from dbaron.
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
Attachment #544299 - Flags: review+
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 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)
(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.
Attachment #544299 - Attachment is obsolete: true
Attachment #544838 - Flags: superreview?(dbaron)
Attachment #544838 - Flags: review?(roc)
(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.
Attachment #544300 - Attachment is obsolete: true
Attachment #544300 - Flags: review?(roc)
Attachment #544841 - Flags: superreview?(dbaron)
Attachment #544841 - Flags: review?(roc)
Attachment #544301 - Attachment is obsolete: true
Attachment #544301 - Flags: review?(roc)
Attachment #544842 - Flags: review?(roc)
Attachment #544302 - Attachment is obsolete: true
Attachment #544302 - Flags: review?(roc)
Attachment #544843 - Flags: review?(roc)
Attachment #544308 - Attachment is obsolete: true
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 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.
One more thing about part 1: Be consistent about Id vs ID. I strongly prefer ID. So ChildListID etc.
(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.
Attachment #544838 - Attachment is obsolete: true
Attachment #544838 - Flags: superreview?(dbaron)
Attachment #545030 - Flags: superreview?(dbaron)
Attachment #545030 - Flags: review?(roc)
Attachment #544841 - Attachment is obsolete: true
Attachment #544841 - Flags: superreview?(dbaron)
Attachment #544841 - Flags: review?(roc)
Attachment #545031 - Flags: superreview?(dbaron)
Attachment #545031 - Flags: review?(roc)
Attachment #544842 - Attachment is obsolete: true
Attachment #544842 - Flags: review?(roc)
Attachment #545032 - Flags: review?(roc)
Attachment #544843 - Attachment is obsolete: true
Attachment #544843 - Flags: review?(roc)
Attachment #545033 - Flags: review?(roc)
Attachment #544844 - Attachment is obsolete: true
Comment on attachment 545030 [details] [diff] [review]
part 1, implement new frame child list types

Review of attachment 545030 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #545030 - Flags: review?(roc) → review+
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 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?
(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.
I think in part 4 it's worth having a method nsIFrame::GetFirstPrincipalChild() (not 100% sure that's the best name).
(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.
(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.
(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::
An inline nsIFrame::GetFirstPrincipalChild() is pretty cheap though,
so let's do that...
(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.
(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?
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....
(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.
Attachment #545031 - Attachment is obsolete: true
Attachment #545031 - Flags: superreview?(dbaron)
Attachment #545031 - Flags: review?(roc)
Attachment #550201 - Flags: superreview?(dbaron)
Attachment #550201 - Flags: review?(roc)
(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.
Attachment #545032 - Attachment is obsolete: true
Attachment #545032 - Flags: review?(roc)
Attachment #550215 - Flags: review?(roc)
(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.
Attachment #545033 - Attachment is obsolete: true
Attachment #545033 - Flags: review?(roc)
Attachment #550217 - Flags: review?(roc)
Attachment #545034 - Attachment is obsolete: true
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.
Attachment #550201 - Flags: review?(roc) → review+
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.
Attachment #550215 - Flags: review?(roc) → review+
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.
Attachment #550217 - Flags: review?(roc) → review+
(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.
> 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.
Attachment #551957 - Flags: review?(roc)
Attachment #550218 - Attachment is obsolete: true
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
Attachment #545030 - Flags: superreview?(dbaron) → superreview+
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 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.
Attachment #550201 - Flags: superreview?(dbaron) → superreview-
Fixed the nit by removing the comment.  Updated the commit message.
Attachment #545030 - Attachment is obsolete: true
Updated the commit message.
Attachment #550201 - Attachment is obsolete: true
Attachment #554606 - Flags: superreview?(dbaron)
Updated the commit message.
Attachment #550215 - Attachment is obsolete: true
Updated the commit message.
Attachment #550217 - Attachment is obsolete: true
Updated the commit message.
Attachment #551957 - Attachment is obsolete: true
Attachment #551958 - Attachment is obsolete: true
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.
Attachment #554606 - Flags: superreview?(dbaron) → superreview+
> 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...
Depends on: 684574
Depends on: 685113
You need to log in before you can comment on or make changes to this bug.