SVG SMIL: Rename nsSMILTimeValue accessors so IsResolved means is resolved

RESOLVED FIXED in mozilla9

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

Trunk
mozilla9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
Created attachment 558380 [details] [diff] [review]
Part 1 - Rename nsSMILTimeValue::IsResolved to IsDefinite v1a

Basically just a s/IsResolved/IsDefinite/ plus a few doc tweaks.
Attachment #558380 - Flags: review?(dholbert)
Created attachment 558381 [details] [diff] [review]
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.
Attachment #558381 - Flags: review?(dholbert)
Attachment #558381 - Attachment description: Part 2 - Add new nsSMILTimeValue::IsResolved → Part 2 - Add new nsSMILTimeValue::IsResolved v1a
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.
Attachment #558380 - Flags: review?(dholbert) → review+
(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 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.
Attachment #558381 - Flags: review?(dholbert) → review+
Created attachment 558665 [details] [diff] [review]
Part 1 - Rename nsSMILTimeValue::IsResolved to IsDefinite v1b, r=dholbert
Attachment #558380 - Attachment is obsolete: true
Attachment #558665 - Flags: review+
Created attachment 558666 [details] [diff] [review]
Part 2 - Add new nsSMILTimeValue::IsResolved v1b, r=dholbert
Attachment #558381 - Attachment is obsolete: true
Attachment #558666 - Flags: review+
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 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!
Pushed to m-i:
http://hg.mozilla.org/integration/mozilla-inbound/rev/0eb03db3606b
http://hg.mozilla.org/integration/mozilla-inbound/rev/da3c0a9bd688
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/0eb03db3606b
http://hg.mozilla.org/mozilla-central/rev/da3c0a9bd688
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.