1049949 - Make it impossible to construct invalid SVGPreserveAspectRatio values

Status: NEW

(Core :: SVG, defect)

Description

[Seth Fowler [:seth] [:s2h]]

Right now it's possible to construct an SVGPreserveAspectRatio value that has SVG_PRESERVEASPECTRATIO_UNKNOWN for mAlign and SVG_MEETORSLICE_UNKNOWN for mMeetOrSlice by using the default constructor, or by passing these values to a different constructor explicitly.

The problem is that the preserveAspectRatio-related code does not actually check for and handle these values. In bug 1043560 I considered adding explicit support for them and using them in the drawing code (they would just mean "don't change anything"), but after discussing the matter with Daniel we concluded that it would be better just to remove the UNKNOWN values. I removed them from my patch in bug 1043560, but it took only a very short time before someone else tried to use them in bug 1028288.

I think these values are footguns that people are going to keep hitting until we remove them.

Comment 1

[Robert Longson [:longsonr]]

They come from the SVG specification. http://www.w3.org/TR/SVG/coords.html#InterfaceSVGPreserveAspectRatio

Comment 2

[Seth Fowler [:seth] [:s2h]]

Thanks Robert. Those values seemed like sentinels, and I could be wrong but I don't think it occurred to anyone I've discussed this with so far that they were standardized.

I think we still need to do something here, though. Having the default constructor produce _UNKNOWN values - error values, in other words - has consequences that people who don't regularly work with this code don't expect. What it sounds to me like we need to do is to remove the default constructor. Callers that actually use these values should use the explicit constructor. People expect a default constructor to produce a reasonable default, and none of the standard values are a reasonable default in practice.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Seth Fowler [:seth] from comment #2)
> Thanks Robert. Those values seemed like sentinels, and I could be wrong but
> I don't think it occurred to anyone I've discussed this with so far that
> they were standardized.

[/me raises hand]

At that point, I'd done a quick MXR search for it and noticed it being hardly being used / checked for anywhere, and I was concerned about giving it extra significance as a "sentinel" value in imagelib, as discussed in bug 1043560 comment 52.

I agree that getting rid of the default constructor here might be wise, though I'm not sure how often there will be cause for people to try to incorrectly use the default constructor, once SVGImageContext takes a Maybe<> value.  Then, we won't have to worry quite as much about people accidentally passing a default-constructed SVGPreserveAspectRatio there (the problematic thing that you pointed out in bug 1028288 comment 37), since it won't be "the obvious easy thing to do" anymore.

Comment 4

[Daniel Holbert [:dholbert]]

("at that point" = "when I discussed this with seth a week or so ago")