Bug 206862 – Replace RGBA32 typedef with a class to improve type safety

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Darin Adler]

Replace RGBA32 typedef with a class to improve type safety

[Darin Adler]

Created attachment 388967 [details]
Patch

[Darin Adler]

First of multiple steps to improve Color and ExtendedColor. Next step will be putting SimpleColor in its own source file. Then use Optional<SimpleColor> for cases where we don’t always have a color, but don't need to specify a color space, support extended color, or support the notion of a semantic color. Then change Color operations so they all do something sensible when called on an ExtendedColor. Then change the Color constructor to make a SimpleColor, not an ExtendedColor, when that can be done losslessly.

[Darin Adler]

Created attachment 388970 [details]
Patch

[Darin Adler]

Created attachment 389011 [details]
Patch

[Darin Adler]

Created attachment 389101 [details]
Patch

[Darin Adler]

Created attachment 389246 [details]
Patch

[Darin Adler]

Created attachment 389460 [details]
Patch

[Darin Adler]

I don’t understand the api-ios failure. Is it real? If it is real, can I get a backtrace from the segfault that happens in the test?

[Sam Weinig]

Comment on attachment 389460 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=389460&action=review

> Source/WebCore/page/cocoa/ResourceUsageOverlayCocoa.mm:131
> +    HistoricMemoryCategoryInfo(unsigned category, unsigned rgba, String name, bool subcategory = false)

I think uint32_t would be slightly better here.

[Darin Adler]

Committed r255785: <https://trac.webkit.org/changeset/255785>

[Radar WebKit Bug Importer]

<rdar://problem/59176489>

[Ryan Haddad]

Follow up build fix for internal bots https://trac.webkit.org/changeset/255817/webkit

[Philippe Normand]

I suspect this patch triggered these warnings (in GTK at least):

In file included from DerivedSources/WebCore/unified-sources/UnifiedSource-3c72abbe-17.cpp:3:
../../Source/WebCore/platform/graphics/BitmapImage.cpp:240:84: warning: implicit conversion from 'double' to 'uint8_t' (aka 'unsigned char') changes value from 0.5 to 0 [-Wliteral-conversion]
                fillWithSolidColor(context, destRect, Color::yellow.colorWithAlpha(0.5), options.compositeOperator());
                                                                    ~~~~~~~~~~~~~~ ^~~
../../Source/WebCore/platform/graphics/BitmapImage.cpp:251:80: warning: implicit conversion from 'double' to 'uint8_t' (aka 'unsigned char') changes value from 0.5 to 0 [-Wliteral-conversion]
            fillWithSolidColor(context, destRect, Color::yellow.colorWithAlpha(0.5), options.compositeOperator());
                                                                ~~~~~~~~~~~~~~ ^~~
../../Source/WebCore/platform/graphics/BitmapImage.cpp:269:84: warning: implicit conversion from 'double' to 'uint8_t' (aka 'unsigned char') changes value from 0.5 to 0 [-Wliteral-conversion]
                fillWithSolidColor(context, destRect, Color::yellow.colorWithAlpha(0.5), options.compositeOperator());
                                                                    ~~~~~~~~~~~~~~ ^~~
3 warnings generated.


Because SimpleColor doesn't have a colorWithAlpha(Optional<float>) method. Should it be added?

[Sam Weinig]

(In reply to Philippe Normand from comment #13)
> I suspect this patch triggered these warnings (in GTK at least):
> 
> In file included from
> DerivedSources/WebCore/unified-sources/UnifiedSource-3c72abbe-17.cpp:3:
> ../../Source/WebCore/platform/graphics/BitmapImage.cpp:240:84: warning:
> implicit conversion from 'double' to 'uint8_t' (aka 'unsigned char') changes
> value from 0.5 to 0 [-Wliteral-conversion]
>                 fillWithSolidColor(context, destRect,
> Color::yellow.colorWithAlpha(0.5), options.compositeOperator());
>                                                                    
> ~~~~~~~~~~~~~~ ^~~
> ../../Source/WebCore/platform/graphics/BitmapImage.cpp:251:80: warning:
> implicit conversion from 'double' to 'uint8_t' (aka 'unsigned char') changes
> value from 0.5 to 0 [-Wliteral-conversion]
>             fillWithSolidColor(context, destRect,
> Color::yellow.colorWithAlpha(0.5), options.compositeOperator());
>                                                                
> ~~~~~~~~~~~~~~ ^~~
> ../../Source/WebCore/platform/graphics/BitmapImage.cpp:269:84: warning:
> implicit conversion from 'double' to 'uint8_t' (aka 'unsigned char') changes
> value from 0.5 to 0 [-Wliteral-conversion]
>                 fillWithSolidColor(context, destRect,
> Color::yellow.colorWithAlpha(0.5), options.compositeOperator());
>                                                                    
> ~~~~~~~~~~~~~~ ^~~
> 3 warnings generated.
> 
> 
> Because SimpleColor doesn't have a colorWithAlpha(Optional<float>) method.
> Should it be added?

Oh, yeah. I will fix this right away. Wonder why we don't have that warning on for other platforms. Will look into that too. Thanks for bringing it to my attention.

[Sam Weinig]

(In reply to Sam Weinig from comment #14)
> (In reply to Philippe Normand from comment #13)
> > I suspect this patch triggered these warnings (in GTK at least):
> > 
> > In file included from
> > DerivedSources/WebCore/unified-sources/UnifiedSource-3c72abbe-17.cpp:3:
> > ../../Source/WebCore/platform/graphics/BitmapImage.cpp:240:84: warning:
> > implicit conversion from 'double' to 'uint8_t' (aka 'unsigned char') changes
> > value from 0.5 to 0 [-Wliteral-conversion]
> >                 fillWithSolidColor(context, destRect,
> > Color::yellow.colorWithAlpha(0.5), options.compositeOperator());
> >                                                                    
> > ~~~~~~~~~~~~~~ ^~~
> > ../../Source/WebCore/platform/graphics/BitmapImage.cpp:251:80: warning:
> > implicit conversion from 'double' to 'uint8_t' (aka 'unsigned char') changes
> > value from 0.5 to 0 [-Wliteral-conversion]
> >             fillWithSolidColor(context, destRect,
> > Color::yellow.colorWithAlpha(0.5), options.compositeOperator());
> >                                                                
> > ~~~~~~~~~~~~~~ ^~~
> > ../../Source/WebCore/platform/graphics/BitmapImage.cpp:269:84: warning:
> > implicit conversion from 'double' to 'uint8_t' (aka 'unsigned char') changes
> > value from 0.5 to 0 [-Wliteral-conversion]
> >                 fillWithSolidColor(context, destRect,
> > Color::yellow.colorWithAlpha(0.5), options.compositeOperator());
> >                                                                    
> > ~~~~~~~~~~~~~~ ^~~
> > 3 warnings generated.
> > 
> > 
> > Because SimpleColor doesn't have a colorWithAlpha(Optional<float>) method.
> > Should it be added?
> 
> Oh, yeah. I will fix this right away. Wonder why we don't have that warning
> on for other platforms. Will look into that too. Thanks for bringing it to
> my attention.

Filed https://bugs.webkit.org/show_bug.cgi?id=213931 to fix this.