Bug 157824 - Add media query support for wide gamut displays on Mac
Summary: Add media query support for wide gamut displays on Mac
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-05-17 17:59 PDT by Dean Jackson
Modified: 2016-05-22 10:28 PDT (History)
5 users (show)

See Also:


Attachments
Patch (10.55 KB, patch)
2016-05-17 18:06 PDT, Dean Jackson
simon.fraser: review+
Details | Formatted Diff | Diff
Patch (10.73 KB, patch)
2016-05-17 18:30 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (10.64 KB, patch)
2016-05-17 18:41 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-05-17 17:59:16 PDT
Add media query support for wide gamut displays on Mac
Comment 1 Dean Jackson 2016-05-17 17:59:49 PDT
<rdar://problem/26333137>
Comment 2 Dean Jackson 2016-05-17 18:06:21 PDT
Created attachment 279190 [details]
Patch
Comment 3 WebKit Commit Bot 2016-05-17 18:08:05 PDT
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.
Comment 4 Simon Fraser (smfr) 2016-05-17 18:08:58 PDT
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
Comment 5 Dean Jackson 2016-05-17 18:30:46 PDT
Created attachment 279195 [details]
Patch
Comment 6 WebKit Commit Bot 2016-05-17 18:31:44 PDT
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.
Comment 7 Dean Jackson 2016-05-17 18:41:14 PDT
Created attachment 279196 [details]
Patch
Comment 8 WebKit Commit Bot 2016-05-17 18:42:25 PDT
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.
Comment 9 Dean Jackson 2016-05-17 19:15:48 PDT
https://trac.webkit.org/r201065
Comment 10 Csaba Osztrogonác 2016-05-17 22:48:07 PDT
(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
Comment 11 Csaba Osztrogonác 2016-05-19 08:21:54 PDT
(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.
Comment 12 Darin Adler 2016-05-22 10:19:39 PDT
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());
Comment 13 Darin Adler 2016-05-22 10:22:45 PDT
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;
Comment 14 Darin Adler 2016-05-22 10:28:21 PDT
I’ll do these.