Closed Bug 1267937 Opened 8 years ago Closed 8 years ago

mProgressOnLastCompose should be nullified in somewhere

Categories

(Core :: DOM: Animation, defect, P3)

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
e10s + ---
firefox49 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(4 files)

step.html
573 bytes, text/html
Details
A reftest
3.91 KB, patch
Details | Diff | Splinter Review
MozReview Request: Bug 1267937 - Part 1: Clear mProgressOnLastCompose once we are not in effect. r?birtles
58 bytes, text/x-review-board-request
birtles
: review+
Details
MozReview Request: Bug 1267937 - Part 2: A reftest which checks mProgressOnLastCompose is surely cleared in before phase. r?birtles
58 bytes, text/x-review-board-request
birtles
: review+
Details
Attached file step.html — 
While I am writing a reftest in bug 1235002, I noticed there are some cases we should nullify mProgressOnLastCompose somewhere.  In UpdateProperties?

Attaching file is a test case to confirm this problem.  The blue box in the test case should be rendered 200px but actually not.
Interesting.  I can not reproduce the problem on today's nightly...
OK. I can not reproduce the problem on non-E10S.  But why?
Attached patch A reftest — — Splinter Review 
I think this reftest covers a test case which I was going to write reftest in bug 1235002.
tracking-e10s: --- → ?
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4e23e2e3068d487a85d36f9caf64be953418bd6
I think we can clear the flag if the animation is not in effect in NotifyAnimationTimingUpdated().  But the test on Android failed on try...
Oops, I did not know that 'mach try' leaves comments on bugzilla.
We need a long enough duration to ensure that the animation is not finished
when reftest takes a snapshot.  Also, The current time value setting before
changing frames should be far from zero.  That's because if the value is near
by zero, restyles requested by setting currentTime or frames will be processed
with *current time > 0* in the next tick. As a result the animation will be
painted with new frames regardless of the fix for this bug.

Review commit: https://reviewboard.mozilla.org/r/49391/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49391/
Comment on attachment 8746382 [details]
MozReview Request: Bug 1267937 - Part 1: Clear mProgressOnLastCompose once we are not in effect. r?birtles

https://reviewboard.mozilla.org/r/49389/#review46219

For my own reference, I think this basically comes down to the fact that we're looking at isRelevant rather than isInEffect. The reason we need to look at "in effect" is that Animation::ComposeStyle now includes an early return for IsInEffect() so we won't update mProgressOnLastCompose there.

My understanding is that if we're not in effect, then GetComputedTiming().mProgress must be null.

Now, if mProgressOnLastCompose is already null, there is nothing to do. However, if it's not-null then assuming mAnimation is set (which it currently always is since we only call NotifyAnimationTimingUpdated from an Animation), GetComputedTiming().mProgress != mProgressOnLastCompose so we will enter the block that is guarded:

  if (mAnimation && GetComputedTiming().mProgress != mProgressOnLastCompose) {
  
However, the problem occurs when we are *relevant* but not *in effect*.

Recall that relevant = "in effect" OR "current". So if we are "current" but NOT "in effect" we won't clear mProgressOnLastCompose here (since the test is for !isRelevant and we *are* still relevant in that case). We can only be current in the before or active phase so I don't think this is a problem for the after phase.

As a result, I suspect we only need to change isRelevant to isInEffect but given that mAnimation may become null in the future, I think this change is fine.

::: dom/animation/KeyframeEffect.cpp:198
(Diff revision 1)
> +  // Once we're not in effect, we need to clear mProgressOnLastCompose.
> +  // Otherwise the cached mProgressOnLastCompose will be used if some animaiton
> +  // properties are changed in the before or after phase.

Nit: Blank line before this comment.

I think we should write this as:

  // If we're no longer "in effect", our ComposeStyle method will never be
  // called and we will never have a chance to update mProgressOnLastCompose.
  // We clear mProgressOnLastCompose here to ensure that if we later become
  // "in effect" we will request a restyle (above).
Attachment #8746382 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #9)
> Comment on attachment 8746382 [details]
> MozReview Request: Bug 1267937 - Part 1: Clear mProgressOnLastCompose once
> we are not in effect. r?birtles
> 
> https://reviewboard.mozilla.org/r/49389/#review46219
> 
> For my own reference, I think this basically comes down to the fact that
> we're looking at isRelevant rather than isInEffect. The reason we need to
> look at "in effect" is that Animation::ComposeStyle now includes an early
> return for IsInEffect() so we won't update mProgressOnLastCompose there.
> 
> My understanding is that if we're not in effect, then
> GetComputedTiming().mProgress must be null.

Yes, my understanding is the same.

> However, the problem occurs when we are *relevant* but not *in effect*.
> 
> Recall that relevant = "in effect" OR "current". So if we are "current" but
> NOT "in effect" we won't clear mProgressOnLastCompose here (since the test
> is for !isRelevant and we *are* still relevant in that case). We can only be
> current in the before or active phase so I don't think this is a problem for
> the after phase.

Maybe.  I thought we can write a test case for the after phase with negative endDelay, but never tried.

> As a result, I suspect we only need to change isRelevant to isInEffect but
> given that mAnimation may become null in the future, I think this change is
> fine.

To be honest I have not considered much about the case when mAnimation is null and mTarget null either.  I will reconsider the case when it comes in our tree.

> ::: dom/animation/KeyframeEffect.cpp:198
> (Diff revision 1)
> > +  // Once we're not in effect, we need to clear mProgressOnLastCompose.
> > +  // Otherwise the cached mProgressOnLastCompose will be used if some animaiton
> > +  // properties are changed in the before or after phase.
> 
> Nit: Blank line before this comment.
> 
> I think we should write this as:
> 
>   // If we're no longer "in effect", our ComposeStyle method will never be
>   // called and we will never have a chance to update mProgressOnLastCompose.
>   // We clear mProgressOnLastCompose here to ensure that if we later become
>   // "in effect" we will request a restyle (above).

Thanks!!
Comment on attachment 8746383 [details]
MozReview Request: Bug 1267937 - Part 2: A reftest which checks mProgressOnLastCompose is surely cleared in before phase. r?birtles

https://reviewboard.mozilla.org/r/49391/#review46227

::: layout/reftests/web-animations/1267937-1.html:31
(Diff revision 1)
> +    // restyles requested by setting currentTime or frames in a next tick.
> +    // If this value is near by zero, say -1, the restyles will be processed in
> +    // the next tick and current time in the next tick must be greater than
> +    // zero at that point, that means the animation with new frame values will
> +    // be painted.  As a result, this test will be useless.
> +    anim.currentTime = -1000;

1s still seems fairly long. Would 500ms be enough? If it occasionally passes accidentally, that would be ok, right?

(We could even check inside the requestAnimationFrame callback that we got at least *one* callback with anim.currentTime < 0 -- or anim.effect.getComputedTiming().progress === null -- and if we didn't, we could log a warning?)

::: layout/reftests/web-animations/1267937-1.html:37
(Diff revision 1)
> +
> +    // Setting another frame does not cause any visual changes because
> +    // the animation is still in the before phase.
> +    anim.effect.setFrames({ marginLeft: [ "0px", "400px" ] });
> +
> +    // Is there any altenative events of animationstart for web-animations?

Not that I can think of. The only other approach I can think of is to use DOMWindowUtils.advanceTimeAndRefresh().

::: layout/reftests/web-animations/1267937-1.html:39
(Diff revision 1)
> +    // the animation is still in the before phase.
> +    anim.effect.setFrames({ marginLeft: [ "0px", "400px" ] });
> +
> +    // Is there any altenative events of animationstart for web-animations?
> +    window.requestAnimationFrame(function handleFrame() {
> +      if (anim.currentTime > 0) {

It might be better to use anim.effect.getComputedTiming().progress !== null ?

The reason is that if a frame falls *exactly* at the start of the active interval we should run the test then, right?

Until the animation starts anim.currentTime == 0 so we can't test anim.currentTime >= 0. However, until the animation starts anim.effect.getComputedTiming().progress === null, I think.
Attachment #8746383 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles. away 29 Apr-1 May) from comment #11)
> ::: layout/reftests/web-animations/1267937-1.html:31
> (Diff revision 1)
> > +    // restyles requested by setting currentTime or frames in a next tick.
> > +    // If this value is near by zero, say -1, the restyles will be processed in
> > +    // the next tick and current time in the next tick must be greater than
> > +    // zero at that point, that means the animation with new frame values will
> > +    // be painted.  As a result, this test will be useless.
> > +    anim.currentTime = -1000;
> 
> 1s still seems fairly long. Would 500ms be enough? If it occasionally passes
> accidentally, that would be ok, right?

Good point! Actually I saw the accidentally failure with 500ms on my local emulator once, so I changed it to 1000ms.  But you are right!  It does not matter in this case.

> (We could even check inside the requestAnimationFrame callback that we got
> at least *one* callback with anim.currentTime < 0 -- or
> anim.effect.getComputedTiming().progress === null -- and if we didn't, we
> could log a warning?)

That's a really good idea.  I will do it.

> ::: layout/reftests/web-animations/1267937-1.html:37
> (Diff revision 1)
> > +
> > +    // Setting another frame does not cause any visual changes because
> > +    // the animation is still in the before phase.
> > +    anim.effect.setFrames({ marginLeft: [ "0px", "400px" ] });
> > +
> > +    // Is there any altenative events of animationstart for web-animations?
> 
> Not that I can think of. The only other approach I can think of is to use
> DOMWindowUtils.advanceTimeAndRefresh().
> 
> ::: layout/reftests/web-animations/1267937-1.html:39
> (Diff revision 1)
> > +    // the animation is still in the before phase.
> > +    anim.effect.setFrames({ marginLeft: [ "0px", "400px" ] });
> > +
> > +    // Is there any altenative events of animationstart for web-animations?
> > +    window.requestAnimationFrame(function handleFrame() {
> > +      if (anim.currentTime > 0) {
> 
> It might be better to use anim.effect.getComputedTiming().progress !== null ?
> 
> The reason is that if a frame falls *exactly* at the start of the active
> interval we should run the test then, right?

Yes, exactly!  I will revise it.

Thanks!
(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> (In reply to Brian Birtles (:birtles. away 29 Apr-1 May) from comment #11)
> > ::: layout/reftests/web-animations/1267937-1.html:31
> > (Diff revision 1)
> > > +    // restyles requested by setting currentTime or frames in a next tick.
> > > +    // If this value is near by zero, say -1, the restyles will be processed in
> > > +    // the next tick and current time in the next tick must be greater than
> > > +    // zero at that point, that means the animation with new frame values will
> > > +    // be painted.  As a result, this test will be useless.
> > > +    anim.currentTime = -1000;
> > 
> > 1s still seems fairly long. Would 500ms be enough? If it occasionally passes
> > accidentally, that would be ok, right?
> 
> Good point! Actually I saw the accidentally failure with 500ms on my local

accidentally *success*  It's confusing.
Comment on attachment 8746382 [details]
MozReview Request: Bug 1267937 - Part 1: Clear mProgressOnLastCompose once we are not in effect. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49389/diff/1-2/
Comment on attachment 8746383 [details]
MozReview Request: Bug 1267937 - Part 2: A reftest which checks mProgressOnLastCompose is surely cleared in before phase. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49391/diff/1-2/
https://reviewboard.mozilla.org/r/49389/#review46219

> Nit: Blank line before this comment.
> 
> I think we should write this as:
> 
>   // If we're no longer "in effect", our ComposeStyle method will never be
>   // called and we will never have a chance to update mProgressOnLastCompose.
>   // We clear mProgressOnLastCompose here to ensure that if we later become
>   // "in effect" we will request a restyle (above).

Fixed. Thanks.
Is this a serious web compat issue? The test case doesn't seem to work in IE or Chrome. Marking lower e10s priority here but if you feel this should block rollout please renom.
Assignee: nobody → hiikezoe
tracking-e10s: ? → +
Priority: -- → P3
(In reply to Jim Mathies [:jimm] from comment #18)
> Is this a serious web compat issue?

I don't think so.

> The test case doesn't seem to work in IE or Chrome.

That's because Chrome and IE do not yet implement KeyframeEffect.setFrames() which is used in the test case.
Comment on attachment 8746382 [details]
MozReview Request: Bug 1267937 - Part 1: Clear mProgressOnLastCompose once we are not in effect. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49389/diff/2-3/
Comment on attachment 8746383 [details]
MozReview Request: Bug 1267937 - Part 2: A reftest which checks mProgressOnLastCompose is surely cleared in before phase. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49391/diff/2-3/
https://hg.mozilla.org/integration/mozilla-inbound/rev/15070beaee3548018781299762f6c15995e05924
Bug 1267937 - Part 1: Clear mProgressOnLastCompose once we are not in effect. r=birtles

https://hg.mozilla.org/integration/mozilla-inbound/rev/c358aa5441c953188a2fd2bc6ae0eeb813285d98
Bug 1267937 - Part 2: A reftest which checks mProgressOnLastCompose is surely cleared in before phase. r=birtles
Blocks: 1235002
https://hg.mozilla.org/mozilla-central/rev/15070beaee35
https://hg.mozilla.org/mozilla-central/rev/c358aa5441c9
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: