Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2280053002:
Restrict supported font formats in Chrome context (Closed)
DescriptionRestrict supported font formats in Chrome context
Chrome's complex text path uses HarfBuzz OpenType glyph lookup
functions. These do not support glyph lookup in Type 1 / Postscript
fonts and we do not wish to support those in Chrome any longer.
In order to avoid matching fonts against Type 1 fonts possible present
on Linux installations and exposed through fontconfig, this CL filters
those out in Skia's font matching code.
This change might not be desirable for all context in which Skia is
used, hence making it conditional on a Chrome context define.
BUG=skia:5685
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2280053002
Committed: https://skia.googlesource.com/skia/+/358f93de3d16bd3fe5b0d043ea15b59ebc994e86
Patch Set 1 #
Total comments: 4
Patch Set 2 : bungeman@'s review comments addressed #Messages
Total messages: 26 (17 generated)
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Description was changed from ========== Restrict supported font formats in Chrome context Chrome's complex text path uses HarfBuzz OpenType glyph lookup functions. These do not support glyph lookup in Type 1 / Postscript fonts and we do not wish to support those in Chrome any longer. In order to avoid matching fonts against Type 1 fonts possible present on Linux installations and exposed through fontconfig, this CL filters those out in Skia's font matching code. This change might not be desirable for all context in which Skia is used, hence making it conditional on a Chrome context define. BUG=skia:5685 ========== to ========== Restrict supported font formats in Chrome context Chrome's complex text path uses HarfBuzz OpenType glyph lookup functions. These do not support glyph lookup in Type 1 / Postscript fonts and we do not wish to support those in Chrome any longer. In order to avoid matching fonts against Type 1 fonts possible present on Linux installations and exposed through fontconfig, this CL filters those out in Skia's font matching code. This change might not be desirable for all context in which Skia is used, hence making it conditional on a Chrome context define. BUG=skia:5685 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2280053002 ==========
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, bungeman@chromium.org
drott@chromium.org changed reviewers: + derat@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bungeman@google.com changed reviewers: + bungeman@google.com
https://codereview.chromium.org/2280053002/diff/1/gyp/skia_for_chromium_defin... File gyp/skia_for_chromium_defines.gypi (right): https://codereview.chromium.org/2280053002/diff/1/gyp/skia_for_chromium_defin... gyp/skia_for_chromium_defines.gypi:17: 'SK_RESTRICT_FONT_FORMATS_OPENTYPE' This define should live in <chromium>/src/skia/config/SkUserConfig.h or <chromium>/src/skia/BUILD.gn:skia_library_config defines. This file is more of a temporary place to land defines, it's not really intended for long term use. In fact, I'm not sure why the other one is currently hanging around in here. For consistency this define should be named SK_FONT_CONFIG_INTERFACE_ONLY_ALLOW_SFNT_FONTS since I think that's what is actually wanted. Though it might be more accurate to state which tables are required (cmap, hmtx). https://codereview.chromium.org/2280053002/diff/1/src/ports/SkFontConfigInter... File src/ports/SkFontConfigInterface_direct.cpp (right): https://codereview.chromium.org/2280053002/diff/1/src/ports/SkFontConfigInter... src/ports/SkFontConfigInterface_direct.cpp:537: return false; nit: Skia uses four space indents. I think I would write this like const char* font_format = get_name(pattern, FC_FONTFORMAT); if (font_format && strcmp(font_format, kFontFormatTrueType) != 0 && strcmp(font_format, kFontFormatCFF) != 0) { return false; } to avoid the extra line wrapping.
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...
Patchset #2 (id:20001) has been deleted
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...
Thanks for the review, CL updated. https://codereview.chromium.org/2280053002/diff/1/gyp/skia_for_chromium_defin... File gyp/skia_for_chromium_defines.gypi (right): https://codereview.chromium.org/2280053002/diff/1/gyp/skia_for_chromium_defin... gyp/skia_for_chromium_defines.gypi:17: 'SK_RESTRICT_FONT_FORMATS_OPENTYPE' Define renamed to SK_FONT_CONFIG_INTERFACE_ONLY_ALLOW_SFNT_FONTS https://codereview.chromium.org/2280053002/diff/1/src/ports/SkFontConfigInter... File src/ports/SkFontConfigInterface_direct.cpp (right): https://codereview.chromium.org/2280053002/diff/1/src/ports/SkFontConfigInter... src/ports/SkFontConfigInterface_direct.cpp:537: return false; Reformatted according to your suggestion.
Could you please add it to the commit queue if it looks okay to you? Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I can't say that the prospect of artificially limiting the fonts used by Chromium thrills me, but this code lgtm.
The CQ bit was checked by bungeman@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Restrict supported font formats in Chrome context Chrome's complex text path uses HarfBuzz OpenType glyph lookup functions. These do not support glyph lookup in Type 1 / Postscript fonts and we do not wish to support those in Chrome any longer. In order to avoid matching fonts against Type 1 fonts possible present on Linux installations and exposed through fontconfig, this CL filters those out in Skia's font matching code. This change might not be desirable for all context in which Skia is used, hence making it conditional on a Chrome context define. BUG=skia:5685 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2280053002 ========== to ========== Restrict supported font formats in Chrome context Chrome's complex text path uses HarfBuzz OpenType glyph lookup functions. These do not support glyph lookup in Type 1 / Postscript fonts and we do not wish to support those in Chrome any longer. In order to avoid matching fonts against Type 1 fonts possible present on Linux installations and exposed through fontconfig, this CL filters those out in Skia's font matching code. This change might not be desirable for all context in which Skia is used, hence making it conditional on a Chrome context define. BUG=skia:5685 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2280053002 Committed: https://skia.googlesource.com/skia/+/358f93de3d16bd3fe5b0d043ea15b59ebc994e86 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://skia.googlesource.com/skia/+/358f93de3d16bd3fe5b0d043ea15b59ebc994e86
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
On 2016/08/26 at 17:07:18, bungeman wrote: > I can't say that the prospect of artificially limiting the fonts used by Chromium thrills me, but this code lgtm. Perhaps some explanations on this. In our code in Chrome in HarfBuzzFace.cpp we were able to reduce memory consumption quite significantly, we removed an extra cache and HarfBuzz access the font blob through a zero-copy access now. This comes at the cost of sacrificing Type 1 fonts, which do not play an important role in a web context. That's why I feel we're on the right track restricting font formats. I wouldn't call it artificial, but consciously doing it for the benefits of lower memory consumption and better maintainability of the Chrome side code. |
