Closed Bug 1223690 Opened 7 years ago Closed 7 years ago

Remove implicit Rect conversions


(Core :: Graphics, defect)

Not set



Tracking Status
firefox45 --- fixed


(Reporter: n.nethercote, Assigned: n.nethercote)



(1 file)

The implicit constructor in gfxRect has some subtle and surprising effects. Let's remove it.
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()
Attachment #8685868 - Flags: review?(jmuizelaar)
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 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.
Attachment #8685868 - Flags: review?(jmuizelaar) → review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.