Reducing overflow risk and simplifying intersection both seem worth it, but godspeed whoever works on this!
On Fri, Feb 10, 2017 at 12:45 PM, Milan Sreckovic <msrecko...@mozilla.com> wrote: > First step needs to happen completely before the second step does, so I > guess the danger is that we start and give up before we get to step 2. I > don't think that will happen, but it is something we should always think > about. > > Third step - sure, I can see this not getting completed - examining places > that do xmax = xmin + width instead of just getting the max X, for example. > > > On 09-Feb-17 20:56, Nicholas Nethercote wrote: >> >> 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 > > > -- > - Milan (mi...@mozilla.com) > > > _______________________________________________ > 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
