Tracking text selection on the browser side in OOPIF.

Tracking text selection on the browser side in OOPIF.

This CL adds the required logic to track the text selection information
at the browser side. The text selection information is now stored in the
WebContent's TextInputManager (outermost WebContents). All
RenderWidgetHostViews as well as RenderWidgetHostViewAura
(TextInputClient) will retrieve the information for a specific view or
the active view from TextInputManager.

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

Committed: https://crrev.com/6f1786b887a51f3fcd3cdeacfd9b455934c570ca
Cr-Commit-Position: refs/heads/master@{#406937}

[EhsanK, 2016-07-08 19:15:33]

Description was changed from

==========
Tracking text selection on the browser side in OOPIF.

This CL adds the required logic to track the text selection information
at the browser side. The text selection information is now stored in the
WebContent's TextInputManager (outermost WebContents). All
RenderWidgetHostViews as well as RenderWidgetHostViewAura
(TextInputClient) will retrieve the information from a specific view or
the active view from TextInputManager.

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

to

==========
Tracking text selection on the browser side in OOPIF.

This CL adds the required logic to track the text selection information
at the browser side. The text selection information is now stored in the
WebContent's TextInputManager (outermost WebContents). All
RenderWidgetHostViews as well as RenderWidgetHostViewAura
(TextInputClient) will retrieve the information for a specific view or
the active view from TextInputManager.

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

[EhsanK, 2016-07-11 17:58:50]

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

[commit-bot: I haz the power, 2016-07-11 17:59:23]

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-11 18:47:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-11 18:47:33]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2016-07-11 20:31:48]

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

[EhsanK, 2016-07-11 20:34:21]

Charlie, Ken: I believe this should conclude most of the work for aura IME on
browser side. Please, take a look.

FYI, similar to the composition range CL, I cannot write a fully-fledged test
unless renderer side is fixed. I have added a test but the test verifies the
correctness of tracking the state for the main frame only.

[kenrb, 2016-07-12 19:15:25]

Looks good, just a couple of questions.

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

https://codereview.chromium.org/2130133004/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.cc:117: #if
defined(USE_AURA)
Shouldn't the SelectionChanged() override in RenderWidgetHostViewChildFrame be
removed?

https://codereview.chromium.org/2130133004/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2130133004/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:470: // The
non-const implementation of RenderWidgetHostView::GetSelectedText().
Why is this needed? There is only one caller, which can access the selected text
directly.

[EhsanK, 2016-07-12 19:24:50]

Description was changed from

==========
Tracking text selection on the browser side in OOPIF.

This CL adds the required logic to track the text selection information
at the browser side. The text selection information is now stored in the
WebContent's TextInputManager (outermost WebContents). All
RenderWidgetHostViews as well as RenderWidgetHostViewAura
(TextInputClient) will retrieve the information for a specific view or
the active view from TextInputManager.

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

to

==========
Tracking text selection on the browser side in OOPIF.

This CL adds the required logic to track the text selection information
at the browser side. The text selection information is now stored in the
WebContent's TextInputManager (outermost WebContents). All
RenderWidgetHostViews as well as RenderWidgetHostViewAura
(TextInputClient) will retrieve the information for a specific view or
the active view from TextInputManager.

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

[EhsanK, 2016-07-12 19:33:48]

Thanks Ken! Please take another look. Any suggestions on how to resolve the
const issue is much appreciated.

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

https://codereview.chromium.org/2130133004/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.cc:117: #if
defined(USE_AURA)
On 2016/07/12 19:15:25, kenrb wrote:
> Shouldn't the SelectionChanged() override in RenderWidgetHostViewChildFrame be
> removed?

Yes, definitely. Sorry it was missing in this patch. It is removed now.

https://codereview.chromium.org/2130133004/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2130133004/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:470: // The
non-const implementation of RenderWidgetHostView::GetSelectedText().
On 2016/07/12 19:15:25, kenrb wrote:
> Why is this needed? There is only one caller, which can access the selected
text
> directly.

The idea was to remove |selection_text_|, |selection_text_offset_|, and
|selection_range_| from this into TextInputManager. The only reason I have not
remove them yet is that Mac uses them so I will have to wait until I start
working on Mac code. Then these will be removed. So for this CL, I pretend they
do not exist and the state is only tracked at TextInputManager.

Now to implement RenderWidgetHostView::GetSelectedText(), I cannot simply call
GetTextInputManager() since GetSelectedText() is const but GetTextInputManager()
and TextInputManager::GetTextSelection are not. So I implemented this extra
method which is not const, and then I call it, after const_cast, from
GetSelectedText().

I am not particularly happy with this const_cast myself, but I could not come up
with a less messy way of getting around this problem.

[kenrb, 2016-07-12 20:15:18]

https://codereview.chromium.org/2130133004/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2130133004/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:470: // The
non-const implementation of RenderWidgetHostView::GetSelectedText().
On 2016/07/12 19:33:48, EhsanK wrote:
> On 2016/07/12 19:15:25, kenrb wrote:
> > Why is this needed? There is only one caller, which can access the selected
> text
> > directly.
> 
> The idea was to remove |selection_text_|, |selection_text_offset_|, and
> |selection_range_| from this into TextInputManager. The only reason I have not
> remove them yet is that Mac uses them so I will have to wait until I start
> working on Mac code. Then these will be removed. So for this CL, I pretend
they
> do not exist and the state is only tracked at TextInputManager.
> 
> Now to implement RenderWidgetHostView::GetSelectedText(), I cannot simply call
> GetTextInputManager() since GetSelectedText() is const but
GetTextInputManager()
> and TextInputManager::GetTextSelection are not. So I implemented this extra
> method which is not const, and then I call it, after const_cast, from
> GetSelectedText().
> 
> I am not particularly happy with this const_cast myself, but I could not come
up
> with a less messy way of getting around this problem.

Yeah, this isn't good. Basically it is trying to cheat on this method being
const.

It looks like TextInputManager::GetTextSelection() should be const to begin
with, which is part of the problem. GetTextInputManager() could almost be const,
but it can lazily create the TextInputManager. Can you make
GetTextInputManager() const if you make text_input_manager_ mutable? That seems
like the right way to do this.

[EhsanK, 2016-07-18 15:21:29]

PTAL.

https://codereview.chromium.org/2130133004/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2130133004/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:470: // The
non-const implementation of RenderWidgetHostView::GetSelectedText().
On 2016/07/12 20:15:17, kenrb wrote:
> On 2016/07/12 19:33:48, EhsanK wrote:
> > On 2016/07/12 19:15:25, kenrb wrote:
> > > Why is this needed? There is only one caller, which can access the
selected
> > text
> > > directly.
> > 
> > The idea was to remove |selection_text_|, |selection_text_offset_|, and
> > |selection_range_| from this into TextInputManager. The only reason I have
not
> > remove them yet is that Mac uses them so I will have to wait until I start
> > working on Mac code. Then these will be removed. So for this CL, I pretend
> they
> > do not exist and the state is only tracked at TextInputManager.
> > 
> > Now to implement RenderWidgetHostView::GetSelectedText(), I cannot simply
call
> > GetTextInputManager() since GetSelectedText() is const but
> GetTextInputManager()
> > and TextInputManager::GetTextSelection are not. So I implemented this extra
> > method which is not const, and then I call it, after const_cast, from
> > GetSelectedText().
> > 
> > I am not particularly happy with this const_cast myself, but I could not
come
> up
> > with a less messy way of getting around this problem.
> 
> Yeah, this isn't good. Basically it is trying to cheat on this method being
> const.
> 
> It looks like TextInputManager::GetTextSelection() should be const to begin
> with, which is part of the problem. GetTextInputManager() could almost be
const,
> but it can lazily create the TextInputManager. Can you make
> GetTextInputManager() const if you make text_input_manager_ mutable? That
seems
> like the right way to do this.

So basically we ended up removing const from this method altogether (in a
separate CL). So the new version does not have the same issue.

https://codereview.chromium.org/2130133004/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2130133004/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:3005: #if
defined(USE_X11) && !defined(OS_CHROMEOS)
This part could live in TextInputManager. But I thought since this is aura and
we don't have to keep it there, move it here.

I guess my mindset has been to avoid platform specific code in
RenderWidgetHostViewBase and TextInputManager. But if we have to have such code
(and we will), keep it inside TextInputManager.

https://codereview.chromium.org/2130133004/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/2130133004/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4480:
TEST_F(InputMethodStateAuraTest, GetSelectedText) {
I added this again since we still have a (now docile)
RenderWidgetHostView::GetSelectedText().

https://codereview.chromium.org/2130133004/diff/120001/content/public/test/te...
File content/public/test/text_input_test_utils.h (right):

https://codereview.chromium.org/2130133004/diff/120001/content/public/test/te...
content/public/test/text_input_test_utils.h:96: 
TextSelection could be for non-<input> but as far as I can tell we do not care
about them in IME. So I made this return true only when we have an
|active_view_|.

[kenrb, 2016-07-19 19:37:54]

lgtm

[Charlie Reis, 2016-07-20 23:00:30]

LGTM

https://codereview.chromium.org/2130133004/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/2130133004/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.cc:150: //
TODO(ekaramad): Use TextInputManager code paths for IME onother platforms.
nit: on other

https://codereview.chromium.org/2130133004/diff/160001/content/browser/render...
File content/browser/renderer_host/text_input_manager.h (right):

https://codereview.chromium.org/2130133004/diff/160001/content/browser/render...
content/browser/renderer_host/text_input_manager.h:106: // The following method
return the text selection state for the given |view|.
nit: returns

[EhsanK, 2016-07-21 16:52:55]

Thanks Charlie.

This CL will have to wait for the other one on initialization to land first and
then rebase this (+1 patch). Then I will need to repeat the same step by
initializing TextSelection for views and erasing it (+1 patch). I will commit
after that.

For reference, this is the CL which is landing before this one:
https://codereview.chromium.org/2170463002/

https://codereview.chromium.org/2130133004/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/2130133004/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.cc:150: //
TODO(ekaramad): Use TextInputManager code paths for IME onother platforms.
On 2016/07/20 23:00:30, Charlie Reis wrote:
> nit: on other

Done. Thanks!

https://codereview.chromium.org/2130133004/diff/160001/content/browser/render...
File content/browser/renderer_host/text_input_manager.h (right):

https://codereview.chromium.org/2130133004/diff/160001/content/browser/render...
content/browser/renderer_host/text_input_manager.h:106: // The following method
return the text selection state for the given |view|.
On 2016/07/20 23:00:30, Charlie Reis wrote:
> nit: returns

Done. Thanks!

[EhsanK, 2016-07-21 18:25:57]

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

[commit-bot: I haz the power, 2016-07-21 18:26:19]

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-21 18:26:20]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

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

Added proper initialization for TextSelection and made GetTextSelection const. I
will commit after dry-run.

Thanks!

[EhsanK, 2016-07-21 19:59:41]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2016-07-21 19:59:41]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2130133004/#ps240001
(title: "Initialize/Clear TextSelection for each view + make GetTextSelection()
const.")

[EhsanK, 2016-07-21 19:59:44]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2016-07-21 20:53:43]

The CQ bit was checked by ekaramad@chromium.org

[commit-bot: I haz the power, 2016-07-21 20:54:21]

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-21 20:54:24]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-21 21:11:24]

Committed patchset #13 (id:240001)

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

Description was changed from

==========
Tracking text selection on the browser side in OOPIF.

This CL adds the required logic to track the text selection information
at the browser side. The text selection information is now stored in the
WebContent's TextInputManager (outermost WebContents). All
RenderWidgetHostViews as well as RenderWidgetHostViewAura
(TextInputClient) will retrieve the information for a specific view or
the active view from TextInputManager.

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

to

==========
Tracking text selection on the browser side in OOPIF.

This CL adds the required logic to track the text selection information
at the browser side. The text selection information is now stored in the
WebContent's TextInputManager (outermost WebContents). All
RenderWidgetHostViews as well as RenderWidgetHostViewAura
(TextInputClient) will retrieve the information for a specific view or
the active view from TextInputManager.

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

Committed: https://crrev.com/6f1786b887a51f3fcd3cdeacfd9b455934c570ca
Cr-Commit-Position: refs/heads/master@{#406937}
==========

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

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