791879 - GFX has two essentially-identical "IntSize" classes: IntSize and nsIntSize

Status: NEW

(Core :: Graphics, defect)

Description

[Daniel Holbert [:dholbert]]

Bug 651858 added[1] a new struct "IntSize", which (now at least) appears near-identical to the existing nsIntSize structs.

Both structs live in GFX. There's no reason to have two of them.  We should just standardize on one of them and drop the other one.

Comparison of the two classes:
33 struct IntSize :
34   public BaseSize<int32_t, IntSize> {
35   typedef BaseSize<int32_t, IntSize> Super;
36 
37   IntSize() : Super() {}
38   IntSize(int32_t aWidth, int32_t aHeight) : Super(aWidth, aHeight) {}
39 };

31 struct nsIntSize : public mozilla::gfx::BaseSize<int32_t, nsIntSize> {
32   typedef mozilla::gfx::BaseSize<int32_t, nsIntSize> Super;
33 
34   nsIntSize() : Super() {}
35   nsIntSize(int32_t aWidth, int32_t aHeight) : Super(aWidth, aHeight) {}
36 
37   inline nsSize ToAppUnits(nscoord aAppUnitsPerPixel) const;
38 };

Aside from the presence of the ToAppUnits() method, I think they're identical.

Filing this bug on dropping IntSize (leaving behind a typedef for convenience, perhaps) and then converting  all IntSize clients to use nsIntSize instead.

(BONUS POINTS: We should probably also do s/gfxIntSize/nsIntSize/, too -- gfxIntSize used to be a distinct type but is now just a typedef for nsIntSize.)

[1] IntSize struct was added here: http://hg.mozilla.org/mozilla-central/rev/4ede291d2e4c#l21.74

Comment 1

[Daniel Holbert [:dholbert]]

Here are MXR links for the two structs, for convenience:
  IntSize:   http://mxr.mozilla.org/mozilla-central/source/gfx/2d/Point.h#33
  nsIntSize: http://mxr.mozilla.org/mozilla-central/source/gfx/src/nsSize.h#34

Comment 2

[Bas Schouten (:bas.schouten)]

IntSize should stay and nsIntSize should go away. The ns prefix has no meaning in Graphics land :)

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> Aside from the presence of the ToAppUnits() method, I think they're
> identical.

That is exactly the important difference. I guess you'll need to make a global function or something to replace it. Which is OK, I suppose, but it's going to make some code a little uglier, which is why this wasn't done before.

Comment 4

[Yonggang Luo]

Ping for remove of IntSize or nsIntSize, that's really messed me up.

Comment 5

[Yonggang Luo]

(In reply to Yonggang Luo from comment #4)
> Ping for remove of IntSize or nsIntSize, that's really messed me up.
For the simplicity, we can do a typedef nsIntSize IntSize or vice versa