537361 - SVG SMIL: Store intervals with state for restoring

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

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

Attachment: Proposed patch -- v1a

In order to implement backwards seeking we need to:

1. Store our past intervals -- not just the current interval and previous interval
2. Record sufficient state against each interval that we can correctly restore it -- in particular we need to record syncbase dependencies against *intervals* so that when we do a backwards seek we can restore the dependencies properly.

I'm filing this as a separate bug to backwards seeking (bug 492458) since I already have a patch for this and I'd like to get it landed so that it doesn't become bitrotted. Furthermore, it addresses some bugs with moving animations between time containers that can't be solved without storing past intervals (see included test case).

Note that attached patch applies on top of the patch for fixing reset problems (bug 534325, which in turn applies on top of the patches for syncbase timing -- bug 474743)

Comment 1

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

Attachment: Patch v1b

Rebasing off underlying changes to syncbase timing patches.

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 419728 [details] [diff] [review]
Patch v1b

Sorry for the long wait on this!  Patch looks great -- just a few minor suggestions -- mostly nits -- which I can fix in a followup patch.

> nsSMILInstanceTime::nsSMILInstanceTime(const nsSMILTimeValue& aTime,
[SNIP]
>+                                       nsSMILTimeValueSpec* aCreator,
>+                                       nsSMILInterval* aBaseInterval)
[SNIP]
>+    mCreator(aCreator),
>+    mBaseInterval(nsnull)

That init-list line for mBaseInterval probably needs a comment noting that mBaseInterval will get initted to aBaseInterval lower down, inside a call to SetBaseInterval(). Without a comment like that, this looks like a typo -- it looks (at first glance) like we're accidentally ignoring aBaseInterval.

>+nsSMILInstanceTime::SetBaseInterval(nsSMILInterval* aBaseInterval)
>+{
[SNIP]
>+    const nsSMILInstanceTime* dependentTime = mCreator->DependsOnBegin()
>+                                            ? aBaseInterval->Begin()
>+                                            : aBaseInterval->End();

Nit: I think general Mozilla style is for "?" and ":" operators to go at the end of lines, not the beginning (per rule "O" of JST Reviewer Simulacrum).

>+nsSMILInterval::NotifyChanged(const nsSMILTimeContainer* aContainer)
>+{
>+  for (PRInt32 i = mDependentTimes.Length() - 1; i >= 0; --i) {
>+    mDependentTimes[i]->HandleChangedInterval(aContainer,
>+        mBeginObjectChanged, mEndObjectChanged);

Indentation nit: |mBeginObjectChanged| should be indented up to under |aContainer| there (with |mEndObjectChanded| underneath, on its own line, so as not to break 80 chars).

>+  PRBool found = mDependentTimes.RemoveElementSorted(&aTime);
>+  NS_ABORT_IF_FALSE(found, "Couldn't find instance time to delete.");
>+  (void)found;

"(void)found" isn't pretty.  I think our preferred method for fixing opt-build "unused variable" warnings in situations like this is to just make the variable debug-only, like so:

#ifdef DEBUG
  PRBool found =
#endif
    mDependentTimes.RemoveElementSorted(&aTime);
  NS_ABORT_IF_FALSE(found, "Couldn't find instance time to delete.");

It's still not pretty, but it's a bit less arcane. :)

>+  // XXX Backwards seeking support
>+  void Unfreeze()
>+  {
>+    // XXX
>+    UnfreezeEnd();
>+  }
>+
>+  void UnfreezeEnd()
>+  {
>+    // XXX
>+  }

This chunk should refer to bug 492458 (supporting backwards seeking).

> nsSMILTimedElement::SampleFillValue()
[SNIP]
>+  const nsSMILInterval& prevInterval = *GetPreviousInterval();
>+  NS_ABORT_IF_FALSE(prevInterval.End()->Time().IsResolved() &&
>+      !prevInterval.End()->MayUpdate(),
>       "Attempting to sample fill value but the endpoint of the previous "
>       "interval is not resolved and frozen");

GetPreviousInterval is allowed to return null, so it seems a bit sketchy to just directly dereference its return value here.  If we expect its return val to be non-null in this particular place (which it looks like we do), we should explicitly assert that, for clarity. So, |prevInterval| should be a pointer, not a reference, and we should assert that it's non-null.

> nsSMILTimedElement::GetNextInterval(const nsSMILInterval* aPrevInterval,
>                                     const nsSMILInstanceTime* aFixedBeginTime,
>                                     nsSMILInterval& aResult)

I'm pretty sure this method GetNextInterval can be const.  We should make it const, for clarity & symmetry with the already-const method GetPreviousInterval().

>@@ -379,39 +422,46 @@ nsSMILTimedElement::DoSampleAt(nsSMILTim

This method (nsSMILTimedElement::DoSampleAt) probably needs an NS_ABORT_IF_FALSE about mCurrentInterval's nullness -- perhaps just inside the "do {" loop, to reevaluate the invariant on every loop iteration.  It looks we expect mCurrentInterval to be null if we're in STATE_STARTUP or STATE_POSTACTIVE, and we expect it to be non-null otherwise.

> nsSMILTimedElement::AddDependent(nsSMILTimeValueSpec& aDependent)
> {
[SNIP]
>+  for (PRUint32 i = 0; i < mOldIntervals.Length(); ++i) {
>+    aDependent.HandleNewInterval(*mOldIntervals[i], GetTimeContainer());
>+  }
>+  if (mCurrentInterval) {
>+    aDependent.HandleNewInterval(*mCurrentInterval, GetTimeContainer());
>   }

We don't need to call GetTimeContainer in each loop iteration there -- we should just call it once and store the result in a local var.

r=dholbert with the above addressed.  I can post a followup patch that makes these fixes, and I'll request r=birtles on it. :)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: followup v1: review fixes

Comment 4

[Jeff Walden [:Waldo]]

(In reply to comment #2)
> Nit: I think general Mozilla style is for "?" and ":" operators to go at the
> end of lines, not the beginning (per rule "O" of JST Reviewer Simulacrum).

I'm not so sure about this being a general Mozilla style.  |ack --js --cpp '^\s+\?' $topsrcdir| returns a fair number of results with it being the other way, which makes me think neither is really dominant -- individual reviewers just choose one or the other, outside of narrow exceptions like SpiderMonkey.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to comment #4)
> (In reply to comment #2)
> > Nit: I think general Mozilla style is for "?" and ":" operators to go at the
> > end of lines, not the beginning (per rule "O" of JST Reviewer Simulacrum).
> 
> I'm not so sure about this being a general Mozilla style.

All right, perhaps "general Mozilla style" was probably putting it too strongly. I was just going off of memory from the code I've worked with, plus an assumption that JST Reviewer Simulacrum is sane.

> individual reviewers
> just choose one or the other, outside of narrow exceptions like SpiderMonkey.

Ok -- this reviewer is choosing end-of-line rather than beginning-of-line operators. :)  (I don't want to start a style debate, but I think there are good reasons behind this particular Simulacrum recommendation. IMHO, "myVariable = myFlag ?" is clearer than "myVariable = myFlag" -- the former makes it clear that the assigned value is still coming, while the latter looks kind of like |myFlag| is the assigned value.)

Comment 6

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

Comment on attachment 427743 [details] [diff] [review]
followup v1: review fixes

Looks great, thanks Daniel.

Regarding the "?" and ":" placement I think elsewhere in the SMIL module we put them at the start of the line (I don't have the source in a suitably greppable state on this computer so I can't check but I'm pretty sure all of the code I've written does this). I personally find this easier to read too but I'm willing to be overruled on this.

Thanks again Daniel!

Comment 7

[Daniel Holbert [:dholbert]]

Landed main patch:   http://hg.mozilla.org/mozilla-central/rev/8bac3bfd59a3
and review followup: http://hg.mozilla.org/mozilla-central/rev/02c434f2fd4a