Remove gfxRGBA (part 2)

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks: 1 bug)

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
A follow-up to 1207944, which started removing gfxRGBA from the tree.
(Assignee)

Comment 1

3 years ago
Created attachment 8665722 [details] [diff] [review]
(part 1) - Change gfxPattern's single-arg constructor to take a gfx::Color& instead of a gfxRGBA&
Attachment #8665722 - Flags: review?(matt.woodrow)
(Assignee)

Comment 2

3 years ago
Created attachment 8665726 [details] [diff] [review]
(part 2) - Change ColorLayer::mColor, ColorLayerProperties::mColor, ReadbackLayer::mBackgroundColor from gfxRBGA to gfx::Color
Attachment #8665726 - Flags: review?(matt.woodrow)
(Assignee)

Comment 3

3 years ago
Created attachment 8665728 [details] [diff] [review]
(part 2) - Rename Color::{To,From}ARGB() so they aren't easily confused with {To,From}ABGR()

This is a little weird, arguably, but while doing this refactoring I kept
worrying I'd type the wrong name and I went back multiple times to check, and
then I got sick of doing that and decided to make it an impossible mistake to
make.
Attachment #8665728 - Flags: review?(matt.woodrow)
(Assignee)

Comment 4

3 years ago
Created attachment 8665732 [details] [diff] [review]
(part 3) - Rename Color::{To,From}ARGB() so they aren't easily confused with {To,From}ABGR()

Fix the part number.
Attachment #8665732 - Flags: review?(matt.woodrow)
(Assignee)

Updated

3 years ago
Attachment #8665728 - Attachment is obsolete: true
Attachment #8665728 - Flags: review?(matt.woodrow)
(Assignee)

Comment 5

3 years ago
Created attachment 8665733 [details] [diff] [review]
(part 4) - Remove the ToDeviceColor() that takes a gfxRGBA
Attachment #8665733 - Flags: review?(matt.woodrow)
(Assignee)

Comment 6

3 years ago
Created attachment 8665735 [details] [diff] [review]
(part 5) - Change FrameMetrics::mBackgroundColor from gfxRGBA to gfx::Color
Attachment #8665735 - Flags: review?(matt.woodrow)
(Assignee)

Comment 7

3 years ago
Created attachment 8665736 [details] [diff] [review]
(part 6) - Pass a gfx::Color instead of a gfxRGBA to PaintRectToSurface()
Attachment #8665736 - Flags: review?(matt.woodrow)
(Assignee)

Updated

3 years ago
Blocks: 1208300
I'm going to take these reviews as well, since Matt's timezone means he's now into his weekend and I'd like to keep this moving while you're churning out patches. Hope no one minds.
Comment on attachment 8665732 [details] [diff] [review]
(part 3) - Rename Color::{To,From}ARGB() so they aren't easily confused with {To,From}ABGR()

Redirecting to Bas, since this concerns Moz2D API.
Attachment #8665732 - Flags: review?(matt.woodrow) → review?(bas)
Comment on attachment 8665722 [details] [diff] [review]
(part 1) - Change gfxPattern's single-arg constructor to take a gfx::Color& instead of a gfxRGBA&

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

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +39,5 @@
>  typedef mozilla::dom::Element Element;
>  
>  mozilla::gfx::UserDataKey gfxTextContextPaint::sUserDataKey;
>  
> +/* static */ const Color SimpleTextContextPaint::sZero = Color();

Boo for the implicit defaults. ;)

::: layout/svg/nsSVGGradientFrame.cpp
@@ +234,5 @@
>  
>    // SVG specification says that no stops should be treated like
>    // the corresponding fill or stroke had "none" specified.
>    if (nStops == 0) {
> +    nsRefPtr<gfxPattern> pattern = new gfxPattern(gfx::Color());

Layout code generally uses mozilla:gfx in .cpp files. Add another 'using' statement at the top of this file so you don't need the gfx prefix.

@@ +244,5 @@
>      // gradient step if there are more than one.
>      float stopOpacity = stopFrames[nStops-1]->StyleSVGReset()->mStopOpacity;
>      nscolor stopColor = stopFrames[nStops-1]->StyleSVGReset()->mStopColor;
>  
> +    gfx::Color stopColor2 = gfx::Color::FromABGR(stopColor);

Here too.

@@ +299,5 @@
>        offset = lastOffset;
>      else
>        lastOffset = offset;
>  
> +    gfx::Color stopColor2 = gfx::Color::FromABGR(stopColor);

And here.
Attachment #8665722 - Flags: review?(matt.woodrow) → review+

Updated

3 years ago
Attachment #8665726 - Flags: review?(matt.woodrow) → review+

Updated

3 years ago
Attachment #8665733 - Flags: review?(matt.woodrow) → review+

Updated

3 years ago
Attachment #8665735 - Flags: review?(matt.woodrow) → review+

Updated

3 years ago
Attachment #8665736 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 11

3 years ago
> I'm going to take these reviews as well... Hope no one minds.

Nope! Thank you.
(Assignee)

Updated

3 years ago
Attachment #8665732 - Attachment is obsolete: true
Attachment #8665732 - Flags: review?(bas)
(Assignee)

Comment 13

3 years ago
> (part 2) - Change ColorLayer::mColor, ColorLayerProperties::mColor,
> ReadbackLayer::mBackgroundColor from gfxRBGA to gfx::Color

I landed all the patches except this one, which I spun off into bug 1208945, because it's something of a tangent to the other patches.
You need to log in before you can comment on or make changes to this bug.