[RenderText] Added append text

[RenderText] Added append text

RenderText previously only had a SetText to alter the text.  Calling SetText would reset the text styling already setup.

It can be more efficient in some cases to preserve the existing font styling and when only adding text to the end of the RenderText.

This patch adds AppendText that allows adding to an exiting RenderText without disturbing the existing styling.

This change is related to (and used by) CL 795933009.

BUG=

Committed: https://crrev.com/92bc0de851bc0c094bbd1e3ac5e6b24202011d69
Cr-Commit-Position: refs/heads/master@{#321507}

[dschuyler, 2015-03-18 01:29:32]

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

[msw, 2015-03-18 18:05:17]

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

[msw, 2015-03-18 18:05:18]

See my comments at https://codereview.chromium.org/795933009/ ... Not sure if
you intend to land this before that...

[dschuyler, 2015-03-18 19:25:11]

On 2015/03/18 18:05:18, msw wrote:
> See my comments at https://codereview.chromium.org/795933009/ ... Not sure if
> you intend to land this before that...

The goal of this CL was to reduce the code in the other CL.  If the larger CL
lands first, this CL can be deleted.
So it may help to land this one first.

[dschuyler, 2015-03-18 19:25:43]

merged review changes from CL 795933009

[dschuyler, 2015-03-18 22:51:54]

+CC jdonnelly and pkasting

[dschuyler, 2015-03-19 23:23:06]

Bringing over comment from CL 795933009:

	
msw 2015/03/18 20:20:16
Sorry, I changed my mind here a bit. I think it'd be more appropriate if this
function limited its behavior to just extending the breaklists, and call it
something like "UpdateStyleLengths". The callers should either assign or append
to |text_|, set |cached_bounds_and_offset_valid_| and |obscured_reveal_index_|,
and call OnTextAttributeChanged()... Otherwise, I fear that this function might
be misconstrued as a sufficient and self-contained mechanism to extend the text
content and leave the RenderText in a good state; it isn't.

Done in patch set 4.

[msw, 2015-03-20 00:24:11]

lgtm with comment nits.

https://codereview.chromium.org/1014163002/diff/60001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/1014163002/diff/60001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1248: // Adjust ranged styles and colors to accommodate a
new text length.
nit: remove this in favor of a comment in the header.

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

https://codereview.chromium.org/1014163002/diff/60001/ui/gfx/render_text.h#ne...
ui/gfx/render_text.h:607: void UpdateStyleLengths();
nit: move the comment in the function body out to be a function comment here:
  // Adjust ranged styles to accommodate a new text length.

[dschuyler, 2015-03-20 00:45:58]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2015-03-20 00:45:58]

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/1014163002/#ps80001
(title: "Moved comment for UpdateStyleLengths")

[dschuyler, 2015-03-20 00:46:17]

https://codereview.chromium.org/1014163002/diff/60001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/1014163002/diff/60001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1248: // Adjust ranged styles and colors to accommodate a
new text length.
On 2015/03/20 00:24:11, msw wrote:
> nit: remove this in favor of a comment in the header.

Done.

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

https://codereview.chromium.org/1014163002/diff/60001/ui/gfx/render_text.h#ne...
ui/gfx/render_text.h:607: void UpdateStyleLengths();
On 2015/03/20 00:24:11, msw wrote:
> nit: move the comment in the function body out to be a function comment here:
>   // Adjust ranged styles to accommodate a new text length.

Done.

[commit-bot: I haz the power, 2015-03-20 00:46:21]

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

[msw, 2015-03-20 01:10:32]

Still lgtm, but I hoped you'd update the comment to what I suggested.

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

https://codereview.chromium.org/1014163002/diff/80001/ui/gfx/render_text.h#ne...
ui/gfx/render_text.h:607: // Adjust ranged styles and colors to accommodate a
new text length.
nit: nix "and colors"

[chromium-reviews, 2015-03-20 01:13:15]

Whoops, I'll do a separate CL

On Thu, Mar 19, 2015 at 6:10 PM, <msw@chromium.org> wrote:

> Still lgtm, but I hoped you'd update the comment to what I suggested.
>
>
> https://codereview.chromium.org/1014163002/diff/80001/ui/gfx/render_text.h
> File ui/gfx/render_text.h (right):
>
> https://codereview.chromium.org/1014163002/diff/80001/ui/
> gfx/render_text.h#newcode607
> ui/gfx/render_text.h:607: // Adjust ranged styles and colors to
> accommodate a new text length.
> nit: nix "and colors"
>
> https://codereview.chromium.org/1014163002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dschuyler, 2015-03-20 01:14:02]

The CQ bit was unchecked by dschuyler@chromium.org

[dschuyler, 2015-03-20 01:17:21]

Ok, looks like I was able to change it in this CL.

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

https://codereview.chromium.org/1014163002/diff/80001/ui/gfx/render_text.h#ne...
ui/gfx/render_text.h:607: // Adjust ranged styles and colors to accommodate a
new text length.
On 2015/03/20 01:10:32, msw wrote:
> nit: nix "and colors"

Done.

[dschuyler, 2015-03-20 01:18:02]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2015-03-20 01:18:02]

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/1014163002/#ps100001
(title: "Changed UpdateStyleLengths comment")

[commit-bot: I haz the power, 2015-03-20 01:18:52]

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

[commit-bot: I haz the power, 2015-03-20 03:24:30]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-20 03:25:26]

Patchset 6 (id:??) landed as
https://crrev.com/92bc0de851bc0c094bbd1e3ac5e6b24202011d69
Cr-Commit-Position: refs/heads/master@{#321507}

[msw, 2015-03-20 16:19:22]

Can you please add a unit test for this new functionality?