485157 - SVG SMIL: Add support for begin/end: event-value

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[worms]

I'm using a Minefield build from http://www.wg9s.com/mozilla/firefox/ which I downloaded on Feb 26th, 2009.

All the SMIL animations I can find that are supposed to start or react to mouse events fail to work. For example:

http://pilat.free.fr/english/animer/animatetransform.htm
http://apike.ca/prog_svg_smil.html
http://srufaculty.sru.edu/david.dailey/svg/newstuff/SMIL1.svg

They all work perfectly in Opera 10a1 alpha.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> All the SMIL animations I can find that are supposed to start or react to mouse
> events fail to work.

That's because our SVG animation implementation doesn't yet support event values for the begin/end attributes.  I'm renaming this bug to be about implementing that, since I don't think we have a bug filed for that yet.

Here's the spec on event-values for begin/end:
http://www.w3.org/TR/SVG11/animate.html#TimingAttributes
http://www.w3.org/TR/SVG11/animate.html#EventValueSyntax

Comment 2

[jonathan chetwynd]

Attachment: set visibility begin

Comment 3

[jonathan chetwynd]

parity opera & safari-webkit

Comment 4

[Jonathan Watt [:jwatt]]

roc: you asked on IRC why event based timing is useful. I think it comes down to convenience for authors, and reducing errors when documents are changed. Whereas with it you can have something like:

<svg>
  <rect id="trigger"/>
  <!-- ... -->
  <rect id="animated">
    <animate begin="trigger.mouseover" attributeName="x" by="200"/>
    <animate begin="trigger.mouseover" attributeName="width" to="500"/>
    <animate begin="trigger.mouseover" attributeName="height" to="500"/>
    <animate begin="trigger.mouseover" attributeName="opacity" to="1"/>
  </rect>
</svg>

Without it you need something like:

<svg>
  <rect id="trigger"
        onmouseover="var a = document.getElementById('animated');
                     a.firstElementChild.beginElement();
                     a.firstElementChild.nextElementSibling.beginElement();"/>
                     a.firstElementChild.nextElementSibling.nextElementSibling.beginElement();
                     a.firstElementChild.nextElementSibling.nextElementSibling.nextElementSibling.beginElement();"/>
  <!-- ... -->
  <rect id="animated" ...>
    <animate attributeName="x" by="200"/>
    <animate attributeName="width" to="500"/>
    <animate attributeName="height" to="500"/>
    <animate attributeName="height" to="1"/>
  </rect>
</svg>

Or more likely without it you'll end up creating a <script> tag in your document to contain the script to iterate through the <animate> element children and pick out those that should be "begun" for the particular animation.

It also seems cleaner to me to have the animate elements know the ID of the element they want to trigger off (one thing) than require the trigger element to know about the <animate> element children of the target (many things) and *which* of those are relevant to the particular animation at hand. If the author changes the <animate> elements in the latter case things are more likely to get out of sync and break.

Comment 5

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

Why not just add a DOM method beginAnimations(), so you can write
  onmouseover="event.target.beginAnimations()"
?

Comment 6

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

For the example in comment 4 you'd have to write
  <rect onmouseover="document.getElementById('animated').beginAnimations()"/>

Comment 7

[Jonathan Watt [:jwatt]]

The issue that immediately springs to mind is that you probably don't want to begin all of the <animate> children of an element. In the case of animating an element "in"/"out" you probably have two sets of animations, one for each direction. Maybe animations could be given some sort of "class" and that could be passed as an argument.

It also raises the question of what to do if the animation elements are not in the element but rather in a <defs>. Also at some point I'd like animations to be able to be applied to multiple elements using CSS selectors or some other mechanism, not just to one element as is possible now. Maybe in both these cases there could be some sort of mechanism to work back to figure out which animations should be begun. Not sure if that would mesh well with a "class" argument.

Comment 8

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

That's true. You're convincing me :-).

Comment 9

[Jeff Schiller]

"Also at some point I'd like animations to be able to be applied to multiple elements using CSS selectors or some other mechanism, not just to one element as is possible now"

Have either of you looked at SMIL Timesheets?  Specifically the 'select' attribute which does exactly what you're proposing: 

http://www.w3.org/TR/timesheets/#smilTimesheetsNS-Elements-Item-Attributes-Select

And sorry, I know this isn't the usual place for these types of discussions, but jwatt started it :)

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

To avoid synchronisation slew we need to store timestamps in the events we dispatch and receive. I've added dependencies on the relevant bugs.

Comment 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

We'd really like to ship this in the next release since a lot of existing content seems to use it.

To do this properly however we need to rework the way timestamps are stored in events (bug 77992 and bug 238041). Without that we'll end up with synchronisation slew due to the amount of time it takes events to be processed.

I think we could split off the synchronisation slew / timestamp stuff into a separate bug and fix that after we ship. Then we could ship something that works but just isn't perfectly synced.

(I think that would address most use cases which seem to use an event to trigger an animation without being overly concerned if the animation begins a fraction of a second after the event. For the case where you have several animations synced off the same event, they could be rewritten to use syncbase timing to keep the animations in sync with one another and just have one master animation keyed off the event. It's not ideal but it might suffice for the time being.)

I have a very basic implementation of this working now but I expect it will be 2~3 weeks before it's review ready.

Comment 14

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

Not blocking, but wanted.

Comment 15

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 1: Refactor end time usage v1a

The patch reworks what we do with dynamic end times (i.e. end times from DOM calls and events). The spec says we should ignore such end times when we're not active. Previously we weren't doing that for DOM times so this patch fixes that.

Comment 16

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 2: Refactor how we clear instance times v1a

This reworks how we clear instance times. Previously we vary carefully only cleared the non-dynamic times upon rewinding the timeline however we then called UnsetBegin/EndSpec / SetBegin/EndSpec which cleared them all anyway. So upon a rewind we need to change the behaviour of these functions to preserve dynamic times.

I'm really unsure about this patch. We want to specify what behaviour to use for clearing instance times depending on if we're setting the attribute via content or because of a rewind. The neatest way is to just pass along an appropriate functor object but that would mean templatizing SetBeginSpec, SetEndSpec, UnsetBeginSpec, UnsetEndSpec, and SetBeginOrEndSpec and I think that might not be such good news for code size. So instead I've made it pass along a function pointer which then gets wrapped in a functor at the last moment.

Not sure if you've got any other ideas. One might be to pass a bool along specifying which behaviour to use but I think we decided those kind of bool params are a bit nasty because it's hard to tell what they do at the call site.

Comment 17

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 3: Registering to receive event notifications v1a

Basic registering to receive arbitrary events.

Comment 18

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 4: Refator use of nsIContent to use mozilla::dom::Element v1a

Just replacing nsIContent with mozilla::dom::Element in a few places in preparation for part 5 which needs an Element for feeding directly to nsReferencedElement in some situations.

Comment 19

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 5: Add support for default target v1a

Event-based times can specify just the event name in which case we should listen for the event on the animation target (this is true for SVG / SMIL Animation but not for SMIL in general).

Comment 20

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 6: More tests for event bubbling and user-defined events v1a

Comment 21

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 7: Support for repeat-based timing v1a

Comment 22

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

Comment on attachment 464308 [details] [diff] [review]
Part 3: Registering to receive event notifications v1a

>+NS_IMPL_ISUPPORTS1(nsSMILTimeValueSpec::EventListener, nsIDOMEventListener)
>+
>+NS_IMETHODIMP
>+nsSMILTimeValueSpec::EventListener::HandleEvent(nsIDOMEvent* aEvent)
>+{
>+  mSpec->HandleEvent(aEvent);
>+  return NS_OK;
>+}
Just verifying that we want to handle the event even if preventDefault was called?

>+nsSMILTimeValueSpec::RegisterEventListener(nsIContent* aTarget)
> {
>-  return GetTimedElementFromContent(mTimebase.get());
>+  NS_ABORT_IF_FALSE(mParams.mType == nsSMILTimeValueSpecParams::EVENT,
>+    "Attempting to register event-listener for non-event nsSMILTimeValueSpec");
>+  NS_ABORT_IF_FALSE(mParams.mEventSymbol,
>+    "Attempting to register event-listener but there is no event name");
>+
>+  // XXX Support default event targets
>+  if (!aTarget)
>+    return;
>+
>+  if (!mEventListener) {
>+    mEventListener = new EventListener(this);
>+  }
>+
>+  nsCOMPtr<nsIEventListenerManager> elm;
>+  nsCOMPtr<nsIDOMEventGroup> sysGroup;
>+  if (!GetEventListenerManager(aTarget, getter_AddRefs(elm),
>+        getter_AddRefs(sysGroup)))
>+    return;
>+  
>+  nsresult rv =
>+    elm->AddEventListenerByType(mEventListener,
>+                                nsDependentAtomString(mParams.mEventSymbol),
>+                                NS_EVENT_FLAG_BUBBLE |
>+                                NS_PRIV_EVENT_UNTRUSTED_PERMITTED,
>+                                sysGroup);
So we really want that content page can control this all by dispatching (untrusted) events?
Is that what other browsers do? If other browsers support that too, how do they handle
.stopPropagation() or .preventDefault()? Please add tests.


>+nsSMILTimeValueSpec::GetEventListenerManager(nsIContent* aTarget,
>+    nsIEventListenerManager** aEventListenerManager,
>+    nsIDOMEventGroup** aSystemGroup)
>+{
>+  if (!aTarget)
>+    return PR_FALSE;
>+
>+  NS_ABORT_IF_FALSE(aEventListenerManager && !*aEventListenerManager,
>+      "Bad out param for event listener");
>+  NS_ABORT_IF_FALSE(aSystemGroup && !*aSystemGroup,
>+      "Bad out param for system group");
>+
>+  nsCOMPtr<nsPIDOMEventTarget> target = aTarget->GetTargetForDOMEvent();
No need for this.

>+  if (!target)
>+    return PR_FALSE;
>+
>+  nsCOMPtr<nsIEventListenerManager> elm = target->GetListenerManager(PR_TRUE);
So just use aTarget here, and not target.
And no need for nsCOMPtr

>+  if (!elm)
>+    return PR_FALSE;
>+
>+  nsCOMPtr<nsIDOMEventGroup> sysGroup;
>+  target->GetSystemEventGroup(getter_AddRefs(sysGroup));
>+  if (!sysGroup)
>+    return PR_FALSE;
>+  
>+  *aEventListenerManager = elm;
>+  NS_ADDREF(*aEventListenerManager);
Could the method return nsIEventListenerManager*

>+  *aSystemGroup = sysGroup;
>+  NS_ADDREF(*aSystemGroup);
And this could be an out param.
And do something like *aSystemGroup = sysGroup.forget().get();


>-  class TimebaseElement : public nsReferencedElement {
>+  class ReferencedElement : public nsReferencedElement {
This looks strange. ReferencedElement and nsReferencedElement are way too similar.
And '{' should be in the next line

>   public:
>-    TimebaseElement(nsSMILTimeValueSpec* aOwner) : mSpec(aOwner) { }
>+    ReferencedElement(nsSMILTimeValueSpec* aOwner) : mSpec(aOwner) { }
> 
>   protected:
>     virtual void ElementChanged(Element* aFrom, Element* aTo) {
Not your code, but '{' should be in the next line.

>+  class EventListener : public nsIDOMEventListener
>+  {
>+  public:
>+    EventListener(nsSMILTimeValueSpec* aOwner) : mSpec(aOwner) { }
>+
>+    NS_DECL_ISUPPORTS
>+    NS_DECL_NSIDOMEVENTLISTENER
>+
>+  private:
>+    nsSMILTimeValueSpec* mSpec;
So nsSMILTimeValueSpec::~nsSMILTimeValueSpec()
guarantees that mSpec is never accessed after mSpec is deleted, right?

>+  };
>+  nsRefPtr<nsIDOMEventListener> mEventListener;
nsCOMPtr

>+function click(targetId) {
So if non-trusted events don't need to be supported, you could add
netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
here and the events would be trusted.

r- mainly because the handling of preventDefault() and stopPropagation() and
non-trusted events isn't clear.

Comment 23

[Robert Longson [:longsonr]]

You should flip the AnimationEventsAttribute to supported. I didn't see that in the patches, although I could have missed it.

Comment 24

[Robert Longson [:longsonr]]

That's a feature string btw.

Comment 25

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 3: Registering to receive event notifications v1b

(In reply to comment #22)
> >+NS_IMPL_ISUPPORTS1(nsSMILTimeValueSpec::EventListener, nsIDOMEventListener)
> >+
> >+NS_IMETHODIMP
> >+nsSMILTimeValueSpec::EventListener::HandleEvent(nsIDOMEvent* aEvent)
> >+{
> >+  mSpec->HandleEvent(aEvent);
> >+  return NS_OK;
> >+}
> Just verifying that we want to handle the event even if preventDefault was
> called?

Good question. I did some cross browser testing with Opera 10.60, Webkit nightly (r64893), Chrome 5.0.375.125, and Batik 1.7 and none of them pay any attention to calls to preventDefault. I've added a test case to make sure we don't either.

> >+  nsresult rv =
> >+    elm->AddEventListenerByType(mEventListener,
> >+                                nsDependentAtomString(mParams.mEventSymbol),
> >+                                NS_EVENT_FLAG_BUBBLE |
> >+                                NS_PRIV_EVENT_UNTRUSTED_PERMITTED,
> >+                                sysGroup);
> So we really want that content page can control this all by dispatching
> (untrusted) events?
> Is that what other browsers do? If other browsers support that too, how do they
> handle
> .stopPropagation() or .preventDefault()? Please add tests.

Other browsers do not support abitrary event names. While SMIL allows for this possibility (http://www.w3.org/TR/SMIL3/smil30.html#smil-timing-q130) SVG restricts the set of supported events to the following list: http://dev.w3.org/SVG/profiles/1.1F2/publish/interact.html#SVGEvents

However, Opera allows synthesised events.

Regarding the event names I'm not sure if we want to be as restrictive as SVG since it seems like it would be useful in the compound document situation to allow for keying off HTML events etc.

What do you think? I'm not sure what the implications are of supporting untrusted events.

If we were to support untrusted events then I don't think preventDefault is a problem as described above. I'm not sure I understand the problem with stopPropagation.

> >+  nsCOMPtr<nsPIDOMEventTarget> target = aTarget->GetTargetForDOMEvent();
> No need for this.
Fixed.

> >+  nsCOMPtr<nsIEventListenerManager> elm = target->GetListenerManager(PR_TRUE);
> So just use aTarget here, and not target.
> And no need for nsCOMPtr
Fixed.

> >+  *aEventListenerManager = elm;
> >+  NS_ADDREF(*aEventListenerManager);
> Could the method return nsIEventListenerManager*
Fixed.

> >+  *aSystemGroup = sysGroup;
> >+  NS_ADDREF(*aSystemGroup);
> And this could be an out param.
> And do something like *aSystemGroup = sysGroup.forget().get();
Fixed.

> >-  class TimebaseElement : public nsReferencedElement {
> >+  class ReferencedElement : public nsReferencedElement {
> This looks strange. ReferencedElement and nsReferencedElement are way too
> similar.
Renamed to TimeReferenceElement.

> And '{' should be in the next line
Fixed.

> >     virtual void ElementChanged(Element* aFrom, Element* aTo) {
> Not your code, but '{' should be in the next line.
Fixed.

> >+  class EventListener : public nsIDOMEventListener
> >+  {
> >+  public:
> >+    EventListener(nsSMILTimeValueSpec* aOwner) : mSpec(aOwner) { }
> >+
> >+    NS_DECL_ISUPPORTS
> >+    NS_DECL_NSIDOMEVENTLISTENER
> >+
> >+  private:
> >+    nsSMILTimeValueSpec* mSpec;
> So nsSMILTimeValueSpec::~nsSMILTimeValueSpec()
> guarantees that mSpec is never accessed after mSpec is deleted, right?

nsSMILTimeValueSpec::~nsSMILTimeValueSpec() calls UnregisterFromEventListener which should mean no one else is referring to the event listener. But to be sure I've made sure nsSMILTimeValueSpec's dtor explicitly clears out this pointer. Good catch.

> >+  };
> >+  nsRefPtr<nsIDOMEventListener> mEventListener;
> nsCOMPtr
Fixed although now we're referring to the subclass EventListener.

> >+function click(targetId) {
> So if non-trusted events don't need to be supported, you could add
> netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> here and the events would be trusted.
I'll wait to see what you have to say about the possibilities described above. In summary I think our options include:
a) Only support trusted events in the set outlined in SVG.
b) Support all trusted events.
c) Support all events, trusted or untrusted, in the set outlined by SVG.
d) Support arbitrary events, trusted or untrusted.

Currently we do (d). Opera seems to do something like (c) although I haven't tested this thoroughly. They may support other HTML events too. Or they may only support synthesised click events.

> r- mainly because the handling of preventDefault() and stopPropagation() and
> non-trusted events isn't clear.
That's fine. I really appreciate your help on this.

Comment 26

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 4: Refactor use of nsIContent to use mozilla::dom::Element v1b

Rebase off changes to part 3

Comment 27

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 5: Add support for default target v1b

Comment 28

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 6: Support for repeat-based timing v1b

Comment 29

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to comment #23)
> You should flip the AnimationEventsAttribute to supported. I didn't see that in
> the patches, although I could have missed it.

Oh, I didn't think about that. Actually I think that belongs with support for TimeEvents however. I've filed a separate bug, bug 586184 for that.

Comment 30

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

Comment on attachment 464692 [details] [diff] [review]
Part 3: Registering to receive event notifications v1b

>Bug 485157: SMIL event timing, part 3 event registration and timing
>
>diff --git a/content/smil/nsSMILTimeValueSpec.cpp b/content/smil/nsSMILTimeValueSpec.cpp
>--- a/content/smil/nsSMILTimeValueSpec.cpp
>+++ b/content/smil/nsSMILTimeValueSpec.cpp
>@@ -39,42 +39,63 @@
> #include "nsSMILInterval.h"
> #include "nsSMILTimeContainer.h"
> #include "nsSMILTimeValue.h"
> #include "nsSMILTimedElement.h"
> #include "nsSMILInstanceTime.h"
> #include "nsSMILParserUtils.h"
> #include "nsISMILAnimationElement.h"
> #include "nsContentUtils.h"
>+#include "nsIEventListenerManager.h"
>+#include "nsIDOMEventGroup.h"
>+#include "nsGUIEvent.h"
> #include "nsString.h"
> 
> //----------------------------------------------------------------------
>+// Nested class: EventListener
>+
>+NS_IMPL_ISUPPORTS1(nsSMILTimeValueSpec::EventListener, nsIDOMEventListener)
>+
>+NS_IMETHODIMP
>+nsSMILTimeValueSpec::EventListener::HandleEvent(nsIDOMEvent* aEvent)
>+{
>+  if (mSpec) {
>+    mSpec->HandleEvent(aEvent);
>+  }
>+  return NS_OK;
>+}
>+
>+//----------------------------------------------------------------------
> // Implementation
> 
> #ifdef _MSC_VER
> // Disable "warning C4355: 'this' : used in base member initializer list".
>-// We can ignore that warning because we know that mTimebase's constructor
>-// doesn't dereference the pointer passed to it.
>+// We can ignore that warning because we know that mReferencedElement's
>+// constructor doesn't dereference the pointer passed to it.
> #pragma warning(push)
> #pragma warning(disable:4355)
> #endif
> nsSMILTimeValueSpec::nsSMILTimeValueSpec(nsSMILTimedElement& aOwner,
>                                          PRBool aIsBegin)
>   : mOwner(&aOwner),
>     mIsBegin(aIsBegin),
>-    mTimebase(this)
>+    mReferencedElement(this)
> #ifdef _MSC_VER
> #pragma warning(pop)
> #endif
> {
> }
> 
> nsSMILTimeValueSpec::~nsSMILTimeValueSpec()
> {
>-  UnregisterFromTimebase(GetTimebaseElement());
>+  UnregisterFromReferencedElement(mReferencedElement.get());
>+  if (mEventListener) {
>+    mEventListener->Unlink();
>+    mEventListener = nsnull;
>+  }
> }
> 
> nsresult
> nsSMILTimeValueSpec::SetSpec(const nsAString& aStringSpec,
>                              nsIContent* aContextNode)
> {
>   nsSMILTimeValueSpecParams params;
>   nsresult rv =
>@@ -86,51 +107,58 @@ nsSMILTimeValueSpec::SetSpec(const nsASt
>   mParams = params;
> 
>   // According to SMIL 3.0:
>   //   The special value "indefinite" does not yield an instance time in the
>   //   begin list. It will, however yield a single instance with the value
>   //   "indefinite" in an end list. This value is not removed by a reset.
>   if (mParams.mType == nsSMILTimeValueSpecParams::OFFSET ||
>       (!mIsBegin && mParams.mType == nsSMILTimeValueSpecParams::INDEFINITE)) {
>-    nsRefPtr<nsSMILInstanceTime> instance =
>-      new nsSMILInstanceTime(mParams.mOffset);
>-    if (!instance)
>-      return NS_ERROR_OUT_OF_MEMORY;
>-    mOwner->AddInstanceTime(instance, mIsBegin);
>+    mOwner->AddInstanceTime(new nsSMILInstanceTime(mParams.mOffset), mIsBegin);
>   }
> 
>   ResolveReferences(aContextNode);
> 
>   return rv;
> }
> 
> void
> nsSMILTimeValueSpec::ResolveReferences(nsIContent* aContextNode)
> {
>-  if (mParams.mType != nsSMILTimeValueSpecParams::SYNCBASE)
>+  if (mParams.mType != nsSMILTimeValueSpecParams::SYNCBASE &&
>+      mParams.mType != nsSMILTimeValueSpecParams::EVENT)
>     return;
> 
>   NS_ABORT_IF_FALSE(aContextNode,
>       "null context node for resolving timing references against");
> 
>   // If we're not bound to the document yet, don't worry, we'll get called again
>   // when that happens
>   if (!aContextNode->IsInDoc())
>     return;
> 
>   // Hold ref to the old content so that it isn't destroyed in between resetting
>-  // the timebase and using the pointer to update the timebase.
>-  nsRefPtr<nsIContent> oldTimebaseContent = mTimebase.get();
>+  // the referenced element and using the pointer to update the referenced
>+  // element.
>+  nsRefPtr<nsIContent> oldReferencedContent = mReferencedElement.get();
> 
>-  NS_ABORT_IF_FALSE(mParams.mDependentElemID, "NULL syncbase element id");
>+  // XXX Support default event targets
>+  NS_ABORT_IF_FALSE(mParams.mDependentElemID, "NULL dependent element id");
>   nsString idStr;
>   mParams.mDependentElemID->ToString(idStr);
>-  mTimebase.ResetWithID(aContextNode, idStr);
>-  UpdateTimebase(oldTimebaseContent, mTimebase.get());
>+  mReferencedElement.ResetWithID(aContextNode, idStr);
>+  UpdateReferencedElement(oldReferencedContent, mReferencedElement.get());
>+}
>+
>+PRBool
>+nsSMILTimeValueSpec::IsEventBased() const
>+{
>+  return mParams.mType == nsSMILTimeValueSpecParams::EVENT ||
>+         mParams.mType == nsSMILTimeValueSpecParams::REPEAT ||
>+         mParams.mType == nsSMILTimeValueSpecParams::ACCESSKEY;
> }
> 
> void
> nsSMILTimeValueSpec::HandleNewInterval(nsSMILInterval& aInterval,
>                                        const nsSMILTimeContainer* aSrcContainer)
> {
>   const nsSMILInstanceTime& baseInstance = mParams.mSyncBegin
>     ? *aInterval.Begin() : *aInterval.End();
>@@ -141,19 +169,16 @@ nsSMILTimeValueSpec::HandleNewInterval(n
>   if (newTime.IsResolved()) {
>     newTime.SetMillis(newTime.GetMillis() + mParams.mOffset.GetMillis());
>   }
> 
>   // Create the instance time and register it with the interval
>   nsRefPtr<nsSMILInstanceTime> newInstance =
>     new nsSMILInstanceTime(newTime, nsSMILInstanceTime::SOURCE_SYNCBASE, this,
>                            &aInterval);
>-  if (!newInstance)
>-    return;
>-
>   mOwner->AddInstanceTime(newInstance, mIsBegin);
> }
> 
> void
> nsSMILTimeValueSpec::HandleChangedInstanceTime(
>     const nsSMILInstanceTime& aBaseTime,
>     const nsSMILTimeContainer* aSrcContainer,
>     nsSMILInstanceTime& aInstanceTimeToUpdate,
>@@ -191,70 +216,171 @@ PRBool
> nsSMILTimeValueSpec::DependsOnBegin() const
> {
>   return mParams.mSyncBegin;
> }
> 
> void
> nsSMILTimeValueSpec::Traverse(nsCycleCollectionTraversalCallback* aCallback)
> {
>-  mTimebase.Traverse(aCallback);
>+  mReferencedElement.Traverse(aCallback);
> }
> 
> void
> nsSMILTimeValueSpec::Unlink()
> {
>-  UnregisterFromTimebase(GetTimebaseElement());
>-  mTimebase.Unlink();
>+  UnregisterFromReferencedElement(mReferencedElement.get());
>+  mReferencedElement.Unlink();
> }
> 
> //----------------------------------------------------------------------
> // Implementation helpers
> 
> void
>-nsSMILTimeValueSpec::UpdateTimebase(nsIContent* aFrom, nsIContent* aTo)
>+nsSMILTimeValueSpec::UpdateReferencedElement(nsIContent* aFrom, nsIContent* aTo)
> {
>   if (aFrom == aTo)
>     return;
> 
>-  UnregisterFromTimebase(GetTimedElementFromContent(aFrom));
>+  UnregisterFromReferencedElement(aFrom);
> 
>-  nsSMILTimedElement* to = GetTimedElementFromContent(aTo);
>-  if (to) {
>-    to->AddDependent(*this);
>+  if (mParams.mType == nsSMILTimeValueSpecParams::SYNCBASE) {
>+    nsSMILTimedElement* to = GetTimedElementFromContent(aTo);
>+    if (to) {
>+      to->AddDependent(*this);
>+    }
>+  } else if (mParams.mType == nsSMILTimeValueSpecParams::EVENT) {
>+    RegisterEventListener(aTo);
>   }
> }
> 
> void
>-nsSMILTimeValueSpec::UnregisterFromTimebase(nsSMILTimedElement* aTimedElement)
>+nsSMILTimeValueSpec::UnregisterFromReferencedElement(nsIContent* aContent)
> {
>-  if (!aTimedElement)
>+  if (!aContent)
>     return;
> 
>-  aTimedElement->RemoveDependent(*this);
>-  mOwner->RemoveInstanceTimesForCreator(this, mIsBegin);
>+  if (mParams.mType == nsSMILTimeValueSpecParams::SYNCBASE) {
>+    nsSMILTimedElement* timedElement = GetTimedElementFromContent(aContent);
>+    if (timedElement) {
>+      timedElement->RemoveDependent(*this);
>+    }
>+    mOwner->RemoveInstanceTimesForCreator(this, mIsBegin);
>+  } else if (mParams.mType == nsSMILTimeValueSpecParams::EVENT) {
>+    UnregisterEventListener(aContent);
>+  }
> }
> 
> nsSMILTimedElement*
> nsSMILTimeValueSpec::GetTimedElementFromContent(nsIContent* aContent)
> {
>   if (!aContent)
>     return nsnull;
> 
>   nsCOMPtr<nsISMILAnimationElement> animElement = do_QueryInterface(aContent);
>   if (!animElement)
>     return nsnull;
> 
>   return &animElement->TimedElement();
> }
> 
>-nsSMILTimedElement*
>-nsSMILTimeValueSpec::GetTimebaseElement()
>+void
>+nsSMILTimeValueSpec::RegisterEventListener(nsIContent* aTarget)
> {
>-  return GetTimedElementFromContent(mTimebase.get());
>+  NS_ABORT_IF_FALSE(mParams.mType == nsSMILTimeValueSpecParams::EVENT,
>+    "Attempting to register event-listener for non-event nsSMILTimeValueSpec");
>+  NS_ABORT_IF_FALSE(mParams.mEventSymbol,
>+    "Attempting to register event-listener but there is no event name");
>+
>+  // XXX Support default event targets
>+  if (!aTarget)
>+    return;
>+
>+  if (!mEventListener) {
>+    mEventListener = new EventListener(this);
>+  }
>+
>+  nsCOMPtr<nsIDOMEventGroup> sysGroup;
>+  nsCOMPtr<nsIEventListenerManager> elm =
>+    GetEventListenerManager(aTarget, getter_AddRefs(sysGroup));
elm doesn't need to be nsCOMPtr.

>+nsIEventListenerManager*
>+nsSMILTimeValueSpec::GetEventListenerManager(nsIContent* aTarget,
>+    nsIDOMEventGroup** aSystemGroup)
Could you align the second parameter closer to under the first parameter.

>+  class EventListener : public nsIDOMEventListener
>+  {
>+  public:
>+    EventListener(nsSMILTimeValueSpec* aOwner) : mSpec(aOwner) { }
>+    void Unlink()
>+    {
>+      mSpec = nsnull;
>+    }
Could you call the method Disconnect().
Unlink sounds like it has something to do with cycle collection.

Comment 31

[Daniel Holbert [:dholbert]]

Comment on attachment 464306 [details] [diff] [review]
Part 1: Refactor end time usage v1a

>diff --git a/layout/reftests/svg/smil/restart/reset-6.svg b/layout/reftests/svg/smil/restart/reset-6.svg
>-  At t=2s we'll go to look for the next interval and construct one from 3s-4s.
>+  At t=2s we'll go to look for the next interval and construct one from 3s-3.5s.

This comment was actually "correct" as-is (at least, it matched the code), but it & the code disagreed with the comment above it.

That is to say: even after your update here, there's a mismatch between the header-comment and the code in this test.  The header comment says:
>  Then at t=1.5s we have the following DOM calls
>
>    anim.beginElementAt(1.5);
>    anim.endElementAt(2);

But the actual code says:
>      svg.setCurrentTime(1.5);
>      var anim = document.getElementById('anim');
>      anim.beginElementAt(1.5);
>      anim.endElementAt(2.5);

(Note the endElement calls don't match)

Can you resolve this so that so they all match?

r=dholbert with that.

Comment 32

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 1: Refactor end time usage v1b r=dholbert

(In reply to comment #31)
> Can you resolve this so that so they all match?
Fixed.

Comment 33

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 3: Registering to receive event notifications v1c r=smaug

(In reply to comment #30)
> >+  nsCOMPtr<nsIDOMEventGroup> sysGroup;
> >+  nsCOMPtr<nsIEventListenerManager> elm =
> >+    GetEventListenerManager(aTarget, getter_AddRefs(sysGroup));
> elm doesn't need to be nsCOMPtr.
Fixed.

> >+nsIEventListenerManager*
> >+nsSMILTimeValueSpec::GetEventListenerManager(nsIContent* aTarget,
> >+    nsIDOMEventGroup** aSystemGroup)
> Could you align the second parameter closer to under the first parameter.
Fixed.

> >+  class EventListener : public nsIDOMEventListener
> >+  {
> >+  public:
> >+    EventListener(nsSMILTimeValueSpec* aOwner) : mSpec(aOwner) { }
> >+    void Unlink()
> >+    {
> >+      mSpec = nsnull;
> >+    }
> Could you call the method Disconnect().
> Unlink sounds like it has something to do with cycle collection.
Fixed.

(In reply to comment #25)
> In summary I think our options include:
> a) Only support trusted events in the set outlined in SVG.
> b) Support all trusted events.
> c) Support all events, trusted or untrusted, in the set outlined by SVG.
> d) Support arbitrary events, trusted or untrusted.

Discussed this with sicking, roc, dholbert and jwatt on IRC and agreed to go with (d) as it doesn't seem harmful but rather seems useful and simpler.

Comment 34

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 4: Refactor use of nsIContent to use mozilla::dom::Element v1c r=dholbert

Rebasing off changes to underlying Part 3.

Comment 35

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

Comment on attachment 465516 [details] [diff] [review]
Part 3: Registering to receive event notifications v1c r=smaug

+  nsCOMPtr<nsIDOMEventGroup> sysGroup;
+  aTarget->GetSystemEventGroup(getter_AddRefs(sysGroup));
+  if (!sysGroup)
+    return nsnull;
+  
+  *aSystemGroup = sysGroup.forget().get();

Can't you just do
  aTarget->GetSystemEventGroup(aSystemGroup);
  if (!*aSystemGroup)
    return nsnull;

Comment 36

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

Comment on attachment 465519 [details] [diff] [review]
Part 4: Refactor use of nsIContent to use mozilla::dom::Element v1c r=dholbert

+using mozilla::dom::Element;

Just "using namespace mozilla::dom;"

In nsSMILTimeValueSpec's class declaration, use "typedef mozilla::dom::Element Element;" to avoid sprinkling mozilla::dom:: all over the place.

Same in nsSMILTimedElement.

We can't do that in nsSVGAnimationElement and nsISMILAnimationElement because, sadly, the typedef would conflict with the Element member function.

Comment 37

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

Comment on attachment 464694 [details] [diff] [review]
Part 5: Add support for default target v1b

nsSMILTimeValueSpec needs an Element typedef.

+    nsString idStr;
+    mParams.mDependentElemID->ToString(idStr);

nsAutoString

Comment 38

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

Comment on attachment 464695 [details] [diff] [review]
Part 6: Support for repeat-based timing v1b

This is all really great!

Comment 39

[Daniel Holbert [:dholbert]]

(In reply to comment #36)
> We can't do that in nsSVGAnimationElement and nsISMILAnimationElement because,
> sadly, the typedef would conflict with the Element member function.

Perhaps the Element member function (in patch 4) should be renamed to "AsElement()", so its name won't clash with the Element type and cause this problem?

Comment 40

[Daniel Holbert [:dholbert]]

Comment on attachment 465516 [details] [diff] [review]
Part 3: Registering to receive event notifications v1c r=smaug

>+nsSMILTimeValueSpec::RegisterEventListener(nsIContent* aTarget)
> {
[...]
>+  nsresult rv =
>+    elm->AddEventListenerByType(mEventListener,
[...]
>+  NS_ENSURE_SUCCESS(rv,);
}

NS_ENSURE_SUCCESS doesn't feel right here -- there's no need for an early-return (it's the last line of the method), and there's no special alternate-return-value. The ",)" also triggers a GCC warning about empty macro arguments being unsupported in ISO C++.

Replace with something like:
  NS_WARN_IF_FALSE(NS_SUCCEEDED(rv),
                   "AddEventListenerByType failed!);

Or just remove the rv-check altogether. (It looks like nsEventListenerManager::AddEventListenerByType never actually fails.)

Also: if you keep the rv-check, you should probably add a similar one after the call to RemoveEventListenerByType a few lines down, for consistency.

>+nsIEventListenerManager*
>+nsSMILTimeValueSpec::GetEventListenerManager(nsIContent* aTarget,
>+                                             nsIDOMEventGroup** aSystemGroup)
>+{
>+  if (!aTarget)
>+    return nsnull;

Looks like this method only has two callers, and they both already null-check aTarget before calling this.  So, there's no need to null-check again here at runtime -- maybe change this to something like:
  NS_ABORT_IF_FALSE(aTarget, "null target; can't get EventListenerManager");

>+nsSMILTimeValueSpec::HandleEvent(nsIDOMEvent* aEvent)
>+{
[...]
>+  nsRefPtr<nsSMILInstanceTime> newInstance =
>+    new nsSMILInstanceTime(newTime, nsSMILInstanceTime::SOURCE_EVENT);
>+  mOwner->AddInstanceTime(newInstance, mIsBegin);
> }

I don't think |newInstance| needs to be a nsRefPtr here -- that just creates a little extra addref/release busy-work for us to do.  Can you just make it a raw pointer (or if you prefer, remove it altogether and call "new nsSMILInstanceTime" within parens of AddInstanceTime()) ?

r=dholbert with the above.

Comment 41

[Daniel Holbert [:dholbert]]

Comment on attachment 464694 [details] [diff] [review]
Part 5: Add support for default target v1b

>+    nsString idStr;
>+    mParams.mDependentElemID->ToString(idStr);
>+    mReferencedElement.ResetWithID(aContextNode, idStr);

Create an on-the-fly nsAtomString instead of calling ToString -- i.e. replace those last three lines with:
  mReferencedElement.ResetWithID(aContextNode,
                                 nsAtomString(mParams.mDependentElemID);

(roc mentioned nsAutoString in comment 37 - I think he meant nsAtomString, unless nsAutoString has gotten more awesome)

>     TimeReferenceElement(nsSMILTimeValueSpec* aOwner) : mSpec(aOwner) { }
>+    void ResetWithElement(Element* aTo) {
>+      nsCOMPtr<Element> from = get();
>+      Unlink();
>+      ElementChanged(from, aTo);

s/nsCOMPtr/nsRefPtr/
(It doesn't look like you need any COM magic -- just refcounting.)

>+nsresult
>+nsSVGAnimationElement::AfterSetAttr(PRInt32 aNamespaceID, nsIAtom* aName,
>+                                    const nsAString* aValue, PRBool aNotify)
[...]
>+  // If we're not yet in a document, it's ok -- we'll update the target on
>+  // next BindToTree call.
>+  if (aValue && !IsInDoc())
>+    return rv;
>+
>+  if (aValue) {
>+    UpdateHrefTarget(this, *aValue);
>+  } else {
>+    mHrefTarget.Unlink();
>+    AnimationTargetChanged();
>   }
>-  return returnVal;
>+
>+  return rv;

The logic here confused me a bit (in particular the multiple-returns and multiple-null-checks on aValue).

Could you clean it up so that we only have to null-check aValue once, like the following (assuming that this is the behavior you'd like -- I think it's equivalent to what you've got):

  if (!aValue) {
     mHrefTarget.Unlink();
     AnimationTargetChanged();
   } else if (IsInDoc()) {
     UpdateHrefTarget(this, *aValue);
   } // else: we're not yet in a document -- we'll update
     // the target on next BindToTree call.

   return rv;

> nsSVGAnimationElement::UnsetAttr(PRInt32 aNamespaceID,
>                                  nsIAtom* aAttribute, PRBool aNotify)
>-  } else if (aNamespaceID == kNameSpaceID_XLink) {
>-    if (aAttribute == nsGkAtoms::href) {
>-      mHrefTarget.Unlink();
>-    }

Why is this going away -- don't we still need it, to handle ?  Does this get handled by the AfterSetAttr chunk above?  (From glancing at the nsGenericElement::AfterSetAttr documentation, it doesn't look like AfterSetAttr gets invoked at all for UnsetAttr calls.)

r=dholbert with the above addressed.

Comment 42

[Daniel Holbert [:dholbert]]

(In reply to comment #41)
> > nsSVGAnimationElement::UnsetAttr(PRInt32 aNamespaceID,
> Why is this going away -- don't we still need it, to handle ?  Does this get
> handled by the AfterSetAttr chunk above?  (From glancing at the
> nsGenericElement::AfterSetAttr documentation, it doesn't look like AfterSetAttr
> gets invoked at all for UnsetAttr calls.)

Ah -- disregard this chunk -- I just checked the implementation of  nsGenericElement::UnsetAttr(), and it indeed does invoke AfterSetAttr.  (The nsGenericElement::AfterSetAttr header-documentation is a bit misleading on that.)

Comment 43

[Daniel Holbert [:dholbert]]

Comment on attachment 464695 [details] [diff] [review]
Part 6: Support for repeat-based timing v1b

r=dholbert on part 6 -- looks great!

Comment 44

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 3: Registering to receive event notifications v1d r=smaug; dholbert, sr=roc

(In reply to comment #35)
> Can't you just do
>   aTarget->GetSystemEventGroup(aSystemGroup);
>   if (!*aSystemGroup)
>     return nsnull;
Fixed.

(In reply to comment #40)
> NS_ENSURE_SUCCESS doesn't feel right here...
... -- there's no need for an
> Or just remove the rv-check altogether. (It looks like
> nsEventListenerManager::AddEventListenerByType never actually fails.)
Fixed.

> Looks like this method only has two callers, and they both already null-check
> aTarget before calling this.  So, there's no need to null-check again here at
> runtime -- maybe change this to something like:
>   NS_ABORT_IF_FALSE(aTarget, "null target; can't get EventListenerManager");
Fixed.

> >+nsSMILTimeValueSpec::HandleEvent(nsIDOMEvent* aEvent)
> >+{
> [...]
> >+  nsRefPtr<nsSMILInstanceTime> newInstance =
> >+    new nsSMILInstanceTime(newTime, nsSMILInstanceTime::SOURCE_EVENT);
> >+  mOwner->AddInstanceTime(newInstance, mIsBegin);
> > }
> 
> I don't think |newInstance| needs to be a nsRefPtr here -- that just creates a
> little extra addref/release busy-work for us to do.  Can you just make it a raw
> pointer (or if you prefer, remove it altogether and call "new
> nsSMILInstanceTime" within parens of AddInstanceTime()) ?
I think we need the ref-counting here since AddInstanceTime doesn't necessarily take ownership of the object. For example, an end instance time added whilst the element is inactive is ignored. Without the nsRefPtr we'd leak the memory in that case.

Comment 45

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 4: Refactor use of nsIContent to use mozilla::dom::Element v1d r=dholbert, sr=roc

(In reply to comment #36)
> +using mozilla::dom::Element;
> 
> Just "using namespace mozilla::dom;"
Fixed.

> In nsSMILTimeValueSpec's class declaration, use "typedef mozilla::dom::Element
> Element;" to avoid sprinkling mozilla::dom:: all over the place.
Fixed.

> Same in nsSMILTimedElement.
Fixed.

> We can't do that in nsSVGAnimationElement and nsISMILAnimationElement because,
> sadly, the typedef would conflict with the Element member function.

(In reply to comment #39)
> (In reply to comment #36)
> > We can't do that in nsSVGAnimationElement and nsISMILAnimationElement because,
> > sadly, the typedef would conflict with the Element member function.
> 
> Perhaps the Element member function (in patch 4) should be renamed to
> "AsElement()", so its name won't clash with the Element type and cause this
> problem?
Fixed.

Comment 46

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 5: Add support for default target v1c, r=dholbert, sr=roc

(In reply to comment #37)
> Comment on attachment 464694 [details] [diff] [review]
> Part 5: Add support for default target v1b
> 
> nsSMILTimeValueSpec needs an Element typedef.
Fixed.

> +    nsString idStr;
> +    mParams.mDependentElemID->ToString(idStr);
> 
> nsAutoString
Addressed below.

(In reply to comment #41)
> Comment on attachment 464694 [details] [diff] [review]
> Part 5: Add support for default target v1b
> 
> >+    nsString idStr;
> >+    mParams.mDependentElemID->ToString(idStr);
> >+    mReferencedElement.ResetWithID(aContextNode, idStr);
> 
> Create an on-the-fly nsAtomString instead of calling ToString -- i.e. replace
> those last three lines with:
>   mReferencedElement.ResetWithID(aContextNode,
>                                  nsAtomString(mParams.mDependentElemID);
> 
> (roc mentioned nsAutoString in comment 37 - I think he meant nsAtomString,
> unless nsAutoString has gotten more awesome)
I've replaced this with nsDependentAtomString since that's what we're using elsewhere. Is that ok?

> >     TimeReferenceElement(nsSMILTimeValueSpec* aOwner) : mSpec(aOwner) { }
> >+    void ResetWithElement(Element* aTo) {
> >+      nsCOMPtr<Element> from = get();
> >+      Unlink();
> >+      ElementChanged(from, aTo);
> 
> s/nsCOMPtr/nsRefPtr/
> (It doesn't look like you need any COM magic -- just refcounting.)
Fixed.

> >+nsresult
> >+nsSVGAnimationElement::AfterSetAttr(PRInt32 aNamespaceID, nsIAtom* aName,
> >+                                    const nsAString* aValue, PRBool aNotify)
> [...]
> The logic here confused me a bit (in particular the multiple-returns and
> multiple-null-checks on aValue).
> 
> Could you clean it up so that we only have to null-check aValue once, like the
> following (assuming that this is the behavior you'd like -- I think it's
> equivalent to what you've got):
> 
>   if (!aValue) {
>      mHrefTarget.Unlink();
>      AnimationTargetChanged();
>    } else if (IsInDoc()) {
>      UpdateHrefTarget(this, *aValue);
>    } // else: we're not yet in a document -- we'll update
>      // the target on next BindToTree call.
> 
>    return rv;
Fixed.

Comment 47

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 6: Support for repeat-based timing v1c, r=dholbert; sr=roc

Rebased off changes to underlying patches

(In reply to comment #38)
> This is all really great!

(In reply to comment #43)
> r=dholbert on part 6 -- looks great!

Thanks guys!

Comment 48

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Roll-up patch for landing approval

Seeking approval to land. We'd really like to see event timing included when we first ship SMIL since a lot of content that uses SMIL seems to also use this feature.

Comment 49

[Daniel Holbert [:dholbert]]

(In reply to comment #44)
> I think we need the ref-counting here since AddInstanceTime doesn't necessarily
> take ownership of the object.

Ok, that makes sense - thanks for the explanation.

(In reply to comment #46)
> I've replaced this with nsDependentAtomString since that's what we're using
> elsewhere. Is that ok?

Great, even better!

Comment 50

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 5: Add support for default target v1d, r=dholbert, sr=roc

I think I introduced a new bug in the last revision to this patch.

Comment 51

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 6: Support for repeat-based timing v1d, r=dholbert; sr=roc

Rebasing off changes to underlying patch and fixing handling of accesskeys in the interim.

Comment 52

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 6: Support for repeat-based timing v1e, r=dholbert; sr=roc

Fix misguided attempt to fix previous patch.

Comment 53

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/d736c1a4ad2f
http://hg.mozilla.org/mozilla-central/rev/f23bab9fa177
http://hg.mozilla.org/mozilla-central/rev/26e34fda3938
http://hg.mozilla.org/mozilla-central/rev/31a2747a096b
http://hg.mozilla.org/mozilla-central/rev/fd27d7619098
http://hg.mozilla.org/mozilla-central/rev/32b5f913f72c

Comment 54

[Jonathan Watt [:jwatt]]

Awesome work Brian! Thanks for getting this done in time for FF4!!