Bug 184096 - Consolidate NSColor to WebCore::Color conversion and fix system colors with alpha
Summary: Consolidate NSColor to WebCore::Color conversion and fix system colors with a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Local Build
Hardware: Mac macOS 10.13
: P2 Normal
Assignee: Timothy Hatcher
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-28 10:26 PDT by Timothy Hatcher
Modified: 2018-03-28 19:39 PDT (History)
10 users (show)

See Also:


Attachments
Patch (16.41 KB, patch)
2018-03-28 10:55 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.57 MB, application/zip)
2018-03-28 11:43 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.88 MB, application/zip)
2018-03-28 12:08 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (3.47 MB, application/zip)
2018-03-28 12:33 PDT, EWS Watchlist
no flags Details
Patch (20.35 KB, patch)
2018-03-28 13:55 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (20.45 KB, patch)
2018-03-28 13:59 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (20.45 KB, patch)
2018-03-28 14:42 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.22 MB, application/zip)
2018-03-28 15:27 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.49 MB, application/zip)
2018-03-28 15:37 PDT, EWS Watchlist
no flags Details
Patch (20.44 KB, patch)
2018-03-28 15:42 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-cq-02 for mac-sierra (1.81 MB, application/zip)
2018-03-28 16:51 PDT, WebKit Commit Bot
no flags Details
Patch (20.44 KB, patch)
2018-03-28 18:13 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (20.22 KB, patch)
2018-03-28 19:01 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2018-03-28 10:26:40 PDT
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.
Comment 1 Timothy Hatcher 2018-03-28 10:26:52 PDT
<rdar://problem/38918925>
Comment 2 Timothy Hatcher 2018-03-28 10:55:21 PDT
Created attachment 336676 [details]
Patch
Comment 3 Tim Horton 2018-03-28 11:06:22 PDT
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?
Comment 4 Timothy Hatcher 2018-03-28 11:13:31 PDT
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.
Comment 5 Tim Horton 2018-03-28 11:15:43 PDT
(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!
Comment 6 EWS Watchlist 2018-03-28 11:43:46 PDT
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
Comment 7 EWS Watchlist 2018-03-28 11:43:48 PDT
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
Comment 8 EWS Watchlist 2018-03-28 12:08:05 PDT
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
Comment 9 EWS Watchlist 2018-03-28 12:08:06 PDT
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
Comment 10 EWS Watchlist 2018-03-28 12:33:08 PDT
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
Comment 11 EWS Watchlist 2018-03-28 12:33:10 PDT
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
Comment 12 Timothy Hatcher 2018-03-28 13:55:36 PDT
Created attachment 336714 [details]
Patch
Comment 13 Timothy Hatcher 2018-03-28 13:59:05 PDT
Created attachment 336715 [details]
Patch
Comment 14 Timothy Hatcher 2018-03-28 14:42:48 PDT
Created attachment 336722 [details]
Patch
Comment 15 EWS Watchlist 2018-03-28 15:27:50 PDT
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
Comment 16 EWS Watchlist 2018-03-28 15:27:52 PDT
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
Comment 17 EWS Watchlist 2018-03-28 15:37:26 PDT
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
Comment 18 EWS Watchlist 2018-03-28 15:37:27 PDT
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
Comment 19 Timothy Hatcher 2018-03-28 15:42:07 PDT
Created attachment 336729 [details]
Patch
Comment 20 WebKit Commit Bot 2018-03-28 16:51:20 PDT
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
Comment 21 WebKit Commit Bot 2018-03-28 16:51:22 PDT
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
Comment 22 Timothy Hatcher 2018-03-28 17:07:48 PDT
Comment on attachment 336729 [details]
Patch

Unrelated HTTP server crash.
Comment 23 Timothy Hatcher 2018-03-28 18:13:57 PDT
Created attachment 336741 [details]
Patch
Comment 24 Timothy Hatcher 2018-03-28 19:01:28 PDT
Created attachment 336744 [details]
Patch
Comment 25 WebKit Commit Bot 2018-03-28 19:39:40 PDT
Comment on attachment 336744 [details]
Patch

Clearing flags on attachment: 336744

Committed r230064: <https://trac.webkit.org/changeset/230064>
Comment 26 WebKit Commit Bot 2018-03-28 19:39:41 PDT
All reviewed patches have been landed.  Closing bug.