Currently we have gfxSize, gfxIntSize, nsSize, nsIntSize gfxPoint, nsIntPoint, nsPoint gfxRect, nsIntRect, nsRect These are all independent types and they have different APIs. I find this a real pain when working in our code. I want to do the following: 1) Create a templated Size type with the union of the APIs on the existing *Size types, and define each *Size type as an instance of that template 2) Make gfxIntSize a typedef for nsIntSize 3) Create a templated Point type with the union of the APIs on the existing *Point types, and define each *Point type as an instance of that template 4) Create a templated Rect type with the union of the APIs on the existing *Rect types, and define each *Rect type as an instance of that template gfx* types instantiate the template with gfxFloat, nsInt* types instantiate with PRInt32, and ns* types instantiate with nscoord. The trickiest part is that gfxRect contains 'pos' and 'size' members and ns(Int)Rect contains 'x', 'y', 'width' and 'height' members. I think the best way to reconcile these is to use x/y/width/height and turn accesses to 'pos' into TopLeft() and accesses to 'size' into Size(). There are about 200 accesses to each of those members. I have a working patch for parts 1-3 already.

\o/ I would like to tie units on these basic types, so that rects etc. might be Rect<int, AppUnit> or Rect<float, CssPixel> (with a set of conversion functions), but that can be built on top of what you're doing. > I think the best > way to reconcile these is to use x/y/width/height and turn accesses to 'pos' > into TopLeft() and accesses to 'size' into Size(). Agreed.

"Empty" is ambiguous as to whether it's a getter or a setter.

These patches are showing green on try.

A bit of explanation: In most cases I just merged the gfx* and ns* APIs. In part 8 I removed some of that duplication by getting rid of Outset/Inset. There were some edge cases where the API behaviours differed. In particular the handling of empty rects for Intersect and Union is a bit arbitrary. I chose the behaviours that are needed by some weird parts of our code. The behavior of Contains(point) is a bit questionable. gfxRect::Contains returns true for a point exactly on the bottom or right edge of the rectangle, which is sort of reasonable for a floating-point rect but a) breaks code that thinks of that point as a 1x1 pixel square, e.g. mouse hit testing and b) gfxRect::Contains isn't used anywhere. So I've defined Contains to return false for that case. There are still two options for defining it: 1) returns true if rect contains (x,y,1,1) 2) returns true if rect contains any nonempty rectangle whose (top,left) is the given point I chose #1 because it seems a bit simpler, but #2 would be a bit more logical and avoid using 1 as a magic value for width/height. I could go either way on this.

Comment on attachment 523258 [details] [diff] [review] Part 3: Convert gfxRect::pos/size to x/y/width/height Yay. I'm not a huge fan of X()/Y()/Width()/Height(), but they're not hurting anybody and maybe we'll find a use for them in <algorithm>s or something.

Yeah, we can remove them in a followup, but they are fairly harmless.

Comment on attachment 523264 [details] [diff] [review] Bug 641426. Part 8: Replace gfxRect::Outset/Inset with Inflate/Deflate. Also slip in a conversion constructor from nsIntRect to gfxRect @@ -899,17 +900,18 @@ nsCSSBorderRenderer::DrawBorderSides(PRI for (unsigned int i = 0; i < borderColorStyleCount; i++) { // walk siRect inwards at the start of the loop to get the // correct inner rect. - siRect.Inset(borderWidths[i]); + siRect.Deflate(gfxMargin(borderWidths[0][3], borderWidths[0][0], + borderWidths[0][1], borderWidths[0][2])); I haven't read this code in detail yet, but is this the change you want?

Yes.

Comment on attachment 523254 [details] [diff] [review] Part 1: Create Point and Size templates s/GenericRect/BaseRect/ per discussion. point + size -> point doesn't make sense to me. point + size -> rect might make sense, per discussion, but for existing callers, I think we should either fix them to use point + point -> point, or if there are a lot of users of point+size, file a followup on removing point+size. Style issues - do we still need/want VERIFY_COORD? If not, now seems like a good time to kill it. If so, it should probably go in a traits-class parameter to the template, and the traits can be shared across Point/Rect. - template<class T, class C> --- it's not immediately obvious what "C" is. "T" is conventional. Maybe "Sub" instead of "C"?

Comment on attachment 523261 [details] [diff] [review] Part 5: Avoid operator== where possible to distinguish between 'equal edges' and 'equal areas' for rectangles Looks good. I think that the different interpretations of x,y,w,h deserve a comment somewhere prominent. Instead of IsEqualEdges/IsEqualInterior, which sound a bit awkward to me, how about HasSameEdges/HasSameInterior?

(In reply to comment #15) > Comment on attachment 523264 [details] [diff] [review] > Bug 641426. Part 8: Replace gfxRect::Outset/Inset with Inflate/Deflate. Also > slip in a conversion constructor from nsIntRect to gfxRect > > @@ -899,17 +900,18 @@ nsCSSBorderRenderer::DrawBorderSides(PRI > for (unsigned int i = 0; i < borderColorStyleCount; i++) { > // walk siRect inwards at the start of the loop to get the > // correct inner rect. > - siRect.Inset(borderWidths[i]); > + siRect.Deflate(gfxMargin(borderWidths[0][3], borderWidths[0][0], > + borderWidths[0][1], borderWidths[0][2])); Ok, then I don't understand why the first index is always 0 and not i.

That is a bug!

(Will fix)

(In reply to comment #18) > Instead of IsEqualEdges/IsEqualInterior, which sound a bit awkward to me, how > about HasSameEdges/HasSameInterior? I think I prefer IsEqualEdges actually.

(In reply to comment #18) > I think that the different interpretations of x,y,w,h deserve a comment > somewhere prominent. I'll put that in BaseRect.

Comment on attachment 524575 [details] [diff] [review] Part 1 v2 >+template <class T, class Sub> struct BasePoint { >+template <class T, class Sub> struct BaseSize { Short comment on each of these describing Sub and expected use (not direct) would be nice. Are you going to post Margin/Rect v2's? I expected a new round of all.

Comment on attachment 524575 [details] [diff] [review] Part 1 v2 >diff --git a/gfx/src/BasePoint.h b/gfx/src/BasePoint.h >+ * The Initial Developer of the Original Code is Mozilla Foundation. >+ * Portions created by the Initial Developer are Copyright (Sub) 2011 "(Sub)" ? I think you went a little search-and-replace happy :) >+template <class T, class Sub> struct BasePoint { Should be formatted more like: template <class T, class Sub> struct BasePoint { This goes along better with our template usage in xpcom. You should do this throughout. >diff --git a/gfx/src/BaseSize.h b/gfx/src/BaseSize.h >+ Sub& operator+=(const Sub& aSize) { >+ width += aSize.width; >+ height += aSize.height; >+ return *static_cast<Sub*>(this); This took me a while to work out. I think you should include a comment at the top of the .h files here to say how Base{Size,Point} should be used so it's more obvious that static_cast<Sub *>(this) is meaningful. >diff --git a/gfx/src/Makefile.in b/gfx/src/Makefile.in > EXPORTS = \ >+ BasePoint.h \ >+ BaseSize.h \ These should probably be exported to mozilla/, no? >diff --git a/gfx/src/nsPoint.h b/gfx/src/nsPoint.h >--- a/gfx/src/nsPoint.h >+++ b/gfx/src/nsPoint.h >@@ -34,99 +34,41 @@ > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > #ifndef NSPOINT_H > #define NSPOINT_H > > #include "nsCoord.h" >+#include "BaseSize.h" >+#include "BasePoint.h" >+#include "nsSize.h" > > struct nsIntPoint; > >-struct nsPoint { >- nscoord x, y; >+struct nsPoint : public mozilla::BasePoint<nscoord, nsPoint> { >+ typedef mozilla::BasePoint<nscoord, nsPoint> Super; > >- // Constructors >- nsPoint() {} >- nsPoint(const nsPoint& aPoint) { x = aPoint.x; y = aPoint.y;} >- nsPoint(nscoord aX, nscoord aY) { VERIFY_COORD(aX); VERIFY_COORD(aY); x = aX; y = aY;} >- >- void MoveTo(nscoord aX, nscoord aY) {x = aX; y = aY;} >- void MoveBy(nscoord aDx, nscoord aDy) {x += aDx; y += aDy;} >- >- // Overloaded operators. Note that '=' isn't defined so we'll get the >- // compiler generated default assignment operator >- PRBool operator==(const nsPoint& aPoint) const { >- return (PRBool) ((x == aPoint.x) && (y == aPoint.y)); >- } >- PRBool operator!=(const nsPoint& aPoint) const { >- return (PRBool) ((x != aPoint.x) || (y != aPoint.y)); >- } >- nsPoint operator+(const nsPoint& aPoint) const { >- return nsPoint(x + aPoint.x, y + aPoint.y); >- } >- nsPoint operator-(const nsPoint& aPoint) const { >- return nsPoint(x - aPoint.x, y - aPoint.y); >- } >- nsPoint& operator+=(const nsPoint& aPoint) { >- x += aPoint.x; >- y += aPoint.y; >- return *this; >- } >- nsPoint& operator-=(const nsPoint& aPoint) { >- x -= aPoint.x; >- y -= aPoint.y; >- return *this; >- } >- >- nsPoint operator-() const { >- return nsPoint(-x, -y); >- } >+ nsPoint() : Super() {} >+ nsPoint(const nsPoint& aPoint) : Super(aPoint) {} >+ nsPoint(nscoord aX, nscoord aY) : Super(aX, aY) {} > > inline nsIntPoint ToNearestPixels(nscoord aAppUnitsPerPixel) const; > > // Converts this point from aFromAPP, an appunits per pixel ratio, to aToAPP. > inline nsPoint ConvertAppUnits(PRInt32 aFromAPP, PRInt32 aToAPP) const; > }; > >-struct nsIntPoint { >- PRInt32 x, y; >+struct nsIntPoint : public mozilla::BasePoint<PRInt32, nsIntPoint> { >+ typedef mozilla::BasePoint<PRInt32, nsIntPoint> Super; > >- // Constructors >- nsIntPoint() {} >- nsIntPoint(const nsIntPoint& aPoint) { x = aPoint.x; y = aPoint.y;} >- nsIntPoint(PRInt32 aX, PRInt32 aY) { x = aX; y = aY;} >- >- PRBool operator==(const nsIntPoint& aPoint) const { >- return (PRBool) ((x == aPoint.x) && (y == aPoint.y)); >- } >- PRBool operator!=(const nsIntPoint& aPoint) const { >- return (PRBool) ((x != aPoint.x) || (y != aPoint.y)); >- } >- nsIntPoint operator+(const nsIntPoint& aPoint) const { >- return nsIntPoint(x + aPoint.x, y + aPoint.y); >- } >- nsIntPoint operator-(const nsIntPoint& aPoint) const { >- return nsIntPoint(x - aPoint.x, y - aPoint.y); >- } >- nsIntPoint& operator+=(const nsIntPoint& aPoint) { >- x += aPoint.x; >- y += aPoint.y; >- return *this; >- } >- nsIntPoint& operator-=(const nsIntPoint& aPoint) { >- x -= aPoint.x; >- y -= aPoint.y; >- return *this; >- } >- nsIntPoint operator-() const { >- return nsIntPoint(-x, -y); >- } >- void MoveTo(PRInt32 aX, PRInt32 aY) {x = aX; y = aY;} >+ nsIntPoint() : Super() {} >+ nsIntPoint(const nsIntPoint& aPoint) : Super(aPoint) {} >+ nsIntPoint(PRInt32 aX, PRInt32 aY) : Super(aX, aY) {} > }; > > inline nsIntPoint > nsPoint::ToNearestPixels(nscoord aAppUnitsPerPixel) const { > return nsIntPoint( > NSToIntRoundUp(NSAppUnitsToDoublePixels(x, aAppUnitsPerPixel)), > NSToIntRoundUp(NSAppUnitsToDoublePixels(y, aAppUnitsPerPixel))); > } >diff --git a/gfx/src/nsSize.h b/gfx/src/nsSize.h >--- a/gfx/src/nsSize.h >+++ b/gfx/src/nsSize.h >@@ -34,75 +34,38 @@ > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > #ifndef NSSIZE_H > #define NSSIZE_H > > #include "nsCoord.h" >+#include "BaseSize.h" > > // Maximum allowable size > #define NS_MAXSIZE nscoord_MAX > >-struct nsSize { >- nscoord width, height; >+struct nsSize : public mozilla::BaseSize<nscoord, nsSize> { >+ typedef mozilla::BaseSize<nscoord, nsSize> Super; > >- // Constructors >- nsSize() {} >- nsSize(const nsSize& aSize) {width = aSize.width; height = aSize.height;} >- nsSize(nscoord aWidth, nscoord aHeight) {width = aWidth; height = aHeight;} >- >- void SizeTo(nscoord aWidth, nscoord aHeight) {width = aWidth; height = aHeight;} >- void SizeBy(nscoord aDeltaWidth, nscoord aDeltaHeight) {width += aDeltaWidth; >- height += aDeltaHeight;} >- >- // Overloaded operators. Note that '=' isn't defined so we'll get the >- // compiler generated default assignment operator >- PRBool operator==(const nsSize& aSize) const { >- return (PRBool) ((width == aSize.width) && (height == aSize.height)); >- } >- PRBool operator!=(const nsSize& aSize) const { >- return (PRBool) ((width != aSize.width) || (height != aSize.height)); >- } >- nsSize operator+(const nsSize& aSize) const { >- return nsSize(width + aSize.width, height + aSize.height); >- } >- nsSize& operator+=(const nsSize& aSize) {width += aSize.width; >- height += aSize.height; >- return *this;} >+ nsSize() : Super() {} >+ nsSize(const nsSize& aSize) : Super(aSize) {} >+ nsSize(nscoord aWidth, nscoord aHeight) : Super(aWidth, aHeight) {} > > // Converts this size from aFromAPP, an appunits per pixel ratio, to aToAPP. > inline nsSize ConvertAppUnits(PRInt32 aFromAPP, PRInt32 aToAPP) const; > }; > >-struct nsIntSize { >- PRInt32 width, height; >+struct nsIntSize : public mozilla::BaseSize<PRInt32, nsIntSize> { >+ typedef mozilla::BaseSize<PRInt32, nsIntSize> Super; > >- nsIntSize() {} >- nsIntSize(const nsIntSize& aSize) {width = aSize.width; height = aSize.height;} >- nsIntSize(PRInt32 aWidth, PRInt32 aHeight) {width = aWidth; height = aHeight;} >- >- // Overloaded operators. Note that '=' isn't defined so we'll get the >- // compiler generated default assignment operator >- PRBool operator==(const nsIntSize& aSize) const { >- return (PRBool) ((width == aSize.width) && (height == aSize.height)); >- } >- PRBool operator!=(const nsIntSize& aSize) const { >- return (PRBool) ((width != aSize.width) || (height != aSize.height)); >- } >- PRBool operator<(const nsIntSize& aSize) const { >- return (PRBool) (operator<=(aSize) && >- (width < aSize.width || height < aSize.height)); >- } >- PRBool operator<=(const nsIntSize& aSize) const { >- return (PRBool) ((width <= aSize.width) && (height <= aSize.height)); >- } >- >- void SizeTo(PRInt32 aWidth, PRInt32 aHeight) {width = aWidth; height = aHeight;} >+ nsIntSize() : Super() {} >+ nsIntSize(const nsIntSize& aSize) : Super(aSize) {} >+ nsIntSize(PRInt32 aWidth, PRInt32 aHeight) : Super(aWidth, aHeight) {} > }; > > inline nsSize > nsSize::ConvertAppUnits(PRInt32 aFromAPP, PRInt32 aToAPP) const { > if (aFromAPP != aToAPP) { > nsSize size; > size.width = NSToCoordRound(NSCoordScale(width, aFromAPP, aToAPP)); > size.height = NSToCoordRound(NSCoordScale(height, aFromAPP, aToAPP)); >diff --git a/gfx/thebes/gfxContext.cpp b/gfx/thebes/gfxContext.cpp >--- a/gfx/thebes/gfxContext.cpp >+++ b/gfx/thebes/gfxContext.cpp >@@ -249,17 +249,17 @@ gfxContext::Rectangle(const gfxRect& rec > > cairo_rectangle(mCairo, rect.pos.x, rect.pos.y, rect.size.width, rect.size.height); > } > > void > gfxContext::Ellipse(const gfxPoint& center, const gfxSize& dimensions) > { > gfxSize halfDim = dimensions / 2.0; >- gfxRect r(center - halfDim, dimensions); >+ gfxRect r(center - gfxPoint(halfDim.width, halfDim.height), dimensions); > gfxCornerSizes c(halfDim, halfDim, halfDim, halfDim); > > RoundedRectangle (r, c); > } > > void > gfxContext::Polygon(const gfxPoint *points, PRUint32 numPoints) > { >@@ -425,19 +425,19 @@ gfxContext::UserToDevicePixelSnapped(gfx > cairo_matrix_t mat; > cairo_get_matrix(mCairo, &mat); > if (!ignoreScale && > (!WITHIN_E(mat.xx,1.0) || !WITHIN_E(mat.yy,1.0) || > !WITHIN_E(mat.xy,0.0) || !WITHIN_E(mat.yx,0.0))) > return PR_FALSE; > #undef WITHIN_E > >- gfxPoint p1 = UserToDevice(rect.pos); >- gfxPoint p2 = UserToDevice(rect.pos + gfxSize(rect.size.width, 0.0)); >- gfxPoint p3 = UserToDevice(rect.pos + rect.size); >+ gfxPoint p1 = UserToDevice(rect.TopLeft()); >+ gfxPoint p2 = UserToDevice(rect.TopRight()); >+ gfxPoint p3 = UserToDevice(rect.BottomRight()); > > // Check that the rectangle is axis-aligned. For an axis-aligned rectangle, > // two opposite corners define the entire rectangle. So check if > // the axis-aligned rectangle with opposite corners p1 and p3 > // define an axis-aligned rectangle whose other corners are p2 and p4. > // We actually only need to check one of p2 and p4, since an affine > // transform maps parallelograms to parallelograms. > if (p2 == gfxPoint(p1.x, p3.y) || p2 == gfxPoint(p3.x, p1.y)) { >diff --git a/gfx/thebes/gfxPoint.h b/gfx/thebes/gfxPoint.h >--- a/gfx/thebes/gfxPoint.h >+++ b/gfx/thebes/gfxPoint.h >@@ -34,135 +34,40 @@ > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > #ifndef GFX_POINT_H > #define GFX_POINT_H > > #include "nsMathUtils.h" >+#include "BaseSize.h" >+#include "BasePoint.h" >+#include "nsSize.h" >+#include "nsPoint.h" > > #include "gfxTypes.h" > >-/* >- * gfxSize and gfxIntSize -- please keep their member functions in sync. >- * also note: gfxIntSize may be replaced by nsIntSize at some point... >- */ >-struct THEBES_API gfxIntSize { >- PRInt32 width, height; >+typedef nsIntSize gfxIntSize; > >- gfxIntSize() {} >- gfxIntSize(PRInt32 _width, PRInt32 _height) : width(_width), height(_height) {} >+struct THEBES_API gfxSize : public mozilla::BaseSize<gfxFloat, gfxSize> { >+ typedef mozilla::BaseSize<gfxFloat, gfxSize> Super; > >- void SizeTo(PRInt32 _width, PRInt32 _height) {width = _width; height = _height;} >- >- int operator==(const gfxIntSize& s) const { >- return ((width == s.width) && (height == s.height)); >- } >- int operator!=(const gfxIntSize& s) const { >- return ((width != s.width) || (height != s.height)); >- } >- bool operator<(const gfxIntSize& s) const { >- return (operator<=(s) && >- (width < s.width || height < s.height)); >- } >- bool operator<=(const gfxIntSize& s) const { >- return (width <= s.width) && (height <= s.height); >- } >- gfxIntSize operator+(const gfxIntSize& s) const { >- return gfxIntSize(width + s.width, height + s.height); >- } >- gfxIntSize operator-() const { >- return gfxIntSize(- width, - height); >- } >- gfxIntSize operator-(const gfxIntSize& s) const { >- return gfxIntSize(width - s.width, height - s.height); >- } >- gfxIntSize operator*(const PRInt32 v) const { >- return gfxIntSize(width * v, height * v); >- } >- gfxIntSize operator/(const PRInt32 v) const { >- return gfxIntSize(width / v, height / v); >- } >+ gfxSize() : Super() {} >+ gfxSize(gfxFloat aWidth, gfxFloat aHeight) : Super(aWidth, aHeight) {} >+ gfxSize(const nsIntSize& aSize) : Super(aSize.width, aSize.height) {} > }; > >-struct THEBES_API gfxSize { >- gfxFloat width, height; >+struct THEBES_API gfxPoint : public mozilla::BasePoint<gfxFloat, gfxPoint> { >+ typedef mozilla::BasePoint<gfxFloat, gfxPoint> Super; > >- gfxSize() {} >- gfxSize(gfxFloat _width, gfxFloat _height) : width(_width), height(_height) {} >- gfxSize(const gfxIntSize& size) : width(size.width), height(size.height) {} >+ gfxPoint() : Super() {} >+ gfxPoint(gfxFloat aX, gfxFloat aY) : Super(aX, aY) {} >+ gfxPoint(const nsIntPoint& aPoint) : Super(aPoint.x, aPoint.y) {} > >- void SizeTo(gfxFloat _width, gfxFloat _height) {width = _width; height = _height;} >- >- int operator==(const gfxSize& s) const { >- return ((width == s.width) && (height == s.height)); >- } >- int operator!=(const gfxSize& s) const { >- return ((width != s.width) || (height != s.height)); >- } >- gfxSize operator+(const gfxSize& s) const { >- return gfxSize(width + s.width, height + s.height); >- } >- gfxSize operator-() const { >- return gfxSize(- width, - height); >- } >- gfxSize operator-(const gfxSize& s) const { >- return gfxSize(width - s.width, height - s.height); >- } >- gfxSize operator*(const gfxFloat v) const { >- return gfxSize(width * v, height * v); >- } >- gfxSize operator/(const gfxFloat v) const { >- return gfxSize(width / v, height / v); >- } >-}; >- >- >- >-struct THEBES_API gfxPoint { >- gfxFloat x, y; >- >- gfxPoint() { } >- gfxPoint(gfxFloat _x, gfxFloat _y) : x(_x), y(_y) {} >- >- void MoveTo(gfxFloat aX, gfxFloat aY) { x = aX; y = aY; } >- >- int operator==(const gfxPoint& p) const { >- return ((x == p.x) && (y == p.y)); >- } >- int operator!=(const gfxPoint& p) const { >- return ((x != p.x) || (y != p.y)); >- } >- const gfxPoint& operator+=(const gfxPoint& p) { >- x += p.x; >- y += p.y; >- return *this; >- } >- gfxPoint operator+(const gfxPoint& p) const { >- return gfxPoint(x + p.x, y + p.y); >- } >- gfxPoint operator+(const gfxSize& s) const { >- return gfxPoint(x + s.width, y + s.height); >- } >- gfxPoint operator-(const gfxPoint& p) const { >- return gfxPoint(x - p.x, y - p.y); >- } >- gfxPoint operator-(const gfxSize& s) const { >- return gfxPoint(x - s.width, y - s.height); >- } >- gfxPoint operator-() const { >- return gfxPoint(- x, - y); >- } >- gfxPoint operator*(const gfxFloat v) const { >- return gfxPoint(x * v, y * v); >- } >- gfxPoint operator/(const gfxFloat v) const { >- return gfxPoint(x / v, y / v); >- } > // Round() is *not* rounding to nearest integer if the values are negative. > // They are always rounding as floor(n + 0.5). > // See https://bugzilla.mozilla.org/show_bug.cgi?id=410748#c14 > // And if you need similar method which is using NS_round(), you should > // create new |RoundAwayFromZero()| method. > gfxPoint& Round() { > x = NS_floor(x + 0.5); > y = NS_floor(y + 0.5); >diff --git a/gfx/thebes/gfxRect.h b/gfx/thebes/gfxRect.h >--- a/gfx/thebes/gfxRect.h >+++ b/gfx/thebes/gfxRect.h >@@ -187,19 +187,20 @@ struct THEBES_API gfxRect { > void RoundIn(); > > // Snap the rectangle edges to integer coordinates, such that the > // resulting rectangle contains the original rectangle. > void RoundOut(); > > // grabbing specific points > gfxPoint TopLeft() const { return gfxPoint(pos); } >- gfxPoint TopRight() const { return pos + gfxSize(size.width, 0.0); } >- gfxPoint BottomLeft() const { return pos + gfxSize(0.0, size.height); } >- gfxPoint BottomRight() const { return pos + size; } >+ gfxPoint TopRight() const { return pos + gfxPoint(size.width, 0.0); } >+ gfxPoint BottomLeft() const { return pos + gfxPoint(0.0, size.height); } >+ gfxPoint BottomRight() const { return pos + gfxPoint(size.width, size.height); } >+ gfxPoint Center() const { return pos + gfxPoint(size.width, size.height)/2.0; } > > gfxPoint AtCorner(mozilla::css::Corner corner) const { > switch (corner) { > case NS_CORNER_TOP_LEFT: return TopLeft(); > case NS_CORNER_TOP_RIGHT: return TopRight(); > case NS_CORNER_BOTTOM_RIGHT: return BottomRight(); > case NS_CORNER_BOTTOM_LEFT: return BottomLeft(); > default: >diff --git a/ipc/glue/IPCMessageUtils.h b/ipc/glue/IPCMessageUtils.h >--- a/ipc/glue/IPCMessageUtils.h >+++ b/ipc/glue/IPCMessageUtils.h >@@ -682,29 +682,11 @@ struct ParamTraits<nsRect> > { > return (ReadParam(msg, iter, &result->x) && > ReadParam(msg, iter, &result->y) && > ReadParam(msg, iter, &result->width) && > ReadParam(msg, iter, &result->height)); > } > }; > >-template<> >-struct ParamTraits<gfxIntSize> >-{ >- typedef gfxIntSize paramType; >- >- static void Write(Message* msg, const paramType& param) >- { >- WriteParam(msg, param.width); >- WriteParam(msg, param.height); >- } >- >- static bool Read(const Message* msg, void** iter, paramType* result) >- { >- return (ReadParam(msg, iter, &result->width) && >- ReadParam(msg, iter, &result->height)); >- } >-}; >- > } /* namespace IPC */ > > #endif /* __IPC_GLUE_IPCMESSAGEUTILS_H__ */ >diff --git a/layout/base/nsCSSRenderingBorders.cpp b/layout/base/nsCSSRenderingBorders.cpp >--- a/layout/base/nsCSSRenderingBorders.cpp >+++ b/layout/base/nsCSSRenderingBorders.cpp >@@ -485,17 +485,17 @@ nsCSSBorderRenderer::DoSideClipSubPath(m > else if (startIsDashed && isDashed) > startType = SIDE_CLIP_RECTANGLE; > > if (!IsZeroSize(mBorderRadii[mozilla::css::Corner(NEXT_SIDE(aSide))])) > endType = SIDE_CLIP_TRAPEZOID_FULL; > else if (endIsDashed && isDashed) > endType = SIDE_CLIP_RECTANGLE; > >- gfxPoint midPoint = mInnerRect.pos + mInnerRect.size / 2.0; >+ gfxPoint midPoint = mInnerRect.Center(); > > start[0] = mOuterRect.CCWCorner(aSide); > start[1] = mInnerRect.CCWCorner(aSide); > > end[0] = mOuterRect.CWCorner(aSide); > end[1] = mInnerRect.CWCorner(aSide); > > if (startType == SIDE_CLIP_TRAPEZOID_FULL) { >diff --git a/layout/generic/nsObjectFrame.cpp b/layout/generic/nsObjectFrame.cpp > ::Point No longer necessary, but I have no issue with including it.

Comment on attachment 524649 [details] [diff] [review] Part 4 v2 >diff --git a/gfx/src/BaseMargin.h b/gfx/src/BaseMargin.h >+ T LeftRight() const { return left + right; } >+ T TopBottom() const { return top + bottom; } >+ >+ Point TopLeft() const { return Point(left, top); } This seems a bit odd, and is confusing wrt LeftRight/TopBottom above. Probably remove depending on whatever callers turn up. >+ T& side(Side aSide) { Side().

Comment on attachment 524650 [details] [diff] [review] Part 7 v2 >diff --git a/gfx/src/BaseRect.h b/gfx/src/BaseRect.h >+ // Returns the rectangle containing the intersection of the points >+ // (including edges) of *this and aRect. If there are no points in that >+ // intersection, returns an empty rectangle with x/y set to the max of the x/y >+ // of *this and aRect. nsMenuPopupFrame depends on this! I suspect the note about nsMenuPopupFrame will not age well. Please nuke. (Either this Intersect() definition makes sense or nsMenuPopupFrame is weird and needs its own impl.) >+ bool IntersectRect(const Sub& aRect1, const Sub& aRect2) I don't particularly like this name compared to Intersect() but don't have any better suggestions.

Comment on attachment 523256 [details] [diff] [review] Part 2: Remove a bunch of direct gfxRect::pos/size usage > static PRBool > ConditionRect(gfxRect& r) { This whole method is sort of half-converted; I'm not sure there's a better way to do what it's doing, unfortunately :( >diff --git a/layout/base/nsCSSRenderingBorders.cpp b/layout/base/nsCSSRenderingBorders.cpp >- offset.x = mOuterRect.size.width - mBorderCornerDimensions[aCorner].width; >+ offset.x = mOuterRect.Width() - mBorderCornerDimensions[aCorner].width; It'd be nice if gfxCornerSizes also had a .Width()/etc for consistency, but it's obviously not that important. >@@ -485,17 +485,17 @@ nsCSSBorderRenderer::DoSideClipSubPath(m >- gfxPoint midPoint = mInnerRect.pos + mInnerRect.size / 2.0; >+ gfxPoint midPoint = mInnerRect.TopLeft() + mInnerRect.Size() / 2.0; mInnerRect.Center()? Does that exist? >diff --git a/layout/mathml/nsMathMLmencloseFrame.cpp b/layout/mathml/nsMathMLmencloseFrame.cpp >@@ -780,17 +780,17 @@ void nsDisplayNotation::Paint(nsDisplayL > case NOTATION_CIRCLE: >- gfxCtx->Ellipse(rect.pos + rect.size / 2.0, rect.size); >+ gfxCtx->Ellipse(rect.TopLeft() + rect.Size() / 2.0, rect.Size()); rect.Center()?

Comment on attachment 523258 [details] [diff] [review] Part 3: Convert gfxRect::pos/size to x/y/width/height >diff --git a/gfx/thebes/gfxRect.h b/gfx/thebes/gfxRect.h > struct THEBES_API gfxRect { { on its own line please

Comment on attachment 524649 [details] [diff] [review] Part 4 v2 >diff --git a/gfx/src/BaseMargin.h b/gfx/src/BaseMargin.h >+ * The Initial Developer of the Original Code is Mozilla Foundation. >+ * Portions created by the Initial Developer are Copyright (Sub) 2011 Sub again >+template <class T, class Sub, class Point> struct BaseMargin { Newlines as earlier please >+ T top, right, bottom, left; Due to the way Side is implemented below, please insert a comment that says "do not change the layout of these members". >+ T& side(Side aSide) { >+ NS_PRECONDITION(aSide <= NS_SIDE_LEFT, "Out of range side"); >+ return *(&top + aSide); >+ } >+ T side(Side aSide) const { >+ NS_PRECONDITION(aSide <= NS_SIDE_LEFT, "Out of range side"); >+ return *(&top + aSide); >+ } I would like to formally register my disapproval.

Comment on attachment 524650 [details] [diff] [review] Part 7 v2 >diff --git a/gfx/src/BaseRect.h b/gfx/src/BaseRect.h >+ * Portions created by the Initial Developer are Copyright (Sub) 2011 Sub >+template <class T, class Sub, class Point, class SizeT, class Margin> struct BaseRect { Newlines

(In reply to comment #29) > "(Sub)" ? I think you went a little search-and-replace happy :) Fixed > >+template <class T, class Sub> struct BasePoint { > > Should be formatted more like: > > template <class T, class Sub> > struct BasePoint > { Done > This took me a while to work out. I think you should include a comment at the > top of the .h files here to say how Base{Size,Point} should be used so it's > more obvious that static_cast<Sub *>(this) is meaningful. Done. > >diff --git a/gfx/src/Makefile.in b/gfx/src/Makefile.in > > > EXPORTS = \ > >+ BasePoint.h \ > >+ BaseSize.h \ > > These should probably be exported to mozilla/, no? Sure. (In reply to comment #32) > >- gfxPoint midPoint = mInnerRect.pos + mInnerRect.size / 2.0; > >+ gfxPoint midPoint = mInnerRect.TopLeft() + mInnerRect.Size() / 2.0; > > mInnerRect.Center()? Does that exist? Done. > > case NOTATION_CIRCLE: > >- gfxCtx->Ellipse(rect.pos + rect.size / 2.0, rect.size); > >+ gfxCtx->Ellipse(rect.TopLeft() + rect.Size() / 2.0, rect.Size()); > > rect.Center()? Done. (In reply to comment #33) > > struct THEBES_API gfxRect { > > { on its own line please Done. (In reply to comment #34) > >+ * The Initial Developer of the Original Code is Mozilla Foundation. > >+ * Portions created by the Initial Developer are Copyright (Sub) 2011 > > Sub again Fixed. > >+template <class T, class Sub, class Point> struct BaseMargin { > > Newlines as earlier please Done. > >+ T top, right, bottom, left; > > Due to the way Side is implemented below, please insert a comment that says "do > not change the layout of these members". Done. > >+ T& side(Side aSide) { > >+ NS_PRECONDITION(aSide <= NS_SIDE_LEFT, "Out of range side"); > >+ return *(&top + aSide); > >+ } > >+ T side(Side aSide) const { > >+ NS_PRECONDITION(aSide <= NS_SIDE_LEFT, "Out of range side"); > >+ return *(&top + aSide); > >+ } > > I would like to formally register my disapproval. Noted. (In reply to comment #31) > >+ // Returns the rectangle containing the intersection of the points > >+ // (including edges) of *this and aRect. If there are no points in that > >+ // intersection, returns an empty rectangle with x/y set to the max of the x/y > >+ // of *this and aRect. nsMenuPopupFrame depends on this! > > I suspect the note about nsMenuPopupFrame will not age well. Please > nuke. (Either this Intersect() definition makes sense or > nsMenuPopupFrame is weird and needs its own impl.) Removed. (In reply to comment #35) > >+ * Portions created by the Initial Developer are Copyright (Sub) 2011 > > Sub Fixed. > >+template <class T, class Sub, class Point, class SizeT, class Margin> struct BaseRect { > > Newlines Fixed.

This creates nsIFrame::GetContent/PaddingRectRelativeToSelf.

Fixed a few more sites.

Comment on attachment 525446 [details] [diff] [review] Part 3.5 v2 nsBulletFrame.cpp: > case NS_STYLE_LIST_STYLE_SQUARE: > { >- nsRect rect(mPadding.TopLeft() + aPt, >- nsSize(mRect.width - mPadding.LeftRight(), >- mRect.height - mPadding.TopBottom())); >+ nsRect rect(mPadding.left + aPt.x, mPadding.top + aPt.y, >+ mRect.width - mPadding.LeftRight(), >+ mRect.height - mPadding.TopBottom()); How about just: nsRect rect(aPt, mRect.Size()); rect.Deflate(mPadding); (Maybe Deflate should be inline, though...?) I presume something ensures that nsObjectFrame::BuildLayer is passed a display item whose underlying frame is that object frame -- it might be nice to assert that, though. r=dbaron with that

(In reply to comment #41) > How about just: > nsRect rect(aPt, mRect.Size()); > rect.Deflate(mPadding); OK. > I presume something ensures that nsObjectFrame::BuildLayer is passed a > display item whose underlying frame is that object frame -- it might be > nice to assert that, though. nsDisplayPlugin::BuildLayer just forwards to its own mFrame, so that invariant is pretty tightly held here.

Comment on attachment 523261 [details] [diff] [review] Part 5: Avoid operator== where possible to distinguish between 'equal edges' and 'equal areas' for rectangles I started off trying to review for other cases that should be converted to IsEqualEdges, but quickly gave up (for the parts of the code I'm less familiar with, anyway). Does == on rects now fail to compile, or do you need to define a private operator== for that? (I think it fails to compile, but I want to check.) Why does nsIntRect still have operator==? Shouldn't that be removed? And why does gfxRect now have operator== but not operator!= ? Shouldn't that be removed? The float region checks in nsBlockReflowState::FlowAndPlaceFloat and nsFloatManager::StoreRegionFor should be IsEqualEdges, though that could be a separate bug if you want. We should also probably file a separate bug on being more consistent about using interiors for visual overflow and edges for scrollable overflow. (There were two places in the patch where I saw code dealing with overflow areas in a loop over types.) r=dbaron with that

(In reply to comment #43) > Does == on rects now fail to compile, or do you need to define > a private operator== for that? (I think it fails to compile, but > I want to check.) There is a private operator== in BaseRect, so code using == will fail to compile. Ditto !=. > Why does nsIntRect still have operator==? Shouldn't that be removed? Yes, I missed that. I'll remove it (in part 7). > And why does gfxRect now have operator== but not operator!= ? > Shouldn't that be removed? It is removed in part 7. > The float region checks in nsBlockReflowState::FlowAndPlaceFloat and > nsFloatManager::StoreRegionFor should be IsEqualEdges, though that > could be a separate bug if you want. I'll fix it now. > We should also probably file a separate bug on being more consistent > about using interiors for visual overflow and edges for scrollable > overflow. (There were two places in the patch where I saw code dealing > with overflow areas in a loop over types.) Maybe, but I don't expect anyone to ever do a systematic audit, so I'm not sure it's worth filing a bug for.

(In reply to comment #44) > (In reply to comment #43) > > Why does nsIntRect still have operator==? Shouldn't that be removed? > > Yes, I missed that. I'll remove it (in part 7). Er, now I remember why it's there. IPDL-generated code involving nsIntRect relies on nsIntRect having an accessible operator== :-(. I'll leave that in for now. I'm really not sure what to do about it long term ... could ipdl do equality testing via some kind of traits class that we can customize for particular types?

(In reply to comment #45) > I'm really not sure what to do about it long term ... could ipdl do equality > testing via some kind of traits class that we can customize for particular > types? Yeah.

Then maybe leave a comment next to the operator== for now, explaining that we want to get rid of it and why it's still there.

http://hg.mozilla.org/mozilla-central/rev/848c520e9158 http://hg.mozilla.org/mozilla-central/rev/36f62297c1e1 http://hg.mozilla.org/mozilla-central/rev/12f37ad53b41 http://hg.mozilla.org/mozilla-central/rev/1baef3c46433 http://hg.mozilla.org/mozilla-central/rev/2629bcfa2816 http://hg.mozilla.org/mozilla-central/rev/c3bf4b3ec506 http://hg.mozilla.org/mozilla-central/rev/964231489dde http://hg.mozilla.org/mozilla-central/rev/90e46b3e8e6f http://hg.mozilla.org/mozilla-central/rev/91e75937d34a http://hg.mozilla.org/mozilla-central/rev/8d64029c1725 http://hg.mozilla.org/mozilla-central/rev/fc1ed658bf4b