Status

()

defect
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

Assignee

Description

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

Comment 1

10 years ago
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
Assignee

Comment 2

10 years ago
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.
Attachment #423988 - Flags: review?(roc)
Assignee

Comment 3

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

Landed patch A:
http://hg.mozilla.org/mozilla-central/rev/a8810d63a74a
Attachment #423988 - Attachment description: patch A: clean up nsSMILValue → patch A: clean up nsSMILValue [landed]
Assignee

Comment 4

10 years ago
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.
Assignee

Comment 5

10 years ago
(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"
Assignee

Updated

10 years ago
Attachment #424341 - Flags: review?(roc)
Assignee

Comment 7

10 years ago
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.
Attachment #424346 - Flags: review?
Assignee

Comment 8

10 years ago
...and it makes nsSMILValue a friend class, of course.
Assignee

Updated

10 years ago
Attachment #424346 - Flags: review? → review?(roc)
Assignee

Comment 9

10 years ago
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.
Assignee

Updated

10 years ago
Attachment #424341 - Attachment description: Patch B: Clean up nsSMILCSSProperty & nsSMILCSSValueType → Patch B: Clean up nsSMILCSSProperty & nsSMILCSSValueType [landed]
Assignee

Updated

10 years ago
Attachment #424342 - Attachment description: Patch C: Clean up nsSVGTransformSMIL[Attr/Type] classes → Patch C: Clean up nsSVGTransformSMIL(Attr/Type) classes [landed]
Assignee

Updated

10 years ago
Attachment #424346 - Attachment description: Patch D: Lock down nsISMILType API → Patch D: Lock down nsISMILType API [landed]
Assignee

Comment 10

10 years ago
(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.
Assignee

Comment 11

10 years ago
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.
Attachment #424714 - Flags: review?(roc)
Assignee

Updated

10 years ago
Attachment #424714 - Attachment description: Patch E: Add explicit label to single-arg constructors → Patch E: Add "explicit" label to single-arg constructors
Assignee

Comment 12

10 years ago
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
Attachment #424714 - Attachment description: Patch E: Add "explicit" label to single-arg constructors → Patch E: Add "explicit" label to single-arg constructors [landed]
Assignee

Comment 13

9 years ago
Closing this bug. Future cleanup can go on future bugs.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.