Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1912013002:
Use the new legacyCreateTypeface that has an SkFontStyle parameter (Closed)
|
Created:
4 years, 8 months ago by Tom (Use chromium acct) Modified:
4 years, 7 months ago CC:
chromium-reviews, krit, pdr+graphicswatchlist_chromium.org, drott+blinkwatch_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, jbroman, Justin Novosad, Rik, Stephen Chennney, blink-reviews, f(malita), danakj+watch_chromium.org, kinuko+watch, rwlbuis, Lei Zhang, bungeman-chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse the new legacyCreateTypeface that has an SkFontStyle parameter
Depends on Skia CL https://codereview.chromium.org/1905253002/
BUG=368442
Committed: https://crrev.com/f24ce736e38fa9b159d59dab44b7cc2c69a67849
Cr-Commit-Position: refs/heads/master@{#390426}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Use the new legacyCreateTypeface that has an SkFontStyle paramete #
Total comments: 4
Patch Set 3 : Remove unnecessary ifdef #
Total comments: 1
Patch Set 4 : Rebase #
Total comments: 6
Patch Set 5 : Only synthetic bold if returned font weight is more than 200 weight units less than requested. #
Total comments: 2
Patch Set 6 : Treat WebKit oblique style as skia italic style #
Total comments: 2
Patch Set 7 : Rebase #Messages
Total messages: 62 (23 generated)
Description was changed from ========== Use the new CreateFromName that has an SkFontStyle parameter BUG=368442 ========== to ========== Use the new CreateFromName that has an SkFontStyle parameter Depends on Skia CL https://codereview.chromium.org/1905253002/ BUG=368442 ==========
thomasanderson@google.com changed reviewers: + eae@chromium.org
LGTM w/nit https://codereview.chromium.org/1912013002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/1912013002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:237: // FIXME: Use m_fontManager, SkFontStyle and matchFamilyStyle instead of Please change this to remove the reference to SkFontStyle as you are effectively making that part of the change.
bungeman@google.com changed reviewers: + bungeman@google.com
https://codereview.chromium.org/1912013002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/1912013002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:239: return adoptRef(SkTypeface::CreateFromName(name.data(), fontStyle(fontDescription))); Why not just // FIXME: Use m_fontManager, matchFamilyStyle legacyCreateTypeface on all platforms. RefPtr<SkFontMgr> fm = adoptRef(SkFontMgr::RefDefault()); return adoptRef(fm->legacyCreateTypeface(name.data(), fontStyle(fontDescription))); No need to add anything to Skia, tested to to work with the 'Ubuntu' font locally (apparently the only font I have locally installed with many weights), makes it obvious that this needs to always have a m_fontManager in the long run.
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/patch-status/1912013002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1912013002/20001
https://codereview.chromium.org/1912013002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/1912013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:174: #if OS(WIN) || OS(LINUX) You'll need to remove this ifdef block for Android I think.
https://codereview.chromium.org/1912013002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/1912013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:174: #if OS(WIN) || OS(LINUX) On 2016/04/22 18:42:28, bungeman1 wrote: > You'll need to remove this ifdef block for Android I think. I stated that poorly, the ifdef is no longer needed, since the fontStyle function is now used on all platforms (the ifdef was just here to prevent platforms which didn't use it from getting unused static function warnings as errors).
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
https://codereview.chromium.org/1912013002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/1912013002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:237: // FIXME: Use m_fontManager, SkFontStyle and matchFamilyStyle instead of On 2016/04/22 02:05:46, eae wrote: > Please change this to remove the reference to SkFontStyle as you are effectively > making that part of the change. Done. https://codereview.chromium.org/1912013002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:239: return adoptRef(SkTypeface::CreateFromName(name.data(), fontStyle(fontDescription))); On 2016/04/22 18:15:36, bungeman1 wrote: > Why not just > > // FIXME: Use m_fontManager, matchFamilyStyle legacyCreateTypeface on all > platforms. > RefPtr<SkFontMgr> fm = adoptRef(SkFontMgr::RefDefault()); > return adoptRef(fm->legacyCreateTypeface(name.data(), > fontStyle(fontDescription))); > > No need to add anything to Skia, tested to to work with the 'Ubuntu' font > locally (apparently the only font I have locally installed with many weights), > makes it obvious that this needs to always have a m_fontManager in the long run. Done. https://codereview.chromium.org/1912013002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/1912013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:174: #if OS(WIN) || OS(LINUX) On 2016/04/22 18:42:28, bungeman1 wrote: > You'll need to remove this ifdef block for Android I think. Done. https://codereview.chromium.org/1912013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:174: #if OS(WIN) || OS(LINUX) On 2016/04/22 18:44:49, bungeman1 wrote: > On 2016/04/22 18:42:28, bungeman1 wrote: > > You'll need to remove this ifdef block for Android I think. > > I stated that poorly, the ifdef is no longer needed, since the fontStyle > function is now used on all platforms (the ifdef was just here to prevent > platforms which didn't use it from getting unused static function warnings as > errors). Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1912013002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1912013002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/1912013002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/1912013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:239: fontStyle(fontDescription))); nit: eae probably knows about this better than I do, but blink tends toward much longer lines to keep each statement on one line if possible. Note the length of the comments above. I generally try to keep it down to ~100, though some of these look more like ~120.
On 2016/04/22 19:00:21, bungeman1 wrote: > https://codereview.chromium.org/1912013002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): > > https://codereview.chromium.org/1912013002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:239: > fontStyle(fontDescription))); > nit: eae probably knows about this better than I do, but blink tends toward much > longer lines to keep each statement on one line if possible. Note the length of > the comments above. I generally try to keep it down to ~100, though some of > these look more like ~120. We're moving towards converging with chromium so 80 cols makes sense for new code.
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/patch-status/1912013002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1912013002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Just wanted to say that I went and built with this for Android and it works, which is great. On the other hand, for some reason 600 is bolder than the other weights, I'll have to see why that is.
Description was changed from ========== Use the new CreateFromName that has an SkFontStyle parameter Depends on Skia CL https://codereview.chromium.org/1905253002/ BUG=368442 ========== to ========== Use the new legacyCreateTypeface that has an SkFontStyle parameter Depends on Skia CL https://codereview.chromium.org/1905253002/ BUG=368442 ==========
I was having a similar issue here: https://deb53d85a427140a6e10ee61e1ceb3408c8407f2.googledrive.com/host/0B153_a... The 100 and 300 font weights were the same, but I learned that's actually a bug in the CJK fonts https://github.com/googlei18n/noto-fonts/issues/200. So, perhaps it's an issue with the installed fonts? Or, when I was debugging my first patch, some fonts were getting rendered really bold, and I found that was because I was applying fake bold on top of an already bold font. On Fri, Apr 22, 2016 at 12:49 PM, <bungeman@google.com> wrote: > Just wanted to say that I went and built with this for Android and it > works, > which is great. On the other hand, for some reason 600 is bolder than the > other > weights, I'll have to see why that is. > > https://codereview.chromium.org/1912013002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2016/04/22 20:05:13, blink-reviews wrote: > I was having a similar issue here: > https://deb53d85a427140a6e10ee61e1ceb3408c8407f2.googledrive.com/host/0B153_a... > > The 100 and 300 font weights were the same, but I learned that's actually a > bug in the CJK fonts https://github.com/googlei18n/noto-fonts/issues/200. > So, perhaps it's an issue with the installed fonts? Or, when I was > debugging my first patch, some fonts were getting rendered really bold, and > I found that was because I was applying fake bold on top of an already bold > font. > > On Fri, Apr 22, 2016 at 12:49 PM, <mailto:bungeman@google.com> wrote: > > > Just wanted to say that I went and built with this for Android and it > > works, > > which is great. On the other hand, for some reason 600 is bolder than the > > other > > weights, I'll have to see why that is. > > > > https://codereview.chromium.org/1912013002/ > > > > -- > You received this message because you are subscribed to the Google Groups "Blink > Reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:blink-reviews+unsubscribe@chromium.org. On a current vanilla build of Android "sans-serif" and "sans-serif-condensed" (Roboto) are the only fonts with more than two weights. A number of fonts have normal and bold, and a number of other fonts just have normal weight.
On 2016/04/22 20:15:09, bungeman1 wrote: > On 2016/04/22 20:05:13, blink-reviews wrote: > > I was having a similar issue here: > > > https://deb53d85a427140a6e10ee61e1ceb3408c8407f2.googledrive.com/host/0B153_a... > > > > The 100 and 300 font weights were the same, but I learned that's actually a > > bug in the CJK fonts https://github.com/googlei18n/noto-fonts/issues/200. > > So, perhaps it's an issue with the installed fonts? Or, when I was > > debugging my first patch, some fonts were getting rendered really bold, and > > I found that was because I was applying fake bold on top of an already bold > > font. > > > > On Fri, Apr 22, 2016 at 12:49 PM, <mailto:bungeman@google.com> wrote: > > > > > Just wanted to say that I went and built with this for Android and it > > > works, > > > which is great. On the other hand, for some reason 600 is bolder than the > > > other > > > weights, I'll have to see why that is. > > > > > > https://codereview.chromium.org/1912013002/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > "Blink > > Reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:blink-reviews+unsubscribe@chromium.org. > > On a current vanilla build of Android "sans-serif" and "sans-serif-condensed" > (Roboto) are the only fonts with more than two weights. A number of fonts have > normal and bold, and a number of other fonts just have normal weight. In the case of the 600 weight being too bold, note that there is no exact 600 weight "sans-serif" on a vanilla Android, so it's going to come back as either 500 or 700 (I haven't checked which yet, but probably 700) and it looks like it's being fake-bolded. Part of the issue is that you only really want to fake bold the heaviest weight in a family. It's possible that there are exceptions, but generally fake bolding a middle weight will produce something that looks strange when used against a heavier designed weight. Before, when the only weights were normal and bold this was fairly simple to figure out (if you asked for bold but got normal, fake bold), but now it's a bit more subtle. Note that 800 weight is also missing but doesn't get fake bolded since it's already considered bold (it's over 70 weight).
I was having a similar issue here: https://deb53d85a427140a6e10ee61e1ceb3408c8407f2.googledrive.com/host/0B153_a... The 100 and 300 font weights were the same, but I learned that's actually a bug in the CJK fonts https://github.com/googlei18n/noto-fonts/issues/200. So, perhaps it's an issue with the installed fonts? Or, when I was debugging my first patch, some fonts were getting rendered really bold, and I found that was because I was applying fake bold on top of an already bold font. On Fri, Apr 22, 2016 at 12:49 PM, <bungeman@google.com> wrote: > Just wanted to say that I went and built with this for Android and it > works, > which is great. On the other hand, for some reason 600 is bolder than the > other > weights, I'll have to see why that is. > > https://codereview.chromium.org/1912013002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org Link to the patchset: https://codereview.chromium.org/1912013002/#ps60001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1912013002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1912013002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1912013002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/1912013002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:157: static inline SkFontStyle fontStyle(const FontDescription& fontDescription) Looks like this moved over to FontDescription::skiaFontStyle, let's not add it back. https://codereview.chromium.org/1912013002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:221: fontStyle(fontDescription))); fontDescription.skiaFontStyle() instead of fontStyle(fontDescription) https://codereview.chromium.org/1912013002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:236: (fontDescription.weight() >= FontWeight600 && !tf->isBold()) || fontDescription.isSyntheticBold(), So what appears to happening on on Android is that there is a request for 600, but 500 is being returned as the closest match (we may need to keep this non-css3 matching behavior for backwards compatibility). As a result, tf->isBold() returns false (since it's only 500 weight), but the fontDescription says it was 600 or greater, so it gets synthetic bold. This is a bit awkward, we should only synthetic bold a returned weight if it is at least 200 or 300 less than what was requested to avoid this.
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/patch-status/1912013002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1912013002/80001
Thanks for working with me on this, Ben. Can you verify the synthetic bolding issue is fixed on Android? Also, it looks like some layout tests related to italic fonts are failing here https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r... It looks like we're applying synthetic italics. Perhaps fontconfig is prioritizing weight over italics when matching fonts? https://codereview.chromium.org/1912013002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/1912013002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:157: static inline SkFontStyle fontStyle(const FontDescription& fontDescription) On 2016/04/25 14:58:00, bungeman1 wrote: > Looks like this moved over to FontDescription::skiaFontStyle, let's not add it > back. Done. https://codereview.chromium.org/1912013002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:221: fontStyle(fontDescription))); On 2016/04/25 14:58:00, bungeman1 wrote: > fontDescription.skiaFontStyle() instead of fontStyle(fontDescription) Done. https://codereview.chromium.org/1912013002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:236: (fontDescription.weight() >= FontWeight600 && !tf->isBold()) || fontDescription.isSyntheticBold(), On 2016/04/25 14:58:01, bungeman1 wrote: > So what appears to happening on on Android is that there is a request for 600, > but 500 is being returned as the closest match (we may need to keep this > non-css3 matching behavior for backwards compatibility). As a result, > tf->isBold() returns false (since it's only 500 weight), but the fontDescription > says it was 600 or greater, so it gets synthetic bold. This is a bit awkward, we > should only synthetic bold a returned weight if it is at least 200 or 300 less > than what was requested to avoid this. Done.
https://codereview.chromium.org/1912013002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (left): https://codereview.chromium.org/1912013002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:206: style |= SkTypeface::kItalic; This or'ed in kItalic if the fontDescription was either italic or oblique. skiaFontStyle only sets the italic bit if the fontDescription was italic (but not when it is oblique). I think I've just convinced reed that we need separate italic and oblique bits in Skia (mostly due to http://lucidafonts.com/fonts/family/lucida-sans and a lesser extent http://www.gust.org.pl/projects/e-foundry/latin-modern ). I'll see if I can get it working in Skia today, but for the time being you may need to have skiaFontStyle set the italic bit on the SkFontStyle using this logic instead of it's current logic to prevent any changes here.
On 2016/04/25 19:18:03, bungeman1 wrote: > https://codereview.chromium.org/1912013002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (left): > > https://codereview.chromium.org/1912013002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:206: style |= > SkTypeface::kItalic; > This or'ed in kItalic if the fontDescription was either italic or oblique. > skiaFontStyle only sets the italic bit if the fontDescription was italic (but > not when it is oblique). I think I've just convinced reed that we need separate > italic and oblique bits in Skia (mostly due to > http://lucidafonts.com/fonts/family/lucida-sans and a lesser extent > http://www.gust.org.pl/projects/e-foundry/latin-modern ). I'll see if I can get > it working in Skia today, but for the time being you may need to have > skiaFontStyle set the italic bit on the SkFontStyle using this logic instead of > it's current logic to prevent any changes here. Also, I'm not sure about this, but does FontCustomPlatformData::fontPlatformData also need to be updated for the fake bold change?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/patch-status/1912013002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1912013002/100001
I'm not 100% sure, but I don't think FontCustomPlatformData::fontPlatformData has an issue with synthetic bold because it takes a "bool bold", not a FontDescription with a real weight. https://codereview.chromium.org/1912013002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (left): https://codereview.chromium.org/1912013002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:206: style |= SkTypeface::kItalic; On 2016/04/25 19:18:02, bungeman1 wrote: > This or'ed in kItalic if the fontDescription was either italic or oblique. > skiaFontStyle only sets the italic bit if the fontDescription was italic (but > not when it is oblique). I think I've just convinced reed that we need separate > italic and oblique bits in Skia (mostly due to > http://lucidafonts.com/fonts/family/lucida-sans and a lesser extent > http://www.gust.org.pl/projects/e-foundry/latin-modern ). I'll see if I can get > it working in Skia today, but for the time being you may need to have > skiaFontStyle set the italic bit on the SkFontStyle using this logic instead of > it's current logic to prevent any changes here. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/1912013002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/FontDescription.cpp (right): https://codereview.chromium.org/1912013002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/FontDescription.cpp:284: // FIXME: handle oblique styles when they're supported in skia So Skia now has an SkFontStyle::kOblique_Slant. I created a cl to update this code here to us it, see https::/crrev.com/1921503006/ . On Windows this changes the four layout tests that actually ask for oblique, but unfortunately the rebaseline bot / NeedsRebaseline is currently disabled, making things difficult.
Thanks for adding the oblique support. I'll hold off on submitting until your new CL lands so I can rebase and get the support in FontDescription.cpp
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/patch-status/1912013002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1912013002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1912013002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/FontDescription.cpp (right): https://codereview.chromium.org/1912013002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/FontDescription.cpp:284: // FIXME: handle oblique styles when they're supported in skia On 2016/04/27 20:42:14, bungeman-skia wrote: > So Skia now has an SkFontStyle::kOblique_Slant. I created a cl to update this > code here to us it, see https::/crrev.com/1921503006/ . On Windows this changes > the four layout tests that actually ask for oblique, but unfortunately the > rebaseline bot / NeedsRebaseline is currently disabled, making things difficult. Acknowledged.
bungeman@chromium.org changed reviewers: + bungeman@chromium.org
lgtm, seems to work well everywhere. On the other hand it is annoying that this doesn't hit any layout tests. It appears that the reason for that on linux is that the default fonts on the bots only have normal and bold weights, so we're not seeing any changes there. On Android we're only running the "smoke tests" and I doubt any of them exercise this. Something for the future I suppose.
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org Link to the patchset: https://codereview.chromium.org/1912013002/#ps120001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1912013002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1912013002/120001
Message was sent while issue was closed.
Description was changed from ========== Use the new legacyCreateTypeface that has an SkFontStyle parameter Depends on Skia CL https://codereview.chromium.org/1905253002/ BUG=368442 ========== to ========== Use the new legacyCreateTypeface that has an SkFontStyle parameter Depends on Skia CL https://codereview.chromium.org/1905253002/ BUG=368442 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/f24ce736e38fa9b159d59dab44b7cc2c69a67849 Cr-Commit-Position: refs/heads/master@{#390426}
Message was sent while issue was closed.
Description was changed from ========== Use the new legacyCreateTypeface that has an SkFontStyle parameter Depends on Skia CL https://codereview.chromium.org/1905253002/ BUG=368442 ========== to ========== Use the new legacyCreateTypeface that has an SkFontStyle parameter Depends on Skia CL https://codereview.chromium.org/1905253002/ BUG=368442 Committed: https://crrev.com/f24ce736e38fa9b159d59dab44b7cc2c69a67849 Cr-Commit-Position: refs/heads/master@{#390426} ==========
Message was sent while issue was closed.
On 2016/04/28 18:09:37, bungeman-chromium wrote: > lgtm, seems to work well everywhere. On the other hand it is annoying that this > doesn't hit any layout tests. It appears that the reason for that on linux is > that the default fonts on the bots only have normal and bold weights, so we're > not seeing any changes there. On Android we're only running the "smoke tests" > and I doubt any of them exercise this. Something for the future I suppose This is great. I'll add all 6 weights of Roboto and all 6 (or 7) weights of Noto Sans CJK to Chrome OS. Then, at least, we can make a layout test for this on Linux with cros=1 (I believe) once cros font package for testing is updated. |
