1353208 - Tidy up SMIL base value handling code

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Description

[Brian Birtles (:birtles)]

Spinning this off from bug 1315874 to cover the various tidy up patches there.

Comment 1

[Brian Birtles (:birtles)]

Attachment: Bug 1353208 - Drop a few remaining references to attributeType;

These are left over from bug 1062106.

Review commit: https://reviewboard.mozilla.org/r/126246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126246/

Comment 2

[Brian Birtles (:birtles)]

Attachment: Bug 1353208 - Sort includes in nsSMILAnimationController.cpp;

We will add to these includes later in this patch series so we sort them now so
it's clear where to add to.

Review commit: https://reviewboard.mozilla.org/r/126248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126248/

Comment 3

[Brian Birtles (:birtles)]

Attachment: Bug 1353208 - Use UniquePtr for handling heap-allocated nsISMILAttr objects;

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.

Review commit: https://reviewboard.mozilla.org/r/126250/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126250/

Comment 4

[Brian Birtles (:birtles)]

Attachment: Bug 1353208 - Don't allocate separate heap memory for nsSMILCompositor::mCachedBaseValue;

nsSMILCompositor::mCachedBaseValue is an nsAutoPtr<nsSMILValue> that we allocate
on the heap. It looks like we did that back in bug 533291 presumably because it
makes transferring these cached values between nsSMILCompositor objects cheaper.

One drawback of this, however, is that mCachedBaseValue has two null states:
the mCachedBaseValue pointer can be null, and the pointed-to nsSMILValue can be
a null value (i.e. IsNull() returns true).

Now that we have move ctors and operators defined for nsSMILValue we can
transfer these objects between compositors cheaply without requiring the object
to be allocated as separate heap object. This patch makes mCachedBaseValue just
a regular nsSMILValue class member (i.e. drops the nsAutoPtr).

There's a subtle difference in behavior with regards to the first sample.
Previously we would compare the (initially) null mCachedBaseValue pointer with
the passed-in nsSMILValue and set mForceCompositing to true. With this patch,
however, we will only set mForceCompositing to true if the passed-in
mCachedBaseValue is not null.

I believe this is correct, however, since if we don't call GetBaseValue in
ComposeAttribute we should not be setting mForceCompositing to true (something
else should ensure that gets set to true), and if we do call GetBaseValue the
result should not be a null nsSMILValue (except in some OOM cases where we don't
really care if we miss a sample). This patch adds an assertion to check that
GetBaseValue does, in fact, return a non-null value. (I checked the code and
this appears to be the case. Even in error cases we typically return an empty
nsSMILValue of a non-null type. For example, the early return in
nsSMILCSSProperty::GetBaseValue() does this.)

Review commit: https://reviewboard.mozilla.org/r/126252/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126252/

Comment 5

[Brian Birtles (:birtles)]

Attachment: Bug 1353208 - Factor out nsSMILCompositor::GetCSSPropertyToAnimate helper method;

In bug 1315874 we will create a method to check if we're likely to need to
resolve base styles or not, and for that we need to extract the check for
whether or not we're animating a CSS property.

Review commit: https://reviewboard.mozilla.org/r/126254/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126254/

Comment 6

[Brian Birtles (:birtles)]

Attachment: Bug 1353208 - Simplify logic in GetCSSPropertyToAnimate to remove double negatives;

Review commit: https://reviewboard.mozilla.org/r/126256/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126256/

Comment 7

[Brian Birtles (:birtles)]

Attachment: Bug 1353208 - Add check for attribute namespace ID when we decide if it should animate as a CSS property or not;

This seems like an existing bug. If the content specifies
attributeType="yer:opacity", we should not try to animate it as a CSS property.
This patch adds a namespace check before we try to animate as a CSS property.

Review commit: https://reviewboard.mozilla.org/r/126258/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126258/

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8854313 [details]
Bug 1353208 - Sort includes in nsSMILAnimationController.cpp;

https://reviewboard.mozilla.org/r/126248/#review128926

Comment 9

[Daniel Holbert [:dholbert]]

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.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8854315 [details]
Bug 1353208 - Don't allocate separate heap memory for nsSMILCompositor::mCachedBaseValue;

https://reviewboard.mozilla.org/r/126252/#review128956

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8854316 [details]
Bug 1353208 - Factor out nsSMILCompositor::GetCSSPropertyToAnimate helper method;

https://reviewboard.mozilla.org/r/126254/#review128960

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8854317 [details]
Bug 1353208 - Simplify logic in GetCSSPropertyToAnimate to remove double negatives;

https://reviewboard.mozilla.org/r/126256/#review128964

Comment 13

[Daniel Holbert [:dholbert]]

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

Comment 14

[Brian Birtles (:birtles)]

Comment on attachment 8854312 [details]
Bug 1353208 - Drop a few remaining references to attributeType;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126246/diff/1-2/

Comment 15

[Brian Birtles (:birtles)]

Comment on attachment 8854313 [details]
Bug 1353208 - Sort includes in nsSMILAnimationController.cpp;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126248/diff/1-2/

Comment 16

[Brian Birtles (:birtles)]

Comment on attachment 8854314 [details]
Bug 1353208 - Use UniquePtr for handling heap-allocated nsISMILAttr objects;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126250/diff/1-2/

Comment 17

[Brian Birtles (:birtles)]

Comment on attachment 8854315 [details]
Bug 1353208 - Don't allocate separate heap memory for nsSMILCompositor::mCachedBaseValue;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126252/diff/1-2/

Comment 18

[Brian Birtles (:birtles)]

Comment on attachment 8854316 [details]
Bug 1353208 - Factor out nsSMILCompositor::GetCSSPropertyToAnimate helper method;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126254/diff/1-2/

Comment 19

[Brian Birtles (:birtles)]

Comment on attachment 8854317 [details]
Bug 1353208 - Simplify logic in GetCSSPropertyToAnimate to remove double negatives;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126256/diff/1-2/

Comment 20

[Brian Birtles (:birtles)]

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;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126258/diff/1-2/

Comment 21

[Hiroyuki Ikezoe (:hiro)]

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/.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Mantaroh ^ ?  If there are still the references we should drop them in a new bug.

Comment 23

[Brian Birtles (:birtles)]

(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.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
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

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c8006685a3b4
https://hg.mozilla.org/mozilla-central/rev/7d1366650614
https://hg.mozilla.org/mozilla-central/rev/7788981a1e02
https://hg.mozilla.org/mozilla-central/rev/ad7060dae117
https://hg.mozilla.org/mozilla-central/rev/0d77b2992fc8
https://hg.mozilla.org/mozilla-central/rev/d3d051929c1e
https://hg.mozilla.org/mozilla-central/rev/95e21766cd72