542731 - Code cleanup in /content/smil

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Been meaning to some misc code cleanup /content/smil. I'll post anything non-trivial here for review.

Comment 1

[Daniel Holbert [:dholbert]]

Landed two trivial cleanup patches:

* Remove trailing "." from warning messages, to prevent ".:" in e.g. the first assertion in bug 526536 comment 0.
   http://hg.mozilla.org/mozilla-central/rev/cff84777576d

* Remove end-of-line whitespace
    http://hg.mozilla.org/mozilla-central/rev/49b45b82988f

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: patch A: clean up nsSMILValue [landed]

This bug does some misc. cleanup in nsSMILValue, namely:
 - Remove unnecessary default-initialization of union "mU()" in constructor's init lists.
 - Factor out helper functions to do "Init + verify postcondition", "Destroy + verify postcondition", "Destroy + init (verifying postconditions)", to remove lots of duplicated code and make the main methods more straightforward.
 - Add newline before return in e.g. "if (foo) return;"
 - Be stricter about a few things:
   * Print a NS_ERROR if we try to construct nsSMILValue with nsnull type-pointer. (should never happen)
   * Remove special case for adding "nsSMILNullType"-typed nsSMILValues to other values. (Until now, this was allowed and treated like adding 0, but we don't actually need it.  In the other mathematical methods -- ComputeDistance/Interpolate -- we enforce type-matching, and we should do the same in the Addition methods, both to catch bugs & to remove a runtime type-check that we don't actually need.)
   * Convert NS_POSTCONDITIONS to NS_ABORT_IF_FALSE so they'll be more likely to get noticed if we ever start failing them. (The warning messages still mentions "post-condition", so the semantics are still clear.)

I ran this through try-server to make sure that the stricter rules don't break anything, and it passed.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 423988 [details] [diff] [review]
patch A: clean up nsSMILValue [landed]

Landed patch A:
http://hg.mozilla.org/mozilla-central/rev/a8810d63a74a

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Patch B: Clean up nsSMILCSSProperty & nsSMILCSSValueType [landed]

Since the "nsISMILType" methods can potentially allocate/destroy/copy memory, and tweak nsSMILValue.mType, I think we should lock down those methods as protected, and only allow the nsSMILValue API to access them (by making it a friend class).

The nsISMILType documentation already indicates this:
> 50 // This interface is never used directly but always through an nsSMILValue that
> 51 // bundles together a pointer to a concrete implementation of this interface and
> 52 // the data upon which it should operate.
... but it's not enforced yet.

In order to enforce it, we first need to remove some direct calls to nsISMILType::Init()/Destroy() within nsSMILCSSProperty and nsSVGTransformSMILAttr (shifting that functionality into their nsISMILType methods).

This patch does that for nsSMILCSSProperty, and does some other misc cleanup in surrounding code.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to comment #4)
> In order to enforce it, we first need to remove some direct calls to
> nsISMILType::Init()/Destroy() within nsSMILCSSProperty and
> nsSVGTransformSMILAttr (shifting that functionality into their nsISMILType
> methods).

er, I meant "shifting that functionality into methods on the corresponding nsISMILType subclass"

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Patch C: Clean up nsSVGTransformSMIL(Attr/Type) classes [landed]

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: Patch D: Lock down nsISMILType API [landed]

This patch locks down nsISMILType interface & its subclasses, as described at beginning of comment 4.  Namely:
 - It makes all the nsISMILType methods protected
 - It leaves the subclasses' singletons & any static helper-methods as public
 - It adds private constructor/destructor in every nsISMILType subclass, to prevent any instances aside from the singleton, and to prevent singletons from being accidentally destroyed
 - It removes 'virtual' from nsISMILType destructor, since none of these classes have any member data to clean up, and none can be destructed via an abstract nsISMILType pointer anyway, which means there's no need for the virtual destructor.

Comment 8

[Daniel Holbert [:dholbert]]

...and it makes nsSMILValue a friend class, of course.

Comment 9

[Daniel Holbert [:dholbert]]

Landed patches B, C, D:
http://hg.mozilla.org/mozilla-central/rev/0a80ce4e6692
http://hg.mozilla.org/mozilla-central/rev/5f645dd4152a
http://hg.mozilla.org/mozilla-central/rev/d9afac0e2458

I made one fix to Patch C before landing, fixing a typo that I caught from reftest failures on tryserver.

In this chunk...

> nsSVGTransformSMILAttr::ParseValue(const nsAString& aSpec,
[...]
>   nsSVGSMILTransform::TransformType transformType;
[...]
>-  return type->AppendTransform(nsSVGSMILTransform(transformType, params),
>-                               aResult);
>+  nsSMILValue val(&nsSVGTransformSMILType::sSingleton);
>+  nsSVGSMILTransform transform(transformType, params);
>+  if (NS_FAILED(nsSVGTransformSMILType::AppendTransform(transformType, val))) {

..the last line there is supposed to be |transform|, not |transformType|.  It seems kinda broken that it even compiled before, since there's no version of "AppendTransform" that takes a TransformType argument.  It looks like it was just automatically creating a temporary nsSVGSMILTransform object to pass in for that argument, using the nsSVGSMILTransform constructor that takes a TransformType.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to comment #9)
> It looks like it was
> just automatically creating a temporary nsSVGSMILTransform object to pass in
> for that argument, using the nsSVGSMILTransform constructor that takes a
> TransformType.

Ah, right -- we need an "explicit" keyword on that constructor in order to catch those sorts of bugs as compiler errors.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: Patch E: Add "explicit" label to single-arg constructors [landed]

This patch adds the "explicit" keyword to all single-argument constructors in our SMIL code (excluding copy constructors), to prevent typos/bugs like the one described in the previous two comments here.

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 424714 [details] [diff] [review]
Patch E: Add "explicit" label to single-arg constructors [landed]

Landed Patch E: http://hg.mozilla.org/mozilla-central/rev/80619dcc4f9c

Comment 13

[Daniel Holbert [:dholbert]]

Closing this bug. Future cleanup can go on future bugs.