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

Issue 23446007: Use unicode-range to prevent unnecessary @font-face donwnloads (Closed)

Created:
7 years, 3 months ago by Kunihiko Sakamoto
Modified:
7 years, 3 months ago
Reviewers:
dglazkov, eseidel
CC:
blink-reviews, jamesr, eae+blinkwatch, danakj, dglazkov+blink, Rik, apavlov+blink_chromium.org, darktears, Stephen Chennney, jeez, pdr.
Visibility:
Public.

Description

Use unicode-range to prevent unnecessary @font-face donwnloads This patch delays font loading until creation of GlyphPage whose codepoint range intersects with the @font-face's unicode-range value. GlyphPage is created when glyph data for a character in its codepoint range is requested, so web fonts are not donwloaded if its glyph data is not used. Even if any glyph is not used, font metrics may be used (for example, <input> uses font metrics to calculate its size). These metrics are accessed via SimpleFontData::primarySimpleFontData, so we start loading there too. Caveat: Since GlyphPage has glyph data for contiguous 256 Unicode codepoints, this patch does not 100% prevent unnecessary downloads. Use of codepoint *near* the unicode-range can trigger the font load. To fix this, we need to delay font loading until a codepoint in unicode-range is requested. That may require changing the GlyphPage data structure. BUG=247920 TEST=fast/css/font-face-unicode-range-load.html Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157893

Patch Set 1 #

Total comments: 8

Patch Set 2 : #

Patch Set 3 : rebase #

Total comments: 2

Patch Set 4 : rebase and add toSegmentedFontData() #

Unified diffs Side-by-side diffs Delta from patch set Stats (+200 lines, -30 lines) Patch
A LayoutTests/fast/css/font-face-unicode-range-load.html View 1 2 3 1 chunk +67 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/font-face-unicode-range-load-expected.txt View 1 chunk +12 lines, -0 lines 0 comments Download
M Source/core/css/CSSFontFace.h View 1 2 3 2 chunks +2 lines, -0 lines 0 comments Download
M Source/core/css/CSSFontFace.cpp View 1 2 3 2 chunks +16 lines, -3 lines 0 comments Download
M Source/core/css/CSSFontFaceSource.h View 1 2 3 chunks +5 lines, -1 line 0 comments Download
M Source/core/css/CSSFontFaceSource.cpp View 1 2 4 chunks +19 lines, -4 lines 0 comments Download
M Source/core/css/CSSSegmentedFontFace.cpp View 1 2 3 3 chunks +6 lines, -8 lines 0 comments Download
M Source/core/platform/graphics/FontFallbackList.h View 2 chunks +5 lines, -2 lines 0 comments Download
M Source/core/platform/graphics/FontFallbackList.cpp View 1 chunk +15 lines, -0 lines 0 comments Download
M Source/core/platform/graphics/GlyphPageTreeNode.cpp View 1 chunk +8 lines, -0 lines 0 comments Download
M Source/core/platform/graphics/SegmentedFontData.h View 1 2 3 1 chunk +6 lines, -0 lines 0 comments Download
M Source/core/platform/graphics/SimpleFontData.h View 1 2 6 chunks +27 lines, -7 lines 0 comments Download
M Source/core/platform/graphics/SimpleFontData.cpp View 1 4 chunks +12 lines, -5 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
Kunihiko Sakamoto
Hi Dimitri, Could you review this patch? I'm asking you for review because you reviewed ...
7 years, 3 months ago (2013-08-28 09:58:35 UTC) #1
Kunihiko Sakamoto
ping?
7 years, 3 months ago (2013-09-04 11:15:23 UTC) #2
eseidel
lgtm I'm sorry you had to wait so long for a review. I think this ...
7 years, 3 months ago (2013-09-10 15:07:26 UTC) #3
dglazkov
https://codereview.chromium.org/23446007/diff/1/Source/core/css/CSSFontFaceSource.h File Source/core/css/CSSFontFaceSource.h (right): https://codereview.chromium.org/23446007/diff/1/Source/core/css/CSSFontFaceSource.h#newcode110 Source/core/css/CSSFontFaceSource.h:110: FontDataTable m_fontDataTable; // The hash key is composed of ...
7 years, 3 months ago (2013-09-10 15:26:41 UTC) #4
Kunihiko Sakamoto
Thank you for the reviews, Eric and Dimitri! Please take another look. https://codereview.chromium.org/23446007/diff/1/Source/core/css/CSSFontFaceSource.h File Source/core/css/CSSFontFaceSource.h ...
7 years, 3 months ago (2013-09-11 13:28:09 UTC) #5
Kunihiko Sakamoto
Dimitri, is this change OK with you? Sorry to keep pinging.
7 years, 3 months ago (2013-09-13 00:36:15 UTC) #6
eseidel
This still lgtm. I suspect we can track down dglazkov for his lgtm today. https://codereview.chromium.org/23446007/diff/18001/Source/core/css/CSSSegmentedFontFace.cpp ...
7 years, 3 months ago (2013-09-16 17:20:16 UTC) #7
dglazkov
lgtm++
7 years, 3 months ago (2013-09-16 17:23:30 UTC) #8
Kunihiko Sakamoto
Thank you for the review! https://codereview.chromium.org/23446007/diff/18001/Source/core/css/CSSSegmentedFontFace.cpp File Source/core/css/CSSSegmentedFontFace.cpp (right): https://codereview.chromium.org/23446007/diff/18001/Source/core/css/CSSSegmentedFontFace.cpp#newcode184 Source/core/css/CSSSegmentedFontFace.cpp:184: SegmentedFontData* segmentedFontData = static_cast<SegmentedFontData*>(fontData.get()); ...
7 years, 3 months ago (2013-09-17 06:16:30 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ksakamoto@chromium.org/23446007/30001
7 years, 3 months ago (2013-09-17 06:16:56 UTC) #10
commit-bot: I haz the power
Retried try job too often on win_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_rel&number=5914
7 years, 3 months ago (2013-09-17 08:21:15 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ksakamoto@chromium.org/23446007/30001
7 years, 3 months ago (2013-09-17 08:27:06 UTC) #12
commit-bot: I haz the power
7 years, 3 months ago (2013-09-17 10:44:33 UTC) #13
Message was sent while issue was closed.
Change committed as 157893

Powered by Google App Engine
This is Rietveld 408576698