879255 - refactor fix for 613888 to make more sense

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

From irc.mozilla.org #layout (times UTC+9):
> [2013-06-03 12:06:19] <nrc> dbaron: ping
> [2013-06-03 12:06:27] <dbaron> nrc, pong, though heading out pretty soon
> [2013-06-03 12:06:45] <nrc> dbaron: http://mxr.mozilla.org/mozilla-central/source/layout/style/nsTransitionManager.cpp#711
> [2013-06-03 12:06:58] <nrc> the comment seems to be very different from the 'if' statement
> [2013-06-03 12:07:05] <nrc> do you know what is going on here?
> [2013-06-03 12:07:16] <nrc> as in, is the ocmment correct or the code?
> [2013-06-03 12:07:20] <nrc> *comment
> [2013-06-03 12:07:52] <nrc> or have I got something mixed up?
> [2013-06-03 12:08:51] <dbaron> so currentIndex != NoIndex corresponds to the "in the middle of a transition" bit
> [2013-06-03 12:09:28] <nrc> ah, that I didn't realise
> [2013-06-03 12:10:09] <nrc> dbaron: but we seem to be comparing two end values, not a 'current in-progress' value
> [2013-06-03 12:10:20] <dbaron> nrc, that's the second paragraph of the comment
> [2013-06-03 12:10:45] <dbaron> nrc, though I'm still thinking through whether it ought to be "!haveValues ||" or "haveValues &&"
> [2013-06-03 12:10:53] <dbaron> (it definitely needs to be protected by one or the other, though)
> [2013-06-03 12:11:35] <nrc> yes, the comment says one current value and one end value, but the code is two end values
> [2013-06-03 12:11:42] <dbaron> nrc, and the !shouldAnimate outside of it is the "to exactly the current in-progress value" bit
> [2013-06-03 12:11:43] <nrc> (i think)
> [2013-06-03 12:12:31] <dbaron> nrc, indeed, that seems odd
> [2013-06-03 12:12:55] <dbaron> nrc, I have to run pretty much now-ish,  though (need to eat lunch and then be somewhere else in 60 minutes)
> [2013-06-03 12:13:04] <nrc> ok
> [2013-06-03 12:13:06] <dbaron> nrc, I'll probably be back in ~4 hours or so
> [2013-06-03 12:13:13] <nrc> thanks for the info
> [2013-06-03 12:13:24] <nrc> I'll see if I can work out some more in the mean-time
> [2013-06-03 12:13:27] <dbaron> nrc, there might be something useful in the bug that added that comment
> [2013-06-03 12:13:38] <dbaron> (particularly the second half of it)
> [2013-06-03 12:13:44] <dbaron> anyway, 'later

> [2013-06-03 15:41:33] <dbaron> nrc, did you make sense of that code in nsTransitionManager you were asking about earlier?

> [2013-06-03 16:46:36] <nrc> dbaron: sort of, I think it is wrong, but I'm not 100% sure. I think that |pts[currentIndex].mEndValue != pt.mEndValue| should be
> [2013-06-03 16:47:08] <nrc> |pts[currentIndex].mEndValue != val && val == pt.mEndValue|
> [2013-06-03 16:47:39] <nrc> where val is the current value of the transition, found using nsStyleAnimation::Interpolate
> [2013-06-03 16:47:45] <nrc> or something similar
> [2013-06-03 16:48:44] <nrc> dbaron: but, I was trying to work out what should actually happen if we hit this path with a potential new transition where the value does not change from the old
to the new context
> [2013-06-03 16:48:53] <nrc> i.e., haveChange is false
> [2013-06-03 16:50:03] <nrc> and in fact, I couldn't imagine when that would happen
> [2013-06-03 16:50:24] <nrc> (actually, I have an example but it doesn't seem to be the common case, but I have no idea what the common case is)

So the main interesting part of that code that isn't from the
original transitions landing comes from
https://bugzilla.mozilla.org/show_bug.cgi?id=613888

There doesn't seem to be much of interest in the bug other than the
diff.

(I don't know why |haveChange| is a separate variable now, but it
should probably be put back into the expression where it was, since
it doesn't make sense to compare pt.mStartValue and pt.mEndValue
unless haveValues is true.)


So, anyway, I suppose there are three values of interest:

  pt.mStartValue is probably the "current" value in the old
  transition, since aOldStyleContext is a with-animation style
  context, but we can't technically guarantee that since the
  transition need not be the winning thing in the CSS cascade.  So
  we __don't want to use that for anything__ other than the origin
  of the transition (and, if this new transition isn't a reverse,
  the reversing test for later transitions).  Essentially the start
  is old data that we really want to use as little as possible.

  pt.mEndValue is the new target of a transition (if there's a
  change to trigger a transition) or simply the new value (if
  there's not to be a transition).  (aNewStyleContext is, on the
  other hand, a without-animation style context.)

  pts[currentIndex].mStartValue is the start of the currently
  running transition.  The same comments as pt.mStartValue apply; it
  shouldn't really be used in any transitions logic other than the
  interpolation.  (We have a separate value for the reversing test.)

  pts[currentIndex].mStartForReversingTest.  Again, this is a value
  we want to avoid as much as possible; it differs from mStartValue
  only in the case of having had previous reverses.

  pts[currentIndex].mEndValue is the other meaningful value; it was
  the old "final" value before the current style change.

Not sure this is all that useful, but I didn't delete it..


Essentially what the code in question is trying to do is ensure that
we hit this case about 35 lines lower:

    if (oldPT.mEndValue == pt.mEndValue) {
      // If we got a style change that changed the value to the endpoint
      // of the currently running transition, we don't want to interrupt
      // its timing function.
      // WalkTransitionRule already called RestyleForAnimation.
      return;
    }

and just do *nothing* if the end value hasn't changed.  I suppose
it would have been clearer to move that chunk of code up.  I think
that's the basic explanation for why the current code is correct.



So I've now written in my tree a small patch series that refactors
this code so that I think it makes sense.  I'll post it for your
review once I check that it compiles.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Refactor fix for bug 613888, step 1: create haveCurrentTransition variable.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Refactor fix for bug 613888, step 2: consolidate oldPT variable.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Refactor fix for bug 613888, step 3: move no-change test earlier so that we don't have to clutter conditions between the new location and old with logic to fall through to it.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Refactor fix for bug 613888, step 4: consolidate conditions, and reindent (and fix bracing while doing so).

Comment 5

[Nick Cameron [:nrc]]

Comment on attachment 757931 [details] [diff] [review]
Refactor fix for bug 613888, step 1:  create haveCurrentTransition variable.

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

::: layout/style/nsTransitionManager.cpp
@@ +825,1 @@
>      pts[currentIndex] = pt;

perhaps it is worth asserting |currentIndex != nsTArray<ElementPropertyTransition>::NoIndex| here, but maybe that is a little paranoid.

Comment 6

[Nick Cameron [:nrc]]

Comment on attachment 757934 [details] [diff] [review]
Refactor fix for bug 613888, step 3:  move no-change test earlier so that we don't have to clutter conditions between the new location and old with logic to fall through to it.

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

::: layout/style/nsTransitionManager.cpp
@@ +716,5 @@
> +  // transition that the current value rounds to the final value.  In
> +  // this case, we'll end up with shouldAnimate as false (because
> +  // there's no value change), but we need to return early here rather
> +  // than cancel the running transition because shouldAnimate is false!
> +  if (haveCurrentTransition && oldPT->mEndValue == pt.mEndValue) {

do we need to check haveValues here? if it is false I think pt.mEndValue will be bogus

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Nick Cameron [:nrc] from comment #6)
> > +  if (haveCurrentTransition && oldPT->mEndValue == pt.mEndValue) {
> 
> do we need to check haveValues here? if it is false I think pt.mEndValue
> will be bogus

Indeed.  Changed to:
  if (haveCurrentTransition && haveValues && oldPT->mEndValue == pt.mEndValue) {

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7cdf2abff971
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d9f698d2b937
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/889e1e5c5f63
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f03f1f19492b

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Nick Cameron [:nrc] from comment #5)
> Comment on attachment 757931 [details] [diff] [review]
> Refactor fix for bug 613888, step 1:  create haveCurrentTransition variable.
> 
> Review of attachment 757931 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/nsTransitionManager.cpp
> @@ +825,1 @@
> >      pts[currentIndex] = pt;
> 
> perhaps it is worth asserting |currentIndex !=
> nsTArray<ElementPropertyTransition>::NoIndex| here, but maybe that is a
> little paranoid.

Sorry -- I missed this earlier -- I tend to think not; these variables as a whole can be assumed valid inside any haveCurrentTransition block.  It's a little annoying, but it doesn't seem worth having every assertion, and this particular case doesn't seem any different from the rest.

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7cdf2abff971
https://hg.mozilla.org/mozilla-central/rev/d9f698d2b937
https://hg.mozilla.org/mozilla-central/rev/889e1e5c5f63
https://hg.mozilla.org/mozilla-central/rev/f03f1f19492b