|
|
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. |
DescriptionSupport 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 #Messages
Total messages: 58 (40 generated)
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
bungeman@google.com changed reviewers: + bungeman@google.com
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 this one can cherry pick in hooks as shown below. https://codereview.chromium.org/2780133002/diff/80001/DEPS#newcode1143 DEPS:1143: }, Go to https://pdfium-review.googlesource.com/c/2971 and click 'Download' and then this is the 'Cherry Pick' option with the addition of '-C src/third_party/pdfium'. Seems to work locally. { 'name': 'fetch_custom_patch', 'pattern': '.', 'action': [ 'git', '-C', 'src/third_party/pdfium/', 'fetch', 'https://pdfium.googlesource.com/pdfium', 'refs/changes/71/2971/3', ], }, { 'name': 'apply_custom_patch', 'pattern': '.', 'action': ['git', '-C', 'src/third_party/pdfium/', 'cherry-pick', 'FETCH_HEAD', ], },
I went and tried that out at https://codereview.chromium.org/2786433004/ , seems to work.
bungeman@chromium.org changed reviewers: + bungeman@chromium.org
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 FreeType commit 7aeee3c50 'Introduce FT_UINT_TO_POINTER macro'. We might need to roll FreeType past that first.
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
drott@chromium.org changed reviewers: + kjellander@chromium.org, machenbach@chromium.org - bungeman@google.com
On 2017/03/29 at 21:23:51, bungeman wrote: > https://savannah.nongnu.org/bugs/index.php?50560 > which was fixed in recent FreeType commit 7aeee3c50 'Introduce FT_UINT_TO_POINTER macro'. We might need to roll FreeType past that first. Thanks, added the warning in https://codereview.chromium.org/2781313002.
kjellander@, machenbach@, can you please review/RS the change in build_overrides/pdfium.gni? This is for enabling linking PDFium against Chromium's own build of FreeType on Windows, which is now identical in version to PDFium's. More details on the bug https://bugs.chromium.org/p/chromium/issues/detail?id=700926 - Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2780133002/diff/100001/build_overrides/pdfium... File build_overrides/pdfium.gni (right): https://codereview.chromium.org/2780133002/diff/100001/build_overrides/pdfium... build_overrides/pdfium.gni:11: # Bundle FreeType on all platforms expect Linux. Use system FreeType on Linux. Please update this comment as well.
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2780133002/diff/100001/build_overrides/pdfium... File build_overrides/pdfium.gni (right): https://codereview.chromium.org/2780133002/diff/100001/build_overrides/pdfium... build_overrides/pdfium.gni:11: # Bundle FreeType on all platforms expect Linux. Use system FreeType on Linux. On 2017/03/30 at 15:34:47, kjellander_chromium wrote: > Please update this comment as well. Done, thanks for taking a look.
lgtm for build_overrides/pdfium.gni - I don't really know much about the other stuff so please refer to other reviewers for that.
drott@chromium.org changed reviewers: + eae@chromium.org
bungeman@google.com changed reviewers: + bungeman@google.com
skia/BUILD.gn lgtm
third_party/freetype lgtm
Also, the change to fast/text/chromium-linux-fontconfig-renderstyle.html lgtm too.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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 ========== to ========== 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
LGTM Yay!
The CQ bit was checked by eae@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bungeman@google.com, kjellander@chromium.org, bungeman@chromium.org Link to the patchset: https://codereview.chromium.org/2780133002/#ps140001 (title: "Add Linux rebaseline")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by drott@chromium.org
The CQ bit was checked by drott@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490900047713120, "parent_rev": "0e417c028e62d771638997c4545a359ba93027ec", "commit_rev": "5265e65e5ea1ae3987c27f76914c9dd9a41506cd"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/5265e65e5ea1ae3987c27f76914c... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/5265e65e5ea1ae3987c27f76914c...
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2786263002/ by ortuno@chromium.org. The reason for reverting is: Compile failure on linux bot: https://uberchromegw.corp.google.com/i/chromium.linux/builders/Linux%20Builde... [6410/39636] CXX obj/third_party/pdfium/formfiller/cffl_interactiveformfiller.o FAILED: obj/third_party/pdfium/formfiller/cffl_interactiveformfiller.o /b/c/goma_client/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/third_party/pdfium/formfiller/cffl_interactiveformfiller.o.d -DV8_DEPRECATION_WARNINGS -DUSE_UDEV -DUSE_AURA=1 -DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -DUSE_X11=1 -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD -DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED -DCR_CLANG_REVISION=\"298539-1\" -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DCOMPONENT_BUILD -D_DEBUG -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -DV8_DEPRECATION_WARNINGS -D_FX_CPU_=_FX_X86_ -DOPJ_STATIC -DPNG_PREFIX -DPNG_USE_READ_MACROS -DPDF_ENABLE_V8 -DPDFIUM_PRINT_TEXT_WITH_GDI -I../.. -Igen -I../../third_party/pdfium -I../../build/linux/debian_jessie_i386-sysroot/usr/include/freetype2 -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -funwind-tables -fPIC -pipe -B../../third_party/binutils/Linux_x64/Release/bin -fcolor-diagnostics -m32 -msse2 -mfpmath=sse -mmmx -momit-leaf-frame-pointer -pthread -mstack-alignment=16 -mstackrealign -Wall -Werror -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -Wno-shift-negative-value -Wno-undefined-var-template -Wno-nonportable-include-path -Wno-address-of-packed-member -Wno-unused-lambda-capture -Wno-user-defined-warnings -O0 -fno-omit-frame-pointer -g2 -gsplit-dwarf --sysroot=../../build/linux/debian_jessie_i386-sysroot -fvisibility=hidden -Xclang -load -Xclang ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang check-auto-raw-pointer -Xclang -plugin-arg-find-bad-constructs -Xclang check-ipc -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -fvisibility-inlines-hidden -Wno-undefined-bool-conversion -Wno-tautological-undefined-compare -std=gnu++11 -fno-rtti -fno-exceptions -c ../../third_party/pdfium/fpdfsdk/formfiller/cffl_interactiveformfiller.cpp -o obj/third_party/pdfium/formfiller/cffl_interactiveformfiller.o In file included from ../../third_party/pdfium/fpdfsdk/formfiller/cffl_interactiveformfiller.cpp:12: In file included from ../../third_party/pdfium/core/fxge/cfx_pathdata.h:14: In file included from ../../third_party/pdfium/core/fxge/cfx_renderdevice.h:12: In file included from ../../third_party/pdfium/core/fxge/cfx_gemodule.h:12: In file included from ../../third_party/pdfium/core/fxge/cfx_fontmgr.h:13: In file included from ../../third_party/pdfium/core/fxge/fx_font.h:17: In file included from ../../third_party/pdfium/core/fxge/fx_freetype.h:11: In file included from ../../build/linux/debian_jessie_i386-sysroot/usr/include/freetype2/freetype.h:33: ../../build/linux/debian_jessie_i386-sysroot/usr/include/freetype2/config/ftconfig.h:453:5: error: 'register' storage class specifier is deprecated and incompatible with C++1z [-Werror,-Wdeprecated-register] register FT_Int32 result; ^~~~~~~~~ 1 error generated..
Message was sent while issue was closed.
On 2017/03/30 22:36:36, ortuno wrote: > A revert of this CL (patchset #8 id:140001) has been created in > https://codereview.chromium.org/2786263002/ by mailto:ortuno@chromium.org. > > The reason for reverting is: Compile failure on linux bot: > https://uberchromegw.corp.google.com/i/chromium.linux/builders/Linux%20Builde... > > [6410/39636] CXX obj/third_party/pdfium/formfiller/cffl_interactiveformfiller.o > FAILED: obj/third_party/pdfium/formfiller/cffl_interactiveformfiller.o > /b/c/goma_client/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++ > -MMD -MF obj/third_party/pdfium/formfiller/cffl_interactiveformfiller.o.d > -DV8_DEPRECATION_WARNINGS -DUSE_UDEV -DUSE_AURA=1 -DUSE_PANGO=1 -DUSE_CAIRO=1 > -DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -DUSE_X11=1 -DFULL_SAFE_BROWSING > -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD > -DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED > -DCR_CLANG_REVISION=\"298539-1\" -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE > -D_LARGEFILE64_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS > -DCOMPONENT_BUILD -D_DEBUG -DDYNAMIC_ANNOTATIONS_ENABLED=1 > -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -DV8_DEPRECATION_WARNINGS -D_FX_CPU_=_FX_X86_ > -DOPJ_STATIC -DPNG_PREFIX -DPNG_USE_READ_MACROS -DPDF_ENABLE_V8 > -DPDFIUM_PRINT_TEXT_WITH_GDI -I../.. -Igen -I../../third_party/pdfium > -I../../build/linux/debian_jessie_i386-sysroot/usr/include/freetype2 > -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector > -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= > -funwind-tables -fPIC -pipe -B../../third_party/binutils/Linux_x64/Release/bin > -fcolor-diagnostics -m32 -msse2 -mfpmath=sse -mmmx -momit-leaf-frame-pointer > -pthread -mstack-alignment=16 -mstackrealign -Wall -Werror -Wextra > -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing > -Wno-covered-switch-default -Wno-unneeded-internal-declaration > -Wno-inconsistent-missing-override -Wno-shift-negative-value > -Wno-undefined-var-template -Wno-nonportable-include-path > -Wno-address-of-packed-member -Wno-unused-lambda-capture > -Wno-user-defined-warnings -O0 -fno-omit-frame-pointer -g2 -gsplit-dwarf > --sysroot=../../build/linux/debian_jessie_i386-sysroot -fvisibility=hidden > -Xclang -load -Xclang > ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang > -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs > -Xclang check-auto-raw-pointer -Xclang -plugin-arg-find-bad-constructs -Xclang > check-ipc -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare > -fvisibility-inlines-hidden -Wno-undefined-bool-conversion > -Wno-tautological-undefined-compare -std=gnu++11 -fno-rtti -fno-exceptions -c > ../../third_party/pdfium/fpdfsdk/formfiller/cffl_interactiveformfiller.cpp -o > obj/third_party/pdfium/formfiller/cffl_interactiveformfiller.o > In file included from > ../../third_party/pdfium/fpdfsdk/formfiller/cffl_interactiveformfiller.cpp:12: > In file included from ../../third_party/pdfium/core/fxge/cfx_pathdata.h:14: > In file included from ../../third_party/pdfium/core/fxge/cfx_renderdevice.h:12: > In file included from ../../third_party/pdfium/core/fxge/cfx_gemodule.h:12: > In file included from ../../third_party/pdfium/core/fxge/cfx_fontmgr.h:13: > In file included from ../../third_party/pdfium/core/fxge/fx_font.h:17: > In file included from ../../third_party/pdfium/core/fxge/fx_freetype.h:11: > In file included from > ../../build/linux/debian_jessie_i386-sysroot/usr/include/freetype2/freetype.h:33: > ../../build/linux/debian_jessie_i386-sysroot/usr/include/freetype2/config/ftconfig.h:453:5: > error: 'register' storage class specifier is deprecated and incompatible with > C++1z [-Werror,-Wdeprecated-register] > register FT_Int32 result; > ^~~~~~~~~ > 1 error generated.. 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).
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 |