RenderText: Allow setting display offset explicitly

RenderText: Allow setting display offset explicitly

Currently, display offset for a gfx::RenderText is computed
automatically to ensure the cursor is always visible. This CL adds the
ability to set display offset explicitly which, as an example, can be
useful in adding touch scrolling to textfields. Display offset set this
way is not permanent and will be recalculated next time a property
(text, font, alignment, etc.) is changed.

BUG=388862
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282308

[mohsen, 2014-07-08 00:11:45]

Can you take a look, please...

[msw, 2014-07-08 17:38:24]

Have you tested using the setter (with left/right/center-aligned text)?
This seems mostly correct, but I'd like to see its usage too.

Can you add a test that setting the offset works as intended?
(check that scrolling beyond the text content isn't possible, etc.)

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

https://codereview.chromium.org/378723003/diff/1/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:880: void RenderText::SetDisplayOffset(const Vector2d&
offset) {
nit: perhaps this should only take a horizontal offset value for now?
(that'll avoid the need to pass in GetUpdatedDisplayOffset().y())

https://codereview.chromium.org/378723003/diff/1/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:889: final_offset.set_x(0);
q: does this prevent scrolling before the start of the text?

[mohsen, 2014-07-08 18:36:16]

I have tested the setter with left- and right-aligned text manually by
implementing touch scrolling for the textfield.

I will try center-aligned text and add tests; and update here ASAP.

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

https://codereview.chromium.org/378723003/diff/1/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:880: void RenderText::SetDisplayOffset(const Vector2d&
offset) {
On 2014/07/08 17:38:23, msw wrote:
> nit: perhaps this should only take a horizontal offset value for now?
> (that'll avoid the need to pass in GetUpdatedDisplayOffset().y())

Done.

https://codereview.chromium.org/378723003/diff/1/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:889: final_offset.set_x(0);
On 2014/07/08 17:38:23, msw wrote:
> q: does this prevent scrolling before the start of the text?

Yes.

[mohsen, 2014-07-09 21:22:30]

I've fixed the issue with center-aligned texts and added some tests. Please take
a look...

[msw, 2014-07-10 00:28:54]

nice; lgtm.

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

https://codereview.chromium.org/378723003/diff/120001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:881: int extra_content = GetContentWidth() -
display_rect_.width();
nit: const

[mohsen, 2014-07-10 06:15:53]

The CQ bit was checked by mohsen@chromium.org

[commit-bot: I haz the power, 2014-07-10 06:16:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mohsen@chromium.org/378723003/140001

[commit-bot: I haz the power, 2014-07-10 11:15:05]

Change committed as 282308