474743 - SVG SMIL: Implement syncbase timing

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

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

We need to implement syncbase timing.

See http://www.w3.org/TR/smil-animation/#Timing-EvaluationOfBeginEndTimeLists for a starting point.

Comment 1

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

Attachment: Initial WIP patch

Very very early start on the parsing for this (and other begin and end spec types).

I've also included the unit test framework I was using just to get the parsing working (and later when syncbase timing was more complete I would have transferred all this over into mochitests).

Like I said, the patch is very rudimentary and is surely bitrotted too but I'm attaching it for reference. If I ever get opportunity I'd love to implement this but whoever does might find this useful anyway.

Comment 2

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

Attachment: Implementation notes

For reference, here are my implementation notes from when I sat down and began to analyse and design for this feature.

Comment 3

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

Attachment: WIP patch v2

Just a work-in-progress patch.

Things still to do:
* Cyclic dependencies
* Animation sandwich priority based on time dependencies
* Cross time container dependencies, including paused time containers
* Further testing of ID look-ups (e.g. tree surgery tests)
* Filtering of old instance times

Comment 4

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

Attachment: WIP patch v3

Updated patch that adds:
* Cyclic dependency checking
* Animation sandwich prioritisation
* Various bug fixes including a memory leak

Still to do:
* Cross time container dependencies
* Further testing of ID look-ups (currently xml:id doesn't work)
* Filtering of old instance times

Comment 5

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

Attachment: WIP patch v4

Updated patch adds:
* Cross-time container dependencies
* Various bug fixes

Still to do:
* Further tree surgery tests
* Filtering of old instance times

Comment 6

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

Attachment: patch v1a

Patch for syncbase timing.

This version includes various bug fixes and tidy ups and should now build on Mac and Linux.

I've decided to defer filtering the instance time lists and tackle that when implementing backwards seeking because there's a bit of overlap there.

It's a pretty big patch so let me know if I ought to split it out into a few pieces for review. I already have some of the parsing refactoring seperated out in my repository.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 415070 [details] [diff] [review]
patch v1a

Haven't done an in-depth review yet, but here are some nits to start off with, from a cursory glance over the patch:

>+void
>+nsSMILAnimationController::DoMilestoneSamples()
[...]
>+  nsSMILMilestone nextMilestone(GetCurrentTime()+1, PR_TRUE);

Add spaces around the "+". (This applies to a few other places as well)

>+nsSMILAnimationController::DoMilestoneSamples()
[...]
>+      if (nextMilestone.mIsEnd)
>+        elem->TimedElement().SampleEndAt(containerTime);
>+      else
>+        elem->TimedElement().SampleAt(containerTime);
>+    }

I think Mozilla's preferred style is to always use braces, even for single-line blocks.
https://developer.mozilla.org/en/Mozilla_Coding_Style_Guide#Control_Structures
(The frequent exception to this is for early-returns at beginning of functions.)

Could you add braces here and in other places where it's not a simple early-return?

>+nsSMILAnimationController::GetNextMilestone(TimeContainerPtrKey* aKey,
>+                                            void* aData)
>+{
>+  NS_ENSURE_TRUE(aKey, PL_DHASH_NEXT);
>+  NS_ENSURE_TRUE(aKey->GetKey(), PL_DHASH_NEXT);
>+  NS_ENSURE_TRUE(aData, PL_DHASH_NEXT);

I guess these NS_ENSURE_TRUEs match what we already do elsewhere in this file, but I'm not sure we actually want them... That is, I don't think we actually expect to sometimes receive null values for those arguments.  Perhaps these should be NS_ABORT_IF_FALSE?  Anyway, these particular ones can probably stay for now since they match what we do in other hashtable-callbacks in this file, but we may want to reevaluate our argument-sanity-checking in all of these callback functions at some point.

>nsSMILAnimationController::AddAnimationToCompositorTable(
>   nsISMILAnimationElement* aElement, nsSMILCompositorTable* aCompositorTable)
> {
>+  NS_ENSURE_TRUE(aElement,);
>+  NS_ENSURE_TRUE(aCompositorTable,);

In contrast, these definitely don't need to be NS_ENSURE_TRUE -- they should be NS_ABORT_IF_FALSE.  We already null-check the first arg up one level (in SampleAnimation), and we allocate & null-check the second arg (currentCompositorTable) up a few levels in DoSample.  So I don't think we need to worry about actually checking for null values here, in non-debug code.

BTW, I personally am not a huge fan of NS_ENSURE_*, because (a) they add potentially-unnecessary checks to optimized builds when we use them to handle error-conditions that we shouldn't ever be in, (b) they hide early returns, and (c) they spam warnings when they fail (which may or may not be appropriate, depending on the situation.)

For the other added NS_ENSURE_* in this patch, I'd suggest weighing whether NS_ABORT_IF_FALSE would be better...

There are a few added NS_ASSERTIONs that should probably be NS_ABORT_IF_FALSE, too.

>+  PRBool operator==(const nsSMILMilestone &aOther) const

I think we prefer having the "&" next to the type, not next to the variable-name.

Same thing elsewhere -- searching the patch for " &a" yields 22 hits -- about half of those are places where this could be fixed (and the other half are false positives e.g. "mEnd = &aEnd;")

>+  public:
>+    TimebaseElement(nsSMILTimeValueSpec *aSpec) : mSpec(aSpec) {}
... and ...
>+static double
>+GetFloat(const char*& aStart, const char* aEnd, nsresult *aErrorCode)

Same thing here -- the "*" should hug the type. (I think two quoted chunks are the only instances of this with "*" in the patch.)

>+  PRBool operator<=(const nsSMILMilestone &aOther) const
>+  {
>+    return operator==(aOther) || operator<(aOther);
>+  }
>+  
>+  PRBool operator>=(const nsSMILMilestone &aOther) const
>+  {
>+    return !operator<(aOther);
>+  }

I'd avoid making explicit calls to operatorX() functions. The following would be clearer, IMHO:
    return *this == aOther || *this < aOther;
and
    return !(*this < aOther);

>+PRBool
>+nsSMILInstanceTime::IsDependent(const nsSMILInstanceTime& aOther,
>+                                PRUint32 aRecursionDepth) const
>+{
>+  NS_ABORT_IF_FALSE(aRecursionDepth < 1000,
>+      "We seem to have created a cycle between instance times.");

NS_ABORT_IF_FALSE messages shouldn't end with a period.  When they fail, we already will print ":" after the warning-message -- so this would end up as ".:", which looks silly.
(see for example the first assertion in bug 526536 comment 0)

This affects a bunch of NS_ABORT_IF_FALSE / NS_ASSERTION statements in this patch.

> nsSMILTimedElement::AddInstanceTimeFromCurrentTime(nsSMILTime aCurrentTime,
>     double aOffsetSeconds, PRBool aIsBegin)
[...]
>+  nsRefPtr<nsSMILInstanceTime> instanceTime = 
>+    new nsSMILInstanceTime(timeVal, nsnull, nsSMILInstanceTime::kClearOnReset |
>+                                            nsSMILInstanceTime::kFromDOM);
>   AddInstanceTime(instanceTime, aIsBegin);
> }

We need to check for OOM on this "new" call here, right?

>+nsSMILTimedElement::RemoveInstanceTime(nsSMILInstanceTime* aInstanceTime,
[...]
>+  PRBool found = 
>+    instanceList.RemoveElementSorted(aInstanceTime, InstanceTimeComparator());
>+  NS_ABORT_IF_FALSE(found, "Couldn't find instance time to delete.");
>+  (void)found;

Remove blank space at the end of line after "PRBool found =" here, and elsewhere in the patch.[1]

Also, I don't think the "(void)found;" there is useful.  If you want to assert that found is unused outside the NS_ABORT_IF_FALSE (and avoid "unused variable" build warnings in optimized builds), you could do something like:
  #ifdef DEBUG
    PRBool found =
  #endif
      instanceList.RemoveElementSorted(aInstanceTime, InstanceTimeComparator());
    NS_ABORT_IF_FALSE(found, "Couldn't find instance time to delete.");

> nsresult
> nsSMILTimedElement::GetNextInterval(const nsSMILInterval* aPrevInterval,
[...]
>+    if (tempEnd->Time() > zeroTime ||
>+       (tempBegin->Time() == zeroTime && tempEnd->Time() == zeroTime)) {
>+      aResult.Set(*tempBegin, *tempEnd);
>       return NS_OK;
>     } else if (mRestartMode == RESTART_NEVER) {
>       // tempEnd <= 0 so we're going to loop which effectively means restarting
>       return NS_ERROR_FAILURE;
>     } else {
>-      beginAfter = tempEnd;
>+      beginAfter = tempEnd->Time();
>     }
>   }

No need for the 2 else-after-returns there -- as long as you're touching this code, we should clean that up.

[1] The JST Reviewer simulacrum is good for catching end-of-line-whitespace and other trivial issues:
http://www.johnkeiser.com/cgi-bin/jst-review-cgi.pl
That says there are 37 whitespace issues in the patch -- some of those are probably tabs in the Makefile, which are fine, but you should fix the rest. :)

Comment 8

[Jeff Walden [:Waldo]]

(In reply to comment #7)
> I think Mozilla's preferred style is to always use braces, even for
> single-line blocks.
> https://developer.mozilla.org/en/Mozilla_Coding_Style_Guide#Control_Structures
> (The frequent exception to this is for early-returns at beginning of
> functions.)

That document lies or is at the very best wishful thinking.  Preferred style for single-line if varies throughout the codebase; follow whatever the prevailing style is where you're working.  Or do what you're already doing, because as far as I'm concerned that's the right style.  ;-)

Comment 9

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

You follow the prevailing style if there is one, otherwise you follow the style guidelines. What you prefer doesn't come into it.

Comment 10

[Jeff Walden [:Waldo]]

Sorry, poorly-phrased joke there -- indeed follow prevailing style.

Comment 11

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

Sorry, knee-jerk reaction to perceived style rebel!

Comment 12

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

+  // So we have the animations (specifically the timed elements) register the
+  // next significant moment (called a milestone) in their lifetime and then we
+  // step through the model at each of these moments and sample those animations
+  // registered for those times. This way events can fire in the correct order,
+  // dependencies can be resolved etc.

Some of this comment should be moved to (or repeated in) nsSMILMilestone.h

+  // Indicates if this instance time should be removed when the owning timed
+  // element is reset. True for events and DOM calls.
+  static const PRUint8 kClearOnReset = 1;
+
+  // Indicates that this instance time is referred to by an nsSMILTimeValueSpec
+  // and as such may be updated. Such instance time should not be filtered out
+  // by the nsSMILTimedElement even if they appear to be in the past as they may
+  // be updated to a future time. This flag is also set for instance times that
+  // represent the beginning of an active interval, or the end of a finished
+  // interval and which therefore should not be modified.
+  static const PRUint8 kMayUpdate    = 2;
+
+  // Indicates that this instance time was generated from the DOM as opposed to
+  // an nsSMILTimeValueSpec. When a 'begin' or 'end' attribute is set or reset
+  // we should clear all the instance times that have been generated by that
+  // attribute (and hence an nsSMILTimeValueSpec), but not those from the DOM.
+  static const PRUint8 kFromDOM      = 4;

In the old days not all our platforms could compile this, and possible some odd ones still can't. Use an enum instead.

I think it would be helpful to split up the patch. Would it make sense to split into
* parsing (but not handling any syncbase times)
* milestone infrastructure and DoMilestoneSamples
* rest of the syncbase implementation
* tests
?

Comment 13

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

To be clear, what can be troublesome is initializing constants at the declaration in a class.

Comment 14

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

Attachment: Patch A: Refactor nsSMILParserUtils to use newer iterators - v1

Comment 15

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

Attachment: Patch B: Milestone sampling behaviour - v1

Comment 16

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

Attachment: Patch C: Syncbase parsing - v1

Comment 17

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

Attachment: Patch D: Syncbase operation - v1

Comment 18

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

Attachment: Patch E: Test cases - v1

Comment 19

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

I've split the patch into several parts based on roc's suggestion.

These updated patches also address the first round of review feedback as described below.

(In reply to comment #7)
> Add spaces around the "+". (This applies to a few other places as well)
Done.

> I think Mozilla's preferred style is to always use braces, even for single-line
> blocks.
Done. Sorry, I became aware of this rule just after posting the patch.

> I guess these NS_ENSURE_TRUEs match what we already do elsewhere in this file,
> but I'm not sure we actually want them...
> ...

Agreed. I don't think I ever deliberately wrote NS_ENSURE_TRUE or NS_ASSERTION--just a copy and paste bug. I've replaced these with NS_ABORT_IF_FALSE for now.

> >nsSMILAnimationController::AddAnimationToCompositorTable(
> >   nsISMILAnimationElement* aElement, nsSMILCompositorTable* aCompositorTable)
> > {
> >+  NS_ENSURE_TRUE(aElement,);
> >+  NS_ENSURE_TRUE(aCompositorTable,);
> 
> In contrast, these definitely don't need to be NS_ENSURE_TRUE...
Removed. Not sure why I added them really.

> There are a few added NS_ASSERTIONs that should probably be NS_ABORT_IF_FALSE,
> too.
Done.

> I think we prefer having the "&" next to the type, not next to the
> variable-name.
Done.

> Same thing here -- the "*" should hug the type.
Done.

> I'd avoid making explicit calls to operatorX() functions.
Done.

> NS_ABORT_IF_FALSE messages shouldn't end with a period.
Fixed.

> We need to check for OOM on this "new" call here, right?
Yes, thanks for that. Fixed.

> Also, I don't think the "(void)found;" there is useful....
Good idea. Yes, I was just trying to eliminate the unused variable warning. Fixed.

> ...
> No need for the 2 else-after-returns there -- as long as you're touching this
> code, we should clean that up.
Done.

> [1] The JST Reviewer simulacrum is good for catching end-of-line-whitespace and
> other trivial issues...

Thanks for pointing me to that tool. I've used to it to fix the whitespace issues and a few other items.

(In reply to comment #12)
> +  // So we have the animations (specifically the timed elements) register the
> +  // next significant moment (called a milestone) in their lifetime and then
> we
> ...
> 
> Some of this comment should be moved to (or repeated in) nsSMILMilestone.h

I've now significantly elaborated on the comment in nsSMILMilestone.h.

> ...
> +  static const PRUint8 kFromDOM      = 4;
> 
> In the old days not all our platforms could compile this, and possible some odd
> ones still can't. Use an enum instead.

I've tidied this up a bit now to use enums instead. I've added a second enum to to hide the flags from callers of this class.

Comment 20

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

Attachment: Patch D: Syncbase operation - v2

Fix a bug introduced in the last update to the patch.

Comment 21

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

Attachment: Patch D: Syncbase operation - v3

Re-added line that was dropped when splitting the patch.

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 416675 [details] [diff] [review]
Patch A: Refactor nsSMILParserUtils to use newer iterators - v1

Patch A looks good -- just a few nits.

>+GetFloat(const char*& aStart, const char* aEnd, nsresult* aErrorCode)
[SNIP]
>   nsresult rv = NS_OK;
> 
>-  if (end == start || end > aIterEnd.get()) {
>+  if (floatEnd == aStart || floatEnd > aEnd) {
>     rv = NS_ERROR_FAILURE;
>   } else {
>-    aIter.advance(end - start);
>+    aStart = floatEnd;
>   }

I think it'd be cleaner to start out with |rv| unininitialized there, and then do |rv = NS_OK| inside the "else" clause (with "aStart = floatEnd").  That way we don't needlessly set |rv| twice, in the failure case.

>+nsSMILParserUtils::ParseClockComponent(const char*& aStart,
>+                                       const char* aEnd,
[SNIP]
>+  char const *begin = aStart;

s/char const */const char*/ in that last line
(for consistency of "const" placement and *-placement.)

>   // Check a number was found
[SNIP]
>   // Check it's not expressed in exponential form

s/Check/Check that/

>diff --git a/content/smil/nsSMILParserUtils.h b/content/smil/nsSMILParserUtils.h

It looks like you've converted *almost* all of the private static methods into static helper functions within the .cpp file.  Can we do the same for the two remaining private methods, ParseClockComponent & ParseMetricMultiplicand?

>   static const PRUint32 MSEC_PER_SEC;
>   static const PRUint32 MSEC_PER_MIN;
>   static const PRUint32 MSEC_PER_HOUR;

Same with these, actually -- I don't see any reason that we need to declare them in the .h file.  Can we just have them in the .cpp file? (This removes the need for the "private" chunk of the class declaration entirely.)

Comment 23

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

Attachment: Patch A: Refactor nsSMILParserUtils to use newer iterators - v2

Address review feedback.

I've also changed the file-scope statics to use an anonymous namespace instead as I believe file-scope statics are deprecated in C++ and I couldn't find anything in the C++ portability guidelines or style guide discouraging use of anonymous namespaces. Please let me know if there's any problem with this.

Also, I've had to update patch C (attachment forthcoming) to likewise remove the private methods and make them file-scope functions instead. I'm not sure if there's much advantage to doing this. Originally I did this for the inline methods as it seems to make sense in that case (as I think there can be some variation between compilers depending on whether the 'inline' keyword appears in the declaration or definition). I'm not sure what the benefit is for non-inline helper functions except perhaps to reduce recompile times when modifying these methods' signatures.

Comment 24

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

Attachment: Patch C: Syncbase parsing - v2

* Rebase off changes to Patch A.
* Follow approach suggested in review feedback for Patch A v1 to remove private static helper methods and make them file-scope functions instead.

Comment 25

[Daniel Holbert [:dholbert]]

(In reply to comment #23)
> I'm not sure what the benefit is for
> non-inline helper functions except perhaps to reduce recompile times when
> modifying these methods' signatures.

Yeah -- that, abstracting away implementation details, & marginally improving readability of the header file, I guess.  It's not a major point -- I was just suggesting it for that particular class, since the patch was already shifting all the other private helper functions out of the header file.

Thanks for the updated patches! More review comments coming soon.

Comment 26

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

(In reply to comment #23)
> I've also changed the file-scope statics to use an anonymous namespace instead
> as I believe file-scope statics are deprecated in C++ and I couldn't find
> anything in the C++ portability guidelines or style guide discouraging use of
> anonymous namespaces. Please let me know if there's any problem with this.

It's fine.

> I'm not sure what the benefit is for
> non-inline helper functions except perhaps to reduce recompile times when
> modifying these methods' signatures.

It's easier for the compiler to do certain optimizations when it knows that no other translation unit can use the function. For example, a static function that's not called can be completely removed, and a static function that's only called from one place can be inlined at that place no matter what the size of the function (you know it's not going to increase overall code size).

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 416676 [details] [diff] [review]
Patch B: Milestone sampling behaviour - v1

Review comments on "Patch B" (milestone sampling patch):

>diff --git a/content/smil/Makefile.in b/content/smil/Makefile.in
> EXPORTS		+= \
[SNIP]
>+	  nsSMILMilestone.h \

I don't think we need to export nsSMILMilestone.h -- the only files that #include it are within the /smil/ directory.

>diff --git a/content/smil/nsSMILTimeValue.h b/content/smil/nsSMILTimeValue.h
>+  nsSMILTime GetMillis() const
>+  {
[SNIP]
>+    if (mState != STATE_RESOLVED)
>+        return kUnresolvedMillis;
>+
>+    return mMilliseconds;
>+  }

"return kUnresolvedMillis" has too much indentation there. However, rather than just fixing that, I'd prefer we just combine the return statements into one line, like this:
  return (mState == STATE_RESOLVED ? mMilliseconds : kUnresolvedMillis);

>diff --git a/content/smil/nsSMILTimedElement.h b/content/smil/nsSMILTimedElement.h
>   /*
>-   * Adds a new begin instance time at the current document time (as defined by
>-   * aContainer) plus or minus the specified offset.
>+   * Adds a new begin instance time at the current document time plus or minus
>+   * the specified offset.
[SNIP]
>+  nsresult BeginElementAt(double aOffsetSeconds);

Unless I misunderstand, I think this comment for BeginElementAt needs s/document/container/.
(same applies to the comment above EndElementAt)

For example, suppose our time container is paused, but its parent isn't.  If we receive a call to elem.BeginElementAt(1) for some |elem| in the paused time container, we'll queue up a "Begin" instance for 1 second after the container unpauses -- *not* for 1 second after the current document time... right?

> nsSMILTimedElement::nsSMILTimedElement()
> :
>+  mAnimationElement(nsnull),
>   mBeginSpecs(),
>   mEndSpecs(),
>   mFillMode(FILL_REMOVE),
>   mRestartMode(RESTART_ALWAYS),
>   mBeginSpecSet(PR_FALSE),
>   mEndHasEventConditions(PR_FALSE),
>   mClient(nsnull),
>-  mCurrentInterval(),
>   mElementState(STATE_STARTUP)

So you're removing the explicit call to mCurrentInterval's no-argument constructor -- can we do the same here for mBeginSpecs() / mEndSpecs()?  (It seems inconsistent to explicitly call the no-argument-constructor for some member variables, while omitting it for others... particularly if it's going to implicitly get called regardless).

>+void
>+nsSMILTimedElement::SampleEndAt(nsSMILTime aContainerTime)
>+{
[SNIP]
>+  // For now we just check that we're actually in an interval. We could also
[SNIP]
>+  if (mElementState != STATE_ACTIVE && mElementState != STATE_STARTUP)
>+    return;
>+
>+  DoSampleAt(aContainerTime, PR_TRUE); // End sample
>+}

The early return seems unnecessary there, for a one-liner-function.  I'd change this to:
  if (mElementState == STATE_ACTIVE || mElementState == STATE_STARTUP) {
    DoSampleAt(aContainerTime, PR_TRUE); // End sample
  }

>+nsSMILTimedElement::DoSampleAt(nsSMILTime aContainerTime, PRBool aEndOnly)
>-  } while (stateChanged);
>+  } while (stateChanged && (!aEndOnly || (mElementState != STATE_WAITING &&
>+                                          mElementState != STATE_POSTACTIVE)));
>+

I don't understand the change to the "while" condition there... Can you add a comment explaining why we've got those particular conditions on continuing to loop?

>     prevIntervalWasZeroDur
>-      = (aPrevInterval->mEnd.CompareTo(aPrevInterval->mBegin) == 0);
>+      = (aPrevInterval->mEnd == aPrevInterval->mBegin);

Put the "=" on the first line there, after "prevIntervalWasZeroDur".

>+nsSMILTimedElement::GetRepeatDuration() const
[SNIP]
>+      nsSMILTime activeDur =
>+        nsSMILTime(mRepeatCount * double(mSimpleDur.GetMillis()));
[SNIP]
>+    nsSMILTime activeDur =
>+      nsSMILTime(mRepeatCount * double(mSimpleDur.GetMillis()));

No need for using assignment in the places quoted above -- do this instead, in both spots:
  nsSMILTime activeDur(mRepeatCount * double(mSimpleDur.GetMillis()));

> void
> nsSMILTimedElement::UpdateCurrentInterval()
> {
>+  // We adopting the convention of not resolving intervals until the first
>+  // sample.

s/We/We're/  :)

>+nsSMILTimedElement::GetNextMilestone(nsSMILMilestone& aNextMilestone) const
>+{
[SNIP]
>+  case STATE_ACTIVE:
[SNIP]
>+      // Check for an early end
>+      if (mRestartMode == RESTART_ALWAYS) {
>+        PRInt32 position = 0;
>+        nsSMILTimeValue nextBegin;
>+        PRBool found = GetNextGreater(mBeginInstances,
>+            mCurrentInterval.mBegin, position, nextBegin);
>+        if (found &&
>+            nextBegin > mCurrentInterval.mBegin &&
>+            nextBegin < mCurrentInterval.mEnd) {
>+          aNextMilestone.mIsEnd = PR_TRUE;
>+          aNextMilestone.mTime = nextBegin.GetMillis();
>+          return PR_TRUE;

This block of code looks almost identical to the existing "CheckForEarlyEnd" method.  Can we tweak that method to make it more generally-useable, and make a call to it here?  (i.e. we could modify CheckForEarlyEnd() to return its "nextBegin" local variable by reference, instead of copying it to mCurrentInterval.mBegin.  Then we could react to that returned value both here and in the other place where CheckForEarlyEnd is already called.)

>+nsSMILAnimationController::DoMilestoneSamples()
[SNIP]
>+  // we initialise the next milestone to the moment after the current sample
[SNIP]
>+  nsSMILMilestone nextMilestone(GetCurrentTime() + 1, PR_TRUE);

The phrase "moment after" in the comment here is vague -- it should probably say "the moment 1ms after", or "the moment after the current sample (1 ms later)", or something to that effect.  I know we're shooting for "an infinitessmial amount of time after", but it's good to document how precise we actually are, and indicate what the "+ 1" in the code actually means.

>+    for (PRUint32 i = 0; i < length; ++i) {
>+      nsISMILAnimationElement* elem = params.mElements[i].get();
>+      NS_ABORT_IF_FALSE(elem, "NULL animation element in list");
>+      if (!elem)
>+        continue;
>+      nsSMILTimeContainer* container = elem->GetTimeContainer();

If we're at an "NS_ABORT_IF_FALSE" level-of-confidence that elem will be non-null, then we shouldn't bother with an "if (!elem)" check here.  In the unanticipated case that |elem| ends up being null, I think we can just let non-debug builds dereference null and crash on "elem->GetTimeContainer()" in the next line.

>+  nsSMILMilestone nextMilestone(GetCurrentTime() + 1, PR_TRUE);
>+  mChildContainerTable.EnumerateEntries(GetNextMilestone, &nextMilestone);
[SNIP]
>+  while (nextMilestone.mTime <= GetCurrentTime()) {
[SNIP]
>+    // As before, initialise to the moment after the current sample
>+    nextMilestone.mTime = GetCurrentTime() + 1;
>+    nextMilestone.mIsEnd = PR_TRUE;
>+    mChildContainerTable.EnumerateEntries(GetNextMilestone, &nextMilestone);
>+  }

I'd prefer to avoid the duplicated code here -- setting up nextMilestone & passing it through our child container table, in the exact same way, twice.  Can we clean that up, maybe like this:
 while(PR_TRUE) {
   nsSMILMilestone nextMilestone(GetCurrentTime() + 1, PR_TRUE);
   mChildContainerTable.EnumerateEntries(GetNextMilestone, &nextMilestone);
   if (nextMilestone.mTime > GetCurrentTime() {
     break;
   }
   // [the rest of the while loop goes here]
 }

>+  while (nextMilestone.mTime <= GetCurrentTime()) {
>+    for (PRUint32 i = 0; i < length; ++i) {
[SNIP]
>+      sampleTime = PR_MAX(nextMilestone.mTime, sampleTime);

Why is the sampleTime-updating-line inside off the "for" loop? AFAICT, nextMilestone's value will only change at the end of each "while" loop, so there's no way that it'd change from one iteration of the "for" loop to the next.

In fact, why do we keep a separate |sampleTime| at all?  AFAICT, it's just a (frequently-updated) copy of nextMilestone.mTime.  Can we just use "nextMilestone.mTime" in place of sampleTime?

>+nsSMILAnimationController::GetNextMilestone(TimeContainerPtrKey* aKey,
[SNIP]
>+  PRBool isMilestone = container->GetNextMilestoneInParentTime(thisMilestone);
>+  if (isMilestone && thisMilestone < *nextMilestone) {
>+    *nextMilestone = thisMilestone;
>+  }

I think the variable "isMilestone" here needs a better name.  It indicates whether GetNextMilestoneInParentTime() succeeded populating |thisMilestone|, so maybe we could call it something like "success", "gotMilestone", "didGetMilestone", or "didGetThisMilestone"? :)

> nsSMILAnimationController::SampleTimeContainer(TimeContainerPtrKey* aKey,
[SNIP]
>-  if (container->NeedsSample() || !params->mSkipUnchangedContainers) {
>+  if (!container->IsPausedByType(nsSMILTimeContainer::PAUSE_BEGIN) &&
>+    (container->NeedsSample() || !params->mSkipUnchangedContainers)) {

The last line there needs 2 spaces more indentation.

Comment 28

[Daniel Holbert [:dholbert]]

(In reply to comment #27)
> >+    nsSMILTime activeDur =
> >+      nsSMILTime(mRepeatCount * double(mSimpleDur.GetMillis()));
> 
> No need for using assignment in the places quoted above -- do this instead, in
> both spots:
>   nsSMILTime activeDur(mRepeatCount * double(mSimpleDur.GetMillis()));

Actually, I'm not sure why we're using "double(" there in the first place -- does that help us avoid overflow somehow?

Comment 29

[Daniel Holbert [:dholbert]]

One other thing I don't completely understand in Patch B -- there are two comments that appear somewhat contradictory to me.
* nsSMILTimeContainer.h says that mMilestoneEntries is "Reset on every full sample".
* nsSMILTimeContainer::AddMilestone (which populates mMilestoneEntries) has a comment that suggests we may do extra samples if "attributes are changed on the timed element in between samples".

How do those two comments mesh together?  i.e. if we're resetting mMilestoneEntries on every sample, then how would tweaks to attributes between samples cause additional samples in AddMilestone?  (given that nsSMILAnimationController only pulls in milestones up to the current sample-time anyway, AFAICT)

(I think that's it for my review comments on Patch B. :))

Comment 30

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

Patch B:

Could AddMilestone become a performance problem? It looks like adding N milestones could be O(N^2)? Do we need to use a priority queue here?

+  // The disadvantage of deferring resolving the interval is that DOM calls to
+  // to getStartTime will throw an INVALID_STATE_ERR exception until the
+  // document timeline begins since the start time has not yet been resolved.

Why is that OK?

Comment 31

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

Patch C:

+      if (escape) {
+        memmove(cur-1, cur, (end - cur) * sizeof(PRUnichar));

This isn't great, it means parsing a long sequence of escapes is O(N^2). That is avoidable.

Comment 32

[Daniel Holbert [:dholbert]]

Comment on attachment 417402 [details] [diff] [review]
Patch C: Syncbase parsing - v2

Comments on patch C:
====================
>+inline void
>+TrimWsp(const PRUnichar*& aStart, const PRUnichar*& aEnd)
>+{
>+  SkipWsp(aStart, aEnd);
>+  while (aEnd != aStart && IsSpace(*(aEnd-1)))
>+    --aEnd;
>+}
>+

Need spaces around the "-".

Also, the naming of "TrimWsp" vs. "SkipWsp" is a bit confusing, and it doesn't really reflect the difference (& similarity) in functionality between these two functions.  Could we perhaps rename SkipWsp to SkipBeginWsp, and this new one to SkipBeginEndWsp?

(It might also be worth refactoring out the second half of this function into a "SkipEndWsp" helper, for symmetry.  Then SkipBeginEndWsp would just call both helpers.  Not really necessary, though, given that nothing else calls SkipEndWsp at this point.)

>+  PRUint32 value = strtol(str, &rest, 10);
>+
>+  if (rest == str)
>+    return 0;
>+
>+  aResult = value;
>+  return rest - str;

10 is a bit of a magic number here -- could we #define it as DECIMAL_BASE or something?

Also: strtol returns signed integers, not unsigned ones, so we should probably be checking for negatives here.  So:
 - |value| should be declared as a PRInt32  (it should be signed)
 - the early-return-case should be "if (rest == str || value < 0)"
 - Use a static-cast when setting aResult to the now-signed |value|:
      aResult = static_cast<PRUint32>(value);
  (to explicitly cast off |value|'s signed-ness -- otherwise, the implicit conversion would trigger a compiler warning)

Also: s/char */char* /
(Only mentioning because I've already got you re-examining this block of code anyway :))

>+static PRBool
>+GetUnsignedIntAndBracket(const nsAString& aStr, PRUint32& aResult)
>+{

"Bracket" in this function name is ambiguous, particularly since it implies [] in US English.  Can we name this function "GetUnsignedIntAndEndParen" to avoid ambiguity about what type(s) and orientation(s) of bracket it accepts?

>+  // Make sure the string is only digit+')'
>+  if (intLen == 0 || *(start + intLen) != ')' || start + intLen + 1 != end)
>+    return PR_FALSE;

You need to reverse the order of the last two conditions in the "if" check there.  Otherwise, we won't yet know whether our string is long enough to contain that final ')' character at start+intlen, and we shouldn't be reading the memory there.

>+  NS_ABORT_IF_FALSE(StringBeginsWith(aSpec, NS_LITERAL_STRING("accesskey(")),
>+      "Calling ParseAccessKey on non-accesskey-type spec");
[SNIP]
>+  const PRUnichar* start = aSpec.BeginReading() + 10;
[SNIP]
>+    } else if (StringBeginsWith(rawToken2, NS_LITERAL_STRING("repeat("))) {
>+      result.mType = nsSMILTimeValueSpecParams::REPEAT;
>+      if (!GetUnsignedIntAndBracket(Substring(tokenStart + 7, tokenEnd),

I'd prefer we don't have the magic numbers "10" and "7" here.  I guess those are the lengths of the strings "accesskey(" and "repeat(". Could we use static constants declared at the top of the file to avoid that?

e.g. like so:
  static const char* ACCESSKEY_PREFIX = NS_LITERAL_STRING("accesskey(");
  static const char* REPEAT_PREFIX = NS_LITERAL_STRING("repeat(");

Then we can replace the NS_LITERAL STRING()s in the code with those constants, and the magic numbers with e.g. "ACCESSKEY_PREFIX.Length();"

>+ParseAccessKey(const nsAString& aSpec, nsSMILTimeValueSpecParams& aResult)
[SNIP]
>+  if (end - start < 2)
>+    return NS_ERROR_FAILURE;

This needs a comment explaining why "< 2" is relevant here.
e.g. // Expecting at least the accesskey char and ')'

>+  if (NS_IS_HIGH_SURROGATE(c)) {
>+    if (end - start < 2)
>+      return NS_ERROR_FAILURE;

Here too. (note that "start" has been incremented since the last < 2 check)

>+        memmove(cur-1, cur, (end - cur) * sizeof(PRUnichar));

Needs spaces around the "-". (though maybe this line is going away anyway, per comment 31)

One way we could avoid the O(n^2) issue that roc brought up in comment 31 would be to build a new string, character-by-character, rather than modifying the existing string. (And then copy the new string over the old string, at the end of the function).  Not sure if that's the best way, but it's something to start with.

>+  const PRUnichar* tokenEnd = GetTokenEnd(aSpec, PR_TRUE);
[SNIP]
>+  // Parse the second token if there is one
>+  if (*tokenEnd == '.') {

So if our token occupies the full string (aSpec), then I think "*tokenEnd" will read past the end of the string.
(In a c-string, this would still be basically okay, because *tokenEnd would be the null terminator.  But https://developer.mozilla.org/en/XPCOM_string_guide says that nsAStrings aren't necessarily null-terminated.)

Basically, I think you need to check whether tokenEnd == aSpec.EndReading() before dereferencing tokenEnd here.

>+  nsresult rv = ParseOptionalOffset(Substring(tokenEnd, specEnd), result);
>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  aResult = result;
>+
>+  return NS_OK;

Those last 6 lines can be shortened to:
  if (NS_SUCCEEDED(rv)) {
    aResult = result;
  }
  return rv;

Comment 33

[Daniel Holbert [:dholbert]]

(In reply to comment #32)
> Also: s/char */char* /
> (Only mentioning because I've already got you re-examining this block of code
> anyway :))

Sorry -- that comment refers to code from just before the block that I quoted.  Here's the bit that was missing:
>+GetUnsignedInt(const nsAString& aStr, PRUint32& aResult)
>+{
>+  const char *str = cstr.get();
>+
>+  char *rest;

Comment 34

[Daniel Holbert [:dholbert]]

Comment on attachment 417401 [details] [diff] [review]
Patch A: Refactor nsSMILParserUtils to use newer iterators - v2

The newer version of "Patch A" actually needs one more change before landing:
>+}; // end anonymous namespace block

This doesn't compile -- it gives this compile error:
  nsSMILParserUtils.cpp:200: error: extra ‘;’

Removing that ";" seems to fix the error.

Comment 35

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

Attachment: Patch A: Refactor nsSMILParserUtils to use newer iterators - v3 (r=dholbert, sr=roc)

(In reply to comment #34)
> (From update of attachment 417401 [details] [diff] [review])
> The newer version of "Patch A" actually needs one more change before landing:
> >+}; // end anonymous namespace block
> 
> This doesn't compile -- it gives this compile error:
>   nsSMILParserUtils.cpp:200: error: extra ‘;’
Fixed. Sorry about that, works in MSVC.

Carrying forward r=dholbert, sr=roc

Comment 36

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

Attachment: Patch B: Milestone sampling behaviour - v2

(In reply to comment #27)
> I don't think we need to export nsSMILMilestone.h -- the only files that
> #include it are within the /smil/ directory.

Yeah, but it's included in nsSMILTimeContainer.h which is included outside /smil/ so it's needed (I've tried).

> I'd prefer we just combine the return statements into one
> line, like this:
>   return (mState == STATE_RESOLVED ? mMilliseconds : kUnresolvedMillis);

Yes, that's good. Just checking because I've often been pulled up for unnecessary parentheses, are the parentheses needed here? I've left them out. Let me know if I ought to include them.

> Unless I misunderstand, I think this comment for BeginElementAt needs
> s/document/container/.
> (same applies to the comment above EndElementAt)

Yes, you're correct. Previously we didn't distinguish between document time and container time on the whole but I've started to update the documentation in this regard to make it more accurate now that we're taking more care to consider cross-time container dependencies. I just missed these instances so thanks for spotting that.

> So you're removing the explicit call to mCurrentInterval's no-argument
> constructor -- can we do the same here for mBeginSpecs() / mEndSpecs()?

Yes, of course. Thanks again. There's a guideline that some people use that says you should make sure the members are initialised in the order they're declared and I think I was just trying to enforce that but I really don't think it makes any difference here.

> ...
> >+  if (mElementState != STATE_ACTIVE && mElementState != STATE_STARTUP)
> >+    return;
> >+
> >+  DoSampleAt(aContainerTime, PR_TRUE); // End sample
> >+}
> 
> The early return seems unnecessary there, for a one-liner-function.  I'd change
> this to:
>   if (mElementState == STATE_ACTIVE || mElementState == STATE_STARTUP) {
>     DoSampleAt(aContainerTime, PR_TRUE); // End sample
>   }

Yep, no problem. Often I've added early returns because I think it documents the "normal" flow of operation more clearly, i.e. deal with preconditions etc at the beginning and then once they're out of the way the main operation is less cluttered. I'm only mentioning it here because this sort of thing comes up a number of times and you might be wondering what on earth I was thinking. That said, I'm more than happy to fit in with your preferences and have done as you've suggested here.

> >+nsSMILTimedElement::DoSampleAt(nsSMILTime aContainerTime, PRBool aEndOnly)
> >-  } while (stateChanged);
> >+  } while (stateChanged && (!aEndOnly || (mElementState != STATE_WAITING &&
> >+                                          mElementState != STATE_POSTACTIVE)));
> >+
> 
> I don't understand the change to the "while" condition there... Can you add a
> comment explaining why we've got those particular conditions on continuing to
> loop?
Added.

> Put the "=" on the first line there, after "prevIntervalWasZeroDur".
Fixed.

> No need for using assignment in the places quoted above -- do this instead, in
> both spots:
>   nsSMILTime activeDur(mRepeatCount * double(mSimpleDur.GetMillis()));

nsSMILTime is a typedef for PRInt64. If we do that we get compiler warnings so we need to make the typecast explicit. We do the multiplication between two doubles (nsSMILRepeatCount has an operator that returns a double) before casting down to a 64-bit integer.  

> >+  // We adopting the convention of not resolving intervals until the first
> >+  // sample.
> s/We/We're/  :)
Fixed. (Made it 'adopt' instead).

> >+nsSMILTimedElement::GetNextMilestone(nsSMILMilestone& aNextMilestone) const
> >+{
> [SNIP]
> >+  case STATE_ACTIVE:
> [SNIP]
> >+      // Check for an early end
> >+      if (mRestartMode == RESTART_ALWAYS) {
> >+        PRInt32 position = 0;
> >+        nsSMILTimeValue nextBegin;
> >+        PRBool found = GetNextGreater(mBeginInstances,
> >+            mCurrentInterval.mBegin, position, nextBegin);
> >+        if (found &&
> >+            nextBegin > mCurrentInterval.mBegin &&
> >+            nextBegin < mCurrentInterval.mEnd) {
> >+          aNextMilestone.mIsEnd = PR_TRUE;
> >+          aNextMilestone.mTime = nextBegin.GetMillis();
> >+          return PR_TRUE;
> 
> This block of code looks almost identical to the existing "CheckForEarlyEnd"
> method.  Can we tweak that method to make it more generally-useable, and make a
> call to it here?  (i.e. we could modify CheckForEarlyEnd() to return its
> "nextBegin" local variable by reference, instead of copying it to
> mCurrentInterval.mBegin.  Then we could react to that returned value both here
> and in the other place where CheckForEarlyEnd is already called.)

Definitely. Fixed.

> The phrase "moment after" in the comment here is vague -- it should probably
> say "the moment 1ms after", or "the moment after the current sample (1 ms
> later)", or something to that effect.  I know we're shooting for "an
> infinitessmial amount of time after", but it's good to document how precise we
> actually are, and indicate what the "+ 1" in the code actually means.

Fixed.

> If we're at an "NS_ABORT_IF_FALSE" level-of-confidence that elem will be
> non-null, then we shouldn't bother with an "if (!elem)" check here.  In the
> unanticipated case that |elem| ends up being null, I think we can just let
> non-debug builds dereference null and crash on "elem->GetTimeContainer()" in
> the next line.

Agreed. Fixed.

> I'd prefer to avoid the duplicated code here -- setting up nextMilestone &
> passing it through our child container table, in the exact same way, twice. 
> Can we clean that up, maybe like this:
>  while(PR_TRUE) {
>    nsSMILMilestone nextMilestone(GetCurrentTime() + 1, PR_TRUE);
>    mChildContainerTable.EnumerateEntries(GetNextMilestone, &nextMilestone);
>    if (nextMilestone.mTime > GetCurrentTime() {
>      break;
>    }
>    // [the rest of the while loop goes here]
>  }

Yep, that's good. Fixed.

> >+  while (nextMilestone.mTime <= GetCurrentTime()) {
> >+    for (PRUint32 i = 0; i < length; ++i) {
> [SNIP]
> >+      sampleTime = PR_MAX(nextMilestone.mTime, sampleTime);
> 
> Why is the sampleTime-updating-line inside off the "for" loop? AFAICT,
> nextMilestone's value will only change at the end of each "while" loop, so
> there's no way that it'd change from one iteration of the "for" loop to the
> next.
> 
> In fact, why do we keep a separate |sampleTime| at all?  AFAICT, it's just a
> (frequently-updated) copy of nextMilestone.mTime.  Can we just use
> "nextMilestone.mTime" in place of sampleTime?

You're right, the sampleTime declaration and updating was in the wrong place. I've fixed it now. sampleTime is declared outside the while-loop so that we ensure we never progress backwards in time whilst running milestone samples.

> I think the variable "isMilestone" here needs a better name.  It indicates
> whether GetNextMilestoneInParentTime() succeeded populating |thisMilestone|, so
> maybe we could call it something like "success", "gotMilestone",
> "didGetMilestone", or "didGetThisMilestone"? :)
Fixed.

> >+    (container->NeedsSample() || !params->mSkipUnchangedContainers)) {
> 
> The last line there needs 2 spaces more indentation.
Fixed.

(In reply to comment #28)
> (In reply to comment #27)
> > >+    nsSMILTime activeDur =
> > >+      nsSMILTime(mRepeatCount * double(mSimpleDur.GetMillis()));
> > 
> > No need for using assignment in the places quoted above -- do this instead, in
> > both spots:
> >   nsSMILTime activeDur(mRepeatCount * double(mSimpleDur.GetMillis()));
> 
> Actually, I'm not sure why we're using "double(" there in the first place --
> does that help us avoid overflow somehow?

Yes, as mentioned above, we're just following the rule of thumb of doing the operation at the highest precision before casting. (I'm not sure but it may also help some compilers work out they need to use nsSMILRepeatCount's double conversion operator.)

(In reply to comment #29)
> One other thing I don't completely understand in Patch B -- there are two
> comments that appear somewhat contradictory to me.
> * nsSMILTimeContainer.h says that mMilestoneEntries is "Reset on every full
> sample".
> * nsSMILTimeContainer::AddMilestone (which populates mMilestoneEntries) has a
> comment that suggests we may do extra samples if "attributes are changed on the
> timed element in between samples".
> 
> How do those two comments mesh together?  i.e. if we're resetting
> mMilestoneEntries on every sample, then how would tweaks to attributes between
> samples cause additional samples in AddMilestone?  (given that
> nsSMILAnimationController only pulls in milestones up to the current
> sample-time anyway, AFAICT)

Imagine a scenario like this:
   <animate begin="0s" end="2s" .../>

...
t=1s Timer event: Conduct sample
  DoMilestoneSamples:
  (none)
  Full sample:
  * nsSMILAnimationController calls nsSMILTimeContainer::ClearMilestones
  * animation is sampled at t=1s, registers (end) milestone at t=2s

t=1.5s Script sets 'end' attribute to 3s
  * animation registers milestone at t=3s, but (and this is the point of the comment in AddMilestone) because we store the milestone times along with the animation element we won't clear the milestone at t=2s so there are now two milestones registered for the animation (one at t=2s, one at t=3s).

t=2.5s Timer event: Conduct sample
  DoMilestoneSamples:
  * animation receives end sample at t=2s, registers (end) milestone at t=3s
    ^ This sample is unnecessary (but cheap)
  Full sample:
  * nsSMILAnimationController calls nsSMILTimeContainer::ClearMilestones
  * animation is sampled at t=2.5s, registers (end) milestone at t=3s

I'm not sure how to clear this up because I'm not sure which bit is confusing.  Do I need to make the distinction between milestone samples and full samples more clear?

(In reply to comment #30)
> Patch B:
> 
> Could AddMilestone become a performance problem? It looks like adding N
> milestones could be O(N^2)? Do we need to use a priority queue here?

I'm not sure. Finding the right index is log(N) since the list is sorted and we don't allow duplicates. In InsertElementAt (which calls ReplaceElementsAt): DestructRange does nothing (count==0); ShiftData, I think, is constant time; as is AssignRange (arrayLen==1). So I think that means it's O(N*log(N))? But yes, it's basically a priority queue.

> +  // The disadvantage of deferring resolving the interval is that DOM calls to
> +  // to getStartTime will throw an INVALID_STATE_ERR exception until the
> +  // document timeline begins since the start time has not yet been resolved.
> 
> Why is that OK?

getStartTime throws INVALID_STATE_ERR whenever there's no current interval (according to SVG 1.1 SE) which can happen whenever an animation is in the postactive state so it's not a particularly exceptional exception.

Until the document is fully parsed we can't be sure of the times of the current interval. For simple documents where we just use offset times we can, but when we have syncbase relationships we have to wait until all the dependent animations have been parsed before we know our start time for sure. By definition, container begin == onLoad, and since we don't support externalResourcesRequired, onLoad ≈ end of parsing. I think this is right anyway.

So it seems like the main alternatives are:

a) Return potentially wrong getStartTimes whilst we're still parsing the document. (And have the extra burden of updating every dependency every time our current interval changes which could be quite a few times if there's a complex network of interdependencies.)

b) Behave as if there's no current interval (which, indeed, there isn't) until the container timeline begins (not the document timeline--my bad, I've fixed the comment) and then return the correct start time (with the exception of cross-time container dependencies for compound documents with multiple SVG document fragments).

c) Behave as if there's no current interval until the compound document is fully
loaded and then return the (definitely) correct start time. This, however, means
that attempts to use getStartTime in handlers of the onLoad event won't work.

Currently we do (b).

Let me know if you think this is acceptable and I can update the comment to
clarify this.

Comment 37

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

Attachment: Patch C: Syncbase parsing - v3

(In reply to comment #31)
> Patch C:
> 
> +      if (escape) {
> +        memmove(cur-1, cur, (end - cur) * sizeof(PRUnichar));
> 
> This isn't great, it means parsing a long sequence of escapes is O(N^2). That
> is avoidable.

I'm not sure I quite understand but I think this should now be fixed. Please let me know if it's not what you meant however.

(In reply to comment #32)
> (From update of attachment 417402 [details] [diff] [review])
> Comments on patch C:
> ====================
> >+inline void
> >+TrimWsp(const PRUnichar*& aStart, const PRUnichar*& aEnd)
> ...
> Need spaces around the "-".
Done.

> Also, the naming of "TrimWsp" vs. "SkipWsp" is a bit confusing, and it doesn't
> really reflect the difference (& similarity) in functionality between these two
> functions.  Could we perhaps rename SkipWsp to SkipBeginWsp, and this new one
> to SkipBeginEndWsp?
Done.

> >+  PRUint32 value = strtol(str, &rest, 10);
> >+
> >+  if (rest == str)
> >+    return 0;
> >+
> >+  aResult = value;
> >+  return rest - str;
> 
> 10 is a bit of a magic number here -- could we #define it as DECIMAL_BASE or
> something?
Done.

> Also: strtol returns signed integers, not unsigned ones, so we should probably
> be checking for negatives here.  So:
>  - |value| should be declared as a PRInt32  (it should be signed)
>  - the early-return-case should be "if (rest == str || value < 0)"
>  - Use a static-cast when setting aResult to the now-signed |value|:
>       aResult = static_cast<PRUint32>(value);
>   (to explicitly cast off |value|'s signed-ness -- otherwise, the implicit
> conversion would trigger a compiler warning)
Yes, that's good. Thanks for that.

> Also: s/char */char* /
> (Only mentioning because I've already got you re-examining this block of code
> anyway :))
Fixed.

> >+static PRBool
> >+GetUnsignedIntAndBracket(const nsAString& aStr, PRUint32& aResult)
> >+{
> 
> "Bracket" in this function name is ambiguous, particularly since it implies []
> in US English.  Can we name this function "GetUnsignedIntAndEndParen" to avoid
> ambiguity about what type(s) and orientation(s) of bracket it accepts?

Yes, good. Fixed.

> >+  // Make sure the string is only digit+')'
> >+  if (intLen == 0 || *(start + intLen) != ')' || start + intLen + 1 != end)
> >+    return PR_FALSE;
> 
> You need to reverse the order of the last two conditions in the "if" check
> there.  Otherwise, we won't yet know whether our string is long enough to
> contain that final ')' character at start+intlen, and we shouldn't be reading
> the memory there.

Ooh, yeah, thanks for that.

> >+  NS_ABORT_IF_FALSE(StringBeginsWith(aSpec, NS_LITERAL_STRING("accesskey(")),
> >+      "Calling ParseAccessKey on non-accesskey-type spec");
> [SNIP]
> >+  const PRUnichar* start = aSpec.BeginReading() + 10;
> [SNIP]
> >+    } else if (StringBeginsWith(rawToken2, NS_LITERAL_STRING("repeat("))) {
> >+      result.mType = nsSMILTimeValueSpecParams::REPEAT;
> >+      if (!GetUnsignedIntAndBracket(Substring(tokenStart + 7, tokenEnd),
> 
> I'd prefer we don't have the magic numbers "10" and "7" here.  I guess those
> are the lengths of the strings "accesskey(" and "repeat(". Could we use static
> constants declared at the top of the file to avoid that?
> 
> e.g. like so:
>   static const char* ACCESSKEY_PREFIX = NS_LITERAL_STRING("accesskey(");
>   static const char* REPEAT_PREFIX = NS_LITERAL_STRING("repeat(");
> 
> Then we can replace the NS_LITERAL STRING()s in the code with those constants,
> and the magic numbers with e.g. "ACCESSKEY_PREFIX.Length();"

I've done something similar (but with #defines since the lines quoted above don't compile.)
I think the alternative is to using #defines is to do:
static const PRUnichar* ACCESSKEY_PREFIX = NS_LITERAL_STRING("accesskey(").get();
and then use nsDependentString(ACCESSKEY_PREFIX) elsewhere.

> >+ParseAccessKey(const nsAString& aSpec, nsSMILTimeValueSpecParams& aResult)
> [SNIP]
> >+  if (end - start < 2)
> >+    return NS_ERROR_FAILURE;
> 
> This needs a comment explaining why "< 2" is relevant here.
> e.g. // Expecting at least the accesskey char and ')'
Fixed.

> >+  if (NS_IS_HIGH_SURROGATE(c)) {
> >+    if (end - start < 2)
> >+      return NS_ERROR_FAILURE;
> 
> Here too. (note that "start" has been incremented since the last < 2 check)
Fixed.

> >+        memmove(cur-1, cur, (end - cur) * sizeof(PRUnichar));
> 
> Needs spaces around the "-". (though maybe this line is going away anyway, per
> comment 31)
Fixed above in response to comment 31.

> >+  const PRUnichar* tokenEnd = GetTokenEnd(aSpec, PR_TRUE);
> [SNIP]
> >+  // Parse the second token if there is one
> >+  if (*tokenEnd == '.') {
> 
> So if our token occupies the full string (aSpec), then I think "*tokenEnd" will
> read past the end of the string.
> (In a c-string, this would still be basically okay, because *tokenEnd would be
> the null terminator.  But https://developer.mozilla.org/en/XPCOM_string_guide
> says that nsAStrings aren't necessarily null-terminated.)
> 
> Basically, I think you need to check whether tokenEnd == aSpec.EndReading()
> before dereferencing tokenEnd here.

Yes, thanks. Fixed.

> >+  nsresult rv = ParseOptionalOffset(Substring(tokenEnd, specEnd), result);
> >+  if (NS_FAILED(rv))
> >+    return rv;
> >+
> >+  aResult = result;
> >+
> >+  return NS_OK;
> 
> Those last 6 lines can be shortened to:
>   if (NS_SUCCEEDED(rv)) {
>     aResult = result;
>   }
>   return rv;
Fixed.

Comment 38

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

Attachment: Patch D: Syncbase operation - v4

Rebase off changes to underlying patches.

Comment 39

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

Attachment: Patch E: Test cases - v2

Fix bitrot.

However, newly added test_smilXHR.xhtml now fails with these syncbase changes. I guess it's relying on beginElement and endElement behaviour that now differs.

Comment 40

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

InsertElementAt is definitely O(N) since we have to copy all the elements after the insertion point. (ShiftData is not constant time.)

Comment 41

[Daniel Holbert [:dholbert]]

(In reply to comment #36)
> unnecessary parentheses, are the parentheses needed here? I've left them out.
> Let me know if I ought to include them.

Good point - I've been called out for that as well. :) This is fine as-is.

> > No need for using assignment in the places quoted above -- do this instead, in
> > both spots:
> >   nsSMILTime activeDur(mRepeatCount * double(mSimpleDur.GetMillis()));
> 
> nsSMILTime is a typedef for PRInt64. If we do that we get compiler warnings so
> we need to make the typecast explicit.

My suggested change (removing " = nsSMILTime") doesn't introduce any compiler warnings on my machine...  AFAICT, the "nsSMILTime()" that you have there doesn't actually "make the typecast explicit", as you suggest it does -- it just invokes the constructor (to make a temporary nsSMILTime()), and then invokes the assignment operator (to assign that to |nsSMILTime activeDur|).  That assignment is extra work that we don't need to do.

So, I'd still prefer that we make this change, unless I'm misunderstanding something.

> > I think the variable "isMilestone" here needs a better name.
> Fixed.

The line you added in its place (PRBool didGetMilestone = ) has a space at the end -- nix that whitespace.

> > >   nsSMILTime activeDur(mRepeatCount * double(mSimpleDur.GetMillis()));
> > 
> > Actually, I'm not sure why we're using "double(" there in the first place --
> > does that help us avoid overflow somehow?
> 
> Yes, as mentioned above, we're just following the rule of thumb of doing the
> operation at the highest precision before casting.

FWIW, we should still do the operation at highest precision, without the explicit "double()" constructor. The only only multiplyable form of mRepeatCount is as a double (via "operator double"), so that argument is already going to be a double.  And when we multiply a double with anything else (besides |long double|), C will promote the other argument to double -- see the "either operand is of type double" case on these pages:
http://msdn.microsoft.com/en-us/library/09ka8bxx%28VS.80%29.aspx
http://docs.sun.com/app/docs/doc/820-7598/6nirjulul
http://www.oit.uci.edu/dcslib/digital_unix/digital-v40d/AQTLTBTE/DOCU_067.HTM

So, I still assert that the double() is unnecessary here.  But if you want to keep it for clarity, that's fine. :)

> > One other thing I don't completely understand in Patch B -- there are two
> > comments that appear somewhat contradictory to me.
[..]
> Imagine a scenario like this:
[...]
> I'm not sure how to clear this up because I'm not sure which bit is confusing. 

Ah, I think I get it now.  I had the order wrong in my head -- I was thinking that each full sample did something like:
    { clear milestones; calculate milestones; react to milestones; }

But really, full sample does this:
    { react to milestones; clear milestones; calculate milestones; }
(And then more "calculate milestones"-type activity can happen before the next sample, e.g. via script.)

I think I was partly confused because, in the "Reset on every full sample" comment, I interpreted "on" to mean "at the beginning of".  Could you change that comment to say something like this: "Reset during every full sample, after we've incorporated these into our timing model"?  I think that would make this clearer.

Comment 42

[Daniel Holbert [:dholbert]]

(In reply to comment #41)
> warnings on my machine...  AFAICT, the "nsSMILTime()" that you have there
> doesn't actually "make the typecast explicit", as you suggest it does -- it
> just invokes the constructor (to make a temporary nsSMILTime())

Apologies -- since nsSMILTime is typedefed as a PRInt64, which is the primitive type |long long|, I guess nsSMILTime() really is a typecast rather than a constructor.

So anyway, if MSVC does indeed warn about "PRInt64 foo(myDouble);" but doesn't complain about "PRInt64 foo = PRInt64(myDouble);" (even GCC likes both), then I guess never mind about this chunk.

Comment 43

[Daniel Holbert [:dholbert]]

(In reply to comment #40)
> InsertElementAt is definitely O(N) since we have to copy all the elements after
> the insertion point. (ShiftData is not constant time.)

That note in Comment 40 affects nsSMILTimedElement::AddDependent() in "Patch D", too -- n calls to that method will have O(n^2) performance, which is undesirable. (In Patch D, there's a comment above |mTimeDependents| in nsSMILTimedElement.h that claims it has O(n*logn) performance for n insertions -- that's based on the incorrect assumption that a sorted-insert takes O(logn) time.)

It sounds like mTimeDependents is trying to act like a "set" container.  (per this comment in AddDependent: "we want to ensure that we don't end up with duplicate entries")  Can we make mTimeDependents an actual "set" of some sort -- e.g. a nsTHashtable?

Comment 44

[Daniel Holbert [:dholbert]]

Comment on attachment 418053 [details] [diff] [review]
Patch D: Syncbase operation - v4

I'm not all the way through Patch D yet, but here's what I've got for it so far -- all minor stuff, aside from the O(n^2) issue mentioned in my previous comment.

>diff --git a/content/smil/nsSMILAnimationFunction.cpp b/content/smil/nsSMILAnimationFunction.cpp
>+  nsSMILTimedElement const &thisTimedElement
>+    = mAnimationElement->TimedElement();
>+  nsSMILTimedElement const &otherTimedElement
>+    = aOther->mAnimationElement->TimedElement();

Use "const nsSMILTimedElement& ", and move the equals signs to the previous line.

>+  nsIContent &thisContent = mAnimationElement->Content();
>+  nsIContent &otherContent = aOther->mAnimationElement->Content();

s/nsIContent &/nsIContent& /

>diff --git a/content/smil/nsSMILInstanceTime.cpp b/content/smil/nsSMILInstanceTime.cpp
>+nsSMILInstanceTime::CheckForCycle(const nsSMILInstanceTime* aNewTail)

This function should be renamed to something like "BreakPotentialCycle", so that it's clear that it actually changes stuff -- it does more than just "Check".

>diff --git a/content/smil/nsSMILInstanceTime.h b/content/smil/nsSMILInstanceTime.h
> #include "nsISupports.h"
> #include "nsSMILTimeValue.h"
>-#include "nsWeakReference.h"
> #include "nsAutoPtr.h"

Cool, we're removing an unneeded #include there -- can you also remove the |#include "nsISupports.h"|, while you're at it? (I tried in my build, and it doesn't break anything.)

>   nsSMILInstanceTime(const nsSMILTimeValue& aTime,
[SNIP]
>+    switch (aSource) {

This switch statement in the constructor is missing a case for enum value SOURCE_NONE, so it triggers this GCC compile warning in every .cpp file that #includes it:
  warning: enumeration value 'SOURCE_NONE' not handled in switch
To fix that, add a 'SOURCE_NONE' case (which could just be "break; // nothing special to initialize").  This should be the first case in the switch statement, to match the ordering in the enum declaration.

Also: this constructor's implementation (starting with the initializer list) is 20 lines long, and it'll be a bit longer with the SOURCE_NONE case.  That's kind of bulky for a function in a header file -- could we move that to the .cpp file?

>diff --git a/content/smil/nsSMILInterval.h b/content/smil/nsSMILInterval.h
>+  void Set(nsSMILInstanceTime& aBegin, nsSMILInstanceTime& aEnd)
>+  {
>+    NS_ABORT_IF_FALSE(aBegin.Time().IsResolved(),
>+        "Attempting to set unresolved begin time on an interval");
>+    if (!aBegin.Time().IsResolved())
>+      return;
[SNIP]
>+  void SetBegin(nsSMILInstanceTime& aBegin)
>+  {
>+    NS_ABORT_IF_FALSE(mBegin, "Calling SetBegin() on un-set interval");
>+    NS_ABORT_IF_FALSE(aBegin.Time().IsResolved(),
>+        "Attempting to set unresolved begin time on interval");
>+    mBegin = &aBegin;
>+  }

So in Set(), we have an early-return in the (unexpected) situation that aBegin is unresolved.  But we don't do that in SetBegin().  We should make those methods match, one way or the other.  (If we actually want that early return, then we presumably want it both functions).

>+  void FreezeBegin()
>+  {
>+    NS_ABORT_IF_FALSE(mBegin, "Calling FreezeBegin() on un-set interval");
>+    mBegin->MarkNoLongerUpdating();
>+  }

I think FreezeBegin could use an assertion to this effect:
  NS_ABORT_IF_FALSE(mEnd->MayUpdate(),
      "Freezing the beginning of an interval whose end is already frozen");
(This mirrors the assertion in FreezeEnd.)

>diff --git a/content/smil/nsSMILTimeContainer.cpp b/content/smil/nsSMILTimeContainer.cpp
> nsSMILTimeContainer::Pause(PRUint32 aType)
> {
>+  PRBool didStartPause = PR_FALSE;
>+
>   if (!mPauseState && aType) {
>     mPauseStart = GetParentTime();
>     mNeedsPauseSample = PR_TRUE;
>+    didStartPause = PR_TRUE;
>   }
[SNIP]
>+  if (didStartPause) {
>+    NotifyTimeChange();
>+  }

So right now, this will have us call NotifyTimeChange() if we're already paused and we receive a Pause() call with a different aType.  Is this what we want?

Superficially, it seems like we'd want to avoid calling NotifyTimeChange if we're already paused (regardless of pause-type).  Do we actually need those NotifyTimeChange calls?

>diff --git a/content/smil/nsSMILTimedElement.h b/content/smil/nsSMILTimedElement.h
>+nsSMILTimedElement::Traverse(nsCycleCollectionTraversalCallback* aCallback)
>+{
>+  PRUint32 count = mBeginSpecs.Length();
>+  nsSMILTimeValueSpec *spec = nsnull;
>+  for (PRUint32 i = 0; i < count; ++i) {
>+    spec = mBeginSpecs[i];
>+    NS_ABORT_IF_FALSE(spec, "null nsSMILTimeValueSpec in list of begin specs");
>+    spec->Traverse(aCallback);
>+  }
>+
>+  count = mEndSpecs.Length();
>+  for (PRUint32 j = 0; j < count; ++j) {
>+    spec = mEndSpecs[j];
>+    NS_ABORT_IF_FALSE(spec, "null nsSMILTimeValueSpec in list of end specs");
>+    spec->Traverse(aCallback);
>+  }
>+}

|spec| doesn't need to be so loosely scoped there -- move it inside the loops.
i.e.:
  nsSMILTimeValueSpec* spec = mBeginSpecs[i];
and
  nsSMILTimeValueSpec* spec = mEndSpecs[j];

Comment 45

[Daniel Holbert [:dholbert]]

(In reply to comment #44)
> |spec| doesn't need to be so loosely scoped there -- move it inside the loops.
> i.e.:
>   nsSMILTimeValueSpec* spec = mBeginSpecs[i];
> and
>   nsSMILTimeValueSpec* spec = mEndSpecs[j];

(maybe replacing |spec| with |endSpec| & |beginSpec|, for clarity)

Comment 46

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

Attachment: Patch B: Milestone sampling behaviour - v3

Just address dholbert's review feedback. Still lacks efficient adding of milestones.

Comment 47

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

(In reply to comment #41)
> The line you added in its place (PRBool didGetMilestone = ) has a space at the
> end -- nix that whitespace.
Cheers, fixed.

> > > >   nsSMILTime activeDur(mRepeatCount * double(mSimpleDur.GetMillis()));
> > > 
> > > Actually, I'm not sure why we're using "double(" there in the first place --
> > > does that help us avoid overflow somehow?
> > 
> > Yes, as mentioned above, we're just following the rule of thumb of doing the
> > operation at the highest precision before casting.
> 
> FWIW, we should still do the operation at highest precision, without the
> explicit "double()" constructor. The only only multiplyable form of
> mRepeatCount is as a double (via "operator double"), so that argument is
> already going to be a double.

Yes, you're right. I think the second part of my comment was the real reason, "I'm not sure but it may also help some compilers work out they need to use nsSMILRepeatCount's double conversion operator." Like you say, I'd like to keep it for clarity's sake if nothing else.

> > > One other thing I don't completely understand in Patch B -- there are two
> > > comments that appear somewhat contradictory to me.
> [..]
> > Imagine a scenario like this:
> [...]
> > I'm not sure how to clear this up because I'm not sure which bit is confusing. 
> 
> Ah, I think I get it now.  I had the order wrong in my head -- I was thinking
> that each full sample did something like:
>     { clear milestones; calculate milestones; react to milestones; }
> 
> But really, full sample does this:
>     { react to milestones; clear milestones; calculate milestones; }
> (And then more "calculate milestones"-type activity can happen before the next
> sample, e.g. via script.)

Yes, that's right. One slight clarification is that milestones can be added whilst reacting to milestones. So actually it can look like this:

* Pop first registered milestone < full sample time
  -- sample the elements registered for that time (t=x)
     --> this step may lead to more milestones being registered (possibly even at t<x)
* Pop first registered milestone < full sample time
  etc. etc.

So the process is iterative. Only once all milestones before or at the full sample time have been processed are the lists cleared (there may still be milestones registered after the full sample time). This is safe because every time we call 'sample' on a timed element it re-registers its milestones and a full sample of a time container involves sampling all of its timed elements.

Probably you knew all this but I'm just documenting it here for posterity's sake.

> I think I was partly confused because, in the "Reset on every full sample"
> comment, I interpreted "on" to mean "at the beginning of".  Could you change
> that comment to say something like this: "Reset during every full sample, after
> we've incorporated these into our timing model"?  I think that would make this
> clearer.

Good idea. Done.

Comment 48

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

I'm having trouble keeping track of which patches I should actually be reviewing right now. Can you remove review requests from patches which you still expect to update, like Patch B? Or do you want me to review them right now, anyway?

Comment 49

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

Comment on attachment 419277 [details] [diff] [review]
Patch B: Milestone sampling behaviour - v3

Removing request for review for now. As discussed with roc I'll try to create a separate data structure nsTPriorityQueue and base the patch on that before putting this back up for review.

Comment 50

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

Comment on attachment 418045 [details] [diff] [review]
Patch A: Refactor nsSMILParserUtils to use newer iterators - v3 (r=dholbert, sr=roc)

Just adding review flags to title since I had to carry the r+/sr+ forward for this.

Comment 51

[Daniel Holbert [:dholbert]]

(In reply to comment #47)
> > I think I was partly confused because, in the "Reset on every full sample"
> > comment, I interpreted "on" to mean "at the beginning of".  Could you change
> > that comment to say something like this: "Reset during every full sample, after
> > we've incorporated these into our timing model"?  I think that would make this
> > clearer.
> 
> Good idea. Done.

I don't think the new version of this comment is much clearer -- looks like you replaced "on" with "at the beginning of", which doesn't actually clear up the confusion I described in the quoted chunk above.  (I was actually saying that "at the beginning of" was my initial _incorrect_ interpretation.)

This comment should be changed to something like the following -- in particular, it needs to mention that we only reset the list only *after* we've reacted (and possibly added more items) to it.
>+  // Array of elements with registered milestones. Reset near the beginning of
>+  // every full sample, after we've incorporated its contents into timing model.

Comment 52

[Daniel Holbert [:dholbert]]

Comment on attachment 418052 [details] [diff] [review]
Patch C: Syncbase parsing - v3

Patch C v3 looks good! Just a few nits:

>+const PRInt32  DECIMAL_BASE  = 10;
>+
>+#define ACCESSKEY_PREFIX NS_LITERAL_STRING("accesskey(")
>+#define REPEAT_PREFIX    NS_LITERAL_STRING("repeat(")
>+#define WALLCLOCK_PREFIX NS_LITERAL_STRING("wallclock(")

We should declare these strings using "const nsString" instead of "#define". That way, we only have to do NS_LITERAL_STRING()'s work once, rather than in every single place these are used.

>+void
>+Unescape(nsAString& aStr)
>+{
>+  const PRUnichar* read = aStr.BeginReading();
>+  const PRUnichar* const end = aStr.EndReading();
>+  PRUnichar* write = aStr.BeginWriting();
>+  PRBool escape = PR_FALSE;
>+
>+  while (read != end) {

Add something like this as the first line of the while loop, to explicitly state this invariant assumption:
  NS_ABORT_IF_FALSE(write <= read, "uh oh, writing past where we've read");

>+  aStr.SetLength(write - aStr.BeginReading());

SetLength() returns a PRBool success value -- we should probably complain if it fails. Something like this:

 #ifdef DEBUG
   PRBool setLengthSuccess =
 #endif
   aStr.SetLength(write - aStr.BeginReading());
   NS_ABORT_IF_FALSE(setLengthSuccess, "Failed to update string length");

r=dholbert with those fixes.

Comment 53

[Daniel Holbert [:dholbert]]

(In reply to comment #44)
> >   if (!mPauseState && aType) {
> >     mPauseStart = GetParentTime();
> >     mNeedsPauseSample = PR_TRUE;
> >+    didStartPause = PR_TRUE;
> So right now, this will have us call NotifyTimeChange() if we're already paused
> and we receive a Pause() call with a different aType.

Oops, sorry, disregard this chunk of my comments for Patch D -- I misread the "if" condition as "if (!(mPauseState & aType)).  We are doing the right thing there after all -- only reacting if we're starting a new pause.

Comment 54

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

Attachment: Patch B part 1: nsTPriorityQueue - v1

First attempt at a priority queue container.

I'm not sure who I need to sr this since I've put it in xpcom/glue?

One issue for consideration is the return value of Pop(). Currently it returns the topmost element by value. This, I think, is the most intuitive behaviour but results in extra temporary copies of the object being created. For this reason STL's priority_queue actually has pop() return nothing. So if you want to do a pop you first need to copy the value by calling top (which returns a const ref) and then call pop. I think for our uses the extra temporary copy is ok but let me know what you think.

Comment 55

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

Attachment: Patch B part 2: Milestone sampling behaviour - v4

Reworked milestone sampling to use the priority queue.

Also, addressed dholbert's review feedback.

(In reply to comment #51)
> (In reply to comment #47)
> > > I think I was partly confused because, in the "Reset on every full sample"
> > > comment, I interpreted "on" to mean "at the beginning of".  Could you change
> > > that comment to say something like this: "Reset during every full sample, after
> > > we've incorporated these into our timing model"?  I think that would make this
> > > clearer.
> > 
> > Good idea. Done.
> 
> I don't think the new version of this comment is much clearer -- looks like you
> replaced "on" with "at the beginning of", which doesn't actually clear up the
> confusion I described in the quoted chunk above.  (I was actually saying that
> "at the beginning of" was my initial _incorrect_ interpretation.)

Oh ok, sorry about that. Actually I think your initial interpretation is correct in that we have the following:

Sample:
  Milestone samples
  Full sample

And the milestones are cleared at the beginning of that second step, the "full sample". I've updated the comment as follows:

> Queue of elements with registered milestones. Used to update the model with
> significant transitions that occur between two samples. Since timed element
> re-register their milestones when they're sampled this is reset once we've
> taken care of the milestones before the current sample time but before we
> actually do the full sample.

Comment 56

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

Attachment: Patch C: Syncbase parsing - v4 (r=dholbert)

(In reply to comment #52)
> (From update of attachment 418052 [details] [diff] [review])
> Patch C v3 looks good! Just a few nits:
> 
> >+const PRInt32  DECIMAL_BASE  = 10;
> >+
> >+#define ACCESSKEY_PREFIX NS_LITERAL_STRING("accesskey(")
> >+#define REPEAT_PREFIX    NS_LITERAL_STRING("repeat(")
> >+#define WALLCLOCK_PREFIX NS_LITERAL_STRING("wallclock(")
> 
> We should declare these strings using "const nsString" instead of "#define".
> That way, we only have to do NS_LITERAL_STRING()'s work once, rather than in
> every single place these are used.

As discussed with dholbert, replacing #define with const nsString here effectively means we end up creating static strings and this generates, "XPCOM objects created/destroyed from static ctor/dtor" warnings for each string.

> Add something like this as the first line of the while loop, to explicitly
> state this invariant assumption:
>   NS_ABORT_IF_FALSE(write <= read, "uh oh, writing past where we've read");
Done.

> >+  aStr.SetLength(write - aStr.BeginReading());
> 
> SetLength() returns a PRBool success value -- we should probably complain if it
> fails. Something like this:
> 
>  #ifdef DEBUG
>    PRBool setLengthSuccess =
>  #endif
>    aStr.SetLength(write - aStr.BeginReading());
>    NS_ABORT_IF_FALSE(setLengthSuccess, "Failed to update string length");

As discussed with dholbert, although nsStringAPI.h declares SetLength with a PRBool return value, nsTSubstring.h (included by nsAString.h) has a void return value and this seems to be the definition in use.

Carrying forward r=dholbert.

Comment 57

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

Attachment: Patch D: Syncbase operation - v5

I'm just updating this to rebase off the underlying changes to patch B. The review feedback from comment 43 onwards for this patch (patch D) have not yet been incorporated.

I've also brought forward the changes to test_smilGetStartTime.xhtml into this patch to make the change more atomic. Likewise, a primitive version of some of the changes of this patch have been pushed into patch B to make it more atomic (although it still is not stable on its own).

(In reply to comment #39)
> However, newly added test_smilXHR.xhtml now fails with these syncbase changes.
> I guess it's relying on beginElement and endElement behaviour that now differs.
This is no longer an issue.

Comment 58

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

Attachment: Patch E: Test cases - v3

Rebase after the changes to test_smilGetStartTime.xhtml were moved into Patch D.

Comment 59

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

(In reply to comment #48)
> I'm having trouble keeping track of which patches I should actually be
> reviewing right now. Can you remove review requests from patches which you
> still expect to update, like Patch B? Or do you want me to review them right
> now, anyway?

All patches are now ready for review. There are no further changes I expect to make except to address review feedback. At this moment, the only outstanding review feedback pertains to patch D.

Comment 60

[Daniel Holbert [:dholbert]]

Comment on attachment 419500 [details] [diff] [review]
Patch D: Syncbase operation - v5

Here are a few more review comments for patch D.  (I've put the more nit-ish ones at the end so that if you don't get to them, I can easily address them in a followup patch. Also, as discussed in IRC, I'm happy to write a followup patch for turning nsSMILTimedElement::mTimeDependents into a hash table, per comment 43.)

>diff --git a/content/smil/nsSMILTimeValueSpec.cpp b/content/smil/nsSMILTimeValueSpec.cpp
>+void
>+nsSMILTimeValueSpec::ResolveReferences(nsIContent* aContextNode)
>+{
[SNIP]
>+  if (!mTimebase) {
>+    mTimebase = new TimebaseElement(this);
>+    if (!mTimebase)
>+      return;
>+  }

Can we make mTimebase just a normal member variable of nsSMILTimeValueSpec (like nsSVGAnimationElement::mHrefTarget)? I don't think we gain anything by having it be a pointer to a separate structure.

>+  nsSMILTimeValue docTime =
>+    aSrcContainer->ContainerToParentTime(aSrcTime.GetMillis());
>+
>+  if (!docTime.IsResolved())
>+    // This will happen if the source container is paused and we have a future
>+    // time. Just return the indefinite time.
>+    return docTime;

This chunk is a bit confusing -- it implies that |docTime| must be either Resolved or Indefinite, and it's not immediately clear why that would be the case. (If I look at the nsSMILTimeContainer::ContainerToParentTime documentation & implementation, then it makes sense, since those are the only two things that function returns.  But it's not clear without looking over there.)

We should change this to something like:

 if (docTime.IsIndefinite())
    // This will happen if the source container is paused and we have a future
    // time. Just return the indefinite time.
    return docTime;

  NS_ABORT_IF_FALSE(docTime.IsResolved(),
      "ContainerToParentTime gave us an unresolved time");

  return dstContainer->ParentToContainerTime(docTime.GetMillis());

***********************************************************
The remaining comments (below) are more nit-ish -- feel free to ignore them if you don't get to them before posting your final patch.
***********************************************************

>diff --git a/content/smil/nsSMILInstanceTime.h b/content/smil/nsSMILInstanceTime.h
>+  nsrefcnt AddRef()
>+  {
>+    NS_PRECONDITION(PRInt32(mRefCnt) >= 0, "illegal refcnt");

This initial precondition is kind of weird -- mRefCnt is an unsigned value (a nsrefcnt), so...
 (a) it's already >= 0 by definition
 (b) The PRInt32 cast will turn very large values from positive to negative, potentially making us "unjustly" fail this NS_PRECONDITION.

I'd prefer that we check vs PR_UINT32_MAX, and then actually handle that edge case, as in nsStyleStruct.h (quoted below):
166   nsrefcnt AddRef() {
167     if (mRefCnt == PR_UINT32_MAX) {
168       NS_WARNING("refcount overflow, leaking nsStyleGradient");
169       return mRefCnt;
170     }
171     ++mRefCnt;
172     NS_LOG_ADDREF(this, mRefCnt, "nsStyleGradient", sizeof(*this));
173     return mRefCnt;
174   }
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.h?mark=166-170#165

This check is clearer, it catches both overflows and underflows (since both would cross PR_UINT32_MAX), and it has safe fall-back behavior. (leaking, rather than potentially double-freeing)

>+  nsrefcnt Release()
>+  {

After the above "refcount overflow" change, we'll needs a corresponding change in Release(), as shown in the nsStyleStruct.h implementations. (basically, if our count is PR_UINT32_MAX, warn instead of decrementing)

>+    NS_ABORT_IF_FALSE(_mOwningThread.GetThread() == PR_GetCurrentThread(),
>+        "nsSMILInstanceTime release isn't thread-safe!");
[snip]
>+    if (mRefCnt == 0) {
>+      mRefCnt = 1; /* stabilize */
>+      delete this;
>+      return 0;
>+    }

I don't think we actually need that "stabilize" adjustment to mRefCnt there -- if we're assuming that AddRef / Release will only be used in a single thread (as the quoted NS_ABORT_IF_FALSE asserts), we won't ever need to "stabilize" anything, right?

(Some other "Release()" implementations in Mozilla include this line, but I think that's usually because they're in code that expects to be multithreaded -- e.g. nsTimerImpl, nsNodeInfoManager -- as judged by the fact that they use "PR_AtomicDecrement" rather than "--".)

>diff --git a/content/smil/nsSMILTimeContainer.cpp b/content/smil/nsSMILTimeContainer.cpp
>+nsSMILTimeContainer::NotifyTimeChange()
>+{
[SNIP]
>+  const MilestoneEntry* p = mMilestoneEntries.Elements();
>+  const MilestoneEntry* const end = p + mMilestoneEntries.Length();
>+  while (p != end) {
>+    nsISMILAnimationElement* elem = p->mTimebase.get();
>+    elem->TimedElement().HandleContainerTimeChange();
>+    ++p;
>+  }

The "Elements()" call seems like overkill here, if we're just iterating through the array.
Can't we just do something like:
  for (PRUint32 i = 0; i < mMilestoneEntries.Length(); i++) {
    nsISMILAnimationElement* elem = mMilestoneEntries[i].mTimebase.get();
    elem->TimedElement().HandleContainerTimeChange()
  }

>diff --git a/content/smil/nsSMILTimeValueSpec.cpp b/content/smil/nsSMILTimeValueSpec.cpp
>+  nsRefPtr<nsIContent> oldTimebaseContent =
>+    mTimebase ? mTimebase->get() : nsnull;

We should add a comment here about why this needs to be a nsRefPtr, since it's not clear from the immediate context. (I think we need it because |mTimebase| might have the last pointer to this element, and we're about to clear that reference when we call mTimebase->Reset(), and we need |oldTimebaseContent| to live a few lines beyond that.)

>+void
>+nsSMILTimeValueSpec::ResolveReferences(nsIContent* aContextNode)
>+{
[snip]
>+  nsString idStr;
>+  mParams.mDependentElemID->ToString(idStr);
>+  nsString url(PRUnichar('#'));
>+  url.Append(idStr);

The variable |url| here isn't actually a URL -- it's a string of the form "#foo". It should probably be called "idStrWithHash", or something.  (This chunk of code already has other variables called |baseURI| and |targetURI|, so other non-URI variables with URI-like names are just confusing. :))

>+nsSMILTimeValueSpec::HandleNewInterval(const nsSMILInterval& aInterval,
[SNIP]
>+  const nsSMILInstanceTime &baseInstance = mParams.mSyncBegin
[SNIP]
>+nsSMILTimeValueSpec::HandleChangedInterval(const nsSMILInterval& aInterval,
[SNIP]
>+  const nsSMILInstanceTime &baseInstance = mParams.mSyncBegin

s/nsSMILInstanceTime &/nsSMILInstanceTime& /

>+nsSMILTimeValueSpec::UpdateTimebase(nsIContent* aFrom, nsIContent* aTo)
[SNIP]
>+  nsSMILTimedElement *to = GetTimedElementFromContent(aTo);

s/nsSMILTimedElement */nsSMILTimedElement* /

>+  // If we're a begin spec but the time is now unresolved delete the interval.

Add comma before "delete the interval"

>diff --git a/content/smil/nsSMILTimedElement.h b/content/smil/nsSMILTimedElement.h
>+  void UpdateInstanceTime(nsSMILInstanceTime* aInstanceTime,
>+                          nsSMILTimeValue &aUpdatedTime,

s/nsSMILTimeValue &/nsSMILTimeValue& /

Comment 61

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

Attachment: Patch D: Syncbase operation - v6

(In reply to comment #43)
...
> That note in Comment 40 affects nsSMILTimedElement::AddDependent() in "Patch
> D", too -- n calls to that method will have O(n^2) performance, which is
> undesirable. (In Patch D, there's a comment above |mTimeDependents| in
> nsSMILTimedElement.h that claims it has O(n*logn) performance for n insertions
> -- that's based on the incorrect assumption that a sorted-insert takes O(logn)
> time.)
> 
> It sounds like mTimeDependents is trying to act like a "set" container.  (per
> this comment in AddDependent: "we want to ensure that we don't end up with
> duplicate entries")  Can we make mTimeDependents an actual "set" of some sort
> -- e.g. a nsTHashtable?
As discussed on IRC, we should definitely do this and Daniel has offered to take care of it.

(In reply to comment #44)
...
> Use "const nsSMILTimedElement& ", and move the equals signs to the previous
> line.
Done.

> s/nsIContent &/nsIContent& /
Done.

> >diff --git a/content/smil/nsSMILInstanceTime.cpp b/content/smil/nsSMILInstanceTime.cpp
> >+nsSMILInstanceTime::CheckForCycle(const nsSMILInstanceTime* aNewTail)
> 
> This function should be renamed to something like "BreakPotentialCycle", so
> that it's clear that it actually changes stuff -- it does more than just
> "Check".
Good call. Fixed.

> Cool, we're removing an unneeded #include there -- can you also remove the
> |#include "nsISupports.h"|, while you're at it? (I tried in my build, and it
> doesn't break anything.)
Yep, great.

> >   nsSMILInstanceTime(const nsSMILTimeValue& aTime,
> [SNIP]
> >+    switch (aSource) {
> 
> This switch statement in the constructor is missing a case for enum value
> SOURCE_NONE, so it triggers this GCC compile warning in every .cpp file that
> #includes it:
>   warning: enumeration value 'SOURCE_NONE' not handled in switch
> To fix that, add a 'SOURCE_NONE' case (which could just be "break; // nothing
> special to initialize").  This should be the first case in the switch
> statement, to match the ordering in the enum declaration.
Fixed.

> Also: this constructor's implementation (starting with the initializer list) is
> 20 lines long, and it'll be a bit longer with the SOURCE_NONE case.  That's
> kind of bulky for a function in a header file -- could we move that to the .cpp
> file?
Agreed. Fixed.

> >diff --git a/content/smil/nsSMILInterval.h b/content/smil/nsSMILInterval.h
> >+  void Set(nsSMILInstanceTime& aBegin, nsSMILInstanceTime& aEnd)
> >+  {
> >+    NS_ABORT_IF_FALSE(aBegin.Time().IsResolved(),
> >+        "Attempting to set unresolved begin time on an interval");
> >+    if (!aBegin.Time().IsResolved())
> >+      return;
> [SNIP]
> >+  void SetBegin(nsSMILInstanceTime& aBegin)
> >+  {
> >+    NS_ABORT_IF_FALSE(mBegin, "Calling SetBegin() on un-set interval");
> >+    NS_ABORT_IF_FALSE(aBegin.Time().IsResolved(),
> >+        "Attempting to set unresolved begin time on interval");
> >+    mBegin = &aBegin;
> >+  }
> 
> So in Set(), we have an early-return in the (unexpected) situation that aBegin
> is unresolved.  But we don't do that in SetBegin().  We should make those
> methods match, one way or the other.  (If we actually want that early return,
> then we presumably want it both functions).

Agreed. I've removed the early return from Set(). We really shouldn't ever be setting an unresolved begin (we have NS_ABORT_IF_FALSE's littered all through the code to ensure this invariant is never violated.)

> I think FreezeBegin could use an assertion to this effect:
>   NS_ABORT_IF_FALSE(mEnd->MayUpdate(),
>       "Freezing the beginning of an interval whose end is already frozen");
> (This mirrors the assertion in FreezeEnd.)
Actually the MayUpdate behaviour is a little confusing. It's mainly there to assist filtering instance times--something we don't do yet but will look at in implementing backwards seeking. But basically not all instance times begin life as being updateable--for example an offset time such as '5s' is not updateable. So the assertion suggested doesn't hold (I tried).

This is something which needs tidying up and clarifying when we implement instance time filtering.

> |spec| doesn't need to be so loosely scoped there -- move it inside the loops.
> i.e.:
>   nsSMILTimeValueSpec* spec = mBeginSpecs[i];
> and
>   nsSMILTimeValueSpec* spec = mEndSpecs[j];
Fixed here and in Unlink and renamed to beginSpec/endSpec as suggested in comment 45.

(In reply to comment #60)
> (From update of attachment 419500 [details] [diff] [review])
> Here are a few more review comments for patch D.  (I've put the more nit-ish
> ones at the end so that if you don't get to them, I can easily address them in
> a followup patch. Also, as discussed in IRC, I'm happy to write a followup
> patch for turning nsSMILTimedElement::mTimeDependents into a hash table, per
> comment 43.)
Cheers Daniel.

> >diff --git a/content/smil/nsSMILTimeValueSpec.cpp b/content/smil/nsSMILTimeValueSpec.cpp
> >+void
> >+nsSMILTimeValueSpec::ResolveReferences(nsIContent* aContextNode)
> >+{
> [SNIP]
> >+  if (!mTimebase) {
> >+    mTimebase = new TimebaseElement(this);
> >+    if (!mTimebase)
> >+      return;
> >+  }
> 
> Can we make mTimebase just a normal member variable of nsSMILTimeValueSpec
> (like nsSVGAnimationElement::mHrefTarget)? I don't think we gain anything by
> having it be a pointer to a separate structure.

Yeah, fair point. The main benefit of having it as a pointer member is the space saving. Probably about 95% of the time we'll be dealing with nsSMILTimeValueSpec's that represent simple offsets ('5s'), indefinite times, etc. where the mTimebase member isn't needed. So by keeping mTimebase as a pointer member we only have one pointer member in those cases, rather than the four contained in nsReferencedElement. Another approach might be to have a subclass nsSMILDependentTimeValueSpec that adds mTimebase as a member. For now, I've just done as you suggested and made it a regular member.

(Also, I've added a separate member for setting the owning TimeValueSpec to avoid the warning some compilers generate about using 'this' in an initializer list.)

...
> >+  if (!docTime.IsResolved())
> >+    // This will happen if the source container is paused and we have a future
> >+    // time. Just return the indefinite time.
> >+    return docTime;
> 
> This chunk is a bit confusing...
> 
> We should change this to something like:
...
>   NS_ABORT_IF_FALSE(docTime.IsResolved(),
>       "ContainerToParentTime gave us an unresolved time");

That's good. Done.

> >diff --git a/content/smil/nsSMILInstanceTime.h b/content/smil/nsSMILInstanceTime.h
> >+  nsrefcnt AddRef()
> >+  {
> >+    NS_PRECONDITION(PRInt32(mRefCnt) >= 0, "illegal refcnt");
> 
> This initial precondition is kind of weird
...

That code was just copied from elsewhere in the codebase. I've updated it (and Release()) to match nsStyleStruct as you suggest.

> >+    NS_ABORT_IF_FALSE(_mOwningThread.GetThread() == PR_GetCurrentThread(),
> >+        "nsSMILInstanceTime release isn't thread-safe!");
> [snip]
> >+    if (mRefCnt == 0) {
> >+      mRefCnt = 1; /* stabilize */
> >+      delete this;
> >+      return 0;
> >+    }
> 
> I don't think we actually need that "stabilize" adjustment to mRefCnt there --
> if we're assuming that AddRef / Release will only be used in a single thread
> (as the quoted NS_ABORT_IF_FALSE asserts), we won't ever need to "stabilize"
> anything, right?

Yep, fixed.

> >diff --git a/content/smil/nsSMILTimeContainer.cpp b/content/smil/nsSMILTimeContainer.cpp
> >+nsSMILTimeContainer::NotifyTimeChange()
> >+{
> [SNIP]
> >+  const MilestoneEntry* p = mMilestoneEntries.Elements();
> >+  const MilestoneEntry* const end = p + mMilestoneEntries.Length();
> >+  while (p != end) {
> >+    nsISMILAnimationElement* elem = p->mTimebase.get();
> >+    elem->TimedElement().HandleContainerTimeChange();
> >+    ++p;
> >+  }
> 
> The "Elements()" call seems like overkill here, if we're just iterating through
> the array.
> Can't we just do something like:
>   for (PRUint32 i = 0; i < mMilestoneEntries.Length(); i++) {
>     nsISMILAnimationElement* elem = mMilestoneEntries[i].mTimebase.get();
>     elem->TimedElement().HandleContainerTimeChange()
>   }

Yeah, it's a good point. It's basically a habit from STL programming :)
However, now that we're using a priority queue to store the elements we don't have the subscript operator so we need to do something like this anyway. I've updated it so that we won't read beyond the end of the array in the unlikely event that the call to HandleContainerTimeChange results in some call back to the time container that alters the priority queue. I've also added an NS_ABORT_IF_FALSE to ensure that's not the case.

(I've also altered Traverse() to use a consistent pattern of access.)

> >diff --git a/content/smil/nsSMILTimeValueSpec.cpp b/content/smil/nsSMILTimeValueSpec.cpp
> >+  nsRefPtr<nsIContent> oldTimebaseContent =
> >+    mTimebase ? mTimebase->get() : nsnull;
> 
> We should add a comment here about why this needs to be a nsRefPtr, since it's
> not clear from the immediate context. (I think we need it because |mTimebase|
> might have the last pointer to this element, and we're about to clear that
> reference when we call mTimebase->Reset(), and we need |oldTimebaseContent| to
> live a few lines beyond that.)

Yes, exactly. I've added a comment to that effect.

> The variable |url| here isn't actually a URL -- it's a string of the form
> "#foo". It should probably be called "idStrWithHash", or something.  (This
> chunk of code already has other variables called |baseURI| and |targetURI|, so
> other non-URI variables with URI-like names are just confusing. :))
Fixed.

> s/nsSMILInstanceTime &/nsSMILInstanceTime& /
Fixed.

> s/nsSMILTimedElement */nsSMILTimedElement* /
Fixed.

> >+  // If we're a begin spec but the time is now unresolved delete the interval.
> 
> Add comma before "delete the interval"
Fixed.

> s/nsSMILTimeValue &/nsSMILTimeValue& /
Fixed. Sorry, old habits die hard.


As far as I am aware, the only outstanding issue now relating to all these patches is the suggestion to use a hashtable for tracking time dependents (comment 43) which Daniel has kindly offered to take care of.

Comment 62

[Daniel Holbert [:dholbert]]

Comment on attachment 419548 [details] [diff] [review]
Patch D: Syncbase operation - v6

(In reply to comment #61)
> > Can we make mTimebase just a normal member variable of nsSMILTimeValueSpec
> > (like nsSVGAnimationElement::mHrefTarget)? I don't think we gain anything by
> > having it be a pointer to a separate structure.
> 
> Yeah, fair point. The main benefit of having it as a pointer member is the
> space saving...
> I've just done as you suggested and made it a regular member.

Mm, good point -- now I wonder if it was better as it was before. :)  But I think I like the new non-pointer version (in the updated patch), for the reduced need for null-checking & cleanup (& resulting improved readability), if nothing else.

> (Also, I've added a separate [method] for setting the owning TimeValueSpec to
> avoid the warning some compilers generate about using 'this' in an initializer
> list.)

Oh, interesting.  From glancing at a windows build log, it looks like we're already triggering that warning for other nsReferencedElement-owners (nsSVGAnimationElement, nsSVGUseElement, nsSVGEffectsElement).

More info about this warning here:
http://bytes.com/topic/net/answers/264514-warning-c4355-used-base-member-initializer-list

I'm not very excited about adding a new "SetOwner" method & method-call to work around this warning. I'd rather just disable the warning for this chunk, since we know that we're safe -- we won't be dereferencing the passed-in |this| pointer in mTimebase's constructor.

Can you fix this (using something like the following), and verify that it fixes the warning?
  #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.
  #pragma warning(push)
  #pragma warning(disable:4355)
  #endif
  nsSMILTimeValueSpec::nsSMILTimeValueSpec(nsSMILTimedElement& aOwner,
                                          PRBool aIsBegin)
    : mOwner(&aOwner),
      mIsBegin(aIsBegin),
      mVisited(PR_FALSE),
      mChainEnd(PR_FALSE),
      mTimebase(this)
  #ifdef _MSC_VER
  #pragma warning(pop)
  #endif
  {
  ...

Assuming that this change fixes this warning, I'll file another bug on adding this #pragma to the other affected places, to reduce warning-spam there as well.

With that change, r=dholbert (given that I'll be switching mTimeDependents to a hashtable in a separate patch)

Comment 63

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

Attachment: Patch D: Syncbase operation - v7 (r=dholbert)

(In reply to comment #62)
> (From update of attachment 419548 [details] [diff] [review])
> (In reply to comment #61)
> > > Can we make mTimebase just a normal member variable of nsSMILTimeValueSpec
> > > (like nsSVGAnimationElement::mHrefTarget)? I don't think we gain anything by
> > > having it be a pointer to a separate structure.
...
> Mm, good point -- now I wonder if it was better as it was before. :)  But I
> think I like the new non-pointer version (in the updated patch), for the
> reduced need for null-checking & cleanup (& resulting improved readability), if
> nothing else.

Yeah it definitely looks better. One alternative is to make a separate subclass for time value specs that have a dependent element and have a factory method that generates the appropriate subclass depending on what kind of value we have.  I think most of the methods could still remain non-virtual (except the dtor) since when we register time dependencies we could register them as nsSMILDependentTimeValueSpec's rather than the base class.

In fact, we hardly even need to keep the nsSMILTimeValueSpec objects for offset and indefinite times. They generate an instance time when we call SetSpec (and we do that only once) and then do nothing else (except we do test whether mEndSpecs is empty at one important point in nsSMILTimedElement, but this could be replaced with a bool--we already have mBeginSpecSet).

Getting rid of the nsSMILTimeValueSpec objects in those cases would be a significant saving for documents that are full of offset times.

> > (Also, I've added a separate [method] for setting the owning TimeValueSpec to
> > avoid the warning some compilers generate about using 'this' in an initializer
> > list.)
> 
> Oh, interesting.  From glancing at a windows build log, it looks like we're
> already triggering that warning for other nsReferencedElement-owners
> (nsSVGAnimationElement, nsSVGUseElement, nsSVGEffectsElement).
Yep, I feel guilty every time I see that warning coming out of nsSVGAnimationElement.

> Can you fix this (using something like the following), and verify that it fixes
> the warning?
Yes, that fixes the warning.

> Assuming that this change fixes this warning, I'll file another bug on adding
> this #pragma to the other affected places, to reduce warning-spam there as
> well.
Ok, sounds good.

> With that change, r=dholbert (given that I'll be switching mTimeDependents to a
> hashtable in a separate patch)

Thanks Daniel!

Comment 64

[Daniel Holbert [:dholbert]]

Comment on attachment 419489 [details] [diff] [review]
Patch B part 2: Milestone sampling behaviour - v4

Just two nits regarding comments, in Patch B part 2 v4  :)

>+void
>+nsSMILTimedElement::RegisterMilestone()
[SNIP]
>+  // This method is called every time we might possibly have updated our
>+  // current interval but since nsSMILTimeContainer makes not attempt to filter
>+  // out redundant milestones we do some rudimentary filtering here. It's not
>+  // perfect but unnecessary samples are fairly cheap.

s/makes not/makes no/
Also, add comma after "current interval", "redundant milestones", and "perfect".

>diff --git a/content/smil/nsSMILTimedElement.h b/content/smil/nsSMILTimedElement.h
>+  /**
>+   * Called when the timed element has been bound to the document so that
>+   * references.
>+   */
>+  void BindToTree();

Looks like this comment ends prematurely there. ("Called...so that references.")  Patch D completes this comment, so it ends up fine, but I'd rather we not have a broken comment in the intermediate state.

Perhaps replace "references." with "milestones can be registered." here?
(You can also manually tweak the removed line within patch D, to avoid having to fix merge issues, if you like.)

r=dholbert with those fixed

Comment 65

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

Attachment: Patch B part 2: Milestone sampling behaviour - v5 (r=dholbert)

(In reply to comment #64)
> s/makes not/makes no/
> Also, add comma after "current interval", "redundant milestones", and
> "perfect".
Fixed.

> >diff --git a/content/smil/nsSMILTimedElement.h b/content/smil/nsSMILTimedElement.h
> >+  /**
> >+   * Called when the timed element has been bound to the document so that
> >+   * references.
> >+   */
> >+  void BindToTree();
> 
> Looks like this comment ends prematurely there. ("Called...so that
> references.")  Patch D completes this comment, so it ends up fine, but I'd
> rather we not have a broken comment in the intermediate state.
Yeah, this is an instance of something in Patch D being brought forward into
Patch B so that it's a bit more atomic. So this line should be replaced in Patch
D. I've just changed it to finish at "document".

> r=dholbert with those fixed
Thanks again!

Also, I've fixed the bitrot since bug 530372 was pushed.

Comment 66

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

Attachment: Patch D: Syncbase operation - v8 (r=dholbert)

Just rebasing off changes to Patch B part 2.

Comment 67

[Daniel Holbert [:dholbert]]

(In reply to comment #62)
> Assuming that this change fixes this warning, I'll file another bug on adding
> this #pragma to the other affected places

Filed Bug 537313 on that, btw.

Comment 68

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

Attachment: Patch B: Milestone sampling behaviour - v6 (r=dholbert)

Stepping through this code with the debugger I noticed a couple of minor bugs.

One relates to which version of SampleAt we call from Begin/EndElementAt. They were always meant to call DoSampleAt so that they behave as if we just reset the milestone in the time container.

Also, the static member sMaxMilestone should be const.

Comment 69

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

Attachment: Patch D: Syncbase operation - v9 (r=dholbert)

Rebase off changes to Patch B part 2.

Comment 70

[Daniel Holbert [:dholbert]]

Comment on attachment 419501 [details] [diff] [review]
Patch E: Test cases - v3

Looking through patch E (tests).  Not through it all the way yet, but here's what I've got so far:

Firstly, as discussed in IRC, it looks like the line-reordering in test_smilTiming.xhtml isn't actually needed.

Secondly, it looks like most of the mochitests include "is()" checks of the form:

>diff --git a/content/smil/test/test_smilCrossContainer.xhtml b/content/smil/test/test_smilCrossContainer.xhtml
>+  // Check state after attaching
>+  is(anim.getStartTime(), 2);
>+  is(circlea.cx.animVal.value, -20);
>+  is(circleb.cx.animVal.value, -20);

A lot of the mochitests have "is()" checks like this, with no message about the failure (the third argument).  When such checks fail, they result in cryptic messages like the one in bug 535850 comment 0:
>46416 ERROR TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_animLengthReadonly.xhtml | undefined - got 4.574999809265137, expected -100

From a glance at that failure, it's hard to track down what line actually failed.  You can get a hint by looking at the expected value, but in the quoted code above, there are multiple checks with the same expected value of "-20".

We need to add messages to these "is()" checks, so that if they do fail, we can narrow in on what check failed, without too much work.  These can be very simple, just something to identify the failure -- e.g. "start time shouldn't have changed", or something.

Comment 71

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

Attachment: Patch E: Test cases - v4

(In reply to comment #70)
> Firstly, as discussed in IRC, it looks like the line-reordering in
> test_smilTiming.xhtml isn't actually needed.
Fixed.

> A lot of the mochitests have "is()" checks like this, with no message about the
> failure (the third argument).
Fixed.

Posting this now even though I realise dholbert hasn't finished reviewing this patch but I'm not sure when I'll next get a chance to look at it.

Comment 72

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

patch B part 1:

Would it make sense to allow the comparator object to be the same as the comparator for nsTArray? I.e., adhering to
template<class A, class B>
class nsDefaultComparator {
  public:
    PRBool Equals(const A& a, const B& b) const {
      return a == b;
    }
    PRBool LessThan(const A& a, const B& b) const {
      return a < b;
    }
};

? Then you don't need to define nsMinComparator.

Otherwise looks great.

Comment 73

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

Part D:

+  PRBool ClearOnReset() const { return mFlags & kClearOnReset; }
+  PRBool MayUpdate() const { return mFlags & kMayUpdate; }
+  PRBool FromDOM() const { return mFlags & kFromDOM; }
 
These need to always return 0 or 1.

+    nsRefPtr<nsSMILInstanceTime> instance =
+      new nsSMILInstanceTime(mParams.mOffset, nsnull);
     mOwner->AddInstanceTime(instance, mIsBegin);

Null check?

+  // To avoid worst-case O(n^2) performance when many time dependents want to
+  // unregister, we keep these arrays sorted so we have worst case O(n*logn) for
+  // add and remove.
+  nsTArray<nsSMILTimeValueSpec*>  mTimeDependents;

I don't understand this. You still have worst case O(N^2) performance for adding or removing N items, right?

In nsSMILTimeValueSpec::ResolveReferences, instead of creating a new URI object for the base URI etc, I think we should just have an alternative method on nsReferencedElement, say ResetWithID(nsIContent* aFromContent, const nsString& aID, PRBool aWatch) where aID is just the ID (no hash) of an element in the same document.

Comment 74

[Daniel Holbert [:dholbert]]

(In reply to comment #73)
> +  nsTArray<nsSMILTimeValueSpec*>  mTimeDependents;
> I don't understand this. You still have worst case O(N^2) performance for
> adding or removing N items, right?

This is the issue mentioned in comment 43, which I'll be fixing in a followup patch by switching to use a hash-table.

Comment 75

[Daniel Holbert [:dholbert]]

Attachment: Patch D followup v1: Switch mTimeDependents to hash table

and here's that followup patch.

Comment 76

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

(In reply to comment #72)
> patch B part 1:
> 
> Would it make sense to allow the comparator object to be the same as the
> comparator for nsTArray? I.e., adhering to
> template<class A, class B>
> class nsDefaultComparator {
>   public:
>     PRBool Equals(const A& a, const B& b) const {
>       return a == b;
>     }
>     PRBool LessThan(const A& a, const B& b) const {
>       return a < b;
>     }
> };
> 
> ? Then you don't need to define nsMinComparator.

Yeah, I tried to do that but I have one concern about it. I thought nsTPriorityQueue should be generic enough to act as either a min-heap or max-heap. If we reuse nsDefaultComparator and you wanted to make a max-heap you'd have to write another comparator with a method called "LessThan" that actually performed a greater-than operation. Or at least I think so anyway. So I just made it take a generic 'Compare' functor instead.

Comment 77

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

Attachment: Patch D: Syncbase operation - v10 (r=dholbert)

(In reply to comment #73)
> Part D:
> 
> +  PRBool ClearOnReset() const { return mFlags & kClearOnReset; }
> +  PRBool MayUpdate() const { return mFlags & kMayUpdate; }
> +  PRBool FromDOM() const { return mFlags & kFromDOM; }
> 
> These need to always return 0 or 1.
Ok, fixed I think.

> +    nsRefPtr<nsSMILInstanceTime> instance =
> +      new nsSMILInstanceTime(mParams.mOffset, nsnull);
>      mOwner->AddInstanceTime(instance, mIsBegin);
> 
> Null check?
Oh yeah, thanks for that. Fixed.

> I don't understand this. You still have worst case O(N^2) performance for
> adding or removing N items, right?
Yeah, this is the same mistake as before and Daniel has fixed this in a separate patch.

> In nsSMILTimeValueSpec::ResolveReferences, instead of creating a new URI object
> for the base URI etc, I think we should just have an alternative method on
> nsReferencedElement, say ResetWithID(nsIContent* aFromContent, const nsString&
> aID, PRBool aWatch) where aID is just the ID (no hash) of an element in the
> same document.
Ok, I've had a go at doing that although there's a little bit of duplicated code. Let me know if it's what you had in mind.

Comment 78

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

Comment on attachment 419723 [details] [diff] [review]
Patch D followup v1: Switch mTimeDependents to hash table

Looks great Daniel. Thanks for that!

Comment 79

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

(In reply to comment #76)
> Yeah, I tried to do that but I have one concern about it. I thought
> nsTPriorityQueue should be generic enough to act as either a min-heap or
> max-heap. If we reuse nsDefaultComparator and you wanted to make a max-heap
> you'd have to write another comparator with a method called "LessThan" that
> actually performed a greater-than operation. Or at least I think so anyway.

I think that's OK. It's what you'd do already if you wanted to sort from greatest to least in nsTArray.

Comment 80

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

Comment on attachment 419723 [details] [diff] [review]
Patch D followup v1: Switch mTimeDependents to hash table

nsSMILTimedElement::SanityCheckTimeDependentCallbackArgs

Make this inline in the header file so it gets completely optimized out in opt builds.

Comment 81

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

Comment on attachment 419727 [details] [diff] [review]
Patch D: Syncbase operation - v10 (r=dholbert)

nsReferencedElement::ResetWithID

Add an XXX comment that we might need to do something for XBL/XBL2.

Comment 82

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

Attachment: Patch B part 1: nsTPriorityQueue v2 (r=dholbert)

(In reply to comment #79)
> (In reply to comment #76)
> > Yeah, I tried to do that but I have one concern about it. I thought
> > nsTPriorityQueue should be generic enough to act as either a min-heap or
> > max-heap. If we reuse nsDefaultComparator and you wanted to make a max-heap
> > you'd have to write another comparator with a method called "LessThan" that
> > actually performed a greater-than operation. Or at least I think so anyway.
> 
> I think that's OK. It's what you'd do already if you wanted to sort from
> greatest to least in nsTArray.

Ok, fixed.

Comment 83

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

Attachment: Patch D: Syncbase operation - v11 (r=dholbert, sr=roc)

(In reply to comment #81)
> Add an XXX comment that we might need to do something for XBL/XBL2.
Added.

Comment 84

[Daniel Holbert [:dholbert]]

Comment on attachment 419657 [details] [diff] [review]
Patch E: Test cases - v4

I've split off the tests for syncbase timing into bug 537852.  I reposted the latest version of "Patch E" there, and  I'll be posting my review feedback as followup patches there.

I'm obsoleting Patch E here, so that the tests are only tracked in one bug.  (The tests still need to land at roughly the same time as this bug's code-changes, though.)

Comment 85

[Benjamin Smedberg]

Comment on attachment 419730 [details] [diff] [review]
Patch B part 1: nsTPriorityQueue v2 (r=dholbert)

I wish we'd use mozilla::PriorityQueue instead, but I don't care enough to insist on it.

Please use /** * */ comments for doccomments instead of // comments.

sr=me with that changed

Comment 86

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

Please bear with me for a few more days to address final review comments and attempt to land this--I'm currently without SSH (and hg) access and once I do get it, I'll need to run a few tryserver builds.

Comment 87

[Daniel Holbert [:dholbert]]

Attachment: Patch D followup v2: Switch mTimeDependents to hash table (r=birtles, sr=roc)

(In reply to comment #80)
> nsSMILTimedElement::SanityCheckTimeDependentCallbackArgs
> Make this inline in the header file so it gets completely optimized out in opt
> builds.

Here's the "Patch D followup" with that fixed. (I fixed this in my patch queue a while back, but I just noticed that I hadn't reposted updated version til now)

Comment 88

[Daniel Holbert [:dholbert]]

Attachment: Prequel patch: nsTPriorityQueue v3 (p=birtles r=dholbert r=roc sr=bsmedberg)

(In reply to comment #85)
> Please use /** * */ comments for doccomments instead of // comments.
> 
> sr=me with that changed

Fixed on Brian's behalf (see bug 537852 comment 15-16).  I also ran this patch through the "dos2unix" tool, since I noticed it used dos-style line-endings in the added files. (I double-checked & found that the rest of the patches here are fine & use unix-style endings.)

Sine the nsTPriorityQueue patch is separate from the core syncbase implementation, I'm renaming this patch to "prequel" instead of "patch B part 1". I intend to land it (on Brian's behalf) before the rest of the patches here, perhaps even in a separate push, as Brian suggests in bug 537852 comment 16.

Comment 89

[Daniel Holbert [:dholbert]]

Comment on attachment 419636 [details] [diff] [review]
Patch B: Milestone sampling behaviour - v6 (r=dholbert)

(renaming "Patch B part 2" back to just "Patch B", per the end of pervious comment)

Comment 90

[Daniel Holbert [:dholbert]]

Landed!
nsTHashtable:     http://hg.mozilla.org/mozilla-central/rev/2a801a88b53b
Patch A:          http://hg.mozilla.org/mozilla-central/rev/567a4da0a3fc
Patch B:          http://hg.mozilla.org/mozilla-central/rev/6955702f91ac
Patch C:          http://hg.mozilla.org/mozilla-central/rev/192adf25beb3
Patch D:          http://hg.mozilla.org/mozilla-central/rev/05710a54577c
Patch D followup: http://hg.mozilla.org/mozilla-central/rev/f361c7470c3a

Landed bug 537852 (tests for this bug), too.

Comment 91

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

The newly added content/smil/nsSMILTimeValueSpecParams.h has "Portions created by the Initial Developer are Copyright (C) 2005". Have you been working on this for five years or is this just a typo?

Comment 92

[Daniel Holbert [:dholbert]]

That's a license-copying typo, I imagine.  I'll push a followup to fix that.

Comment 93

[Jeff Walden [:Waldo]]

Might it be really old bits from the original SVG animation work Back In The Day?  I think that was happening in the 2005 era or so.

Comment 94

[Daniel Holbert [:dholbert]]

correct, Brian began work on SMIL stuff in general around 2005.  After checking in IRC, I don't actually think it matters -- I'm not bothering to tweak it after all.

Comment 95

[Daniel Holbert [:dholbert]]

Pushed bustage fix for thunderbird:
http://hg.mozilla.org/mozilla-central/rev/2030568e5f9a
Sample failure log:
http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1263326603.1263327901.27947.gz

We need <stdio.h> in TestPriorityQueue.cpp for the printf usage. (though apparently mozilla-central magically didn't care)