Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2698043005:
Wire up Skia axis parameters to HarfBuzz (Closed)
|
Created:
3 years, 10 months ago by drott Modified:
3 years, 9 months ago CC:
ajuma+watch_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jbroman, Justin Novosad, kinuko+watch, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWire up Skia axis parameters to HarfBuzz
HarfBuzz requires the current axis coordinates to perform correct mark
placement. Now that Skia provides the required API [1] we pass them from
Skia to HarfBuzz.
[1] https://skia-review.googlesource.com/c/8861/
TEST=Currently manually using Noto Sans Arabic, TODO issue 693065
BUG=674879
Review-Url: https://codereview.chromium.org/2698043005
Cr-Commit-Position: refs/heads/master@{#453256}
Committed: https://chromium.googlesource.com/chromium/src/+/9d6c0f9ad3a5d10c6ffc45119308026be3ec9a86
Patch Set 1 #Patch Set 2 : The actual rebased version #
Total comments: 1
Patch Set 3 : CrOS build fix #
Total comments: 2
Messages
Total messages: 31 (16 generated)
The actual rebased version
Patchset #2 (id:20001) has been deleted
The actual rebased version
drott@chromium.org changed reviewers: + bungeman@chromium.org, eae@chromium.org
drott@chromium.org changed reviewers: + behdad@chromium.org
PTAL. I'll run it on the bots once the change lands in Skia and rolls to Chromium.
LGTM
https://codereview.chromium.org/2698043005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp (right): https://codereview.chromium.org/2698043005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp:407: } Shouldn't you set_var_coords to zero in the else branch? Aren't we reusing hb_font's? Maybe just remove the conditional.
On 2017/02/17 at 19:13:49, behdad wrote: > https://codereview.chromium.org/2698043005/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp (right): > > https://codereview.chromium.org/2698043005/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp:407: } > Shouldn't you set_var_coords to zero in the else branch? Aren't we reusing hb_font's? Maybe just remove the conditional. We reuse hb_font_t's but only based on SkTypeFace::uniqueId(), and since we instantiate a new SkTypeface for each new set of font-variation-settings, there's not hb_font_t with stale axis settings.
lgtm
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...
Description was changed from ========== Wire up Skia axis parameters to HarfBuzz HarfBuzz requires the current axis coordinates to perform correct mark placement. Now that Skia provides the required API [1] we pass them from Skia to HarfBuzz. [1] https://skia-review.googlesource.com/c/7130/ TEST=Currently manually using Noto Sans Arabic, TODO issue 693065 BUG=674879 ========== to ========== Wire up Skia axis parameters to HarfBuzz HarfBuzz requires the current axis coordinates to perform correct mark placement. Now that Skia provides the required API [1] we pass them from Skia to HarfBuzz. [1] https://skia-review.googlesource.com/c/8861/ TEST=Currently manually using Noto Sans Arabic, TODO issue 693065 BUG=674879 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
CrOS build fix
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...
The CQ bit was unchecked by drott@chromium.org
The CQ bit was checked by drott@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org, behdad@chromium.org Link to the patchset: https://codereview.chromium.org/2698043005/#ps60001 (title: "CrOS build fix")
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": 1488211027392090,
"parent_rev": "52deea2a2071bf852efc0c98268fbafcca6c6174", "commit_rev":
"9d6c0f9ad3a5d10c6ffc45119308026be3ec9a86"}
Message was sent while issue was closed.
Description was changed from ========== Wire up Skia axis parameters to HarfBuzz HarfBuzz requires the current axis coordinates to perform correct mark placement. Now that Skia provides the required API [1] we pass them from Skia to HarfBuzz. [1] https://skia-review.googlesource.com/c/8861/ TEST=Currently manually using Noto Sans Arabic, TODO issue 693065 BUG=674879 ========== to ========== Wire up Skia axis parameters to HarfBuzz HarfBuzz requires the current axis coordinates to perform correct mark placement. Now that Skia provides the required API [1] we pass them from Skia to HarfBuzz. [1] https://skia-review.googlesource.com/c/8861/ TEST=Currently manually using Noto Sans Arabic, TODO issue 693065 BUG=674879 Review-Url: https://codereview.chromium.org/2698043005 Cr-Commit-Position: refs/heads/master@{#453256} Committed: https://chromium.googlesource.com/chromium/src/+/9d6c0f9ad3a5d10c6ffc45119308... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/9d6c0f9ad3a5d10c6ffc45119308...
Message was sent while issue was closed.
bungeman@google.com changed reviewers: + bungeman@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/2698043005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp (right): https://codereview.chromium.org/2698043005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp:402: typeface->getVariationDesignPosition(axisValues.data(), axisValues.size()); Note that you should test that this comes back > 0 also. There are cases where we can determine how many axes there are, but then fail to retrieve them. https://codereview.chromium.org/2698043005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp:403: Vector<float> axisValuesFloat; Seems like this should be Vector<hb_variation_t> and call hb_font_set_variations below instead of hb_font_set_var_coords_design. While it might be the case that the axisValues are written in the order they are in the font, I can't really guarantee that all the time (which is why the axis tags are in there).
Message was sent while issue was closed.
Thanks, good points, addressing those in: https://codereview.chromium.org/2730443002
Message was sent while issue was closed.
On 2017/02/27 15:56:12, drott wrote: > CrOS build fix It's odd that you need this #if .... #endif on Feb 27 because CrOS harfbuzz was updated to 1.4.2 on Feb 25 (https://chromium-review.googlesource.com/c/444323/ )
Message was sent while issue was closed.
2017-03-06 22:34 GMT-08:00 <jshin@chromium.org>: > On 2017/02/27 15:56:12, drott wrote: > > CrOS build fix > > It's odd that you need this #if .... #endif on Feb 27 because CrOS > harfbuzz was > updated to 1.4.2 on Feb 25 > (https://chromium-review.googlesource.com/c/444323/ ) > Oh.. It's based on trybot failures a week before that. Anyway, that #if for hb-version check can be removed. > > https://codereview.chromium.org/2698043005/ > -- 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.
Message was sent while issue was closed.
2017-03-06 22:34 GMT-08:00 <jshin@chromium.org>: > On 2017/02/27 15:56:12, drott wrote: > > CrOS build fix > > It's odd that you need this #if .... #endif on Feb 27 because CrOS > harfbuzz was > updated to 1.4.2 on Feb 25 > (https://chromium-review.googlesource.com/c/444323/ ) > Oh.. It's based on trybot failures a week before that. Anyway, that #if for hb-version check can be removed. > > https://codereview.chromium.org/2698043005/ > -- 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. |
