987432 - Clean up possibly-unnecessary float casts in SVG matrix-creation code (maybe just replacing with gfx::Float)

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

While reviewing Bug 984278 part 2 (attachment 8393317 [details] [diff] [review]), I noticed some unnecessary-looking float casts, e.g.:

> already_AddRefed<SVGMatrix>
> SVGMatrix::SkewX(float angle, ErrorResult& rv)
> {
[...]
>-  const gfxMatrix& mx = GetMatrix();
>-  gfxMatrix skewMx(mx.xx, mx.yx,
>-                   (float) (mx.xy + mx.xx*ta), (float) (mx.yy + mx.yx*ta),
>-                   mx.x0, mx.y0);
>+  const Matrix& mx = GetMatrix();
>+  Matrix skewMx(mx._11, mx._12,
>+                (float) (mx._21 + mx._11*ta), (float) (mx._22 + mx._12*ta),
>+                 mx._31, mx._32);

Note that in the old code with gfxMatrix, the cast was undesirable (losing precision unnecessarily), since gfxMatrix's constructor takes gfxFloat which is a double.

It looks like this has been the case ever since we switched from nsSVGMatrix to gfxMatrix in Bug 602759 -- the float-casts are left over from calls to NS_NewSVGMatrix:
 http://hg.mozilla.org/mozilla-central/rev/38e6f7980e5e#l1.64

Now with bug 984278 landing, the cast might actually be necessary again, since the Moz2D Matrix class uses gfx::Float which seems to actually be "float".  And in this case, "ta" is a double, so we do need to downconvert the result of arithmetic that involves "ta".

However, we should at least downconvert using the correct typename ("gxf::Float"), rather than simply "float", so that if we redefine gfx::Float later, this continues to behave as we'd like it to.

Comment 1

[Daniel Holbert [:dholbert]]

Marking as depending on bug 984278, since IIUC, using "gfx::Float" will only become correct after that bug is completed.

Comment 2

[Daniel Holbert [:dholbert]]

(Alternately, we might even just be able to ditch these explicit casts and just let the double-->float conversion happen automatically; but I seem to recall MSVC warning about that sort of thing, which is probably why we added these back in the pre-bug-602759 days.)

Comment 3

[Robert Longson [:longsonr]]

MSVC does warn.

Comment 4

[Daniel Holbert [:dholbert]]

OK. Assuming that this code is approximately as it was in comment 0, then, I think we should be casting to the (typedef-defined) type that the function actually expects (which seems to be gfx::Float).

Comment 5

[Daniel Holbert [:dholbert]]

(...though I suppose Bug 984278 part 2 (and its patch-chunk quoted in comment 0) hasn't landed yet, so we probably shouldn't do this yet, to avoid bitrotting that patch.)

Comment 6

[Robert Longson [:longsonr]]

Attachment: Bug 987432 - remove unnecessary float casts in SkewX and SkewY

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7efd94e5f609
remove unnecessary float casts in SkewX and SkewY r=dholbert

Comment 8

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/7efd94e5f609