Add ExtendSelectionAndDelete() method to ui::TextInputClient.

Add ExtendSelectionAndDelete() method to ui::TextInputClient.

This cl only implements RenderWidgetHostViewWin::ExtendSelectionAndDelete() which is needed to support HANJA key of Korean IME in Metro.

BUG=148355
TEST=trybots

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

[horo, 2012-09-14 02:59:20]

Nona-san.
Cloud you please review this CL?

[Seigo Nonaka, 2012-09-14 04:21:03]

http://codereview.chromium.org/10928199/diff/3001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

http://codereview.chromium.org/10928199/diff/3001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:1187: Send(new
ViewMsg_ExtendSelectionAndDelete(GetRoutingID(),
nit: can be one line? or align arguments.

http://codereview.chromium.org/10928199/diff/3001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.h (right):

http://codereview.chromium.org/10928199/diff/3001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.h:300: void
ExtendSelectionAndDelete(int before, int after);
|before| and |after| can be negative? Please add document.

http://codereview.chromium.org/10928199/diff/3001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/10928199/diff/3001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:1238: NOTREACHED();
please add return; just in case.

[horo, 2012-09-14 04:34:50]

https://codereview.chromium.org/10928199/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/10928199/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1187: Send(new
ViewMsg_ExtendSelectionAndDelete(GetRoutingID(),
On 2012/09/14 04:21:03, Seigo Nonaka wrote:
> nit: can be one line? or align arguments.

Done.

https://codereview.chromium.org/10928199/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/10928199/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.h:300: void
ExtendSelectionAndDelete(int before, int after);
On 2012/09/14 04:21:03, Seigo Nonaka wrote:
> |before| and |after| can be negative? Please add document.

Done.

https://codereview.chromium.org/10928199/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

https://codereview.chromium.org/10928199/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.cc:1238: NOTREACHED();
On 2012/09/14 04:21:03, Seigo Nonaka wrote:
> please add return; just in case.

Done.

[Seigo Nonaka, 2012-09-14 04:49:19]

lgtm

lgtm with nits

http://codereview.chromium.org/10928199/diff/3002/ui/base/ime/input_method_ib...
File ui/base/ime/input_method_ibus_unittest.cc (right):

http://codereview.chromium.org/10928199/diff/3002/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus_unittest.cc:217: virtual void
ExtendSelectionAndDelete(int before, int after) OVERRIDE { }
remove white space after brace.

http://codereview.chromium.org/10928199/diff/5007/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

http://codereview.chromium.org/10928199/diff/5007/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:1188: DCHECK(after >=
0);
DCHECK_LE?

http://codereview.chromium.org/10928199/diff/5007/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/10928199/diff/5007/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:1237: if
(!base::win::IsTsfAwareRequired())
if (!base::win::IsTsfAwareRequired()) {
  NOTREACHED();
  return;
}

is fine because
- Release build ignores NOTREACHED macro
- Good to track if unexpected code path uses TSF related stuff.

[horo, 2012-09-14 05:01:20]

https://codereview.chromium.org/10928199/diff/5007/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/10928199/diff/5007/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1188: DCHECK(after >=
0);
On 2012/09/14 04:49:19, Seigo Nonaka wrote:
> DCHECK_LE?

Done.

https://codereview.chromium.org/10928199/diff/5007/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

https://codereview.chromium.org/10928199/diff/5007/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.cc:1237: if
(!base::win::IsTsfAwareRequired())
On 2012/09/14 04:49:19, Seigo Nonaka wrote:
> if (!base::win::IsTsfAwareRequired()) {
>   NOTREACHED();
>   return;
> }
> 
> is fine because
> - Release build ignores NOTREACHED macro
> - Good to track if unexpected code path uses TSF related stuff.

Done.

[horo, 2012-09-14 05:03:25]

yusukes@ and sky@
Cloud you please review this CL?

Thank you.

[Yusuke Sato, 2012-09-14 05:56:15]

ui/base/ime/ and ui/views/ime/ LGTM

https://codereview.chromium.org/10928199/diff/4016/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/10928199/diff/4016/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:1170: //
TODO(horo): implement this method if it is required.
I think the new TextInputClient interface would be useful not only for Metro TSF
but also for ui::InputMethod IMEs to implement "delete surrounding text" like
feature, right?

If that's correct, file a bug like "Add delete surrounging text feature to
ui::InputMethodIBus" on crbug.com, assign it someone in your team, and add the
crbug.com bug # here.

https://codereview.chromium.org/10928199/diff/4016/ui/views/controls/textfiel...
File ui/views/controls/textfield/native_textfield_views.cc (right):

https://codereview.chromium.org/10928199/diff/4016/ui/views/controls/textfiel...
ui/views/controls/textfield/native_textfield_views.cc:857: NOTIMPLEMENTED();
ditto. crbug,com bug #.

[horo, 2012-09-14 06:28:01]

Thank you very much yusuke-san!

https://codereview.chromium.org/10928199/diff/4016/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/10928199/diff/4016/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:1170: //
TODO(horo): implement this method if it is required.
On 2012/09/14 05:56:16, Yusuke Sato wrote:
> I think the new TextInputClient interface would be useful not only for Metro
TSF
> but also for ui::InputMethod IMEs to implement "delete surrounding text" like
> feature, right?
> 
> If that's correct, file a bug like "Add delete surrounging text feature to
> ui::InputMethodIBus" on http://crbug.com, assign it someone in your team, and
add the
> http://crbug.com bug # here.

Done.

https://codereview.chromium.org/10928199/diff/4016/ui/views/controls/textfiel...
File ui/views/controls/textfield/native_textfield_views.cc (right):

https://codereview.chromium.org/10928199/diff/4016/ui/views/controls/textfiel...
ui/views/controls/textfield/native_textfield_views.cc:857: NOTIMPLEMENTED();
On 2012/09/14 05:56:16, Yusuke Sato wrote:
> ditto. crbug,com bug #.

Done.

[Yohei Yukawa, 2012-09-14 07:01:20]

lgtm

[sky, 2012-09-14 16:14:11]

https://codereview.chromium.org/10928199/diff/21/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/10928199/diff/21/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.h:301: void
ExtendSelectionAndDelete(int before, int after);
This is a weird method, why not DeleteRange(const ui::Range&);

[horo, 2012-09-14 16:29:19]

https://codereview.chromium.org/10928199/diff/21/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/10928199/diff/21/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.h:301: void
ExtendSelectionAndDelete(int before, int after);
On 2012/09/14 16:14:11, sky wrote:
> This is a weird method, why not DeleteRange(const ui::Range&);

Because of the asynchronous IPC between the browser process and the renderer
process, the selection position which RenderWidgetHostView knows as
RenderWidgetHostViewBase.selection_range_ which is set in
RenderWidgetHostViewBase::SelectionChanged() may be different from the actual
position.

So calling GetSelectionRange() and DeleteRange() is not appropriate.

[sky, 2012-09-14 17:13:53]

That seems like an implementation for render widgets implementation.

[sky, 2012-09-14 23:16:26]

Make size_t and LGTM

https://codereview.chromium.org/10928199/diff/21/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/10928199/diff/21/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.cc:1187: DCHECK_GE(before,
0);
Use size_t so you don't have these DCHECKs.

https://codereview.chromium.org/10928199/diff/21/ui/base/ime/text_input_client.h
File ui/base/ime/text_input_client.h (right):

https://codereview.chromium.org/10928199/diff/21/ui/base/ime/text_input_clien...
ui/base/ime/text_input_client.h:133: // |before| and |after| can't be negative.
remove comment once you make size_t

[horo, 2012-09-14 23:50:38]

Thank you for your review!

https://codereview.chromium.org/10928199/diff/21/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/10928199/diff/21/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.cc:1187: DCHECK_GE(before,
0);
On 2012/09/14 23:16:26, sky wrote:
> Use size_t so you don't have these DCHECKs.

Done.

https://codereview.chromium.org/10928199/diff/21/ui/base/ime/text_input_client.h
File ui/base/ime/text_input_client.h (right):

https://codereview.chromium.org/10928199/diff/21/ui/base/ime/text_input_clien...
ui/base/ime/text_input_client.h:133: // |before| and |after| can't be negative.
On 2012/09/14 23:16:26, sky wrote:
> remove comment once you make size_t

Done.

[commit-bot: I haz the power, 2012-09-15 01:19:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/horo@chromium.org/10928199/1035

[commit-bot: I haz the power, 2012-09-15 02:20:28]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-15 03:05:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/horo@chromium.org/10928199/1035

[commit-bot: I haz the power, 2012-09-15 03:23:21]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-15 03:30:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/horo@chromium.org/10928199/1035

[commit-bot: I haz the power, 2012-09-15 05:12:53]

Change committed as 156992