Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(23)

Issue 2453813004: WebFonts cache-aware timeout adaptation (Closed)

Created:
4 years, 1 month ago by Shao-Chuan Lee
Modified:
4 years, 1 month ago
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, chromium-reviews, dglazkov+blink, gavinp+loader_chromium.org, Nate Chapin, loading-reviews+fetch_chromium.org, rwlbuis, tyoshino+watch_chromium.org, Takashi Toyoshima
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

WebFonts cache-aware timeout adaptation This CL introduces adaptive webfont display behavior, reducing unnecessary Flash of Unstyled Text (FOUT) if webfont is already available in disk cache. This is available as a experimental Web Platform feature. In webfont display, fallback font will be used if webfont loading time exceeds certain timeout values in slow network. However it's observed that disk cache RTT may also hit the timeout regardless of network speed. In such cases we would like to block display until disk cache misses and actual network request is being sent. BUG=570205 Committed: https://crrev.com/dc8303234f47ccfc841185d33c45e4852776b65e Cr-Commit-Position: refs/heads/master@{#432425}

Patch Set 1 #

Patch Set 2 : rebase, handle FontResource::didAddClient() #

Patch Set 3 : remove callback called flag, DCHECKs #

Patch Set 4 : simplify by not calling cache miss callback in didAddClient() #

Total comments: 6

Patch Set 5 : rebase #

Patch Set 6 : determine |m_period| for FontDisplaySwap in beginLoadIfNeeded() #

Total comments: 1

Patch Set 7 : move commment #

Total comments: 5

Patch Set 8 : rebase, disable cache-aware behavior for font-display: swap, histogram #

Total comments: 4

Patch Set 9 : prohibit add/removeClient in all callbacks, histograms.xml #

Patch Set 10 : unit test, rebase, grammar #

Total comments: 8

Patch Set 11 : comment #20 #

Patch Set 12 : rebase #

Total comments: 4

Patch Set 13 : test callbacks in didAddClient(), rebase #

Total comments: 2

Patch Set 14 : rebase, FRIEND_TEST_ALL_PREFIXES #

Patch Set 15 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+204 lines, -8 lines) Patch
M third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp View 2 chunks +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/fetch/FontResource.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 5 chunks +13 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/fetch/FontResource.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 4 chunks +48 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/core/fetch/MockResourceClients.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +37 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/fetch/MockResourceClients.cpp View 1 2 3 4 5 6 7 8 9 10 1 chunk +29 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +57 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +16 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 45 (19 generated)
Shao-Chuan Lee
PTAL Changes for layout test is large, currently in another CL https://codereview.chromium.org/2438033003/
4 years, 1 month ago (2016-10-31 06:00:13 UTC) #2
Kunihiko Sakamoto
I wonder if we could add UMA to measure the impact of this change, e.g. ...
4 years, 1 month ago (2016-10-31 08:22:46 UTC) #3
yhirano
https://codereview.chromium.org/2453813004/diff/60001/third_party/WebKit/Source/core/fetch/FontResource.h File third_party/WebKit/Source/core/fetch/FontResource.h (right): https://codereview.chromium.org/2453813004/diff/60001/third_party/WebKit/Source/core/fetch/FontResource.h#newcode70 third_party/WebKit/Source/core/fetch/FontResource.h:70: // Calls to addClient() and removeClient() is forbidden in ...
4 years, 1 month ago (2016-10-31 12:44:32 UTC) #4
Shao-Chuan Lee
https://codereview.chromium.org/2453813004/diff/60001/third_party/WebKit/Source/core/fetch/FontResource.h File third_party/WebKit/Source/core/fetch/FontResource.h (right): https://codereview.chromium.org/2453813004/diff/60001/third_party/WebKit/Source/core/fetch/FontResource.h#newcode70 third_party/WebKit/Source/core/fetch/FontResource.h:70: // Calls to addClient() and removeClient() is forbidden in ...
4 years, 1 month ago (2016-11-01 08:57:07 UTC) #5
Kunihiko Sakamoto
https://codereview.chromium.org/2453813004/diff/60001/third_party/WebKit/Source/core/fetch/FontResource.h File third_party/WebKit/Source/core/fetch/FontResource.h (right): https://codereview.chromium.org/2453813004/diff/60001/third_party/WebKit/Source/core/fetch/FontResource.h#newcode119 third_party/WebKit/Source/core/fetch/FontResource.h:119: virtual void fontLoadShortLimitExceeded(FontResource*) {} On 2016/10/31 08:22:46, Kunihiko Sakamoto ...
4 years, 1 month ago (2016-11-02 03:46:36 UTC) #6
Shao-Chuan Lee
https://codereview.chromium.org/2453813004/diff/120001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2453813004/diff/120001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp#newcode260 third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:260: m_period = SwapPeriod; On 2016/11/02 03:46:36, Kunihiko Sakamoto wrote: ...
4 years, 1 month ago (2016-11-02 04:22:41 UTC) #7
Kunihiko Sakamoto
https://codereview.chromium.org/2453813004/diff/120001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2453813004/diff/120001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp#newcode260 third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:260: m_period = SwapPeriod; On 2016/11/02 04:22:40, Shao-Chuan Lee wrote: ...
4 years, 1 month ago (2016-11-02 05:49:16 UTC) #8
Shao-Chuan Lee
https://codereview.chromium.org/2453813004/diff/120001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2453813004/diff/120001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp#newcode260 third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:260: m_period = SwapPeriod; On 2016/11/02 05:49:16, Kunihiko Sakamoto wrote: ...
4 years, 1 month ago (2016-11-02 06:24:10 UTC) #9
Kunihiko Sakamoto
https://codereview.chromium.org/2453813004/diff/120001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2453813004/diff/120001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp#newcode260 third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:260: m_period = SwapPeriod; Let me check if my understanding ...
4 years, 1 month ago (2016-11-02 07:46:42 UTC) #10
Shao-Chuan Lee
To support cache-aware behavior in font-display: swap, we have to deal with more state transitions ...
4 years, 1 month ago (2016-11-07 06:52:47 UTC) #11
Kunihiko Sakamoto
+1 to start without font-display:swap. Can you reflect the new behavior in your layout test ...
4 years, 1 month ago (2016-11-07 07:56:24 UTC) #12
Shao-Chuan Lee
Layout test modified, looks fine. Note that the layout test is not for commit since ...
4 years, 1 month ago (2016-11-07 08:43:44 UTC) #13
Shao-Chuan Lee
Ping for comments on unit test, thanks.
4 years, 1 month ago (2016-11-09 07:08:12 UTC) #19
yhirano
https://codereview.chromium.org/2453813004/diff/180001/third_party/WebKit/Source/core/fetch/MockResourceClients.h File third_party/WebKit/Source/core/fetch/MockResourceClients.h (right): https://codereview.chromium.org/2453813004/diff/180001/third_party/WebKit/Source/core/fetch/MockResourceClients.h#newcode123 third_party/WebKit/Source/core/fetch/MockResourceClients.h:123: void removeAsClient(); The only caller is dispose(). Can you ...
4 years, 1 month ago (2016-11-09 07:29:53 UTC) #20
Shao-Chuan Lee
https://codereview.chromium.org/2453813004/diff/180001/third_party/WebKit/Source/core/fetch/MockResourceClients.h File third_party/WebKit/Source/core/fetch/MockResourceClients.h (right): https://codereview.chromium.org/2453813004/diff/180001/third_party/WebKit/Source/core/fetch/MockResourceClients.h#newcode123 third_party/WebKit/Source/core/fetch/MockResourceClients.h:123: void removeAsClient(); On 2016/11/09 07:29:52, yhirano wrote: > The ...
4 years, 1 month ago (2016-11-09 08:04:32 UTC) #21
Kunihiko Sakamoto
lgtm
4 years, 1 month ago (2016-11-10 03:38:51 UTC) #22
yhirano
https://codereview.chromium.org/2453813004/diff/190009/third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp File third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp (right): https://codereview.chromium.org/2453813004/diff/190009/third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp#newcode770 third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:770: Can you add one more client to the resource ...
4 years, 1 month ago (2016-11-10 04:17:54 UTC) #23
Shao-Chuan Lee
https://codereview.chromium.org/2453813004/diff/190009/third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp File third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp (right): https://codereview.chromium.org/2453813004/diff/190009/third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp#newcode770 third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:770: On 2016/11/10 04:17:54, yhirano wrote: > Can you add ...
4 years, 1 month ago (2016-11-14 08:08:02 UTC) #24
yhirano
lgtm
4 years, 1 month ago (2016-11-15 03:15:46 UTC) #25
Shao-Chuan Lee
tkent@: please review changes under core/css and platform isherman@: please review histograms Thanks.
4 years, 1 month ago (2016-11-15 05:43:42 UTC) #27
tkent
lgtm https://codereview.chromium.org/2453813004/diff/230001/third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp File third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp (right): https://codereview.chromium.org/2453813004/diff/230001/third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp#newcode720 third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:720: // Helper for accessing private members of FontResource. ...
4 years, 1 month ago (2016-11-15 05:59:23 UTC) #28
Shao-Chuan Lee
cc toyoshim@ for upcoming ResourceFetcherTest migration in another CL https://codereview.chromium.org/2453813004/diff/230001/third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp File third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp (right): https://codereview.chromium.org/2453813004/diff/230001/third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp#newcode720 third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:720: ...
4 years, 1 month ago (2016-11-15 06:47:45 UTC) #29
Ilya Sherman
histograms lgtm
4 years, 1 month ago (2016-11-16 06:31:41 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2453813004/270001
4 years, 1 month ago (2016-11-16 08:09:15 UTC) #41
commit-bot: I haz the power
Committed patchset #15 (id:270001)
4 years, 1 month ago (2016-11-16 08:15:32 UTC) #43
commit-bot: I haz the power
4 years, 1 month ago (2016-11-16 08:18:42 UTC) #45
Message was sent while issue was closed.
Patchset 15 (id:??) landed as
https://crrev.com/dc8303234f47ccfc841185d33c45e4852776b65e
Cr-Commit-Position: refs/heads/master@{#432425}

Powered by Google App Engine
This is Rietveld 408576698