Closed
Bug 653649
Opened 13 years ago
Closed 13 years ago
Kill nsIFrame::GetAdditionalChildListName
Categories
(Core :: Layout, defect)
Core
Layout
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).
Reporter | ||
Comment 1•13 years ago
|
||
We can even avoid the runtime cost of computing the array with some template magics and some helper macros to use them...
What do you mean by that?
Reporter | ||
Comment 3•13 years ago
|
||
(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.
Assignee | ||
Comment 5•13 years ago
|
||
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.
Assignee | ||
Comment 7•13 years ago
|
||
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).
Assignee | ||
Comment 8•13 years ago
|
||
... 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.
Assignee | ||
Comment 10•13 years ago
|
||
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.
Assignee | ||
Comment 12•13 years ago
|
||
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)
Assignee | ||
Comment 13•13 years ago
|
||
* 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)
Assignee | ||
Comment 14•13 years ago
|
||
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)
Assignee | ||
Comment 15•13 years ago
|
||
Replace all nsIAtom* child list usage with ChildListId. (mostly boring mechanical changes) Remove the nsGkAtoms::*List atoms.
Attachment #544302 -
Flags: review?(roc)
Assignee | ||
Comment 16•13 years ago
|
||
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 22•13 years ago
|
||
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)
Assignee | ||
Comment 23•13 years ago
|
||
(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)
Assignee | ||
Comment 24•13 years ago
|
||
(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)
Assignee | ||
Comment 25•13 years ago
|
||
Attachment #544301 -
Attachment is obsolete: true
Attachment #544301 -
Flags: review?(roc)
Attachment #544842 -
Flags: review?(roc)
Assignee | ||
Comment 26•13 years ago
|
||
Attachment #544302 -
Attachment is obsolete: true
Attachment #544302 -
Flags: review?(roc)
Attachment #544843 -
Flags: review?(roc)
Assignee | ||
Comment 27•13 years ago
|
||
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
Attachment #544838 -
Flags: review?(roc) → review+
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.
Assignee | ||
Comment 31•13 years ago
|
||
(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)
Assignee | ||
Comment 32•13 years ago
|
||
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)
Assignee | ||
Comment 33•13 years ago
|
||
Attachment #544842 -
Attachment is obsolete: true
Attachment #544842 -
Flags: review?(roc)
Attachment #545032 -
Flags: review?(roc)
Assignee | ||
Comment 34•13 years ago
|
||
Attachment #544843 -
Attachment is obsolete: true
Attachment #544843 -
Flags: review?(roc)
Attachment #545033 -
Flags: review?(roc)
Assignee | ||
Comment 35•13 years ago
|
||
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).
Assignee | ||
Comment 41•13 years ago
|
||
(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.
Assignee | ||
Comment 42•13 years ago
|
||
(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.
Assignee | ||
Comment 43•13 years ago
|
||
(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::
Assignee | ||
Comment 44•13 years ago
|
||
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?
Comment 47•13 years ago
|
||
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....
Assignee | ||
Comment 48•13 years ago
|
||
(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)
Assignee | ||
Comment 49•13 years ago
|
||
(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)
Assignee | ||
Comment 50•13 years ago
|
||
(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)
Assignee | ||
Comment 51•13 years ago
|
||
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.
Assignee | ||
Comment 56•13 years ago
|
||
> 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)
Assignee | ||
Comment 57•13 years ago
|
||
Attachment #550218 -
Attachment is obsolete: true
Attachment #551957 -
Flags: review?(roc) → review+
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-
Assignee | ||
Comment 61•13 years ago
|
||
Fixed the nit by removing the comment. Updated the commit message.
Attachment #545030 -
Attachment is obsolete: true
Assignee | ||
Comment 62•13 years ago
|
||
Updated the commit message.
Attachment #550201 -
Attachment is obsolete: true
Attachment #554606 -
Flags: superreview?(dbaron)
Assignee | ||
Comment 63•13 years ago
|
||
Updated the commit message.
Attachment #550215 -
Attachment is obsolete: true
Assignee | ||
Comment 64•13 years ago
|
||
Updated the commit message.
Attachment #550217 -
Attachment is obsolete: true
Assignee | ||
Comment 65•13 years ago
|
||
Updated the commit message.
Attachment #551957 -
Attachment is obsolete: true
Assignee | ||
Comment 66•13 years ago
|
||
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+
Assignee | ||
Comment 68•13 years ago
|
||
> 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...
Assignee | ||
Comment 69•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/5c0d46747250 http://hg.mozilla.org/integration/mozilla-inbound/rev/bbb68899df56 http://hg.mozilla.org/integration/mozilla-inbound/rev/de17763f5ba7 http://hg.mozilla.org/integration/mozilla-inbound/rev/d9797d99f5f7 http://hg.mozilla.org/integration/mozilla-inbound/rev/427a6b2313db
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
Comment 70•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/427a6b2313db http://hg.mozilla.org/mozilla-central/rev/d9797d99f5f7 http://hg.mozilla.org/mozilla-central/rev/de17763f5ba7 http://hg.mozilla.org/mozilla-central/rev/bbb68899df56 http://hg.mozilla.org/mozilla-central/rev/5c0d46747250
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
You need to log in
before you can comment on or make changes to this bug.
Description
•