542670 - Add equality operator for nsSMILValue

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

In bug 533291 comment 9, I note that we'll need to...
> check the base value from last sample's compositor against the base value from
> this samples compositor

To do that, we need to be able to compare two nsSMILValues for equality.  Filing this bug on that.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Patch A: Add method to interface, w/ conservative implementation

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 423891 [details] [diff] [review]
Patch A: Add method to interface, w/ conservative implementation

This first patch adds operator== and operator!= to nsSMILValue. These just check the object's address for equality, and if that doesn't match, they fall back on a nsISMILType helper method "TestForEquality".

So far, that helper method is extremely conservative -- it only has one implementation, inherited by all subclasses, which unconditionally returns PR_FALSE. (See its header comment in nsISMILType. This is the correct fallback behavior, so that bug 533291's optimization will only take effect if we're *sure* the base value hasn't changed.)

I'm adding smarter, per-subclass specializations of this method in a separate patch.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: Patch A v2: Add method to interface, w/ conservative implementation

This version just tweaks the header comment for nsISMILType::TestForEquality

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Patch B: Add specializations of TestForEquality in nsISMILType subclasses

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 423925 [details] [diff] [review]
Patch B: Add specializations of TestForEquality in nsISMILType subclasses

Note that Patch B removes the dummy "return PR_FALSE" implementation that Patch A provides in nsISMILType.h.  Patch A only includes this dummy implementation to allow you to compile (and get working behavior) with *just* Patch A applied.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Patch B v2: Add specializations of TestForEquality in nsISMILType subclasses

(this version just removes an accidentally-added comment that's unrelated to this patch, & removes a period at end of an added warning message.)

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 423924 [details] [diff] [review]
Patch A v2: Add method to interface, w/ conservative implementation

+  return mType == aVal.mType ?
+    mType->TestForEquality(*this, aVal) : PR_FALSE;

Use && instead of ?:

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh, I think TestForEquality would be better named IsEqual.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: Patch A v3: Add method to interface, w/ conservative implementation [r=roc]

(In reply to comment #7)
> Use && instead of ?:

Fixed.

(In reply to comment #8)
> Oh, I think TestForEquality would be better named IsEqual.

Fixed.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: Patch B v3: Add specializations of TestForEquality in nsISMILType subclasses [r=roc]

Fixed patch B to use "IsEqual" method name.

Also fixed a recursive death-spiral in "nsSVGSMILTransform::operator!=" -- previous version had that method calling itself recursively instead of calling !operator==().  (I caught that with a run through mochitests with this plus the not-yet-posted base-value-caching patch for bug 533291.)

Carrying forward r=roc

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: Patch A v4: Add method to interface, w/ conservative implementation [r=roc]

Rebased Patch A to apply on top of Bug 542731 Patches A-D.  (Just fixing bitrot in context)

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: Patch B v4: Add specializations of TestForEquality in nsISMILType subclasses [r=roc]

Rebased Patch B to apply on top of Bug 542731 Patches A-D.

In particular, I've shifted some code-cleanup in nsSVGTransformSMILAttr from this patch into Patch C on Bug 542731 -- so, this version no longer applies that cleanup.

Comment 13

[Daniel Holbert [:dholbert]]

Landed:
http://hg.mozilla.org/mozilla-central/rev/da48199647d0
http://hg.mozilla.org/mozilla-central/rev/a3cd5f10742b

The latter patch included some (trivial) new IsEqual implementations for a few nsISMILType subclasses that have recently appeared on trunk: SMILIntegerType, SVGOrientSMILType, and SVGViewBoxSMILType (along with nsSVGViewBoxRect::operator= to support SVGViewBoxSMILType)