Closed Bug 1353208 Opened 3 years ago Closed 3 years ago

Tidy up SMIL base value handling code

Categories

(Core :: SVG, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(7 files)

Spinning this off from bug 1315874 to cover the various tidy up patches there.
Comment on attachment 8854313 [details]
Bug 1353208 - Sort includes in nsSMILAnimationController.cpp;

https://reviewboard.mozilla.org/r/126248/#review128926
Attachment #8854313 - Flags: review?(dholbert) → review+
Comment on attachment 8854314 [details]
Bug 1353208 - Use UniquePtr for handling heap-allocated nsISMILAttr objects;

https://reviewboard.mozilla.org/r/126250/#review128952

::: commit-message-0e748:3
(Diff revision 1)
> +Later in this patch series we will add another call site for CreateSMILAttr so
> +this seems like a good opportunity to convert this to UniquePtr which should be
> +safer than passing back raw pointers the caller needs to free.

Nit: this part of the extended-commit-message is no longer true (if "this patch series" means "this bug's patch stack").

This sentence was useful context originally, but now I think it can just be dropped. The patch is useful cleanup, worth doing on its own -- no need for extra justification based on future patches.
Attachment #8854314 - Flags: review?(dholbert) → review+
Comment on attachment 8854315 [details]
Bug 1353208 - Don't allocate separate heap memory for nsSMILCompositor::mCachedBaseValue;

https://reviewboard.mozilla.org/r/126252/#review128956
Attachment #8854315 - Flags: review?(dholbert) → review+
Comment on attachment 8854316 [details]
Bug 1353208 - Factor out nsSMILCompositor::GetCSSPropertyToAnimate helper method;

https://reviewboard.mozilla.org/r/126254/#review128960
Attachment #8854316 - Flags: review?(dholbert) → review+
Comment on attachment 8854317 [details]
Bug 1353208 - Simplify logic in GetCSSPropertyToAnimate to remove double negatives;

https://reviewboard.mozilla.org/r/126256/#review128964
Attachment #8854317 - Flags: review?(dholbert) → review+
Comment on attachment 8854318 [details]
Bug 1353208 - Add check for attribute namespace ID when we decide if it should animate as a CSS property or not;

https://reviewboard.mozilla.org/r/126258/#review128976

r=me with nits

::: commit-message-0bc94:1
(Diff revision 1)
> +Bug 1353208 - Add check for attribute namespace ID when decided if it should animate as a CSS property or not; r?dholbert
> +
> +This seems like an existing bug. If the content specifies
> +attributeType="yer:opacity", we should not try to animate it as a CSS property.

Two things:
(1) s/when decided/when deciding/  (or perhaps "when we decide"? the current "when decided" doesn't make sense in my head)

(2) As you note, this is fixing an existing bug.  Could you include an automated testcase for this behavior-change?  Maybe just a modified copy of some preexisting reftest in layout/reftests/svg/smil/style
Attachment #8854318 - Flags: review?(dholbert) → review+
Attachment #8854312 - Flags: review?(mantaroh) → review?(hikezoe)
Comment on attachment 8854312 [details]
Bug 1353208 - Drop a few remaining references to attributeType;

https://reviewboard.mozilla.org/r/126246/#review129198

It seems to me that thre are other remaining references to attributeType in test files in dom/smil/test/.
Attachment #8854312 - Flags: review?(hikezoe) → review+
Mantaroh ^ ?  If there are still the references we should drop them in a new bug.
Flags: needinfo?(mantaroh)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #21)
> Comment on attachment 8854312 [details]
> Bug 1353208 - Drop a few remaining references to attributeType;
> 
> https://reviewboard.mozilla.org/r/126246/#review129198
> 
> It seems to me that thre are other remaining references to attributeType in
> test files in dom/smil/test/.

Yeah, there are lots. I don't know that there's a lot of value in removing them from the tests, but there are also references in the HTML parser that seem worth removing -- I'm just not sure if that's safe to do or if there would be observable changes from doing that. If we did that, though, we could probably remove the entry from nsGkAtoms too. Anyway, that's another bug. I just made this change since I was looking at updating SVGAnimationElement::ParseAttribute but in the end I didn't need to.
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c8006685a3b4
Drop a few remaining references to attributeType; r=hiro
https://hg.mozilla.org/integration/autoland/rev/7d1366650614
Sort includes in nsSMILAnimationController.cpp; r=dholbert
https://hg.mozilla.org/integration/autoland/rev/7788981a1e02
Use UniquePtr for handling heap-allocated nsISMILAttr objects; r=dholbert
https://hg.mozilla.org/integration/autoland/rev/ad7060dae117
Don't allocate separate heap memory for nsSMILCompositor::mCachedBaseValue; r=dholbert
https://hg.mozilla.org/integration/autoland/rev/0d77b2992fc8
Factor out nsSMILCompositor::GetCSSPropertyToAnimate helper method; r=dholbert
https://hg.mozilla.org/integration/autoland/rev/d3d051929c1e
Simplify logic in GetCSSPropertyToAnimate to remove double negatives; r=dholbert
https://hg.mozilla.org/integration/autoland/rev/95e21766cd72
Add check for attribute namespace ID when we decide if it should animate as a CSS property or not; r=dholbert
Depends on: 1411963
Flags: needinfo?(mantaroh)
You need to log in before you can comment on or make changes to this bug.