Bug 213707 - Simplify Color's interface by removing isDark()
Summary: Simplify Color's interface by removing isDark()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-28 18:13 PDT by Sam Weinig
Modified: 2020-06-29 18:33 PDT (History)
10 users (show)

See Also:


Attachments
Patch (14.89 KB, patch)
2020-06-28 18:21 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (4.47 KB, patch)
2020-06-29 15:14 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (4.46 KB, patch)
2020-06-29 15:17 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2020-06-28 18:13:48 PDT
Simplify Color's interface by removing isDark() and lightness()
Comment 1 Sam Weinig 2020-06-28 18:21:16 PDT
Created attachment 403022 [details]
Patch
Comment 2 Sam Weinig 2020-06-28 20:14:46 PDT
Not a huge surprise that paste-dark-mode-color-filtered.html is failing, as the heuristic is changing. Need to see if the new results are acceptable.
Comment 3 Darin Adler 2020-06-29 09:57:36 PDT
Comment on attachment 403022 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        - Move Color::isDark to RenderThemeIOS.mm, it's one client and rename it

it's -> its

> Source/WebCore/rendering/RenderThemeIOS.mm:1055
> +static bool useConvexGradient(const Color& backgroundColor)

Typically we try not to name functions that compute values with verbs. So we would name it shouldUseConvexGradient.

> Source/WebKit/ChangeLog:10
> +        - Remove WebCore::lightness() and Color::lightness(), and replace with
> +          Color::luminance(), which is almost the same, but likely to be more
> +          in line with perceptual lightness people had in mind.

Maybe do the behavior change, the switch to luminance, first in a patch that doesn’t also refactor or remove.
Comment 4 Sam Weinig 2020-06-29 15:14:35 PDT
Created attachment 403120 [details]
Patch
Comment 5 Sam Weinig 2020-06-29 15:17:45 PDT
Created attachment 403121 [details]
Patch
Comment 6 Sam Weinig 2020-06-29 15:18:57 PDT
Just going to do the isDark change in this one. Leave the lightness() for separate change.
Comment 7 EWS 2020-06-29 18:32:06 PDT
Committed r263713: <https://trac.webkit.org/changeset/263713>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403121 [details].
Comment 8 Radar WebKit Bug Importer 2020-06-29 18:33:14 PDT
<rdar://problem/64917999>