682184 - SVG SMIL: Rename nsSMILTimeValue accessors so IsResolved means is resolved

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles)]

nsSMILTimeValue.h tracks tri-state times which can be:

* resolved with a millisecond time
* indefinite
* unresolved

The indefinite case is a bit confusing. Really, it is a resolved time of indefinite length but for convenience I originally decided to make nsSMILTimeValue::IsResolved return PR_FALSE for indefinite times. This is well-documented in nsSMILTimeValue.h but looking back I think this is just confusing.

I propose adding a new method: HasMillisecondValue? IsDefiniteTime? or something of the sort and making IsResolved return PR_TRUE for both indefinite and "resolved with millisecond time" cases.

Comment 1

[Brian Birtles (:birtles)]

Attachment: Part 1 - Rename nsSMILTimeValue::IsResolved to IsDefinite v1a

Basically just a s/IsResolved/IsDefinite/ plus a few doc tweaks.

Comment 2

[Brian Birtles (:birtles)]

Attachment: Part 2 - Add new nsSMILTimeValue::IsResolved v1a

Now that nsSMILTimeValue::IsResolved is gone, add a new one that actually means "is resolved" and replace "IsDefinite() || IsIndefinite()" with "IsResolved()". Also further doc tweaks.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 558380 [details] [diff] [review]
Part 1 - Rename nsSMILTimeValue::IsResolved to IsDefinite v1a

>+++ b/content/smil/nsSMILInterval.cpp
>@@ -105,17 +105,17 @@ nsSMILInterval::End()
> void
> nsSMILInterval::SetBegin(nsSMILInstanceTime& aBegin)
> {
>-  NS_ABORT_IF_FALSE(aBegin.Time().IsResolved(),
>+  NS_ABORT_IF_FALSE(aBegin.Time().IsDefinite(),
>       "Attempting to set unresolved begin time on interval");

Assertion message doesn't agree with what we're checking here.  (This was a problem before this patch, but it's more clearly a problem with the new function name.)  Can you fix whichever one is wrong, to make them consistent?

>@@ -525,13 +525,13 @@ nsSMILTimeValueSpec::ConvertBetweenTimeC
>   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(),
>+   NS_ABORT_IF_FALSE(docTime.IsDefinite(),
>        "ContainerToParentTime gave us an unresolved time");

The last 2 lines here are indented 1 space too far -- could you fix that, while you're touching this chunk?

r=dholbert with the above.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> Assertion message doesn't agree with what we're checking here.  (This was a
> problem before this patch, but it's more clearly a problem with the new
> function name.)  Can you fix whichever one is wrong, to make them consistent?

Ah, nevermind about this part - you've already got a fix for it in the second patch. :)

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 558381 [details] [diff] [review]
Part 2 - Add new nsSMILTimeValue::IsResolved v1a

>@@ -992,17 +992,17 @@ nsSMILTimedElement::SetMax(const nsAStri
>-  if (NS_FAILED(rv) || (!duration.IsDefinite() && !duration.IsIndefinite())) {
>+  if (NS_FAILED(rv) || (!duration.IsResolved())) {
>     mMax.SetIndefinite();
>     return NS_ERROR_FAILURE;
>   }

The extra parens make this harder to read - s/(!duration.IsResolved())/!duration.IsResolved()/

r=dholbert with that.

Comment 6

[Brian Birtles (:birtles)]

Attachment: Part 1 - Rename nsSMILTimeValue::IsResolved to IsDefinite v1b, r=dholbert

Comment 7

[Brian Birtles (:birtles)]

Attachment: Part 2 - Add new nsSMILTimeValue::IsResolved v1b, r=dholbert

Comment 8

[Brian Birtles (:birtles)]

Addressed review feedback.

(In reply to Daniel Holbert [:dholbert] from comment #4)
> (In reply to Daniel Holbert [:dholbert] from comment #3)
> > Assertion message doesn't agree with what we're checking here.  (This was a
> > problem before this patch, but it's more clearly a problem with the new
> > function name.)  Can you fix whichever one is wrong, to make them consistent?
> 
> Ah, nevermind about this part - you've already got a fix for it in the
> second patch. :)

Sorry, my bad for not being entirely consistent with where the doc fixes went.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 558665 [details] [diff] [review]
Part 1 - Rename nsSMILTimeValue::IsResolved to IsDefinite v1b, r=dholbert

>   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");
>+   NS_ABORT_IF_FALSE(docTime.IsDefinite(),
>+     "ContainerToParentTime gave us an unresolved time");
> 
>   return dstContainer->ParentToContainerTime(docTime.GetMillis());

Sorry, I think I wasn't particularly clear about this chunk before -- I was pointing out that the NS_ABORT_IF_FALSE is indented 1 space more than the "if" check and the "return". Specifically, it's indented by 3 spaces when it should be indented by 2.

(Only pointing it out because I initially thought (confusedly) it was part of the "if" clause above it, due to its extra indentation.)

Definitely not a big deal, but it'd be great if you could fix it before landing.  Thanks!

Comment 10

[Brian Birtles (:birtles)]

Pushed to m-i:
http://hg.mozilla.org/integration/mozilla-inbound/rev/0eb03db3606b
http://hg.mozilla.org/integration/mozilla-inbound/rev/da3c0a9bd688

Comment 11

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/0eb03db3606b
http://hg.mozilla.org/mozilla-central/rev/da3c0a9bd688