Last Comment Bug 641426 - Unify point/size/rect types
: Unify point/size/rect types
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
: Milan Sreckovic [:milan]
Mentors:
: 470506 491593 (view as bug list)
Depends on: 651468 654811 654858 657401
Blocks: 430829 647914
  Show dependency treegraph
 
Reported: 2011-03-13 22:06 PDT by Robert O'Callahan (:roc) (email my personal email if necessary)
Modified: 2011-10-07 11:43 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Create Point and Size templates (24.05 KB, patch)
2011-03-31 02:16 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Part 2: Remove a bunch of direct gfxRect::pos/size usage (63.14 KB, patch)
2011-03-31 02:17 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
joe: review+
Details | Diff | Splinter Review
Part 2.5: Make gfxBlur bail out in all cases where the blur rect is empty (6.09 KB, patch)
2011-03-31 02:18 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Part 3: Convert gfxRect::pos/size to x/y/width/height (36.43 KB, patch)
2011-03-31 02:19 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
joe: review+
cjones.bugs: superreview+
Details | Diff | Splinter Review
Part 4: Create Margin template (13.03 KB, patch)
2011-03-31 02:19 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Part 5: Avoid operator== where possible to distinguish between 'equal edges' and 'equal areas' for rectangles (72.60 KB, patch)
2011-03-31 02:20 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
dbaron: review+
cjones.bugs: superreview+
Details | Diff | Splinter Review
Part 6: Rename Empty to SetEmpty (13.99 KB, patch)
2011-03-31 02:21 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
cjones.bugs: review+
Details | Diff | Splinter Review
Part 7: Create Rect template (46.88 KB, patch)
2011-03-31 02:23 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Bug 641426. Part 8: Replace gfxRect::Outset/Inset with Inflate/Deflate. Also slip in a conversion constructor from nsIntRect to gfxRect (22.54 KB, patch)
2011-03-31 02:23 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Part 1 v2 (29.10 KB, patch)
2011-04-07 22:08 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
joe: review+
cjones.bugs: superreview+
Details | Diff | Splinter Review
Part 8 v2 (22.74 KB, patch)
2011-04-08 00:46 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Part 4 v2 (13.00 KB, patch)
2011-04-08 10:09 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
joe: review+
cjones.bugs: superreview+
Details | Diff | Splinter Review
Part 7 v2 (48.13 KB, patch)
2011-04-08 10:10 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
joe: review+
cjones.bugs: superreview+
Details | Diff | Splinter Review
Part 3.5: Remove usage of nsMargin::TopLeft and nsMargin::IsZero (20.46 KB, patch)
2011-04-11 23:52 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Part 4.2: Rename BaseMargin::side to BaseMargin::Side (9.53 KB, patch)
2011-04-11 23:53 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
cjones.bugs: review+
Details | Diff | Splinter Review
Part 3.5 v2 (23.22 KB, patch)
2011-04-12 11:12 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
dbaron: review+
Details | Diff | Splinter Review

Description Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-13 22:06:21 PDT
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.
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-14 12:21:13 PDT
\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.
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-31 02:16:27 PDT
Created attachment 523254 [details] [diff] [review]
Part 1: Create Point and Size templates
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-31 02:17:30 PDT
Created attachment 523256 [details] [diff] [review]
Part 2: Remove a bunch of direct gfxRect::pos/size usage
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-31 02:18:08 PDT
Created attachment 523257 [details] [diff] [review]
Part 2.5: Make gfxBlur bail out in all cases where the blur rect is empty
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-31 02:19:06 PDT
Created attachment 523258 [details] [diff] [review]
Part 3: Convert gfxRect::pos/size to x/y/width/height
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-31 02:19:35 PDT
Created attachment 523260 [details] [diff] [review]
Part 4: Create Margin template
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-31 02:20:24 PDT
Created attachment 523261 [details] [diff] [review]
Part 5: Avoid operator== where possible to distinguish between 'equal edges' and 'equal areas' for rectangles
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-31 02:21:12 PDT
Created attachment 523262 [details] [diff] [review]
Part 6: Rename Empty to SetEmpty

"Empty" is ambiguous as to whether it's a getter or a setter.
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-31 02:23:18 PDT
Created attachment 523263 [details] [diff] [review]
Part 7: Create Rect template
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-31 02:23:48 PDT
Created 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
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-31 02:25:36 PDT
These patches are showing green on try.
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-31 12:24:29 PDT
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 13 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-01 15:01:11 PDT
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.
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-01 15:02:57 PDT
Yeah, we can remove them in a followup, but they are fairly harmless.
Comment 15 Timothy Nikkel (:tnikkel) 2011-04-03 21:23:10 PDT
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?
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-06 01:00:13 PDT
Yes.
Comment 17 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-06 13:28:44 PDT
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 18 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-06 14:33:38 PDT
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?
Comment 19 Timothy Nikkel (:tnikkel) 2011-04-07 18:02:42 PDT
(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.
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-07 19:12:04 PDT
That is a bug!
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-07 19:12:13 PDT
(Will fix)
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-07 22:08:03 PDT
Created attachment 524575 [details] [diff] [review]
Part 1 v2
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-07 22:45:32 PDT
(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.
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-07 22:53:56 PDT
(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 25 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-08 00:46:39 PDT
Created attachment 524596 [details] [diff] [review]
Part 8 v2
Comment 26 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-08 09:32:34 PDT
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 27 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-08 10:09:30 PDT
Created attachment 524649 [details] [diff] [review]
Part 4 v2
Comment 28 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-08 10:10:25 PDT
Created attachment 524650 [details] [diff] [review]
Part 7 v2
Comment 29 Joe Drew (not getting mail) 2011-04-11 10:43:11 PDT
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 30 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-11 13:15:39 PDT
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 31 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-11 13:34:41 PDT
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 32 Joe Drew (not getting mail) 2011-04-11 14:47:25 PDT
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 33 Joe Drew (not getting mail) 2011-04-11 16:35:32 PDT
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 34 Joe Drew (not getting mail) 2011-04-11 16:47:45 PDT
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 35 Joe Drew (not getting mail) 2011-04-11 17:15:13 PDT
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
Comment 36 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-11 23:50:17 PDT
(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.
Comment 37 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-11 23:52:15 PDT
Created attachment 525320 [details] [diff] [review]
Part 3.5: Remove usage of nsMargin::TopLeft and nsMargin::IsZero

This creates nsIFrame::GetContent/PaddingRectRelativeToSelf.
Comment 38 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-11 23:53:09 PDT
Created attachment 525321 [details] [diff] [review]
Part 4.2: Rename BaseMargin::side to BaseMargin::Side
Comment 39 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-12 11:12:53 PDT
Created attachment 525446 [details] [diff] [review]
Part 3.5 v2

Fixed a few more sites.
Comment 40 Zack Weinberg (:zwol) 2011-04-14 07:31:22 PDT
*** Bug 470506 has been marked as a duplicate of this bug. ***
Comment 41 David Baron :dbaron: ⌚️UTC-10 2011-04-18 14:46:40 PDT
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
Comment 42 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-18 15:05:12 PDT
(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 43 David Baron :dbaron: ⌚️UTC-10 2011-04-18 15:16:12 PDT
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
Comment 44 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-18 15:32:35 PDT
(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.
Comment 45 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-18 16:21:20 PDT
(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?
Comment 46 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-18 19:38:48 PDT
(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.
Comment 47 David Baron :dbaron: ⌚️UTC-10 2011-04-18 19:48:03 PDT
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.
Comment 49 Ali Juma [:ajuma] 2011-10-07 11:43:36 PDT
*** Bug 491593 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.