Closed
Bug 1075137
Opened 10 years ago
Closed 10 years ago
refactor IsProcessingRestyles/IsProcessingAnimationStyleChange into SkipAnimationRestyles / PostAnimationRestyles
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
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.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8498555 -
Flags: review?(birtles)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8498556 -
Flags: review?(birtles)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8498557 -
Flags: review?(birtles)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8498558 -
Flags: review?(birtles)
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8498560 -
Flags: review?(birtles)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8498561 -
Flags: review?(birtles)
Assignee | ||
Comment 8•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b76d3e0291a5
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8498557 -
Flags: review?(birtles) → review+
Updated•10 years ago
|
Attachment #8498558 -
Flags: review?(birtles) → review+
Comment 11•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8498560 -
Flags: review?(birtles) → review+
Updated•10 years ago
|
Attachment #8498561 -
Flags: review?(birtles) → review+
Assignee | ||
Comment 12•10 years ago
|
||
(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.)
Assignee | ||
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b35d7246cc83 https://hg.mozilla.org/integration/mozilla-inbound/rev/43da66148c28 https://hg.mozilla.org/integration/mozilla-inbound/rev/a9163aa983a5 https://hg.mozilla.org/integration/mozilla-inbound/rev/6e06546591fc https://hg.mozilla.org/integration/mozilla-inbound/rev/40a25cf2bcfb https://hg.mozilla.org/integration/mozilla-inbound/rev/9ad680392072 https://hg.mozilla.org/integration/mozilla-inbound/rev/de4f6c938b6a
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b35d7246cc83 https://hg.mozilla.org/mozilla-central/rev/43da66148c28 https://hg.mozilla.org/mozilla-central/rev/a9163aa983a5 https://hg.mozilla.org/mozilla-central/rev/6e06546591fc https://hg.mozilla.org/mozilla-central/rev/40a25cf2bcfb https://hg.mozilla.org/mozilla-central/rev/9ad680392072 https://hg.mozilla.org/mozilla-central/rev/de4f6c938b6a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•