My concern is that this will get partially done but never completed, and we'll end up in a situation where we have two styles in place. We have a bad track record with that sort of thing.
Nick On Thu, Feb 9, 2017 at 1:45 PM, Jeff Muizelaar <jrmui...@gmail.com> wrote: > It's not very easy to reason about overflow issues with our current > representation. This means that we currently just pretend that they > don't happen. The idea for changing the representation came up in > response to a security bug where we didn't really have a better > solution. > > Changing to x1, x2, y1, y2 will allow us to match the pixman rectangle > representation that we use for regions. The region code shows quite a > bit profiles so even a small improvement in this area is nice to have. > > -Jeff > > On Wed, Feb 8, 2017 at 9:19 PM, David Major <dma...@mozilla.com> wrote: > > Is there a specific problem that's being solved by this proposal? It > would > > be helpful to make this a bit more concrete, like "these benchmarks go x% > > faster", or "here's a list of overflow bugs that will just vanish", or > > "here's some upcoming work that this would facilitate". > > > > On Thu, Feb 9, 2017 at 1:56 PM, Botond Ballo <bba...@mozilla.com> wrote: > >> > >> Hi everyone! > >> > >> I would like to propose changing the internal representation of > >> rectangles in platform code. > >> > >> We currently represent a rectangle by storing the coordinates of its > >> top-left corner, its width, and its height. I'll refer to this > >> representation as "x/y/w/h". > >> > >> I would like to propose storing instead the coordinates of the > >> top-left corner, and the coordinates of the bottom-right corner. I'll > >> refer to this representation as "x1/y1/x2/y2". > >> > >> The x1/y1/x2/y2 representation has several advantages over x/y/w/h: > >> > >> - Several operations are more efficient with x1/y1/x2/y2, including > >> intersection, > >> union, and point-in-rect. > >> - The representation is more symmetric, since it stores two quantities > >> of the > >> same kind (two points) rather than a point and a dimension > >> (width/height). > >> - The representation is less susceptible to overflow. With x/y/w/h, > >> computation > >> of x2/y2 can overflow for a large range of values of x/y and w/h. > >> However, > >> with x1/y1/x2/y2, computation of w/h cannot overflow if the > >> coordinates are > >> signed and the resulting w/h is unsigned. > >> > >> A known disadvantage of x1/y1/x2/y2 is that translating the rectangle > >> requires translating both points, whereas translating x/y/w/h only > >> requires translating one point. I think this disadvantage is minor in > >> comparison to the above advantages. > >> > >> The proposed change would affect the class mozilla::gfx::BaseRect, and > >> classes that derive from it (such as CSSRect, LayoutRect, etc., and, > >> notably, nsRect and nsIntRect), but NOT other rectangle classes like > >> DOMRect. > >> > >> I would like to propose making the transition as follows: > >> > >> - Replace direct accesses to the 'width' and 'height' fields > throughout > >> the codebase with calls to getter and setter methods. (There aren't > >> that many - on the order of a few dozen, last I checked.) > >> > >> - Make the representation change, which is non-breaking now that > >> the direct accesses to 'width' and 'height' have been removed. > >> > >> - Examine remaining calls to the getters and setters for width and > >> height and see if any can be better expressed using operations > >> on the points instead. > >> > >> The Graphics team, which owns this code, is supportive of this change. > >> However, since this is a fundamental utility type that's used by a > >> variety of platform code, I would like to ask the wider platform > >> development community for feedback before proceeding. Please let me > >> know if you have any! > >> > >> Thanks, > >> Botond > >> > >> [1] > >> http://searchfox.org/mozilla-central/rev/672c83ed65da286b68be1d02799c35 > fdd14d0134/gfx/2d/BaseRect.h#46 > >> _______________________________________________ > >> dev-platform mailing list > >> dev-platform@lists.mozilla.org > >> https://lists.mozilla.org/listinfo/dev-platform > > > > > _______________________________________________ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform > _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
