As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
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, away most of Jan 21 - Feb 1)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-25 19:37 PDT by Brian Birtles (:birtles, away most of Jan 21 - Feb 1)
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, away most of Jan 21 - Feb 1)
dholbert: review+
Details | Diff | Splinter Review
Part 2 - Add new nsSMILTimeValue::IsResolved v1a (12.87 KB, patch)
2011-09-05 20:26 PDT, Brian Birtles (:birtles, away most of Jan 21 - Feb 1)
dholbert: review+
Details | Diff | Splinter Review
Part 1 - Rename nsSMILTimeValue::IsResolved to IsDefinite v1b, r=dholbert (26.40 KB, patch)
2011-09-06 16:55 PDT, Brian Birtles (:birtles, away most of Jan 21 - Feb 1)
bbirtles: review+
Details | Diff | Splinter Review
Part 2 - Add new nsSMILTimeValue::IsResolved v1b, r=dholbert (12.88 KB, patch)
2011-09-06 16:55 PDT, Brian Birtles (:birtles, away most of Jan 21 - Feb 1)
bbirtles: review+
Details | Diff | Splinter Review

Description User image Brian Birtles (:birtles, away most of Jan 21 - Feb 1) 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 User image Brian Birtles (:birtles, away most of Jan 21 - Feb 1) 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 User image Brian Birtles (:birtles, away most of Jan 21 - Feb 1) 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 User image 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 User image 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 User image 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 User image Brian Birtles (:birtles, away most of Jan 21 - Feb 1) 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 User image Brian Birtles (:birtles, away most of Jan 21 - Feb 1) 2011-09-06 16:55:41 PDT
Created attachment 558666 [details] [diff] [review]
Part 2 - Add new nsSMILTimeValue::IsResolved v1b, r=dholbert
Comment 8 User image Brian Birtles (:birtles, away most of Jan 21 - Feb 1) 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 User image 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!
Comment 10 User image Brian Birtles (:birtles, away most of Jan 21 - Feb 1) 2011-09-06 17:26:32 PDT
Pushed to m-i:
http://hg.mozilla.org/integration/mozilla-inbound/rev/0eb03db3606b
http://hg.mozilla.org/integration/mozilla-inbound/rev/da3c0a9bd688

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