Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2969623004:
RenderText: Allow strike-through line thickness to be customized. (Closed)
DescriptionRenderText: Allow strike-through line thickness to be customized.
This CL plumbs in the ability to override the default strike-through
line thickness used by RenderText. Along the way:
- Remove unused underscore thickness configuration hooks.
- Remove unncessary DrawDecorations wrapper.
BUG=735759
Review-Url: https://codereview.chromium.org/2969623004
Cr-Commit-Position: refs/heads/master@{#484580}
Committed: https://chromium.googlesource.com/chromium/src/+/6c111c024ce31e4de508032dc8b7a18564007ed5
Patch Set 1 #
Total comments: 23
Patch Set 2 : Address comments; fix fraction. #Patch Set 3 : Reincorporate URL bar unittest improvements. #
Total comments: 12
Patch Set 4 : Address final nits. #
Messages
Total messages: 29 (16 generated)
The CQ bit was checked by cjgrant@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...
cjgrant@chromium.org changed reviewers: + msw@chromium.org
Mike, please see my comments in-line. There are some open questions about the best way to achieve what I'm trying to do here. Thanks! https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc File ui/gfx/render_text.cc (left): https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc#oldco... ui/gfx/render_text.cc:193: const SkScalar kUnderlineMetricsNotSet = -1.0f; I don't see that underscore overrides are actually used anywhere. If they are, I need to figure out how to get codesearch to show that. https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:57: // Fraction of the text size to lower a strike through below the baseline. Should this be "raise the strike through above the baseline" and just adjust the math to not need a negative in the constant? Also, if my changes are okay, I can also document that this value is the center of the strike-through line rather than the upper edge. https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:774: renderer.SetStrikeThicknessFactor(strike_thickness_factor_); I must be doing this wrong, but I don't see a better approach. What's the best way to feed a RenderText setting through to SkiaTextRenderer?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This approach seems fine to me, sorry for not responding to your earlier email, I'm still catching up from extended vacation. Let me know if I missed something. https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc File ui/gfx/render_text.cc (left): https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc#oldco... ui/gfx/render_text.cc:193: const SkScalar kUnderlineMetricsNotSet = -1.0f; On 2017/06/30 19:04:25, cjgrant wrote: > I don't see that underscore overrides are actually used anywhere. If they are, > I need to figure out how to get codesearch to show that. Afaict, you're right; thanks for removing this unused code. https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:57: // Fraction of the text size to lower a strike through below the baseline. On 2017/06/30 19:04:25, cjgrant wrote: > Should this be "raise the strike through above the baseline" and just adjust the > math to not need a negative in the constant? > > Also, if my changes are okay, I can also document that this value is the center > of the strike-through line rather than the upper edge. That seems fine to me, just make sure you don't accidentally shift the underline position by its thickness in the calculation on line 257. aside: you can also hyphenate "strike-through" here (and elsewhere) if that's your preference. https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:61: // Default fraction of the text size to use for a strike through or under-line. optional nit: "underline" here to match most uses elsewhere. https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:62: const SkScalar kLineThickness = (SK_Scalar1 / 18); optional nit: kLineThicknessFactor https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:774: renderer.SetStrikeThicknessFactor(strike_thickness_factor_); On 2017/06/30 19:04:25, cjgrant wrote: > I must be doing this wrong, but I don't see a better approach. What's the best > way to feed a RenderText setting through to SkiaTextRenderer? I would suggest removing SkiaTextRenderer::SetStrikeThicknessFactor *and* its |strike_thickness_factor_| member, instead just pass the RenderText member's value into the SkiaTextRenderer::DrawStrike functions. You'll need to plumb that value through SkiaTextRenderer::DrawDecorations too; or my preference would be to eliminate that helper and just inline its simple logic at the callsites. Otherwise, this might be acceptable. Maybe asvitkine@ would have alternate suggestions as the designer of SkiaTextRenderer, but he also doesn't actively work in this area anymore. https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:972: void RenderText::SetStrikeThicknessFactor(SkScalar factor) { nit: this could be a simple setter defined in the header: void set_strike_thickness_factor(SkScalar f) { strike_thickness_factor_ = f; } (ditto for the SkiaTextRenderer function, if you keep that around) https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.h#newcode89 ui/gfx/render_text.h:89: SkScalar underline_thickness_; Please also remove these two underline_* customization values. https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:840: SkScalar strike_thickness_factor_; nit: comment like "The ratio of strike-through line thickness to text height." (I realize it's redundant with the setter, but I think it's more helpful here)
https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:58: const SkScalar kStrikeThroughOffset = (-SK_Scalar1 * 29 / 126); Forgot to say, this relatively nasty-looking constant is simply (6/21 - 1/9), to ensure I'm not moving the existing line when changing to a center-based offset. https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:774: renderer.SetStrikeThicknessFactor(strike_thickness_factor_); On 2017/06/30 20:51:23, msw wrote: > On 2017/06/30 19:04:25, cjgrant wrote: > > I must be doing this wrong, but I don't see a better approach. What's the > best > > way to feed a RenderText setting through to SkiaTextRenderer? > > I would suggest removing SkiaTextRenderer::SetStrikeThicknessFactor *and* its > |strike_thickness_factor_| member, instead just pass the RenderText member's > value into the SkiaTextRenderer::DrawStrike functions. You'll need to plumb that > value through SkiaTextRenderer::DrawDecorations too; or my preference would be > to eliminate that helper and just inline its simple logic at the callsites. > Otherwise, this might be acceptable. Maybe asvitkine@ would have alternate > suggestions as the designer of SkiaTextRenderer, but he also doesn't actively > work in this area anymore. I'd thought about plumbing through the call path, but doesn't that mean supplying the value as an arg to DrawVisualText, through both the mac and harfbuzz implementations, and down to the renderer? I feel like we should just configure the renderer here to save all the hops. It's just hard for me to fathom that this would be the first such setting that would warrant such treatment. I could also make a SkiaTextRendererParameters struct/class, housing only this one property, and pass it in... That'd look cleaner, and could accommodate other properties.
https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:58: const SkScalar kStrikeThroughOffset = (-SK_Scalar1 * 29 / 126); On 2017/06/30 21:06:24, cjgrant wrote: > Forgot to say, this relatively nasty-looking constant is simply (6/21 - 1/9), to > ensure I'm not moving the existing line when changing to a center-based offset. Acknowledged. https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:774: renderer.SetStrikeThicknessFactor(strike_thickness_factor_); On 2017/06/30 21:06:24, cjgrant wrote: > On 2017/06/30 20:51:23, msw wrote: > > On 2017/06/30 19:04:25, cjgrant wrote: > > > I must be doing this wrong, but I don't see a better approach. What's the > > best > > > way to feed a RenderText setting through to SkiaTextRenderer? > > > > I would suggest removing SkiaTextRenderer::SetStrikeThicknessFactor *and* its > > |strike_thickness_factor_| member, instead just pass the RenderText member's > > value into the SkiaTextRenderer::DrawStrike functions. You'll need to plumb > that > > value through SkiaTextRenderer::DrawDecorations too; or my preference would be > > to eliminate that helper and just inline its simple logic at the callsites. > > Otherwise, this might be acceptable. Maybe asvitkine@ would have alternate > > suggestions as the designer of SkiaTextRenderer, but he also doesn't actively > > work in this area anymore. > > I'd thought about plumbing through the call path, but doesn't that mean > supplying the value as an arg to DrawVisualText, through both the mac and > harfbuzz implementations, and down to the renderer? I feel like we should just > configure the renderer here to save all the hops. It's just hard for me to > fathom that this would be the first such setting that would warrant such > treatment. > > I could also make a SkiaTextRendererParameters struct/class, housing only this > one property, and pass it in... That'd look cleaner, and could accommodate > other properties. You could add a simple accessor |RenderText::strike_thickness_factor() const { return strike_thickness_factor_; }| that Mac/Harfbuzz subclasses could use, that'd probably be my top preference (it seems more natural as a simple property of the strike/underline, like the dimensions passed to DrawDecorations, not some renderer configuration value). Otherwise, I prefer what you have now over creating a params struct with one value.
Description was changed from ========== RenderText: Allow strike-through line thickness to be customized. This CL plumbs in the ability to override the default strike-through line thickness used by Skia. Along the way, it removes previous hooks to tune underscore thickness, which don't appear used anymore. BUG=735759 ========== to ========== RenderText: Allow strike-through line thickness to be customized. This CL plumbs in the ability to override the default strike-through line thickness used by RenderText. Along the way: - Remove unused underscore thickness configuration hooks. - Remove unncessary DrawDecorations wrapper. BUG=735759 ==========
https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:57: // Fraction of the text size to lower a strike through below the baseline. On 2017/06/30 20:51:23, msw wrote: > On 2017/06/30 19:04:25, cjgrant wrote: > > Should this be "raise the strike through above the baseline" and just adjust > the > > math to not need a negative in the constant? > > > > Also, if my changes are okay, I can also document that this value is the > center > > of the strike-through line rather than the upper edge. > > That seems fine to me, just make sure you don't accidentally shift the underline > position by its thickness in the calculation on line 257. aside: you can also > hyphenate "strike-through" here (and elsewhere) if that's your preference. I did some underline and strike before/after tests to make sure I'm not changing the default positions/sizes of the lines. https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:58: const SkScalar kStrikeThroughOffset = (-SK_Scalar1 * 29 / 126); On 2017/06/30 23:01:32, msw wrote: > On 2017/06/30 21:06:24, cjgrant wrote: > > Forgot to say, this relatively nasty-looking constant is simply (6/21 - 1/9), > to > > ensure I'm not moving the existing line when changing to a center-based > offset. > > Acknowledged. And, it's now actually correct. :| https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:61: // Default fraction of the text size to use for a strike through or under-line. On 2017/06/30 20:51:23, msw wrote: > optional nit: "underline" here to match most uses elsewhere. Done. https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:62: const SkScalar kLineThickness = (SK_Scalar1 / 18); On 2017/06/30 20:51:23, msw wrote: > optional nit: kLineThicknessFactor Done. https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:774: renderer.SetStrikeThicknessFactor(strike_thickness_factor_); On 2017/06/30 23:01:32, msw wrote: > On 2017/06/30 21:06:24, cjgrant wrote: > > On 2017/06/30 20:51:23, msw wrote: > > > On 2017/06/30 19:04:25, cjgrant wrote: > > > > I must be doing this wrong, but I don't see a better approach. What's the > > > best > > > > way to feed a RenderText setting through to SkiaTextRenderer? > > > > > > I would suggest removing SkiaTextRenderer::SetStrikeThicknessFactor *and* > its > > > |strike_thickness_factor_| member, instead just pass the RenderText member's > > > value into the SkiaTextRenderer::DrawStrike functions. You'll need to plumb > > that > > > value through SkiaTextRenderer::DrawDecorations too; or my preference would > be > > > to eliminate that helper and just inline its simple logic at the callsites. > > > Otherwise, this might be acceptable. Maybe asvitkine@ would have alternate > > > suggestions as the designer of SkiaTextRenderer, but he also doesn't > actively > > > work in this area anymore. > > > > I'd thought about plumbing through the call path, but doesn't that mean > > supplying the value as an arg to DrawVisualText, through both the mac and > > harfbuzz implementations, and down to the renderer? I feel like we should > just > > configure the renderer here to save all the hops. It's just hard for me to > > fathom that this would be the first such setting that would warrant such > > treatment. > > > > I could also make a SkiaTextRendererParameters struct/class, housing only this > > one property, and pass it in... That'd look cleaner, and could accommodate > > other properties. > > You could add a simple accessor |RenderText::strike_thickness_factor() const { > return strike_thickness_factor_; }| that Mac/Harfbuzz subclasses could use, > that'd probably be my top preference (it seems more natural as a simple property > of the strike/underline, like the dimensions passed to DrawDecorations, not some > renderer configuration value). Otherwise, I prefer what you have now over > creating a params struct with one value. I did your top-preference, and removed the DrawDecorations wrapper as well. https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:972: void RenderText::SetStrikeThicknessFactor(SkScalar factor) { On 2017/06/30 20:51:23, msw wrote: > nit: this could be a simple setter defined in the header: > void set_strike_thickness_factor(SkScalar f) { strike_thickness_factor_ = f; } > (ditto for the SkiaTextRenderer function, if you keep that around) Done. https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.h#newcode89 ui/gfx/render_text.h:89: SkScalar underline_thickness_; On 2017/06/30 20:51:23, msw wrote: > Please also remove these two underline_* customization values. Done. https://codereview.chromium.org/2969623004/diff/1/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:840: SkScalar strike_thickness_factor_; On 2017/06/30 20:51:23, msw wrote: > nit: comment like "The ratio of strike-through line thickness to text height." > (I realize it's redundant with the setter, but I think it's more helpful here) Done. Agreed. https://codereview.chromium.org/2969623004/diff/40001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (left): https://codereview.chromium.org/2969623004/diff/40001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:383: This was also unused I believe.
The CQ bit was checked by cjgrant@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.
lgtm with minor comments; thanks! https://codereview.chromium.org/2969623004/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/textures/render_text_wrapper.cc (right): https://codereview.chromium.org/2969623004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/render_text_wrapper.cc:32: void RenderTextWrapper::set_strike_thickness_factor(SkScalar f) { Ah, as this is defined out-of-line with a 'non-trivial' body, this function should have a CamelCase name, SetStrikeThicknessFactor, and the |f| param can be renamed |factor| for clarity. https://codereview.chromium.org/2969623004/diff/40001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/2969623004/diff/40001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:489: // Sets the ratio of strike-through line thickness to text height. optional nit: comment not needed on simple setter. https://codereview.chromium.org/2969623004/diff/40001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:491: strike_thickness_factor_ = f; nit: fits on line above with semi removed: void set_strike_thickness_factor(SkScalar f) { strike_thickness_factor_ = f; } https://codereview.chromium.org/2969623004/diff/40001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:492: }; nit: no semi after function body https://codereview.chromium.org/2969623004/diff/40001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (left): https://codereview.chromium.org/2969623004/diff/40001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:383: On 2017/07/05 16:16:48, cjgrant wrote: > This was also unused I believe. Acknowledged; please also remove |decoration_log_| and |GetDecorationLogAndReset|
https://codereview.chromium.org/2969623004/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/textures/render_text_wrapper.cc (right): https://codereview.chromium.org/2969623004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/render_text_wrapper.cc:32: void RenderTextWrapper::set_strike_thickness_factor(SkScalar f) { On 2017/07/05 19:55:00, msw wrote: > Ah, as this is defined out-of-line with a 'non-trivial' body, this function > should have a CamelCase name, SetStrikeThicknessFactor, and the |f| param can be > renamed |factor| for clarity. Done. Heh, back to how it was. ;) https://codereview.chromium.org/2969623004/diff/40001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/2969623004/diff/40001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:489: // Sets the ratio of strike-through line thickness to text height. On 2017/07/05 19:55:00, msw wrote: > optional nit: comment not needed on simple setter. Done. I was concerned about explaining what the ratio means, but I assume the expected behavior is to look right at the member being set. https://codereview.chromium.org/2969623004/diff/40001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:491: strike_thickness_factor_ = f; On 2017/07/05 19:55:00, msw wrote: > nit: fits on line above with semi removed: > void set_strike_thickness_factor(SkScalar f) { strike_thickness_factor_ = f; } Done. https://codereview.chromium.org/2969623004/diff/40001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:492: }; On 2017/07/05 19:55:00, msw wrote: > nit: no semi after function body Done. Oops! https://codereview.chromium.org/2969623004/diff/40001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (left): https://codereview.chromium.org/2969623004/diff/40001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:383: On 2017/07/05 19:55:00, msw wrote: > On 2017/07/05 16:16:48, cjgrant wrote: > > This was also unused I believe. > > Acknowledged; please also remove |decoration_log_| and > |GetDecorationLogAndReset| Done (with the associated struct as well).
PS: Thanks for the quick reviews on this change! Very much appreciated...
The CQ bit was checked by cjgrant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2969623004/#ps60001 (title: "Address final nits.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2969623004/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/textures/render_text_wrapper.cc (right): https://codereview.chromium.org/2969623004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/render_text_wrapper.cc:32: void RenderTextWrapper::set_strike_thickness_factor(SkScalar f) { On 2017/07/05 20:44:43, cjgrant wrote: > On 2017/07/05 19:55:00, msw wrote: > > Ah, as this is defined out-of-line with a 'non-trivial' body, this function > > should have a CamelCase name, SetStrikeThicknessFactor, and the |f| param can > be > > renamed |factor| for clarity. > > Done. Heh, back to how it was. ;) Yeah, thanks for addressing my erratic comments, and sorry for churn.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by cjgrant@chromium.org
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": 1499351051169510,
"parent_rev": "adb6ffe9685ba87bd998582cd06600da17b69a27", "commit_rev":
"6c111c024ce31e4de508032dc8b7a18564007ed5"}
Message was sent while issue was closed.
Description was changed from ========== RenderText: Allow strike-through line thickness to be customized. This CL plumbs in the ability to override the default strike-through line thickness used by RenderText. Along the way: - Remove unused underscore thickness configuration hooks. - Remove unncessary DrawDecorations wrapper. BUG=735759 ========== to ========== RenderText: Allow strike-through line thickness to be customized. This CL plumbs in the ability to override the default strike-through line thickness used by RenderText. Along the way: - Remove unused underscore thickness configuration hooks. - Remove unncessary DrawDecorations wrapper. BUG=735759 Review-Url: https://codereview.chromium.org/2969623004 Cr-Commit-Position: refs/heads/master@{#484580} Committed: https://chromium.googlesource.com/chromium/src/+/6c111c024ce31e4de508032dc8b7... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/6c111c024ce31e4de508032dc8b7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
