RenderText: Add a test to ensure strings fit their own widths

RenderText: Add a test to ensure strings fit their own widths

This CL tests against regressions of http://crbug.com/402347

BUG=402347
Committed: https://crrev.com/28da9ffbd9e39edb8fd707ec6129343a74e3816b
Cr-Commit-Position: refs/heads/master@{#299997}

[ckocagil, 2014-10-12 16:54:02]

ckocagil@chromium.org changed reviewers:
- msw@chromium.org

[ckocagil, 2014-10-15 01:07:45]

ckocagil@chromium.org changed reviewers:
+ derat@chromium.org

[ckocagil, 2014-10-15 01:12:08]

Can you test this CL on CrOS to make sure it passes with and fails without your
https://codereview.chromium.org/631363002/ ? Thanks.

[Daniel Erat, 2014-10-15 15:54:24]

i think that you can verify this by modifying
IsBrowserTextSubpixelPositioningEnabled() in ui/gfx/font_render_params_linux.cc
to always return true and then doing a linux build.

when i do this, StringFitsOwnWidth fails (along with a bunch of tests that i
think were already failing when subpixel positioning is enabled).

when i restore my static_cast<int>s from
https://codereview.chromium.org/631363002/, StringFitsOwnWidth passes.

so the new test looks like it's testing that successfully, but it won't be able
to catch regressions because the problem only existed when subpixel positioning
was enabled, which i don't think we currently test (at least on linux and chrome
os).

[ckocagil, 2014-10-15 16:20:39]

On 2014/10/15 15:54:24, Daniel Erat wrote:
> i think that you can verify this by modifying
> IsBrowserTextSubpixelPositioningEnabled() in
ui/gfx/font_render_params_linux.cc
> to always return true and then doing a linux build.
> 
> when i do this, StringFitsOwnWidth fails (along with a bunch of tests that i
> think were already failing when subpixel positioning is enabled).
> 
> when i restore my static_cast<int>s from
> https://codereview.chromium.org/631363002/, StringFitsOwnWidth passes.
> 
> so the new test looks like it's testing that successfully, but it won't be
able
> to catch regressions because the problem only existed when subpixel
positioning
> was enabled, which i don't think we currently test (at least on linux and
chrome
> os).

Maybe it won't regress in the same way. But since we have code that depends on
the fact that strings fit their own  GetContentWidth(), I'd still prefer testing
this regardless of subpixel positioning.

[Daniel Erat, 2014-10-15 16:25:17]

On 2014/10/15 16:20:39, ckocagil wrote:
> On 2014/10/15 15:54:24, Daniel Erat wrote:
> > i think that you can verify this by modifying
> > IsBrowserTextSubpixelPositioningEnabled() in
> ui/gfx/font_render_params_linux.cc
> > to always return true and then doing a linux build.
> > 
> > when i do this, StringFitsOwnWidth fails (along with a bunch of tests that i
> > think were already failing when subpixel positioning is enabled).
> > 
> > when i restore my static_cast<int>s from
> > https://codereview.chromium.org/631363002/, StringFitsOwnWidth passes.
> > 
> > so the new test looks like it's testing that successfully, but it won't be
> able
> > to catch regressions because the problem only existed when subpixel
> positioning
> > was enabled, which i don't think we currently test (at least on linux and
> chrome
> > os).
> 
> Maybe it won't regress in the same way. But since we have code that depends on
> the fact that strings fit their own  GetContentWidth(), I'd still prefer
testing
> this regardless of subpixel positioning.

sure, i'm definitely fine with this going in (as long as you revert the removal
of those casts and the enabling-by-default of RTHB). :-) was just pointing out
that we probably want to find a way to also test the subpixel positioning path
at some point.

[ckocagil, 2014-10-16 00:24:10]

ckocagil@chromium.org changed reviewers:
+ msw@chromium.org

[ckocagil, 2014-10-16 00:34:56]

+msw: PTAL

[msw, 2014-10-16 20:32:21]

lgtm with a nit

https://codereview.chromium.org/652573002/diff/150001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/652573002/diff/150001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2321: EXPECT_EQ(base::char16('m'),
*(render_text->GetLayoutText().end() - 1));
nit: Just compare the whole string (avoid accidental ELIDE_HEAD, etc.)

[ckocagil, 2014-10-16 20:40:20]

https://codereview.chromium.org/652573002/diff/150001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/652573002/diff/150001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2321: EXPECT_EQ(base::char16('m'),
*(render_text->GetLayoutText().end() - 1));
On 2014/10/16 20:32:21, msw wrote:
> nit: Just compare the whole string (avoid accidental ELIDE_HEAD, etc.)

Done.

[ckocagil, 2014-10-16 20:40:24]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-10-16 20:42:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/652573002/170001

[commit-bot: I haz the power, 2014-10-16 22:49:19]

Committed patchset #5 (id:170001)

[commit-bot: I haz the power, 2014-10-16 22:49:51]

Patchset 5 (id:??) landed as
https://crrev.com/28da9ffbd9e39edb8fd707ec6129343a74e3816b
Cr-Commit-Position: refs/heads/master@{#299997}