Fix cursor positioning regression from r201136. GetCursorPos() shouldn't assume

Fix cursor positioning regression from r201136.  GetCursorPos() shouldn't assume
vertically-centered text.

BUG=242801
TEST=Arrow through the omnibox in 200% mode and ensure the cursor does not appear vertically too low
R=asvitkine@chromium.org, msw@chromium.org

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

[Peter Kasting, 2013-05-22 20:26:08]

Adding all three of you for discussion of this fix.

The current code can change the cursor (and selection?) height as the user
arrows through the text.  This seems kind of wrong to Alexei and me but maybe
it's desirable for some kind of font fallback case.  I need to understand
whether (and why) we want to keep this behavior, because this patch removes it
and always makes the cursor use GetStringSize().height().

[msw, 2013-05-23 19:54:26]

This looks right, as ToViewPoint should take top-aligned text-space bounds.
There's no real reason to use the height of particular glyphs (for now), IIRC.
I just have one suggestion to simplify the GetGlyphBounds signature.

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

https://codereview.chromium.org/15746013/diff/1/ui/gfx/render_text.h#newcode375
ui/gfx/render_text.h:375: virtual void GetGlyphBounds(size_t index, ui::Range*
xspan) = 0;
This should now return a ui::Range instead of using an out-param.

[Peter Kasting, 2013-05-23 21:01:29]

Are you comfortable giving LGTM, then?

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

https://codereview.chromium.org/15746013/diff/1/ui/gfx/render_text.h#newcode375
ui/gfx/render_text.h:375: virtual void GetGlyphBounds(size_t index, ui::Range*
xspan) = 0;
On 2013/05/23 19:54:26, msw wrote:
> This should now return a ui::Range instead of using an out-param.

Good call.  Done.

[Alexei Svitkine (slow), 2013-05-23 21:03:47]

LGTM (put please wait for msw's too)

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

https://codereview.chromium.org/15746013/diff/26001/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:373: // xspan->is_reversed(). This does not return a Rect
because a Rect can't have
Please update this comment to not mention xpan anymore.

[Peter Kasting, 2013-05-23 21:06:58]

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

https://codereview.chromium.org/15746013/diff/26001/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:373: // xspan->is_reversed(). This does not return a Rect
because a Rect can't have
On 2013/05/23 21:03:47, Alexei Svitkine wrote:
> Please update this comment to not mention xpan anymore.

Oops.  Fixed (but not re-uploaded).

[msw, 2013-05-23 21:07:27]

LGTM with nits.

https://codereview.chromium.org/15746013/diff/26001/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

https://codereview.chromium.org/15746013/diff/26001/ui/gfx/render_text_mac.cc...
ui/gfx/render_text_mac.cc:72: // TODO(asvitkine): Implement this.
http://crbug.com/131618
nit: This probably needs a return statement.

https://codereview.chromium.org/15746013/diff/26001/ui/gfx/render_text_unitte...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/15746013/diff/26001/ui/gfx/render_text_unitte...
ui/gfx/render_text_unittest.cc:288: ui::Range
bounds(render_text->GetGlyphBounds(0U));
nit: you can inline this call below and eliminate |bounds|.

[Peter Kasting, 2013-05-23 21:09:10]

https://codereview.chromium.org/15746013/diff/26001/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

https://codereview.chromium.org/15746013/diff/26001/ui/gfx/render_text_mac.cc...
ui/gfx/render_text_mac.cc:72: // TODO(asvitkine): Implement this.
http://crbug.com/131618
On 2013/05/23 21:07:27, msw wrote:
> nit: This probably needs a return statement.

Yes.

https://codereview.chromium.org/15746013/diff/26001/ui/gfx/render_text_unitte...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/15746013/diff/26001/ui/gfx/render_text_unitte...
ui/gfx/render_text_unittest.cc:288: ui::Range
bounds(render_text->GetGlyphBounds(0U));
On 2013/05/23 21:07:27, msw wrote:
> nit: you can inline this call below and eliminate |bounds|.

Good point.

[Peter Kasting, 2013-05-24 00:49:08]

Committed patchset #3 manually as r201944 (presubmit successful).