Closed
Bug 682184
Opened 13 years ago
Closed 13 years ago
SVG SMIL: Rename nsSMILTimeValue accessors so IsResolved means is resolved
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: birtles, Assigned: birtles)
Details
Attachments
(2 files, 2 obsolete files)
26.40 KB,
patch
|
birtles
:
review+
|
Details | Diff | Splinter Review |
12.88 KB,
patch
|
birtles
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
Basically just a s/IsResolved/IsDefinite/ plus a few doc tweaks.
Attachment #558380 -
Flags: review?(dholbert)
Assignee | ||
Comment 2•13 years ago
|
||
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)
Assignee | ||
Updated•13 years ago
|
Attachment #558381 -
Attachment description: Part 2 - Add new nsSMILTimeValue::IsResolved → Part 2 - Add new nsSMILTimeValue::IsResolved v1a
Comment 3•13 years ago
|
||
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+
Comment 4•13 years ago
|
||
(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•13 years ago
|
||
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+
Assignee | ||
Comment 6•13 years ago
|
||
Attachment #558380 -
Attachment is obsolete: true
Attachment #558665 -
Flags: review+
Assignee | ||
Comment 7•13 years ago
|
||
Attachment #558381 -
Attachment is obsolete: true
Attachment #558666 -
Flags: review+
Assignee | ||
Comment 8•13 years ago
|
||
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•13 years ago
|
||
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!
Assignee | ||
Comment 10•13 years ago
|
||
Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/0eb03db3606b http://hg.mozilla.org/integration/mozilla-inbound/rev/da3c0a9bd688
Whiteboard: [inbound]
Comment 11•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/0eb03db3606b http://hg.mozilla.org/mozilla-central/rev/da3c0a9bd688
Status: ASSIGNED → RESOLVED
Closed: 13 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.
Description
•