Add text offset to OnSurroundingTextChanged

Add text offset to OnSurroundingTextChanged.

See bug below for proposal doc.

BUG=518921
Committed: https://crrev.com/df0c56f1d2b8bb37141d368c80e677e0f8c2321d
Cr-Commit-Position: refs/heads/master@{#343243}

[rsadam, 2015-08-10 18:42:18]

rsadam@chromium.org changed reviewers:
+ shuchen@chromium.org

[rsadam, 2015-08-10 18:42:39]

Hi Shu, please take a look!

[rsadam, 2015-08-10 19:46:53]

Fix some formatting issues.

[rsadam, 2015-08-11 17:45:14]

Friendly ping!

[Shu Chen, 2015-08-12 07:23:16]

https://codereview.chromium.org/1282823002/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/input_ime.json (right):

https://codereview.chromium.org/1282823002/diff/20001/chrome/common/extension...
chrome/common/extensions/api/input_ime.json:708: "description": "The offset
position of the selection. This value indicates the number of characters not
included in the surrounding text."
Please describe clearly for the difference between offset and focus/anchor:
  focus/anchor is relative to the |text|, while offset is relative to the whole
text of the input field, which include the text isn't included in |text|.

https://codereview.chromium.org/1282823002/diff/20001/ui/base/ime/chromeos/im...
File ui/base/ime/chromeos/ime_bridge.h (right):

https://codereview.chromium.org/1282823002/diff/20001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_bridge.h:103: // text is given |offset_pos| indicates
the starting offset of |text|.
Here is "offset of |text|", but in input_ime.json it is "offset of selection".
Which is correct?

https://codereview.chromium.org/1282823002/diff/20001/ui/base/ime/text_input_...
File ui/base/ime/text_input_client.h (right):

https://codereview.chromium.org/1282823002/diff/20001/ui/base/ime/text_input_...
ui/base/ime/text_input_client.h:123: virtual uint32 GetSelectionOffset() const =
0;
What's the difference between "GetSelectionRange(&range); range.start();" and
"GetSelectionOffset()"?

https://codereview.chromium.org/1282823002/diff/20001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1282823002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1562: return 0;
Why always return 0?

[rsadam, 2015-08-12 21:46:52]

https://codereview.chromium.org/1282823002/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/input_ime.json (right):

https://codereview.chromium.org/1282823002/diff/20001/chrome/common/extension...
chrome/common/extensions/api/input_ime.json:708: "description": "The offset
position of the selection. This value indicates the number of characters not
included in the surrounding text."
On 2015/08/12 07:23:15, Shu Chen wrote:
> Please describe clearly for the difference between offset and focus/anchor:
>   focus/anchor is relative to the |text|, while offset is relative to the
whole
> text of the input field, which include the text isn't included in |text|.

Done.

https://codereview.chromium.org/1282823002/diff/20001/ui/base/ime/chromeos/im...
File ui/base/ime/chromeos/ime_bridge.h (right):

https://codereview.chromium.org/1282823002/diff/20001/ui/base/ime/chromeos/im...
ui/base/ime/chromeos/ime_bridge.h:103: // text is given |offset_pos| indicates
the starting offset of |text|.
On 2015/08/12 07:23:16, Shu Chen wrote:
> Here is "offset of |text|", but in input_ime.json it is "offset of selection".
> Which is correct?

Should be offset of |text|. Fixed.

https://codereview.chromium.org/1282823002/diff/20001/ui/base/ime/text_input_...
File ui/base/ime/text_input_client.h (right):

https://codereview.chromium.org/1282823002/diff/20001/ui/base/ime/text_input_...
ui/base/ime/text_input_client.h:123: virtual uint32 GetSelectionOffset() const =
0;
On 2015/08/12 07:23:16, Shu Chen wrote:
> What's the difference between "GetSelectionRange(&range); range.start();" and
> "GetSelectionOffset()"?
> 

Renamed it to GetTextOffset and updated the comment.

https://codereview.chromium.org/1282823002/diff/20001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1282823002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1562: return 0;
On 2015/08/12 07:23:16, Shu Chen wrote:
> Why always return 0?

From my understanding textfield.h is for all native textboxes e.g omnibox. These
return the complete surrounding text, and so the offset is always 0. We could
change textfield.h behaviour to mimic blink, and only return the last 100
characters.

[Shu Chen, 2015-08-13 01:27:24]

https://codereview.chromium.org/1282823002/diff/20001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1282823002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1562: return 0;
On 2015/08/12 21:46:52, rsadam wrote:
> On 2015/08/12 07:23:16, Shu Chen wrote:
> > Why always return 0?
> 
> From my understanding textfield.h is for all native textboxes e.g omnibox.
These
> return the complete surrounding text, and so the offset is always 0. We could
> change textfield.h behaviour to mimic blink, and only return the last 100
> characters.

Let's not change the existing behavior for now.
Returning zero here sounds fine.

https://codereview.chromium.org/1282823002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1282823002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1709: return
selection_text_offset_;
Are you sure |selection_text_offset_| is the offset of text instead of offset of
selection?

https://codereview.chromium.org/1282823002/diff/40001/ui/base/ime/text_input_...
File ui/base/ime/text_input_client.h (right):

https://codereview.chromium.org/1282823002/diff/40001/ui/base/ime/text_input_...
ui/base/ime/text_input_client.h:123: virtual uint32 GetTextOffset() const = 0;
What's the difference between GetTextRange and GetTextOffset?

[rsadam, 2015-08-13 04:31:11]

Removed the new method from text_input_client. Rely on textRange.start() instead

https://codereview.chromium.org/1282823002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1282823002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1709: return
selection_text_offset_;
On 2015/08/13 01:27:24, Shu Chen wrote:
> Are you sure |selection_text_offset_| is the offset of text instead of offset
of
> selection?

In render_widget_host_view_base we set selection_text_offset_ to offset.

https://codereview.chromium.org/1282823002/diff/40001/ui/base/ime/text_input_...
File ui/base/ime/text_input_client.h (right):

https://codereview.chromium.org/1282823002/diff/40001/ui/base/ime/text_input_...
ui/base/ime/text_input_client.h:123: virtual uint32 GetTextOffset() const = 0;
On 2015/08/13 01:27:24, Shu Chen wrote:
> What's the difference between GetTextRange and GetTextOffset?

Oh wow, nice catch. They're identical. This simplifies this CL significantly.

[Shu Chen, 2015-08-13 06:15:46]

lgtm

[rsadam, 2015-08-13 16:47:49]

rsadam@chromium.org changed reviewers:
+ rockot@chromium.org

[rsadam, 2015-08-13 16:47:49]

rockot@chromium.org: Please review changes in input_ime.json

[Ken Rockot(use gerrit already), 2015-08-13 17:15:59]

https://codereview.chromium.org/1282823002/diff/60001/chrome/common/extension...
File chrome/common/extensions/api/input_ime.json (right):

https://codereview.chromium.org/1282823002/diff/60001/chrome/common/extension...
chrome/common/extensions/api/input_ime.json:696: "description": "The text around
cursor. This is only a subset of all text in the input field."
maybe "around the cursor"?

https://codereview.chromium.org/1282823002/diff/60001/chrome/common/extension...
chrome/common/extensions/api/input_ime.json:708: "description": "The offset
position of |text|. Since |text| only includes a subset of text around the
cursor, offset indicates the absolute position of the first character of
|text|."
In JSON schemas, description strings need to use use <code>text</code> instead
of |text|.

[rsadam, 2015-08-13 17:22:42]

Patchset #5 (id:80001) has been deleted

[rsadam, 2015-08-13 17:23:18]

Thanks for the feedback. I addressed the concerns you listed, PTANL!

https://codereview.chromium.org/1282823002/diff/60001/chrome/common/extension...
File chrome/common/extensions/api/input_ime.json (right):

https://codereview.chromium.org/1282823002/diff/60001/chrome/common/extension...
chrome/common/extensions/api/input_ime.json:696: "description": "The text around
cursor. This is only a subset of all text in the input field."
On 2015/08/13 17:15:59, Ken Rockot wrote:
> maybe "around the cursor"?

Done.

https://codereview.chromium.org/1282823002/diff/60001/chrome/common/extension...
chrome/common/extensions/api/input_ime.json:708: "description": "The offset
position of |text|. Since |text| only includes a subset of text around the
cursor, offset indicates the absolute position of the first character of
|text|."
On 2015/08/13 17:15:59, Ken Rockot wrote:
> In JSON schemas, description strings need to use use <code>text</code> instead
> of |text|.

Ack! Fixed all instances in description.

[Ken Rockot(use gerrit already), 2015-08-13 17:27:15]

lgtm

[rsadam, 2015-08-13 17:58:50]

The CQ bit was checked by rsadam@chromium.org

[rsadam, 2015-08-13 17:58:51]

The patchset sent to the CQ was uploaded after l-g-t-m from shuchen@chromium.org
Link to the patchset: https://codereview.chromium.org/1282823002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-08-13 17:59:26]

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

[commit-bot: I haz the power, 2015-08-13 18:42:19]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-08-13 18:42:58]

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