Last Comment Bug 682184 - SVG SMIL: Rename nsSMILTimeValue accessors so IsResolved means is resolved
: SVG SMIL: Rename nsSMILTimeValue accessors so IsResolved means is resolved
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Brian Birtles (:birtles)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-25 19:37 PDT by Brian Birtles (:birtles)
Modified: 2011-09-07 07:58 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - Rename nsSMILTimeValue::IsResolved to IsDefinite v1a (26.33 KB, patch)
2011-09-05 20:24 PDT, Brian Birtles (:birtles)
dholbert: review+
Details | Diff | Review
Part 2 - Add new nsSMILTimeValue::IsResolved v1a (12.87 KB, patch)
2011-09-05 20:26 PDT, Brian Birtles (:birtles)
dholbert: review+
Details | Diff | Review
Part 1 - Rename nsSMILTimeValue::IsResolved to IsDefinite v1b, r=dholbert (26.40 KB, patch)
2011-09-06 16:55 PDT, Brian Birtles (:birtles)
bbirtles: review+
Details | Diff | Review
Part 2 - Add new nsSMILTimeValue::IsResolved v1b, r=dholbert (12.88 KB, patch)
2011-09-06 16:55 PDT, Brian Birtles (:birtles)
bbirtles: review+
Details | Diff | Review

Description Brian Birtles (:birtles) 2011-08-25 19:37:52 PDT
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) 2011-09-05 20:24:47 PDT
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.
Comment 2 Brian Birtles (:birtles) 2011-09-05 20:26:27 PDT
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.
Comment 3 Daniel Holbert [:dholbert] 2011-09-06 01:26:19 PDT
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] 2011-09-06 01:27:54 PDT
(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] 2011-09-06 01:34:15 PDT
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) 2011-09-06 16:55:06 PDT
Created attachment 558665 [details] [diff] [review]
Part 1 - Rename nsSMILTimeValue::IsResolved to IsDefinite v1b, r=dholbert
Comment 7 Brian Birtles (:birtles) 2011-09-06 16:55:41 PDT
Created attachment 558666 [details] [diff] [review]
Part 2 - Add new nsSMILTimeValue::IsResolved v1b, r=dholbert
Comment 8 Brian Birtles (:birtles) 2011-09-06 16:57:16 PDT
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] 2011-09-06 17:07:49 PDT
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!

Note You need to log in before you can comment on or make changes to this bug.