Issue 1429713004: Implement CSS font-display

Issue 1429713004: Implement CSS font-display (Closed)

Created:
5 years, 1 month ago by Kunihiko Sakamoto

Modified:
5 years ago

Reviewers:
kinuko, Yoav Weiss, eae

CC:
chromium-reviews, tyoshino+watch_chromium.org, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, gavinp+loader_chromium.org, darktears, blink-reviews, loading-reviews+fetch_chromium.org, Nate Chapin, rwlbuis, drott, Takashi Toyoshima

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Implement CSS font-display Initial implementation of CSS font-display, a new @font-face descriptor behind the experimental web platform feature flag. Spec: https://tabatkins.github.io/specs/css-font-display/ Intent to implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/7s4-eQTAxqs/SoahsGpMAQAJ BUG=530015 Committed: https://crrev.com/d034ad372b0d7fdf5a3f04e16eddbb111ccbedb3 Cr-Commit-Position: refs/heads/master@{#361322}

Patch Set 1 #

Total comments: 6

Patch Set 2 : #

Total comments: 5

Patch Set 3 : rebase #

Patch Set 4 : kinuko's comment, rebase #

Patch Set 5 : rebase #

Patch Set 6 : failure does not mean loaded #

Created: 5 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+289 lines, -35 lines)			Patch
A	third_party/WebKit/LayoutTests/http/tests/webfont/font-display.html	View	1	1 chunk	+67 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/http/tests/webfont/font-display-expected.html	View	1	1 chunk	+86 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/webexposed/css-properties-as-js-properties-expected.txt	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/CSSFontFace.h	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/css/CSSFontFace.cpp	View	1 2 3 4 5	2 chunks	+6 lines, -3 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/CSSProperties.in	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/CSSSegmentedFontFace.h	View	1 2 3	1 chunk	+3 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/CSSSegmentedFontFace.cpp	View		1 chunk	+1 line, -6 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/CSSValueKeywords.in	View	1 2 3	1 chunk	+9 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/FontFace.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/FontFace.cpp	View		5 chunks	+27 lines, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/css/RemoteFontFaceSource.h	View	1 2 3 4 5	2 chunks	+19 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp	View	1 2 3 4 5	3 chunks	+34 lines, -6 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/fetch/FontResource.h	View		3 chunks	+6 lines, -5 lines	0 comments	Download
M	third_party/WebKit/Source/core/fetch/FontResource.cpp	View	1	5 chunks	+21 lines, -8 lines	0 comments	Download
M	third_party/WebKit/Source/core/frame/UseCounter.cpp	View	1 2 3 4	2 chunks	+2 lines, -1 line	0 comments	Download
M	third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 28 (10 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Yoav Weiss

Thanks for working on this! :) Since it's a fairly large CL, it'd take me ...

5 years, 1 month ago (2015-10-30 17:22:31 UTC) #5

kinuko

Will take one more look but looking good overall. Would be nice to have a ...

5 years, 1 month ago (2015-11-01 16:32:05 UTC) #6

eae

Implementation LGTM, as suggested would love to see a test for the transition. https://codereview.chromium.org/1429713004/diff/1/third_party/WebKit/LayoutTests/http/tests/webfont/font-display.html File ...

5 years, 1 month ago (2015-11-02 00:56:22 UTC) #7

Kunihiko Sakamoto

https://codereview.chromium.org/1429713004/diff/1/third_party/WebKit/LayoutTests/http/tests/webfont/font-display.html File third_party/WebKit/LayoutTests/http/tests/webfont/font-display.html (right): https://codereview.chromium.org/1429713004/diff/1/third_party/WebKit/LayoutTests/http/tests/webfont/font-display.html#newcode1 third_party/WebKit/LayoutTests/http/tests/webfont/font-display.html:1: <!DOCTYPE html> On 2015/10/30 17:22:30, Yoav Weiss wrote: > ...

5 years, 1 month ago (2015-11-02 05:30:16 UTC) #8

https://codereview.chromium.org/1429713004/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/http/tests/webfont/font-display.html
(right):

https://codereview.chromium.org/1429713004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/webfont/font-display.html:1: <!DOCTYPE
html>
On 2015/10/30 17:22:30, Yoav Weiss wrote:
> It would have been great if this test could have been testharness based (so
> exportable to other browsers and web-platform-tests).
> Also - as far as I understand, you're only testing the final state here, and
not
> the transition times.
> 
> Could you observe the font changes from JS snd test it that way?(e.g. using
the
> font load API or something like
> https://github.com/bramstein/fontfaceobserver/blob/master/src/observer.js)
> 
> On that note, maybe we need a first class "font loaded and applied" event
> (AFAIU, the font loading API's promises require you to load the font through
the
> API as well).

Ah sorry, I didn't write a high level description for this test. This isn't
testing only the final state.

The meaning of test config is,

 delay: The time it takes to load the font
 time: Elapsed time since load start

So, the row for {time: 1000, delay: 3000} tests how text with a font that takes
3 sec to load look like after 1 sec from the load start. (This is done by
starting the load at T=4s, and taking snapshot for the reftest at T=5s.)

Can we write a reftest in cross-browser way? I wanted this to be a test for
actual rendering results, testharness isn't a great fit for this purpose.

https://codereview.chromium.org/1429713004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/webfont/font-display.html:40: for (var
config of configs) {
On 2015/11/02 00:56:22, eae wrote:
> Don't use for in loops for arrays (as per js style guide).
> 
> for (var config, i = 0; config = configs[i]; i++) is better

Done.

https://codereview.chromium.org/1429713004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/FontResource.cpp (right):

https://codereview.chromium.org/1429713004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/FontResource.cpp:42: static const double
fontLoadWaitLongLimitSec = 3.0;
On 2015/11/01 16:32:04, kinuko wrote:
> I assume this corresponds to "'100ms' or less is recommended" and "'3s' is
> recommended in most cases" respectively- maybe have a short description from a
> spec text and link to the spec in a comment?

Done.

Yoav Weiss

Filed a bug on the spec: https://github.com/tabatkins/specs/issues/46 I don't think it impacts the implementation, but ...

5 years, 1 month ago (2015-11-02 08:57:29 UTC) #9

Kunihiko Sakamoto

https://codereview.chromium.org/1429713004/diff/20001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/1429713004/diff/20001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp#newcode54 third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:54: return m_period == FailurePeriod || m_font->isLoaded(); On 2015/11/02 08:57:28, ...

5 years, 1 month ago (2015-11-04 02:32:51 UTC) #10

kinuko

lgtm, hope spec things become clearer for the issues filed. https://codereview.chromium.org/1429713004/diff/20001/third_party/WebKit/Source/core/css/CSSFontFace.h File third_party/WebKit/Source/core/css/CSSFontFace.h (right): https://codereview.chromium.org/1429713004/diff/20001/third_party/WebKit/Source/core/css/CSSFontFace.h#newcode73 ...

5 years, 1 month ago (2015-11-05 05:17:36 UTC) #11

Kunihiko Sakamoto

https://codereview.chromium.org/1429713004/diff/20001/third_party/WebKit/Source/core/css/CSSFontFace.h File third_party/WebKit/Source/core/css/CSSFontFace.h (right): https://codereview.chromium.org/1429713004/diff/20001/third_party/WebKit/Source/core/css/CSSFontFace.h#newcode73 third_party/WebKit/Source/core/css/CSSFontFace.h:73: void fontDataInvalidated(RemoteFontFaceSource*); On 2015/11/05 05:17:36, kinuko wrote: > This ...

5 years, 1 month ago (2015-11-05 08:41:27 UTC) #13

Kunihiko Sakamoto

yoav@, PTAL? https://codereview.chromium.org/1429713004/diff/20001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/1429713004/diff/20001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp#newcode54 third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:54: return m_period == FailurePeriod || m_font->isLoaded(); Changed ...

5 years, 1 month ago (2015-11-17 05:47:43 UTC) #16

Kunihiko Sakamoto

Hi Yoav, Have you had a chance to look at this? The spec bugs are ...

5 years ago (2015-11-24 05:36:21 UTC) #17

Yoav Weiss

On 2015/11/24 05:36:21, Kunihiko Sakamoto wrote: > Hi Yoav, > Have you had a chance ...

5 years ago (2015-11-24 07:55:25 UTC) #18

Yoav Weiss

On 2015/11/24 05:36:21, Kunihiko Sakamoto wrote: > Hi Yoav, > Have you had a chance ...

5 years ago (2015-11-24 07:55:25 UTC) #19

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429713004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429713004/160001

5 years ago (2015-11-24 08:05:25 UTC) #22

commit-bot: I haz the power

Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_rel_ng/builds/100504)

5 years ago (2015-11-24 10:46:08 UTC) #24

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429713004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429713004/160001

5 years ago (2015-11-24 10:48:40 UTC) #26

commit-bot: I haz the power

5 years ago (2015-11-24 12:13:15 UTC) #28

Message was sent while issue was closed.

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

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages