Bug 155994 - Add color-gamut media query support
Summary: Add color-gamut media query support
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: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-03-29 16:20 PDT by Dean Jackson
Modified: 2020-04-05 10:18 PDT (History)
7 users (show)

See Also:


Attachments
Patch (6.82 KB, patch)
2016-03-29 16:32 PDT, Dean Jackson
darin: review+
Details | Formatted Diff | Diff
Patch (9.27 KB, patch)
2016-04-04 14:50 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2016-03-29 16:20:38 PDT
Add color-gamut media query support
Comment 1 Dean Jackson 2016-03-29 16:31:44 PDT
<rdar://problem/23282326>
Comment 2 Dean Jackson 2016-03-29 16:32:17 PDT
Created attachment 275147 [details]
Patch
Comment 3 WebKit Commit Bot 2016-03-29 16:34:06 PDT
Attachment 275147 [details] did not pass style-queue:


ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:253:  color_gamutMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Darin Adler 2016-03-30 09:36:05 PDT
Comment on attachment 275147 [details]
Patch

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

They breaks GTK, EFL, and Windows builds because screenSupportsExtendedColor is declared but not defined for all those platforms. Please fix that before landing!

> Source/WebCore/css/MediaQueryEvaluator.cpp:258
> +    const CSSValueID id = downcast<CSSPrimitiveValue>(*value).getValueID();

The const here seems gratuitous. Most local variables could be marked const.

> Source/WebCore/css/MediaQueryEvaluator.cpp:265
> +    // FIXME: At some point we should start detecting displays that
> +    // support more colors. For the moment we'll just assume an "extended
> +    // color" display is at least as good as P3.

I think this FIXME could be clearer. I think the real issue is that we will always return false for rec2020 and it might be better to be more specific about this. Maybe the code should be written like this:

    switch (downcast<CSSPrimitiveValue>(*value).getValueID()) {
    case CSSValueSrgb:
        return true;
    case CSSValueP3:
        return screenSupportsExtendedColor();
    case CSSValueRec2020:
        return false;
    }

    ASSERT_NOT_REACHED();
    return true;

I think the behavior would be clearer then; the FIXME could go in the CSSValueRec2020 case.

> Source/WebCore/css/MediaQueryEvaluator.cpp:267
> +    return (id == CSSValueP3 && screenSupportsExtendedColor());

Please omit the parentheses if you actually keep this.
Comment 5 Dean Jackson 2016-04-04 14:50:36 PDT
Created attachment 275577 [details]
Patch
Comment 6 WebKit Commit Bot 2016-04-04 14:52:30 PDT
Attachment 275577 [details] did not pass style-queue:


ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:253:  color_gamutMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Dean Jackson 2016-04-04 15:21:10 PDT
Comment on attachment 275147 [details]
Patch

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

>> Source/WebCore/css/MediaQueryEvaluator.cpp:265
>> +    // color" display is at least as good as P3.
> 
> I think this FIXME could be clearer. I think the real issue is that we will always return false for rec2020 and it might be better to be more specific about this. Maybe the code should be written like this:
> 
>     switch (downcast<CSSPrimitiveValue>(*value).getValueID()) {
>     case CSSValueSrgb:
>         return true;
>     case CSSValueP3:
>         return screenSupportsExtendedColor();
>     case CSSValueRec2020:
>         return false;
>     }
> 
>     ASSERT_NOT_REACHED();
>     return true;
> 
> I think the behavior would be clearer then; the FIXME could go in the CSSValueRec2020 case.

Much better
Comment 8 Alexey Shvayka 2020-04-05 04:46:22 PDT
(In reply to Dean Jackson from comment #5)
> Created attachment 275577 [details]
> Patch

Landed in https://trac.webkit.org/changeset/199024.
Comment 9 Radar WebKit Bug Importer 2020-04-05 04:47:14 PDT
<rdar://problem/61313919>