Bug 184096 – Consolidate NSColor to WebCore::Color conversion and fix system colors with alpha

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Timothy Hatcher]

Some system colors use alpha, and RenderThemeMac is not honoring that when converting the NSColor to RGB. All conversions should use colorFromNSColor in ColorMac.h.

[Timothy Hatcher]

<rdar://problem/38918925>

[Timothy Hatcher]

Created attachment 336676 [details]
Patch

[Tim Horton]

Comment on attachment 336676 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/ColorMac.mm:55
> +static RGBA32 makeRGBAFromNSColor(NSColor *color, NSColorSpace *usingColorSpace)

There's no point in adding this parameter if the only thing you pass is Device. Device is sRGB now.

> Source/WebCore/platform/graphics/mac/ColorMac.mm:81
> +        [NSGraphicsContext saveGraphicsState];

Surely we have an RAII something for this.

> Source/WebCore/platform/graphics/mac/ColorMac.mm:82
> +        [NSGraphicsContext setCurrentContext:[NSGraphicsContext graphicsContextWithBitmapImageRep:offscreenRep]];

Should this be a LocalCurrentGraphicsContext so that we put it back?

[Timothy Hatcher]

Comment on attachment 336676 [details]
Patch

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

>> Source/WebCore/platform/graphics/mac/ColorMac.mm:55
>> +static RGBA32 makeRGBAFromNSColor(NSColor *color, NSColorSpace *usingColorSpace)
> 
> There's no point in adding this parameter if the only thing you pass is Device. Device is sRGB now.

I didn't know, or forgot, that Device is sRGB. This function was used by a number of places in WebKit code, with one parameter, that assumed sRGB was used. So I didn't want to change those calls. But I can drop it and assume sRGB.

>> Source/WebCore/platform/graphics/mac/ColorMac.mm:82
>> +        [NSGraphicsContext setCurrentContext:[NSGraphicsContext graphicsContextWithBitmapImageRep:offscreenRep]];
> 
> Should this be a LocalCurrentGraphicsContext so that we put it back?

Yes, LocalCurrentGraphicsContext would be better to use here. I was moving the code and not thinking to hard about this.

[Tim Horton]

(In reply to Timothy Hatcher from comment #4)
> Comment on attachment 336676 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336676&action=review
> 
> >> Source/WebCore/platform/graphics/mac/ColorMac.mm:55
> >> +static RGBA32 makeRGBAFromNSColor(NSColor *color, NSColorSpace *usingColorSpace)
> > 
> > There's no point in adding this parameter if the only thing you pass is Device. Device is sRGB now.
> 
> I didn't know, or forgot, that Device is sRGB.

This changed a few years back, so you easily could know the wrong thing :)

> This function was used by a
> number of places in WebKit code, with one parameter, that assumed sRGB was
> used. So I didn't want to change those calls. But I can drop it and assume
> sRGB.
> 
> >> Source/WebCore/platform/graphics/mac/ColorMac.mm:82
> >> +        [NSGraphicsContext setCurrentContext:[NSGraphicsContext graphicsContextWithBitmapImageRep:offscreenRep]];
> > 
> > Should this be a LocalCurrentGraphicsContext so that we put it back?
> 
> Yes, LocalCurrentGraphicsContext would be better to use here. I was moving
> the code and not thinking to hard about this.

Sure!

[EWS Watchlist]

Comment on attachment 336676 [details]
Patch

Attachment 336676 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7126347

New failing tests:
accessibility/mac/abbr-acronym-tags.html
accessibility/mac/attributed-string-includes-highlighting.html
fast/css/apple-system-control-colors.html
accessibility/mac/attributed-string/attributed-string-does-not-includes-misspelled-for-non-editable.html
accessibility/mac/misspelled-attributed-string.html
accessibility/content-editable-as-textarea.html
accessibility/mac/attributed-string/attributed-string-for-range-with-options.html

[EWS Watchlist]

Created attachment 336684 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 336676 [details]
Patch

Attachment 336676 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7126525

New failing tests:
fast/css/apple-system-control-colors.html
accessibility/mac/abbr-acronym-tags.html
accessibility/mac/misspelled-attributed-string.html
accessibility/content-editable-as-textarea.html
accessibility/mac/attributed-string/attributed-string-for-range-with-options.html

[EWS Watchlist]

Created attachment 336690 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 336676 [details]
Patch

Attachment 336676 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7126486

New failing tests:
accessibility/mac/abbr-acronym-tags.html
accessibility/mac/attributed-string-includes-highlighting.html
fast/css/apple-system-control-colors.html
accessibility/mac/attributed-string/attributed-string-does-not-includes-misspelled-for-non-editable.html
accessibility/mac/misspelled-attributed-string.html
accessibility/content-editable-as-textarea.html
accessibility/mac/attributed-string/attributed-string-for-range-with-options.html

[EWS Watchlist]

Created attachment 336692 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Timothy Hatcher]

Created attachment 336714 [details]
Patch

[Timothy Hatcher]

Created attachment 336715 [details]
Patch

[Timothy Hatcher]

Created attachment 336722 [details]
Patch

[EWS Watchlist]

Comment on attachment 336722 [details]
Patch

Attachment 336722 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7129607

New failing tests:
fast/css/apple-system-control-colors.html

[EWS Watchlist]

Created attachment 336727 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 336722 [details]
Patch

Attachment 336722 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7129638

New failing tests:
fast/css/apple-system-control-colors.html

[EWS Watchlist]

Created attachment 336728 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Timothy Hatcher]

Created attachment 336729 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 336729 [details]
Patch

Rejecting attachment 336729 [details] from commit-queue.

Number of test failures exceeded the failure limit.
Full output: http://webkit-queues.webkit.org/results/7130473

[WebKit Commit Bot]

Created attachment 336734 [details]
Archive of layout-test-results from webkit-cq-02 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-02  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Timothy Hatcher]

Comment on attachment 336729 [details]
Patch

Unrelated HTTP server crash.

[Timothy Hatcher]

Created attachment 336741 [details]
Patch

[Timothy Hatcher]

Created attachment 336744 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 336744 [details]
Patch

Clearing flags on attachment: 336744

Committed r230064: <https://trac.webkit.org/changeset/230064>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.