572270 - SVG SMIL: Implement SMIL TimeEvents

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles)]

SMIL Animation and hence SVG 1.1 provides for TimeEvents to be dispatched when animations begin, end, and repeat.

It makes sense to implement this prior to implementing begin/end event values (bug 485157) since such specifications often refer to TimeEvents, i.e. begin="a.beginEvent"

Implementing this should probably include note only dispatching the events but also supporting the animation event attributes (onbegin, onend, onrepeat) for animation elements (animate, set, etc.)

Comment 1

[Brian Birtles (:birtles)]

Attachment: WIP patch

Work in progress. Mostly implemented. Still need to:
* Confirm tests pass (I've changed the order of some of the operations in nsSMILTimedElement::DoSampleAt and I'm not yet sure they're correct)
* Remove some test assertions
* Decide what to do with the timestamp field. See bug 77992, bug 238041, bug 323039 and http://www.w3.org/TR/SMIL/smil-timing.html#q135

Comment 2

[Brian Birtles (:birtles)]

Attachment: WIP patch v2

Just updated the license block on a couple of files.

Comment 3

[Brian Birtles (:birtles)]

Attachment: Patch v1a

Implementation of SMIL TimeEvents.

Currently the timeStamp is 0 which DOM2 allows but we'll need to fix that to implement event-timing properly (bug 485157). Currently that depends on bug 77992 and bug 238041.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 458259 [details] [diff] [review]
Patch v1a

Looks good to me!  Just some nits:

>diff --git a/content/smil/nsDOMTimeEvent.cpp b/content/smil/nsDOMTimeEvent.cpp
>new file mode 100644
[...]
>+ * The Initial Developer of the Original Code is Mozilla Corporation.

Technically that should be s/Corporation/Foundation/, for the added files in this patch -- Gerv clarified this earlier this year:

http://weblogs.mozillazine.org/gerv/archives/2010/02/mpl_initial_developer_for_mozilla_employ.html

>+nsDOMTimeEvent::nsDOMTimeEvent(nsPresContext* aPresContext, nsEvent* aEvent)
>+  : nsDOMEvent(aPresContext, aEvent ?
>+               aEvent : static_cast<nsEvent *>(new nsUIEvent(PR_FALSE, 0, 0))),

I don't think you need that static_cast, right? nsUIEvent is a subclass of nsEvent, so I'd think you should be able to just pass |new nsUIEvent| without casting.

>+nsresult NS_NewDOMTimeEvent(nsIDOMEvent** aInstancePtrResult,
>+                            nsPresContext* aPresContext,
>+                            nsEvent *aEvent)

This chunk has mixed star-hugging within a single function-declaration - could you make aEvent's star be left-hugging, to match the other args?

> //----------------------------------------------------------------------
>+// Helper class: AsyncTimeEventRunner
>+
>+namespace
>+{
>+  class nsAsyncTimeEventRunner : public nsRunnable

The comment here says AsyncTimeEventRunner, but the code uses an "ns" prefix.  One needs to change to match the other.  I'd probably lean towards the non-"ns" version, since this is just an internal-to-this-file helper-class anyway, and since Mozilla's moving towards non-ns-prefixed classes in general.

>diff --git a/content/smil/test/test_smilTimeEvents.xhtml b/content/smil/test/test_smilTimeEvents.xhtml
[...]
>+  gTimeoutID = setTimeout(timeout, gTimeoutDur);

(there are multiple instances of this line)

Perhaps rename "timeout" to something like "timeoutFail" or "failFromTimeout", to be more clear what these lines are for?

>+    <circle cx="0" cy="0" r="15" fill="blue" id="circle"
>+      onbegin="parentHandler(evt)" onrepeat="parentHandler(evt)"
>+      onend="parentHandler(evt)">
>+      <animate attributeName="cy" from="0" to="100" dur="60s" begin="2s"
>+        id="anim" repeatCount="2"
>+        onbegin="handleOnBegin(evt)" onrepeat="handleOnRepeat(evt)"
>+        onend="handleOnEnd(evt)"/>
[...]
>+gAnim.addEventListener("beginEvent", handleOnBegin, false);
>+gAnim.addEventListener("repeatEvent", handleOnRepeat, false);
>+gAnim.addEventListener("endEvent", handleOnEnd, false);
>+gCircle.addEventListener("beginEvent", parentHandler, false);
>+]]>
>+</script>

So, it looks like we're registering the handlers twice -- at first I thought that was accidental, but based on a comment higher up ("// Two registered handlers"), it looks like its intentional & useful after all.

To prevent confusion, could you add a comment above the first |gAnim.addEventListener| line, saying something like:
  // NOTE: The handlers registered below are **in addition** to the handlers
  // that we've already registered via |onbegin| etc.  We use both types of
  // registration to ensure that they both work.

>diff --git a/content/svg/content/src/nsSVGAnimationElement.cpp b/content/svg/content/src/nsSVGAnimationElement.cpp
>+PRBool
>+nsSVGAnimationElement::IsEventName(nsIAtom* aName)
>+{
>+  return nsContentUtils::IsEventAttributeName(aName, EventNameType_SMIL) ||
>+    nsSVGAnimationElementBase::IsEventName(aName);
>+}

AFAICT, none of the existing "IsEventName" overrides bother to call the inherited method, since the inherited version just unconditionally returns PR_FALSE.  We should probably match that convention here, for consistency (and for speed, since we don't need the extra function-call).

>diff --git a/widget/public/nsGUIEvent.h b/widget/public/nsGUIEvent.h
> #define NS_SVGZOOM_EVENT                  31
> #endif // MOZ_SVG
>+#ifdef MOZ_SMIL
>+#define NS_SMIL_TIMEEVENT                 32
>+#endif // MOZ_SMIL

Looks like this should have an underscore between TIME and EVENT, for consistency with the rest of this file.

> #define NS_TRANSITION_EVENT_START    4200
> #define NS_TRANSITION_END            (NS_TRANSITION_EVENT_START)
> 
>+#ifdef MOZ_SMIL
>+#define NS_SMIL_TIMEEVENT_START      4300
>+#define NS_SMIL_BEGIN                (NS_SMIL_TIMEEVENT_START)
>+#define NS_SMIL_END                  (NS_SMIL_TIMEEVENT_START + 1)
>+#define NS_SMIL_REPEAT               (NS_SMIL_TIMEEVENT_START + 2)

Same applies here (note NS_TRANSITION_EVENT_START in the chunk above it).

r=dholbert with that

Comment 5

[Brian Birtles (:birtles)]

Attachment: Patch v1b, r=dholbert

Address review feedback from comment 4.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 458523 [details] [diff] [review]
Patch v1b, r=dholbert

nsAsyncTimeEventRunner should just be AsyncTimeEventRunner.

Why are you calling HandleDOMEventWithTarget instead of DispatchEvent? We should be able to have events on documents without a presentation, shouldn't we?

Needs review from smaug.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Huh, SMIL spec has really bad interfaces. It could just reuse UIEvent interface
and not invent almost-a-clone of it.

Review coming soon.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 458523 [details] [diff] [review]
Patch v1b, r=dholbert


>+namespace
>+{
Why this "namespace"?


>+  class AsyncTimeEventRunner : public nsRunnable
>+  {
>+  protected:
>+    nsRefPtr<nsIContent> mTarget;
>+    PRUint32             mMsg;
>+    PRInt32              mDetail;
>+
>+  public:
>+    AsyncTimeEventRunner(nsIContent& aTarget, PRUint32 aMsg, PRInt32 aDetail)
nsIContent* aTarget


>+      : mTarget(&aTarget), mMsg(aMsg), mDetail(aDetail)
And then remove '&'

>+    {
>+    }
>+
>+    NS_IMETHOD Run() {
{ should be in the next line


>+      nsIDocument* doc = mTarget->GetCurrentDoc();
>+      if (!doc)
>+        return NS_ERROR_FAILURE;
>+
>+      nsCOMPtr<nsIPresShell> presShell = doc->GetShell();
>+      if (!presShell)
>+        return NS_ERROR_FAILURE;
>+
>+      nsUIEvent event(PR_TRUE, mMsg, mDetail);
>+      event.eventStructType = NS_SMIL_TIME_EVENT;
>+      return presShell->HandleDOMEventWithTarget(mTarget, &event, nsnull);
Just dispatch using nsEventDispatcher::Dispatch(mTarget, nsnull, &event)

Comment 9

[Brian Birtles (:birtles)]

Attachment: Patch v1c, r=dholbert, smaug; sr=roc

(In reply to comment #6)
> Why are you calling HandleDOMEventWithTarget instead of DispatchEvent? We
> should be able to have events on documents without a presentation, shouldn't
> we?

Ok. I've made it pass the pres context when it's available (it's used for setting the view property on the event) and just pass nsnull otherwise.

(In reply to comment #7)
> Review coming soon.

Thanks very much smaug! Much appreciated!

(In reply to comment #8)
> >+namespace
> >+{
> Why this "namespace"?

This is kind of like the C++ way of doing static. The anonymous namespace block means the names can't be seen in other translation units.

> Just dispatch using nsEventDispatcher::Dispatch(mTarget, nsnull, &event)

As mentioned above, instead of always passing nsnull for the pres context I've changed it to pass it when it's available. Currently we use that for setting up the view property on the event.

I noticed we weren't actually checking the view property in the unit tests so I've corrected that too.

Does this sound reasonable?

Comment 10

[Brian Birtles (:birtles)]

Attachment: Patch v1d, r=dholbert, smaug; sr=roc

I'm a bit concerned about the mochitest causing random orange when tinderbox is really bogged down so I've just tweaked the timing in the mochitest to make it a bit more resilient to events getting delayed.

Comment 11

[Brian Birtles (:birtles)]

Comment on attachment 458859 [details] [diff] [review]
Patch v1d, r=dholbert, smaug; sr=roc

Requesting approval to land. We'd like to ship this with our initial version of SMIL since otherwise it's not possible for script to by synchronised with SMIL.

Furthermore, we're hoping to ship event timing (bug 485157) which depends on this.

Comment 12

[Brian Birtles (:birtles)]

Not landing yet since I was getting some random orange on TryServer. I think there's a potential bug when two identical repeat samples are received.

Comment 13

[Brian Birtles (:birtles)]

Attachment: Patch v1e, r=dholbert, smaug; sr=roc

Fix for random orange. I'll hopefully land this before Monday's freeze.

Comment 14

[Brian Birtles (:birtles)]

Pushed: http://hg.mozilla.org/mozilla-central/rev/f73e5032cfad

I'll wait to see how it lands before marking fixed.