Closed Bug 1334036 Opened 8 years ago Closed 8 years ago

stylo: Support off-main-thread animations

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: boris, Assigned: boris)

References

(Depends on 1 open bug)

Details

Attachments

(13 files, 10 obsolete files)

Bug 1334036 - Part 1: Avoid mutating mElementsToRestyle during pre-traversal.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1334036 - Part 7: Merge two similiar MaybeUpdateCascadeResults functions.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1334036 - Part 5: Implement FFI for finding properties overriding animations.
59 bytes, text/x-review-board-request
emilio
: review+
birtles
: review+
Details
Bug 1334036 - Part 8: Add AddLayerChangesForAnimation in ServoRestyleManager.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1334036 - Part 9: Add one FFI which return None transform.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1334036 - Part 12: Enable off-main thread animations.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1334036 - Part 10: Return AnimationValue for BaseStyle.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1334036 - Part 4: Remove unused UpdateCascadeResults function.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1334036 - Part 3: Add a flag to represent we are in pre-traversal.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1334036 - Part 6: Trigger restyle if important rules are changed.
59 bytes, text/x-review-board-request
birtles
: review+
emilio
: review+
Details
Bug 1334036 - Part 2: Restyle all elements with animations if there are non-animation restyles.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1334036 - Part 11: Trigger animation-only restyle when we handle an event with coordinates.
59 bytes, text/x-review-board-request
birtles
: review+
heycam
: review+
Details
Servo PR, #16963
41 bytes, text/x-github-pull-request
Details | Review
We don't support off-main-thread animations on stylo now, so currently we skip all the related implementation. File this bug for tracking them.
See Also: → 1324691
See Also: → 1324695
Blocks: 1324695
See Also: 1324695 →
Blocks: 1324693
Priority: -- → P3
Note: need to verify these crashtests after fixing this bug: 1216842-1.html 1216842-2.html 1216842-5.html 1216842-6.html
No longer blocks: 1324693, 1324695
See Also: → 1324695, 1324693
Note that if we skip MaybeUpdateCascadeResults() in FindAnimationsForCompositor(), opacity animation runs on the compositor. We still use StyleAnimationValue though.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2) > Note that if we skip MaybeUpdateCascadeResults() in > FindAnimationsForCompositor(), opacity animation runs on the compositor. We > still use StyleAnimationValue though. Great, Thanks, after you finish transform property, I can find a way to use RawServoAnimationValue for OMTA on both opacity and transform.
Just to be clear we don't need to RawServoAnimationValue for OMTA for now because we have valid StyleAnimationValue even in case of stylo. Also I have a half-baked patch for bug 1335942.
Note also that we need to the same mechanism what we do in MaybeUpdateCascadeResults() for OMTA in stylo (servo?), i.e. pulling back compositor animations when cascading result is changed and re-send them that updated by the new cascading result.
Depends on: 1335942
Assignee: nobody → boris.chiou
Status: NEW → ASSIGNED
See Also: → 1335308
There is an assertion caused by Bug 1335308 [1], which triggers "Assertion failure: !ServoStyleSet::IsInServoTraversal(false), at /layout/style/nsCSSValue.cpp:434" while doing SampleAnimations() in the compositor thread [2]. ServoStyleSet::IsInServoTraversal checks a static flag, ServoStyleSets::sInServoTraversal, which is set during Servo_TraverseSubtree() on the main thread. However, we create/use/relase a StyleAnimationValue which contains a nsCSSValueShreadList (transform value) in the compositor thread at the same time. I think they are not related, so it is thread-safe, and I guess we need a way to work around this assertion or add more flag to avoid this case. heycam, do you have any suggestion? [1] http://searchfox.org/mozilla-central/source/layout/style/nsCSSValue.cpp#418 [2] http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/gfx/layers/composite/AsyncCompositionManager.cpp#641-655 [3] http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/layout/style/ServoStyleSet.h#59
Flags: needinfo?(cam)
Can't we use CompositorThreadHolder::IsInCompositorThread() there? I am not really sure.
The comment in nsCSSValue.cpp mentions an aAssertServoOrMainThread argument but that doesn't seem to exist. It does sound like explicitly checking whether we're on the compositor thread would be OK. Can you try that?
Flags: needinfo?(cam)
I should also note that we are using only nsCSSValueSharedList on the compositor thread for now. We should allow only nsCSSValueSharedList.
Do we both create and destroy the nsCSSValueSharedList on the compositor thread, or do we create it on the main thread and destroy it on the compositor thread? Maybe we can have a DEBUG-only bool on nsCSSValueSharedList that records that it is owned by the compositor thread, so that we can skip this assertion without having to pass an argument (like what I assume aAssertServoOrMainThread would have been) into nsCSSValue::DoReset, just for these objects.
(In reply to Cameron McCormack (:heycam) from comment #10) > Do we both create and destroy the nsCSSValueSharedList on the compositor > thread. Yes. The nsCSSValueSharedList is alive only on the compositor thread. > Maybe we can have a DEBUG-only bool on > nsCSSValueSharedList that records that it is owned by the compositor thread, > so that we can skip this assertion without having to pass an argument (like > what I assume aAssertServoOrMainThread would have been) into > nsCSSValue::DoReset, just for these objects. Yeah, that's a good idea.
Maybe Boris has already noticed, but I just notice now, we allocate nsCSSValueSharedList during building display list for the compositor animations.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #12) > Maybe Boris has already noticed, but I just notice now, we allocate > nsCSSValueSharedList during building display list for the compositor > animations. Yes, we create a nsCSSValueShreadList which is copied from RawServoAnimationValue [1]. I guess we copy its data into layers::Animatable, and then free it on the main thread. Does that cause your assertion? [1] http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/layout/painting/nsDisplayList.cpp#456-458
No my try does not have your patches for bug 1338087. I guess the nsCSSValueShreadList created during building display list might cause the assertion on the main-thread since it will be destroyed after servo' traversal tree.
Depends on: 1340005
Blocks: 1339704
Blocks: 1341003
Boris, could you please hold on this bug? Part 7 will be fixed by a patch for bug 1338927. Part 9 will be fixed by bug 1340961.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #28) > Boris, could you please hold on this bug? Part 7 will be fixed by a patch > for bug 1338927. Part 9 will be fixed by bug 1340961. Sure. Let's wait for your patches merged first. However, I still have one problem I have no idea: In some test cases: for example: [1] When we create a css animation (transform), it is running on compositor, so SpecialPowers.DOMWindowUtils.getOMTAStyle() could get the correct style. However, while calling window.getComputedStyle(), I noticed that sometimes we return "None" transform, and it seems we didn't trigger animations update on main thread? Looks like Gecko updates the AnimationRule somewhere like RuleNodeWithReplacement(), but stylo doesn't, in this case. [1] http://searchfox.org/mozilla-central/rev/12cf11303392edac9f1da0c02e3d9ad2ecc8f4d3/layout/style/test/file_animations_pausing.html#44-49
Depends on: 1338927, 1340961
Blocks: 1341195
I will update these patches after bug 1338927 and bug 1340961 are landed.
No longer blocks: 1339704
Depends on: 1340938
According to our meeting on 2017-02-23, I think we should wait for hiro's work on Two pass style resolution proposal. Therefore, I prefer to hold these patches until we fix those bugs on CSS Animations, and then I will retry these patches and re-check the test cases.
It's bug 1341985.
Depends on: 1341985
No longer depends on: 1340938
Comment on attachment 8839109 [details] Bug 1334036 - Part 8: Need to update cascade result after change the style. https://reviewboard.mozilla.org/r/113700/#review119016 I think we should process UpdateCascadeResults() just before triggering the second traversal for CSS animations. Otherwise we don't process the restyles which is the result of UpdateCascadeResults() in the same tick.
Priority: P3 → P1
(In reply to Hiroyuki Ikezoe (:hiro) from comment #33) > Comment on attachment 8839109 [details] > Bug 1334036 - Part 8: Need to update cascade result after change the style. > > https://reviewboard.mozilla.org/r/113700/#review119016 > > I think we should process UpdateCascadeResults() just before triggering the > second traversal for CSS animations. Otherwise we don't process the restyles > which is the result of UpdateCascadeResults() in the same tick. I'm thinking where is the suitable place to call UpdateCascadeResults(). I agree that we should call it after the first normal traversal and before the second animation traversal (which updating animation or transition). 1. a) PreTraversal (Main thread) * Call MaybeUpdateCascadeResult, but only update aEffectSet.PropertiesForAnimationsLevel(), so we can filter out transitions if we have both transitions and animation on the same property. I think we should avoiding request restyle if there are new added animation here. I'd like to delay this request restyle until (e), so I guess we might need to add a flag for this. b) Animation only Traversal c) Normal Traversal d) The SequentialTask * Note: UpdateEffectProperties() also calls MarkCascadeNeedUpdate(), so it will trigger (e) if there are style changes. e) Call MaybeUpdateCascadeResults() (Main Thread) * If we want to find the aEffectSet.PropertiesWithImportantRules(), we should call this after cascading (i.e. normal traversal). * This updates aEffectSet.PropertiesForAnimationsLevel() (if there are new added css animation?) and aEffectSet.PropertiesWithImportantRules(), * May request restyles if there are new added animations (from (a) or new added css animation in this tick) or new added !important rules [1]. 2. f) Second PreTraversal g) Second Animation only Traversal h) Second Normal Traversal There is another question: we don't have mElementsToRestyle in (e), so we need to find an efficient way to traverse the elements with animations. [1] http://searchfox.org/mozilla-central/rev/ce5ccb6a8ca803271c946ccb8b43b7e7d8b64e7a/dom/animation/EffectCompositor.cpp#910-931
Hi Hiro, I need your help for this test case [1]. Before changing the target, everything looks good; however, after we change the target, we run on both compositor thread (i.e. AnimationHelper::SampleAnimationForEachNode() is still working.) and main thread (i.e. animation-only restyle starts to run after changing target.). And we didn't call FindAnimationForCompositor() any more after changing target. Do you remember where we check this kind of situation in Gecko? Thanks a lot. [1] http://searchfox.org/mozilla-central/rev/784ec1af1451fd479641b40efd7720a2f3f5781a/dom/animation/test/chrome/test_running_on_compositor.html#475-497
Flags: needinfo?(hikezoe)
Will the test case pass if we use opacity: 0.8 instead of 100% opacity? If so nsIFrame::HasOpacityInternal() is the suspicious function.
Flags: needinfo?(hikezoe)
My gut feeling tells me that the failure is somewhat related to bug 1346049, why test cases for checking stacking context fail on stylo intermittently.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #36) > Will the test case pass if we use opacity: 0.8 instead of 100% opacity? If > so nsIFrame::HasOpacityInternal() is the suspicious function. Yes, using opacity: 0.8, e.g. var animation = div.animate([{ opacity: 1, offset: 0 }, { opacity: 0.8, offset: 0.99 }, { opacity: 0, offset: 1 }], 100 * MS_PER_SEC); We can pass this test case. OK, I should check nsIFrame::HasOpacityInternal(). Thank you!
(In reply to Boris Chiou [:boris] from comment #38) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #36) > > Will the test case pass if we use opacity: 0.8 instead of 100% opacity? If > > so nsIFrame::HasOpacityInternal() is the suspicious function. > > Yes, using opacity: 0.8, e.g. > > var animation = div.animate([{ opacity: 1, offset: 0 }, > { opacity: 0.8, offset: 0.99 }, > { opacity: 0, offset: 1 }], 100 * MS_PER_SEC); > > We can pass this test case. > > OK, I should check nsIFrame::HasOpacityInternal(). Thank you! Still pass the test if: var animation = div.animate([{ opacity: 0.8, offset: 0 }, { opacity: 0.8, offset: 0.99 }, { opacity: 0, offset: 1 }], 100 * MS_PER_SEC);
(In reply to Boris Chiou [:boris] from comment #35) > Hi Hiro, > > I need your help for this test case [1]. > > Before changing the target, everything looks good; however, after we change > the target, we run on both compositor thread (i.e. > AnimationHelper::SampleAnimationForEachNode() is still working.) and main > thread (i.e. animation-only restyle starts to run after changing target.). > And we didn't call FindAnimationForCompositor() any more after changing > target. Do you remember where we check this kind of situation in Gecko? > Thanks a lot. > > [1] > http://searchfox.org/mozilla-central/rev/ > 784ec1af1451fd479641b40efd7720a2f3f5781a/dom/animation/test/chrome/ > test_running_on_compositor.html#475-497 I guess all test failures in test_running_on_compositor.html are similar: 1. 100% opacity animations with keeps running on the compositor after changing the target element [1] 2. Clearing *important* opacity style on the target element sends the animation to the compositor even if the animation is in the delay phase [2] 3. Changing target element of opacity animation sends the animation to the the compositor even if the animation is in the delay phase [3] 4. Dynamic change to a property runnable on the compositor in the delay phase [4] All four tests didn't call FindAnimationForCompositor() after setting new target/setting new keyframes/updating styles. (1) is in 100% opacity, (2)-(4) are in delay phase. (BTW, both opacity and transform have this problem.) Therefore, I think there must be a general problem I haven't noticed. [1] http://searchfox.org/mozilla-central/rev/784ec1af1451fd479641b40efd7720a2f3f5781a/dom/animation/test/chrome/test_running_on_compositor.html#475-497 [2] http://searchfox.org/mozilla-central/rev/784ec1af1451fd479641b40efd7720a2f3f5781a/dom/animation/test/chrome/test_running_on_compositor.html#855-875 [3] http://searchfox.org/mozilla-central/rev/784ec1af1451fd479641b40efd7720a2f3f5781a/dom/animation/test/chrome/test_running_on_compositor.html#877-899 [4] http://searchfox.org/mozilla-central/rev/784ec1af1451fd479641b40efd7720a2f3f5781a/dom/animation/test/chrome/test_running_on_compositor.html#901-921
I haven't looked into any of these issues at all so this might not be helpful, but one thing that can trigger us to update animations on layers is the animation generation. Specifically, when we know we need to update animations on layers in a way that otherwise wouldn't be detected by our regular style change processing (e.g. when we are in a backwards fill etc.) we call RequestRestyle with the RestyleType::Layer which bumps the animation generation. Then we check that in ElementRestyler::AddLayerChangesForAnimation() in GeckoRestyleManager.cpp and set the change hint that will cause us to update layers. That will only effect transform animations (since we would normally skip updating layers if the transform value hasn't changed) but if opacity animations are failing too then it may be the second part of that function that is checking for newly compositor-animatable animations.
I just realized that calling MaybeUpdateCascadeResults() in PreTraverseInSubtree() is a problem because MaybeUpdateCascadeResults() may modify mElementsToRestyle during iteration mElementsToRestyle PreTraverseInSubtree(). Boris, do you already have a patch to solve this problem?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #42) > I just realized that calling MaybeUpdateCascadeResults() in > PreTraverseInSubtree() is a problem because MaybeUpdateCascadeResults() may > modify mElementsToRestyle during iteration mElementsToRestyle > PreTraverseInSubtree(). *during iteration of mElementsToRestyle in PreTraverseInSubtree().
(In reply to Brian Birtles (:birtles) from comment #41) > I haven't looked into any of these issues at all so this might not be > helpful, but one thing that can trigger us to update animations on layers is > the animation generation. Specifically, when we know we need to update > animations on layers in a way that otherwise wouldn't be detected by our > regular style change processing (e.g. when we are in a backwards fill etc.) > we call RequestRestyle with the RestyleType::Layer which bumps the animation > generation. Then we check that in > ElementRestyler::AddLayerChangesForAnimation() in GeckoRestyleManager.cpp > and set the change hint that will cause us to update layers. > > That will only effect transform animations (since we would normally skip > updating layers if the transform value hasn't changed) but if opacity > animations are failing too then it may be the second part of that function > that is checking for newly compositor-animatable animations. Thanks, Brian. I will try to call this function to check it. (Currently, I only fix EffectCompositor::GetOverriddenProperties() and call MaybeUpdateCascadeResults before second PreTraversal for testing.)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #43) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #42) > > I just realized that calling MaybeUpdateCascadeResults() in > > PreTraverseInSubtree() is a problem because MaybeUpdateCascadeResults() may > > modify mElementsToRestyle during iteration mElementsToRestyle > > PreTraverseInSubtree(). > > *during iteration of mElementsToRestyle in PreTraverseInSubtree(). Yes, so I implement a simplified version of MaybeUpdateCascadeResult() to update only EffectSet::mPropertiesForAnimationsLevel in PreTraverse (without requesting restyle), so mElementsToRestyle won't be modified.
(In reply to Boris Chiou [:boris] from comment #44) > (In reply to Brian Birtles (:birtles) from comment #41) > > I haven't looked into any of these issues at all so this might not be > > helpful, but one thing that can trigger us to update animations on layers is > > the animation generation. Specifically, when we know we need to update > > animations on layers in a way that otherwise wouldn't be detected by our > > regular style change processing (e.g. when we are in a backwards fill etc.) > > we call RequestRestyle with the RestyleType::Layer which bumps the animation > > generation. Then we check that in > > ElementRestyler::AddLayerChangesForAnimation() in GeckoRestyleManager.cpp > > and set the change hint that will cause us to update layers. > > Thanks, Brian. I will try to call this function to check it. (Currently, I > only fix EffectCompositor::GetOverriddenProperties() and call > MaybeUpdateCascadeResults before second PreTraversal for testing.) OK. I guess this may be the root cause because I check this on both Gecko and Stylo, and looks like it is necessary. Now I need to find a suitable place to put this function.
Attachment #8839104 - Attachment is obsolete: true
Attachment #8839105 - Attachment is obsolete: true
Attachment #8839108 - Attachment is obsolete: true
Attachment #8839109 - Attachment is obsolete: true
Attachment #8839110 - Attachment is obsolete: true
Attachment #8839111 - Attachment is obsolete: true
Attachment #8839112 - Attachment is obsolete: true
Attachment #8839113 - Attachment is obsolete: true
Still got many test failures and some assertions (missing keyframes) [1]. I will keep looking at these failures and file a bug for the assertions. [1] http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/layout/style/test/test_animations_omta.html#1517-1568
(In reply to Boris Chiou [:boris] from comment #53) > Still got many test failures and some assertions (missing keyframes) [1]. I > will keep looking at these failures and file a bug for the assertions. > > [1] > http://searchfox.org/mozilla-central/rev/ > ae8c2e2354db652950fe0ec16983360c21857f2a/layout/style/test/ > test_animations_omta.html#1517-1568 I guess the failures will be fixed by bug 1359669.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #54) > (In reply to Boris Chiou [:boris] from comment #53) > > Still got many test failures and some assertions (missing keyframes) [1]. I > > will keep looking at these failures and file a bug for the assertions. > > > > [1] > > http://searchfox.org/mozilla-central/rev/ > > ae8c2e2354db652950fe0ec16983360c21857f2a/layout/style/test/ > > test_animations_omta.html#1517-1568 > > I guess the failures will be fixed by bug 1359669. One kind of test failures are: tiny difference between the results from OMTA and getComputeStyle(). And the results of tests of animation-direction or animation-play-state are wired. I will check them again after Bug 1359669 merged. Besides, all the assertions happen in the compositor thread (i.e. in SampleValue()) because we may get null aUnderlyingValue and start or end value is also null in the meantime. Thanks for your update, hiro. :)
Got some interesting results in some test cases in test_animations_omta.html: I dumped some results of interpolation of StyleAnimationValue and ServoAnimationValue: a. Compositor thread (StyleAnimationValue): In SampleValue(): * property: transform, * from "translate3d(20px, 0px, 0px)" to "translate3d(60px, 0px, 0px)" at 0.975625 => "translate3d(59.025px, 0px, 0px)" b. Main thread (ServoAnimationValue): In Servo_AnimationCompose(): * from "Transform(T(Some([Translate(20px, 0px, 0px)])))" to "Transform(T(Some([Translate(60px, 0px, 0px)])))" at 0.9756253556623609 => "Transform(T(Some([Translate(59.03333333333333px, 0px, 0px)])))" I use python to verify the computation of the interpolation result in Servo, i.e. (b): 20 + (60-20) * 0.9756253556623609 = 59.025014226494434 The result is different from my log in (b), so I guess our interpolation is not accurate enough in Servo. I got many test failures related to this bug, I think.
Blocks: 1361663
Blocks: 1361933
Blocks: 1361938
Hmm. I got different results on e10s and non-e10s. test_animations_omta_start.html has 3 tests failed if I disable e10s. However, everything in test_animations_omta_start.html looks good with e10s.
Blocks: 1361981
Blocks: 1362292
I'm not sure why MozReview doesn't let me comment on commit messages today, but I just started looking at these and the first commit message is: "We should avoid requesting restyle and updating mElementsToRestyle in PreTraverse(), so simplify the function of updating properties for animations level and delay its potential requesting restyle. The delay request restyle will be implemented in the latter patch." This will be a lot easier to review if there was a bit more explanation. Specifically, why should we avoid requesting restyle and updating mElementsToRestyle in PreTraverse?
Comment on attachment 8865335 [details] Bug 1334036 - Part 10: Return AnimationValue for BaseStyle. https://reviewboard.mozilla.org/r/136974/#review139948
Attachment #8865335 - Flags: review?(hikezoe) → review+
BTW, the idea of part 1 and part 3 comes from Comment 34.
Comment on attachment 8865336 [details] Bug 1334036 - Part 4: Remove unused UpdateCascadeResults function. https://reviewboard.mozilla.org/r/136976/#review139950 ::: layout/style/test/mochitest.ini:63 (Diff revision 2) > [test_animations_event_order.html] > [test_animations_event_handler_attribute.html] > [test_animations_iterationstart.html] > support-files = file_animations_iterationstart.html > [test_animations_omta.html] > +skip-if = (stylo && !e10s) # mochitest expectations are different with non-e10s Bug number?
Attachment #8865336 - Flags: review?(hikezoe) → review+
Comment on attachment 8865336 [details] Bug 1334036 - Part 4: Remove unused UpdateCascadeResults function. https://reviewboard.mozilla.org/r/136976/#review139950 > Bug number? Oops, I will put the bug number. We have more test cases with non-e10s (according to the test log), so I will put the same bug number in stylo-failure, i.e. bug 1361938, bug 1361663.
Comment on attachment 8839103 [details] Bug 1334036 - Part 5: Implement FFI for finding properties overriding animations. https://reviewboard.mozilla.org/r/113644/#review139964 ::: dom/animation/EffectCompositor.cpp:668 (Diff revision 3) > if (!effects || !effects->CascadeNeedsUpdate()) { > return; > } > > nsStyleContext* styleContext = aStyleContext; > - if (!styleContext) { > + if (aBackend == StyleBackendType::Gecko && !styleContext) { Why this? Doesn't necessarily seem like we should skip it for stylo. Indeed, we should probably just look at the `::before` and `::after` element's style and avoid the pseudo-tag business to get the up-to-date style with animations instead of bypassing this. ::: servo/components/style/rule_tree/mod.rs:806 (Diff revision 3) > + > + /// Get a set of properties whose CascadeLevel are higher than Animations but not equal to > + /// Transitions. > + #[cfg(feature = "gecko")] > + pub fn get_properties_overriding_animations(&self, guard: &SharedRwLockReadGuard) > + -> HashSet<nsCSSPropertyID> { Probably this function should use a `LonghandIdSet` (perhaps with an extra field indicating that it's using custom properties? Something like `(LonghandIdSet, bool)`?) With that, you avoid hashing, this can be compiled for Servo too, and you move the gecko code to the `glue` file. Is there any reason it can't be done that way? ::: servo/components/style/rule_tree/mod.rs:816 (Diff revision 3) > + let iter = self.self_and_ancestors() > + .skip_while(|node| node.cascade_level() == CascadeLevel::Transitions) > + .take_while(|node| node.cascade_level() > CascadeLevel::Animations); > + let mut result = HashSet::new(); > + for node in iter { > + if let Some(style) = node.style_source() { You can use `unwrap` here: The only node without an style source is the root, and it has a less-specific-than-animation cascade level. ::: servo/components/style/rule_tree/mod.rs:821 (Diff revision 3) > + if let Some(style) = node.style_source() { > + let ids = style.read(guard) > + .declarations() > + .iter() > + .filter_map(|&(ref decl, imp)| { > + if imp == Importance::Important { You can use `impl.important()`, which may be somewhat nicer to read. ::: servo/ports/geckolib/glue.rs:1107 (Diff revision 3) > } > } > > #[no_mangle] > +pub extern "C" fn Servo_GetProperties_Overriding_Animation(element: RawGeckoElementBorrowed, > + pseudo_tag: *mut nsIAtom, Instead of the `pseudo_tag` argument, you should probably pass the pseudo-element itself here, and just use the primary style for that.
Attachment #8839103 - Flags: review?(emilio+bugs)
Comment on attachment 8839107 [details] Bug 1334036 - Part 9: Add one FFI which return None transform. https://reviewboard.mozilla.org/r/113696/#review139968 ::: servo/ports/geckolib/glue.rs:417 (Diff revision 3) > list: *mut structs::RefPtr<nsCSSValueSharedList>) > { > let value = AnimationValue::as_arc(&value); > if let AnimationValue::Transform(ref servo_list) = **value { > - style_structs::Box::convert_transform(servo_list.0.clone().unwrap(), unsafe { &mut *list }); > + let list = unsafe { &mut *list }; > + if servo_list.0.is_none() { This can use `match` instead of unwrapping below: ``` match servo_list.0 { Some(ref list) => { convert_transform(...) } None => { unsafe { list.set_move(..) } } } ``` ::: servo/ports/geckolib/glue.rs:418 (Diff revision 3) > { > let value = AnimationValue::as_arc(&value); > if let AnimationValue::Transform(ref servo_list) = **value { > - style_structs::Box::convert_transform(servo_list.0.clone().unwrap(), unsafe { &mut *list }); > + let list = unsafe { &mut *list }; > + if servo_list.0.is_none() { > + unsafe{ list.set_move(RefPtr::from_addrefed(Gecko_NewNoneTransform())) }; nit: space before the opening brace: `unsafe {`. ::: servo/ports/geckolib/glue.rs:420 (Diff revision 3) > if let AnimationValue::Transform(ref servo_list) = **value { > - style_structs::Box::convert_transform(servo_list.0.clone().unwrap(), unsafe { &mut *list }); > + let list = unsafe { &mut *list }; > + if servo_list.0.is_none() { > + unsafe{ list.set_move(RefPtr::from_addrefed(Gecko_NewNoneTransform())) }; > + } else { > + style_structs::Box::convert_transform(servo_list.0.clone().unwrap(), list); Either here or as a followup, please make this function not take a `Vec<ComputedOperation>`, but a `&[ComputedOperation]`. Copying the array here is pretty wasteful.
Attachment #8839107 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8839103 [details] Bug 1334036 - Part 5: Implement FFI for finding properties overriding animations. https://reviewboard.mozilla.org/r/113644/#review139964 > Why this? Doesn't necessarily seem like we should skip it for stylo. > > Indeed, we should probably just look at the `::before` and `::after` element's style and avoid the pseudo-tag business to get the up-to-date style with animations instead of bypassing this. OK. So we should get the generated element (```before``` or ```after```), and use it as the argument of Servo_GetProperties_Overriding_Animation() to get the primary rules. If we can't get the generated element (e.g. lack of frame info?), we should probably just try to get the primary rules from the |```aElement```| directly, right? > Probably this function should use a `LonghandIdSet` (perhaps with an extra field indicating that it's using custom properties? Something like `(LonghandIdSet, bool)`?) > > With that, you avoid hashing, this can be compiled for Servo too, and you move the gecko code to the `glue` file. > > Is there any reason it can't be done that way? I will try that.
Comment on attachment 8839103 [details] Bug 1334036 - Part 5: Implement FFI for finding properties overriding animations. https://reviewboard.mozilla.org/r/113644/#review139964 > OK. So we should get the generated element (```before``` or ```after```), and use it as the argument of Servo_GetProperties_Overriding_Animation() to get the primary rules. If we can't get the generated element (e.g. lack of frame info?), we should probably just try to get the primary rules from the |```aElement```| directly, right? Right. Indeed I thought I'd killed all the `aPseudoTag` from the glue code, but apparently I forgot `GetBaseComputedValues` :). I'll remove the pseudo-tag from there too.
Comment on attachment 8839101 [details] Bug 1334036 - Part 1: Avoid mutating mElementsToRestyle during pre-traversal. https://reviewboard.mozilla.org/r/113640/#review140320 As per comment 66, this patch needs a suitable commit message explaining *why* we should avoid requesting restyle and updating mElementsToRestyle in PreTraverse. Please re-request review after adding a suitable explanation.
Attachment #8839101 - Flags: review?(bbirtles)
Comment on attachment 8839101 [details] Bug 1334036 - Part 1: Avoid mutating mElementsToRestyle during pre-traversal. https://reviewboard.mozilla.org/r/113640/#review140320 > As per comment 66, this patch needs a suitable commit message explaining *why* we should avoid requesting restyle and > updating mElementsToRestyle in PreTraverse. Please re-request review after adding a suitable explanation. Thanks, Brian. I will update my comment to explain why I did that.
Comment on attachment 8839103 [details] Bug 1334036 - Part 5: Implement FFI for finding properties overriding animations. https://reviewboard.mozilla.org/r/113644/#review140340 This looks mostly good but it seems like Emilio has suggested some significant changes so I would like to check this again once those have been made. ::: dom/animation/EffectCompositor.h:195 (Diff revision 3) > // |aStyleContext| may be nullptr in which case we will use the > // nsStyleContext of the primary frame of the specified (pseudo-)element. > + // However, The Servo backend doesn't use the nsStyleContext to get the rule > + // node to traverse the style tree to find !important rules and instead > + // gets the rule node from |aElement|, so we use |aBackend| to know which > + // backend we are using and retrieve the rule node from the correct place. We should rewrite the comment to make sense rather than just adding to it. e.g. "When |aBackend| is StyleBackendType::Gecko, |aStyleContext| is used to find overridden properties. If it is nullptr, the nsStyleContext of the primary frame of the specified (pseudo-)element, if available, is used. When |aBackend| is StyleBackendType::Servo, we fetch the rule node from the |aElement| (i.e. |aStyleContext| is ignored)." ::: dom/animation/EffectCompositor.h:219 (Diff revision 3) > // Update the mPropertiesWithImportantRules and > // mPropertiesForAnimationsLevel members of the corresponding EffectSet. > // > // This can be expensive so we should only call it if styles that apply > // above the animation level of the cascade might have changed. For all > // other cases we should call MaybeUpdateCascadeResults. Let's add a sentence to this comment, "As with MaybeUpdateCascadeResults above, |aStyleContext| is only used when |aBackend| is StyleBackendType::Gecko. When |aBackend| is StyleBackendType::Servo, it is ignored. Unlike MaybeUpdateCascadeResults, however, if |aStyleContext| is nullptr, we do NOT automatically look up the style context of primary frame of the (pseudo-)element." ::: dom/animation/EffectCompositor.h:228 (Diff revision 3) > // above the animation level of the cascade might have changed. For all > // other cases we should call MaybeUpdateCascadeResults. > static void > UpdateCascadeResults(dom::Element* aElement, > CSSPseudoElementType aPseudoType, > + StyleBackendType aBackend, In some places we call this |aBackend| and in some places |aType|. We should call them all |aBackend| or all |aBackendType| (whichever you prefer). ::: dom/animation/EffectCompositor.h:276 (Diff revision 3) > // Get the properties in |aEffectSet| that we are able to animate on the > // compositor but which are also specified at a higher level in the cascade > - // than the animations level in |aStyleContext|. > + // than the animations level in |aStyleContext| on Gecko. > + // stylo: Servo-backend stores the RuleNode in |aElement| (i.e. GeckoElement). Again, we generally try not to use "stylo" in code comments -- it's just the name of a project (that many people who read the code in the future, will be unfamiliar with). We should refer to Servo and Gecko. So let's make this comment: "Get the properties in |aEffectSet| that we are able to animate on the compositor but which are also specified at a higher level in the cascade than the animations level. When |aBackendType| is StyleBackendType::Gecko, we determine which properties are specified using the provided |aStyleContext| and |aElement| and |aPseudoType| are ignored. If |aStyleContext| is nullptr, this function is a no-op. [Although I think we should probably change this -- see comments below.] When |aBackend| is StyleBackendType::Servo, we use the RuleNode stored on the (pseudo-)element indicated by |aElement| and |aPseudoType|." You will need to mention what |aPseudoType| is for but it sounds like that might change based on Emilio's feedback. ::: dom/animation/EffectCompositor.h:280 (Diff revision 3) > static void > - GetOverriddenProperties(nsStyleContext* aStyleContext, > + GetOverriddenProperties(const dom::Element* aElement, > + CSSPseudoElementType aPseudoType, > + nsStyleContext* aStyleContext, > EffectSet& aEffectSet, > - nsCSSPropertyIDSet& aPropertiesOverridden); > + nsCSSPropertyIDSet& aPropertiesOverridden, > + StyleBackendType aType); |aPropertiesOverridden| is an out param so it should go last. Better yet, we should make this function return an nsCSSPropertyIDSet. ::: dom/animation/EffectCompositor.h:281 (Diff revision 3) > - GetOverriddenProperties(nsStyleContext* aStyleContext, > + GetOverriddenProperties(const dom::Element* aElement, > + CSSPseudoElementType aPseudoType, > + nsStyleContext* aStyleContext, > EffectSet& aEffectSet, > - nsCSSPropertyIDSet& aPropertiesOverridden); > + nsCSSPropertyIDSet& aPropertiesOverridden, > + StyleBackendType aType); It's probably also better to make |aType| the first parameter since that's what we use to determine which of the following parameters we actually look at (i.e. aElement/aPseudoType or aStyleContext). We should also rename |aType| to either |aBackendType| or |aBackend|. ::: dom/animation/EffectCompositor.cpp:843 (Diff revision 3) > + bool isServo = aType == StyleBackendType::Servo; > + if (!isServo && !aStyleContext) { > + return; > + } Is it a bit odd to have this method return early if |aStyleContext| is null? Would it be more clear if we: 1. Make the call site just not call this if |aBackend| is StyleBackendType::Gecko and |aStyleContext| is nullptr. 2. Just assert that here, e.g. MOZ_ASSERT(aBackend != StyleBackendType::Gecko || aStyleContext, "Should have a style context if we are using the Gecko backend"); MOZ_ASSERT(aBackend != StyleBackendType::Servo || aElement, "Should have an element to get style data from if we are using" " the Servo backend"); (See comments below.) ::: dom/animation/EffectCompositor.cpp:873 (Diff revision 3) > if (propertiesToTrack.IsEmpty()) { > return; > } > > + if (isServo) { > + MOZ_ASSERT(aElement); Move this assert to the top of the function? (See comments above.) ::: servo/components/style/rule_tree/mod.rs:811 (Diff revision 3) > + let iter = self.self_and_ancestors() > + .skip_while(|node| node.cascade_level() == CascadeLevel::Transitions) > + .take_while(|node| node.cascade_level() > CascadeLevel::Animations); > + let mut result = HashSet::new(); > + for node in iter { > + if let Some(style) = node.style_source() { > + let ids = style.read(guard) > + .declarations() > + .iter() > + .filter_map(|&(ref decl, imp)| { > + if imp == Importance::Important { > + decl.id().to_nscsspropertyid().ok() I'm curious how this works. The cascade is here: https://drafts.csswg.org/css-cascade-3/#cascade-origin If I understand correctly, our |iter| is already going to ensure we only visit important declarations. So do we need to check imp == Importance::Important?
Attachment #8839103 - Flags: review?(bbirtles)
Comment on attachment 8839106 [details] Bug 1334036 - Part 8: Add AddLayerChangesForAnimation in ServoRestyleManager. https://reviewboard.mozilla.org/r/113694/#review140378 Looks good but I'd like to check again after we factor out the common code here. ::: layout/base/ServoRestyleManager.cpp:139 (Diff revision 3) > +static void > +AddLayerChangesForAnimation(nsIFrame* aFrame, > + Element* aElement, > + nsStyleChangeList& aChangeListToProcess) > +{ > + if (!aFrame || !aElement) { > + return; > + } > + > + uint64_t frameGeneration = > + RestyleManager::GetAnimationGenerationForFrame(aFrame); > + > + nsChangeHint hint = nsChangeHint(0); > + for (const LayerAnimationInfo::Record& layerInfo : > + LayerAnimationInfo::sRecords) { > + layers::Layer* layer = > + FrameLayerBuilder::GetDedicatedLayer(aFrame, layerInfo.mLayerType); > + if (layer && frameGeneration != layer->GetAnimationGeneration()) { > + // If we have a transform layer but don't have any transform style, we > + // probably just removed the transform but haven't destroyed the layer > + // yet. In this case we will add the appropriate change hint > + // (nsChangeHint_UpdateContainingBlock) when we compare style contexts > + // so we can skip adding any change hint here. (If we *were* to add > + // nsChangeHint_UpdateTransformLayer, ApplyRenderingChangeToTree would > + // complain that we're updating a transform layer without a transform). > + if (layerInfo.mLayerType == nsDisplayItem::TYPE_TRANSFORM && > + !aFrame->StyleDisplay()->HasTransformStyle()) { > + continue; > + } > + hint |= layerInfo.mChangeHint; > + } > + > + // We consider it's the first paint for the frame if we have an animation > + // for the property but have no layer. > + // Note that in case of animations which has properties preventing running > + // on the compositor, e.g., width or height, corresponding layer is not > + // created at all, but even in such cases, we normally set valid change > + // hint for such animations in each tick, i.e. restyles in each tick. As > + // a result, we usually do restyles for such animations in every tick on > + // the main-thread. The only animations which will be affected by this > + // explicit change hint are animations that have opacity/transform but did > + // not have those properies just before. e.g, setting transform by > + // setKeyframes or changing target element from other target which prevents > + // running on the compositor, etc. > + if (!layer && > + nsLayoutUtils::HasEffectiveAnimation(aFrame, layerInfo.mProperty)) { > + hint |= layerInfo.mChangeHint; > + } > + } > + > + if (hint) { > + aChangeListToProcess.AppendChange(aFrame, aElement, hint); > + } > +} This is about ~90% the same as ElementRestyler::AddLayerChangesForAnimation() We should factor our a common utility method for this common code. ::: layout/base/ServoRestyleManager.cpp:307 (Diff revision 3) > + // Some changes to animations don't affect the computed style and yet still > + // require the layer to be updated. For example, pausing an animation via > + // the Web Animations API won't affect an element's style but still > + // requires us to pull the animation off the layer. > + AddLayerChangesForAnimation(styleFrame, aElement, aChangeList); The comment before calling this method in ElementRestyler::Restyle mentions that we need to call it *after* calling RestyleSelf. Do we have the same requirements here? (It's not totally clear to me why this is the right place to call this. Are we sure it is?) ::: layout/base/ServoRestyleManager.cpp:309 (Diff revision 3) > + // the Web Animations API won't affect an element's style but still > + // requires us to pull the animation off the layer. This is not quite right anymore. We don't necessarily pull paused animations of layers anymore. We should change this comment (and the one in ElementRestyler::Restyle) to say, "... but still requires to update the animation on the layer."
Attachment #8839106 - Flags: review?(bbirtles)
Comment on attachment 8865334 [details] Bug 1334036 - Part 12: Enable off-main thread animations. https://reviewboard.mozilla.org/r/136972/#review140388 r=me but shouldn't this be combined with the next two patches to make this commit atomic?
Attachment #8865334 - Flags: review?(bbirtles) → review+
Comment on attachment 8865334 [details] Bug 1334036 - Part 12: Enable off-main thread animations. https://reviewboard.mozilla.org/r/136972/#review140390 ::: dom/animation/EffectCompositor.cpp (Diff revision 1) > EffectSet* effects = EffectSet::GetEffectSet(aFrame); > if (!effects || effects->IsEmpty()) { > return false; > } > > - // FIXME: Bug 1334036: stylo: Implement off-main-thread animations. What about the comments referencing this bug in: * EffectCompositor::RequestRestyle * GetMinAndMaxScaleForAnimationProperty * AddAnimationForProperty * layout/reftests/invalidation/reftest.list Can we drop any of them? (maybe we did in one of these patches and I missed it?) Otherwise we should update them to point to the follow-up bug where we will fix them.
Comment on attachment 8839102 [details] Bug 1334036 - Part 7: Merge two similiar MaybeUpdateCascadeResults functions. https://reviewboard.mozilla.org/r/113642/#review139932 Clearing review for now because I don't really understand the desired behavior so I'll wait for the explanation of part 1 before reviewing this. (Also, if at all possible, it would be nice to include this work in PreTraverse and drop the extra mHasDifferentAnimationsLevel flag -- but I don't understand enough to know if that will be possible.) ::: dom/animation/EffectCompositor.cpp:739 (Diff revision 3) > // If we have transition properties for compositor and if the same propery > // for animations level is newly added or removed, we need to update layers > // for transitions level because composite order has been changed now. > if (prevCompositorPropertiesForAnimationsLevel != > GenerateCompositorPropertiesInSet(propertiesForAnimationsLevel)) { > - // FIXME: We will set a flag and trigger a restyle later. > + effects->SetHasDifferentAnimaitonsLevel(true); Animations ::: dom/animation/EffectCompositor.cpp:1133 (Diff revision 3) > + static CSSPseudoElementType sAnimablePseudos[3] = { > + CSSPseudoElementType::NotPseudo, > + CSSPseudoElementType::before, > + CSSPseudoElementType::after > + }; Can't you omit the '3' here? ::: dom/animation/EffectCompositor.cpp:1138 (Diff revision 3) > + static CSSPseudoElementType sAnimablePseudos[3] = { > + CSSPseudoElementType::NotPseudo, > + CSSPseudoElementType::before, > + CSSPseudoElementType::after > + }; > + for (size_t i = 0; i < 3; ++i) { You can't use ArrayLength (see ArrayUtils.h) here? Or, better still, a range-based for loop? ::: dom/animation/EffectCompositor.cpp:1143 (Diff revision 3) > + // Actually, we only need to do MaybeUpdateCascadeResults for elements in > + // mElementsToRestyle we added before current tick; however, we clear them in > + // the previous PreTraverse(), so instead, we use dirty bit to traverse the > + // subtree, just like what we do in the ProcessPostTraversal. An alternative > + // is put this in SequentialTask, so we don't need re-traverse the subtree. > + // We may need to do that if re-traverse has an impact on performance. I think I should probably just wait for your explanation of part 1 because I don't quite follow. We say we can't use mElementsToRestyle (and comment 34 says the same thing) but then at the call site for this, the next thing we do is call PreTraverse(InSubtree) which relies on mElementsToRestyle being set. So I don't really understand the problem. ::: dom/animation/EffectSet.h:255 (Diff revision 3) > // animations level of the cascade and hence should be skipped when we are > // composing the animation style for the transitions level of the cascede. > nsCSSPropertyIDSet mPropertiesForAnimationsLevel; > > + // A flag represents that the mPropertiesForAnimationsLevel is updated and is > + // different from its origin value in PreTraverse(). We use this flag to know original value?
Attachment #8839102 - Flags: review?(bbirtles)
Comment on attachment 8839103 [details] Bug 1334036 - Part 5: Implement FFI for finding properties overriding animations. https://reviewboard.mozilla.org/r/113644/#review140340 > Is it a bit odd to have this method return early if |aStyleContext| is null? Would it be more clear if we: > > 1. Make the call site just not call this if |aBackend| is StyleBackendType::Gecko and |aStyleContext| is nullptr. > 2. Just assert that here, e.g. > > MOZ_ASSERT(aBackend != StyleBackendType::Gecko || aStyleContext, > "Should have a style context if we are using the Gecko backend"); > MOZ_ASSERT(aBackend != StyleBackendType::Servo || aElement, > "Should have an element to get style data from if we are using" > " the Servo backend"); > > (See comments below.) Now only GetOverriddenProperties() use ```nsStyleContext```, so I'd like to move the code of retrieving ```nsStyleContext``` into ```GetOverriddenProperties()```. I will update this patch soon to let you re-review it. Thanks. > I'm curious how this works. The cascade is here: > > https://drafts.csswg.org/css-cascade-3/#cascade-origin > > If I understand correctly, our |iter| is already going to ensure we only visit important declarations. So do we need to check imp == Importance::Important? I guess this may be a Servo bug? I add imp.important() here because I notice that we still have some PropertyDeclataion with _Normal_ importance in these cascade_levels, so still need to filter them out. e.g. ``` #target { width: 100px; height: 100px; background-color: blue; transform: translateX(123px) !important; } ``` I dumped |```style```| in this function: Style(StyleRule { selectors: SelectorList([Selector(#target, specificity = 0x100000)]), block: [(width: 100px, Normal), (height: 100px, Normal), (background-color: blue, Normal), (transform: translateX(123px), Important)] }) We still have _Normal_ importance.
Oh I see, so it's just because of the way Servo stores declarations as declaration blocks. Fair enough, that makes sense I guess.
I just updated the commit message of part 1. I think I should also explain part 1 and part 3 here: 1. a) EffectCompositor::PreTravere() (Main thread) * (Part 1): Update aEffectSet.PropertiesForAnimationsLevel(). There are two reasons I want to simplify this function, MaybeUpdatePropertiesForAnimationsLevel(). 1) In PreTraverse(): for (auto iter = mElementsToRestyle[cascadeLevel].Iter(); !iter.Done(); iter.Next()) { ... MaybeUpdatePropertiesForAnimationsLevel(target.mElement, target.mPseudoType); ... } MaybeUpdatePropertiesForAnimationsLevel(...) is in the loop of mElementsToRestyle. If we call EffectCompositor::RequestRestyle() in MaybeUpdatePropertiesForAnimationsLevel(...), which also update mElementsToRestyle, and this may cause undefined behaviors, so we have to avoid it. That is why I try to _delay_ the calling of "EffectCompositor::RequestRestyle()". 2) If we call EffectCompositor::RequestRestyle() in the first PreTraverse(), we assert [1]. Let's see the code in : if (mPresContext->StyleSet()->IsServo()) { if (ServoStyleSet::IsInServoTraversal()) { ... return; } else { MOZ_ASSERT(!mPresContext->RestyleManager()->IsInStyleRefresh()); // We got this assertion. } } ServoStyleSet::IsInServoTraversal() becomes true only after the first PreTraverse() [2], so we will enter the *else* block, and "mPresContext->RestyleManager()->IsInStyleRefresh()" is true during entire restyle process [3]. So now MaybeUpdatePropertiesForAnimationsLevel(...) only update aEffectSet.PropertiesForAnimationsLevel(), and add a flag to know if PropertiesForAnimationsLevel() is changed, so step (e) can handle it. b) Animation only Traversal c) Normal Traversal d) The SequentialTask e) (Part 3) Call MaybeUpdateCascadeResults() (Main Thread) * MaybeUpdateCascadeResults() just do the same things as that in Gecko: we try to update important properties and properties for animations level. * This function should be called after we finish cascade. * May request restyles if there are new added animations (from (a) or new added css animation in this tick) or new added !important rules. 2. f) Second PreTraversal g) Second Animation only Traversal h) Second Normal Traversal 3. PostTraversal * (Part 4) Call AddLayerChangesForAnimation because I think ProcessPostTraversal() could be an equivalent position that after we call the same function in Gecko (i.e. after RestyleSelf). (Yeah, Heycam suggested me call that in ProcessPostTraversal() many weeks ago.) [1] http://searchfox.org/mozilla-central/rev/8b70b0a5038ef9472fe7c53e04a70c978bb06aed/dom/animation/EffectCompositor.cpp#332 [2] http://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/layout/style/ServoStyleSet.cpp#298 [3] http://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/layout/base/ServoRestyleManager.cpp#353-390
Comment on attachment 8839106 [details] Bug 1334036 - Part 8: Add AddLayerChangesForAnimation in ServoRestyleManager. https://reviewboard.mozilla.org/r/113694/#review140378 > The comment before calling this method in ElementRestyler::Restyle mentions that we need to call it *after* calling RestyleSelf. Do we have the same requirements here? > > (It's not totally clear to me why this is the right place to call this. Are we sure it is?) It seems to me that calling AddLayerChangesForAnimation() here is really like calling it after ElementRestyler::RestyleSelf. We resolve servo style and do UpdateStyleOfownedAnonBoxes() before calling AddLayerChangesForAnimation(), so this place should be ok for now.
Comment on attachment 8865334 [details] Bug 1334036 - Part 12: Enable off-main thread animations. https://reviewboard.mozilla.org/r/136972/#review140388 > shouldn't this be combined with the next two patches to make this commit atomic? I will merge this commit with the next two patches. Thanks.
Comment on attachment 8865334 [details] Bug 1334036 - Part 12: Enable off-main thread animations. https://reviewboard.mozilla.org/r/136972/#review140390 > What about the comments referencing this bug in: > > * EffectCompositor::RequestRestyle > * GetMinAndMaxScaleForAnimationProperty > * AddAnimationForProperty > * layout/reftests/invalidation/reftest.list > > Can we drop any of them? (maybe we did in one of these patches and I missed it?) Otherwise we should update them to point to the follow-up bug where we will fix them. Oops. I didn't noticed these comments. Thanks a lot. I will update it.
Attachment #8865335 - Flags: review+ → review?(hikezoe)
Comment on attachment 8839102 [details] Bug 1334036 - Part 7: Merge two similiar MaybeUpdateCascadeResults functions. https://reviewboard.mozilla.org/r/113642/#review139932 > (Also, if at all possible, it would be nice to include this work in PreTraverse and > drop the extra mHasDifferentAnimationsLevel flag -- but I don't understand enough to know if that will be possible.) I still don't have any good idea to update cascade result (i.e. both important rules and properties for aniamtions level) during iterating mElementsToRestyle (i.e. PreTraverse()). we should update important rules after cascade, but need to update properties for animations level before both 1st and 2nd animation-only restyle. If we try to update important rules before cascade (i.e. normal restyle), we will do nothing on EffectSet::mPropertiesWithImportantRules but consume EffectSet::mCascadeNeedsUpdate in the current tick.
Comment on attachment 8865335 [details] Bug 1334036 - Part 10: Return AnimationValue for BaseStyle. https://reviewboard.mozilla.org/r/136974/#review140978 Hi MozReview, did I stamp r+ for this patch? ::: dom/animation/KeyframeEffectReadOnly.h:278 (Diff revision 2) > - StyleAnimationValue BaseStyle(nsCSSPropertyID aProperty) const > + AnimationValue BaseStyle(nsCSSPropertyID aProperty, > + StyleBackendType aBackendType) const > { This function looks odd to me. The function returns an abstract struct (AnimationValue), whereas the function needs a concrete backend type as an argument...
Attachment #8865335 - Flags: review?(hikezoe)
(In reply to Boris Chiou [:boris] from comment #99) > Comment on attachment 8839102 [details] > Bug 1334036 - Part 4: Delay the request style from cascade update and add > TraverseCascadeUpdate. > > https://reviewboard.mozilla.org/r/113642/#review139932 > > > (Also, if at all possible, it would be nice to include this work in PreTraverse and > > drop the extra mHasDifferentAnimationsLevel flag -- but I don't understand enough to know if that will be possible.) > > I still don't have any good idea to update cascade result (i.e. both > important rules and properties for aniamtions level) during iterating > mElementsToRestyle (i.e. PreTraverse()). we should update important rules > after cascade, but need to update properties for animations level before > both 1st and 2nd animation-only restyle. If we try to update important rules > before cascade (i.e. normal restyle), we will do nothing on > EffectSet::mPropertiesWithImportantRules but consume > EffectSet::mCascadeNeedsUpdate in the current tick. An alternative way is move MaybeUpdatePropertiesForAnimationsLevel outside the loop, and traverse each element with the dirty bit, so we don't need to use the extra flag. However, we don't consume EffectSet::mCascadeNeedsUpdate in MaybeUpdatePropertiesForAnimationsLevel(), so MaybeUpdateCascadeUpdate() (which called after the 1st animation-only/normal restyle) still work and we can update important rules properly there.
Comment on attachment 8865335 [details] Bug 1334036 - Part 10: Return AnimationValue for BaseStyle. https://reviewboard.mozilla.org/r/136974/#review140978 > This function looks odd to me. The function returns an abstract struct (AnimationValue), whereas the function needs a concrete backend type as an argument... OK, I will remove StyleBackendType. So we check both mBaseStyleValues and mBaseStyleValuesForServo, and if one of them has the value, (Servo backend first), we return the AnimationValue which contains that value. Or should we combine mBaseStyleValues and mBaseStyleValuesForServo together? into something like ```nsDataHashtable<nsUint32HashKey, AnimationValue>```. However, AnimationValue contains a RefPtr, so I'm not sure if there is any performance impact. BTW, maybe we can move this function into AnimationValue, and add a method like: ```AnimationValue::FromKeyframeEffectBaseStyle(KeyframeEffectReadOnly);```
Comment on attachment 8865335 [details] Bug 1334036 - Part 10: Return AnimationValue for BaseStyle. https://reviewboard.mozilla.org/r/136974/#review140978 > OK, I will remove StyleBackendType. So we check both mBaseStyleValues and mBaseStyleValuesForServo, and if one of them has the value, (Servo backend first), we return the AnimationValue which contains that value. > > Or should we combine mBaseStyleValues and mBaseStyleValuesForServo together? into something like ```nsDataHashtable<nsUint32HashKey, AnimationValue>```. However, AnimationValue contains a RefPtr, so I'm not sure if there is any performance impact. > > > BTW, maybe we can move this function into AnimationValue, and add a method like: ```AnimationValue::FromKeyframeEffectBaseStyle(KeyframeEffectReadOnly);``` Can't we just check mDocument->IsStyledByServo()?
Comment on attachment 8866245 [details] Bug 1334036 - Part 10: Use &[ComputedOperation] as the argument type of convert_transform. https://reviewboard.mozilla.org/r/137866/#review140992
Attachment #8866245 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8865335 [details] Bug 1334036 - Part 10: Return AnimationValue for BaseStyle. https://reviewboard.mozilla.org/r/136974/#review140978 > Can't we just check mDocument->IsStyledByServo()? Oh yes, I totally forget ```mDocument```. Thanks.
Comment on attachment 8865335 [details] Bug 1334036 - Part 10: Return AnimationValue for BaseStyle. https://reviewboard.mozilla.org/r/136974/#review141012
Attachment #8865335 - Flags: review?(hikezoe) → review+
Attachment #8839101 - Flags: review?(bbirtles)
Attachment #8865336 - Flags: review?(bbirtles)
Attachment #8839103 - Flags: review?(emilio+bugs)
Attachment #8839103 - Flags: review?(bbirtles)
Attachment #8839106 - Flags: review?(bbirtles)
Comment on attachment 8865336 [details] Bug 1334036 - Part 4: Remove unused UpdateCascadeResults function. The r+ in part 2 is a mozreview bug. :(
Attachment #8865336 - Flags: review?(hikezoe)
Attachment #8839101 - Flags: review?(bbirtles)
Attachment #8839103 - Flags: review?(bbirtles)
Attachment #8839103 - Flags: review?(emilio+bugs)
Attachment #8839106 - Flags: review?(bbirtles)
Component: DOM: Animation → CSS Parsing and Computation
Comment on attachment 8839103 [details] Bug 1334036 - Part 5: Implement FFI for finding properties overriding animations. https://reviewboard.mozilla.org/r/113644/#review141016 ::: dom/animation/EffectCompositor.cpp:852 (Diff revision 5) > > + Element* elementToRestyle = GetElementToRestyle(aElement, aPseudoType); > + switch (aBackendType) { > + case StyleBackendType::Servo: > + Servo_GetProperties_Overriding_Animation(elementToRestyle > + ? elementToRestyle Can `elementToRestyle` be null here? Do we have animation restyles for non-existing pseudos? I know for gecko we do, as discovered in bug 1331047, but I don't think that's true for Servo. If it can be null for the servo backend, then wouldn't this get confused with the parent's animation? Is that intended? ::: dom/animation/EffectCompositor.cpp:862 (Diff revision 5) > + case StyleBackendType::Gecko: { > + nsStyleContext* styleContext = aStyleContext; > + if (!styleContext) { > + if (elementToRestyle) { > + nsIFrame* frame = elementToRestyle->GetPrimaryFrame(); > + if (frame) { nit: you save a line if you do: `if (nsIFrame* frame = elementToRestyle->GetPrimaryFrame())` ::: servo/ports/geckolib/glue.rs:1107 (Diff revision 5) > #[no_mangle] > +pub extern "C" fn Servo_GetProperties_Overriding_Animation(element: RawGeckoElementBorrowed, > + list: nsTArrayBorrowed_nsCSSPropertyID, > + set: nsCSSPropertyIDSetBorrowedMut) { > + let element = GeckoElement(element); > + let elementData = element.borrow_data().unwrap(); nit: use `snake_case` for variable names.
Attachment #8839103 - Flags: review?(emilio+bugs)
Comment on attachment 8839103 [details] Bug 1334036 - Part 5: Implement FFI for finding properties overriding animations. https://reviewboard.mozilla.org/r/113644/#review141016 > Can `elementToRestyle` be null here? Do we have animation restyles for non-existing pseudos? I know for gecko we do, as discovered in bug 1331047, but I don't think that's true for Servo. > > If it can be null for the servo backend, then wouldn't this get confused with the parent's animation? Is that intended? Yes, you are right. I thought it is possible that we still haven't generated the pseudo element after the first restyle. We call this function only if the pseudo element has animations (i.e. EffectSet is non-null) and ehe first animation-only and normal restyle are finished, so here we should get a valid ```elementToRestyle```.
(In reply to Boris Chiou [:boris] from comment #118) > Comment on attachment 8839103 [details] > Bug 1334036 - Part 3: Implement FFI for finding properties overriding > animations. > > https://reviewboard.mozilla.org/r/113644/#review141016 > > > Can `elementToRestyle` be null here? Do we have animation restyles for non-existing pseudos? I know for gecko we do, as discovered in bug 1331047, but I don't think that's true for Servo. > > > > If it can be null for the servo backend, then wouldn't this get confused with the parent's animation? Is that intended? > > Yes, you are right. I thought it is possible that we still haven't generated > the pseudo element after the first restyle. We call this function only if > the pseudo element has animations (i.e. EffectSet is non-null) and ehe first > animation-only and normal restyle are finished, so here we should get a > valid ```elementToRestyle```. So according to bug 1331047, the element we restyle is the generated pseudo element or normal element in Servo. However, in Gecko side, I'd like to make sure one thing: In ServoStyleSet::PrepareAndTraverseSubtree() [1], we traverse the RawGeckoElementBorrowed, |aRoot|. If the element here has a before of after pseudo, is the element we traverse the parent dom element of its before/after pseudo, or is it the generated before/after element? Thanks. [1] http://searchfox.org/mozilla-central/rev/7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/layout/style/ServoStyleSet.cpp#288
Flags: needinfo?(emilio+bugs)
It's the generated content, so presumably we may want to special-case there using IsGeneratedContentContainerFor{Before,After}.
Flags: needinfo?(emilio+bugs)
Comment on attachment 8865336 [details] Bug 1334036 - Part 4: Remove unused UpdateCascadeResults function. https://reviewboard.mozilla.org/r/136976/#review141390 I think this part is OK, but should be reviwed by Brian because it depends on other patches (i.e. how we treat cascading results and where we should do it, etc.). ::: dom/animation/EffectCompositor.h:268 (Diff revision 4) > GetOverriddenProperties(nsStyleContext* aStyleContext, > EffectSet& aEffectSet, > nsCSSPropertyIDSet& aPropertiesOverridden); > > + // Update the mPropertiesWithImportantRules and > + // mPropertiesForAnimationsLevel members of the corresponding EffectSet. Nit: of the given EffectSet?
Attachment #8865336 - Flags: review?(hikezoe) → review+
Thank you for the detailed explanation! However, I'm still not completely convinced this is the right setup. See comments below. (In reply to Boris Chiou [:boris] from comment #85) > I just updated the commit message of part 1. I think I should also explain > part 1 and part 3 here: > > 1. > a) EffectCompositor::PreTravere() (Main thread) > * (Part 1): Update aEffectSet.PropertiesForAnimationsLevel(). > There are two reasons I want to simplify this function, > MaybeUpdatePropertiesForAnimationsLevel(). > 1) In PreTraverse(): > > for (auto iter = mElementsToRestyle[cascadeLevel].Iter(); > !iter.Done(); iter.Next()) { > > ... > MaybeUpdatePropertiesForAnimationsLevel(target.mElement, > target.mPseudoType); > ... > } > > MaybeUpdatePropertiesForAnimationsLevel(...) is in the loop of > mElementsToRestyle. > If we call EffectCompositor::RequestRestyle() in > MaybeUpdatePropertiesForAnimationsLevel(...), > which also update mElementsToRestyle, and this may cause undefined > behaviors, so we have to avoid it. > That is why I try to _delay_ the calling of > "EffectCompositor::RequestRestyle()". We should just copy mElementsToRestyle before iterating over it like we do in EffectCompositor::AddStyleUpdatesTo. > 2) If we call EffectCompositor::RequestRestyle() in the first > PreTraverse(), we assert [1]. > Let's see the code in : > > if (mPresContext->StyleSet()->IsServo()) { > if (ServoStyleSet::IsInServoTraversal()) { > ... > return; > } else { > MOZ_ASSERT(!mPresContext->RestyleManager()->IsInStyleRefresh()); // > We got this assertion. > } > } > > ServoStyleSet::IsInServoTraversal() becomes true only after the first > PreTraverse() [2], so we will enter the *else* > block, and "mPresContext->RestyleManager()->IsInStyleRefresh()" is true > during entire restyle process [3]. That sounds like we should fix the assertion. > So now MaybeUpdatePropertiesForAnimationsLevel(...) only update > aEffectSet.PropertiesForAnimationsLevel(), > and add a flag to know if PropertiesForAnimationsLevel() is changed, so > step (e) can handle it. That seems like the wrong place to handle it. Shouldn't we handle any restyles from existing changes to the cascade in the first traversal? Won't deferring the update mean we sometimes do two traversals when we really only need one?
Attachment #8839101 - Flags: review?(bbirtles)
Attachment #8839103 - Flags: review?(bbirtles)
Attachment #8839106 - Flags: review?(bbirtles)
I will rewrite part 1 and try to drop the flag and merge MaybeUpdateCascadeNeedsUpdate into PreTraverse.
Comment on attachment 8865336 [details] Bug 1334036 - Part 4: Remove unused UpdateCascadeResults function. https://reviewboard.mozilla.org/r/136976/#review141390 > should be reviwed by Brian because it depends on other patches Thanks, hiro. I will send review request to Brian later.
(In reply to Brian Birtles (:birtles) from comment #122) > That seems like the wrong place to handle it. Shouldn't we handle any > restyles from existing changes to the cascade in the first traversal? Won't > deferring the update mean we sometimes do two traversals when we really only > need one? Fixing the assertion and copy mElementsToRestyle are easy, but the timing of update cascade results is not easy to choose. Start to think that should we split the updating cascade results into two functions, one for important rules and one for properties for animations level? The reason is: 1) In PreTraverse(), we iterate mElementsToRestyle and update properties for animations level. This is OK because we only need to check the elements which need to be restyled for animations. 2) However, the change of important rules shouldn't be bounded to mElementsToRestyle. We update the important rules on elements which have animations and the style attribute of element is just changed. That is why we call MarkCascadeNeedUpdate() on those elements in UpdateEffectProperties. Therefore, we update the important rules only after the first traversal because we have to wait that cascading important rules is finished. For the updating of properties for animations level, we can put it in the current place (i.e. in the loop of mElementsToRestyle in PreTraverse()). e.g. a) 1st traversal 1) PreTraverse() * Update properties for animations level 2) Animation-only 3) Normal * we cascade the newly added/removed *important* rules, if there is a style change on the property. 4) UpdateEffectProperties * This is called because we update the style. * call EffectSet::MarkCascadeNeedsUpdate() * This won't call RequestRestyle because KeyframeEffectReadOnly::mProperties is not changed. b) 2nd traversal 1) PreTraverse() * Check if we need to update important rules. (Outside the loop, so we may maintain a list for it?) * Update properties for animations level if needed. (This is in the loop of mElementsToRestyle) 2) ... What do you think, Brian?
Flags: needinfo?(bbirtles)
Remove the ni. We decide to call RequestRestyle() if the important rules are changed while/after calling UpdateEffectProperties. The problem is how to know it is changed. 1. Check in servo side, i.e. process_animations(), and then send a SequentialTask to update cascade result. The problem is how to know the important rules are changed. The |primary_rules| is *new* in cascade_primary_or_pseudo(). Maybe we need to add a flag in matching code, so we can know it. However, not sure how many things we have to do. 2. Check in gecko side. i.e. Gecko_UpdateAnimations(). In this function, if we have a task for UpdateEffectPropeties(), we can also check if effectSet->PropertiesWithImportantRules() is equal to the new one. This is much easier, I guess, but need to get the overridden properties more than once. (i.e. one in Gecko_UpdateAnimations(), one in MaybeUpdateCascadeResults() in the 2nd pre-traversal). However, this approach doesn't cause false alarm. Either 1) or 2) trigger 2nd traversal. The potential problem is: we trigger the 2nd traversal, so if there is a significant time difference between 1st and 2nd traversal, the interpolation results of the 1st animation-only restyle and the 2nd animation-only restyle may be different (if there is any animation running on the main thread).
Flags: needinfo?(bbirtles)
(In reply to Boris Chiou [:boris] from comment #126) > Remove the ni. > > We decide to call RequestRestyle() if the important rules are changed > while/after calling UpdateEffectProperties. The problem is how to know it is > changed. > > 1. Check in servo side, i.e. process_animations(), and then send a > SequentialTask to update cascade result. > The problem is how to know the important rules are changed. The > |primary_rules| is *new* in cascade_primary_or_pseudo(). I think we have still old primary rules before we call set_primary_rules().
(In reply to Hiroyuki Ikezoe (:hiro) from comment #127) > (In reply to Boris Chiou [:boris] from comment #126) > > Remove the ni. > > > > We decide to call RequestRestyle() if the important rules are changed > > while/after calling UpdateEffectProperties. The problem is how to know it is > > changed. > > > > 1. Check in servo side, i.e. process_animations(), and then send a > > SequentialTask to update cascade result. > > The problem is how to know the important rules are changed. The > > |primary_rules| is *new* in cascade_primary_or_pseudo(). > > I think we have still old primary rules before we call set_primary_rules(). When reframing happens, the old primary rules have gone, I am not sure what happens in that case.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #127) > (In reply to Boris Chiou [:boris] from comment #126) > > Remove the ni. > > > > We decide to call RequestRestyle() if the important rules are changed > > while/after calling UpdateEffectProperties. The problem is how to know it is > > changed. > > > > 1. Check in servo side, i.e. process_animations(), and then send a > > SequentialTask to update cascade result. > > The problem is how to know the important rules are changed. The > > |primary_rules| is *new* in cascade_primary_or_pseudo(). > > I think we have still old primary rules before we call set_primary_rules(). I also think so, but match_primary() is just one of the cases we need to handle. Another code path is CascadeWithReplacements() [1]. This is happened when we clear the important rules [2]. In this case, we call replace_rules(). Therefore, we might need to handle both cases. Need more time to check them. [1] http://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/servo/components/style/traversal.rs#749-753 [2] http://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/dom/animation/test/chrome/test_running_on_compositor.html#555
(In reply to Boris Chiou [:boris] from comment #129) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #127) > > (In reply to Boris Chiou [:boris] from comment #126) > > > Remove the ni. > > > > > > We decide to call RequestRestyle() if the important rules are changed > > > while/after calling UpdateEffectProperties. The problem is how to know it is > > > changed. > > > > > > 1. Check in servo side, i.e. process_animations(), and then send a > > > SequentialTask to update cascade result. > > > The problem is how to know the important rules are changed. The > > > |primary_rules| is *new* in cascade_primary_or_pseudo(). > > > > I think we have still old primary rules before we call set_primary_rules(). > > I also think so, but match_primary() is just one of the cases we need to > handle. Another code path is CascadeWithReplacements() [1]. This is happened > when we clear the important rules [2]. In this case, we call > replace_rules(). Therefore, we might need to handle both cases. Need more > time to check them. Ah, right. The case does not doing match just do cascading. But in that case we still have the old rules there.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #130) > Ah, right. The case does not doing match just do cascading. But in that case > we still have the old rules there. Yes, maybe in update_rule_at_level() we can do something like that if |pdb| is important, we set an extra flag. Now I'm thinking where is a suitable place to store the flag, or just add a method argument and pass the flag to cascade_primary_or_pseudo().
Depends on: 1346049
Comment on attachment 8839103 [details] Bug 1334036 - Part 5: Implement FFI for finding properties overriding animations. https://reviewboard.mozilla.org/r/113644/#review142550 The bindings and the servo functions look sensible to me, with the following nit addressed. Thanks! ::: layout/style/ServoBindingList.h:175 (Diff revision 6) > nsCSSPropertyID property) > SERVO_BINDING_FUNC(Servo_Property_IsDiscreteAnimatable, bool, > nsCSSPropertyID property) > +SERVO_BINDING_FUNC(Servo_GetProperties_Overriding_Animation, void, > + RawGeckoElementBorrowed, > + nsTArrayBorrowed_nsCSSPropertyID, I think you can just use `const nsTArray<nsCSSPropertyID>*`. The `uintptr_t` thing was a workaround for a bindgen bug related to typedefs that is fixed in libclang 4.0, but this should not be affected I believe.
Attachment #8839103 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8867632 [details] Bug 1334036 - Part 6: Trigger restyle if important rules are changed. https://reviewboard.mozilla.org/r/139198/#review142552 I think this patch might be way cleaner. ::: servo/components/style/matching.rs:963 (Diff revision 1) > + /// rules. > fn match_primary(&self, > context: &mut StyleContext<Self>, > data: &mut ElementData, > relations: &mut StyleRelations) > - -> bool > + -> (bool, bool) ::: servo/components/style/matching.rs:963 (Diff revision 1) > + /// rules. > fn match_primary(&self, > context: &mut StyleContext<Self>, > data: &mut ElementData, > relations: &mut StyleRelations) > - -> bool > + -> (bool, bool) I think the return value of this function is not really accurate. This only returns the second boolean if there are animations. This should be at least documented. Also, perhaps it's worth changing the return value to something else that is not a tuple of booleans. For `replace_rules` I suggest bitflags below, here it may be a bit more difficult to find a type that suits... Perhaps a struct with two booleans (`rule_node_changed`, `important_rules_overriding_animation_changed`) is good enough? That would make it clearer I think, what do you think? ::: servo/components/style/matching.rs:1043 (Diff revision 1) > let primary_rule_node = > compute_rule_node::<Self>(&stylist.rule_tree, > &mut applicable_declarations, > &context.shared.guards); > > - return data.set_primary_rules(primary_rule_node); > + let important_rules_changed = if self.has_animations() && data.has_styles() { Maybe: ``` let important_rules_changed = self.has_animations() && data.has_styles() && data.important_rules_are_different(..); ``` (Though I wonder how can we have animations without having styles? Via the DOM API maybe?) ::: servo/components/style/matching.rs:1192 (Diff revision 1) > + /// and second true if the important rules changed. > fn replace_rules(&self, > hint: RestyleHint, > context: &StyleContext<Self>, > data: &mut AtomicRefMut<ElementData>) > - -> bool { > + -> (bool, bool) { Perhaps converting this two booleans here, is cleaner, either in a `RulesChanged` struct or some enum? like `{ NoChanges, ImportantRulesChanged, NormalRulesChanged, NormalAndImportantRulesChanged }`? You can implement convenience methods like: ``` impl RulesChanged { fn rule_changed(&mut self, level: CascadeLevel) { *self = match (*self, level.is_important()) { (RulesChanged::NoChanges, true) => RulesChanged::ImportantRulesChanged, (RulesChanged::NormalRulesChanged, true) | (RulesChanged::ImportantRulesChanged, false) => RulesChanged::NormalAndImportantRulesChanged, (RulesChanged::NoChanges, false) => RulesChanged::NormalRulesChanged, _ => *self, }; } fn important_rules_changed(&self) -> bool { matches!(*self, ImportantRulesChanged | NormalAndImportantRulesChanged) } } ``` Or even a `bitflags!`? That should be faster, and you can implement similar methods. Whatever you find cleaner (perhaps I like bitflags the most, if it matters). Then, to know if it changed you'd test `!is_empty()`, and to know if important rules changed you'd check `intersects(IMPORTANT_RULES)`, or something like that.
Attachment #8867632 - Flags: review?(emilio+bugs)
Comment on attachment 8839101 [details] Bug 1334036 - Part 1: Avoid mutating mElementsToRestyle during pre-traversal. https://reviewboard.mozilla.org/r/113640/#review142802 First a high-level comment about this patch series. I think we need to have two modes for this traversal: a) One where we just restyle elements that have posted animation restyles (i.e. skip throttled animations) b) One where we restyle all elements with animations (i.e. even if the animations are throttled) Gecko has this distinction and does: a) When there are only animation restyles b) When there are non-animation restyles OR when we handle an event with coordinates (PresShell::HandleEvent). I think with this patch series we still don't implement (b)? In PreTraverse, perhaps should we check if the regular dirty-bit is set on the root, and, if it is, do (b)? We'll still need to implement FlushThrottledStyles in PresShell.cpp, however. Looking at this patch, there are a few things I don't quite follow like: * Why is it ok, at the end of the function, to NOT remove elements from mElementsToRestyle that we called PostRestyleEventForAnimations on? * Why do we need to call PostRestyleEventForAnimations twice? * Why do we need to call WillComposeStyle twice? * Can we avoid calling nsContentUtils::ContentIsFlattenedTreeDescendantOf twice? I *think* what we want to do in this function is simply: 1. Bring mElementsToRestyle up-to-date to include any elements that have cascade updates. 2. Iterate over mElementsToRestyle and call PostRestyle / WillComposeStyle etc. (And for each of the above steps, take into account whether we want behavior (a) or (b) from above) So maybe we want something like: // Flush cascade updates -- Build up a hashset |elementsWithCascadeUpdates| by iterating *BOTH* cascade levels and adding: -- only elements that have posted a restyle if we are doing (a) -- possibly, we could check the effect set's "needs cascade update" flag here (If we don't check the needs cascade update flag, we'll want to call this something else like elementsThatMightNeedCascadeUpdates or elementsForPossibleCascadeUpdate or something) -- I suspect we could skip calling nsContentUtils::ContentIsFlattenedTreeDescendantOf here. That would mean we will end up calling MaybeUpdateCascadeResults on all elements needing a restyle but maybe that's ok? I'm not sure how often we call PreTraverseInSubtree and what the performance requirements are there. If we're not sure, it might be better to call ContentIsFlattenedTreeDescendantOf just in case. -- Also, I'm suggesting we *don't* remove elements from mElementsToRestyle at this point (That allows us to remove them in one place later, and not have to worry about calling ContentIsFlattenedTreeDescendantOf here) // Iterate over elementsWithCascadeUpdates -- Just call MaybeUpdateCascadeResults on them -- Then throw away elementsWithCascadeUpdates // Iterate over mElementsToRestyle by cascade level -- If we are doing (a) from above, skip elements that don't need a restyle -- Use nsContentUtils::ContentIsFlattenedTreeDescendantOf to skip elements not in this subtree -- Call PostRestyleEventForAnimations -- Set foundElementsNeedingRestyle = true here -- Drop the record from mElementsToRestyle if the effect set is missing (i.e. iter.Remove() -- Call WillComposeStyle -- Drop the record from mElementToRestyle Would that work?
Attachment #8839101 - Flags: review?(bbirtles)
Comment on attachment 8867631 [details] Bug 1334036 - Part 3: Add a flag to represent we are in pre-traversal. https://reviewboard.mozilla.org/r/139196/#review142816 I think it might be better just to use mozilla::AutoRestore. What do you think? ::: dom/animation/EffectCompositor.h:287 (Diff revision 1) > > static nsPresContext* GetPresContext(dom::Element* aElement); > > + // On construction, set mIsInPreTraverse to true for the given > + // EffectCompositor. On destruction, unset it. > + class MOZ_STACK_CLASS AutoSetInPreTraversal I suspect MOZ_RAII would be even better? Or, better still, just drop this class and use mozilla::AutoRestore? ::: dom/animation/EffectCompositor.h:289 (Diff revision 1) > > + // On construction, set mIsInPreTraverse to true for the given > + // EffectCompositor. On destruction, unset it. > + class MOZ_STACK_CLASS AutoSetInPreTraversal > + { > + friend EffectCompositor; I don't think you need this? In C++11 nested classes can access private members of their enclosing class. You only need this so the enclosing class can access the private destructor but there's no need to make that destructor private -- the inner class is private so no-one else will be using it. ::: dom/animation/EffectCompositor.h:299 (Diff revision 1) > + MOZ_ASSERT(aEffectCompositor && NS_IsMainThread()); > + aEffectCompositor->mIsInPreTraverse = true; > + } > + > + private: > + AutoSetInPreTraversal() = delete; I don't think this is needed since we have a user-declared constructor. ::: dom/animation/EffectCompositor.h:300 (Diff revision 1) > + ~AutoSetInPreTraversal() > + { > + mEffectCompositor->mIsInPreTraverse = false; > + mEffectCompositor = nullptr; > + } I think this should be public, right? (I think it only works now because you have 'friend EffectCompositor' ?) ::: dom/animation/EffectCompositor.h:306 (Diff revision 1) > + { > + mEffectCompositor->mIsInPreTraverse = false; > + mEffectCompositor = nullptr; > + } > + > + EffectCompositor* mEffectCompositor = nullptr; (Nit: You don't need the default initializer here since the only constructor for this always initializes this member.)
Attachment #8867631 - Flags: review?(bbirtles)
Comment on attachment 8865336 [details] Bug 1334036 - Part 4: Remove unused UpdateCascadeResults function. https://reviewboard.mozilla.org/r/136976/#review142822
Attachment #8865336 - Flags: review?(bbirtles) → review+
Comment on attachment 8839103 [details] Bug 1334036 - Part 5: Implement FFI for finding properties overriding animations. https://reviewboard.mozilla.org/r/113644/#review142824 Looks good with the following changes. I'll take another look though since it sounds like Emilio has requested a few significant changes. Also, I'm curious how the custom property handling works? Is there just a single property in an nsCSSPropertyIDSet that represents all custom properties? ::: dom/animation/EffectCompositor.cpp:176 (Diff revision 6) > - EffectCompositor::MaybeUpdateCascadeResults(pseudoElement->mElement, > + StyleBackendType backend = > + aFrame->StyleContext()->StyleSource().IsServoComputedValues() > + ? StyleBackendType::Servo > + : StyleBackendType::Gecko; (Is this the best way to test the backend type? Maybe it is?) ::: dom/animation/EffectCompositor.cpp:768 (Diff revision 6) > -/* static */ void > -EffectCompositor::GetOverriddenProperties(nsStyleContext* aStyleContext, > +/* static */ nsCSSPropertyIDSet > +EffectCompositor::GetOverriddenProperties(StyleBackendType aBackendType, > EffectSet& aEffectSet, > - nsCSSPropertyIDSet& > - aPropertiesOverridden) > + Element* aElement, > + CSSPseudoElementType aPseudoType, > + nsStyleContext* aStyleContext) > { > + MOZ_ASSERT(aBackendType != StyleBackendType::Servo || aElement, > + "Should have an element to get style data from if we are using" > + " the Servo backend"); > + Should we put the part where we look up the style context here at the start? That seems like it would be easier to read since you could easily identify the early returns. i.e. nsCSSPropertyIDSet result; // If no style context is supplied for Gecko backend, look up the style // context on the primary frame of the element. if (aBackendType == StyleBackendType::Gecko && !aStyleContext) { Element* elementToRestyle = GetElementToRestyle(aElement, aPseudoType); if (!elementToRestyle) { return result; } nsIFrame* frame = elementToRestyle->GetPrimaryFrame(); if (!frame) { return result; } aStyleContext = frame->StyleContext(); if (!aStyleContext) { return result; } } Or, more simply: if (aBackendType == StyleBackendType::Gecko && !aStyleContext) { Element* elementToRestyle = GetElementToRestyle(aElement, aPseudoType); if (elementToRestyle) { nsIFrame* frame = elementToRestyle->GetPrimaryFrame(); if (frame) { aStyleContext = frame->StyleContext(); } } if (!aStyleContext) { return result; } } ::: dom/animation/EffectCompositor.cpp:805 (Diff revision 6) > + Element* elementToRestyle = GetElementToRestyle(aElement, aPseudoType); > + switch (aBackendType) { > + case StyleBackendType::Servo: > + MOZ_ASSERT(elementToRestyle); Based on my suggestion above, this would need to go into the case branch. But why is it ok to assert that elementToRestyle is non-null? Everywhere else we call that method we null-check the result. ::: servo/components/style/properties/properties.mako.rs:929 (Diff revision 6) > % endfor > _ => Err(()) > } > } > > - /// Returns a property id from Gecko's nsCSSPropertyID. > + /// Returns a nsCSSPropertyID. Very very very minor nit: 'an nsCSSPropertyID' ::: servo/components/style/rule_tree/mod.rs:1096 (Diff revision 6) > .take_while(|node| node.cascade_level() >= CascadeLevel::SMILOverride) > .any(|node| node.cascade_level().is_animation()) > } > + > + /// Get a set of properties whose CascadeLevel are higher than Animations but not equal to > + /// Transitions. If there is any custom property, we set the boolean value of the returned Nit: s/is any custom property/are any custom properties/ ::: servo/components/style/rule_tree/mod.rs:1102 (Diff revision 6) > + // Walk up until the first less-or-equally specific rule. > + // Note: Transitions are the only non-!important level overriding animations in the cascade > + // ordering. They also don't actually override animations, since transitions are > + // suppressed when both are present. We should skip it explicitly. I know this comment is based on nsRuleNode::ComputePropertiesOverridingAnimation, but it doesn't really fit here. I think we want to say something like: // We want to iterate over cascade levels that override the animations level, i.e. // !important levels and the transitions level. However, we actually want to skip the // transitions level because although it is higher in the cascade than animations, when // both transitions and animations are present for a given element and property, transitions // are suppressed so that they don't actually override animations. ::: servo/components/style/rule_tree/mod.rs:1115 (Diff revision 6) > + for node in iter { > + let style = node.style_source().unwrap(); > + for &(ref decl, important) in style.read(node.cascade_level().guard(guards)) > + .declarations() > + .iter() { > + if important.important() { We should add a comment above this saying why we need to check this. Something like: "Although we are only iterating over cascade levels that override animations, in a given property declaration block we can have a mixture of !important and non-!important declarations but only the !important declarations actually override animations."
Attachment #8839103 - Flags: review?(bbirtles)
Comment on attachment 8839102 [details] Bug 1334036 - Part 7: Merge two similiar MaybeUpdateCascadeResults functions. https://reviewboard.mozilla.org/r/113642/#review142836
Attachment #8839102 - Flags: review?(bbirtles) → review+
Comment on attachment 8839106 [details] Bug 1334036 - Part 8: Add AddLayerChangesForAnimation in ServoRestyleManager. https://reviewboard.mozilla.org/r/113694/#review142840 r=me with some further explanation about why that is the appropriate place to call this method. ::: layout/base/RestyleManager.cpp:1782 (Diff revision 6) > > +/* static */ void > +RestyleManager::AddLayerChangesForAnimation(nsIFrame* aFrame, > + nsIContent* aContent, > + nsStyleChangeList& > + aChangeListToProcess) Nit, can we either indent 'aChangeListToProcess' by two more spaces, or bring the whole list of parameters back to column 2. At the moment it looks like 'nsStyleChangeList&' and 'aChangeListToProcess' are two separate parameters. ::: layout/base/ServoRestyleManager.cpp:328 (Diff revision 6) > + // We should call this after resolving the current context and updating > + // style of owned anonymous boxes. Why is that?
Attachment #8839106 - Flags: review?(bbirtles) → review+
Comment on attachment 8867631 [details] Bug 1334036 - Part 3: Add a flag to represent we are in pre-traversal. https://reviewboard.mozilla.org/r/139196/#review142816 Yes. I will use mozilla::AutoRestore and drop the class I added. Thanks for the suggestion.
Comment on attachment 8867632 [details] Bug 1334036 - Part 6: Trigger restyle if important rules are changed. https://reviewboard.mozilla.org/r/139198/#review142886 In the commit message (which MozReview doesn't let you comment on anymore?) "only if this elements needs to be restyled" I don't quite follow this sentence: "If we have animations on this element, we update its effect properties, and also send a task to update cascade results." What is the relevance of updating effect properties here? I don't understand this paragraph either: "However, only OMTA needs this flag, so for optimization, we don't need to check the important rules (i.e. call get_properties_overriding_animations()) for elements without animations. Further, we can just store this set into TNode, so we only need to find the *new* properties overriding animations." (Also, we should refer to "animations that run on the compositor" or something other than OMTA. Many people reading this code won't know what OMTA is.) r=me with the comment changes but I'd also be curious to know if we can skip calling RequestRestyle in Gecko_UpdateAnimations when we're sure we have no compositor animations. I've only skimmed the Servo parts since it looks like Emilio had a good look at that. ::: layout/style/ServoBindings.cpp:544 (Diff revision 1) > + // We send a task of CascadeResults if any important rule is changed, so > + // we request a restyle, which updates the cascade result in the 2nd > + // pre-traversal. "This task will be scheduled if we detected any changes to !important rules. We post a restyle here so that we can update the cascade results in the pre-traversal of the next restyle." ::: layout/style/ServoBindings.cpp:547 (Diff revision 1) > + presContext->EffectCompositor() > + ->RequestRestyle(const_cast<dom::Element*>(aElement), > + pseudoType, > + EffectCompositor::RestyleType::Standard, > + EffectCompositor::CascadeLevel::Animations); I wonder if it makes sense to try to detect if we might have animations for the compositor, and, if we don't skip requesting this restyle?
Attachment #8867632 - Flags: review?(bbirtles) → review+
Comment on attachment 8867632 [details] Bug 1334036 - Part 6: Trigger restyle if important rules are changed. https://reviewboard.mozilla.org/r/139198/#review142890 One thing I am concerned about this patch is that it seems to me that important_rules_are_different() checks all properties, not only for compositor running properties (i.e. opacity and transform). Am I missing something? Actually I haven't looked other patches closely. Or have we decided to check important rules changes for all properties? If we decided it we have a change to fix bug 1300983 and bug 1300982. Also it will fix bug 1336772 incidentally on stylo.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #153) > Comment on attachment 8867632 [details] > Bug 1334036 - Part 5: Trigger restyle if important rules are changed. > > https://reviewboard.mozilla.org/r/139198/#review142890 > > One thing I am concerned about this patch is that it seems to me that > important_rules_are_different() checks all properties, not only for > compositor running properties (i.e. opacity and transform). Am I missing > something? Actually I haven't looked other patches closely. > > Or have we decided to check important rules changes for all properties? If > we decided it we have a change to fix bug 1300983 and bug 1300982. Also it have a *chance*
Comment on attachment 8839103 [details] Bug 1334036 - Part 5: Implement FFI for finding properties overriding animations. https://reviewboard.mozilla.org/r/113644/#review142550 > I think you can just use `const nsTArray<nsCSSPropertyID>*`. The `uintptr_t` thing was a workaround for a bindgen bug related to typedefs that is fixed in libclang 4.0, but this should not be affected I believe. I see. I will define RawGeckoCSSPropertyIDList as nsTArray<nsCSSPropertyID> and use it as the argument (i.e. RawGeckoCSSPropertyIDListBorrowed).
Comment on attachment 8839103 [details] Bug 1334036 - Part 5: Implement FFI for finding properties overriding animations. https://reviewboard.mozilla.org/r/113644/#review142824 > Also, I'm curious how the custom property handling works? Is there just a single property in an nsCSSPropertyIDSet that represents all custom properties? Yes, I think we only use eCSSPropertyExtra_variable as the custom property id now. We could fix it in a different bug. > (Is this the best way to test the backend type? Maybe it is?) We have many different ways to know the backend, and I'm not sure which one is better. Maybe we can also use ```presContext->RestyleManager()``` to know which RestyleManager is used and it would be better? For now, using nsStyleContext is also fine because it seems to me that we usually use it to know the backend. > Should we put the part where we look up the style context here at the start? That seems like it would be easier to read since you could easily identify the early returns. > > i.e. > > nsCSSPropertyIDSet result; > > // If no style context is supplied for Gecko backend, look up the style > // context on the primary frame of the element. > > if (aBackendType == StyleBackendType::Gecko && !aStyleContext) { > Element* elementToRestyle = GetElementToRestyle(aElement, aPseudoType); > if (!elementToRestyle) { > return result; > } > nsIFrame* frame = elementToRestyle->GetPrimaryFrame(); > if (!frame) { > return result; > } > aStyleContext = frame->StyleContext(); > if (!aStyleContext) { > return result; > } > } > > Or, more simply: > > if (aBackendType == StyleBackendType::Gecko && !aStyleContext) { > Element* elementToRestyle = GetElementToRestyle(aElement, aPseudoType); > if (elementToRestyle) { > nsIFrame* frame = elementToRestyle->GetPrimaryFrame(); > if (frame) { > aStyleContext = frame->StyleContext(); > } > } > > if (!aStyleContext) { > return result; > } > } I see. I will move the handle of nsStyleContext to the start of this function. > Based on my suggestion above, this would need to go into the case branch. > > But why is it ok to assert that elementToRestyle is non-null? Everywhere else we call that method we null-check the result. I thought |```elementToRestyle```| should be a non-null value in this function because we should already generated this element while calling this function (on servo backend only, by Comment 117). However, we can reuse the null-check and early return (based on the suggestion above).
Comment on attachment 8867632 [details] Bug 1334036 - Part 6: Trigger restyle if important rules are changed. https://reviewboard.mozilla.org/r/139198/#review142552 > I think the return value of this function is not really accurate. This only returns the second boolean if there are animations. This should be at least documented. > > Also, perhaps it's worth changing the return value to something else that is not a tuple of booleans. > > For `replace_rules` I suggest bitflags below, here it may be a bit more difficult to find a type that suits... Perhaps a struct with two booleans (`rule_node_changed`, `important_rules_overriding_animation_changed`) is good enough? That would make it clearer I think, what do you think? I agree. I will add a struct for this. > Maybe: > > ``` > let important_rules_changed = self.has_animations() && data.has_styles() && data.important_rules_are_different(..); > ``` > > (Though I wonder how can we have animations without having styles? Via the DOM API maybe?) This is called in normal restyle. If we have animation, we should put the animation rule into the rule tree already, I think, because animation-only restyle is done before normal restyle. Therefore, it seems to me that it is impossble to have animations without having styles here. Therefore, looks like we don't need to check ```data.has_styles()``` here. > Perhaps converting this two booleans here, is cleaner, either in a `RulesChanged` struct or some enum? like `{ NoChanges, ImportantRulesChanged, NormalRulesChanged, NormalAndImportantRulesChanged }`? > > You can implement convenience methods like: > > ``` > impl RulesChanged { > fn rule_changed(&mut self, level: CascadeLevel) { > *self = match (*self, level.is_important()) { > (RulesChanged::NoChanges, true) => RulesChanged::ImportantRulesChanged, > (RulesChanged::NormalRulesChanged, true) | > (RulesChanged::ImportantRulesChanged, false) => RulesChanged::NormalAndImportantRulesChanged, > (RulesChanged::NoChanges, false) => RulesChanged::NormalRulesChanged, > _ => *self, > }; > } > > fn important_rules_changed(&self) -> bool { > matches!(*self, ImportantRulesChanged | NormalAndImportantRulesChanged) > } > } > ``` > > Or even a `bitflags!`? That should be faster, and you can implement similar methods. > > Whatever you find cleaner (perhaps I like bitflags the most, if it matters). > > Then, to know if it changed you'd test `!is_empty()`, and to know if important rules changed you'd check `intersects(IMPORTANT_RULES)`, or something like that. Thanks. I also prefer ```bitflags```.
Comment on attachment 8867632 [details] Bug 1334036 - Part 6: Trigger restyle if important rules are changed. https://reviewboard.mozilla.org/r/139198/#review142886 > "only if this elements needs to be restyled" > I don't quite follow this sentence: That means only elements in mElementsToRestyle. I should update it. > "If we have animations on this element, we update its effect properties, and also send a task to update cascade results." > What is the relevance of updating effect properties here? I should mention the task in process_animation(). > I don't understand this paragraph either: > "However, only OMTA needs this flag, so for optimization, ... " Sorry, I will rewrite this paragraph. > I wonder if it makes sense to try to detect if we might have animations for the compositor, and, if we don't skip requesting this restyle? Yes, it makes sense. We should avoid requesting restyle for that case. Maybe we can use the flag in KeyframeEffectReadOnly. I think I can do something like this: ``` bool hasAnimationRunningOnCompositor = false; EffectSet* effsetSet = EffectSet::GetEffectSet(element, pseudoType); MOZ_ASSERT(effectSet); for (KeyframeEffectReadOnly* effect: effectSet) { if (effect->IsRunningOnCompositor()) { hasAnimationRunningOnCompositor = true; break; } } if (hasAnimationRunningOnCompositor) { RequestRestyle(...); } ```
Comment on attachment 8867632 [details] Bug 1334036 - Part 6: Trigger restyle if important rules are changed. https://reviewboard.mozilla.org/r/139198/#review142890 > important_rules_are_different() checks all properties, not only for compositor running properties Yes. We checks all properties in servo side because I don't maintain a list for compositor running properties. We can do that actually, but it seems to me that we only need a roughly check from Servo side. Therefore, there might be a false alarm, and we rely on UpdateCascadeResults() to check only the compositor running properties.
(In reply to Boris Chiou [:boris] from comment #159) > Comment on attachment 8867632 [details] > Bug 1334036 - Part 5: Trigger restyle if important rules are changed. > > https://reviewboard.mozilla.org/r/139198/#review142890 > > > important_rules_are_different() checks all properties, not only for compositor running properties > > Yes. We checks all properties in servo side because I don't maintain a list > for compositor running properties. We can do that actually, but it seems to > me that we only need a roughly check from Servo side. Therefore, there might > be a false alarm, and we rely on UpdateCascadeResults() to check only the > compositor running properties. It's rough but higher cost, no? Also you mean we walk up rule trees twice? The one is in servo side, the other is in gecko side?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #160) > (In reply to Boris Chiou [:boris] from comment #159) > > Comment on attachment 8867632 [details] > > Bug 1334036 - Part 5: Trigger restyle if important rules are changed. > > > > https://reviewboard.mozilla.org/r/139198/#review142890 > > > > > important_rules_are_different() checks all properties, not only for compositor running properties > > > > Yes. We checks all properties in servo side because I don't maintain a list > > for compositor running properties. We can do that actually, but it seems to > > me that we only need a roughly check from Servo side. Therefore, there might > > be a false alarm, and we rely on UpdateCascadeResults() to check only the > > compositor running properties. > > It's rough but higher cost, no? Also you mean we walk up rule trees twice? > The one is in servo side, the other is in gecko side? In replace_rules(), it is very cheap because we just replace the rules. However, in match_primary(), it is expensive because we need to walk up the rule tree, and I have no idea to avoid it now. Yes, we walk up rule tree twice. One in servo (to check it), and one in gecko (if we really need to update it). I can add a list in servo side, so we only check opacity and transform to reduce the false alarm.
Comment on attachment 8867632 [details] Bug 1334036 - Part 6: Trigger restyle if important rules are changed. https://reviewboard.mozilla.org/r/139198/#review142886 > Yes, it makes sense. We should avoid requesting restyle for that case. Maybe we can use the flag in KeyframeEffectReadOnly. I think I can do something like this: > ``` > bool hasAnimationRunningOnCompositor = false; > EffectSet* effsetSet = EffectSet::GetEffectSet(element, pseudoType); > MOZ_ASSERT(effectSet); > for (KeyframeEffectReadOnly* effect: effectSet) { > if (effect->IsRunningOnCompositor()) { > hasAnimationRunningOnCompositor = true; > break; > } > } > > if (hasAnimationRunningOnCompositor) { > RequestRestyle(...); > } > ``` Sorry. This code is not correct. Just notice that I got null effectSet, and IsRunningOnCompositor() is the flag for animation running on compositor, not may have animations for the compositor.
Comment on attachment 8867632 [details] Bug 1334036 - Part 6: Trigger restyle if important rules are changed. https://reviewboard.mozilla.org/r/139198/#review142886 > if we can skip calling RequestRestyle in Gecko_UpdateAnimations when we're sure we have no compositor animations. Actually, I'm not sure how to know we *might* have animation running on compositor easily here. It seems to me that we need do something like FindAnimationsForCompositor() (maybe I'm wrong) on each property of the keyframe effects of this element/pseudo element.
(In reply to Boris Chiou [:boris] from comment #163) > Comment on attachment 8867632 [details] > Bug 1334036 - Part 5: Trigger restyle if important rules are changed. > > https://reviewboard.mozilla.org/r/139198/#review142886 > > > if we can skip calling RequestRestyle in Gecko_UpdateAnimations when we're sure we have no compositor animations. > > Actually, I'm not sure how to know we *might* have animation running on > compositor easily here. It seems to me that we need do something like > FindAnimationsForCompositor() (maybe I'm wrong) on each property of the > keyframe effects of this element/pseudo element. mIsRunningOnCompositor?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #164) > (In reply to Boris Chiou [:boris] from comment #163) > > Comment on attachment 8867632 [details] > > Bug 1334036 - Part 5: Trigger restyle if important rules are changed. > > > > https://reviewboard.mozilla.org/r/139198/#review142886 > > > > > if we can skip calling RequestRestyle in Gecko_UpdateAnimations when we're sure we have no compositor animations. > > > > Actually, I'm not sure how to know we *might* have animation running on > > compositor easily here. It seems to me that we need do something like > > FindAnimationsForCompositor() (maybe I'm wrong) on each property of the > > keyframe effects of this element/pseudo element. > > mIsRunningOnCompositor? I tried, but this flag causes some test failures in test_running_on_compositor.html while we are trying to clear an important rule. I guess the reason is: when we have an important rule, we don't run the animation on compositor, so the flag is false. And then we remove the important rule, the flag is still false in the normal restyle, so Gecko_UpdateAnimations() skip RequestRestyle().
Comment on attachment 8839101 [details] Bug 1334036 - Part 1: Avoid mutating mElementsToRestyle during pre-traversal. https://reviewboard.mozilla.org/r/113640/#review142802 > a) When there are only animation restyles > b) When there are non-animation restyles OR when we handle an event with coordinates (PresShell::HandleEvent). > I think with this patch series we still don't implement (b)? No, I didn't implement (b). > In PreTraverse, perhaps should we check if the regular dirty-bit is set on the root, and, if it is, do (b)? > We'll still need to implement FlushThrottledStyles in PresShell.cpp, however. I see. I will check FlushThrottledStyles, for (b) > * Why is it ok, at the end of the function, to NOT remove elements from mElementsToRestyle that we called > PostRestyleEventForAnimations on? I forgot to remove the elements. :( > * Why do we need to call PostRestyleEventForAnimations twice? > * Why do we need to call WillComposeStyle twice? > * Can we avoid calling nsContentUtils::ContentIsFlattenedTreeDescendantOf twice? The second time to call PostRestyleEventForAnimations and WillComposeStyle is that I want to handle the newly added restyle in the same PreTraverse(). Anyway, I will try your suggestion, which can reduce the duplicated code above, and try to use FlushThrottledStyles() for servo backend.
(In reply to Boris Chiou [:boris] from comment #165) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #164) > > (In reply to Boris Chiou [:boris] from comment #163) > > > Comment on attachment 8867632 [details] > > > Bug 1334036 - Part 5: Trigger restyle if important rules are changed. > > > > > > https://reviewboard.mozilla.org/r/139198/#review142886 > > > > > > > if we can skip calling RequestRestyle in Gecko_UpdateAnimations when we're sure we have no compositor animations. > > > > > > Actually, I'm not sure how to know we *might* have animation running on > > > compositor easily here. It seems to me that we need do something like > > > FindAnimationsForCompositor() (maybe I'm wrong) on each property of the > > > keyframe effects of this element/pseudo element. > > > > mIsRunningOnCompositor? > > I tried, but this flag causes some test failures in > test_running_on_compositor.html while we are trying to clear an important > rule. > > I guess the reason is: when we have an important rule, we don't run the > animation on compositor, so the flag is false. And then we remove the > important rule, the flag is still false in the normal restyle, so > Gecko_UpdateAnimations() skip RequestRestyle(). Oh, sorry. I didn't quite follow what you meant. What does the "we *might* have animation running on compositor" mean? You just want the property can be run on the compositor?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #167) > Oh, sorry. I didn't quite follow what you meant. What does the "we *might* > have animation running on compositor" mean? You just want the property can > be run on the compositor? Actually I'm not sure which thing we could check. Maybe checking the properties of the keyframes could be a possible way.
Comment on attachment 8839101 [details] Bug 1334036 - Part 1: Avoid mutating mElementsToRestyle during pre-traversal. https://reviewboard.mozilla.org/r/113640/#review142802 > b) When there are non-animation restyles OR when we handle an event with coordinates (PresShell::HandleEvent). > We'll still need to implement FlushThrottledStyles in PresShell.cpp, however. Hi, Brian, Cameron, I am trying to trigger animation-only restyle for this case (i.e. HandleEvent for FlushThrottledStyles)[1]. According to current implementation of restyle (i.e. Servo_TraverseSubtree()), we might also update non-animation styles if there is any because we handle animation-only and non-animation restyle together in Servo_TraverseSubtree(). Therefore, I wanna know, should we avoid updating non-animation styles while handing events? Or it's ok to trigger them together becase there must be only animation-only dirty bits while handling events? [1] http://searchfox.org/mozilla-central/rev/9a7fbdee1d54f99cd548af95b81231d80e5f9ad1/layout/base/PresShell.cpp#7374
Flags: needinfo?(cam)
Flags: needinfo?(bbirtles)
(In reply to Boris Chiou [:boris] from comment #169) > I am trying to trigger animation-only restyle for this case (i.e. > HandleEvent for FlushThrottledStyles)[1]. According to current > implementation of restyle (i.e. Servo_TraverseSubtree()), we might also > update non-animation styles if there is any because we handle animation-only > and non-animation restyle together in Servo_TraverseSubtree(). Therefore, I > wanna know, should we avoid updating non-animation styles while handing > events? Or it's ok to trigger them together becase there must be only > animation-only dirty bits while handling events? I think there can be non-animation restyles pending, so we should avoid flushing them. (Otherwise we wouldn't need FlushThrottledStyles and we could just do a normal flush.) You're right that Servo_TraverseSubtree will do the wrong thing here, because it will also flush non-animation restyles. Maybe we can add a new value to TraversalRestyleBehavior, which means process animation restyles only?
Flags: needinfo?(cam)
(In reply to Cameron McCormack (:heycam) from comment #170) > (In reply to Boris Chiou [:boris] from comment #169) > > I am trying to trigger animation-only restyle for this case (i.e. > > HandleEvent for FlushThrottledStyles)[1]. According to current > > implementation of restyle (i.e. Servo_TraverseSubtree()), we might also > > update non-animation styles if there is any because we handle animation-only > > and non-animation restyle together in Servo_TraverseSubtree(). Therefore, I > > wanna know, should we avoid updating non-animation styles while handing > > events? Or it's ok to trigger them together becase there must be only > > animation-only dirty bits while handling events? > > I think there can be non-animation restyles pending, so we should avoid > flushing them. (Otherwise we wouldn't need FlushThrottledStyles and we > could just do a normal flush.) > > You're right that Servo_TraverseSubtree will do the wrong thing here, > because it will also flush non-animation restyles. Maybe we can add a new > value to TraversalRestyleBehavior, which means process animation restyles > only? Thanks for the suggestion, Cameron. I will add a flag to TraversalRestyleBehavior so that we only need process animation restyle.
Flags: needinfo?(bbirtles)
Comment on attachment 8867632 [details] Bug 1334036 - Part 6: Trigger restyle if important rules are changed. https://reviewboard.mozilla.org/r/139198/#review142552 > This is called in normal restyle. If we have animation, we should put the animation rule into the rule tree already, I think, because animation-only restyle is done before normal restyle. Therefore, it seems to me that it is impossble to have animations without having styles here. Therefore, looks like we don't need to check ```data.has_styles()``` here. Oops, it is possible. I still need to add this check (i.e. data.has_styles()) for this test case: ```layout/reftests/web-animations/stacking-context-transform-none-animation-before-appending-element.html```
Comment on attachment 8839106 [details] Bug 1334036 - Part 8: Add AddLayerChangesForAnimation in ServoRestyleManager. https://reviewboard.mozilla.org/r/113694/#review142840 > Why is that? The comment is not correct. Sorry. AddLayerChangesForAnimation() checks if aFrame has a transform or not, so we needs the up-to-date |aFrame->StyleDisplay()| to ensure the animated transform is removed first; otherwise, we got an assertion in |ApplyRenderingChangeToTree()| if the transform is removed in the new context but still there in the old context. Therefore, the correct place is after: ``` for (nsIFrame* f = styleFrame; f; f = GetNextContinuationWithSameStyle(f, oldStyleContext)) { f->SetStyleContext(newContext); } ``` In other words, we should make sure that we set |nsStyleContext| to the frame before AddLayerChangesForAnimation(), so |aFrame->StyleDisplay()| can get the up-to-date result. In conclusion, the current place it fine. I will update the comment. Thanks.
Comment on attachment 8867632 [details] Bug 1334036 - Part 6: Trigger restyle if important rules are changed. https://reviewboard.mozilla.org/r/139198/#review143462 r=me with those nits, thanks! ::: servo/components/style/matching.rs:520 (Diff revision 2) > /// setting them on the ElementData. > fn cascade_primary_or_pseudo(&self, > context: &mut StyleContext<Self>, > data: &mut ElementData, > - pseudo: Option<&PseudoElement>) { > + pseudo: Option<&PseudoElement>, > + important_rules_changed: bool) { Let's make the flags arrive here, and document that they're guaranteed to be empty if there's a pseudo-element, given we don't compute it for both. Also, let's pass the flags direclty to process_animations too. ::: servo/components/style/matching.rs:663 (Diff revision 2) > fn process_animations(&self, > context: &mut StyleContext<Self>, > old_values: &mut Option<Arc<ComputedValues>>, > new_values: &mut Arc<ComputedValues>, > - primary_style: &ComputedStyle) { > - use context::{CSS_ANIMATIONS, CSS_TRANSITIONS, EFFECT_PROPERTIES}; > + primary_style: &ComputedStyle, > + important_rules_changed: bool) { ::: servo/components/style/matching.rs:1235 (Diff revision 2) > use properties::PropertyDeclarationBlock; > use shared_lock::Locked; > > let element_styles = &mut data.styles_mut(); > let primary_rules = &mut element_styles.primary.rules; > - let mut rule_node_changed = false; > + let mut result: RulesChangedFlags = RulesChangedFlags::empty(); nit: no need for the type annotation, just: ``` let mut result = RulesChangedFlags::empty(); ``` Also, how do you feel about renaming `RulesChangedFlags` to just `RulesChanged`? (not a big deal). ::: servo/components/style/matching.rs:1481 (Diff revision 2) > > /// Performs the cascade for the element's primary style. > fn cascade_primary(&self, > context: &mut StyleContext<Self>, > - mut data: &mut ElementData) > + mut data: &mut ElementData, > + important_rules_changed: bool)
Attachment #8867632 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8867632 [details] Bug 1334036 - Part 6: Trigger restyle if important rules are changed. https://reviewboard.mozilla.org/r/139198/#review143462 > nit: no need for the type annotation, just: > > ``` > let mut result = RulesChangedFlags::empty(); > ``` > > Also, how do you feel about renaming `RulesChangedFlags` to just `RulesChanged`? (not a big deal). Sure. I just wonder could we make it simpler. I will update its name. Thanks.
Comment on attachment 8867632 [details] Bug 1334036 - Part 6: Trigger restyle if important rules are changed. https://reviewboard.mozilla.org/r/139198/#review143482 ::: layout/style/ServoBindings.cpp:547 (Diff revision 2) > + presContext->EffectCompositor() > + ->RequestRestyle(const_cast<Element*>(aElement), > + pseudoType, > + EffectCompositor::RestyleType::Standard, > + EffectCompositor::CascadeLevel::Animations); > + } Hiro, I think we can add the check, just as you mentioned yesterday. ``` EffectSet* effectSet = EffectSet::GetEffectSet(element, pseudoType); if (!effectSet) { return; } bool hasPropertiesRunningOnCompositor = false; for (const KeyframeEffectReadOnly* effect: *effectSet) for (size_t i = 0; i < LayerAnimationInfo::kRecords; i++) { if (effect->HasAnimationOfProperty(LayerAnimationInfo::sRecords[i].mProperty)) { hasPropertiesRunningOnCompositor = true; break; } } } if (hasPropertiesRunningOnCompositior) { RequestRestyle(...); } ``` This needs traverse all properties per effect. If it is worth to do, I can add this into this patch. Thanks.
(In reply to Boris Chiou [:boris] from comment #189) > Comment on attachment 8867632 [details] > Bug 1334036 - Part 6: Trigger restyle if important rules are changed. > > https://reviewboard.mozilla.org/r/139198/#review143482 > > ::: layout/style/ServoBindings.cpp:547 > (Diff revision 2) > > + presContext->EffectCompositor() > > + ->RequestRestyle(const_cast<Element*>(aElement), > > + pseudoType, > > + EffectCompositor::RestyleType::Standard, > > + EffectCompositor::CascadeLevel::Animations); > > + } > > Hiro, I think we can add the check, just as you mentioned yesterday. > ``` > EffectSet* effectSet = EffectSet::GetEffectSet(element, pseudoType); > if (!effectSet) { > return; > } > bool hasPropertiesRunningOnCompositor = false; > for (const KeyframeEffectReadOnly* effect: *effectSet) > for (size_t i = 0; i < LayerAnimationInfo::kRecords; i++) { > if > (effect->HasAnimationOfProperty(LayerAnimationInfo::sRecords[i].mProperty)) { > hasPropertiesRunningOnCompositor = true; > break; > } > } > } > if (hasPropertiesRunningOnCompositior) { > RequestRestyle(...); > } > ``` > This needs traverse all properties per effect. If it is worth to do, I can > add this into this patch. Thanks. What I was thinking is that we filtered out non-compositor running properties before creating a CascadeResults task. Anyway, I don't think it's a good idea that we check whether important rule is change or not for *all* properties in this bug. Is there no performance/memory impact? It should be done in another independent bug.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #190) > What I was thinking is that we filtered out non-compositor running > properties before creating a CascadeResults task. > Anyway, I don't think it's a good idea that we check whether important rule > is change or not for *all* properties in this bug. Is there no > performance/memory impact? It should be done in another independent bug. OK. Let's do it in another bug. Thanks.
Comment on attachment 8867632 [details] Bug 1334036 - Part 6: Trigger restyle if important rules are changed. https://reviewboard.mozilla.org/r/139198/#review143462 > Let's make the flags arrive here, and document that they're guaranteed to be empty if there's a pseudo-element, given we don't compute it for both. > > Also, let's pass the flags direclty to process_animations too. Thanks. And I will also implement a From trait: ```impl From<RulesMatchedResult> for RulesChanged```.
Comment on attachment 8839101 [details] Bug 1334036 - Part 1: Avoid mutating mElementsToRestyle during pre-traversal. https://reviewboard.mozilla.org/r/113640/#review143764 This looks a lot better. r=me with the simplification suggested below. ::: dom/animation/EffectCompositor.cpp:970 (Diff revision 7) > // Ignore throttled restyle. > - if (!postedRestyle) { > + if (!iter.Data()) { > + continue; > + } > + > + const NonOwningAnimationTarget& target = iter.Key(); > + > + // Ignore restyles that aren't in the flattened tree subtree rooted at > + // aRoot. > + if (aRoot && > + !nsContentUtils::ContentIsFlattenedTreeDescendantOf(target.mElement, > + aRoot)) { > + continue; > + } These lines are identical in both loops. Can you factor our a lambda function for this part? I suppose it would take an iterator as a parameter (unfortunately we're not allowed to use C++14 generic lambdas in Mozilla code yet so you'll have to type out the full iterator type) and return a const NonOwningAnimationTarget* I guess? If the result is null we should continue. aRoot we can capture. Factoring it out like this will help when we go to make this method something flush all animations. (Which I guess is later in this patch series?) ::: dom/animation/EffectCompositor.cpp:1032 (Diff revision 7) > EffectSet* effects = > EffectSet::GetEffectSet(target.mElement, target.mPseudoType); > if (!effects) { > // Drop EffectSets that have been destroyed. > iter.Remove(); > continue; > } Does it make sense to do this step in the first loop? (And then just assert here that effects is non-null if we assume that MaybeUpdateCascadeResults won't affect the result of GetEffectSet.) It's up to you which you prefer.
Attachment #8839101 - Flags: review?(bbirtles) → review+
Comment on attachment 8868509 [details] Bug 1334036 - Part 2: Restyle all elements with animations if there are non-animation restyles. https://reviewboard.mozilla.org/r/140110/#review143776 ::: dom/animation/EffectCompositor.cpp:973 (Diff revision 1) > auto& elementSet = mElementsToRestyle[cascadeLevel]; > for (auto iter = elementSet.Iter(); !iter.Done(); iter.Next()) { > + const NonOwningAnimationTarget& target = iter.Key(); > + > // Ignore throttled restyle. > - if (!iter.Data()) { > + if (!iter.Data() && !target.mElement->HasDirtyDescendantsForServo()) { This patch causes crashes in some crashtests and reftests (still don't know why, and I'm looking at them now.)
Comment on attachment 8868509 [details] Bug 1334036 - Part 2: Restyle all elements with animations if there are non-animation restyles. https://reviewboard.mozilla.org/r/140110/#review143766 I this right? Don't we want to check if |aRoot| has dirty descendants? Isn't it possible that target.mElement->HasDirtyDescendantsForServo() returns false but an ancestor returns true? And if the ancestor returns true we want to update throttled animations, right? (Also, in any case, we'll need to update the "Ignore throttled restyle" comments here.)
Comment on attachment 8867631 [details] Bug 1334036 - Part 3: Add a flag to represent we are in pre-traversal. https://reviewboard.mozilla.org/r/139196/#review143784 ::: dom/animation/EffectCompositor.h:296 (Diff revision 2) > // posting a restyle to update it. > EnumeratedArray<CascadeLevel, CascadeLevel(kCascadeLevelCount), > nsDataHashtable<PseudoElementHashEntry, bool>> > mElementsToRestyle; > > + // A flag represents we are in a pre-traversal. Maybe we don't need this comment?
Attachment #8867631 - Flags: review?(bbirtles) → review+
Comment on attachment 8868509 [details] Bug 1334036 - Part 2: Restyle all elements with animations if there are non-animation restyles. https://reviewboard.mozilla.org/r/140110/#review143766 > Don't we want to check if |aRoot| has dirty descendants? OK. Looks like we only need to check aRoot. e.g. ```bool needsForceFlush = aRoot && aRoot->HasDirtyDescendantsForServo()```. I will update this. Besides, do we need to restyle all thorttled animations for this function: ```EffectCompositor::PreTraverse(dom::Element* aElement, nsIAtom* aPseudoTagOrNull)```? In that function, we only check one target.
Comment on attachment 8839103 [details] Bug 1334036 - Part 5: Implement FFI for finding properties overriding animations. https://reviewboard.mozilla.org/r/113644/#review143774 ::: dom/animation/EffectCompositor.cpp:785 (Diff revision 7) > + if (nsIFrame* frame = elementToRestyle->GetPrimaryFrame()) { > + aStyleContext = frame->StyleContext(); > + } Nit: I think it's easier to read if you don't put the initialization in the if condition unless you need to. At least in this case, I think the following is easier to read: if (elementToRestyle) { nsIFrame* frame = elementToRestyle->GetPrimaryFrame(); if (frame) { ... } }
Attachment #8839103 - Flags: review?(bbirtles) → review+
Comment on attachment 8868509 [details] Bug 1334036 - Part 2: Restyle all elements with animations if there are non-animation restyles. https://reviewboard.mozilla.org/r/140110/#review143790 ::: dom/animation/EffectCompositor.cpp:1076 (Diff revision 1) > > for (size_t i = 0; i < kCascadeLevelCount; ++i) { > CascadeLevel cascadeLevel = CascadeLevel(i); > auto& elementSet = mElementsToRestyle[cascadeLevel]; > > - if (!elementSet.Get(key)) { > + if (!elementSet.Get(key) && !needsForceFlush) { This is not correct. ```!elementSet.Get(key)``` has two meanings, one for throttle, one for no this target. I should fix this by: ``` Element* element = GetElementToRestyle(aElement, pseudoType); bool needsForceFlush = element && element->HasDirtyDescendantsForServo(); ... if (!elementSet.Contain(Key)) { // no restyle request. continue; } if (!elementSet.Get(Key) && !needsForceFlush) { // ignore thorttled restyle if |needsForceFlush| is false continue; } ``` Otherwise, this causes some crashes.
Attachment #8868509 - Flags: review?(bbirtles)
Comment on attachment 8839101 [details] Bug 1334036 - Part 1: Avoid mutating mElementsToRestyle during pre-traversal. https://reviewboard.mozilla.org/r/113640/#review143764 > These lines are identical in both loops. Can you factor our a lambda function for this part? I suppose it would take an iterator as a parameter (unfortunately we're not allowed to use C++14 generic lambdas in Mozilla code yet so you'll have to type out the full iterator type) and return a const NonOwningAnimationTarget* I guess? If the result is null we should continue. aRoot we can capture. > > Factoring it out like this will help when we go to make this method something flush all animations. (Which I guess is later in this patch series?) OK. Let's use a lamda to factor it out. > Does it make sense to do this step in the first loop? (And then just assert here that effects is non-null if we assume that MaybeUpdateCascadeResults won't affect the result of GetEffectSet.) > > It's up to you which you prefer. I think yes. We can remove this in the first loop. MaybeUpdateCascadeResults() doesn't add a new restyle on an element without a EffectSet, so it is safe. (still can assert it.)
Comment on attachment 8868510 [details] Bug 1334036 - Part 11: Trigger animation-only restyle when we handle an event with coordinates. https://reviewboard.mozilla.org/r/140112/#review143768 I've mostly just checked stylistic issues here. The logic seems right but Cameron will know better. ::: dom/animation/EffectCompositor.h:236 (Diff revision 1) > // traversal (e.g. changing member variables) for all elements that we expect > // to restyle on the next traversal. > + // When |aFlushThrottledAnimations| is true, we force to restyle throttled > + // animations. > // Returns true if there are elements needing a restyle for animation. > - bool PreTraverse(); > + bool PreTraverse(bool aFlushThrottledAnimations = false); Any chance we can replace this bool parameter with an enum (or even an empty struct -- just something that is understandable at the call site)? If there aren't too many call sites of this I'd probably drop the default value too and explicitly specify the behavior we want at each call site. ::: dom/animation/EffectCompositor.cpp:983 (Diff revision 1) > CascadeLevel cascadeLevel = CascadeLevel(i); > auto& elementSet = mElementsToRestyle[cascadeLevel]; > for (auto iter = elementSet.Iter(); !iter.Done(); iter.Next()) { > const NonOwningAnimationTarget& target = iter.Key(); > > - // Ignore throttled restyle. > + // When there are non-animation restyles *OR* |aFlushThrottledAniamtions| Typo here: aFlushThrottledAnimations But perhaps we could make this whole section a bit more clear with a comment like: "We can skip this element if it has a throttled restyle (in which case iter.Data() will be false), but not if we also have non-animation restyles (since we'll want the up-to-date animation style when we go to process them so we can trigger transitions correctly), and not if we are currently flushing all throttled animation restyles." ::: layout/base/PresShell.cpp:6949 (Diff revision 1) > if (presContext->RestyleManager()->IsGecko()) { > - // XXX stylo: ServoRestyleManager doesn't support animations yet. > presContext->RestyleManager()->AsGecko()->UpdateOnlyAnimationStyles(); > + } else { > + presContext->RestyleManager()->AsServo()->FlushPendingAnimationStyles(); Cameron might have a comment about this -- e.g. can we define a method in RestyleManager.h for this and use MOZ_STYLO_FORWARD for this? ::: layout/base/ServoRestyleManager.cpp:673 (Diff revision 1) > } > > +void > +ServoRestyleManager::FlushPendingAnimationStyles() > +{ > + // Bug 1302948: We also need to implement this for SMIL. This is not the right bug number. You need to file a bug blocking bug 1302948 and put *that* bug number here. ::: layout/style/ServoTypes.h:56 (Diff revision 1) > // Indicates whether the Servo style system should perform normal processing or > +// whether it should perform only animation-only processing, or > // whether it should traverse in a mode that doesn't generate any change hints, "Indicates whether the Servo style system should perform normal processing, animation-only processing (so we can flush any throttled animation styles), or whether it should traverse..." ::: servo/ports/geckolib/glue.rs:248 (Diff revision 1) > + let (without_normal_restyle, restyle_behavior) = match restyle_behavior { > + Restyle::ForAnimationOnly => (true, Restyle::Normal), > + other => (false, other) > + }; > + > let traversal_flags = match (root_behavior, restyle_behavior) { > (Root::Normal, Restyle::Normal) => TraversalFlags::empty(), > (Root::UnstyledChildrenOnly, Restyle::Normal) => UNSTYLED_CHILDREN_ONLY, > (Root::Normal, Restyle::ForReconstruct) => FOR_RECONSTRUCT, > _ => panic!("invalid combination of TraversalRootBehavior and TraversalRestyleBehavior"), > }; > > if element.has_animation_only_dirty_descendants() || > element.has_animation_restyle_hints() { > traverse_subtree(element, > raw_data, > traversal_flags | ANIMATION_ONLY, > unsafe { &*snapshots }); > } > > + if without_normal_restyle { > + return false; > + } This seems a bit confusing to me, especially the negative in 'without_normal_restyle' (skip_normal_restyle?). Could we just drop this first check, make the traversal_flags part something like: let traversal_flags = match (root_behavior, restyle_behavior) { (Root::Normal, Restyle::Normal) | (Root::Normal, Restyle::ForAnimationOnly) => TraversalFlags::empty(), (Root::UnstyledChildrenOnly, Restyle::Normal) | (Root::UnstyledChildrenOnly, Restyle::ForAnimationOnly) => UNSTYLED_CHILDREN_ONLY, (Root::Normal, Restyle::ForReconstruct) => FOR_RECONSTRUCT, _ => panic!("invalid combination of TraversalRootBehavior and TraversalRestyleBehavior"), }; And then change the last part to: if restyle_behavior == Restyle::ForAnimationOnly { return false; }
Attachment #8868510 - Flags: review?(bbirtles) → review+
Comment on attachment 8868510 [details] Bug 1334036 - Part 11: Trigger animation-only restyle when we handle an event with coordinates. https://reviewboard.mozilla.org/r/140112/#review143800
Comment on attachment 8868510 [details] Bug 1334036 - Part 11: Trigger animation-only restyle when we handle an event with coordinates. https://reviewboard.mozilla.org/r/140112/#review143768 > Cameron might have a comment about this -- e.g. can we define a method in RestyleManager.h for this and use MOZ_STYLO_FORWARD for this? Yes, we could, and it would be nice to get rid of the switching here and have a single method name that we call.
Comment on attachment 8868510 [details] Bug 1334036 - Part 11: Trigger animation-only restyle when we handle an event with coordinates. https://reviewboard.mozilla.org/r/140112/#review143820 r=me with Brian's comments addressed.
Attachment #8868510 - Flags: review?(cam) → review+
Comment on attachment 8839101 [details] Bug 1334036 - Part 1: Avoid mutating mElementsToRestyle during pre-traversal. https://reviewboard.mozilla.org/r/113640/#review143764 > I think yes. We can remove this in the first loop. MaybeUpdateCascadeResults() doesn't add a new restyle on an element without a EffectSet, so it is safe. (still can assert it.) OK. I think we shouldn't move this into the 1st loop because we still need to set ```foundElementsNeedingRestyle = true``` even if this target doesn't have an EffectSet.
Comment on attachment 8868510 [details] Bug 1334036 - Part 11: Trigger animation-only restyle when we handle an event with coordinates. https://reviewboard.mozilla.org/r/140112/#review143768 > Any chance we can replace this bool parameter with an enum (or even an empty struct -- just something that is understandable at the call site)? > > If there aren't too many call sites of this I'd probably drop the default value too and explicitly specify the behavior we want at each call site. OK. I will drop the default value and try an enum for this. e.g. ``` enum class AnimationRestyleOperator { Normal, // restyle elements that have posted animation restyles All // restyle all elements with animations (i.e. even if the animations are throttled) } ``` > Yes, we could, and it would be nice to get rid of the switching here and have a single method name that we call. OK. Let me unify them.
Comment on attachment 8868510 [details] Bug 1334036 - Part 11: Trigger animation-only restyle when we handle an event with coordinates. Hi, Cameron, could you please check to "post traversal" part in ServoRestyleManager::UpdateOnlyAnimationStyles()? Thanks.
Attachment #8868510 - Flags: review+ → review?(cam)
Comment on attachment 8868510 [details] Bug 1334036 - Part 11: Trigger animation-only restyle when we handle an event with coordinates. https://reviewboard.mozilla.org/r/140112/#review143980 ::: layout/base/ServoRestyleManager.cpp:684 (Diff revision 2) > + MOZ_ASSERT(!mInStyleRefresh); > + > + nsIDocument* doc = PresContext()->Document(); > + ServoStyleSet* styleSet = StyleSet(); > + MOZ_ASSERT(doc, "Invalid document"); > + MOZ_ASSERT(styleSet, "Invalid servo style set"); StyleSet() never returns null, so I wouldn't bother asserting it here. ::: layout/base/ServoRestyleManager.cpp:685 (Diff revision 2) > + > + nsIDocument* doc = PresContext()->Document(); > + ServoStyleSet* styleSet = StyleSet(); > + MOZ_ASSERT(doc, "Invalid document"); > + MOZ_ASSERT(styleSet, "Invalid servo style set"); > + Comparing to ProcessPendingRestyles: * Do we need an AnimationsWithDestroyedFrame in here, and a call to StopAnimationsForElementsWithoutFrames at the end? If not, why not? * Do we need a FlushOverflowChangedTracker call at the end? If not, why not? * Do we need a MostRecentRefresh call in here? If not, why not? I think the answers to the first two are probably "yes", since it looks like Gecko will do that. I'm not sure about the third one. If we do need all of these, then it might be easier, in your following refactoring patch, just to have a single function that does all of the current ProcessPendingRestyles work, and which takes an argument that says if it's animation-only, so it can skip doing bits like the ClearSnapshots call, etc.
Comment on attachment 8868510 [details] Bug 1334036 - Part 11: Trigger animation-only restyle when we handle an event with coordinates. https://reviewboard.mozilla.org/r/140112/#review143980 > Comparing to ProcessPendingRestyles: > > * Do we need an AnimationsWithDestroyedFrame in here, and a call to StopAnimationsForElementsWithoutFrames at the end? If not, why not? > > * Do we need a FlushOverflowChangedTracker call at the end? If not, why not? > > * Do we need a MostRecentRefresh call in here? If not, why not? > > I think the answers to the first two are probably "yes", since it looks like Gecko will do that. I'm not sure about the third one. > > If we do need all of these, then it might be easier, in your following refactoring patch, just to have a single function that does all of the current ProcessPendingRestyles work, and which takes an argument that says if it's animation-only, so it can skip doing bits like the ClearSnapshots call, etc. I think "AnimationsWithDestroyedFrame" is only for changing display::none, and we don't need to handle this in animation-only. However, calling this is fine. FlushOverflowChangedTracker and MostRecentRefresh call look like we need them. The following function we shouldn't call: 1.if (mHaveNonAnimationRestyles) { ++mAnimationGeneration; } I think we don't want to add the animation generation for animation-only update. 2. CleanSnapShot() -> We didn't run normal restyle, so I guess we shouldn't clean it. 3. styleSet->AssertTreeIsClean(); -> We might still have some elements needed to be restyle by normal restyle later, so this might assert. Therefore, yes, thanks for the suggestion. I will update this and the refactor patch.
Comment on attachment 8868510 [details] Bug 1334036 - Part 11: Trigger animation-only restyle when we handle an event with coordinates. https://reviewboard.mozilla.org/r/140112/#review144332 Just a few drive-by nits ::: dom/animation/EffectCompositor.h:232 (Diff revision 3) > nsCSSPropertyID aProperty, > const AnimationPerformanceWarning& aWarning); > > + // The operator to represent what kind of animation restyle we want. > + enum class AnimationFlushOperator { > + Normal, // Restyle elements that have posted animation restyles. Perhaps this would make more sense as an enum called AnimationRestyleType? With values Throttled / Full? ::: dom/animation/EffectCompositor.h:241 (Diff revision 3) > + // When |aFlushThrottledAnimations| is true, we force to restyle throttled > + // animations. This comment looks to be out of date. ::: layout/base/ServoRestyleManager.cpp:551 (Diff revision 3) > + // Recreate style contexts, and queue up change hints (which also handle > + // lazy frame construction). > + nsStyleChangeList currentChanges(StyleBackendType::Servo); > + DocumentStyleRootIterator iter(doc); > + while (Element* root = iter.GetNextStyleRoot()) { > + ProcessPostTraversal(root, nullptr, styleSet, currentChanges); > + } > + > + // Process the change hints. > + // > + // Unfortunately, the frame constructor can generate new change hints while > + // processing existing ones. We redirect those into a secondary queue and > + // iterate until there's nothing left. > + ReentrantChangeList newChanges; > + mReentrantChanges = &newChanges; > + while (!currentChanges.IsEmpty()) { > + ProcessRestyledFrames(currentChanges); > + MOZ_ASSERT(currentChanges.IsEmpty()); > + for (ReentrantChange& change: newChanges) { > + currentChanges.AppendChange(change.mContent->GetPrimaryFrame(), > + change.mContent, change.mHint); > + } > + newChanges.Clear(); > + } > + mReentrantChanges = nullptr; This block of code seems identical to ProcessPendingRestyles. Can't you factor out the common code somehow?
Comment on attachment 8868509 [details] Bug 1334036 - Part 2: Restyle all elements with animations if there are non-animation restyles. https://reviewboard.mozilla.org/r/140110/#review144354 r=me with the following addressed. ::: dom/animation/EffectCompositor.cpp:965 (Diff revision 2) > MOZ_ASSERT(NS_IsMainThread()); > MOZ_ASSERT(mPresContext->RestyleManager()->IsServo()); > > + // We need to force flush all throttled animations if there are any > + // non-animation restyles. > + bool needsForceFlush = aRoot && aRoot->HasDirtyDescendantsForServo(); Perhaps 'flushThrottledRestyles' would be more clear? ::: dom/animation/EffectCompositor.cpp:973 (Diff revision 2) > - // Ignore throttled restyle. > - if (!aIter.Data()) { > + // Ignore throttled restyle if |needsForceFlush| is unset. > + if (!aIter.Data() && !needsForceFlush) { > return returnTarget; > } This might be more clear as: // If aIter.Data() is false, the element only requested a throttled // (skippable) restyle, so we can skip it if flushThrottledRestyles is not // true. if (!flushThrottledRestyles && !aIter.Data()) { return returnTarget; } ::: dom/animation/EffectCompositor.cpp:1080 (Diff revision 2) > + // We need to force flush all throttled animations if there are any > + // non-animation restyles. Nit: "We need to flush all throttled animation restyles too if there are any non-animation restyles." ::: dom/animation/EffectCompositor.cpp:1083 (Diff revision 2) > + bool needsForceFlush = elementToRestyle && > + elementToRestyle->HasDirtyDescendantsForServo(); Again, 'flushThrottledRestyles' might be better here. ::: dom/animation/EffectCompositor.cpp:1090 (Diff revision 2) > - if (!elementSet.Get(key)) { > - // Ignore throttled restyle and no restyle request. > + if (!elementSet.Contains(key)) { > + // No restyle request. > + continue; > + } > + > + if (!elementSet.Get(key) && !needsForceFlush) { > + // Ignore throttled restyle if |needsForceFlush| is unset. > continue; > } Let's just combine this into one hashmap lookup: // Skip if we don't have a restyle, or if we only have a throttled (skippable) // restyle and we're not required to flush throttled restyles. bool hasUnthrottledRestyle = false; if (!element.Get(key, &hasUnthrottledRestyle) || (!flushThrottledRestyles && !hasUnthrottledRestyle)) { continue; }
Attachment #8868509 - Flags: review?(bbirtles) → review+
Comment on attachment 8868510 [details] Bug 1334036 - Part 11: Trigger animation-only restyle when we handle an event with coordinates. https://reviewboard.mozilla.org/r/140112/#review144332 > Perhaps this would make more sense as an enum called AnimationRestyleType? With values Throttled / Full? OK, I will update its name. Thanks for re-review this patch. > This block of code seems identical to ProcessPendingRestyles. Can't you factor out the common code somehow? Yes, I factor out this in the next patch. Thanks for reminder.
Comment on attachment 8868510 [details] Bug 1334036 - Part 11: Trigger animation-only restyle when we handle an event with coordinates. https://reviewboard.mozilla.org/r/140112/#review144332 > OK, I will update its name. Thanks for re-review this patch. AnimationRestyleType::Full means we want to restyle all animations, but AnimationRestyleType::Throttled means we only restyle throttled animations or we only restyle posted animations?
(In reply to Boris Chiou [:boris] from comment #230) > Comment on attachment 8868510 [details] > Bug 1334036 - Part 12: Trigger animation-only restyle when we handle an > event with coordinates. > > https://reviewboard.mozilla.org/r/140112/#review144332 > > > OK, I will update its name. Thanks for re-review this patch. > > AnimationRestyleType::Full means we want to restyle all animations, but > AnimationRestyleType::Throttled means we only restyle throttled animations > or we only restyle posted animations? It's a throttled restyle meaning we only restyle animations that posted a restyle.
(In reply to Brian Birtles (:birtles) from comment #231) > It's a throttled restyle meaning we only restyle animations that posted a > restyle. OK, I see. Thanks.
No longer depends on: 1346049
See Also: → 1346049
Blocks: 1346049
See Also: 1346049 →
Comment on attachment 8868510 [details] Bug 1334036 - Part 11: Trigger animation-only restyle when we handle an event with coordinates. https://reviewboard.mozilla.org/r/140112/#review144440
Attachment #8868510 - Flags: review?(cam) → review+
Comment on attachment 8868947 [details] Bug 1334036 - Part 13: Factor out ProcessPendingRestyles. https://reviewboard.mozilla.org/r/140602/#review144442 Looks good, thanks. (Might make sense to just fold this patch into the previous one.)
Attachment #8868947 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #234) > Comment on attachment 8868947 [details] > Bug 1334036 - Part 13: Factor out ProcessPendingRestyles. > > https://reviewboard.mozilla.org/r/140602/#review144442 > > Looks good, thanks. (Might make sense to just fold this patch into the > previous one.) OK, I will merge this with the previous one. Thanks.
Attachment #8868947 - Attachment is obsolete: true
Attachment #8866245 - Attachment is obsolete: true
Attached file Servo PR, #16963 —
Servo PR was landed, but tree (autoland) is closed, so please feel free to push to button of those patches. (They are ready now.)
Pushed by bchiou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bb7ba1de3ff3 Part 1: Avoid mutating mElementsToRestyle during pre-traversal. r=birtles https://hg.mozilla.org/integration/autoland/rev/ce3fdd4c0d51 Part 2: Restyle all elements with animations if there are non-animation restyles. r=birtles https://hg.mozilla.org/integration/autoland/rev/babbbe4db707 Part 3: Add a flag to represent we are in pre-traversal. r=birtles https://hg.mozilla.org/integration/autoland/rev/e417ced91e37 Part 4: Remove unused UpdateCascadeResults function. r=birtles,hiro https://hg.mozilla.org/integration/autoland/rev/750c87d61884 Part 5: Implement FFI for finding properties overriding animations. r=birtles,emilio https://hg.mozilla.org/integration/autoland/rev/933bece8ce16 Part 6: Trigger restyle if important rules are changed. r=birtles,emilio https://hg.mozilla.org/integration/autoland/rev/663f9e058cd3 Part 7: Merge two similiar MaybeUpdateCascadeResults functions. r=birtles https://hg.mozilla.org/integration/autoland/rev/3d4a563e33be Part 8: Add AddLayerChangesForAnimation in ServoRestyleManager. r=birtles https://hg.mozilla.org/integration/autoland/rev/3fbe04823914 Part 9: Add one FFI which return None transform. r=emilio https://hg.mozilla.org/integration/autoland/rev/db95d6d01b0b Part 10: Return AnimationValue for BaseStyle. r=hiro https://hg.mozilla.org/integration/autoland/rev/096add92a79f Part 11: Trigger animation-only restyle when we handle an event with coordinates. r=birtles,heycam https://hg.mozilla.org/integration/autoland/rev/3ca6e6b80f47 Part 12: Enable off-main thread animations. r=birtles
No longer depends on: 1340005
Depends on: 1388566
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: