Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2259733002:
Require certain faimilies to contain certain styles (Closed)
|
Created:
4 years, 4 months ago by Ilya Kulshin Modified:
4 years, 4 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRequire certain faimilies to contain certain styles
This is a workaround for 635932, so that Chrome will ignore certain font
families unless they contain the specified font styles. We start out
requiring that Helvetica and Open Sans contain the regular variant,
because in some cases they get installed only in specialized variants
which results in fonts incorrectly displaying bold or condensed.
BUG=635932
Committed: https://crrev.com/0bf300d07fe226dc48da9cbfaa6d9ea36480f66d
Cr-Commit-Position: refs/heads/master@{#413323}
Patch Set 1 #Patch Set 2 : Update comments and add telemetry #Patch Set 3 : Add a dcheck and don't block attempted loading if font cannot be loaded at all #
Total comments: 2
Messages
Total messages: 29 (18 generated)
The CQ bit was checked by kulshin@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: This issue passed the CQ dry run.
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
The CQ bit was unchecked by kulshin@chromium.org
The CQ bit was checked by kulshin@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_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by kulshin@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...
kulshin@chromium.org changed reviewers: + ananta@chromium.org, eae@chromium.org, holte@chromium.org
ptal: this is a workaround for crbug.com/635932 ananata@ - dwrite_font_proxy_message_filter_win.cc holte@ - histograms.xml eae@ - FYI: I still intend to pursue a fix similar to what is outlined in https://codereview.chromium.org/2248693003/, but I'd like to get this landed in M53 to help users currently experiencing this bug.
The CQ bit was unchecked by kulshin@chromium.org
Seems entirely reasonable to me and will certainly help a lot of people missing the standard variant of those fonts.
histograms lgtm
https://codereview.chromium.org/2259733002/diff/40001/content/browser/rendere... File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc (right): https://codereview.chromium.org/2259733002/diff/40001/content/browser/rendere... content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:146: DWRITE_FONT_STYLE_NORMAL}, Is this list likely to grow?. Do IE and Firefox do something like this
https://codereview.chromium.org/2259733002/diff/40001/content/browser/rendere... File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc (right): https://codereview.chromium.org/2259733002/diff/40001/content/browser/rendere... content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:146: DWRITE_FONT_STYLE_NORMAL}, On 2016/08/19 21:19:49, ananta wrote: > Is this list likely to grow?. Do IE and Firefox do something like this I think we will want to add 'Gill Sans' to the list, since we already have a similar mechanism that excludes certain Gill Sans styles completely (that approach is more difficult to implement in this case, since I don't want to blanket-ban Helvetica Bold and Open Sans Condensed, and I don't know the full set of Helvetica and Open Sans styles anyway). I didn't make that change in this patch to keep this straightforward. I did an informal survey on the forums and did not get any reports of anything other than Open Sans and Helvetica (and Arial, but that's a different bug). Longer term, I want to do a more comprehensive solution similar to https://codereview.chromium.org/2248693003/ so I don't anticipate this list growing much. Don't know about IE/Firefox, but Edge somehow also manages to exclude Open Sans if it is only available in the condensed style.
LGTM.
The CQ bit was checked by kulshin@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by kulshin@chromium.org
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Require certain faimilies to contain certain styles This is a workaround for 635932, so that Chrome will ignore certain font families unless they contain the specified font styles. We start out requiring that Helvetica and Open Sans contain the regular variant, because in some cases they get installed only in specialized variants which results in fonts incorrectly displaying bold or condensed. BUG=635932 ========== to ========== Require certain faimilies to contain certain styles This is a workaround for 635932, so that Chrome will ignore certain font families unless they contain the specified font styles. We start out requiring that Helvetica and Open Sans contain the regular variant, because in some cases they get installed only in specialized variants which results in fonts incorrectly displaying bold or condensed. BUG=635932 Committed: https://crrev.com/0bf300d07fe226dc48da9cbfaa6d9ea36480f66d Cr-Commit-Position: refs/heads/master@{#413323} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0bf300d07fe226dc48da9cbfaa6d9ea36480f66d Cr-Commit-Position: refs/heads/master@{#413323} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
