Add CSS support for font-variation-settings

Add CSS support for font-variation-settings

Implemented analogously to font-feature-settings, as their definition in
the spec is quite similar as well [1].

Add an experimental web platform features flag for CSSVariableFonts.

[1] https://drafts.csswg.org/css-fonts-4/#low-level-font-variation-settings-control-the-font-variation-settings-property

TEST=fast/css/font-variation-settings-css-support.html
BUG=669455, 669460
Committed: https://crrev.com/ba39ed57a09afdcf993eff77d70268b8c110be34
Cr-Commit-Position: refs/heads/master@{#437853}

[drott, 2016-12-07 14:39:41]

Description was changed from

==========
Add CSS support for font-variation-settings

Implemented analogously to font-feature-settings, as their definition in
the spec is quite similar as well.

Add an experimental web platform features flag for CSSVariableFonts.

TEST=fast/css/font-variation-settings-css-support.html
BUG=669455,669460
==========

to

==========
Add CSS support for font-variation-settings

Implemented analogously to font-feature-settings, as their definition in
the spec is quite similar as well [1].

Add an experimental web platform features flag for CSSVariableFonts.

[1]
https://drafts.csswg.org/css-fonts-4/#low-level-font-variation-settings-contr...

TEST=fast/css/font-variation-settings-css-support.html
BUG=669455,669460
==========

[drott, 2016-12-07 14:40:23]

drott@chromium.org changed reviewers:
+ sashab@chromium.org, timloh@chromium.org

[drott, 2016-12-07 14:40:37]

PTAL, thanks.

[drott, 2016-12-07 14:40:54]

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

[commit-bot: I haz the power, 2016-12-07 14:41:03]

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

[drott, 2016-12-07 14:55:05]

Remove empty if statement

[drott, 2016-12-07 14:55:06]

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

[commit-bot: I haz the power, 2016-12-07 14:55:23]

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, 2016-12-07 16:50:26]

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

[commit-bot: I haz the power, 2016-12-07 16:50:26]

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

[Timothy Loh, 2016-12-08 03:36:05]

Looks good, but there are a few crashing tests?

https://codereview.chromium.org/2558053002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css/resources/font-variation-settings-css-support.js
(right):

https://codereview.chromium.org/2558053002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/resources/font-variation-settings-css-support.js:4:
"'abcd' 0.3, 'efgh' 1.4",
Let's have at least one test with a negative value.

https://codereview.chromium.org/2558053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
(right):

https://codereview.chromium.org/2558053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:2368: if
(RuntimeEnabledFeatures::cssVariableFontsEnabled()) {
I think this can be a DCHECK? Otherwise it's weird because we'd fall-through to
the next case if it's not enabled.

https://codereview.chromium.org/2558053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2558053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:423: if
(range.peek().type() == NumberToken) {
You probably want to use consumeNumberRaw to be able to handle calcs.

https://codereview.chromium.org/2558053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/2558053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:1287: return
RuntimeEnabledFeatures::cssVariableFontsEnabled();
It looks like this (and the text-deco props) should just return true and DCHECK
the flag, but I'm not sure.

[drott, 2016-12-08 07:53:45]

Addressing Tim's review comments, adjusting test expectations

[drott, 2016-12-08 07:53:46]

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

[commit-bot: I haz the power, 2016-12-08 07:53:59]

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, 2016-12-08 10:16:09]

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

[commit-bot: I haz the power, 2016-12-08 10:16:10]

Dry run: This issue passed the CQ dry run.

[drott, 2016-12-09 10:25:50]

Thanks for the review, updated.

https://codereview.chromium.org/2558053002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css/resources/font-variation-settings-css-support.js
(right):

https://codereview.chromium.org/2558053002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/resources/font-variation-settings-css-support.js:4:
"'abcd' 0.3, 'efgh' 1.4",
On 2016/12/08 at 03:36:05, Timothy Loh wrote:
> Let's have at least one test with a negative value.

Added.

https://codereview.chromium.org/2558053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
(right):

https://codereview.chromium.org/2558053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:2368: if
(RuntimeEnabledFeatures::cssVariableFontsEnabled()) {
On 2016/12/08 at 03:36:05, Timothy Loh wrote:
> I think this can be a DCHECK? Otherwise it's weird because we'd fall-through
to the next case if it's not enabled.

Changed to DCHECK.

https://codereview.chromium.org/2558053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2558053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:423: if
(range.peek().type() == NumberToken) {
On 2016/12/08 at 03:36:05, Timothy Loh wrote:
> You probably want to use consumeNumberRaw to be able to handle calcs.

Changed to use consumeNumberRaw.

https://codereview.chromium.org/2558053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/2558053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:1287: return
RuntimeEnabledFeatures::cssVariableFontsEnabled();
On 2016/12/08 at 03:36:05, Timothy Loh wrote:
> It looks like this (and the text-deco props) should just return true and
DCHECK the flag, but I'm not sure.

Makes sense to me, changed, and corrected the inverted logic of the DCHECK
below.

[Timothy Loh, 2016-12-12 02:15:38]

lgtm

https://codereview.chromium.org/2558053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/2558053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:1394:
DCHECK(!RuntimeEnabledFeatures::css3TextDecorationsEnabled());
This one is correct -- when the flag is off we treat text-decoration as a
property but when it's on we treat it as a shorthand (so we should only have the
longhand values when we get here).

https://codereview.chromium.org/2558053002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/resource/FontResource.h (right):

https://codereview.chromium.org/2558053002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/resource/FontResource.h:1: 
unintended change?

[drott, 2016-12-12 08:14:03]

The CQ bit was checked by drott@chromium.org

[drott, 2016-12-12 08:14:04]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/2558053002/#ps60001
(title: "DCHECK corrected, newline removed.")

[commit-bot: I haz the power, 2016-12-12 08:14:20]

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, 2016-12-12 08:23:22]

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

[commit-bot: I haz the power, 2016-12-12 08:23:24]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[drott, 2016-12-12 09:02:28]

drott@chromium.org changed reviewers:
+ jochen@chromium.org, mkwst@chromium.org

[drott, 2016-12-12 09:02:50]

Jochen, Mike, could you please take a look at the histograms.xml change? Thanks.

[Mike West, 2016-12-12 11:44:41]

histograms.xml LGTM.

[drott, 2016-12-12 11:45:20]

The CQ bit was checked by drott@chromium.org

[commit-bot: I haz the power, 2016-12-12 11:45:34]

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, 2016-12-12 12:40:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481543120460740,
"parent_rev": "26486538af80a09ca5882cf65006b3a60b240d8d", "commit_rev":
"bc8c02d3996d81becf73fd75f6977e51892df8b7"}

[commit-bot: I haz the power, 2016-12-12 12:40:42]

Description was changed from

==========
Add CSS support for font-variation-settings

Implemented analogously to font-feature-settings, as their definition in
the spec is quite similar as well [1].

Add an experimental web platform features flag for CSSVariableFonts.

[1]
https://drafts.csswg.org/css-fonts-4/#low-level-font-variation-settings-contr...

TEST=fast/css/font-variation-settings-css-support.html
BUG=669455,669460
==========

to

==========
Add CSS support for font-variation-settings

Implemented analogously to font-feature-settings, as their definition in
the spec is quite similar as well [1].

Add an experimental web platform features flag for CSSVariableFonts.

[1]
https://drafts.csswg.org/css-fonts-4/#low-level-font-variation-settings-contr...

TEST=fast/css/font-variation-settings-css-support.html
BUG=669455,669460

Review-Url: https://codereview.chromium.org/2558053002
==========

[commit-bot: I haz the power, 2016-12-12 12:40:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-12 15:11:39]

Description was changed from

==========
Add CSS support for font-variation-settings

Implemented analogously to font-feature-settings, as their definition in
the spec is quite similar as well [1].

Add an experimental web platform features flag for CSSVariableFonts.

[1]
https://drafts.csswg.org/css-fonts-4/#low-level-font-variation-settings-contr...

TEST=fast/css/font-variation-settings-css-support.html
BUG=669455,669460

Review-Url: https://codereview.chromium.org/2558053002
==========

to

==========
Add CSS support for font-variation-settings

Implemented analogously to font-feature-settings, as their definition in
the spec is quite similar as well [1].

Add an experimental web platform features flag for CSSVariableFonts.

[1]
https://drafts.csswg.org/css-fonts-4/#low-level-font-variation-settings-contr...

TEST=fast/css/font-variation-settings-css-support.html
BUG=669455,669460

Committed: https://crrev.com/ba39ed57a09afdcf993eff77d70268b8c110be34
Cr-Commit-Position: refs/heads/master@{#437853}
==========

[commit-bot: I haz the power, 2016-12-12 15:11:41]

Patchset 4 (id:??) landed as
https://crrev.com/ba39ed57a09afdcf993eff77d70268b8c110be34
Cr-Commit-Position: refs/heads/master@{#437853}