955854 - Remove unused colour space mapping code and move the rest to nsSVGMaskFrame

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: patch

Since we converted filters to Moz2d, masks are the only callers of the colour space mapping code in nsSVGUtils. This patch removes everything that masks don't use and moves that which it does into the nsSVGMaskFrame itself.

Comment 1

[Robert Longson [:longsonr]]

Attachment: move more code to nsSVGMaskFrame

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8355012 [details] [diff] [review]
move more code to nsSVGMaskFrame

Looks good!

Just one nit:

>   if (StyleSVGReset()->mMaskType == NS_STYLE_MASK_TYPE_LUMINANCE) {
>     if (StyleSVG()->mColorInterpolation ==
>         NS_STYLE_COLOR_INTERPOLATION_LINEARRGB) {
>-      nsSVGUtils::ComputeLinearRGBLuminanceMask(data, stride, rect, aOpacity);
>+      ComputeLinearRGBLuminance(data, stride, rect, aOpacity);
>     } else {
>-      nsSVGUtils::ComputesRGBLuminanceMask(data, stride, rect, aOpacity);
>+      ComputesRGBLuminance(data, stride, rect, aOpacity);
>     }
>   } else {
>-    nsSVGUtils::ComputeAlphaMask(data, stride, rect, aOpacity);
>+    ComputeAlpha(data, stride, rect, aOpacity);
>   }

Looks like you're renaming these functions to drop the "Mask" suffix -- why? That suffix seems useful from a clarity perspective.

The new name "ComputeAlpha" sounds like a function that computes an *actual* alpha value across a grid of pixels, and similarly Compute*Luminance sounds like it's computing an actual luminance value.  But these methods aren't computing alpha or luminance values; they're computing masks which are then used for determining those things (IIUC).

r=me regardless (but I lean a bit towards keeping the Mask suffixes)

Comment 3

[Robert Longson [:longsonr]]

I don't mind adding it back. It just seemed superfluous given that they are private to a file named mask now.

Comment 4

[Daniel Holbert [:dholbert]]

Understood. I still think they add clarity, per e.g. the easy misunderstanding of what "ComputeAlpha" might do (even in a file with Mask in the name), as noted above.

Comment 5

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6900b944694d

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6900b944694d