Tracking composition range on the browser side (Aura)

Tracking composition range on the browser side (Aura)

Currently, the composition range information which is sent by RenderWidget in the IPC
InputHostMsg_ImeCompositionRangeChanged is not being used when the source is a child frame.
This CL modifies the RenderWidgetHostViewBase class to report the composition range to the
TextInputManager which aggregates all such information from sub frames.

The CL adds a unit test to verify that the InputMethod uses the correct composition range
values for the active widget. A TODO is also left in site_per_process_text_input_browsertest.cc
to come back and write an interactive test for tracking composition range for sub frames
after the renderer side of IME is fixed.

BUG=578168, 602723
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/b8e23a96c9c1d2933c57ba99aeac3199845fba99
Cr-Commit-Position: refs/heads/master@{#404873}

[EhsanK, 2016-07-07 15:15:07]

Description was changed from

==========
Tracking composition range on the browser side (Aura)

Currently, the composition range information which is sent by RenderWidget in
the IPC
InputHostMsg_ImeCompositionRangeChanged is not being used when the source is a
child frame.
This CL modifies the RenderWidgetHostViewBase class to report the composition
range to the
TextInputManager which aggregates all such information from sub frames.

The CL adds a unit test to verify that the InputMethod uses the correct
composition range
values for the active widget. A TODO is also left in
site_per_process_text_input_browsertest.cc
to come back and write an interactive test for tracking composition range for
sub frames
after the renderer side of IME is fixed.

BUG=578168, 602723
==========

to

==========
Tracking composition range on the browser side (Aura)

Currently, the composition range information which is sent by RenderWidget in
the IPC
InputHostMsg_ImeCompositionRangeChanged is not being used when the source is a
child frame.
This CL modifies the RenderWidgetHostViewBase class to report the composition
range to the
TextInputManager which aggregates all such information from sub frames.

The CL adds a unit test to verify that the InputMethod uses the correct
composition range
values for the active widget. A TODO is also left in
site_per_process_text_input_browsertest.cc
to come back and write an interactive test for tracking composition range for
sub frames
after the renderer side of IME is fixed.

BUG=578168, 602723
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[EhsanK, 2016-07-07 22:10:58]

ekaramad@chromium.org changed reviewers:
+ creis@chromium.org, kenrb@chromium.org

[EhsanK, 2016-07-07 22:10:58]

Please, take a look. Thanks!

https://codereview.chromium.org/2132633002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/text_input_manager.h (right):

https://codereview.chromium.org/2132633002/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_manager.h:19: namespace gfx {
Not quite sure if space is needed.

https://codereview.chromium.org/2132633002/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_manager.h:23: namespace content {
Same dilemma, but maybe a different case than above.

https://codereview.chromium.org/2132633002/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_manager.h:145: struct
CompositionRangeInfo {
We do not store gfx::Range for Aura since it was not used. But it is used for
Mac so I think it is best to have this as a struct.

[kenrb, 2016-07-08 18:13:42]

lgtm with nits

https://codereview.chromium.org/2132633002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/2132633002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.cc:430: #if
defined(USE_AURA)
I would add the TODO for removal on this guard, as you do for the other ones.

https://codereview.chromium.org/2132633002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/text_input_manager.h (right):

https://codereview.chromium.org/2132633002/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_manager.h:19: namespace gfx {
On 2016/07/07 22:10:58, EhsanK wrote:
> Not quite sure if space is needed.

It isn't.

https://codereview.chromium.org/2132633002/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_manager.h:23: namespace content {
On 2016/07/07 22:10:58, EhsanK wrote:
> Same dilemma, but maybe a different case than above.

A blank line here is appropriate.

https://codereview.chromium.org/2132633002/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_manager.h:78: const
std::vector<gfx::Rect>* GetCompositionCharacterBounds();
In RenderWidgetHostViewAura you call this without checking the return for
nullptr, but the implementation looks like it can return nullptr. Is returning a
value normally guaranteed?

Either way, it would be good to add to the comment saying when it can return
null.

[Charlie Reis, 2016-07-08 18:55:24]

Seems mostly good.  I'm a bit unclear on the state of the tests below-- can you
clarify?

https://codereview.chromium.org/2132633002/diff/60001/chrome/browser/renderer...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2132633002/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:519: //
tracked for all of the child frames.Composition range update could occur in
nit: Missing space between sentences.

https://codereview.chromium.org/2132633002/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:521: //
implemented for WebFrameWidgetImpl (crbug.com/602723).
This comment would be clearer if you started with the prerequisite for writing
the test.  (At first, it sounds like we could just write it now if we wanted
to.)  Maybe something like:

Once WebWidget::setComposition is implemented for WebFrameWidgetImpl, add a test
here to show that...

Also, is there a bug you can reference for that particular work, as opposed to
just the "add tests" bug?

And is there anything else you can test in this CL?  If there's nothing
observable, maybe we should be fixing WebFrameWidgetImpl first?

https://codereview.chromium.org/2132633002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/2132633002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4317: //
valid bound at index 0.
nit: Valid

[Charlie Reis, 2016-07-08 18:56:01]

[+site-isolation-reviews]

Also, please CC site-isolation-reviews on these CLs.  :)

[EhsanK, 2016-07-11 18:07:21]

Thank you for the reviews! Please, take another look.

https://codereview.chromium.org/2132633002/diff/60001/chrome/browser/renderer...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2132633002/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:519: //
tracked for all of the child frames.Composition range update could occur in
On 2016/07/08 18:55:24, Charlie Reis wrote:
> nit: Missing space between sentences.

Acknowledged.

https://codereview.chromium.org/2132633002/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:521: //
implemented for WebFrameWidgetImpl (crbug.com/602723).
On 2016/07/08 18:55:24, Charlie Reis wrote:
> This comment would be clearer if you started with the prerequisite for writing
> the test.  (At first, it sounds like we could just write it now if we wanted
> to.)  Maybe something like:
> 
> Once WebWidget::setComposition is implemented for WebFrameWidgetImpl, add a
test
> here to show that...
> 
> Also, is there a bug you can reference for that particular work, as opposed to
> just the "add tests" bug?
> 
> And is there anything else you can test in this CL?  If there's nothing
> observable, maybe we should be fixing WebFrameWidgetImpl first?

Fair enough. After giving it some more thought, I guess a dangling TODO does not
look really nice.

I did add the test I had in mind, but right now it is only on a page with one
frame (main frame). It passes because WebViewImpl takes care of IME for that. I
added a short TODO with a new bug number to say that we should change the test
after that bug is fixed.

We could also wait for the IME CL to land first. But I am not sure how long
would that be.

https://codereview.chromium.org/2132633002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/2132633002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4317: //
valid bound at index 0.
On 2016/07/08 18:55:24, Charlie Reis wrote:
> nit: Valid

Done.

https://codereview.chromium.org/2132633002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/2132633002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.cc:430: #if
defined(USE_AURA)
On 2016/07/08 18:13:42, kenrb wrote:
> I would add the TODO for removal on this guard, as you do for the other ones.

Acknowledged.

https://codereview.chromium.org/2132633002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/text_input_manager.h (right):

https://codereview.chromium.org/2132633002/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_manager.h:19: namespace gfx {
On 2016/07/08 18:13:42, kenrb wrote:
> On 2016/07/07 22:10:58, EhsanK wrote:
> > Not quite sure if space is needed.
> 
> It isn't.

Acknowledged.

https://codereview.chromium.org/2132633002/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_manager.h:23: namespace content {
On 2016/07/08 18:13:42, kenrb wrote:
> On 2016/07/07 22:10:58, EhsanK wrote:
> > Same dilemma, but maybe a different case than above.
> 
> A blank line here is appropriate.

Acknowledged.

https://codereview.chromium.org/2132633002/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_manager.h:78: const
std::vector<gfx::Rect>* GetCompositionCharacterBounds();
On 2016/07/08 18:13:42, kenrb wrote:
> In RenderWidgetHostViewAura you call this without checking the return for
> nullptr, but the implementation looks like it can return nullptr. Is returning
a
> value normally guaranteed?
> 
> Either way, it would be good to add to the comment saying when it can return
> null.

In RWHVAura we check if there is an active widget first, which guarantees this
will return something.

I refactored the comments by explaining above them that for these Get* methods
we will return nullptr if there is no active widgets.

[Charlie Reis, 2016-07-11 23:56:20]

Thanks, LGTM with nits.

https://codereview.chromium.org/2132633002/diff/60001/chrome/browser/renderer...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2132633002/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:521: //
implemented for WebFrameWidgetImpl (crbug.com/602723).
On 2016/07/11 18:07:21, EhsanK wrote:
> On 2016/07/08 18:55:24, Charlie Reis wrote:
> > This comment would be clearer if you started with the prerequisite for
writing
> > the test.  (At first, it sounds like we could just write it now if we wanted
> > to.)  Maybe something like:
> > 
> > Once WebWidget::setComposition is implemented for WebFrameWidgetImpl, add a
> test
> > here to show that...
> > 
> > Also, is there a bug you can reference for that particular work, as opposed
to
> > just the "add tests" bug?
> > 
> > And is there anything else you can test in this CL?  If there's nothing
> > observable, maybe we should be fixing WebFrameWidgetImpl first?
> 
> Fair enough. After giving it some more thought, I guess a dangling TODO does
not
> look really nice.
> 
> I did add the test I had in mind, but right now it is only on a page with one
> frame (main frame). It passes because WebViewImpl takes care of IME for that.
I
> added a short TODO with a new bug number to say that we should change the test
> after that bug is fixed.
> 
> We could also wait for the IME CL to land first. But I am not sure how long
> would that be.

Acknowledged.

https://codereview.chromium.org/2132633002/diff/80001/content/public/test/tex...
File content/public/test/text_input_test_utils.cc (right):

https://codereview.chromium.org/2132633002/diff/80001/content/public/test/tex...
content/public/test/text_input_test_utils.cc:26: 
nit: No blank line needed.

https://codereview.chromium.org/2132633002/diff/80001/content/public/test/tex...
content/public/test/text_input_test_utils.cc:28: }  // namespace ui
nit: No close comment.

https://codereview.chromium.org/2132633002/diff/80001/content/public/test/tex...
content/public/test/text_input_test_utils.cc:239: // We need the tab's view.
Please elaborate why this one needs to care about outer WebContentses when the
other methods in here don't.

https://codereview.chromium.org/2132633002/diff/80001/content/public/test/tex...
File content/public/test/text_input_test_utils.h (right):

https://codereview.chromium.org/2132633002/diff/80001/content/public/test/tex...
content/public/test/text_input_test_utils.h:15: 
nit: No blank line here, as in text_input_manager.h's gfx.

https://codereview.chromium.org/2132633002/diff/80001/content/public/test/tex...
content/public/test/text_input_test_utils.h:17: }  // namespace ui
nit: No close comment needed on something this simple.
There's other examples in content/public/test/content_browser_test_utils.h.

https://codereview.chromium.org/2132633002/diff/80001/content/public/test/tex...
content/public/test/text_input_test_utils.h:47: // and then uses that to send an
IPC to set composition to the currently
nit: s/uses/use/

[EhsanK, 2016-07-12 18:31:14]

Thanks Charlie! Please take another look. I will CQ after.

Thanks!

https://codereview.chromium.org/2132633002/diff/80001/content/public/test/tex...
File content/public/test/text_input_test_utils.cc (right):

https://codereview.chromium.org/2132633002/diff/80001/content/public/test/tex...
content/public/test/text_input_test_utils.cc:26: 
On 2016/07/11 23:56:19, Charlie Reis wrote:
> nit: No blank line needed.

Acknowledged.

https://codereview.chromium.org/2132633002/diff/80001/content/public/test/tex...
content/public/test/text_input_test_utils.cc:28: }  // namespace ui
On 2016/07/11 23:56:19, Charlie Reis wrote:
> nit: No close comment.

Acknowledged.

https://codereview.chromium.org/2132633002/diff/80001/content/public/test/tex...
content/public/test/text_input_test_utils.cc:239: // We need the tab's view.
On 2016/07/11 23:56:19, Charlie Reis wrote:
> Please elaborate why this one needs to care about outer WebContentses when the
> other methods in here don't.
I wanted to make sure the cast to RenderWidgetHostViewAura which follows on line
243 is valid (i.e., it is not a RenderWidgetHostViewChildFrame).

But anyhow, I changed the method to the new version which takes a
RenderWidgetHost and sends the IPC. This is more useful since it is not platform
specific anymore.

https://codereview.chromium.org/2132633002/diff/80001/content/public/test/tex...
File content/public/test/text_input_test_utils.h (right):

https://codereview.chromium.org/2132633002/diff/80001/content/public/test/tex...
content/public/test/text_input_test_utils.h:15: 
On 2016/07/11 23:56:19, Charlie Reis wrote:
> nit: No blank line here, as in text_input_manager.h's gfx.

Acknowledged.

https://codereview.chromium.org/2132633002/diff/80001/content/public/test/tex...
content/public/test/text_input_test_utils.h:17: }  // namespace ui
On 2016/07/11 23:56:19, Charlie Reis wrote:
> nit: No close comment needed on something this simple.
> There's other examples in content/public/test/content_browser_test_utils.h.

Acknowledged.

https://codereview.chromium.org/2132633002/diff/80001/content/public/test/tex...
content/public/test/text_input_test_utils.h:47: // and then uses that to send an
IPC to set composition to the currently
On 2016/07/11 23:56:19, Charlie Reis wrote:
> nit: s/uses/use/

Done.

[Charlie Reis, 2016-07-12 18:40:07]

Thanks, LGTM.

[EhsanK, 2016-07-12 19:16:19]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 19:17:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 21:52:26]

The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 21:52:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[EhsanK, 2016-07-12 22:15:12]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2016-07-12 22:15:13]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org
Link to the patchset: https://codereview.chromium.org/2132633002/#ps100001
(title: "Addressing creis@'s comments and changing a test method")

[commit-bot: I haz the power, 2016-07-12 22:17:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 01:21:50]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-13 01:22:13]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 01:24:18]

Description was changed from

==========
Tracking composition range on the browser side (Aura)

Currently, the composition range information which is sent by RenderWidget in
the IPC
InputHostMsg_ImeCompositionRangeChanged is not being used when the source is a
child frame.
This CL modifies the RenderWidgetHostViewBase class to report the composition
range to the
TextInputManager which aggregates all such information from sub frames.

The CL adds a unit test to verify that the InputMethod uses the correct
composition range
values for the active widget. A TODO is also left in
site_per_process_text_input_browsertest.cc
to come back and write an interactive test for tracking composition range for
sub frames
after the renderer side of IME is fixed.

BUG=578168, 602723
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Tracking composition range on the browser side (Aura)

Currently, the composition range information which is sent by RenderWidget in
the IPC
InputHostMsg_ImeCompositionRangeChanged is not being used when the source is a
child frame.
This CL modifies the RenderWidgetHostViewBase class to report the composition
range to the
TextInputManager which aggregates all such information from sub frames.

The CL adds a unit test to verify that the InputMethod uses the correct
composition range
values for the active widget. A TODO is also left in
site_per_process_text_input_browsertest.cc
to come back and write an interactive test for tracking composition range for
sub frames
after the renderer side of IME is fixed.

BUG=578168, 602723
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/b8e23a96c9c1d2933c57ba99aeac3199845fba99
Cr-Commit-Position: refs/heads/master@{#404873}
==========

[commit-bot: I haz the power, 2016-07-13 01:24:20]

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