Closed
Bug 879255
Opened 11 years ago
Closed 11 years ago
refactor fix for 613888 to make more sense
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: dbaron, Assigned: dbaron)
Details
Attachments
(4 files)
3.53 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
4.03 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
4.02 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
3.86 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #757931 -
Flags: review?(ncameron)
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #757933 -
Flags: review?(ncameron)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #757934 -
Flags: review?(ncameron)
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #757936 -
Flags: review?(ncameron)
Comment 5•11 years ago
|
||
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.
Attachment #757931 -
Flags: review?(ncameron) → review+
Updated•11 years ago
|
Attachment #757933 -
Flags: review?(ncameron) → review+
Comment 6•11 years ago
|
||
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
Attachment #757934 -
Flags: review?(ncameron) → review+
Updated•11 years ago
|
Attachment #757936 -
Flags: review?(ncameron) → review+
Assignee | ||
Comment 7•11 years ago
|
||
(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) {
Assignee | ||
Comment 8•11 years ago
|
||
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
Assignee | ||
Comment 9•11 years ago
|
||
(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•11 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in
before you can comment on or make changes to this bug.
Description
•