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

Issue 2780133002: Support for OpenType Font Variations on Windows (Closed)

Created:
3 years, 8 months ago by drott
Modified:
3 years, 8 months ago
CC:
ajuma+watch_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, fmalita+watch_chromium.org, jbroman, Justin Novosad, kinuko+watch, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Support for OpenType Font Variations on Windows Enable support for variable fonts on Windows through using SkFontMgr_Custom_Empty, a FreeType backed font-blob only SkTypeface factory (no access to system fonts). We will use a hybrid DirectWrite and FreeType font stack on Windows for at least as long as most of Windows versions we support with Chrome still do not have native support for font variations. Thanks to Ben Wagner for the help with enabling and prototyping this. BUG=700926 Review-Url: https://codereview.chromium.org/2780133002 Cr-Commit-Position: refs/heads/master@{#460895} Committed: https://chromium.googlesource.com/chromium/src/+/5265e65e5ea1ae3987c27f76914c9dd9a41506cd

Patch Set 1 #

Patch Set 2 : Combine with PDFium shared linkage candidate CL #

Patch Set 3 : Tidy up BUILD.gn, Custom_Empty only on Windows #

Patch Set 4 : Rebased #

Patch Set 5 : Rebased PDFium change #

Total comments: 2

Patch Set 6 : Test case modified to ensure going through variations path, rebased on PDFium roll #

Total comments: 2

Patch Set 7 : Harmonize FreeType configuration, update pdfium.gni comment #

Patch Set 8 : Add Linux rebaseline #

Unified diffs Side-by-side diffs Delta from patch set Stats (+58 lines, -23 lines) Patch
M DEPS View 1 2 3 4 5 6 7 1 chunk +3 lines, -0 lines 0 comments Download
M build_overrides/pdfium.gni View 1 2 3 4 5 6 1 chunk +3 lines, -2 lines 0 comments Download
M skia/BUILD.gn View 1 2 5 chunks +24 lines, -7 lines 0 comments Download
M third_party/WebKit/LayoutTests/NeverFixTests View 1 chunk +3 lines, -5 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/text/variable-fonts/variable-box-font-expected.html View 1 2 3 4 5 1 chunk +7 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/platform/linux/fast/text/chromium-linux-fontconfig-renderstyle-expected.png View 1 2 3 4 5 6 7 Binary file 0 comments Download
M third_party/WebKit/Source/platform/fonts/FontCustomPlatformData.cpp View 1 2 2 chunks +8 lines, -1 line 0 comments Download
M third_party/freetype/BUILD.gn View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/freetype/include/freetype-custom-config/ftmodule.h View 1 2 3 4 5 6 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/freetype/include/freetype-custom-config/ftoption.h View 1 2 3 4 5 6 4 chunks +4 lines, -4 lines 0 comments Download

Messages

Total messages: 58 (40 generated)
bungeman-skia
Because I tinker with things... https://codereview.chromium.org/2780133002/diff/80001/DEPS File DEPS (right): https://codereview.chromium.org/2780133002/diff/80001/DEPS#newcode68 DEPS:68: 'pdfium_revision': '186e7787629b07cd9253d43db92e10cbcf3ca1dc', Instead of ...
3 years, 8 months ago (2017-03-29 20:17:04 UTC) #18
bungeman-skia
I went and tried that out at https://codereview.chromium.org/2786433004/ , seems to work.
3 years, 8 months ago (2017-03-29 20:58:29 UTC) #19
bungeman-chromium
Oh, and be aware of this issue https://codereview.chromium.org/2682803002/diff/60001/thir _party/freetype/BUILD.gn https://savannah.nongnu.org/bugs/index.php?50560 which was fixed in recent ...
3 years, 8 months ago (2017-03-29 21:23:51 UTC) #21
drott
On 2017/03/29 at 21:23:51, bungeman wrote: > https://savannah.nongnu.org/bugs/index.php?50560 > which was fixed in recent FreeType ...
3 years, 8 months ago (2017-03-30 11:55:02 UTC) #25
drott
kjellander@, machenbach@, can you please review/RS the change in build_overrides/pdfium.gni? This is for enabling linking ...
3 years, 8 months ago (2017-03-30 11:56:22 UTC) #26
kjellander_chromium
https://codereview.chromium.org/2780133002/diff/100001/build_overrides/pdfium.gni File build_overrides/pdfium.gni (right): https://codereview.chromium.org/2780133002/diff/100001/build_overrides/pdfium.gni#newcode11 build_overrides/pdfium.gni:11: # Bundle FreeType on all platforms expect Linux. Use ...
3 years, 8 months ago (2017-03-30 15:34:47 UTC) #29
drott
https://codereview.chromium.org/2780133002/diff/100001/build_overrides/pdfium.gni File build_overrides/pdfium.gni (right): https://codereview.chromium.org/2780133002/diff/100001/build_overrides/pdfium.gni#newcode11 build_overrides/pdfium.gni:11: # Bundle FreeType on all platforms expect Linux. Use ...
3 years, 8 months ago (2017-03-30 15:52:30 UTC) #32
kjellander_chromium
lgtm for build_overrides/pdfium.gni - I don't really know much about the other stuff so please ...
3 years, 8 months ago (2017-03-30 16:06:49 UTC) #33
bungeman-skia
skia/BUILD.gn lgtm
3 years, 8 months ago (2017-03-30 17:27:22 UTC) #36
bungeman-chromium
third_party/freetype lgtm
3 years, 8 months ago (2017-03-30 17:29:43 UTC) #37
bungeman-chromium
Also, the change to fast/text/chromium-linux-fontconfig-renderstyle.html lgtm too.
3 years, 8 months ago (2017-03-30 17:31:28 UTC) #38
eae
LGTM Yay!
3 years, 8 months ago (2017-03-30 18:50:47 UTC) #46
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/2780133002/140001
3 years, 8 months ago (2017-03-30 18:52:08 UTC) #49
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/2780133002/140001
3 years, 8 months ago (2017-03-30 18:54:39 UTC) #52
commit-bot: I haz the power
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/5265e65e5ea1ae3987c27f76914c9dd9a41506cd
3 years, 8 months ago (2017-03-30 21:49:04 UTC) #55
ortuno
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2786263002/ by ortuno@chromium.org. ...
3 years, 8 months ago (2017-03-30 22:36:36 UTC) #56
bungeman-skia
On 2017/03/30 22:36:36, ortuno wrote: > A revert of this CL (patchset #8 id:140001) has ...
3 years, 8 months ago (2017-03-31 03:26:13 UTC) #57
drott
3 years, 8 months ago (2017-03-31 05:27:52 UTC) #58
Message was sent while issue was closed.
On 2017/03/31 at 03:26:13, bungeman wrote:

> While the actual issue turned out to be
https://codereview.chromium.org/2780623003/ (and the above revert reverted),
note that this means when pdfium is building in the new world it is including
system FreeType headers. I don't think we want that at all, since those headers
may differ from the ones used by Chromium's FreeType build (which is what it
will end up running against).

On second thought, on Linux this is currently intended. we haven't decided
whether we want to ship our own there. I CC'ed you on:
https://bugs.chromium.org/p/chromium/issues/detail?id=707084

Powered by Google App Engine
This is Rietveld 408576698