Remove gfxRGBA (part 1)

RESOLVED FIXED in Firefox 44

Status

()

Core
Graphics
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks: 1 bug)

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(10 attachments)

3.18 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
6.79 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
2.17 KB, patch
jwatt
: review-
Details | Diff | Splinter Review
16.81 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
3.89 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
1.72 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
13.29 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
10.11 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
4.49 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
1.76 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
I have a stack of 22 patches that remove gfxRGBA from the tree. I'll do the first batch in this bug.
(Assignee)

Comment 1

2 years ago
Created attachment 8665288 [details] [diff] [review]
(part 1) - Pass a gfx::Color& instead of a gfxRGBA& to SetDeviceColor()
Attachment #8665288 - Flags: review?(bas)
(Assignee)

Comment 2

2 years ago
Created attachment 8665289 [details] [diff] [review]
(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().
Attachment #8665289 - Flags: review?(bas)
(Assignee)

Comment 3

2 years ago
Created attachment 8665290 [details] [diff] [review]
(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.
Attachment #8665290 - Flags: review?(bas)
(Assignee)

Comment 4

2 years ago
Created attachment 8665292 [details] [diff] [review]
(part 4) - Use SetColor(const Color&) when setting from an nscolor
Attachment #8665292 - Flags: review?(matt.woodrow)
(Assignee)

Comment 5

2 years ago
Created attachment 8665293 [details] [diff] [review]
(part 5) - Use SetColor(const Color&) when setting from a constructed gfxRGBA
Attachment #8665293 - Flags: review?(bas)
(Assignee)

Comment 6

2 years ago
Created attachment 8665295 [details] [diff] [review]
(part 6) - Remove an unnecessary ThebesColor() call
Attachment #8665295 - Flags: review?(matt.woodrow)
(Assignee)

Comment 7

2 years ago
Created attachment 8665309 [details] [diff] [review]
(part 7) - Use gfx::Color instead of gfxRGBA in ColorStop
Attachment #8665309 - Flags: review?(matt.woodrow)
(Assignee)

Comment 8

2 years ago
Created attachment 8665312 [details] [diff] [review]
(part 8) - Use gfx::Color instead of gfxRGBA in BlurCache
Attachment #8665312 - Flags: review?(bas)
(Assignee)

Comment 9

2 years ago
Created attachment 8665313 [details] [diff] [review]
(part 9) - Change nsDisplayBackgroundColor::mColor from gfxRGBA to gfx::Color
Attachment #8665313 - Flags: review?(matt.woodrow)
(Assignee)

Comment 10

2 years ago
Created attachment 8665314 [details] [diff] [review]
(part 10) - Remove SetColor(const gfxRGBA&)
Attachment #8665314 - Flags: review?(bas)
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.
Attachment #8665290 - Flags: review?(bas) → review-
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.
Attachment #8665288 - Flags: review?(bas) → review+
Attachment #8665289 - Flags: review?(bas) → review+
Attachment #8665293 - Flags: review?(bas) → review+
Attachment #8665292 - Flags: review?(matt.woodrow) → review+
Attachment #8665295 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8665309 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8665312 - Flags: review?(bas) → review+
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.
Attachment #8665313 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8665314 [details] [diff] [review]
(part 10) - Remove SetColor(const gfxRGBA&)

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

\o/
Attachment #8665314 - Flags: review?(bas) → review+
(Assignee)

Comment 17

2 years ago
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.
(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.
(Assignee)

Comment 19

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1208283
You need to log in before you can comment on or make changes to this bug.