Last Comment Bug 699143 - beginElement() begins not taking document time into account
: beginElement() begins not taking document time into account
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Daniel Holbert [:dholbert] (largely AFK until June 28)
:
Mentors:
: 646134 (view as bug list)
Depends on: 700077
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-02 10:49 PDT by Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
Modified: 2012-04-16 10:52 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (962 bytes, image/svg+xml)
2011-11-02 10:51 PDT, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
no flags Details
reference case (just added a <set/> element) (1.01 KB, image/svg+xml)
2011-11-02 16:13 PDT, Daniel Holbert [:dholbert] (largely AFK until June 28)
no flags Details
fix (1.77 KB, patch)
2011-11-02 17:13 PDT, Daniel Holbert [:dholbert] (largely AFK until June 28)
no flags Details | Diff | Review
fix v2 (1.56 KB, patch)
2011-11-02 18:36 PDT, Daniel Holbert [:dholbert] (largely AFK until June 28)
no flags Details | Diff | Review
fix v2a (now with mochitest) (5.81 KB, patch)
2011-11-03 16:17 PDT, Daniel Holbert [:dholbert] (largely AFK until June 28)
no flags Details | Diff | Review
fix v3 (now calling Sample() instead of UpdateCurrentTime()) (5.88 KB, patch)
2011-11-03 19:26 PDT, Daniel Holbert [:dholbert] (largely AFK until June 28)
no flags Details | Diff | Review
fix v3a (test updated) (5.80 KB, patch)
2011-11-03 19:38 PDT, Daniel Holbert [:dholbert] (largely AFK until June 28)
bbirtles: review+
Details | Diff | Review

Description Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-11-02 10:49:19 PDT
beginElement() does not seem to work if you have a document with no SMIL elements, then insert one and call beginElement() on it in the same script. In this case it seems like we're not keeping a timeline, so when nsSMILTimedElement::BeginElementAt calls container->GetCurrentTime() it returns 0, since we haven't been bothering to set its mCurrentTime.
Comment 1 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-11-02 10:51:34 PDT
Created attachment 571373 [details]
testcase

In this testcase, when you click it inserts a blue rect with an animation element. You should see the rect animate from 0% to 100% width. However, if you wait a second after loading the page before clicking, it will start midway through the animation.
Comment 2 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-11-02 11:08:37 PDT
Calling nsSMILTimedElement::BeginElementAt in nsSMILTimedElement::BeginElementAt does not seem to be sufficient to fix this bug.
Comment 3 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-11-02 11:09:03 PDT
Err, calling nsSMILTimedElement::UpdateCurrentTime() I mean.
Comment 4 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-11-02 16:13:38 PDT
Created attachment 571497 [details]
reference case (just added a <set/> element)

Here's a tweaked version of the testcase, with just a <set/> element added (not targeting any particular attribute).

This test-tweak is sufficient to init the SMIL timeline correctly and produce expected behavior.
Comment 5 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-11-02 16:38:16 PDT
UpdateCurrentTime is semi-misleading -- we actually receive the "current time" value in nsSMILAnimationController::WillRefresh().

That callback arrives asynchronously, after we've registered with the refresh driver, and we only register with the refresh driver when the first animation element is added to the document.  So presumably our first WillRefresh() is firing *after* our beginElement() call, so the timestamp that beginElement sees is the (stale) document-start-time timestamp.

I suspect we might need to update mCurrentSampleTime in the mDeferredStartSampling case within nsSMILAnimationController::RegisterAnimationElement()
Comment 6 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-11-02 16:47:49 PDT
(In reply to Daniel Holbert [:dholbert] from comment #5)
> I suspect we might need to update mCurrentSampleTime in the
> mDeferredStartSampling case within
> nsSMILAnimationController::RegisterAnimationElement()

Oh, nevermind -- we already update it inside of nsSMILAnimationController::StartSampling.  Hmm... investigating more.
Comment 7 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-11-02 16:53:45 PDT
OK, calling UpdateCurrentTime() inside of nsSMILAnimationController::RegisterAnimationElement (after the StartSampling call) seems to fix this.  I think that makes sense...
Comment 8 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-11-02 17:13:14 PDT
Created attachment 571515 [details] [diff] [review]
fix

Still need to write automated tests, but this should do it for the fix, I think.
Comment 9 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-11-02 18:36:00 PDT
Created attachment 571531 [details] [diff] [review]
fix v2

(reverted an accidental/unrelated newline-insertion)
Comment 10 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-11-03 16:17:24 PDT
Created attachment 571826 [details] [diff] [review]
fix v2a (now with mochitest)

I've added a mochitest to the patch now.

For the record, there are exactly two checks in the mochitest that fail in unpatched builds.  Those failing checks are this "isnot":
>  a.beginElement();
>  isnot(svg.getCurrentTime(), 0,
>       "beginElement should've triggered a synchronous sample " +
>       "and updated our current time");

and this "is()":

>  // Now, if we rewind to time=0, our "beginElement"-triggered animation
>  // shouldn't be in effect yet.
>  svg.setCurrentTime(0);
>  is(r.width.animVal.valueAsString, INITIAL_VAL,
>     "after rewinding to 0, our beginElement()-generated interval " +
>     "shouldn't be active yet");

(These checks pass with the fix applied, of course.)

I ran the fix through TryServer yesterday, and it passed.  Now I'm running it through TryServer with the mochitest included.
Comment 11 Brian Birtles (:birtles) 2011-11-03 17:16:25 PDT
Comment on attachment 571826 [details] [diff] [review]
fix v2a (now with mochitest)

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

Sorry for the delay in getting to this (public holiday yesterday). Thanks for taking care of this!

::: content/smil/nsSMILAnimationController.cpp
@@ +213,5 @@
> +      // mCurrentTime is probably stale.  We need to update it here (using the
> +      // just-updated mCurrentSampleTime that StartSampling has given us), or
> +      // else an immediate beginElement() call on the just-registered animation
> +      // element would incorrectly use our stale mCurrentTime as its beginning.
> +      UpdateCurrentTime();

I think this is fine but I wonder if we can somehow align with what we do in nsSMILAnimationController::AddChild. There we call Sample() before MaybeStartSampling to force a sample. Could we do the same here? (With the exception that we'd use StartSampling here instead of MaybeStartSampling).

The behaviour would be different of course--we'd actually be doing a sample and the test case behaviour would differ--but I wonder if that's a problem or if in fact it's more intuitive as well as making the code a little more consistent. I need to think it through a bit more.

::: content/smil/test/test_smilDynamicDelayedBeginElement.xhtml
@@ +1,3 @@
> +<html xmlns="http://www.w3.org/1999/xhtml">
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=

Missing id here.
Comment 12 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-11-03 19:26:40 PDT
Created attachment 571872 [details] [diff] [review]
fix v3 (now calling Sample() instead of UpdateCurrentTime())

(In reply to Brian Birtles (:birtles) from comment #11)
> Sorry for the delay in getting to this (public holiday yesterday). Thanks
> for taking care of this!

No worries! Didn't seem like much of a delay at all, actually. :) Thanks for the review.

> I think this is fine but I wonder if we can somehow align with what we do in
> nsSMILAnimationController::AddChild. There we call Sample() before
> MaybeStartSampling to force a sample. Could we do the same here?

Almost. We can definitely replace the UpdateCurrentTime() call with a Sample() call (since Sample will trigger an UpdateCurrentTime) -- however, we can't move that call *before* StartSampling(), or else it won't benefit from the updated mCurrentSampleTime that we get inside of StartSampling.

So, this patch adds a synchronous Sample() as you suggest, but it adds it _after_ StartSampling() so that we use the updated mCurrentSampleTime.  (otherwise the attached mochitest fails)

(I also updated the mochitest so it'll now expect getCurrentTime to be updated slightly earlier now -- after the element-insertion, rather than after the beginElement call)

Also: For consistency, I tweaked AddChild so that its "[Maybe]StartSampling(); Sample();" order matches the new RegisterAnimationElement() code.  I don't anticipate that change having any significant functional effect.

> > +https://bugzilla.mozilla.org/show_bug.cgi?id=
> 
> Missing id here.

Oops. :)  As I wrote this test, I was actually laughing internally at how many different places one needs to paste the bug ID into an autogenerated mochitest template... I pasted it in 4 places, but missed the 5th. :)  Fixed now.

Currently running this through TryServer as a sanity-check, btw:
  https://tbpl.mozilla.org/?tree=Try&rev=2bc097621248
Comment 13 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-11-03 19:28:47 PDT
Comment on attachment 571872 [details] [diff] [review]
fix v3 (now calling Sample() instead of UpdateCurrentTime())

oops, there's a comment I need to update in the test. doing that...
Comment 14 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-11-03 19:38:41 PDT
Created attachment 571874 [details] [diff] [review]
fix v3a (test updated)

OK, updated the prose in the test, now that getCurrentTime will be updated as soon as we insert the animation element.

I also added one more check (to verify that simply inserting the animation doesn't have any effect on its target attr, since it has no intervals yet due to begin='indefinite').
Comment 15 Brian Birtles (:birtles) 2011-11-03 20:12:17 PDT
Comment on attachment 571874 [details] [diff] [review]
fix v3a (test updated)

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

Looks great! Thanks Daniel!
Comment 16 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-11-04 09:33:29 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6b5c76be4d0
Comment 17 Marco Bonardo [::mak] 2011-11-05 02:49:08 PDT
https://hg.mozilla.org/mozilla-central/rev/d6b5c76be4d0
Comment 18 Daniel Holbert [:dholbert] (largely AFK until June 28) 2012-04-16 10:52:39 PDT
*** Bug 646134 has been marked as a duplicate of this bug. ***

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