Bug 1110277 patch 3 - Look for the GenConPseudos() property on the first continuation/ib-split so that we can find it when looking for the ::after frame. r=bzbarsky
authorL. David Baron <dbaron@dbaron.org>
Sun, 11 Jan 2015 15:43:11 -0800
changeset 248999 1bbebe9fec17e88234845d8da22c8b44f394121b
parent 248998 fbbafc2a957318cc4fa2c2dbfd774124ccf597be
child 249000 98e34be1fb44344c1173d9d3c981b3e5a2da4762
push id4489
push userraliiev@mozilla.com
push dateMon, 23 Feb 2015 15:17:55 +0000
treeherdermozilla-beta@fd7c3dc24146 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbzbarsky
bugs1110277, 1115691, 600100
milestone37.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1110277 patch 3 - Look for the GenConPseudos() property on the first continuation/ib-split so that we can find it when looking for the ::after frame. r=bzbarsky The change to GetAfterFrameForContent prevents the reframe that is part of the chain of events leading to this bug, and thus fixes the bug on its own. The change to GetBeforeFrameForContent seems desirable for symmetry. Note that patch 6 also independently fixes the reported bug. This probably needs somewhat careful review. We should examine: (1) what the rules for calling nsLayoutUtils::GetBeforeFrame and nsLayoutUtils::GetAfterFrame are, and whether both (or neither) need to be patched. (2) What the rules are for which frame the GenConProperty() lives on, and whether we should adjust nsIFrame::GetGenConPseudos() to either do something more intelligent, or assert about callers. (We should probably clean up some of these things in a followup bug.) Since the symptom of this bug is (once patch 4 is in the tree) only causing extra reframes, it can only be tested using the new API (from bug 1115691) for observing reframes. I confirmed that the test for this bug fails without the patch and passes with the patch (as noted by the removal of its todo annotation). This patch fixes the assertion on layout/generic/crashtests/600100.xhtml, though I haven't investigated why.
layout/base/nsCSSFrameConstructor.cpp
layout/base/nsLayoutUtils.cpp
layout/base/tests/test_frame_reconstruction_for_pseudo_elements.html
layout/generic/crashtests/crashtests.list
layout/generic/nsIFrame.h
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -5784,16 +5784,19 @@ nsCSSFrameConstructor::AddFrameConstruct
     item->mIsLineParticipant = true;
     aItems.LineParticipantItemAdded();
   }
 }
 
 static void
 AddGenConPseudoToFrame(nsIFrame* aOwnerFrame, nsIContent* aContent)
 {
+  NS_ASSERTION(nsLayoutUtils::IsFirstContinuationOrIBSplitSibling(aOwnerFrame),
+               "property should only be set on first continuation/ib-sibling");
+
   typedef nsAutoTArray<nsIContent*, 2> T;
   const FramePropertyDescriptor* prop = nsIFrame::GenConProperty();
   FrameProperties props = aOwnerFrame->Properties();
   T* value = static_cast<T*>(props.Get(prop));
   if (!value) {
     value = new T;
     props.Set(prop, value);
   }
--- a/layout/base/nsLayoutUtils.cpp
+++ b/layout/base/nsLayoutUtils.cpp
@@ -1166,17 +1166,20 @@ nsLayoutUtils::GetChildListNameFor(nsIFr
 
   return id;
 }
 
 /*static*/ nsIFrame*
 nsLayoutUtils::GetBeforeFrameForContent(nsIFrame* aFrame,
                                         nsIContent* aContent)
 {
-  nsContainerFrame* genConParentFrame = aFrame->GetContentInsertionFrame();
+  // We need to call GetGenConPseudos() on the first continuation/ib-split.
+  // Find it, for symmetry with GetAfterFrameForContent.
+  nsContainerFrame* genConParentFrame =
+    FirstContinuationOrIBSplitSibling(aFrame)->GetContentInsertionFrame();
   if (!genConParentFrame) {
     return nullptr;
   }
   nsTArray<nsIContent*>* prop = genConParentFrame->GetGenConPseudos();
   if (prop) {
     const nsTArray<nsIContent*>& pseudos(*prop);
     for (uint32_t i = 0; i < pseudos.Length(); ++i) {
       if (pseudos[i]->GetParent() == aContent &&
@@ -1202,35 +1205,43 @@ nsLayoutUtils::GetBeforeFrame(nsIFrame* 
 {
   return GetBeforeFrameForContent(aFrame, aFrame->GetContent());
 }
 
 /*static*/ nsIFrame*
 nsLayoutUtils::GetAfterFrameForContent(nsIFrame* aFrame,
                                        nsIContent* aContent)
 {
-  nsContainerFrame* genConParentFrame = aFrame->GetContentInsertionFrame();
+  // We need to call GetGenConPseudos() on the first continuation,
+  // but callers are likely to pass the last.
+  nsContainerFrame* genConParentFrame =
+    FirstContinuationOrIBSplitSibling(aFrame)->GetContentInsertionFrame();
   if (!genConParentFrame) {
     return nullptr;
   }
   nsTArray<nsIContent*>* prop = genConParentFrame->GetGenConPseudos();
   if (prop) {
     const nsTArray<nsIContent*>& pseudos(*prop);
     for (uint32_t i = 0; i < pseudos.Length(); ++i) {
       if (pseudos[i]->GetParent() == aContent &&
           pseudos[i]->Tag() == nsGkAtoms::mozgeneratedcontentafter) {
         return pseudos[i]->GetPrimaryFrame();
       }
     }
   }
   // If the last child frame is a pseudo-frame, then try that.
   // Note that the frame we create for the generated content is also a
   // pseudo-frame and so don't drill down in that case.
+  genConParentFrame = aFrame->GetContentInsertionFrame();
+  if (!genConParentFrame) {
+    return nullptr;
+  }
   nsIFrame* lastParentContinuation =
-    nsLayoutUtils::LastContinuationWithChild(genConParentFrame);
+    LastContinuationWithChild(static_cast<nsContainerFrame*>(
+      LastContinuationOrIBSplitSibling(genConParentFrame)));
   nsIFrame* childFrame =
     lastParentContinuation->GetLastChild(nsIFrame::kPrincipalList);
   if (childFrame &&
       childFrame->IsPseudoFrame(aContent) &&
       !childFrame->IsGeneratedContentFrame()) {
     return GetAfterFrameForContent(childFrame->FirstContinuation(), aContent);
   }
   return nullptr;
--- a/layout/base/tests/test_frame_reconstruction_for_pseudo_elements.html
+++ b/layout/base/tests/test_frame_reconstruction_for_pseudo_elements.html
@@ -30,36 +30,36 @@ https://bugzilla.mozilla.org/show_bug.cg
   </style>
   <script type="application/javascript">
 
   /** Test for Bug 1110277 **/
 
   SimpleTest.waitForExplicitFinish();
 
   function run() {
-    runtest("first line test", "#firstlinetest > .testspan", {});
-    runtest("after test", "#aftertest > .testspan", { todo: true });
+    runtest("first line test", "#firstlinetest > .testspan");
+    runtest("after test", "#aftertest > .testspan");
     SimpleTest.finish();
   }
 
-  function runtest(description, selector, flags) {
+  function runtest(description, selector) {
     var utils = SpecialPowers.getDOMWindowUtils(window);
     var span = document.querySelector(selector);
     var cs = getComputedStyle(span, "");
 
     var startcolor = cs.color;
     var startcount = utils.framesConstructed;
     is(startcolor, "rgb(255, 255, 0)", description + ": initial color");
 
     span.setAttribute("attributestate", "true");
 
     var endcolor = cs.color;
     var endcount = utils.framesConstructed;
     is(endcolor, "rgb(0, 0, 255)", description + ": final color");
-    (flags.todo ? todo_is : is)(endcount, startcount,
+    is(endcount, startcount,
        description + ": should not do frame construction")
   }
 
   </script>
 </head>
 <body onload="run()">
 <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1110277">Mozilla Bug 1110277</a>
 <div id="firstlinetest">
--- a/layout/generic/crashtests/crashtests.list
+++ b/layout/generic/crashtests/crashtests.list
@@ -420,17 +420,17 @@ load 586806-3.html
 load 586973-1.html
 load 589002-1.html
 load 590404.html
 load 591141.html
 asserts(0-1) load 592118.html
 load 594808-1.html
 load 595435-1.xhtml
 load 595740-1.html
-pref(layout.float-fragments-inside-column.enabled,true) asserts(1) load 600100.xhtml # bug 866955
+pref(layout.float-fragments-inside-column.enabled,true) load 600100.xhtml
 pref(layout.float-fragments-inside-column.enabled,false) load 600100.xhtml
 load 603490-1.html
 load 603510-1.html
 load 604314-1.html
 load 604843.html
 load 605340.html
 load 606642.xhtml
 load 619021.html
--- a/layout/generic/nsIFrame.h
+++ b/layout/generic/nsIFrame.h
@@ -866,16 +866,18 @@ public:
 
   NS_DECLARE_FRAME_PROPERTY(InvalidationRect, DestroyRect)
 
   NS_DECLARE_FRAME_PROPERTY(RefusedAsyncAnimation, nullptr)
 
   NS_DECLARE_FRAME_PROPERTY(GenConProperty, DestroyContentArray)
 
   nsTArray<nsIContent*>* GetGenConPseudos() {
+    NS_ASSERTION(nsLayoutUtils::IsFirstContinuationOrIBSplitSibling(this),
+                 "should only call on first continuation/ib-sibling");
     const FramePropertyDescriptor* prop = GenConProperty();
     return static_cast<nsTArray<nsIContent*>*>(Properties().Get(prop));
   }
 
   /**
    * Return the distance between the border edge of the frame and the
    * margin edge of the frame.  Like GetRect(), returns the dimensions
    * as of the most recent reflow.