Bug 1320484 part 1: Improve documentation for debug-only function FrameWantsToBeInAnonymousItem(), & change its arg from nsIAtom* to nsIFrame*. r?mats draft
authorDaniel Holbert <dholbert@cs.stanford.edu>
Wed, 04 Jan 2017 20:29:19 -0800
changeset 456207 971360ba52c02edb41731b6566dd0271b6e9396f
parent 455536 57ac9f63fc6953f4efeb0cc84a60192d3721251f
child 456208 5f0add6689ed0fd85ff7bfc777bea1806df6d2c6
push id40421
push userdholbert@mozilla.com
push dateThu, 05 Jan 2017 04:32:08 +0000
reviewersmats
bugs1320484
milestone53.0a1
Bug 1320484 part 1: Improve documentation for debug-only function FrameWantsToBeInAnonymousItem(), & change its arg from nsIAtom* to nsIFrame*. r?mats This patch doesn't affect behavior at all -- it just refactors the sanity-checking function "FrameWantsToBeInAnonymousItem()", so that the next patch in this series can give it a new special case that checks a state bit on the container frame. This patch renames "parent" to "container" in this function's variable-names, for clarity, because when this function returns true, the flex/grid container is actually NOT expected to be the parent of aFrame. Rather, it's expected to be the grandparent, and the anonymous flex/grid item would be the parent. So, "aContainerFrame"/"containerType" is a bit more accurate (representing the flex/grid container for aFrame). Also worth mentioning: this patch makes FrameWantsToBeInAnonymousItem() perform its own local GetType() call, instead of accepting an already-queried GetType() result from the caller (as it previously did). Technically this could cause a slight perf hit, but it doesn't really matter since this is in "#ifdef DEBUG" sanity-checking code anyway. We could keep the nsIAtom* as an additional arg to avoid this new call, but it seems better to fall on the side of simplicity & just look up GetType() independently, rather than complicating the function signature with an extra arg. MozReview-Commit-ID: 4oJFkQMuH9c
layout/base/nsCSSFrameConstructor.cpp
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -10598,40 +10598,57 @@ void nsCSSFrameConstructor::CreateNeeded
                               pseudoStyle,
                               true, nullptr);
   newItem->mIsAllInline = true;
   newItem->mChildItems.SetParentHasNoXBLChildren(true);
   iter.InsertItem(newItem);
 }
 
 #ifdef DEBUG
+/**
+ * Returns true iff aFrame should be wrapped in an anonymous flex/grid item,
+ * rather than being a direct child of aContainerFrame.
+ *
+ * NOTE: aContainerFrame must be a flex or grid container - this function is
+ * purely for sanity-checking the children of these container types.
+ * NOTE: See also NeedsAnonFlexOrGridItem(), for the non-debug version of this
+ * logic (which operates a bit earlier, on FCData instead of frames).
+ */
 static bool
-FrameWantsToBeInAnonymousItem(const nsIAtom* aParentType, const nsIFrame* aFrame)
-{
-  MOZ_ASSERT(aParentType == nsGkAtoms::flexContainerFrame ||
-             aParentType == nsGkAtoms::gridContainerFrame);
-
-  return aFrame->IsFrameOfType(nsIFrame::eLineParticipant);
+FrameWantsToBeInAnonymousItem(const nsIFrame* aContainerFrame,
+                              const nsIFrame* aFrame)
+{
+  nsIAtom* containerType = aContainerFrame->GetType();
+  MOZ_ASSERT(containerType == nsGkAtoms::flexContainerFrame ||
+             containerType == nsGkAtoms::gridContainerFrame);
+
+  // Any line-participant frames (e.g. text) definitely want to be wrapped in
+  // an anonymous flex/grid item.
+  if (aFrame->IsFrameOfType(nsIFrame::eLineParticipant)) {
+    return true;
+  }
+
+  return false;
 }
 #endif
 
 static void
 VerifyGridFlexContainerChildren(nsIFrame* aParentFrame,
                                 const nsFrameList& aChildren)
 {
 #ifdef DEBUG
   auto parentType = aParentFrame->GetType();
   if (parentType != nsGkAtoms::flexContainerFrame &&
       parentType != nsGkAtoms::gridContainerFrame) {
     return;
   }
 
   bool prevChildWasAnonItem = false;
   for (const nsIFrame* child : aChildren) {
-    MOZ_ASSERT(!FrameWantsToBeInAnonymousItem(parentType, child),
+    MOZ_ASSERT(!FrameWantsToBeInAnonymousItem(aParentFrame, child),
                "frame wants to be inside an anonymous item, but it isn't");
     if (IsAnonymousFlexOrGridItem(child)) {
       AssertAnonymousFlexOrGridItemParent(child, aParentFrame);
       MOZ_ASSERT(!prevChildWasAnonItem, "two anon items in a row");
       nsIFrame* firstWrappedChild = child->PrincipalChildList().FirstChild();
       MOZ_ASSERT(firstWrappedChild, "anonymous item shouldn't be empty");
       prevChildWasAnonItem = true;
     } else {