Fix RenderText cached bounds and offset logic; update clients.

Fix RenderText cached bounds and offset logic; update clients.
Ensure the display offset and cursor bounds are valid on use.
Build in offset logic to RenderText display and hit-testing.
Implement simpler temporary GetStringWidth and GetCursorBounds.
Fix SetDisplayRect signature, code file ordering, invalidation.
Rename and refactor a bit, update comments.

Fixes adornment display & hit testing on text field overflows.
Increases abstraction/encapsulation for easier client use.

BUG=90426
TEST=--use-pure-views / touch_ui textfield use with overflow.

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

[xji, 2011-08-05 23:15:53]

lgtm

[msw, 2011-08-05 23:22:57]

Unfortunately, this doesn't pass tests because other areas of code are relying
on undesirable/broken behavior.

I'll try to take a look over the weekend, although this isn't necessarily
blocking ongoing bidi work.

[msw, 2011-08-09 01:39:00]

I've fixed the remaining issues I found with the cursor bounds and display
offset. Tests pass locally. Please take a look, thanks!

[varunjain, 2011-08-09 20:45:15]

On 2011/08/09 01:39:00, msw wrote:
> I've fixed the remaining issues I found with the cursor bounds and display
> offset. Tests pass locally. Please take a look, thanks!

LGTM

[msw, 2011-08-09 22:37:55]

XJI: Please review the improvements we discussed offline.
VarunJain FYI (no need to re-review): I made a minor test fix.

[xji, 2011-08-10 00:36:15]

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

http://codereview.chromium.org/7466048/diff/13001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:175: cached_bounds_and_offset_valid_ = false;
does it make sense to invalidate this in SetText() as well?

http://codereview.chromium.org/7466048/diff/13001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:598: cursor_bounds_.Offset(delta_offset, 0);
In the case of both cursor_bounds_ and display_offset_ need to be updated, looks
like GetCursorBounds() will compute the cursor_bounds_ using old
display_offset_, and the delta_offset will offset cursor_bounds_ to the correct
value. 

Then, does it mean GetCursorBounds() is not an accurate function to get the
correct cursor_bounds_ (if the value is not just used for comparison) and should
not be exposed as public API? Or it is ok for all the callers to get might be
overflowed cursor_bounds by GetCursorBounds()?

[msw, 2011-08-10 01:27:38]

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

http://codereview.chromium.org/7466048/diff/13001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:175: cached_bounds_and_offset_valid_ = false;
On 2011/08/10 00:36:15, xji wrote:
> does it make sense to invalidate this in SetText() as well?

Done.

http://codereview.chromium.org/7466048/diff/13001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:598: cursor_bounds_.Offset(delta_offset, 0);
On 2011/08/10 00:36:15, xji wrote:
> In the case of both cursor_bounds_ and display_offset_ need to be updated,
looks
> like GetCursorBounds() will compute the cursor_bounds_ using old
> display_offset_, and the delta_offset will offset cursor_bounds_ to the
correct
> value. 
> 
> Then, does it mean GetCursorBounds() is not an accurate function to get the
> correct cursor_bounds_ (if the value is not just used for comparison) and
should
> not be exposed as public API? Or it is ok for all the callers to get might be
> overflowed cursor_bounds by GetCursorBounds()?

Your first paragraph is correct, I've added a comment explaining that.

GetCursorBounds and its functional core, GetSubstringBounds, are okay as they
are. GetSubstringBounds calls GetUpdatedDisplayOffset(), which will fix the
current offset (and current cached cursor bounds) if needed, and then calculate
the correct substring bounds. If users call these functions for bounds that fall
outside the display rect, it won't re-adjust the offset to accommodate those
bounds, it'll just return results outside the display rect. It's up to the user
to check and handle that condition if it's relevant.

The only (fairly obvious) caveat is that: if a some client code caches their own
selection or cursor bounds (ex/ GetCursorBounds(new_cursor, insert)) before
calling SetSelectionModel(new_cursor) to update display_offset_, their cached
bounds could be stale. I think common sense, our API offering of
GetUpdatedCursorBounds, and our comment around cached_bounds_and_offset_valid_
cover this scenario. Do you have any additional ideas to safeguard against this?

[xji, 2011-08-10 18:15:53]

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

http://codereview.chromium.org/7466048/diff/13001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:598: cursor_bounds_.Offset(delta_offset, 0);
On 2011/08/10 01:27:39, msw wrote:
> On 2011/08/10 00:36:15, xji wrote:
> > In the case of both cursor_bounds_ and display_offset_ need to be updated,
> looks
> > like GetCursorBounds() will compute the cursor_bounds_ using old
> > display_offset_, and the delta_offset will offset cursor_bounds_ to the
> correct
> > value. 
> > 
> > Then, does it mean GetCursorBounds() is not an accurate function to get the
> > correct cursor_bounds_ (if the value is not just used for comparison) and
> should
> > not be exposed as public API? Or it is ok for all the callers to get might
be
> > overflowed cursor_bounds by GetCursorBounds()?
> 
> Your first paragraph is correct, I've added a comment explaining that.
> 
> GetCursorBounds and its functional core, GetSubstringBounds, are okay as they
> are. GetSubstringBounds calls GetUpdatedDisplayOffset(), which will fix the
> current offset (and current cached cursor bounds) if needed, and then
calculate
> the correct substring bounds.

I looked at GetCursorBounds() again. And I think you are right since it calles
GetUpdatedDisplayOffset() which will update both cursor_bounds_ and
display_offset_ to the correct values if needed.


> If users call these functions for bounds that fall
> outside the display rect, it won't re-adjust the offset to accommodate those
> bounds, it'll just return results outside the display rect. It's up to the
user
> to check and handle that condition if it's relevant.

But I am confused by the above statement.

> 
> The only (fairly obvious) caveat is that: if a some client code caches their
own
> selection or cursor bounds (ex/ GetCursorBounds(new_cursor, insert)) before
> calling SetSelectionModel(new_cursor) to update display_offset_, their cached
> bounds could be stale. I think common sense, our API offering of
> GetUpdatedCursorBounds, and our comment around cached_bounds_and_offset_valid_
> cover this scenario. Do you have any additional ideas to safeguard against
this?

I do not have a better way (algorithm) wise to safeguard this on top of my head.

[msw, 2011-08-10 18:20:30]

> > If users call these functions for bounds that fall
> > outside the display rect, it won't re-adjust the offset to accommodate those
> > bounds, it'll just return results outside the display rect. It's up to the
> user
> > to check and handle that condition if it's relevant.
> 
> But I am confused by the above statement.

I just mean that if a consumer asks for selection or cursor bounds, those may
fall outside the display rect. That shouldn't be something that we handle,
they'll need to handle that if they want to. I'll update the comments around
those functions to reflect that possibility.

> 
> > 
> > The only (fairly obvious) caveat is that: if a some client code caches their
> own
> > selection or cursor bounds (ex/ GetCursorBounds(new_cursor, insert)) before
> > calling SetSelectionModel(new_cursor) to update display_offset_, their
cached
> > bounds could be stale. I think common sense, our API offering of
> > GetUpdatedCursorBounds, and our comment around
cached_bounds_and_offset_valid_
> > cover this scenario. Do you have any additional ideas to safeguard against
> this?
> 
> I do not have a better way (algorithm) wise to safeguard this on top of my
head.

That's fine, I'll just comment on those functions. I think we're doing the right
thing.

[varunjain, 2011-08-10 18:21:47]

http://codereview.chromium.org/7466048/diff/15001/views/touchui/touch_selecti...
File views/touchui/touch_selection_controller_impl_unittest.cc (right):

http://codereview.chromium.org/7466048/diff/15001/views/touchui/touch_selecti...
views/touchui/touch_selection_controller_impl_unittest.cc:41: params.bounds =
gfx::Rect(0, 0, 200, 200);
why is this needed?

[msw, 2011-08-10 18:27:44]

http://codereview.chromium.org/7466048/diff/15001/views/touchui/touch_selecti...
File views/touchui/touch_selection_controller_impl_unittest.cc (right):

http://codereview.chromium.org/7466048/diff/15001/views/touchui/touch_selecti...
views/touchui/touch_selection_controller_impl_unittest.cc:41: params.bounds =
gfx::Rect(0, 0, 200, 200);
On 2011/08/10 18:21:47, varunjain wrote:
> why is this needed?

Because with the updated code fixing the overflow problem, the
SelectRectCallbackTest text overflows the 100 pixel width bounds, and the text
pans, but your test doesn't expect any changes in the text placement.
Specifically, SelectRectCallbackTest was failing inside the new line 181
VerifySelectionHandlePositions(false); at new line 104&105.

[msw, 2011-08-10 19:03:46]

Comments updated. PTAL, thanks!

[xji, 2011-08-10 19:06:05]

lgtm. Thanks!

[commit-bot: I haz the power, 2011-08-11 07:05:49]

Change committed as 96334