Closed Bug 1303235 Opened 8 years ago Closed 7 years ago

stylo: Implement KeyframeEffectReadOnly::CalculateCumulativeChangeHint

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox51 --- wontfix
firefox-esr52 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: birtles, Assigned: boris)

References

Details

Attachments

(6 files, 4 obsolete files)

Bug 1303235 - Part 1: Add ResolveServoStyleByAddingAnimation.
59 bytes, text/x-review-board-request
hiro
: review+
ritu
: approval-mozilla-beta+
Details
Bug 1303235 - Part 2: Templatize CalculateCumulativeChangeHint.
59 bytes, text/x-review-board-request
hiro
: review+
ritu
: approval-mozilla-beta+
Details
Bug 1303235 - Part 3: Enable test_restyle.html and remove the early return in CanIgnoreIfNotVisible.
59 bytes, text/x-review-board-request
hiro
: review+
ritu
: approval-mozilla-beta+
Details
Servo PR, #18945
41 bytes, text/x-github-pull-request
ritu
: approval-mozilla-beta+
Details | Review
The servo PR for beta
10.49 KB, patch
hiro
: review+
Details | Diff | Splinter Review
The part 1 patch for beta
3.64 KB, patch
hiro
: review+
Details | Diff | Splinter Review 
This is used for an optimization that determines which animations can be throttled if they are offscreen.

As far as I can tell it depends on calling CreateStyleContextForAnimationValue which uses the StyleAnimationValues stored in the properties array (mProperties) and passes them to nsStyleSet::ResolveStyleByAddingRules to create a temporary style context (which it does by adding the StyleAnimationValue to an AnimValuesStyleRule that uncomputes it to an nsCSSValue).

Once we have some sort of opaque reference to servo computed values (and the means to uncompute them) perhaps we'll be able to call something on the Servo side to generate this temporary nsStyleContext. From there we should be able to call nsStyleContext::CalcStyleDifference as we currently do since I believe that works for servo-back nsStyleContexts.
Priority: -- → P3
Moving this to block bug 1317211 instead since this is not required to bootstrap CSS animations/transitions.
Blocks: 1317211
No longer blocks: 1302945
Priority: P3 → --
Component: DOM: Animation → CSS Parsing and Computation
Priority: -- → P4
Blocks: 1405744
Given bug 1405744, this probably should be P3?
Priority: P4 → P3
Oops. It's time to fix this.
Attached file A work-in-progress patch for reference (obsolete) — 
What we need here is that a function which is similar to Servo_StyleSet_GetBaseComputedValuesForElement. Unlike Servo_StyleSet_GetBaseComputedValuesForElement, the new function takes AnimationValue and adds the AnimationValue in rule tree.

This patch has the function but it has horribly duplicated code, needs to be cleaned up.
Assignee: nobody → boris.chiou
Status: NEW → ASSIGNED
(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> Created attachment 8915846 [details]
> A work-in-progress patch for reference
> 
> What we need here is that a function which is similar to
> Servo_StyleSet_GetBaseComputedValuesForElement. Unlike
> Servo_StyleSet_GetBaseComputedValuesForElement, the new function takes
> AnimationValue and adds the AnimationValue in rule tree.
> 
> This patch has the function but it has horribly duplicated code, needs to be
> cleaned up.

Yes. Your function really helps me. Thanks a lot.
Attached patch Whole things what we need to do (obsolete) — — Splinter Review 
For reference whether we can uplift the fix or not, this is an updated patch that contains whole things we need to do here.  The new function, Servo_StyleSet_GetComputedValuesByAddingAnimation, in glue.rs still needs to be cleaned up, but anyway I think this can be uplifted to beta.

Also I did push a try with this patch to make sure this patch really fixes the bbc issue (bug 1405744) or not.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdcd22b1255f6ba2b62f03193a3d0d5887e19b27
Attachment #8915846 - Attachment is obsolete: true
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> Created attachment 8916853 [details] [diff] [review]
> Whole things what we need to do
> 
> For reference whether we can uplift the fix or not, this is an updated patch
> that contains whole things we need to do here.  The new function,
> Servo_StyleSet_GetComputedValuesByAddingAnimation, in glue.rs still needs to
> be cleaned up, but anyway I think this can be uplifted to beta.
> 
> Also I did push a try with this patch to make sure this patch really fixes
> the bbc issue (bug 1405744) or not.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=fdcd22b1255f6ba2b62f03193a3d0d5887e19b27

Thanks, Hiro.

Besides, in this bug, we also need to remove the early return in CanIgnoreIfNotVisible() [1] and the empty "void CalculateCumulativeChangeHint(const ServoStyleContext* aComputedValues)" [2] because we call this function actually on Stylo.

[1] http://searchfox.org/mozilla-central/rev/b53e29293c9e9a2905f4849f4e3c415e2013f0cb/dom/animation/KeyframeEffectReadOnly.cpp#1795-1799
[2] http://searchfox.org/mozilla-central/rev/b53e29293c9e9a2905f4849f4e3c415e2013f0cb/dom/animation/KeyframeEffectReadOnly.h#263-265
Oops! Thanks for pointing it out. The early return is really an import thing. Why the test cases in test_restyle.html passed locally?  I must be still missing something.
I did intentionally not use template function to make the patch minimum but we might want to..
Attachment #8916853 - Attachment is obsolete: true
(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> Created attachment 8916859 [details] [diff] [review]
> Whole things what we need to do v2
> 
> I did intentionally not use template function to make the patch minimum but
> we might want to..

Yap~, and I am trying to minimize it. Thanks.
(In reply to Boris Chiou [:boris] from comment #10)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> > Created attachment 8916859 [details] [diff] [review]
> > Whole things what we need to do v2
> > 
> > I did intentionally not use template function to make the patch minimum but
> > we might want to..
> 
> Yap~, and I am trying to minimize it. Thanks.

Nice! Really good to hear!
Comment on attachment 8916859 [details] [diff] [review]
Whole things what we need to do v2

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

::: dom/animation/KeyframeEffectReadOnly.cpp
@@ +1744,5 @@
> +                                        mTarget->mElement,
> +                                        mTarget->mPseudoType,
> +                                        aComputedValues,
> +                                        segment.mFromValue.mServo,
> +                                        mAnimation->CascadeLevel());

We need to get the cascade level without mAnimation since there are cases that mAnimation is null at this moment.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> Comment on attachment 8916859 [details] [diff] [review]
> Whole things what we need to do v2
> 
> Review of attachment 8916859 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/KeyframeEffectReadOnly.cpp
> @@ +1744,5 @@
> > +                                        mTarget->mElement,
> > +                                        mTarget->mPseudoType,
> > +                                        aComputedValues,
> > +                                        segment.mFromValue.mServo,
> > +                                        mAnimation->CascadeLevel());
> 
> We need to get the cascade level without mAnimation since there are cases
> that mAnimation is null at this moment.

We don't need to pass the cascade level since gecko does use transitions level for animations too for now.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> > Comment on attachment 8916859 [details] [diff] [review]
> > Whole things what we need to do v2
> > 
> > Review of attachment 8916859 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/animation/KeyframeEffectReadOnly.cpp
> > @@ +1744,5 @@
> > > +                                        mTarget->mElement,
> > > +                                        mTarget->mPseudoType,
> > > +                                        aComputedValues,
> > > +                                        segment.mFromValue.mServo,
> > > +                                        mAnimation->CascadeLevel());
> > 
> > We need to get the cascade level without mAnimation since there are cases
> > that mAnimation is null at this moment.
> 
> We don't need to pass the cascade level since gecko does use transitions
> level for animations too for now.

Yeah, so we probably could rename RuleTree::add_animation_rule as RuleTree::add_animation_rule_at_transition_level and use CascadeLevel::Transitions directly, so we could drop this argument for simplification.
(In reply to Boris Chiou [:boris] from comment #14)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> > > Comment on attachment 8916859 [details] [diff] [review]
> > > Whole things what we need to do v2
> > > 
> > > Review of attachment 8916859 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: dom/animation/KeyframeEffectReadOnly.cpp
> > > @@ +1744,5 @@
> > > > +                                        mTarget->mElement,
> > > > +                                        mTarget->mPseudoType,
> > > > +                                        aComputedValues,
> > > > +                                        segment.mFromValue.mServo,
> > > > +                                        mAnimation->CascadeLevel());
> > > 
> > > We need to get the cascade level without mAnimation since there are cases
> > > that mAnimation is null at this moment.
> > 
> > We don't need to pass the cascade level since gecko does use transitions
> > level for animations too for now.
> 
> Yeah, so we probably could rename RuleTree::add_animation_rule as
> RuleTree::add_animation_rule_at_transition_level and use
> CascadeLevel::Transitions directly, so we could drop this argument for
> simplification.

Yep, sounds reasonable.
Almost done, but I found it's a little bit hard to refactor the rust code because it seems we should keep the some variable on the top of the rust call stack. Anyway, I believe hiro and emilio may have a better idea during reviewing. :)
Comment on attachment 8918127 [details]
Bug 1303235 - Part 1: Remove unused nsPresContext function argument.

https://reviewboard.mozilla.org/r/188976/#review194290
Attachment #8918127 - Flags: review?(hikezoe) → review+
Comment on attachment 8918128 [details]
Bug 1303235 - Part 1: Add ResolveServoStyleByAddingAnimation.

https://reviewboard.mozilla.org/r/188978/#review194294

::: servo/ports/geckolib/glue.rs:797
(Diff revision 1)
> +    // FIXME: StyleResolverForElement works on the pseudo element itself, instead of the parent
> +    // element, so if we pass the parent element and the |pseudo_type| is Some(_), we return the
> +    // style context of this pseudo element directly. Therefore, I think this is a bug because
> +    // we always use the parent element and a pseudo tag for a pseudo element from the caller.
> +    if let Some(pseudo) = PseudoElement::from_pseudo_type(pseudo_type) {
> +        let styles = &element_data.styles;
> +        return styles
> +            .pseudos
> +            .get(&pseudo)
> +            .expect("GetComputedValuesByAddingAnimation for an unexisting pseudo?")
> +            .clone().into();
> +    }

Actually, I'm thinking should CanIgnoreIfNotVisible() always return false if this is a pseudo element, or we must convert the |parent element, psuedo type| pair into a generated element and pass this generated element into this function? Still think this is a bug.
Comment on attachment 8918128 [details]
Bug 1303235 - Part 1: Add ResolveServoStyleByAddingAnimation.

https://reviewboard.mozilla.org/r/188978/#review194292

::: layout/style/ServoStyleSet.h:401
(Diff revision 1)
> +  //   The additional rules must be appropriate for the transition
> +  //   level of the cascade, which is the highest level of the cascade.
> +  //   (This is the case for one current caller, the cover rule used
> +  //   for CSS transitions.)

I guess if we pass correct cascade level, we can optimize further but it's another story.

::: layout/style/ServoStyleSet.h:405
(Diff revision 1)
> +  // This must hold:
> +  //   The additional rules must be appropriate for the transition
> +  //   level of the cascade, which is the highest level of the cascade.
> +  //   (This is the case for one current caller, the cover rule used
> +  //   for CSS transitions.)
> +  // Note: |aElement| is the original parent element, not the generated element.

We should clearly say it's pseudo element case? I.e. |aElement| is not the parent if the element is not pseudo.

::: servo/ports/geckolib/glue.rs:743
(Diff revision 1)
>              .get(&pseudo)
>              .expect("GetBaseComputedValuesForElement for an unexisting pseudo?")
>              .clone().into();
>      }
>  
> +    debug_assert!(!snapshots.is_null());

Why this asserton moved down here?

::: servo/ports/geckolib/glue.rs:797
(Diff revision 1)
> +    // FIXME: StyleResolverForElement works on the pseudo element itself, instead of the parent
> +    // element, so if we pass the parent element and the |pseudo_type| is Some(_), we return the
> +    // style context of this pseudo element directly. Therefore, I think this is a bug because
> +    // we always use the parent element and a pseudo tag for a pseudo element from the caller.
> +    if let Some(pseudo) = PseudoElement::from_pseudo_type(pseudo_type) {
> +        let styles = &element_data.styles;
> +        return styles
> +            .pseudos
> +            .get(&pseudo)
> +            .expect("GetComputedValuesByAddingAnimation for an unexisting pseudo?")
> +            .clone().into();
> +    }

This is apparently wrong. Actually it's originally written by me!
In the case of pseudo, |computed_values| is for the pseudoe element, so I think Servo_StyleSet_GetComputedValuesByAddingAnimation should take the pseudo element it self and doesnot need to take CSSPseudoElementType.  So this function will get more simpler.
Attachment #8918128 - Flags: review?(hikezoe)
Comment on attachment 8918129 [details]
Bug 1303235 - Part 2: Templatize CalculateCumulativeChangeHint.

https://reviewboard.mozilla.org/r/188980/#review194296

::: dom/animation/KeyframeEffectReadOnly.cpp:1728
(Diff revision 1)
> +  ServoStyleSet* styleSet =
> +    aBaseStyleContext->PresContext()->StyleSet()->AsServo();
> +  return styleSet->ResolveServoStyleByAddingAnimation(mTarget->mElement,
> +                                                      mTarget->mPseudoType,
> +                                                      aBaseStyleContext,
> +                                                      aValue.mServo);

As I commented for the previous patch, we should pass the pseudo element itself instead of the parent, so we need to get the pseudo element here, or inside ServoStyleSet::ResolveServoStyleByAddingAnimation.
Attachment #8918129 - Flags: review?(hikezoe) → review+
Comment on attachment 8918130 [details]
Bug 1303235 - Part 3: Enable test_restyle.html and remove the early return in CanIgnoreIfNotVisible.

https://reviewboard.mozilla.org/r/188982/#review194300
Attachment #8918130 - Flags: review?(hikezoe) → review+
Comment on attachment 8918128 [details]
Bug 1303235 - Part 1: Add ResolveServoStyleByAddingAnimation.

https://reviewboard.mozilla.org/r/188978/#review194292

> I guess if we pass correct cascade level, we can optimize further but it's another story.

OK, I will pass the correct cascade level.

> Why this asserton moved down here?

Just move it before the usage. I can put it back. :)

> This is apparently wrong. Actually it's originally written by me!
> In the case of pseudo, |computed_values| is for the pseudoe element, so I think Servo_StyleSet_GetComputedValuesByAddingAnimation should take the pseudo element it self and doesnot need to take CSSPseudoElementType.  So this function will get more simpler.

OK, let's pass the pseudo element itself. Thanks a lot.
Comment on attachment 8918129 [details]
Bug 1303235 - Part 2: Templatize CalculateCumulativeChangeHint.

https://reviewboard.mozilla.org/r/188980/#review194296

> As I commented for the previous patch, we should pass the pseudo element itself instead of the parent, so we need to get the pseudo element here, or inside ServoStyleSet::ResolveServoStyleByAddingAnimation.

Agree. Really thanks for this suggestion.
(In reply to Boris Chiou [:boris] from comment #26)
> Comment on attachment 8918128 [details]
> Bug 1303235 - Part 2: Add ResolveServoStyleByAddingAnimation and the related
> FFI.
> 
> https://reviewboard.mozilla.org/r/188978/#review194292
> 
> > I guess if we pass correct cascade level, we can optimize further but it's another story.
> 
> OK, I will pass the correct cascade level.

No, I don't think it's worth doing now since we'd like to uplift these patches so we should make them as simple as possible.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #28)
> (In reply to Boris Chiou [:boris] from comment #26)
> > Comment on attachment 8918128 [details]
> > Bug 1303235 - Part 2: Add ResolveServoStyleByAddingAnimation and the related
> > FFI.
> > 
> > https://reviewboard.mozilla.org/r/188978/#review194292
> > 
> > > I guess if we pass correct cascade level, we can optimize further but it's another story.
> > 
> > OK, I will pass the correct cascade level.
> 
> No, I don't think it's worth doing now since we'd like to uplift these
> patches so we should make them as simple as possible.

OK. I see. Thanks.
Attachment #8918128 - Flags: review?(hikezoe)
Comment on attachment 8918128 [details]
Bug 1303235 - Part 1: Add ResolveServoStyleByAddingAnimation.

https://reviewboard.mozilla.org/r/188978/#review194348

::: servo/ports/geckolib/glue.rs:770
(Diff revision 2)
> +    let rules = match computed_values.rules {
> +        None => return computed_values.clone_arc().into(),

One thing I am concerned is that we should add debug_assert here.  If we fail to get rules from the computed_values and then return the original computed_values,  we might accidentally throttle animations.

I am wondering when we don't get the rules?

::: servo/ports/geckolib/glue.rs:792
(Diff revision 2)
> +    if element.borrow_data().is_none() {
> +        return computed_values.clone_arc().into();
> +    }

Likewise here.  We should add debug_assert here.
Attachment #8918128 - Flags: review+
Comment on attachment 8918128 [details]
Bug 1303235 - Part 1: Add ResolveServoStyleByAddingAnimation.

https://reviewboard.mozilla.org/r/188978/#review194348

> One thing I am concerned is that we should add debug_assert here.  If we fail to get rules from the computed_values and then return the original computed_values,  we might accidentally throttle animations.
> 
> I am wondering when we don't get the rules?

I'm thinking should we return ```null``` for these cases? If we found the returned style context is null, we set ```mCumulativeChangeHint = ~nsChangeHint_Hints_CanIgnoreIfNotVisible```

> Likewise here.  We should add debug_assert here.

I found this may happen on some crashtests, so that's why I add this. Maybe this element is not restyle in some cases? I'm not really sure.
(In reply to Boris Chiou [:boris] from comment #35)
> Comment on attachment 8918128 [details]
> Bug 1303235 - Part 2: Add ResolveServoStyleByAddingAnimation and the related
> FFI.
> 
> https://reviewboard.mozilla.org/r/188978/#review194348
> 
> > One thing I am concerned is that we should add debug_assert here.  If we fail to get rules from the computed_values and then return the original computed_values,  we might accidentally throttle animations.
> > 
> > I am wondering when we don't get the rules?
> 
> I'm thinking should we return ```null``` for these cases? If we found the
> returned style context is null, we set ```mCumulativeChangeHint =
> ~nsChangeHint_Hints_CanIgnoreIfNotVisible```

Yeah, sounds good to me.

> > Likewise here.  We should add debug_assert here.
> 
> I found this may happen on some crashtests, so that's why I add this. Maybe
> this element is not restyle in some cases? I'm not really sure.

If we have computed values but have no style data, it means... the element is display:none subtree?  We should check the case, and if it happens only display:none case, we can also set ~nsChangeHint_Hints_CanIgnoreIfNotVisible. (it actually throttle regardless of this change hint)
Comment on attachment 8918128 [details]
Bug 1303235 - Part 1: Add ResolveServoStyleByAddingAnimation.

Resend the review request.

We return a Strong::null() if an error happens. And we set "mCumulativeChangeHint = ~nsChangeHint_Hints_CanIgnoreIfNotVisible;" if the context is null.
Attachment #8918128 - Flags: review+ → review?(hikezoe)
Got many failures on try but cannot reproduce locally on mac. :(

1  libxul.so!mozilla::UniquePtr<SystemGroupImpl, mozilla::DefaultDelete<SystemGroupImpl> >::~UniquePtr [nsCOMPtr.h:17a6cfc4e486 : 2  libc-2.23.so + 0x39ff8
3  libc-2.23.so + 0x3a045
4  libxul.so!atomic_refcell::AtomicBorrowRef::do_panic [os.rs:0ade339411587887bf01bcfa2e9ae4414c8900d4 : 511 + 0xa]
5  libxul.so!_fini + 0x6578e8
6  libxul.so!geckoservo::glue::Servo_StyleSet_GetComputedValuesByAddingAnimation [lib.rs:17a6cfc4e486 : 130 + 0x5]

Especially most of them happened only on opt build. Need to check them later.
Comment on attachment 8918128 [details]
Bug 1303235 - Part 1: Add ResolveServoStyleByAddingAnimation.

Too many crashes on add_animation_rules_at_transition_level() on try server. I should update this function...
Attachment #8918128 - Flags: review?(hikezoe)
(In reply to Boris Chiou [:boris] from comment #43)
> OK, I fixed the crashes on Linux because of Arc panic.

Could you please explain what happens there exactly?

+        // FIXME: remove animation or transition rules first.
+        let mut current = self.remove_animation_rules(path);

We don't need to remove any animation or transition level rules at all. If we try to update *transitons* level rule, it should replace old transition level, no?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #44)
> (In reply to Boris Chiou [:boris] from comment #43)
> > OK, I fixed the crashes on Linux because of Arc panic.
> 
> Could you please explain what happens there exactly?
> 
> +        // FIXME: remove animation or transition rules first.
> +        let mut current = self.remove_animation_rules(path);
> 
> We don't need to remove any animation or transition level rules at all. If
> we try to update *transitons* level rule, it should replace old transition
> level, no?

Sorry, the code in try is a temporary version to fix the crashes I met, and we shouldn't remove animation rules. I will update this later. Besides, I notice that even if we call update_rule_at_level(CascadeLevel::Transitions, ...) directly, we still have the same failures.
(In reply to Boris Chiou [:boris] from comment #45)
> Sorry, the code in try is a temporary version to fix the crashes I met, and
> we shouldn't remove animation rules. I will update this later. Besides, I
> notice that even if we call update_rule_at_level(CascadeLevel::Transitions,
> ...) directly, we still have the same failures.

For example [1]:
var p = document.getElementById("display");
var cs = getComputedStyle(p, "");
p.style.transition = "opacity 200ms linear";

p.style.opacity = "1";
is(cs.opacity, "1", "bug 1144410 test - initial opacity");
p.style.opacity = "0";
is(cs.opacity, "1", "bug 1144410 test - opacity after starting transition");  // UNEXPECTED_ERROR, cs.opacity == "0"

When calling cs.opacity, we create a transition (opacity, from 1 to 0), and calculate the mCumulativeChangeHint for this transition. Both Gecko and Stylo have the same mCumulativeChangeHint now, and so CanIgnoreIfNotVisible() and CanThrottle() return true on both Gecko and Stylo.

However, GetComputedStyle() in Stylo returns the incorrect result. I think our calculation of mCumulativeChangeHint is correct, but why cs.opacity returns the style attribute, i.e. opacity = 0, instead of the transition value?

[1] http://searchfox.org/mozilla-central/rev/40b456626e2d0409b7034768b4d9526fc7235ea4/layout/style/test/test_transitions_dynamic_changes.html#79-83
(In reply to Boris Chiou [:boris] from comment #46)
> (In reply to Boris Chiou [:boris] from comment #45)
> > Sorry, the code in try is a temporary version to fix the crashes I met, and
> > we shouldn't remove animation rules. I will update this later. Besides, I
> > notice that even if we call update_rule_at_level(CascadeLevel::Transitions,
> > ...) directly, we still have the same failures.
> 
> For example [1]:
> var p = document.getElementById("display");
> var cs = getComputedStyle(p, "");
> p.style.transition = "opacity 200ms linear";
> 
> p.style.opacity = "1";
> is(cs.opacity, "1", "bug 1144410 test - initial opacity");
> p.style.opacity = "0";
> is(cs.opacity, "1", "bug 1144410 test - opacity after starting transition");
> // UNEXPECTED_ERROR, cs.opacity == "0"
> 
> When calling cs.opacity, we create a transition (opacity, from 1 to 0), and
> calculate the mCumulativeChangeHint for this transition. Both Gecko and
> Stylo have the same mCumulativeChangeHint now, and so
> CanIgnoreIfNotVisible() and CanThrottle() return true on both Gecko and
> Stylo.

CanThrottle() should not return true, if the target element is not out-of-view.  That's the problem.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #47)
> CanThrottle() should not return true, if the target element is not
> out-of-view.  That's the problem.

Yes. However, this target element is
  <p id="display" style="text-indent: 100px"></p>
the target element doesn't have any content, so that's why "frame->IsScrolledOutOfView" returns true in both Gecko and Stylo.

Besides, even if this restyle type is throttled, we should get correct computed value of this transition while calling getComputedStyle(). This may be a different bug, and I should check what happened during restyling.
(In reply to Boris Chiou [:boris] from comment #48)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #47)
> > CanThrottle() should not return true, if the target element is not
> > out-of-view.  That's the problem.
> 
> Yes. However, this target element is
>   <p id="display" style="text-indent: 100px"></p>
> the target element doesn't have any content, so that's why
> "frame->IsScrolledOutOfView" returns true in both Gecko and Stylo.

I don't think IsScrolledOutOfView() should return true for the case, it's bug 1385013?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #49)
> (In reply to Boris Chiou [:boris] from comment #48)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #47)
> > > CanThrottle() should not return true, if the target element is not
> > > out-of-view.  That's the problem.
> > 
> > Yes. However, this target element is
> >   <p id="display" style="text-indent: 100px"></p>
> > the target element doesn't have any content, so that's why
> > "frame->IsScrolledOutOfView" returns true in both Gecko and Stylo.
> 
> I don't think IsScrolledOutOfView() should return true for the case, it's
> bug 1385013?

Not pretty sure, but very similar.

If I use "<p id="display" style="text-indent: 100px">temp</p>", IsScrolledOutOfView() is false, and everything is ok. I guess Servo doesn't work because we use a different animation-only restyling for this new created transition. If the restyle is throttled, we don't restyle it in the 2nd animation-only restyling?, so getComputedStyle returns the incorrect result. Gecko uses the different restyling mechanism for animation, so we don't see this bug in this test case. (just a guess)
Depends on: 1385013
Attachment #8918127 - Attachment is obsolete: true
Attachment #8918128 - Flags: review?(hikezoe)
Attachment #8916859 - Attachment is obsolete: true
Attachment #8918128 - Flags: review?(hikezoe)
Try looks good, so will land this after bug 1385013.
I don't still understand what the Arc panic issue is.  Is it solved once we eliminate Arc<Locked<PropertyDeclarationBlcok>> in bug 1404006?  Can it be solved with introducing temporary scopes?
Which panic is it?
I didn't try temporary scopes, but it seems we shouldn't use ArcBorrow() on the top function call stack in the FFI. That call stack is hard to understand what happened because it may panic on different places, and most panics happened on AtomicBorrowRef::do_panic() on opt build (so I don't have enough information for now), and sometimes it happened in rule_tree::ensure_child(). I think bug 1404006 may help this because we could avoid using Arc<Lock<_>> and ArcBorrow<Lock<_>>.
This is the try result I pushed after getting all r+: https://treeherder.mozilla.org/#/jobs?repo=try&revision=17a6cfc4e486c4e970d6e90d1b0cb143d1e73d00

It seems we already put the ArcBorrow() in a temporary scope?
Ah, OK. That's an easy mistake but a horrible one.  The returned value from PropertyDeclarationBlock::with_one is not Arc-ed one, we can't use ArcBorrow there.  Unfortunately we can't use where clause for ArcBorrow::from_ref() since the function takes XXBorrowed types, right?
Attachment #8918128 - Flags: review?(hikezoe) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #64)
> Ah, OK. That's an easy mistake but a horrible one.  The returned value from
> PropertyDeclarationBlock::with_one is not Arc-ed one, we can't use ArcBorrow
> there.  Unfortunately we can't use where clause for ArcBorrow::from_ref()
> since the function takes XXBorrowed types, right?

Yeah, so using an Arc to take it and converting it into ArcBorrow make things work. Thanks. :)
Attached file Servo PR, #18945 — 

    
    

    
  
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b363b9f12d29
Part 1: Add ResolveServoStyleByAddingAnimation. r=hiro
https://hg.mozilla.org/integration/autoland/rev/424a9f6cd53c
Part 2: Templatize CalculateCumulativeChangeHint. r=hiro
https://hg.mozilla.org/integration/autoland/rev/fbf6d7bf4c7d
Part 3: Enable test_restyle.html and remove the early return in CanIgnoreIfNotVisible. r=hiro
Depends on: 1383239

    
    

    
  
Is this something that can/should ride the 58 train?

          status-firefox51:
          affected → wontfix

          status-firefox56:
          --- → wontfix

          status-firefox57:
          --- → ?

          status-firefox-esr52:
          --- → wontfix
Flags: needinfo?(boris.chiou)

    
    

    
  
I think it's worth uplifting this if the high CPU usage on bbc.com (bug 1405744) is solved by this, if not, I dont't think it's worth uplifting.

    
    

    
  
(In reply to Hiroyuki Ikezoe (:hiro) from comment #73)
> I think it's worth uplifting this if the high CPU usage on bbc.com (bug
> 1405744) is solved by this, if not, I dont't think it's worth uplifting.

OK, I see. So we should wait someone to check if these patches are useful for bug 1405744. Thanks.

    
    

    
  
> OK, I see. So we should wait someone to check if these patches are useful
> for bug 1405744. Thanks.

seems this is useful, see bug 1405744 comment 14

    
    

    
  
OK. we will uplift this bug (i.e. the Servo PR and all gecko patches, part 1 ~ part 3) to beta. Let's wait bug 1385013.
Flags: needinfo?(boris.chiou)

    
    

    
  
You don't need to wait for bug 1385013, you can put the bug number in [List of other uplifts needed for the feature/fix].

    
    

    
  
Comment on attachment 8919983 [details] [review]
Servo PR, #18945

Approval Request Comment
[Feature/Bug causing the regression]: This bug fixed a top site issue, Bug 1405744. See bug 1405744 comment 14.
[User impact if declined]: The CPU usage becomes high at night on https://www.bbc.co.uk/news.
[Is this code covered by automated tests?]: Yes, in test_restyle.html.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: We verified this. See bug 1405744 comment 14.
[List of other uplifts needed for the feature/fix]: Bug 1385013.
[Is the change risky?]: low risk.
[Why is the change risky/not risky?]: We add one more FFI, and do some computation while creating an animation/transition. This FFI should be safe. Besides, in terms of performance, we add some minor computation which avoid unnecessary animations on some top websites, and we have verified that by CPU usages, so this advantage outweighs the disadvantage.
[String changes made/needed]: No.

Please uplift following patches in order:
1. Servo PR, #18945
2. Part 1: Add ResolveServoStyleByAddingAnimation.
3. Part 2: Templatize CalculateCumulativeChangeHint.
4. Part 3: Enable test_restyle.html and remove the early return in CanIgnoreIfNotVisible.

        Attachment #8919983 -
        Flags: approval-mozilla-beta?

    
  

        Attachment #8918128 -
        Flags: approval-mozilla-beta?

    
  

        Attachment #8918129 -
        Flags: approval-mozilla-beta?

    
  

        Attachment #8918130 -
        Flags: approval-mozilla-beta?

    
    

    
  
Hi Jet, Chris, I am worried about the size of this change and beta57 uplift. I took https://bugzilla.mozilla.org/show_bug.cgi?id=1385013#c30 as it would help with high cpu usage problem on bbc.co.uk (bug 1405744). Is this one a must for 57? Our review of beta uplifts and risk to quality has mostly shown a direct correlation between size of patch and risk to release quality. I hesitate to take a big change like this late in beta57 cycle. Thanks for your help!
Flags: needinfo?(cpeterson)
Flags: needinfo?(bugs)

    
    

    
  
I think bug 1385013 fixed some non-animating animations and was not specifically about reducing high CPU usage. So I don't think bug 1385013 would lessen the benefit of this bug's patches.

According to BBC bug 1405744 comment 0, Beta 57's CPU usage on bbc.co.uk is about 50% while Gecko's is about 14%. So already Gecko is using a fair amount of CPU. Perhaps the Beta 57 regression to 50% is something we can live with until we ship 58?
Flags: needinfo?(cpeterson)

    
    

    
  
Ritu, I spoke with Hiro on the Stylo team about this bug. He thinks this fix is not that risky and that we should uplift it to Beta 57.

    
    

    
  
Comment on attachment 8919983 [details] [review]
Servo PR, #18945

Thanks Chris. Given the perf wins and low risk, let's uplift to Beta57.

        Attachment #8919983 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

        Attachment #8918129 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

        Attachment #8918130 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

        Attachment #8918128 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

    
    

    
  
The Servo patch doesn't graft cleanly to Beta. Hiro, can you please provide a rebased patch since Boris is away?
Flags: needinfo?(bugs) → needinfo?(hikezoe)

    
    

    
  
Yep, no problem.  Now building them locally. Servo side patch and part 1 had to be rebased. The patches are coming soon.

    
    

    
  


  
Flags: needinfo?(hikezoe)

        Attachment #8921678 -
        Flags: review+

        Attachment #8921678 -
        Attachment is patch: true

    
    

    
  
Checked that building on linux is fine and run mochitests in dom/animations, layout/style/test_animationXX and layout/style/test_transitionXX, and run crash tests in dom/animation.

    
    

    
  
Sorry I'm out of office now. Hiro, thanks for rebasing this.

    
    

    
  
(In reply to Boris Chiou [:boris] (away 24 Oct – 12 Nov) from comment #78)
> [Is this code covered by automated tests?]: Yes, in test_restyle.html.
> [Has the fix been verified in Nightly?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: We verified this.
> See bug 1405744 comment 14.

Setting qe-verify- based on the fact that this fix has automated coverage.
Flags: qe-verify-

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: