Bug 157824 – Add media query support for wide gamut displays on Mac

• Component: New Bugs
• Status: RESOLVED FIXED

[Dean Jackson]

Add media query support for wide gamut displays on Mac

[Dean Jackson]

<rdar://problem/26333137>

[Dean Jackson]

Created attachment 279190 [details]
Patch

[WebKit Commit Bot]

Attachment 279190 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/PlatformScreen.h:66:  The parameter name "widget" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/spi/cg/CoreGraphicsSPI.h:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:249:  color_gamutMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 279190 [details]
Patch

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

> Source/WebCore/css/MediaQueryEvaluator.cpp:249
> +static bool color_gamutMediaFeatureEval(CSSValue* value, const CSSToLengthConversionData&, Frame* frame, MediaFeaturePrefix)

Can this be a Frame&?

> Source/WebCore/platform/mac/PlatformScreenMac.mm:159
> +    CFDataRef iccData = CGColorSpaceCopyICCProfile(colorSpace);

RetainPtr

> Source/WebCore/platform/mac/PlatformScreenMac.mm:160
> +    ColorSyncProfileRef profile = ColorSyncProfileCreate(iccData, NULL);

RetainPtr

[Dean Jackson]

Created attachment 279195 [details]
Patch

[WebKit Commit Bot]

Attachment 279195 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/PlatformScreen.h:66:  The parameter name "widget" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:249:  color_gamutMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Created attachment 279196 [details]
Patch

[WebKit Commit Bot]

Attachment 279196 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/PlatformScreen.h:66:  The parameter name "widget" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:249:  color_gamutMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

https://trac.webkit.org/r201065

[Csaba Osztrogonác]

(In reply to comment #9)
> https://trac.webkit.org/r201065

It broke the Apple Mac cmke build: https://build.webkit.org/builders/Apple%20El%20Capitan%20CMake%20Debug%20%28Build%29/builds/5181

[Csaba Osztrogonác]

(In reply to comment #10)
> (In reply to comment #9)
> > https://trac.webkit.org/r201065
> 
> It broke the Apple Mac cmke build:
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20CMake%20Debug%20%28Build%29/builds/5181

build log:

/Volumes/Data/slave/elcapitan-cmake-debug/build/Source/WebCore/platform/spi/cg/CoreGraphicsSPI.h:37:10: fatal error: 'ColorSync/ColorSync.h' file not found
#include <ColorSync/ColorSync.h>
         ^
1 error generated.

[Darin Adler]

Comment on attachment 279196 [details]
Patch

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

Some null checks and coding style tweak thoughts.

> Source/WebCore/css/MediaQueryEvaluator.cpp:260
> +        return screenSupportsExtendedColor(frame->page()->mainFrame().view());

Two thoughts:

1) No null check needed for frame? We should change this file so it uses a reference for those arguments if they are guaranteed never to be null.
2) Why not use frame->mainFrame().view() without mentioning page()? That will obviate the need for a null check of page(), and will be more efficient, too.

> Source/WebCore/platform/mac/PlatformScreenMac.mm:152
> +    NSWindow *window = [widget->platformWidget() window];
> +    NSScreen *screen = screenForWidget(widget, window);
> +    CGColorSpaceRef colorSpace = screen.colorSpace.CGColorSpace;

Maybe this is sacrilege, but I would be tempted to write this all in one line:

    auto colorSpace = screenForWidget(widget, [widget->platformWidget() window]).colorSpace.CGColorSpace;

> Source/WebCore/platform/mac/PlatformScreenMac.mm:158
> +    RetainPtr<CFDataRef> iccData = adoptCF(CGColorSpaceCopyICCProfile(colorSpace));

auto is better, I think

> Source/WebCore/platform/mac/PlatformScreenMac.mm:159
> +    RetainPtr<ColorSyncProfileRef> profile = adoptCF(ColorSyncProfileCreate(iccData.get(), NULL));

auto is better here too, I think.

> Source/WebCore/platform/mac/PlatformScreenMac.mm:162
> +    if (profile)
> +        isWideGamut = ColorSyncProfileIsWideGamut(profile.get());
> +    return isWideGamut;

No need for the local variable. We should write this:

    return profile && ColorSyncProfileIsWideGamut(profile.get());

[Darin Adler]

Comment on attachment 279196 [details]
Patch

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

>> Source/WebCore/platform/mac/PlatformScreenMac.mm:152
>> +    CGColorSpaceRef colorSpace = screen.colorSpace.CGColorSpace;
> 
> Maybe this is sacrilege, but I would be tempted to write this all in one line:
> 
>     auto colorSpace = screenForWidget(widget, [widget->platformWidget() window]).colorSpace.CGColorSpace;

Actually, we need to fix screenForWidget and remove its NSWindow argument. All callers find the window the same way, by calling [widget->platformWidget() window]. So that makes this even better:

    auto colorSpace = screenForWidget(widget).colorSpace.CGColorSpace;

[Darin Adler]

I’ll do these.