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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: wormsxulla, Assigned: birtles)

Tracking

(Depends on 1 bug, {testcase})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 -)

Details

(Whiteboard: [parity-Opera][parity-webkit], )

Attachments

(8 attachments, 15 obsolete attachments)

747 bytes, image/svg+xml
Details
16.64 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
10.95 KB, patch
birtles
: review+
Details | Diff | Splinter Review
37.75 KB, patch
birtles
: review+
birtles
: superreview+
Details | Diff | Splinter Review
23.65 KB, patch
birtles
: review+
birtles
: superreview+
Details | Diff | Splinter Review
100.89 KB, patch
Details | Diff | Splinter Review
28.03 KB, patch
birtles
: review+
birtles
: superreview+
Details | Diff | Splinter Review
14.23 KB, patch
birtles
: review+
birtles
: superreview+
Details | Diff | Splinter Review
Reporter

Description

10 years ago
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.

Updated

10 years ago
Depends on: 216462
(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
OS: Windows XP → All
Hardware: x86 → All
Summary: SMIL animations do not start on mouse click → SVG SMIL: Add support for begin/end: event-value

Comment 2

10 years ago
Posted image set visibility begin

Comment 3

10 years ago
parity opera & safari-webkit
Keywords: testcase
Whiteboard: [parity-Opera][parity-webkit]

Updated

10 years ago
Depends on: 474743
Assignee

Updated

10 years ago
Blocks: svg11tests
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.
Why not just add a DOM method beginAnimations(), so you can write
  onmouseover="event.target.beginAnimations()"
?
For the example in comment 4 you'd have to write
  <rect onmouseover="document.getElementById('animated').beginAnimations()"/>
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.
That's true. You're convincing me :-).

Comment 9

10 years ago
"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 :)
Duplicate of this bug: 571731
Assignee

Updated

9 years ago
Depends on: 572270
Duplicate of this bug: 574603
Assignee

Updated

9 years ago
Assignee: nobody → birtles
Assignee

Updated

9 years ago
Status: NEW → ASSIGNED
Assignee

Comment 12

9 years ago
To avoid synchronisation slew we need to store timestamps in the events we dispatch and receive. I've added dependencies on the relevant bugs.
Depends on: 77992, 238041
Assignee

Comment 13

9 years ago
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.
blocking2.0: --- → ?
Not blocking, but wanted.
blocking2.0: ? → -
Assignee

Comment 15

9 years ago
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.
Attachment #464306 - Flags: superreview?(dholbert)
Attachment #464306 - Flags: review?(roc)
Assignee

Updated

9 years ago
Attachment #464306 - Flags: superreview?(roc)
Attachment #464306 - Flags: superreview?(dholbert)
Attachment #464306 - Flags: review?(roc)
Attachment #464306 - Flags: review?(dholbert)
Assignee

Comment 16

9 years ago
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.
Attachment #464307 - Flags: superreview?(roc)
Attachment #464307 - Flags: review?(dholbert)
Assignee

Comment 17

9 years ago
Basic registering to receive arbitrary events.
Attachment #464308 - Flags: superreview?(roc)
Attachment #464308 - Flags: review?(dholbert)
Assignee

Updated

9 years ago
Attachment #464308 - Flags: review?(Olli.Pettay)
Attachment #464308 - Flags: review?
Assignee

Updated

9 years ago
Attachment #464308 - Flags: review?
Assignee

Comment 18

9 years ago
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.
Attachment #464312 - Flags: superreview?(roc)
Attachment #464312 - Flags: review?(dholbert)
Assignee

Comment 19

9 years ago
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).
Attachment #464314 - Flags: superreview?(roc)
Attachment #464314 - Flags: review?(dholbert)
Assignee

Updated

9 years ago
Attachment #464307 - Attachment description: Part 2: Refactor how we clear instance times → Part 2: Refactor how we clear instance times v1a
Assignee

Updated

9 years ago
Attachment #464308 - Attachment description: Part 3: Registering to receive event notifications → Part 3: Registering to receive event notifications v1a
Assignee

Updated

9 years ago
Attachment #464312 - Attachment description: Part 4: Refator use of nsIContent to use mozilla::dom::Element → Part 4: Refator use of nsIContent to use mozilla::dom::Element v1a
Assignee

Comment 20

9 years ago
Attachment #464315 - Flags: review?(dholbert)
Attachment #464315 - Flags: review?(Olli.Pettay)
Assignee

Updated

9 years ago
Attachment #464315 - Attachment description: Part 6: More tests for event bubbling and user-defined events → Part 6: More tests for event bubbling and user-defined events v1a
Assignee

Comment 21

9 years ago
Attachment #464317 - Flags: superreview?(roc)
Attachment #464317 - Flags: review?(dholbert)
Assignee

Updated

9 years ago
Depends on: 585872
Assignee

Updated

9 years ago
Depends on: 585874
Assignee

Updated

9 years ago
No longer depends on: 77992, 238041
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.
Attachment #464308 - Flags: review?(Olli.Pettay) → review-
You should flip the AnimationEventsAttribute to supported. I didn't see that in the patches, although I could have missed it.
That's a feature string btw.
Assignee

Comment 25

9 years ago
(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.
Attachment #464308 - Attachment is obsolete: true
Attachment #464315 - Attachment is obsolete: true
Attachment #464692 - Flags: superreview?(roc)
Attachment #464692 - Flags: review?(dholbert)
Attachment #464692 - Flags: review?(Olli.Pettay)
Attachment #464315 - Flags: review?(dholbert)
Attachment #464315 - Flags: review?(Olli.Pettay)
Attachment #464308 - Flags: superreview?(roc)
Attachment #464308 - Flags: review?(dholbert)
Assignee

Comment 26

9 years ago
Rebase off changes to part 3
Attachment #464312 - Attachment is obsolete: true
Attachment #464693 - Flags: superreview?(roc)
Attachment #464693 - Flags: review?(dholbert)
Attachment #464312 - Flags: superreview?(roc)
Attachment #464312 - Flags: review?(dholbert)
Assignee

Comment 27

9 years ago
Attachment #464314 - Attachment is obsolete: true
Attachment #464694 - Flags: superreview?(roc)
Attachment #464694 - Flags: review?(dholbert)
Attachment #464314 - Flags: superreview?(roc)
Attachment #464314 - Flags: review?(dholbert)
Assignee

Comment 28

9 years ago
Attachment #464317 - Attachment is obsolete: true
Attachment #464695 - Flags: superreview?(roc)
Attachment #464695 - Flags: review?(dholbert)
Attachment #464317 - Flags: superreview?(roc)
Attachment #464317 - Flags: review?(dholbert)
Assignee

Comment 29

9 years ago
(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 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.
Attachment #464692 - Flags: review?(Olli.Pettay) → review+
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.
Attachment #464306 - Flags: review?(dholbert) → review+
Attachment #464693 - Flags: review?(dholbert) → review+
Assignee

Comment 32

9 years ago
(In reply to comment #31)
> Can you resolve this so that so they all match?
Fixed.
Attachment #464306 - Attachment is obsolete: true
Attachment #465510 - Flags: superreview?(roc)
Attachment #465510 - Flags: review+
Attachment #464306 - Flags: superreview?(roc)
Assignee

Comment 33

9 years ago
(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.
Attachment #464692 - Attachment is obsolete: true
Attachment #465516 - Flags: superreview?(roc)
Attachment #465516 - Flags: review?(dholbert)
Attachment #464692 - Flags: superreview?(roc)
Attachment #464692 - Flags: review?(dholbert)
Assignee

Comment 34

9 years ago
Rebasing off changes to underlying Part 3.
Attachment #464693 - Attachment is obsolete: true
Attachment #465519 - Flags: superreview?(roc)
Attachment #465519 - Flags: review?(dholbert)
Attachment #464693 - Flags: superreview?(roc)
Assignee

Updated

9 years ago
Attachment #465519 - Attachment description: Part 4: Refactor use of nsIContent to use mozilla::dom::Element v1c → Part 4: Refactor use of nsIContent to use mozilla::dom::Element v1c r=dholbert
Attachment #465519 - Flags: review?(dholbert) → review+
Attachment #465510 - Flags: superreview?(roc) → superreview+
Attachment #464307 - Flags: superreview?(roc) → superreview+
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;
Attachment #465516 - Flags: superreview?(roc) → superreview+
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.
Attachment #465519 - Flags: superreview?(roc) → superreview+
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
Attachment #464694 - Flags: superreview?(roc) → superreview+
Comment on attachment 464695 [details] [diff] [review]
Part 6: Support for repeat-based timing v1b

This is all really great!
Attachment #464695 - Flags: superreview?(roc) → superreview+
(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?
Attachment #464307 - Flags: review?(dholbert) → review+
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.
Attachment #465516 - Flags: review?(dholbert) → review+
Flags: in-testsuite?
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.
Attachment #464694 - Flags: review?(dholbert) → review+
(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 on attachment 464695 [details] [diff] [review]
Part 6: Support for repeat-based timing v1b

r=dholbert on part 6 -- looks great!
Attachment #464695 - Flags: review?(dholbert) → review+
Assignee

Comment 44

9 years ago
(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.
Attachment #465516 - Attachment is obsolete: true
Attachment #466200 - Flags: superreview+
Attachment #466200 - Flags: review+
Assignee

Comment 45

9 years ago
(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.
Attachment #465519 - Attachment is obsolete: true
Attachment #466201 - Flags: superreview+
Attachment #466201 - Flags: review+
Assignee

Comment 46

9 years ago
(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.
Attachment #464694 - Attachment is obsolete: true
Attachment #466202 - Flags: superreview+
Attachment #466202 - Flags: review+
Assignee

Comment 47

9 years ago
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!
Attachment #464695 - Attachment is obsolete: true
Attachment #466203 - Flags: superreview+
Attachment #466203 - Flags: review+
Assignee

Comment 48

9 years ago
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.
Attachment #466204 - Flags: approval2.0?
(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!
Assignee

Comment 50

9 years ago
I think I introduced a new bug in the last revision to this patch.
Attachment #466202 - Attachment is obsolete: true
Attachment #466542 - Flags: superreview+
Attachment #466542 - Flags: review+
Assignee

Comment 51

9 years ago
Rebasing off changes to underlying patch and fixing handling of accesskeys in the interim.
Attachment #466203 - Attachment is obsolete: true
Attachment #466543 - Flags: superreview+
Attachment #466543 - Flags: review+
Assignee

Comment 52

9 years ago
Fix misguided attempt to fix previous patch.
Attachment #466543 - Attachment is obsolete: true
Attachment #466898 - Flags: superreview+
Attachment #466898 - Flags: review+
Assignee

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Awesome work Brian! Thanks for getting this done in time for FF4!!
You need to log in before you can comment on or make changes to this bug.