Add IPCs/methods for additional IME actions.

Add IPCs/methods for additional IME actions.

This adds IPCs and support in RenderViewImpl for the following: ReplaceAll, Unselect,
SetEditableSelectionOffsets, SetCompositionFromExistingText, ExtendSelectionAndDelete.

These are used to implement IME actions in the Android port, providing functions as required
by the Android InputConnection interface.

BUG=136738
TEST=RenderViewImplTest (new tests added: SetEditableSelectionAndComposition, OnReplaceAll, OnExtendSelectionAndDelete)

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

[bulach, 2012-08-01 20:09:45]

thanks oli! drive by, not an owner here.. (for the android's usage looks fine,
but darin needs to approve the API changes..)

http://codereview.chromium.org/10836053/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10836053/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:1311: void
RenderViewImpl::OnSetComposingRegion(int start, int end,
nit: params on the next line

http://codereview.chromium.org/10836053/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/10836053/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.h:968: const
std::vector<WebKit::WebCompositionUnderline>& underlines);
nit: I think it's either left aligned (as 957) or no param on the first line
(973).

[darin (slow to review), 2012-08-02 06:25:12]

http://codereview.chromium.org/10836053/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10836053/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:1342:
webview()->focusedFrame()->executeCommand(
can't executing this command have side-effects?  for example, suppose the cursor
position changed as a side-effect?  are you sure you don't want to implement
this in a more robust manner?  it seems like you could compute a dom range and
perform a single delete of that, or perhaps webcore has some other primitives
that can help with batch deletion?

i'm not an editing expert, but doesn't this result in a N states being pushed
onto the undo stack?  is that intended?  should the deletions be pushed onto the
undo stack as a single entity instead?  would help to get someone like rniwa@ to
review this too.

[olilan, 2012-08-02 09:55:44]

http://codereview.chromium.org/10836053/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10836053/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:1342:
webview()->focusedFrame()->executeCommand(
On 2012/08/02 06:25:13, darin wrote:
> can't executing this command have side-effects?  for example, suppose the
cursor
> position changed as a side-effect?  are you sure you don't want to implement
> this in a more robust manner?  it seems like you could compute a dom range and
> perform a single delete of that, or perhaps webcore has some other primitives
> that can help with batch deletion?
> 
> i'm not an editing expert, but doesn't this result in a N states being pushed
> onto the undo stack?  is that intended?  should the deletions be pushed onto
the
> undo stack as a single entity instead?  would help to get someone like rniwa@
to
> review this too.

Yes, agreed this implementation is not ideal. At the least we could select the
range to be deleted and issue a single delete command, unless there's a better
way in WebKit.

rniwa, can you advise?

[rniwa-cr, 2012-08-02 17:11:54]

http://codereview.chromium.org/10836053/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10836053/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:1330:
webview()->setEditableSelectionOffsets(info.selectionStart,
We should just add a new method to Editor that does all of this.
The way it's currently written is extremely inefficient and potentially error
prone.

http://codereview.chromium.org/10836053/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:1342:
webview()->focusedFrame()->executeCommand(
First off, we should move all of this to Source/WebKit/chromium. We can then use
TextIterator to get a range out of before/after. We can then delete all of
characters in one step.

[olilan, 2012-08-03 15:55:08]

http://codereview.chromium.org/10836053/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10836053/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:1342:
webview()->focusedFrame()->executeCommand(
On 2012/08/02 17:11:54, rniwa-cr wrote:
> First off, we should move all of this to Source/WebKit/chromium. We can then
use
> TextIterator to get a range out of before/after. We can then delete all of
> characters in one step.

Thanks. One question: is there a way to delete a Range without selecting it
first? From what I understand, DeleteSelectionCommand has much of the code we'd
need to do a delete like this.

[rniwa-cr, 2012-08-03 20:00:07]

http://codereview.chromium.org/10836053/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10836053/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:1342:
webview()->focusedFrame()->executeCommand(
There isn't if it needs to be in the undo stack.

[olilan, 2012-08-10 16:40:38]

Thanks. I've created a patch to implement SetComposingRegion and
DeleteSurroundingText in WebKit, here:
https://bugs.webkit.org/show_bug.cgi?id=93724

[olilan, 2012-08-21 14:36:15]

Updated with calls to new WebKit methods as added in
https://bugs.webkit.org/show_bug.cgi?id=93724 (once that lands). Also tests
added to RenderViewImplTest.

http://codereview.chromium.org/10836053/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10836053/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:1311: void
RenderViewImpl::OnSetComposingRegion(int start, int end,
On 2012/08/01 20:09:45, bulach wrote:
> nit: params on the next line

Done.

http://codereview.chromium.org/10836053/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:1330:
webview()->setEditableSelectionOffsets(info.selectionStart,
On 2012/08/02 17:11:54, rniwa-cr wrote:
> We should just add a new method to Editor that does all of this.
> The way it's currently written is extremely inefficient and potentially error
> prone.

Replaced by a call to new WebKit method setCompositionFromExistingText.

http://codereview.chromium.org/10836053/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:1342:
webview()->focusedFrame()->executeCommand(
On 2012/08/03 20:00:07, rniwa-cr wrote:
> There isn't if it needs to be in the undo stack.

Implementation replaced by call to new WebKit method ExtendSelectionAndDelete.

http://codereview.chromium.org/10836053/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/10836053/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.h:968: const
std::vector<WebKit::WebCompositionUnderline>& underlines);
On 2012/08/01 20:09:45, bulach wrote:
> nit: I think it's either left aligned (as 957) or no param on the first line
> (973).

Done.

[olilan, 2012-08-23 17:17:26]

rniwa, could you take another look at this please, now that the WebKit changes
in https://bugs.webkit.org/show_bug.cgi?id=93724 have landed?

[rniwa-cr, 2012-08-23 17:46:26]

http://codereview.chromium.org/10836053/diff/1006/content/renderer/render_vie...
File content/renderer/render_view_browsertest.cc (right):

http://codereview.chromium.org/10836053/diff/1006/content/renderer/render_vie...
content/renderer/render_view_browsertest.cc:1731: 
Why didn't we add these tests in webkit side?

[rniwa-cr, 2012-08-23 17:47:13]

I don't have any other comment as I don't know this code base.

[olilan, 2012-08-24 09:30:49]

http://codereview.chromium.org/10836053/diff/1006/content/renderer/render_vie...
File content/renderer/render_view_browsertest.cc (right):

http://codereview.chromium.org/10836053/diff/1006/content/renderer/render_vie...
content/renderer/render_view_browsertest.cc:1731: 
On 2012/08/23 17:46:26, rniwa-cr wrote:
> Why didn't we add these tests in webkit side?

Equivalent tests have been added on the webkit side for
setCompositionFromExistingText and ExtendSelectionAndDelete. The tests here are
to ensure that the new methods in RenderViewImpl are calling through correctly.

[olilan, 2012-08-28 17:48:44]

jam, could you ptal?

[jam, 2012-08-28 20:48:58]

On 2012/08/28 17:48:44, olilan wrote:
> jam, could you ptal?

if Darin reviewed this before, he should take a look again since I don't know if
he has outstanding comments or not

[olilan, 2012-09-03 14:49:05]

darin, could you take another look please?

[darin (slow to review), 2012-09-04 20:12:58]

LGTM w/ nit fixed

https://chromiumcodereview.appspot.com/10836053/diff/14001/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10836053/diff/14001/content/renderer/r...
content/renderer/render_view_impl.cc:1390: WebKit::WebNode node =
GetFocusedNode();
nit: no need for WebKit:: before WebNode

[commit-bot: I haz the power, 2012-09-06 15:20:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/olilan@chromium.org/10836053/19001

[commit-bot: I haz the power, 2012-09-06 15:47:29]

Try job failure for 10836053-19001 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-09-06 17:27:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/olilan@chromium.org/10836053/18003

[commit-bot: I haz the power, 2012-09-06 20:01:20]

Try job failure for 10836053-18003 (retry) (retry) on win_rel for step
"runhooks".
It's a second try, previously, step "runhooks" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-09-07 09:06:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/olilan@chromium.org/10836053/18003

[commit-bot: I haz the power, 2012-09-07 10:44:34]

Try job failure for 10836053-18003 (retry) on mac_rel for step
"sync_integration_tests".
It's a second try, previously, step "sync_integration_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-09-07 17:41:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/olilan@chromium.org/10836053/18003

[commit-bot: I haz the power, 2012-09-07 20:09:08]

Change committed as 155461