Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2017533002:
Simplify OpenType caps feature detection logic (Closed)
|
Created:
4 years, 7 months ago by drott Modified:
4 years, 6 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, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSimplify OpenType caps feature detection logic
We can rely on HarfBuzz API for finding the required OpenType
features, instead of implementing our own find loops.
Thanks to Behdad for the suggestion.
TEST=LayoutTest/fast/text/font-features/*
BUG=603137
R=behdad,eae
Committed: https://crrev.com/c51ee10d5afd2a657ed357cda9caf31c788790c1
Cr-Commit-Position: refs/heads/master@{#396579}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 16 (4 generated)
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/patch-status/2017533002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2017533002/1
PTAL https://codereview.chromium.org/2017533002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/opentype/OpenTypeCapsSupportMPL.cpp (right): https://codereview.chromium.org/2017533002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/opentype/OpenTypeCapsSupportMPL.cpp:53: return false; This was not in your original suggestion. I guess I can return early here if we can't find GSUB for any script?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dry run: None
Ping eae@, behdad@ - thanks for taking a look.
lgtm after addressing comment. Thanks! https://codereview.chromium.org/2017533002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/opentype/OpenTypeCapsSupportMPL.cpp (right): https://codereview.chromium.org/2017533002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/opentype/OpenTypeCapsSupportMPL.cpp:53: return false; On 2016/05/26 08:14:05, drott wrote: > This was not in your original suggestion. I guess I can return early here if we > can't find GSUB for any script? No no. The return value means something else. It will be false if default script was found... Don't prematurely optimize; the cost if there's no GSUB is unmeasurable.
LGTM
The CQ bit was checked by eae@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2017533002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2017533002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Simplify OpenType caps feature detection logic We can rely on HarfBuzz API for finding the required OpenType features, instead of implementing our own find loops. Thanks to Behdad for the suggestion. TEST=LayoutTest/fast/text/font-features/* BUG=603137 R=behdad,eae ========== to ========== Simplify OpenType caps feature detection logic We can rely on HarfBuzz API for finding the required OpenType features, instead of implementing our own find loops. Thanks to Behdad for the suggestion. TEST=LayoutTest/fast/text/font-features/* BUG=603137 R=behdad,eae Committed: https://crrev.com/c51ee10d5afd2a657ed357cda9caf31c788790c1 Cr-Commit-Position: refs/heads/master@{#396579} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/c51ee10d5afd2a657ed357cda9caf31c788790c1 Cr-Commit-Position: refs/heads/master@{#396579}
Message was sent while issue was closed.
drott, can you please address my comment in a separate commit? Thanks.
Message was sent while issue was closed.
On 2016/05/27 at 17:33:29, behdad wrote: > lgtm after addressing comment. Thanks! > > https://codereview.chromium.org/2017533002/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/fonts/opentype/OpenTypeCapsSupportMPL.cpp (right): > > https://codereview.chromium.org/2017533002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/fonts/opentype/OpenTypeCapsSupportMPL.cpp:53: return false; > On 2016/05/26 08:14:05, drott wrote: > > This was not in your original suggestion. I guess I can return early here if we > > can't find GSUB for any script? > > No no. The return value means something else. It will be false if default script was found... Don't prematurely optimize; the cost if there's no GSUB is unmeasurable. Thanks for the review, addressed in: https://codereview.chromium.org/2026433002 I did not mean to optimize here, but not just ignore a return value of non-void function. And the meaning of the return values is not documented, that's why I was asking. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
