Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2877813002:
[headless] Enable headless embedders to specify fonts. (Closed)
Description[headless] Enable headless embedders to specify fonts.
R=skyostil@chromium.org
Review-Url: https://codereview.chromium.org/2877813002
Cr-Commit-Position: refs/heads/master@{#471790}
Committed: https://chromium.googlesource.com/chromium/src/+/eb2f886f0b9491f8e01ee197cd82a909c11102a6
Patch Set 1 #
Total comments: 16
Patch Set 2 : addressed skyostil@'s comments #
Total comments: 2
Patch Set 3 : addressed comments #Patch Set 4 : rebased #
Messages
Total messages: 30 (21 generated)
PTAL
The CQ bit was checked by altimin@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Thanks! https://codereview.chromium.org/2877813002/diff/1/build/args/headless.gn File build/args/headless.gn (right): https://codereview.chromium.org/2877813002/diff/1/build/args/headless.gn#newc... build/args/headless.gn:18: # Expose headless binginds for freetype library bundled with Chromium. typo: bindings https://codereview.chromium.org/2877813002/diff/1/headless/headless.gni File headless/headless.gni (right): https://codereview.chromium.org/2877813002/diff/1/headless/headless.gni#newcode9 headless/headless.gni:9: # Provide bindins for font loading for headless embedders. typo: bindings https://codereview.chromium.org/2877813002/diff/1/headless/public/util/fontco... File headless/public/util/fontconfig.cc (right): https://codereview.chromium.org/2877813002/diff/1/headless/public/util/fontco... headless/public/util/fontconfig.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. ++year https://codereview.chromium.org/2877813002/diff/1/headless/public/util/fontco... headless/public/util/fontconfig.cc:67: // The fonts must be loaded in a consistent order. This makes the rendered rendered? https://codereview.chromium.org/2877813002/diff/1/headless/public/util/fontco... File headless/public/util/fontconfig.h (right): https://codereview.chromium.org/2877813002/diff/1/headless/public/util/fontco... headless/public/util/fontconfig.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. ++year https://codereview.chromium.org/2877813002/diff/1/headless/public/util/fontco... headless/public/util/fontconfig.h:5: #ifndef HEADLESS_PUBLIC_INIT_FONTS_H_ HEADLESS_PUBLIC_UTIL_FONTCONFIG_H https://codereview.chromium.org/2877813002/diff/1/headless/public/util/fontco... headless/public/util/fontconfig.h:10: // Wrapper around FcInit() given that libfreetype is built into chromium. Let's describe what this actually does, i.e., initializes fontconfig without following symlinks and preloads all fonts. https://codereview.chromium.org/2877813002/diff/1/headless/public/util/fontco... headless/public/util/fontconfig.h:17: #endif // HEADLESS_PUBLIC_INIT_FONTS_H_ Ditto.
PTAL https://codereview.chromium.org/2877813002/diff/1/build/args/headless.gn File build/args/headless.gn (right): https://codereview.chromium.org/2877813002/diff/1/build/args/headless.gn#newc... build/args/headless.gn:18: # Expose headless binginds for freetype library bundled with Chromium. On 2017/05/12 16:35:51, Sami wrote: > typo: bindings Done. https://codereview.chromium.org/2877813002/diff/1/headless/headless.gni File headless/headless.gni (right): https://codereview.chromium.org/2877813002/diff/1/headless/headless.gni#newcode9 headless/headless.gni:9: # Provide bindins for font loading for headless embedders. On 2017/05/12 16:35:51, Sami wrote: > typo: bindings I don't like that word very much, do I? https://codereview.chromium.org/2877813002/diff/1/headless/public/util/fontco... File headless/public/util/fontconfig.cc (right): https://codereview.chromium.org/2877813002/diff/1/headless/public/util/fontco... headless/public/util/fontconfig.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/05/12 16:35:52, Sami wrote: > ++year Done. https://codereview.chromium.org/2877813002/diff/1/headless/public/util/fontco... headless/public/util/fontconfig.cc:67: // The fonts must be loaded in a consistent order. This makes the rendered On 2017/05/12 16:35:52, Sami wrote: > rendered? Done. https://codereview.chromium.org/2877813002/diff/1/headless/public/util/fontco... File headless/public/util/fontconfig.h (right): https://codereview.chromium.org/2877813002/diff/1/headless/public/util/fontco... headless/public/util/fontconfig.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/05/12 16:35:52, Sami wrote: > ++year Done. https://codereview.chromium.org/2877813002/diff/1/headless/public/util/fontco... headless/public/util/fontconfig.h:5: #ifndef HEADLESS_PUBLIC_INIT_FONTS_H_ On 2017/05/12 16:35:52, Sami wrote: > HEADLESS_PUBLIC_UTIL_FONTCONFIG_H Done. https://codereview.chromium.org/2877813002/diff/1/headless/public/util/fontco... headless/public/util/fontconfig.h:10: // Wrapper around FcInit() given that libfreetype is built into chromium. On 2017/05/12 16:35:52, Sami wrote: > Let's describe what this actually does, i.e., initializes fontconfig without > following symlinks and preloads all fonts. Done. https://codereview.chromium.org/2877813002/diff/1/headless/public/util/fontco... headless/public/util/fontconfig.h:17: #endif // HEADLESS_PUBLIC_INIT_FONTS_H_ On 2017/05/12 16:35:52, Sami wrote: > Ditto. Done.
The CQ bit was checked by altimin@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm, thanks! https://codereview.chromium.org/2877813002/diff/20001/headless/public/util/fo... File headless/public/util/fontconfig.h (right): https://codereview.chromium.org/2877813002/diff/20001/headless/public/util/fo... headless/public/util/fontconfig.h:13: // enviroments. typo: environments
altimin@chromium.org changed reviewers: + jochen@chromium.org
+jochen@: can you please stamp build/args/headless.gn?
lgtm
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2877813002/diff/20001/headless/public/util/fo... File headless/public/util/fontconfig.h (right): https://codereview.chromium.org/2877813002/diff/20001/headless/public/util/fo... headless/public/util/fontconfig.h:13: // enviroments. On 2017/05/15 10:18:38, Sami wrote: > typo: environments Done.
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by altimin@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: 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 altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2877813002/#ps60001 (title: "rebased")
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": 60001, "attempt_start_ts": 1494862460886120,
"parent_rev": "4f5fc1b7bd67d34a07bdeb5c69cd8ae101d65083", "commit_rev":
"eb2f886f0b9491f8e01ee197cd82a909c11102a6"}
Message was sent while issue was closed.
Description was changed from ========== [headless] Enable headless embedders to specify fonts. R=skyostil@chromium.org ========== to ========== [headless] Enable headless embedders to specify fonts. R=skyostil@chromium.org Review-Url: https://codereview.chromium.org/2877813002 Cr-Commit-Position: refs/heads/master@{#471790} Committed: https://chromium.googlesource.com/chromium/src/+/eb2f886f0b9491f8e01ee197cd82... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/eb2f886f0b9491f8e01ee197cd82... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
