[RenderText] added test for AppendText

[RenderText] added unit test for AppendText.

BUG=469362
Committed: https://crrev.com/010e98a82c8be97665adea37003db928ac688235
Cr-Commit-Position: refs/heads/master@{#322190}

[dschuyler, 2015-03-24 00:11:10]

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

[msw, 2015-03-24 01:47:16]

https://codereview.chromium.org/1024543006/diff/1/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/1024543006/diff/1/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:618: FRIEND_TEST_ALL_PREFIXES(RenderTextTest, AppendText);
Does this test actually use anything protected or private? Need it be a friend?

https://codereview.chromium.org/1024543006/diff/1/ui/gfx/render_text_unittest.cc
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/1024543006/diff/1/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:219: TEST_F(RenderTextTest, AppendText) {
Move this test below ApplyStyles.

https://codereview.chromium.org/1024543006/diff/1/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:223: render_text->SetColor(color);
This test should also ensure ranged values from Apply[Color|Style|BaselineStyle]
aren't lost on AppendText. As is, this test doesn't ensure anything beyond the
SetStyles test, and would pass even if ranged styles were cleared on AppendText.

[dschuyler, 2015-03-24 19:37:08]

https://codereview.chromium.org/1024543006/diff/1/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/1024543006/diff/1/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:618: FRIEND_TEST_ALL_PREFIXES(RenderTextTest, AppendText);
On 2015/03/24 01:47:16, msw wrote:
> Does this test actually use anything protected or private? Need it be a
friend?

Yeah, the style arrays for the expect calls.

https://codereview.chromium.org/1024543006/diff/1/ui/gfx/render_text_unittest.cc
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/1024543006/diff/1/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:219: TEST_F(RenderTextTest, AppendText) {
On 2015/03/24 01:47:16, msw wrote:
> Move this test below ApplyStyles.

Done.

[msw, 2015-03-24 20:06:03]

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

https://codereview.chromium.org/1024543006/diff/40001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:376: expected_color.push_back(std::pair<size_t,
SkColor>(0, color));
None of this makes sense... Call ApplyStyle / ApplyColor / ApplyBaseLineStyle...

This test would be a lot simpler if you removed the loop and just did:
  // Set some text with ranged styles, colors, and baseline styles.
  set text ("abc")
  apply style (underline, true, 1-2) ... etc ...
  vector expected (0=false, 1=true, 2=false) ... etc ...
  expect style[underline]==expected ... etc ...
  // Append some text and ensure that ranged styles are not lost.
  append text ("def")
  expect text=="abcdef"
  expect style[underline]==expected .... etc ...

[dschuyler, 2015-03-24 20:54:08]

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

https://codereview.chromium.org/1024543006/diff/40001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:376: expected_color.push_back(std::pair<size_t,
SkColor>(0, color));
On 2015/03/24 20:06:03, msw wrote:
> None of this makes sense... Call ApplyStyle / ApplyColor /
ApplyBaseLineStyle...
> 
> This test would be a lot simpler if you removed the loop and just did:
>   // Set some text with ranged styles, colors, and baseline styles.
>   set text ("abc")
>   apply style (underline, true, 1-2) ... etc ...
>   vector expected (0=false, 1=true, 2=false) ... etc ...
>   expect style[underline]==expected ... etc ...
>   // Append some text and ensure that ranged styles are not lost.
>   append text ("def")
>   expect text=="abcdef"
>   expect style[underline]==expected .... etc ...

Done.

[msw, 2015-03-24 22:20:59]

Good, just some nits left.

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

https://codereview.chromium.org/1024543006/diff/60001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:355: TEST_F(RenderTextTest, AppendText) {
nit: name AppendTextKeepsStyles

https://codereview.chromium.org/1024543006/diff/60001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:356: // Ensure AppendText maintains current text
styles.
nit: move this comment down to replace the comment at line 383.

https://codereview.chromium.org/1024543006/diff/60001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:360: const SkColor color = SK_ColorRED;
nit: just inline SK_ColorRED here and line 367.

https://codereview.chromium.org/1024543006/diff/60001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:361: render_text->ApplyColor(color, Range(1, 2));
nit: make the text red over range 0-1 (for more coverage).

https://codereview.chromium.org/1024543006/diff/60001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:363: render_text->ApplyStyle(UNDERLINE, true,
Range(1, 2));
nit: apply underline over range 2-3 (for more coverage).

[dschuyler, 2015-03-24 22:38:13]

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

https://codereview.chromium.org/1024543006/diff/60001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:355: TEST_F(RenderTextTest, AppendText) {
On 2015/03/24 22:20:59, msw wrote:
> nit: name AppendTextKeepsStyles

Done.

https://codereview.chromium.org/1024543006/diff/60001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:356: // Ensure AppendText maintains current text
styles.
On 2015/03/24 22:20:59, msw wrote:
> nit: move this comment down to replace the comment at line 383.

Done.

https://codereview.chromium.org/1024543006/diff/60001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:360: const SkColor color = SK_ColorRED;
On 2015/03/24 22:20:59, msw wrote:
> nit: just inline SK_ColorRED here and line 367.

Done.

https://codereview.chromium.org/1024543006/diff/60001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:361: render_text->ApplyColor(color, Range(1, 2));
On 2015/03/24 22:20:59, msw wrote:
> nit: make the text red over range 0-1 (for more coverage).

Done.

https://codereview.chromium.org/1024543006/diff/60001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:363: render_text->ApplyStyle(UNDERLINE, true,
Range(1, 2));
On 2015/03/24 22:20:59, msw wrote:
> nit: apply underline over range 2-3 (for more coverage).

Done.

[msw, 2015-03-24 22:48:09]

lgtm

[dschuyler, 2015-03-25 17:13:20]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2015-03-25 17:13:58]

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

[commit-bot: I haz the power, 2015-03-25 17:50:18]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-25 17:51:53]

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