1223690 - Remove implicit Rect conversions

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Nicholas Nethercote [:njn]]

The implicit constructor in gfxRect has some subtle and surprising effects. Let's remove it.

Comment 1

[Nicholas Nethercote [:njn]]

Attachment: Remove implicit Rect conversions

gfxRect can be implicitly constructed from IntRect, which hides a number of
implicit conversion points, makes Moz2Dification harder, and has some
surprising effects.

This patch removes the implicit constructor and replaces it with an explicit
conversion function:

  gfxRect ThebesRect(const IntRect&)

This is the obvious outcome of removing the constructor.

But there is also a second, less obvious outcome: currently we do a number of
IntRect-to-Rect conversions using ToRect(), which (surprisingly) works because
it turns into an implicit IntRect-to-gfxRect conversion (via the implicit
constructor) combined with an explicit gfxRect-to-Rect conversion (via
ToRect()). I.e. we do two conversions, going from a Moz2D type to a Thebes
type and back to a Moz2D type!

So this patch also changes these conversion. It moves this existing function:

  Rect ToRect(const IntRect&)

from gfx2DGlue.h -- where it doesn't really belong because it doesn't involve
any Thebes types -- to gfx/2d/Rect.h, templatifying and renaming it as
IntRectToRect() in the process.

The rest of the patch deals with fall-out from these changes. The call sites
change as follows:

- IntRect-to-gfxRect conversions:
  - old: implicit
  - new: ThebesRect()

- IntRect-to-Rect conversions:
  - old: ToRect()
  - new: IntRectToRect()

Comment 2

[Nicholas Nethercote [:njn]]

Comment on attachment 8685868 [details] [diff] [review]
Remove implicit Rect conversions

Review of attachment 8685868 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxBlur.cpp
@@ -954,5 @@
>    // Dirty rect and skip rect are null for the min inset shadow.
>    // When rendering inset box shadows, we respect the spread radius by changing
>    //  the shape of the unblurred shadow, and can pass a spread radius of zero here.
>    IntSize zeroSpread(0, 0);
> -  gfxContext* minGfxContext = Init(ThebesRect(ToRect(outerRect)),

This line is worth highlighting in particular.

outerRect is an IntRect, so this line is doing the following conversions:

  IntRect -(implicit)-> gfxRect -(explicit)-> Rect -(explicit)-> gfxRect

The new code just does:

  IntRect -(explicit)-> gfxRect

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8685868 [details] [diff] [review]
Remove implicit Rect conversions

Review of attachment 8685868 [details] [diff] [review]:
-----------------------------------------------------------------

I think I put the implicit conversion, but I'm happy to have it go.

Comment 4

[Nicholas Nethercote [:njn]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed48eac74e39a37bf9b1fc21e1ae16699965b19f
Bug 1223690 - Remove implicit Rect conversions. r=jrmuizel.

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ed48eac74e39