Implement color-gamut media query

Implement color-gamut media query

This is implementing the following specification:
https://drafts.csswg.org/mediaqueries-4/#color-gamut

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/36CcloDrB3E/1wMSNMl9BQAJ

BUG=685456
Review-Url: https://codereview.chromium.org/2652313004
Cr-Commit-Position: refs/heads/master@{#446946}
Committed: https://chromium.googlesource.com/chromium/src/+/2575c4f8b8a7172537a762d069486d906bed1076

[mlamouri (slow - plz ping), 2017-01-27 07:39:19]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 07:39:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mlamouri (slow - plz ping), 2017-01-27 07:41:18]

mlamouri@chromium.org changed reviewers:
+ ccameron@chromium.org, eae@chromium.org, hubbe@chromium.org

[mlamouri (slow - plz ping), 2017-01-27 07:41:19]

hubbe@, PTAL at the use of your algorithm.
eae@, PTAL at the implementation
ccameron@, FYI :)

[commit-bot: I haz the power, 2017-01-27 07:41:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 07:41:24]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Yoav Weiss, 2017-01-27 12:13:38]

yoav@yoav.ws changed reviewers:
+ yoav@yoav.ws

[Yoav Weiss, 2017-01-27 12:13:39]

core/css/ LGTM

Can you add some more tests for this MQ with `<picture>`? I suspect it will be
used there, so it'd be better to make sure the two are working properly
together.

https://codereview.chromium.org/2652313004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/MediaQueryEvaluator.cpp (right):

https://codereview.chromium.org/2652313004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/MediaQueryEvaluator.cpp:789: case
ColorSpaceGamut::End:
Potential nit: Can we use `default:` here and avoid one of the returns?

https://codereview.chromium.org/2652313004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/MediaValuesCached.cpp (right):

https://codereview.chromium.org/2652313004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/MediaValuesCached.cpp:33:
colorGamut(ColorSpaceGamut::Unknown) {}
Oh, so when we were creating MediaValuesCached() all these values were
uninitialized? Thanks for catching that!

[Yoav Weiss, 2017-01-27 12:15:36]

On 2017/01/27 12:13:39, Yoav Weiss wrote:
> core/css/ LGTM

One exception to my LGTM is the CSSValueKeywords.json5 file, which I'm not
familiar with, so someone else should review that.

[mlamouri (slow - plz ping), 2017-01-27 16:54:29]

For the <picture>, I looked into this but after a quick check (MDN), I couldn't
find an API telling me which image was selected. Maybe I was too lazy to do a
good ref test. Maybe :)

https://codereview.chromium.org/2652313004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/MediaQueryEvaluator.cpp (right):

https://codereview.chromium.org/2652313004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/MediaQueryEvaluator.cpp:789: case
ColorSpaceGamut::End:
On 2017/01/27 at 12:13:38, Yoav Weiss wrote:
> Potential nit: Can we use `default:` here and avoid one of the returns?

I prefer to avoid `default` so if a new value gets added, it will fail to
compile. Most compilers will understand that all values of the enum are handled
but every time I try one Android or Microsoft compiler is too dumb. Happy to
give it a shot again :)

https://codereview.chromium.org/2652313004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/MediaValuesCached.cpp (right):

https://codereview.chromium.org/2652313004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/MediaValuesCached.cpp:33:
colorGamut(ColorSpaceGamut::Unknown) {}
On 2017/01/27 at 12:13:38, Yoav Weiss wrote:
> Oh, so when we were creating MediaValuesCached() all these values were
uninitialized? Thanks for catching that!

They were not, I moved it from the header so I didn't have to add ColorSpace.h
:)

[Yoav Weiss, 2017-01-27 16:58:14]

On 2017/01/27 16:54:29, mlamouri (slow) wrote:
> For the <picture>, I looked into this but after a quick check (MDN), I
couldn't
> find an API telling me which image was selected. Maybe I was too lazy to do a
> good ref test. Maybe :)

HTMLImageElement.currentSrc will give you the URL that was picked

> 
>
https://codereview.chromium.org/2652313004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/css/MediaQueryEvaluator.cpp (right):
> 
>
https://codereview.chromium.org/2652313004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/css/MediaQueryEvaluator.cpp:789: case
> ColorSpaceGamut::End:
> On 2017/01/27 at 12:13:38, Yoav Weiss wrote:
> > Potential nit: Can we use `default:` here and avoid one of the returns?
> 
> I prefer to avoid `default` so if a new value gets added, it will fail to
> compile. Most compilers will understand that all values of the enum are
handled
> but every time I try one Android or Microsoft compiler is too dumb. Happy to
> give it a shot again :)

Just a nit. Cleaner if you remove the extra return, but not awful if you don't.

> 
>
https://codereview.chromium.org/2652313004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/css/MediaValuesCached.cpp (right):
> 
>
https://codereview.chromium.org/2652313004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/css/MediaValuesCached.cpp:33:
> colorGamut(ColorSpaceGamut::Unknown) {}
> On 2017/01/27 at 12:13:38, Yoav Weiss wrote:
> > Oh, so when we were creating MediaValuesCached() all these values were
> uninitialized? Thanks for catching that!
> 
> They were not, I moved it from the header so I didn't have to add ColorSpace.h
> :)

OK. I feel better then :)

[hubbe, 2017-01-27 18:19:38]

LGTM (with one suggestion)

https://codereview.chromium.org/2652313004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/MediaQueryEvaluator.cpp (right):

https://codereview.chromium.org/2652313004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/MediaQueryEvaluator.cpp:789: case
ColorSpaceGamut::End:
On 2017/01/27 16:54:29, mlamouri (slow) wrote:
> On 2017/01/27 at 12:13:38, Yoav Weiss wrote:
> > Potential nit: Can we use `default:` here and avoid one of the returns?
> 
> I prefer to avoid `default` so if a new value gets added, it will fail to
> compile. Most compilers will understand that all values of the enum are
handled
> but every time I try one Android or Microsoft compiler is too dumb. Happy to
> give it a shot again :)


I think it would be easier to just do something like:

switch (value.id) {
   case CSSValueP3:
       return gamut >= ColorSpaceGamut::P3;
   ....
}

[mlamouri (slow - plz ping), 2017-01-27 18:54:17]

https://codereview.chromium.org/2652313004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/MediaQueryEvaluator.cpp (right):

https://codereview.chromium.org/2652313004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/MediaQueryEvaluator.cpp:789: case
ColorSpaceGamut::End:
On 2017/01/27 at 18:19:37, hubbe wrote:
> On 2017/01/27 16:54:29, mlamouri (slow) wrote:
> > On 2017/01/27 at 12:13:38, Yoav Weiss wrote:
> > > Potential nit: Can we use `default:` here and avoid one of the returns?
> > 
> > I prefer to avoid `default` so if a new value gets added, it will fail to
> > compile. Most compilers will understand that all values of the enum are
handled
> > but every time I try one Android or Microsoft compiler is too dumb. Happy to
> > give it a shot again :)
> 
> 
> I think it would be easier to just do something like:
> 
> switch (value.id) {
>    case CSSValueP3:
>        return gamut >= ColorSpaceGamut::P3;
>    ....
> }

That is true but because `value.id` isn't a "restricted" enum, new values on
either side (color-gamut MQ or ColorSpaceGamut) could be added without this code
being looked at. The primary goal here is making sure we don't forget about this
code :)

[eae, 2017-01-27 20:19:36]

LGTM for Source/platform

[mlamouri (slow - plz ping), 2017-01-28 01:33:20]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-28 01:34:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mlamouri (slow - plz ping), 2017-01-28 01:38:00]

All comments applied. I will land this when the intent will be approved.

ccameron@, it should leave you enough time to look at the CL if you want to. I
assume that hubbe@'s review should be enough on the color logic side otherwise.

[ccameron, 2017-01-28 01:46:32]

lgtm as well

[commit-bot: I haz the power, 2017-01-28 03:27:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-28 03:27:59]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[mlamouri (slow - plz ping), 2017-01-28 17:20:27]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2017-01-28 17:20:28]

The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws,
hubbe@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2652313004/#ps20001
(title: "moar tests")

[commit-bot: I haz the power, 2017-01-28 17:20:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-28 19:19:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-28 19:19:29]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[mlamouri (slow - plz ping), 2017-01-28 20:14:03]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-28 20:14:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-28 20:50:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-28 20:50:27]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[mlamouri (slow - plz ping), 2017-01-29 00:20:07]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-29 00:20:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-29 01:43:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-29 01:43:19]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[pdknsk, 2017-01-29 15:44:54]

I think the gamut-comparison is implemented somewhat poorly. (Sorry.)

As per spec.

> An output device can return true for multiple values of this media feature, if
its full output gamut is large enough, or one gamut is a subset of another
supported gamut.

Currently it basically averages the "wideness" across all primaries, rather than
check if the gamut is actually a subset (or larger than).

[jochen (gone - plz use gerrit), 2017-01-29 17:25:32]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2017-01-29 17:25:33]

https://codereview.chromium.org/2652313004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/MediaValuesCached.h (right):

https://codereview.chromium.org/2652313004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/MediaValuesCached.h:17: struct
MediaValuesCachedData final {
also put CORE_EXPORT on the struct

[mlamouri (slow - plz ping), 2017-01-29 17:32:53]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2017-01-29 17:32:54]

The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws,
hubbe@chromium.org, eae@chromium.org, ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/2652313004/#ps80001
(title: "fix windows build")

[commit-bot: I haz the power, 2017-01-29 17:33:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mlamouri (slow - plz ping), 2017-01-29 17:34:12]

pdknsk@, thanks for the comment. We are aware of this and hubbe@ believes this
approach is working. Feel free to file a bug to follow-up on this.

jochen@, thanks! \o/

[commit-bot: I haz the power, 2017-01-29 21:07:44]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485711173793790,
"parent_rev": "39063b5d891442ff46c836a4bcef645ba4ad7b40", "commit_rev":
"2575c4f8b8a7172537a762d069486d906bed1076"}

[commit-bot: I haz the power, 2017-01-29 21:08:41]

Description was changed from

==========
Implement color-gamut media query

This is implementing the following specification:
https://drafts.csswg.org/mediaqueries-4/#color-gamut

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/36CcloDrB3E/1wMSNMl9...

BUG=685456
==========

to

==========
Implement color-gamut media query

This is implementing the following specification:
https://drafts.csswg.org/mediaqueries-4/#color-gamut

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/36CcloDrB3E/1wMSNMl9...

BUG=685456

Review-Url: https://codereview.chromium.org/2652313004
Cr-Commit-Position: refs/heads/master@{#446946}
Committed:
https://chromium.googlesource.com/chromium/src/+/2575c4f8b8a7172537a762d06948...
==========

[commit-bot: I haz the power, 2017-01-29 21:08:43]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/2575c4f8b8a7172537a762d06948...

[Noel Gordon, 2017-01-31 02:29:56]

noel@chromium.org changed reviewers:
+ noel@chromium.org

[Noel Gordon, 2017-01-31 02:29:57]

https://codereview.chromium.org/2652313004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ColorSpace.cpp (right):

https://codereview.chromium.org/2652313004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ColorSpace.cpp:130: ColorSpaceGamut
getColorSpaceGamut(SkColorSpace* colorSpace) {
The color profile media spec does not say how to compute the color space gamut. 
How do we achieve compat while that is so?

https://codereview.chromium.org/2652313004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ColorSpace.cpp:151: float score =
out[0][0] * out[1][1] * out[2][2];
Aside: by the looks, this appears to compute a score from the principle diagnol
of the tristimulus matrix.   That is a poor measure of gamut area for profiles
having strong off-diagnol elements.  I expect that this score will produces
outliers, in that it will classify some wide gamut profiles as not being wide
gamut [1].

[1] And again for browser compat, this is the reason why the media color profile
spec needs to specify how to compute the gamut area.

[hubbe, 2017-01-31 04:06:20]

https://codereview.chromium.org/2652313004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ColorSpace.cpp (right):

https://codereview.chromium.org/2652313004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ColorSpace.cpp:151: float score =
out[0][0] * out[1][1] * out[2][2];
On 2017/01/31 02:29:57, noel gordon wrote:
> Aside: by the looks, this appears to compute a score from the principle
diagnol
> of the tristimulus matrix.   That is a poor measure of gamut area for profiles
> having strong off-diagnol elements.  I expect that this score will produces
> outliers, in that it will classify some wide gamut profiles as not being wide
> gamut [1].
> 
> [1] And again for browser compat, this is the reason why the media color
profile
> spec needs to specify how to compute the gamut area.

While I agree that this is a hacky method, I do think it is good enough to
work[1]. The matrix is in the sRGB coordinate space, so if the primaries are
more or less "red, green and blue" they will be roughly parallel with the sRGB
primaries. While other primaries are theoretically possible, we do not currently
support non-RGB colorspaces.

If we want a better method, we'll probably want to measure triangle overlap in
XYZ or CIELAB space...

[1] but not good enough to be standardized.

[Noel Gordon, 2017-02-01 13:44:29]

On 2017/01/31 04:06:20, hubbe wrote:
>
https://codereview.chromium.org/2652313004/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/ColorSpace.cpp (right):
> 
>
https://codereview.chromium.org/2652313004/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/ColorSpace.cpp:151: float score =
> out[0][0] * out[1][1] * out[2][2];
> On 2017/01/31 02:29:57, noel gordon wrote:
> > Aside: by the looks, this appears to compute a score from the principle
> diagnol
> > of the tristimulus matrix.   That is a poor measure of gamut area for
profiles
> > having strong off-diagnol elements.  I expect that this score will produces
> > outliers, in that it will classify some wide gamut profiles as not being
wide
> > gamut [1].
> > 
> > [1] And again for browser compat, this is the reason why the media color
> profile
> > spec needs to specify how to compute the gamut area.
> 
> While I agree that this is a hacky method, I do think it is good enough to
> work[1]. The matrix is in the sRGB coordinate space, so if the primaries are
> more or less "red, green and blue" they will be roughly parallel with the sRGB
> primaries. While other primaries are theoretically possible, we do not
currently
> support non-RGB colorspaces.

I wouldn't say a hacky method, I'd say interesting :) and there will be possible
misclassifications.

> If we want a better method, we'll probably want to measure triangle overlap in
> XYZ or CIELAB space...

Agree, this would be better (and it is both simple and quick to do, there's an
implementation in QCMS already).

> [1] but not good enough to be standardized.

My main concern is if my screen gamut is somewhat b/w sRGB and P3 say, then the
media query impl has to decide which one: sRGB or P3.  If the spec doesn't say
how to do that exactly (doesn't seem to on my reading), then we will end up with
compat problems.  For example, does the implementation in this CL match Safari?

[hubbe, 2017-02-01 18:19:38]

On 2017/02/01 13:44:29, noel gordon wrote:
> On 2017/01/31 04:06:20, hubbe wrote:
> >
>
https://codereview.chromium.org/2652313004/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/platform/graphics/ColorSpace.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2652313004/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/platform/graphics/ColorSpace.cpp:151: float score
=
> > out[0][0] * out[1][1] * out[2][2];
> > On 2017/01/31 02:29:57, noel gordon wrote:
> > > Aside: by the looks, this appears to compute a score from the principle
> > diagnol
> > > of the tristimulus matrix.   That is a poor measure of gamut area for
> profiles
> > > having strong off-diagnol elements.  I expect that this score will
produces
> > > outliers, in that it will classify some wide gamut profiles as not being
> wide
> > > gamut [1].
> > > 
> > > [1] And again for browser compat, this is the reason why the media color
> > profile
> > > spec needs to specify how to compute the gamut area.
> > 
> > While I agree that this is a hacky method, I do think it is good enough to
> > work[1]. The matrix is in the sRGB coordinate space, so if the primaries are
> > more or less "red, green and blue" they will be roughly parallel with the
sRGB
> > primaries. While other primaries are theoretically possible, we do not
> currently
> > support non-RGB colorspaces.
> 
> I wouldn't say a hacky method, I'd say interesting :) and there will be
possible
> misclassifications.
> 
> > If we want a better method, we'll probably want to measure triangle overlap
in
> > XYZ or CIELAB space...
> 
> Agree, this would be better (and it is both simple and quick to do, there's an
> implementation in QCMS already).
> 
> > [1] but not good enough to be standardized.
> 
> My main concern is if my screen gamut is somewhat b/w sRGB and P3 say, then
the
> media query impl has to decide which one: sRGB or P3.  If the spec doesn't say
> how to do that exactly (doesn't seem to on my reading), then we will end up
with
> compat problems.  For example, does the implementation in this CL match
Safari?

We don't know what Safari does, so it would be odd if it matches.
(Extra super-odd since the I made up the algorithm and cut-offs used in this CL
myself.)
If someone wants to update the standard to specify the method, I don't mind
swapping out the algorithm.

[mlamouri (slow - plz ping), 2017-02-01 19:27:12]

FWIW, I think we should have this discussion on a bug or on a mailing list. Not
sure if a closed CL is the best medium :)

[Noel Gordon, 2017-02-20 11:41:30]

On 2017/02/01 18:19:38, hubbe wrote:

> We don't know what Safari does, so it would be odd if it matches.
> (Extra super-odd since the I made up the algorithm and cut-offs used in this
CL
> myself.)

I believe Safari is open source, and we can see exactly what they do: see

https://bugs.webkit.org/show_bug.cgi?id=157824

[Noel Gordon, 2017-02-20 11:42:38]

On 2017/02/01 19:27:12, mlamouri wrote:
> FWIW, I think we should have this discussion on a bug or on a mailing list.
Not
> sure if a closed CL is the best medium :)

Concur.  www-style@ would be perhaps the proper place, but I have seen no
message about this compat concern.  mlamouri@ if you need to contact the spec
editor, I can provide details.  Let me know how you'd like to proceed, perhaps
in your bug.