Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2871133004:
Build FreeType with HarfBuzz support (Closed)
DescriptionBuild FreeType with HarfBuzz support
FreeType's autohinter uses HarfBuzz API to collect additional GSUB and
GPOS mappings to detect ligatures that should be aligned by the
autohinter. Previously we were not able to build FreeType with HarfBuzz
support because of the cyclic dependency. This CL resolves the cyclic
dependency by building a bootstrap FreeType in order to build HarfBuzz'
hb_ft_* functions as a static library called harfbuzz-ng-ft. Then we
build harfbuzz-ng separately (which does not depend on FreeType), then
we build FreeType depending on harfbuzz-ng and harfbuzz-ng-ft.
This resolves issues with fi and ffi ligatures in Roboto looking like
they were shifted to a different baseline.
I tried developing a pixel test for this, which works if I force usage
of the FreeType autohinter through SkPaint::kSlight_Hinting, however we
are currently unable to automatically test this since our Linux layout
tests do not exercise the autohinting code and do not set this hinting
mode, probably due to the special fontconfig settings that we are using
for the layout tests. Manually verifying the Roboto ligatures however
confirms that this works.
BUG=617168
Review-Url: https://codereview.chromium.org/2871133004
Cr-Commit-Position: refs/heads/master@{#471277}
Committed: https://chromium.googlesource.com/chromium/src/+/a377e3e1fa4f44a58f70abb2402e6cc79e210945
Patch Set 1 #Patch Set 2 : Don't touch linkage of content_shell #Patch Set 3 : Try fixing the ChromeOS build #Patch Set 4 : Attempt to resolve duplicate symbols #Patch Set 5 : Attempt to resolve remaining CrOS build failures #Patch Set 6 : Try to fix chromeos_amd64_generic_chromium_compile_only_ng builder, which seems to use own freetype… #Patch Set 7 : Rebaseline expectation #
Messages
Total messages: 52 (37 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...
drott@chromium.org changed reviewers: + dpranke@chromium.org, mkwst@chromium.org, peter@chromium.org
PTAL, this the new approach replacing the previous attempt in https://codereview.chromium.org/2872333002 which wouldn't work on Windows. peter@, could you please review the content_shell BUILD.gn change?
//c/s/BUILD.gn change lgtm, although it seems unrelated to the act of actually enabling HarfBuzz since it only means that Content Shell now respects `use_system_freetype`?
On 2017/05/11 at 16:12:48, peter wrote: > //c/s/BUILD.gn change lgtm, although it seems unrelated to the act of actually enabling HarfBuzz since it only means that Content Shell now respects `use_system_freetype`? You're right, which is not the intention. I'll remove this part. Thanks for pointing this out.
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: + behdad@chromium.org
drott@chromium.org changed reviewers: - mkwst@chromium.org, peter@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Description was changed from ========== Compile FreeType with HarfBuzz support FreeType's autohinter uses HarfBuzz API to collect additional GSUB and GPOS mappings to detect ligatures that should be aligned by the autohinter. Previously we were not able to build FreeType with HarfBuzz support because of the cyclic dependency. This CL resolves the cyclic dependency by building a bootstrap FreeType in order to build HarfBuzz' hb_ft_* functions as a static library called harfbuzz-ng-ft. Then we build harfbuzz-ng separately (which does not depend on FreeType), then we build FreeType depending on harfbuzz-ng and harfbuzz-ng-ft. This resolves issues with fi and ffi ligatures in Roboto looking like they were shifted to a different baseline. I tried developing a pixel test for this, which works if I force usage of the FreeType autohinter through SkPaint::kSlight_Hinting, however we are currently unable to automatically test this since our Linux layout tests do not exercise the autohinting code and do not set this hinting mode, probably due to the special fontconfig settings that we are using for the layout tests. Manually verifying the Roboto ligatures however confirms that this works. BUG=617168 ========== to ========== Build FreeType with HarfBuzz support FreeType's autohinter uses HarfBuzz API to collect additional GSUB and GPOS mappings to detect ligatures that should be aligned by the autohinter. Previously we were not able to build FreeType with HarfBuzz support because of the cyclic dependency. This CL resolves the cyclic dependency by building a bootstrap FreeType in order to build HarfBuzz' hb_ft_* functions as a static library called harfbuzz-ng-ft. Then we build harfbuzz-ng separately (which does not depend on FreeType), then we build FreeType depending on harfbuzz-ng and harfbuzz-ng-ft. This resolves issues with fi and ffi ligatures in Roboto looking like they were shifted to a different baseline. I tried developing a pixel test for this, which works if I force usage of the FreeType autohinter through SkPaint::kSlight_Hinting, however we are currently unable to automatically test this since our Linux layout tests do not exercise the autohinting code and do not set this hinting mode, probably due to the special fontconfig settings that we are using for the layout tests. Manually verifying the Roboto ligatures however confirms that this works. BUG=617168 ==========
drott@chromium.org changed reviewers: + peter@chromium.org
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: linux_chromium_chromeos_ozone_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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
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...
lgtm
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_...)
The CQ bit was checked by drott@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2871133004/#ps110001 (title: "Rebaseline expectation")
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
Try jobs failed on following builders: linux_chromium_chromeos_ozone_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
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
Try jobs failed on following builders: linux_chromium_chromeos_ozone_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
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": 110001, "attempt_start_ts": 1494588415378510,
"parent_rev": "54a03d79877c3cacd79e37e33ec715abb3460bd7", "commit_rev":
"d732632acb1fe8e86107933cfddfe9407cf78155"}
CQ is committing da patch.
Bot data: {"patchset_id": 110001, "attempt_start_ts": 1494588415378510,
"parent_rev": "1e6a1b081607e6fcf90c5c85e46d54ed23d0cf6b", "commit_rev":
"a377e3e1fa4f44a58f70abb2402e6cc79e210945"}
Message was sent while issue was closed.
Description was changed from ========== Build FreeType with HarfBuzz support FreeType's autohinter uses HarfBuzz API to collect additional GSUB and GPOS mappings to detect ligatures that should be aligned by the autohinter. Previously we were not able to build FreeType with HarfBuzz support because of the cyclic dependency. This CL resolves the cyclic dependency by building a bootstrap FreeType in order to build HarfBuzz' hb_ft_* functions as a static library called harfbuzz-ng-ft. Then we build harfbuzz-ng separately (which does not depend on FreeType), then we build FreeType depending on harfbuzz-ng and harfbuzz-ng-ft. This resolves issues with fi and ffi ligatures in Roboto looking like they were shifted to a different baseline. I tried developing a pixel test for this, which works if I force usage of the FreeType autohinter through SkPaint::kSlight_Hinting, however we are currently unable to automatically test this since our Linux layout tests do not exercise the autohinting code and do not set this hinting mode, probably due to the special fontconfig settings that we are using for the layout tests. Manually verifying the Roboto ligatures however confirms that this works. BUG=617168 ========== to ========== Build FreeType with HarfBuzz support FreeType's autohinter uses HarfBuzz API to collect additional GSUB and GPOS mappings to detect ligatures that should be aligned by the autohinter. Previously we were not able to build FreeType with HarfBuzz support because of the cyclic dependency. This CL resolves the cyclic dependency by building a bootstrap FreeType in order to build HarfBuzz' hb_ft_* functions as a static library called harfbuzz-ng-ft. Then we build harfbuzz-ng separately (which does not depend on FreeType), then we build FreeType depending on harfbuzz-ng and harfbuzz-ng-ft. This resolves issues with fi and ffi ligatures in Roboto looking like they were shifted to a different baseline. I tried developing a pixel test for this, which works if I force usage of the FreeType autohinter through SkPaint::kSlight_Hinting, however we are currently unable to automatically test this since our Linux layout tests do not exercise the autohinting code and do not set this hinting mode, probably due to the special fontconfig settings that we are using for the layout tests. Manually verifying the Roboto ligatures however confirms that this works. BUG=617168 Review-Url: https://codereview.chromium.org/2871133004 Cr-Commit-Position: refs/heads/master@{#471277} Committed: https://chromium.googlesource.com/chromium/src/+/a377e3e1fa4f44a58f70abb2402e... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:110001) as https://chromium.googlesource.com/chromium/src/+/a377e3e1fa4f44a58f70abb2402e...
Message was sent while issue was closed.
machenbach@chromium.org changed reviewers: + machenbach@chromium.org
Message was sent while issue was closed.
Is this possibly breaking: https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux... ?
Message was sent while issue was closed.
On 2017/05/12 at 14:03:51, machenbach wrote: > Is this possibly breaking: > https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux... > ? Possible - I am checking locally, almost done building is_official build. If it's mine, I may have a simple fix.
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:110001) has been created in https://codereview.chromium.org/2879843003/ by drott@chromium.org. The reason for reverting is: Speculative revert for possibly fixing Linux builder compile failure which lead to tree closure https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux... .
Message was sent while issue was closed.
On 2017/05/12 14:40:25, drott wrote: > A revert of this CL (patchset #7 id:110001) has been created in > https://codereview.chromium.org/2879843003/ by mailto:drott@chromium.org. > > The reason for reverting is: Speculative revert for possibly fixing Linux > builder compile failure which lead to tree closure > https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux... > . I think this also leads to a crash on my linux ToT where I can't type a character.
Message was sent while issue was closed.
I posted the wrong build link the reason, this one should be it: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.chrome%2FGoogle... from builder https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
