Closed Bug 996796 Opened 11 years ago Closed 10 years ago

refactor miniflush coalescing to use a RestyleTracker and rename miniflush to animation-only style flush

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla34

People

(Reporter: dbaron, Assigned: dbaron)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(26 files, 4 obsolete files)

patch 2 - Change the public API to updating main-thread-suppressed animation styles (miniflush) in preparation for refactoring how it works
4.19 KB, patch
birtles
: review+
Details | Diff | Splinter Review
patch 3 - Move the knowledge of when we last updated main-thread-suppressed animation styles into the restyle manager rather than have two separate but always equal timestamps for animations and transitions
11.77 KB, patch
birtles
: review+
Details | Diff | Splinter Review
patch 4 - Add a new type of RestyleTracker for handling animation-only style flushes
2.65 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 5 - Move the guts of UpdateThrottledStyle into nsStyleSet, where it can be reused
7.38 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 6 - Adjust checks of restyle types to accomodate new types weaker than eRestyle_Self
3.74 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 7 - Add new restyle types that replace only the data from CSS transitions or animations
6.64 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 8 - Pass the replacements through to ResolveStyleWithReplacement
10.51 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 9 - Make nsStyleSet::ResolveStyleWithReplacement handle changing between having and not having animation or transition rules, make it set IsImportantRule on rule nodes correctly, and merge the bogus ResolveStyleForRules into it
10.96 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 10 - Separate the rule node replacement code from the style context handling so that we can make visited handling correct
6.24 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 11 - Fix the visited flag handling in ResolveStyleWithReplacement
1.46 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 12 - Fix the visited rule node handling in ResolveStyleWithReplacement
2.18 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 13 - Fix the pseudo element information created by ResolveStyleWithReplacement
1.38 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 15 - Copy the eSkipParentDisplayBasedStyleFixup bit from ReparentStyleContext as well
1.60 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 18 - Fix RestyleTracker to handle restyle hints exactly rather than pessimistically when restyling continuations with varying styles (e.g., spans inside ::first-line or ::first-letter)
10.80 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 19 - Avoid reresolution for extra style contexts when the restyle hint is not eRestyle_Self or eRestyle_Subtree
2.18 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 20 - Make restyling exact - Avoid rerunning selector matching on everything when the basis of rem units changes
14.15 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 21 - Make restyling exact - Only call MaybeRecreateFramesForElement when we have a nonzero restyle hint
1.80 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 22 - Make restyling exact - only call RestyleBeforePseudo and RestyleAfterPseudo when we have an eRestyle_Subtree hint
2.89 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 23 - Make transition manager ignore StyleContextChanged notifications during an animation-only style update
5.19 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 24 - Use a RestyleTracker for the coalescing in the animation-only style flush (miniflush)
7.59 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 25 - Remove the old (now-unused) miniflush code (preserving one of the header comments)
23.32 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 14 - Fix the aElementForAnimation passed from ResolveStyleWithReplacement
3.21 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 0 - Fix test_animations_omta_start.html so that additional tests will involve onscreen layers
2.31 KB, patch
birtles
: review+
Details | Diff | Splinter Review
patch 1 - Perform a miniflush on both animations and transitions before processing restyles
5.46 KB, patch
birtles
: review+
Details | Diff | Splinter Review
patch 16 - Add comment about potential performance impromevent to ResolveStyleWithReplacement
1.52 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 17 - Add comment about how RuleNodeWithReplacement should interact with nsTransitionManager and nsAnimationManager
2.22 KB, patch
heycam
: review+
Details | Diff | Splinter Review 
I originally saw this work as part of bug 960465, but I'm splitting it out into a separate bug because it can land sooner, and it's the part that's needed for bug 977991.

As I wrote in https://groups.google.com/d/msg/mozilla.dev.tech.layout/0Ehe1BO5ixw/78F_kCoG3TYJ this is to:
   refactor the current miniflush code (code to update the animation
   styles to the current refresh driver time while leaving style
   data otherwise out-of-date, so that we can do correct style
   comparisons when deciding whether to start CSS transitions when
   we've suppressed style updates on the main thread while running
   animations on the compositor) to do coalescing using a
   RestyleTracker rather than their current ad-hoc tree-walking and
   coalescing code (which will also, I believe, fix some correctness
   bugs).  This also means introducing the miniflush's ability to
   replace certain special style rules without rerunning selector
   matching into the RestyleTracker, which is useful in bug 977991.

There are also a bunch of correctness fixes lurking in here that I discovered while doing the work.
More platforms at https://tbpl.mozilla.org/?tree=Try&rev=33ce7938c9f0
Comment on attachment 8447633 [details] [diff] [review]
patch 1 - Perform a miniflush on both animations and transitions before processing restyles

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

r=birtles

We may need to add some small margin of error to the matrix comparisons when we turn on OMTA on Android (see bug 1029969) but that can happen later.
Attachment #8447633 - Flags: review?(birtles) → review+
Comment on attachment 8447634 [details] [diff] [review]
patch 2 - Change the public API to updating main-thread-suppressed animation styles (miniflush) in preparation for refactoring how it works

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

r=birtles
Attachment #8447634 - Flags: review?(birtles) → review+
Comment on attachment 8447635 [details] [diff] [review]
patch 3 - Move the knowledge of when we last updated main-thread-suppressed animation styles into the restyle manager rather than have two separate but always equal timestamps for animations and transitions

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

r=birtles with possible comment tweak.

::: layout/base/nsPresContext.cpp
@@ +1017,5 @@
>  
>    // Initialise refresh tick counters for OMTA
> +  mLastStyleUpdateForAllAnimations = mRefreshDriver->MostRecentRefresh();
> +
> +  // Initialize after initializing the refresh driver.

I don't understand this sentence. Initialize what?
Attachment #8447635 - Flags: review?(birtles) → review+
(In reply to Brian Birtles (:birtles) from comment #31)
> > +  // Initialize after initializing the refresh driver.
> 
> I don't understand this sentence. Initialize what?

restyle manager after refresh driver; I'll fix the comment.


It turns out that since I last tested, one of the todo()s in patch 1 has started passing (the second passes; first still fails).  Not sure what fixed it, but there's still one failure (bug already filed and noted in patch).

That said, I also discovered that patch 14 (which was the last patch I wrote) introduced some test failures (in test_animations_omta.html and test_animations_omta_start.html), I think because setting the elementForAnimation makes us recur back into the animation code.  I need to figure out the right way to suppress that problem.
Attachment #8447646 - Attachment is obsolete: true
Attachment #8447646 - Flags: review?(cam)
Comment on attachment 8447636 [details] [diff] [review]
patch 4 - Add a new type of RestyleTracker for handling animation-only style flushes

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

::: content/base/public/Element.h
@@ +87,5 @@
> +  // it was).
> +  ELEMENT_HAS_PENDING_ANIMATION_ONLY_RESTYLE =  ELEMENT_FLAG_BIT(4),
> +
> +  // Set if the element is a potential animation-only restyle root (that
> +  // is, has an animation style change pending _and_ that style change

s/animation/animation-only/
Attachment #8447636 - Flags: review?(cam) → review+
This removes each test element when we're done with it so that each
successive test element isn't 100px lower.  This is required to keep the
third test element (added in the next patch) onscreen when running tests
on the B2G emulator (other than when running a single test at a time).
This, in turn, is required to get animations in that test properly
shipped to the compositor thread, which is required for the test to
pass.
Attachment #8461804 - Flags: review?(birtles)
This affects the correctness of transitions that take over from a
running animation.  (In the current code this can happen on a single
element; once the cascading changes in bug 960465 are complete it can
only happen via inheritance.)

This version of the patch changes to do the test using opacity rather
than transform, since testing using transforms encountered issues
related to bug 1031688:  the presence of phantom transitions due to the
interaction of the computed value rules for transforms distinguishing
between values that the interpolation rules consider identical.  (These
problems only appear after patch 24 in this bug changes the coalescing
order between a parent with animations and a child with transitions so
that the parent is handled before the child, instead of transitions
being handled before animations.)

The final two added tests fail without the patch and pass with the patch.
(With the opacity version, the third to last test also fails without the
patch, probably due to the value not having yet been sent to the layer.
This was not an issue pre-patch with the original test using transform,
though.)
Attachment #8461805 - Flags: review?(birtles)
Comment on attachment 8461804 [details] [diff] [review]
patch 0 - Fix test_animations_omta_start.html so that additional tests will involve onscreen layers

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

r=birtles

::: layout/style/test/test_animations_omta_start.html
@@ +93,5 @@
>        var transform = gUtils.getOMTAStyle(target, "transform");
>        ok(matricesRoughlyEqual(convertTo3dMatrix(transform),
>                                convertTo3dMatrix("matrix(1, 0, 0, 1, 50, 0)")),
>           "transform is set on compositor thread after delayed start");
> +      target.parentNode.removeChild(target);

This is fine, but I'm pretty sure we can now just do:
  target.remove()
(https://developer.mozilla.org/en-US/docs/Web/API/ChildNode.remove)
Attachment #8461804 - Flags: review?(birtles) → review+
Comment on attachment 8461805 [details] [diff] [review]
patch 1 - Perform a miniflush on both animations and transitions before processing restyles

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

I was looking at this exact code yesterday wondering why we weren't updating animations!

One reason we see different behavior for opacity as opposed to transforms is that getOMTAStyle behaves differently for each one. For transforms we have a flag on the layer to indicate if the transform was set by animation or not and we only return a value if that flag is set. For opacity we don't have such a flag so we return the value on the layer whether or not it was set by animation. The other main difference is, as the commit message points out, redundant transform changes getting skipped meaning the animation doesn't get sent to the compositor immediately in some cases.

I'm still not sure why the third-to-last test passed with transforms but I agree that the the failure with opacity appears to be bug 1039799.

r=birtles

::: layout/style/test/test_animations_omta_start.html
@@ +157,5 @@
> +      todo_is(opacity, "0.4",
> +         "transition that interrupted animation is correct");
> +      gUtils.advanceTimeAndRefresh(0);
> +      waitForAllPaints(function() {
> +        var opacity = gUtils.getOMTAStyle(child, "opacity");

We already have "opacity" in this scope so "var" is unnecessary here (and below) but perhaps its more clear/portable if we have it?

@@ +167,5 @@
> +          is(opacity, "0.7",
> +             "transition that interrupted animation is correct");
> +          is(childCS.opacity, "0.7",
> +             "transition that interrupted animation is correct");
> +          parent.parentNode.removeChild(parent);

parent.remove() if you prefer.
Attachment #8461805 - Flags: review?(birtles) → review+
Comment on attachment 8447637 [details] [diff] [review]
patch 5 - Move the guts of UpdateThrottledStyle into nsStyleSet, where it can be reused

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

I see the FIXME is addressed in patch 12.  (I wonder if the "TODO: REVIEW THIS CAREFULLY" commit message in patch 12 is aimed at yourself at me!)
Attachment #8447637 - Flags: review?(cam) → review+
Attachment #8447638 - Flags: review?(cam) → review+
Comment on attachment 8447639 [details] [diff] [review]
patch 7 - Add new restyle types that replace only the data from CSS transitions or animations

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

Maybe s/a refactoring/part of a refactoring/ in the commit message correct, because just looking at the patch by itself it doesn't seem like a refactoring of those classes.
Attachment #8447639 - Flags: review?(cam) → review+
Comment on attachment 8447640 [details] [diff] [review]
patch 8 - Pass the replacements through to ResolveStyleWithReplacement

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

::: layout/style/nsStyleSet.cpp
@@ +1330,5 @@
> +  NS_ABORT_IF_FALSE(!(aReplacements & ~(eRestyle_CSSTransitions |
> +                                        eRestyle_CSSAnimations)),
> +                    nsPrintfCString("unexpected replacement bits 0x%lX",
> +                                    uint32_t(aReplacements)).get());
> +

I have a DEBUG-only function that prints an nsRestyleHint symbolically that I should upload as part of bug 979133 and which could be used here, once bug 931668 is landed.
Attachment #8447640 - Flags: review?(cam) → review+
Comment on attachment 8447641 [details] [diff] [review]
patch 9 - Make nsStyleSet::ResolveStyleWithReplacement handle changing between having and not having animation or transition rules, make it set IsImportantRule on rule nodes correctly, and merge the bogus ResolveStyleForRules into it

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

::: layout/style/nsStyleSet.cpp
@@ +1324,5 @@
> +  { nsStyleSet::eOverrideSheet,   true,  nsRestyleHint(0) },
> +  { nsStyleSet::eUserSheet,       true,  nsRestyleHint(0) },
> +  { nsStyleSet::eAgentSheet,      true,  nsRestyleHint(0) },
> +  { nsStyleSet::eTransitionSheet, false, eRestyle_CSSTransitions },
> +};

Would be nice if we could use this table in FileRules, to keep the cascade order in sync automatically, though the XBL stuff and the scoped sheet handling might make that tricky.

@@ +1358,5 @@
> +    bool doReplace = level->mLevelReplacementHint & aReplacements;
> +    if (doReplace) {
> +      switch (level->mLevelReplacementHint) {
> +      case eRestyle_CSSAnimations:
> +        {

Style guide says to write it this like:

  switch (...) {
    case ...: {
      ...
      break;
    }
Attachment #8447641 - Flags: review?(cam) → review+
Attachment #8447642 - Flags: review?(cam) → review+
Attachment #8447643 - Flags: review?(cam) → review+
Comment on attachment 8447644 [details] [diff] [review]
patch 12 - Fix the visited rule node handling in ResolveStyleWithReplacement

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

Please add a test with this patch.  Here's one you can adapt I was just trying locally in a Nightly without this patch queue applied, which I assume will work with the queue up to this patch applied (where work means it will transition from white to green instead of nearly-red to red):

<!DOCTYPE html>
<style>
span { transition: 1s background-color; font: 48px sans-serif; }
:link span { background-color: rgb(254, 0, 0); }
:link span:hover { background-color: red; }
:visited span { background-color: white; }
:visited span:hover { background-color: green; }
</style>
<a href=""><span>some text here</span></a>

The patch itself looks OK.
Attachment #8447644 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #47)
> Please add a test with this patch.  Here's one you can adapt I was just
> trying locally in a Nightly without this patch queue applied, which I assume
> will work with the queue up to this patch applied (where work means it will
> transition from white to green instead of nearly-red to red):

Or will we not be running this code yet because background-color isn't a property that gets animated on the compositor?
Attachment #8447645 - Flags: review?(cam) → review+
Attachment #8451361 - Flags: review?(cam) → review+
Attachment #8447647 - Flags: review?(cam) → review+
Comment on attachment 8447648 [details] [diff] [review]
patch 16 - Add comment about potential performance impromevent to ResolveStyleWithReplacement

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

::: layout/style/nsStyleSet.cpp
@@ +1341,5 @@
>  
> +  // FIXME (perf): This should probably not rebuild the whole path, but
> +  // only the path from the last change in the rule tree, like
> +  // nsAnimationManager::CheckAnimationRule does.  (That could then
> +  // perhaps share this code, too?)

Which bit of nsAnimationManager::CheckAnimationRule are you referring to?
Comment on attachment 8447649 [details] [diff] [review]
patch 17 - Add comment about how ResolveStyleWithReplacement interacts with nsTransitionManager and nsAnimationManager

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

I'm not sure the commit message matches the comments?
Attachment #8447650 - Flags: review?(cam) → review+
Attachment #8447651 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #49)
> Comment on attachment 8447648 [details] [diff] [review]
> patch 16 - Add comment about potential performance impromevent to
> ResolveStyleWithReplacement
...
> Which bit of nsAnimationManager::CheckAnimationRule are you referring to?

Er, sorry, I really meant the ReplaceAnimationRule function in nsStyleSet.


(In reply to Cameron McCormack (:heycam) from comment #50)
> Comment on attachment 8447649 [details] [diff] [review]
> patch 17 - Add comment about how ResolveStyleWithReplacement interacts with
> nsTransitionManager and nsAnimationManager
...
> I'm not sure the commit message matches the comments?

How about:

Add comment about how RuleNodeWithReplacement should interact with nsTransitionManager and nsAnimationManager.

?
Comment on attachment 8447652 [details] [diff] [review]
patch 20 - Make restyling exact - Avoid rerunning selector matching on everything when the basis of rem units changes

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

::: layout/base/RestyleManager.cpp
@@ +1394,5 @@
> +  // because they're changing things that affect data computation rather
> +  // than selector matching; we could have a restyle hint passed in, and
> +  // substantially improve the performance of things like pref changes
> +  // and the restyling that we do for downloadable font loads.
> +  DoRebuildAllStyleData(mPendingRestyles, aExtraHint, eRestyle_Subtree);

Can you file a bug to track this followup work?

@@ +1419,5 @@
>    }
>  
> +  if (aRestyleHint & ~eRestyle_Subtree) {
> +    // We want this hint to apply to the root node's primary frame
> +    // rather than the root frame.

Can you explain why?

@@ +1433,5 @@
>    // (but note that nsPresShell::SetPreferenceStyleRules currently depends
>    // on us re-running rule matching here
>    nsStyleChangeList changeList;
>    // XXX Does it matter that we're passing aExtraHint to the real root
>    // frame and not the root node's primary frame?

Is this comment obsoleted given the above change?

@@ +2340,5 @@
>    nsRestyleHint hintToRestore = nsRestyleHint(0);
> +  if (mContent && mContent->IsElement() &&
> +      // If we're we're resolving from the root of the frame tree (which
> +      // we do in DoRebuildAllStyleData), we need to avoid getting the
> +      // root's restyle data until we get to its primary frame.

Can you help me understand why that's important?

@@ +2740,4 @@
>                                                  mFrame->StyleContext(),
>                                                  undisplayed->mStyle,
>                                                  thisChildHint);
> +      } else if (styleSet->IsInRuleTreeReconstruct()) {

Can you assert somewhere that the restyle hint will be 0 when IsInRuleTreeReconstruct() is true?
Attachment #8447653 - Flags: review?(cam) → review+
Attachment #8447654 - Flags: review?(cam) → review+
Attachment #8447655 - Flags: review?(cam) → review+
Comment on attachment 8447656 [details] [diff] [review]
patch 24 - Use a RestyleTracker for the coalescing in the animation-only style flush (miniflush)

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

r=me with comments addressed.

::: layout/style/nsAnimationManager.cpp
@@ +805,5 @@
>  
> +void
> +nsAnimationManager::AddStyleUpdatesTo(RestyleTracker& aTracker)
> +{
> +  PRCList *next = PR_LIST_HEAD(&mElementCollections);

"*" next to type.

@@ +807,5 @@
> +nsAnimationManager::AddStyleUpdatesTo(RestyleTracker& aTracker)
> +{
> +  PRCList *next = PR_LIST_HEAD(&mElementCollections);
> +  while (next != &mElementCollections) {
> +    ElementAnimationCollection *collection = static_cast<ElementAnimationCollection*>(next);

And here.

::: layout/style/nsTransitionManager.cpp
@@ +123,5 @@
>    UpdateAllThrottledStylesInternal();
>  }
>  
>  void
> +nsTransitionManager::AddStyleUpdatesTo(RestyleTracker& aTracker)

Consider factoring out these two methods and having a common implementation on CommonAnimationManager that takes an nsIAtom* argument and restyle hint?

@@ +127,5 @@
> +nsTransitionManager::AddStyleUpdatesTo(RestyleTracker& aTracker)
> +{
> +  PRCList *next = PR_LIST_HEAD(&mElementCollections);
> +  while (next != &mElementCollections) {
> +    ElementAnimationCollection *collection = static_cast<ElementAnimationCollection*>(next);

Ditto for these two lines.
Attachment #8447656 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #52)
> Comment on attachment 8447652 [details] [diff] [review]
> patch 20 - Make restyling exact - Avoid rerunning selector matching on
> everything when the basis of rem units changes

> > +  if (aRestyleHint & ~eRestyle_Subtree) {
> > +    // We want this hint to apply to the root node's primary frame
> > +    // rather than the root frame.
> 
> Can you explain why?

Because that's the one whose style context has the styles for the root element.  Thus, if we have an eRestyle_Self, CSSAnimations, or CSSTransitions for the root, and we apply it to an ancestor of the root node's primary frame, we'll end up not doing the restyle that we need to do.

> @@ +1433,5 @@
> >    // (but note that nsPresShell::SetPreferenceStyleRules currently depends
> >    // on us re-running rule matching here
> >    nsStyleChangeList changeList;
> >    // XXX Does it matter that we're passing aExtraHint to the real root
> >    // frame and not the root node's primary frame?
> 
> Is this comment obsoleted given the above change?

No.  We might want to switch the extra hint processing in the same way -- then again, it's not causing a problem and extra hints are rare.

> @@ +2340,5 @@
> >    nsRestyleHint hintToRestore = nsRestyleHint(0);
> > +  if (mContent && mContent->IsElement() &&
> > +      // If we're we're resolving from the root of the frame tree (which
> > +      // we do in DoRebuildAllStyleData), we need to avoid getting the
> > +      // root's restyle data until we get to its primary frame.
> 
> Can you help me understand why that's important?

Same as above.

> > +      } else if (styleSet->IsInRuleTreeReconstruct()) {
> 
> Can you assert somewhere that the restyle hint will be 0 when
> IsInRuleTreeReconstruct() is true?

I don't see why that would be true.  We might have pending style changes at the time we do a rule tree reconstruct.

I'm just using ResolveStyleWithReplacement instead of ReparentStyleContext because ReparentStyleContext would reuse the rule node from the old rule tree, whereas ResolveStyleWithReplacement rebuilds the rule tree path and thus returns the equivalent style context using the new rule tree.  That probably deserves a comment, though, since it took me a minute to remember.
Comment on attachment 8447657 [details] [diff] [review]
patch 25 - Remove the old (now-unused) miniflush code (preserving one of the header comments)

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

::: layout/base/RestyleManager.h
@@ +164,5 @@
>    // whose updating is suppressed on the main thread (to save
>    // unnecessary work), while leaving all other aspects of style
>    // out-of-date.
> +  //
> +  // Performs a 'mini-flush' to make styles from throttled transitions

The bug title says we're renaming the term "mini-flush", so call it "animation-only flush" here?

::: layout/style/AnimationCommon.h
@@ -124,5 @@
> -    ElementAnimationCollection* collection =                                   \
> -      static_cast<ElementAnimationCollection*>(next);                          \
> -    next = PR_NEXT_LINK(next);                                                 \
> -                                                                               \
> -    if (collection->mFlushGeneration == now) {                                 \

In the new code in the previous patch we don't do any generation checking.  Can we run into the same problem of flushing the animation styles twice in the new code, or will the use of the RestyleTracker prevent that?
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #54)
> (In reply to Cameron McCormack (:heycam) from comment #52)
> > Comment on attachment 8447652 [details] [diff] [review]
> > patch 20 - Make restyling exact - Avoid rerunning selector matching on
> > everything when the basis of rem units changes
> 
> > > +  if (aRestyleHint & ~eRestyle_Subtree) {
> > > +    // We want this hint to apply to the root node's primary frame
> > > +    // rather than the root frame.
> > 
> > Can you explain why?
> 
> Because that's the one whose style context has the styles for the root
> element.  Thus, if we have an eRestyle_Self, CSSAnimations, or
> CSSTransitions for the root, and we apply it to an ancestor of the root
> node's primary frame, we'll end up not doing the restyle that we need to do.

Understood.

> > > +      } else if (styleSet->IsInRuleTreeReconstruct()) {
> > 
> > Can you assert somewhere that the restyle hint will be 0 when
> > IsInRuleTreeReconstruct() is true?
> 
> I don't see why that would be true.  We might have pending style changes at
> the time we do a rule tree reconstruct.

Oh right we would want to redo selector matching there.

> I'm just using ResolveStyleWithReplacement instead of ReparentStyleContext
> because ReparentStyleContext would reuse the rule node from the old rule
> tree, whereas ResolveStyleWithReplacement rebuilds the rule tree path and
> thus returns the equivalent style context using the new rule tree.  That
> probably deserves a comment, though, since it took me a minute to remember.

I figured so, but yes a comment would be good.
Attachment #8447652 - Flags: review?(cam) → review+
Attachment #8447648 - Attachment is obsolete: true
Attachment #8447648 - Flags: review?(cam)
Attachment #8447649 - Attachment is obsolete: true
Attachment #8447649 - Flags: review?(cam)
(In reply to Cameron McCormack (:heycam) from comment #55)
> In the new code in the previous patch we don't do any generation checking. 
> Can we run into the same problem of flushing the animation styles twice in
> the new code, or will the use of the RestyleTracker prevent that?

I believe the RestyleTracker should merge fully over the tree with no duplication, so the single overall last-update check in patch 3 should be sufficient.

(I also didn't like the term "generation" there.  I prefer to restrict the term generation to things that increase every time something happens (e.g., on every style change that we flush, which can happen more than once per refresh cycle) rather than things that increase with the refresh clock.  This is the way we use the term for mAnimationGeneration, which is used to track what animation data has been shipped to the compositor thread.)
Comment on attachment 8465832 [details] [diff] [review]
patch 16 - Add comment about potential performance impromevent to ResolveStyleWithReplacement

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

r=me with s/ResolveStyleWithReplacement/RuleNodeWithReplacement/ in the commit message.
Attachment #8465832 - Flags: review?(cam) → review+
Attachment #8465834 - Flags: review?(cam) → review+
Comment on attachment 8447657 [details] [diff] [review]
patch 25 - Remove the old (now-unused) miniflush code (preserving one of the header comments)

r=me with s/a 'mini-flush'/an "animation-only style flush/ or something similar in the comment.
Attachment #8447657 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #47)
> Comment on attachment 8447644 [details] [diff] [review]
> patch 12 - Fix the visited rule node handling in ResolveStyleWithReplacement

> Please add a test with this patch.  Here's one you can adapt I was just
> trying locally in a Nightly without this patch queue applied, which I assume
> will work with the queue up to this patch applied (where work means it will
> transition from white to green instead of nearly-red to red):
> 
> <!DOCTYPE html>
> <style>
> span { transition: 1s background-color; font: 48px sans-serif; }
> :link span { background-color: rgb(254, 0, 0); }
> :link span:hover { background-color: red; }
> :visited span { background-color: white; }
> :visited span:hover { background-color: green; }
> </style>
> <a href=""><span>some text here</span></a>

(In reply to Cameron McCormack (:heycam) from comment #48)
> Or will we not be running this code yet because background-color isn't a
> property that gets animated on the compositor?

This is a testcase for bug 868975 rather than something fixed by this patch.

Writing a testcase for the problem fixed by this bug is somewhat more complicated.  I think it could be done by setting up a compositor-driven animation on a visited link, and while that animation is running making a style change on a different (independent) element to trigger an animation-only style flush, and then using DOMWindowUtils.getVisitedDependentComputedStyle() to observe that the :visited styles have disappeared.  I guess that's not as hard as I was initially thinking it would be, so maybe I'll give it a shot.
continued from the previous comment:

I tried writing that test, and then debugged why the test didn't fail without the patch.  This led me to conclude that the bug being fixed in patch 12 is currently untestable, but might become testable in the future.

In particular, currently, PresShell::FlushPendingNotifications has this chunk of code:

      if (aFlush.mFlushAnimations &&
          !mPresContext->StyleUpdateForAllAnimationsIsUpToDate()) {
        if (mPresContext->AnimationManager()) {
          mPresContext->AnimationManager()->
            FlushAnimations(CommonAnimationManager::Cannot_Throttle);
        }
        if (mPresContext->TransitionManager()) {
          mPresContext->TransitionManager()->
            FlushTransitions(CommonAnimationManager::Cannot_Throttle);
        }
        mPresContext->TickLastStyleUpdateForAllAnimations();
      }

right before its call to ProcessPendingRestyles.  This posts restyles for everything that is animating -- restyles that will be processed after the animation-only style flush (which happens inside ProcessPendingRestyles).  These calls thus cover up any damage done by incorrect behavior of the animation-only style flush (and also duplicate its work!) except in so far as that damage relates to incorrect starting of transitions.

I think I plan to land the test anyway, even though it doesn't currently test anything useful, because it might do so if we reduce the duplication involved (which I may well do as part of bug 960465).
That said, I do have a test that now:

* without the change in patch 12, hits the fatal assertion at http://hg.mozilla.org/mozilla-central/file/a4f779bd7cc2/layout/style/nsStyleContext.cpp#l154 due to the bug being fixed in patch 12

* without the patch *and* with the rearrangement to depend on the animation-only style flush more (i.e., remove the chunk of code in the previous comment and make the call to UpdateOnlyAnimationStyles in RestyleManager::ProcessPendingRestyles unconditional) *and* with a change to work around the fatal assertion by tweaking the flags in ResolveStyleWithReplacement, shows a test failure

* passes with the patches

So I think that's definitely worth adding.
 > >  void
> > +nsTransitionManager::AddStyleUpdatesTo(RestyleTracker& aTracker)
> 
> Consider factoring out these two methods and having a common implementation
> on CommonAnimationManager that takes an nsIAtom* argument and restyle hint?

I'm actually going to have the entire implementation on CommonAnimationManager with no extra hintsparameters, since the data is on the collection, and that approach fits better with the patches higher in my queue for bug 960465.
https://hg.mozilla.org/integration/mozilla-inbound/rev/96186774f07c
https://hg.mozilla.org/integration/mozilla-inbound/rev/8da0b361d546
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8a810bc1b36
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9db9020d57a
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a395d400f60
https://hg.mozilla.org/integration/mozilla-inbound/rev/720eed827027
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a9de658d4b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/d13154302d77
https://hg.mozilla.org/integration/mozilla-inbound/rev/119416a35fa8
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ee0ebcf0602
https://hg.mozilla.org/integration/mozilla-inbound/rev/a911c666406f
https://hg.mozilla.org/integration/mozilla-inbound/rev/06ffae59cea9
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5b4fbbcbcd4
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe97c2db729
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc528616d916
https://hg.mozilla.org/integration/mozilla-inbound/rev/10438983fda7
https://hg.mozilla.org/integration/mozilla-inbound/rev/d776e50cd140
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c1136091a68
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc7a3787b584
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebf41f7c81b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/9719c08c3144
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #64)
> So I think that's definitely worth adding.

Worth noting, though, that as of today it won't actually run anywhere, since B2G doesn't have global history, and all other platforms don't have OMT animations.  But it should start running automatically if other platforms get OMT animations turned on, and it will TEST-UNEXPECTED-PASS if B2G gets global history.
Partially backed out in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0d4dd2261a5
Keywords: leave-open
According to try runs, the orange was from patch 24:
https://tbpl.mozilla.org/?tree=Try&rev=7c06c13be60b
https://tbpl.mozilla.org/?tree=Try&rev=c970a0ba48e0
which really doesn't make much sense to me since that code should only run on platforms with OMT animations enabled.
Oh, except https://hg.mozilla.org/mozilla-central/file/e6614d8d85f9/layout/style/crashtests/972199-1.html is setting the pref to true, so we have OMT animations enabled for the latter part of the crashtest run (order is in https://hg.mozilla.org/mozilla-central/file/e6614d8d85f9/testing/crashtest/crashtests.list ).
(It should probably be using pushPrefEnv.)

That said, it may well be showing a real bug, although I also can't repro that bug by enabling OMT animations locally on Linux.
Given that I've been unable to reproduce the failure locally either on Linux (with OMT animations on) or on B2G emulator, I'm inclined to just fix comment 70.  While it's probably papering over something, if that something turns up again it seems likely to be turning up in a more reproducable way.
Also, the try run showing the intermittent crashtest orange is fixed with that change:
https://tbpl.mozilla.org/?tree=Try&rev=af55063ec9b8

(I had equivalent orange try runs without it.)
https://hg.mozilla.org/mozilla-central/rev/a2f481de3386
https://hg.mozilla.org/mozilla-central/rev/96483fbec785
https://hg.mozilla.org/mozilla-central/rev/12959495c74e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1081013
Depends on: 1119140
See Also: → 1341003
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: