Use pango underline metrics in RenderTextLinux.

Use pango underline metrics in RenderTextLinux.

Also, changes type of SkiaTextRenderer::SetTextSize() to SkScalar to match the underlying Skia type, which is needed for future RenderTextMac implementation.

BUG=126506, 125664, 105550
TEST=Eyeball underlined text in views_examples' text style example.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141772

[msw, 2012-06-06 01:12:51]

http://codereview.chromium.org/10520017/diff/12001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/10520017/diff/12001/ui/gfx/render_text.cc#newc...
ui/gfx/render_text.cc:183: const SkScalar kUnderlineMetricsNotSet = -1.0f;
nit: technically this applies to a single metric value, not both. Maybe use
singular 'metric' terminology: kUnderlineMetricNotSet / "[the|an] underline
metric has".

http://codereview.chromium.org/10520017/diff/12001/ui/gfx/render_text.cc#newc...
ui/gfx/render_text.cc:318: if (underline_position_ == kUnderlineMetricsNotSet) {
Take the 'not set' path if either value isn't set (also consider checking < 0
instead?).

http://codereview.chromium.org/10520017/diff/12001/ui/gfx/render_text_linux.cc
File ui/gfx/render_text_linux.cc (right):

http://codereview.chromium.org/10520017/diff/12001/ui/gfx/render_text_linux.c...
ui/gfx/render_text_linux.cc:77: const int underline_thickness =
nit: nix "underline_" from the names to fit each on one line.

[Alexei Svitkine (slow), 2012-06-06 14:46:27]

I've addressed your comments and also fixes a few other problems with the
previous patch, including:

- Using pango_quantize_line_geometry() to make sure the line has pixel
boundaries (and thus doesn't look blurry).
- Correctly negating |position| to get a y-offset from Pango's "position above
the baseline".
- Making sure to add the underline_position_ to the y coordinate instead of
using it directly.

(Wow that was an embarrassing amount of issues with the previous patch!)

http://codereview.chromium.org/10520017/diff/12001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/10520017/diff/12001/ui/gfx/render_text.cc#newc...
ui/gfx/render_text.cc:183: const SkScalar kUnderlineMetricsNotSet = -1.0f;
On 2012/06/06 01:12:51, msw wrote:
> nit: technically this applies to a single metric value, not both. Maybe use
> singular 'metric' terminology: kUnderlineMetricNotSet / "[the|an] underline
> metric has".

Done.

I've also changed the value to be SK_ScalarNaN, which makes more sense for this
special value.

http://codereview.chromium.org/10520017/diff/12001/ui/gfx/render_text.cc#newc...
ui/gfx/render_text.cc:318: if (underline_position_ == kUnderlineMetricsNotSet) {
On 2012/06/06 01:12:51, msw wrote:
> Take the 'not set' path if either value isn't set (also consider checking < 0
> instead?).

Made the two fields independent - so that none, one, or both could have value
kUnderlineMetricNotSet.

http://codereview.chromium.org/10520017/diff/12001/ui/gfx/render_text_linux.cc
File ui/gfx/render_text_linux.cc (right):

http://codereview.chromium.org/10520017/diff/12001/ui/gfx/render_text_linux.c...
ui/gfx/render_text_linux.cc:77: const int underline_thickness =
On 2012/06/06 01:12:51, msw wrote:
> nit: nix "underline_" from the names to fit each on one line.

Done.

[msw, 2012-06-06 18:35:25]

Thanks for be so thorough and checking/fixing these issues before landing!

http://codereview.chromium.org/10520017/diff/8004/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/10520017/diff/8004/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:183: const SkScalar kUnderlineMetricNotSet = SK_ScalarNaN;
optional nit: just using SK_ScalarNaN directly it would be sufficient.

http://codereview.chromium.org/10520017/diff/8004/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:264: printf("Set to %f %f\n", underline_thickness_,
underline_position_);
oops :) please remove

http://codereview.chromium.org/10520017/diff/8004/ui/gfx/render_text_linux.cc
File ui/gfx/render_text_linux.cc (right):

http://codereview.chromium.org/10520017/diff/8004/ui/gfx/render_text_linux.cc...
ui/gfx/render_text_linux.cc:81: pango_quantize_line_geometry(&thickness,
&position);
Does this work ok with DIP / high dpi?

http://codereview.chromium.org/10520017/diff/8004/ui/gfx/render_text_linux.cc...
ui/gfx/render_text_linux.cc:82:
renderer->SetUnderlineMetrics(PANGO_PIXELS(thickness),
Should these still use pango_units_to_double as a matter of good practice to
avoid them incurring any bad rounding?

[Alexei Svitkine (slow), 2012-06-06 18:46:23]

http://codereview.chromium.org/10520017/diff/8004/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/10520017/diff/8004/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:183: const SkScalar kUnderlineMetricNotSet = SK_ScalarNaN;
On 2012/06/06 18:35:25, msw wrote:
> optional nit: just using SK_ScalarNaN directly it would be sufficient.

Done.

http://codereview.chromium.org/10520017/diff/8004/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:264: printf("Set to %f %f\n", underline_thickness_,
underline_position_);
On 2012/06/06 18:35:25, msw wrote:
> oops :) please remove

Done.

http://codereview.chromium.org/10520017/diff/8004/ui/gfx/render_text_linux.cc
File ui/gfx/render_text_linux.cc (right):

http://codereview.chromium.org/10520017/diff/8004/ui/gfx/render_text_linux.cc...
ui/gfx/render_text_linux.cc:81: pango_quantize_line_geometry(&thickness,
&position);
On 2012/06/06 18:35:25, msw wrote:
> Does this work ok with DIP / high dpi?

It shouldn't cause any problems - though it will still result in the line
landing on pixel boundaries. Maybe in HighDPI mode, it's not such a big deal if
it wouldn't land on pixel boundaries (since pixels are small so blurred pixels
don't look as bad), but I don't think restricting to pixel boundaries would
causes any problems.

(It may be the case that we'll want to support non-pixel boundaries for the
underline in high DPI mode, but let's cross that bridge when we see evidence of
demand for that.)

http://codereview.chromium.org/10520017/diff/8004/ui/gfx/render_text_linux.cc...
ui/gfx/render_text_linux.cc:82:
renderer->SetUnderlineMetrics(PANGO_PIXELS(thickness),
On 2012/06/06 18:35:25, msw wrote:
> Should these still use pango_units_to_double as a matter of good practice to
> avoid them incurring any bad rounding?

Since pango_quantize_line_geometry() guarantees pixel boundaries, there won't be
any bad rounding here (since the units will be evenly divisible).

[msw, 2012-06-06 18:51:52]

Seems like we ought to use pango_units_to_double anyway, but LGTM either way.

http://codereview.chromium.org/10520017/diff/8004/ui/gfx/render_text_linux.cc
File ui/gfx/render_text_linux.cc (right):

http://codereview.chromium.org/10520017/diff/8004/ui/gfx/render_text_linux.cc...
ui/gfx/render_text_linux.cc:82:
renderer->SetUnderlineMetrics(PANGO_PIXELS(thickness),
On 2012/06/06 18:46:23, Alexei Svitkine wrote:
> On 2012/06/06 18:35:25, msw wrote:
> > Should these still use pango_units_to_double as a matter of good practice to
> > avoid them incurring any bad rounding?
> 
> Since pango_quantize_line_geometry() guarantees pixel boundaries, there won't
be
> any bad rounding here (since the units will be evenly divisible).

I guess my point was why not use pango_units_to_double? What if a later change
[re]moves the pango_quantize_line_geometry call?

[Alexei Svitkine (slow), 2012-06-06 19:03:05]

http://codereview.chromium.org/10520017/diff/8004/ui/gfx/render_text_linux.cc
File ui/gfx/render_text_linux.cc (right):

http://codereview.chromium.org/10520017/diff/8004/ui/gfx/render_text_linux.cc...
ui/gfx/render_text_linux.cc:82:
renderer->SetUnderlineMetrics(PANGO_PIXELS(thickness),
On 2012/06/06 18:51:52, msw wrote:
> On 2012/06/06 18:46:23, Alexei Svitkine wrote:
> > On 2012/06/06 18:35:25, msw wrote:
> > > Should these still use pango_units_to_double as a matter of good practice
to
> > > avoid them incurring any bad rounding?
> > 
> > Since pango_quantize_line_geometry() guarantees pixel boundaries, there
won't
> be
> > any bad rounding here (since the units will be evenly divisible).
> 
> I guess my point was why not use pango_units_to_double? What if a later change
> [re]moves the pango_quantize_line_geometry call?

I've added a comment about it.

[commit-bot: I haz the power, 2012-06-06 19:03:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10520017/14005

[commit-bot: I haz the power, 2012-06-06 19:23:24]

Try job failure for 10520017-14005 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[Alexei Svitkine (slow), 2012-06-06 19:29:13]

Hmm, looks like there's a problem compiling code that uses SK_ScalarNaN on
Windows...

[reed1, 2012-06-12 19:26:58]

x == NaN;

...

if (x == NaN) ...

This check will always be false I think. If x is not a nan, it will be false. If
x is nan, then the nan-rule that all compares return false kicks in, so it will
be false again.

Could you init, instead, your thickness to -1 (which is never meaningful for a
thickness) and use that as a sentinel instead? Since you always assign both
offset and thickness in your setter, you may not need a 2nd sentinel value for
offset...

[Alexei Svitkine (slow), 2012-06-12 19:52:58]

Updated per Mike Reed's suggestion.

The CL now looks similar to my original version.

[msw, 2012-06-12 20:44:39]

http://codereview.chromium.org/10520017/diff/18005/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/10520017/diff/18005/ui/gfx/render_text.cc#newc...
ui/gfx/render_text.cc:190: underline_position_(0.0f) {
You should also init this to kUnderlineMetricsNotSet.
It's better to be explicit with its value.

http://codereview.chromium.org/10520017/diff/18005/ui/gfx/render_text.cc#newc...
ui/gfx/render_text.cc:323: r.fTop = y + underline_position_;
You should probably DCHECK that underline_position_ is not
kUnderlineMetricsNotSet. I know they're set together now, but that may not
always be the case...

[reed1, 2012-06-12 20:46:39]

lgtm

[Alexei Svitkine (slow), 2012-06-12 20:47:43]

http://codereview.chromium.org/10520017/diff/18005/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/10520017/diff/18005/ui/gfx/render_text.cc#newc...
ui/gfx/render_text.cc:323: r.fTop = y + underline_position_;
On 2012/06/12 20:44:39, msw wrote:
> You should probably DCHECK that underline_position_ is not
> kUnderlineMetricsNotSet. I know they're set together now, but that may not
> always be the case...

-1.0 may be a valid position offset, but definitely never a valid thickness.

[msw, 2012-06-12 21:28:11]

LGTM

http://codereview.chromium.org/10520017/diff/18005/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/10520017/diff/18005/ui/gfx/render_text.cc#newc...
ui/gfx/render_text.cc:323: r.fTop = y + underline_position_;
> -1.0 may be a valid position offset, but definitely never a valid thickness.

Fair enough, then it might be worthwhile to add a comment at the declaration /
setter that the two must be set together, etc.

[commit-bot: I haz the power, 2012-06-12 21:31:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10520017/21002

[commit-bot: I haz the power, 2012-06-12 22:59:42]

Change committed as 141772