1207944 - Remove gfxRGBA (part 1)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Nicholas Nethercote [inactive]]

I have a stack of 22 patches that remove gfxRGBA from the tree. I'll do the first batch in this bug.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Pass a gfx::Color& instead of a gfxRGBA& to SetDeviceColor()

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Pass a gfx::Color& instead of a gfxRGBA& to GetDeviceColor()

This requires doing likewise for GetSolidColor(), PushSolidColor()
HasNonOpaqueColor().

This removes a ThebesRGBA() call in GetDeviceColor() and a ThebesColor() call in
GetSolidColor().

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Add a gfxContext::SetColor method that takes a gfx::Color&

The existing one, which takes a gfxRGBA&, will be removed a few patches hence.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Use SetColor(const Color&) when setting from an nscolor

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Use SetColor(const Color&) when setting from a constructed gfxRGBA

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Remove an unnecessary ThebesColor() call

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Use gfx::Color instead of gfxRGBA in ColorStop

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Use gfx::Color instead of gfxRGBA in BlurCache

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 9) - Change nsDisplayBackgroundColor::mColor from gfxRGBA to gfx::Color

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: (part 10) - Remove SetColor(const gfxRGBA&)

Comment 11

[Jonathan Watt [:jwatt]]

Comment on attachment 8665290 [details] [diff] [review]
(part 3) - Add a gfxContext::SetColor method that takes a gfx::Color&

You need to call ToDeviceColor() call in the new function too. Consider this an r+ with that added.

Comment 12

[Jonathan Watt [:jwatt]]

Comment on attachment 8665288 [details] [diff] [review]
(part 1) - Pass a gfx::Color& instead of a gfxRGBA& to SetDeviceColor()

Might as well r+ these since I read them too.

Comment 13

[Jonathan Watt [:jwatt]]

Comment on attachment 8665309 [details] [diff] [review]
(part 7) - Use gfx::Color instead of gfxRGBA in ColorStop

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

::: layout/base/nsCSSRendering.cpp
@@ +2588,5 @@
>                // Compute how far the new position 0.0 is along the interval
>                // between pos and nextPos.
>                // XXX Color interpolation (in cairo, too) should use the
>                // CSS 'color-interpolation' property!
> +              float frac = float((0.0 - pos)/(nextPos - pos));

It doesn't look like 'pos' and 'nextPos' need to be double (or ColorStop.mPosition), but you don't need to change that now I guess.

Comment 14

[Jonathan Watt [:jwatt]]

Comment on attachment 8665312 [details] [diff] [review]
(part 8) - Use gfx::Color instead of gfxRGBA in BlurCache

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

::: gfx/thebes/gfxBlur.h
@@ +128,5 @@
>      static void BlurRectangle(gfxContext *aDestinationCtx,
>                                const gfxRect& aRect,
>                                RectCornerRadii* aCornerRadii,
>                                const gfxPoint& aBlurStdDev,
> +                              const mozilla::gfx::Color& aShadowColor,

Add a typedef like the one for RectCornerRadii at the beginning of the class declaration.

Comment 15

[Jonathan Watt [:jwatt]]

Comment on attachment 8665313 [details] [diff] [review]
(part 9) - Change nsDisplayBackgroundColor::mColor from gfxRGBA to gfx::Color

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

::: layout/base/nsDisplayList.cpp
@@ +2988,5 @@
>  void
>  nsDisplayBackgroundColor::Paint(nsDisplayListBuilder* aBuilder,
>                                  nsRenderingContext* aCtx)
>  {
> +  if (mColor == Color()) {

Personally I'd prefer Color(0.f, 0.f, 0.f) to be clear, but if the defaults are there I guess others disagrees, so either way.

Comment 16

[Jonathan Watt [:jwatt]]

Comment on attachment 8665314 [details] [diff] [review]
(part 10) - Remove SetColor(const gfxRGBA&)

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

\o/

Comment 17

[Nicholas Nethercote [inactive]]

jwatt: thank you for the fast and unexpected reviews :)


> (part 3) - Add a gfxContext::SetColor method that takes a gfx::Color&
> 
> You need to call ToDeviceColor() call in the new function too.

Good catch.


> > +              float frac = float((0.0 - pos)/(nextPos - pos));
> 
> It doesn't look like 'pos' and 'nextPos' need to be double (or
> ColorStop.mPosition), but you don't need to change that now I guess.

I considered it but changing mPosition would have propagated through to a lot more places so I left it alone for now. I'll add it to my list of things to fix later.


> Add a typedef like the one for RectCornerRadii at the beginning of the class
> declaration.

Will do.


> > +  if (mColor == Color()) {
> 
> Personally I'd prefer Color(0.f, 0.f, 0.f) to be clear, but if the defaults
> are there I guess others disagrees, so either way.

Careful! Color(0,0,0) gives opaque black. Color() is equivalent to Color(0,0,0,0) which is transparent black.

Thinking some more, the special-casing of transparent black here is strange to me... I would have thought any transparent colour would trigger the early return case here. But I don't want to combine functional changes with this refactoring so I'll leave it alone.

Comment 18

[Jonathan Watt [:jwatt]]

(In reply to Nicholas Nethercote [:njn] from comment #17)
> Careful! Color(0,0,0) gives opaque black. Color() is equivalent to
> Color(0,0,0,0) which is transparent black.

Haha, there you go. ;)

> Thinking some more, the special-casing of transparent black here is strange
> to me... I would have thought any transparent colour would trigger the early
> return case here. But I don't want to combine functional changes with this
> refactoring so I'll leave it alone.

I wasn't suggesting getting rid of the default values, but rather that you continue to pass values explicitly rather than rely on them.

Comment 19

[Nicholas Nethercote [inactive]]

I rebased after bug 1188075 landed. Part 6 disappeared and I had to remove some more ThebesColor() calls in part 7. I'll rename bugs 7--10 before landing.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/619a48655962
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6d617597f21
https://hg.mozilla.org/integration/mozilla-inbound/rev/49006d284866
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbfb007252f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e4a409f1dcd

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b48f790070bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/80409f3546ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb172c22b2aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6d1add6938d

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/619a48655962
https://hg.mozilla.org/mozilla-central/rev/a6d617597f21
https://hg.mozilla.org/mozilla-central/rev/49006d284866
https://hg.mozilla.org/mozilla-central/rev/bbfb007252f1
https://hg.mozilla.org/mozilla-central/rev/3e4a409f1dcd

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b48f790070bb
https://hg.mozilla.org/mozilla-central/rev/80409f3546ac
https://hg.mozilla.org/mozilla-central/rev/bb172c22b2aa
https://hg.mozilla.org/mozilla-central/rev/b6d1add6938d