Closed Bug 1075137 Opened 5 years ago Closed 5 years ago

refactor IsProcessingRestyles/IsProcessingAnimationStyleChange into SkipAnimationRestyles / PostAnimationRestyles

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: dbaron, Assigned: dbaron)

Details

Attachments

(7 files)

5.36 KB, patch
birtles
: review+
Details | Diff | Splinter Review
5.94 KB, patch
birtles
: review+
Details | Diff | Splinter Review
960 bytes, patch
birtles
: review+
Details | Diff | Splinter Review
15.53 KB, patch
birtles
: review+
Details | Diff | Splinter Review
4.28 KB, patch
birtles
: review+
Details | Diff | Splinter Review
4.68 KB, patch
birtles
: review+
Details | Diff | Splinter Review
6.03 KB, patch
birtles
: review+
Details | Diff | Splinter Review
We currently have logic for the non-animation and animation phases of style changes that, during the non-animation phase, skips animation styles and posts an animation restyle.

This will go away in bug 960465.

But before bug 960465 happens, we need to refix bug 686656 in the new architecture, so that we use without-animation styles as the base values for CSS animations whose 0% and 100% keyframes aren't fully specified.

This requires computing non-animation styles, but doing so without posting animation restyles.

So in this bug, I propose to refactor the booleans that record the style phases (which are currently only checked in a single pattern, to determine whether to skip animation styles and post animation restyles for them, except for one assertion) into two separate booleans that say whether we should skip animation styles and whether we should post animation restyles when we're skipping styles.  The current code will set both together, but I will use them separately when refixing bug 686656 for the new architecture.
This is just moving one bit of data from the pres context without any
logic change.  But given the other refactoring, it seems to make more
sense here now.
Attachment #8498559 - Flags: review?(birtles)
Comment on attachment 8498555 [details] [diff] [review]
patch 1 - Convert one use of IsProcessingRestyles that doesn't follow normal pattern to a debug-only member on the restyle manager

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

::: layout/base/RestyleManager.cpp
@@ +1433,5 @@
>  
>    nsAutoScriptBlocker scriptBlocker;
>  
> +  NS_ABORT_IF_FALSE(!mIsProcessingRestyles,
> +                    "Nesting calls to processing restyles");

dholbert tells me MOZ_ASSERT is the thing to use for new assertions. I really don't know how much it matters, if at all, but I notice this would fit on one line with MOZ_ASSERT.
Attachment #8498555 - Flags: review?(birtles) → review+
Comment on attachment 8498556 [details] [diff] [review]
patch 2 - Add new booleans for whether to skip animation styles and whether to post animation restyles

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

::: layout/base/RestyleManager.cpp
@@ +1444,5 @@
>  
> +  // Until we get rid of these phases in bug 960465, we need to skip
> +  // animation restyles during the non-animation phase, and post
> +  // animation restyles so that we restyle those elements again in the
> +  // animation phase.

I guess we will update this comment when we re-fix bug 686656 to mention the exceptional case of calculating base values for animations with not-fully-specified 0%/100% keyframes.

::: layout/base/RestyleManager.h
@@ +97,5 @@
> +
> +  // Whether rule matching should post animation restyles when it skips
> +  // styles associated with animation.  Only true when
> +  // SkipAnimationRules() is also true.
> +  bool PostAnimationRestyles() const { return mPostAnimationRestyles; }

I think this is fine as-is, but if the code that sets these gets any more complex it might be worth adding an assertion here that mSkipAnimationRules || !mPostAnimationRestyles.
Attachment #8498556 - Flags: review?(birtles) → review+
Attachment #8498557 - Flags: review?(birtles) → review+
Attachment #8498558 - Flags: review?(birtles) → review+
Comment on attachment 8498559 [details] [diff] [review]
patch 5 - Add new boolean to RestyleManager for whether we're currently processing animation restyles

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

::: layout/base/RestyleManager.cpp
@@ +1576,5 @@
>    // mid-transition (since processing the non-animation restyle ignores
>    // the running transition so it can check for a new change on the same
>    // property, and then posts an immediate animation style change).
>    mPresContext->SetProcessingAnimationStyleChange(true);
> +  mIsProcessingAnimationStyleChange = true;

Is it worth having an assertion that this isn't nested like we have in nsPresContext::SetProcessingAnimationStyleChange ?
Attachment #8498559 - Flags: review?(birtles) → review+
Attachment #8498560 - Flags: review?(birtles) → review+
Attachment #8498561 - Flags: review?(birtles) → review+
(In reply to Brian Birtles (:birtles) from comment #10)
> ::: layout/base/RestyleManager.cpp
> @@ +1444,5 @@
> >  
> > +  // Until we get rid of these phases in bug 960465, we need to skip
> > +  // animation restyles during the non-animation phase, and post
> > +  // animation restyles so that we restyle those elements again in the
> > +  // animation phase.
> 
> I guess we will update this comment when we re-fix bug 686656 to mention the
> exceptional case of calculating base values for animations with
> not-fully-specified 0%/100% keyframes.

The code for that will probably be elsewhere, and then the above code will go away with bug 960465.  (I think, anyway... I haven't written it yet.)
You need to log in before you can comment on or make changes to this bug.