Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2104653002:
Fix content shell layout test failure related to Fontconfig (Closed)
|
Created:
4 years, 5 months ago by Tom (Use chromium acct) Modified:
4 years, 5 months ago CC:
chromium-reviews, Daniel Erat Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix content shell layout test failure related to Fontconfig
Work around an issue that only occurs on libfontconfig.so.1.9.0
Also update a comment that's no longer accurate
BUG=621695
Committed: https://crrev.com/fb607474cba9881f85565dffaf924a55c6994bb5
Cr-Commit-Position: refs/heads/master@{#402342}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Oops #
Total comments: 1
Patch Set 3 : Add call to FcConfigBuildFonts #
Total comments: 1
Messages
Total messages: 25 (11 generated)
Description was changed from ========== Fix content shell layout test failure related to Fontconfig BUG=621695 ========== to ========== Fix content shell layout test failure related to Fontconfig Work around an issue that only occurs on libfontconfig.so.1.9.0 Also remove a comment that's no longer accurate BUG=621695 ==========
thomasanderson@google.com changed reviewers: + msw@chromium.org
The CQ bit was checked by thomasanderson@google.com 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...
msw@chromium.org changed reviewers: + behdad@chromium.org
It seems like this causes some cq failures? +behdad for review, +cc derat (jic you're interested) https://codereview.chromium.org/2104653002/diff/1/ui/gfx/test/fontconfig_util... File ui/gfx/test/fontconfig_util_linux.cc (left): https://codereview.chromium.org/2104653002/diff/1/ui/gfx/test/fontconfig_util... ui/gfx/test/fontconfig_util_linux.cc:8: nit: keep this blank line between include types?
derat@chromium.org changed reviewers: + derat@chromium.org
https://codereview.chromium.org/2104653002/diff/20001/ui/gfx/test/fontconfig_... File ui/gfx/test/fontconfig_util_linux.cc (right): https://codereview.chromium.org/2104653002/diff/20001/ui/gfx/test/fontconfig_... ui/gfx/test/fontconfig_util_linux.cc:68: CHECK(FcConfigGetCurrent()); i think that the reason for the old code was to start out with an empty config. does that happen with this approach, or do you end up with the user's local config? the test failures suggest that the latter may be happening.
The CQ bit was checked by thomasanderson@google.com 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...
Trying this again.. Verified that content_shell --run-layout-test and gfx_unittests both work with libfontconfig.so.1.8.0 and libfontconfig.so.1.9.0
On 2016/06/27 22:40:15, Daniel Erat wrote: > https://codereview.chromium.org/2104653002/diff/20001/ui/gfx/test/fontconfig_... > File ui/gfx/test/fontconfig_util_linux.cc (right): > > https://codereview.chromium.org/2104653002/diff/20001/ui/gfx/test/fontconfig_... > ui/gfx/test/fontconfig_util_linux.cc:68: CHECK(FcConfigGetCurrent()); > i think that the reason for the old code was to start out with an empty config. > does that happen with this approach, or do you end up with the user's local > config? the test failures suggest that the latter may be happening. Aha, that explains why the unit tests were failing :) Added FcConfigSetCurrent() back in patchset 3 and the tests pass.
lgtm
lgtm https://codereview.chromium.org/2104653002/diff/40001/ui/gfx/test/fontconfig_... File ui/gfx/test/fontconfig_util_linux.cc (right): https://codereview.chromium.org/2104653002/diff/40001/ui/gfx/test/fontconfig_... ui/gfx/test/fontconfig_util_linux.cc:79: CHECK(FcConfigSetCurrent(config)); interesting. https://www.freedesktop.org/software/fontconfig/fontconfig-devel/fcconfigsetc... says "Implicitly calls FcConfigBuildFonts if necessary". this seems fine to me if it fixes failures, though.
lgtm; please update the cl desc: "Also update a comment"
Description was changed from ========== Fix content shell layout test failure related to Fontconfig Work around an issue that only occurs on libfontconfig.so.1.9.0 Also remove a comment that's no longer accurate BUG=621695 ========== to ========== Fix content shell layout test failure related to Fontconfig Work around an issue that only occurs on libfontconfig.so.1.9.0 Also update a comment that's no longer accurate BUG=621695 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thomasanderson@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 ========== Fix content shell layout test failure related to Fontconfig Work around an issue that only occurs on libfontconfig.so.1.9.0 Also update a comment that's no longer accurate BUG=621695 ========== to ========== Fix content shell layout test failure related to Fontconfig Work around an issue that only occurs on libfontconfig.so.1.9.0 Also update a comment that's no longer accurate BUG=621695 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix content shell layout test failure related to Fontconfig Work around an issue that only occurs on libfontconfig.so.1.9.0 Also update a comment that's no longer accurate BUG=621695 ========== to ========== Fix content shell layout test failure related to Fontconfig Work around an issue that only occurs on libfontconfig.so.1.9.0 Also update a comment that's no longer accurate BUG=621695 Committed: https://crrev.com/fb607474cba9881f85565dffaf924a55c6994bb5 Cr-Commit-Position: refs/heads/master@{#402342} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/fb607474cba9881f85565dffaf924a55c6994bb5 Cr-Commit-Position: refs/heads/master@{#402342} |
