Tracking SelectionBounds for all RenderWidgets on the Browser Side (Aura Only)

Tracking SelectionBounds for all RenderWidgets on the browser side.

This patch implements the RenderWidgetHostViewBase::SelectionBoundsChanged as well as
RenderWidgetHostViewChildFrame::SelectionBoundsChanged and moves the logic inside the
RenderWidgetHostViewAura::SelectionBoundsChanged to the TextInputManager owned by the
WebContentsImpl. This patch also adds a test to verify that TextInputManager tracks
all the RWHVs selection bounds.

The same implementation will be required for Mac; but perhaps it requires slight changes
on the logic to store the state from ViewHostMsg_SelectionBounds_Params.

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

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

[EhsanK, 2016-06-09 21:14:35]

Description was changed from

==========
This patch implements the RenderWidgetHostViewBase::SelectionBoundsChanged as
well as
RenderWidgetHostViewChildFrame::SelectionBoundsChanged and moves the logic
inside the
RenderWidgetHostViewAura::SelectionBoundsChanged to the TextInputManager owned
by the
WebContentsImpl. This patch also adds a test to verify that TextInputManager
tracks
all the RWHVs selection bounds.

The same implementation will be required for Mac; but perhaps it requires slight
changes
on the logic to store the state from ViewHostMsg_SelectionBounds_Params.

BUG=578168
==========

to

==========
This patch implements the RenderWidgetHostViewBase::SelectionBoundsChanged as
well as
RenderWidgetHostViewChildFrame::SelectionBoundsChanged and moves the logic
inside the
RenderWidgetHostViewAura::SelectionBoundsChanged to the TextInputManager owned
by the
WebContentsImpl. This patch also adds a test to verify that TextInputManager
tracks
all the RWHVs selection bounds.

The same implementation will be required for Mac; but perhaps it requires slight
changes
on the logic to store the state from ViewHostMsg_SelectionBounds_Params.

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

[EhsanK, 2016-06-09 21:15:31]

Description was changed from

==========
This patch implements the RenderWidgetHostViewBase::SelectionBoundsChanged as
well as
RenderWidgetHostViewChildFrame::SelectionBoundsChanged and moves the logic
inside the
RenderWidgetHostViewAura::SelectionBoundsChanged to the TextInputManager owned
by the
WebContentsImpl. This patch also adds a test to verify that TextInputManager
tracks
all the RWHVs selection bounds.

The same implementation will be required for Mac; but perhaps it requires slight
changes
on the logic to store the state from ViewHostMsg_SelectionBounds_Params.

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

to

==========
Tracking SelectionBounds for all RenderWidgets on the browser side.

This patch implements the RenderWidgetHostViewBase::SelectionBoundsChanged as
well as
RenderWidgetHostViewChildFrame::SelectionBoundsChanged and moves the logic
inside the
RenderWidgetHostViewAura::SelectionBoundsChanged to the TextInputManager owned
by the
WebContentsImpl. This patch also adds a test to verify that TextInputManager
tracks
all the RWHVs selection bounds.

The same implementation will be required for Mac; but perhaps it requires slight
changes
on the logic to store the state from ViewHostMsg_SelectionBounds_Params.

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

[EhsanK, 2016-06-27 14:32:49]

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

[commit-bot: I haz the power, 2016-06-27 14:33:01]

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-06-27 16:15:37]

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

[commit-bot: I haz the power, 2016-06-27 16:15:38]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2016-06-27 17:18:13]

Hello Ken,

Could you please take a look? I have broken the IME related communication from
renderer->browser into smaller parts. This is one.

Thanks,

Ehsan

https://codereview.chromium.org/2057803002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/text_input_manager.cc (right):

https://codereview.chromium.org/2057803002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/text_input_manager.cc:87: void
TextInputManager::SelectionBoundsChanged(
This will change when we add the same logic for Mac. (some #ifdef and a few
lines for Mac). Android does not need this code path according to the comment on
RenderWidgetHostViewAndroid::SelectionBoundsChanged (cc file).

https://codereview.chromium.org/2057803002/diff/1/content/public/test/text_in...
File content/public/test/text_input_test_utils.cc (right):

https://codereview.chromium.org/2057803002/diff/1/content/public/test/text_in...
content/public/test/text_input_test_utils.cc:30:
base::Bind(&DefaultTextInputManagerObserverCallback);
This is to avoid crashes in the observers in
'site_per_process_text_input_browsertest.cc'. There are two callbacks and each
observer only needs one. So when the other one is not initialized, we get a
crash. The other alternative is to go back to having std::unique_ptr to
callbacks and only invoking them when they're not null.

[EhsanK, 2016-06-27 19:58:15]

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

[EhsanK, 2016-06-27 20:07:19]

On 2016/06/27 17:18:13, EhsanK wrote:
> Hello Ken,
> 
> Could you please take a look? I have broken the IME related communication from
> renderer->browser into smaller parts. This is one.
> 
> Thanks,
> 
> Ehsan
> 
>
https://codereview.chromium.org/2057803002/diff/1/content/browser/renderer_ho...
> File content/browser/renderer_host/text_input_manager.cc (right):
> 
>
https://codereview.chromium.org/2057803002/diff/1/content/browser/renderer_ho...
> content/browser/renderer_host/text_input_manager.cc:87: void
> TextInputManager::SelectionBoundsChanged(
> This will change when we add the same logic for Mac. (some #ifdef and a few
> lines for Mac). Android does not need this code path according to the comment
on
> RenderWidgetHostViewAndroid::SelectionBoundsChanged (cc file).
> 
>
https://codereview.chromium.org/2057803002/diff/1/content/public/test/text_in...
> File content/public/test/text_input_test_utils.cc (right):
> 
>
https://codereview.chromium.org/2057803002/diff/1/content/public/test/text_in...
> content/public/test/text_input_test_utils.cc:30:
> base::Bind(&DefaultTextInputManagerObserverCallback);
> This is to avoid crashes in the observers in
> 'site_per_process_text_input_browsertest.cc'. There are two callbacks and each
> observer only needs one. So when the other one is not initialized, we get a
> crash. The other alternative is to go back to having std::unique_ptr to
> callbacks and only invoking them when they're not null.

Sorry I forgot to actually add you with the previous message.

[kenrb, 2016-06-28 19:15:23]

Looks good: just some nits, plus as I mentioned offline, a suspicion that this
might be buggy on high DPI screens. Try running with
--force-device-scale-factor=2 (along with --site-per-process, of course) and see
if things position correctly.

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

https://codereview.chromium.org/2057803002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:184: //
This class observers the |expected_view| for the first change in its
nit: /s/observers/observes

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

https://codereview.chromium.org/2057803002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:146: // coordinate
system of the render view. |start_direction| and |end_direction|
nit: "render view" here is now more correctly "render widget".

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

https://codereview.chromium.org/2057803002/diff/40001/content/browser/rendere...
content/browser/renderer_host/text_input_manager.h:123:
ViewMap<gfx::SelectionBound> selection_focus_map_;
I'm not sure about this, but is it better to do it this way or to have a single
map, from RenderWidgetHostView to a struct containing all of the state for that
view? Selection anchor and selection focus bounds seem to be always accessed and
set together, at least, but TextInputState is get and set at different times
from those.

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

https://codereview.chromium.org/2057803002/diff/40001/content/public/test/tex...
content/public/test/text_input_test_utils.h:69: // i.e., selection bonuds or
TextInputState, with the TextInputManager that is
nit: typo 'bonuds'

[EhsanK, 2016-06-29 14:59:13]

Patchset #4 (id:60001) has been deleted

[EhsanK, 2016-06-29 17:09:57]

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

[EhsanK, 2016-06-29 17:09:58]

Thanks for the reviews!

PTAL.

Adding creis@.

creis@: Charlie, can you please take a look?

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

https://codereview.chromium.org/2057803002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:184: //
This class observers the |expected_view| for the first change in its
On 2016/06/28 19:15:23, kenrb wrote:
> nit: /s/observers/observes

Done.

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

https://codereview.chromium.org/2057803002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:146: // coordinate
system of the render view. |start_direction| and |end_direction|
On 2016/06/28 19:15:23, kenrb wrote:
> nit: "render view" here is now more correctly "render widget".

Acknowledged.

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

https://codereview.chromium.org/2057803002/diff/40001/content/browser/rendere...
content/browser/renderer_host/text_input_manager.h:123:
ViewMap<gfx::SelectionBound> selection_focus_map_;
On 2016/06/28 19:15:23, kenrb wrote:
> I'm not sure about this, but is it better to do it this way or to have a
single
> map, from RenderWidgetHostView to a struct containing all of the state for
that
> view? Selection anchor and selection focus bounds seem to be always accessed
and
> set together, at least, but TextInputState is get and set at different times
> from those.

I made the two selection related one a map into a pair of selection bounds. But
I might have to do a refactoring after finishing the code for MAC.

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

https://codereview.chromium.org/2057803002/diff/40001/content/public/test/tex...
content/public/test/text_input_test_utils.h:69: // i.e., selection bonuds or
TextInputState, with the TextInputManager that is
On 2016/06/28 19:15:23, kenrb wrote:
> nit: typo 'bonuds'

Done.

[EhsanK, 2016-06-29 18:36:43]

Updated patch.

https://codereview.chromium.org/2057803002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/2057803002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.h:590:
gfx::SelectionBound selection_focus_;
These are no longer needed as we track the state in TextInputManager.

[Charlie Reis, 2016-06-29 20:18:38]

It's not clear to me if this intentionally differs from the ImeCancel pattern or
not-- I've left some questions below.

https://codereview.chromium.org/2057803002/diff/120001/chrome/browser/rendere...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2057803002/diff/120001/chrome/browser/rendere...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:150:
ASSERT_EQ(tester(), text_input_manager_tester);
Should we have this check in the classes below?

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

https://codereview.chromium.org/2057803002/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.h:588: // The current
composition character bounds.
nit: Wrong indent.

https://codereview.chromium.org/2057803002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2057803002/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:271: //
(https://crbug.com/578168).
I'm just trying to follow the pattern-- does this TODO apply to
SelectionBoundsChanged?

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

https://codereview.chromium.org/2057803002/diff/120001/content/browser/render...
content/browser/renderer_host/text_input_manager.h:86: void
SelectionBoundsChanged(RenderWidgetHostViewBase* view,
Please add a comment about what selection bounds are.  It's a bit puzzling to me
that keyboard input triggers this, when it sounds like a gfx::Rect.

https://codereview.chromium.org/2057803002/diff/120001/content/browser/render...
content/browser/renderer_host/text_input_manager.h:135:
ViewMap<std::pair<gfx::SelectionBound, gfx::SelectionBound>>
I'm not a fan of using a map of pairs.  Can we have a struct rather than a pair
to make the members more explicit?

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

https://codereview.chromium.org/2057803002/diff/120001/content/public/test/te...
content/public/test/text_input_test_utils.cc:33:
base::Bind(&DefaultTextInputManagerObserverCallback);
We should not be adding new statically allocated objects.  There's
LAZY_INSTANCE_INITIALIZER if necessary, but can we avoid this?

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

https://codereview.chromium.org/2057803002/diff/120001/content/public/test/te...
content/public/test/text_input_test_utils.h:71: // i.e., selection bounds or
TextInputState, with the TextInputManager that is
Are these the only things that will trigger it, or do we need to keep adding
reasons?

[EhsanK, 2016-06-30 00:23:11]

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

[commit-bot: I haz the power, 2016-06-30 00:24:42]

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

[EhsanK, 2016-06-30 00:24:45]

Thanks for the reviews.

Charlie: Need your feedback on:
1- Use of LazyInstance/keeping ptr to callbacks?
2- Using GetUpdatedView() when there are mutiple things we update a view with?
Do you want me to define specific methods/members for them?

Thanks!

https://codereview.chromium.org/2057803002/diff/120001/chrome/browser/rendere...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2057803002/diff/120001/chrome/browser/rendere...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:150:
ASSERT_EQ(tester(), text_input_manager_tester);
On 2016/06/29 20:18:37, Charlie Reis wrote:
> Should we have this check in the classes below?
I don't think this check can even hit.

1- tester() is owned by this class.
2- An internal observer which calls VerifyChange as a callback is owned solely
by the tester().
3- Therefore, there is no way tester() != text_input_manager_tester. 

I should remove the argument from all methods.

The original reason behind having this argument was that I was initially using
static methods as callbacks and needed this to get information to verify if I
should stop wait. I can't remember why ASSERT_EQ however? Maybe just extra
caution which now seems unnecessary.

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

https://codereview.chromium.org/2057803002/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.h:588: // The current
composition character bounds.
On 2016/06/29 20:18:37, Charlie Reis wrote:
> nit: Wrong indent.

Sorry. Done.

https://codereview.chromium.org/2057803002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2057803002/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:271: //
(https://crbug.com/578168).
On 2016/06/29 20:18:37, Charlie Reis wrote:
> I'm just trying to follow the pattern-- does this TODO apply to
> SelectionBoundsChanged?

Yes. I actually moved the method down here with the rest of them. Added the TODO
as well although it is starting to look very redundant.

So basically these information are to be collected and used again by tab's view
for IME purposes. Similar to TextInputStateChanged.

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

https://codereview.chromium.org/2057803002/diff/120001/content/browser/render...
content/browser/renderer_host/text_input_manager.h:86: void
SelectionBoundsChanged(RenderWidgetHostViewBase* view,
On 2016/06/29 20:18:37, Charlie Reis wrote:
> Please add a comment about what selection bounds are.  It's a bit puzzling to
me
> that keyboard input triggers this, when it sounds like a gfx::Rect.

Good point.

This is not necessarily triggered by keyboard:
https://cs.chromium.org/chromium/src/content/renderer/render_widget.cc?sq=pac...

In aura, we use the selection bounds in determine the caret position for IME:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...
But there we use the type ui::SelectionBound which seems more relevant than
gfx::Rect. But why the bounds in IPC are of type gfx::Rect, I am not sure. Maybe
because InputMethod on ui/ side asks for a rect so initially they were
gfx::Rect? 

But apparently there has been movements in removing this IPC and use compositor
meta data. But IME (specifically for <webview>) has been a blocker for that. See
here:
https://cs.chromium.org/chromium/src/content/renderer/render_widget.cc?rcl=0&...

The rest of selection as I understand get updates through some compositing meta
data. That is as much as I could dig :P.

Finally, this update is only used by Mac and Aura.

https://codereview.chromium.org/2057803002/diff/120001/content/browser/render...
content/browser/renderer_host/text_input_manager.h:135:
ViewMap<std::pair<gfx::SelectionBound, gfx::SelectionBound>>
On 2016/06/29 20:18:37, Charlie Reis wrote:
> I'm not a fan of using a map of pairs.  Can we have a struct rather than a
pair
> to make the members more explicit?

Acknowledged.

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

https://codereview.chromium.org/2057803002/diff/120001/content/public/test/te...
content/public/test/text_input_test_utils.cc:33:
base::Bind(&DefaultTextInputManagerObserverCallback);
On 2016/06/29 20:18:37, Charlie Reis wrote:
> We should not be adding new statically allocated objects.  There's
> LAZY_INSTANCE_INITIALIZER if necessary, but can we avoid this?
I understand.

In initial patches of TextInputManager we had std::unique_ptr to callbacks which
would allow us to do:

if (callback_) callback_->Run()

and avoid the use of a static default fallback value. But we later changed that
along the review process. Would you suggest we use that idea?

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

https://codereview.chromium.org/2057803002/diff/120001/content/public/test/te...
content/public/test/text_input_test_utils.h:71: // i.e., selection bounds or
TextInputState, with the TextInputManager that is
On 2016/06/29 20:18:37, Charlie Reis wrote:
> Are these the only things that will trigger it, or do we need to keep adding
> reasons?

We might keep adding for similar additions:
RenderWidgetHostViewBase::SelectionChanged
RenderWidgetHostViewBase::ImeCompositionRangeChanged
are similar changes for their application in IME.

[commit-bot: I haz the power, 2016-06-30 00:41:35]

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

[commit-bot: I haz the power, 2016-06-30 00:41:36]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Charlie Reis, 2016-06-30 20:27:21]

Thanks-- some thoughts below.

https://codereview.chromium.org/2057803002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2057803002/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:271: //
(https://crbug.com/578168).
On 2016/06/30 00:24:45, EhsanK wrote:
> On 2016/06/29 20:18:37, Charlie Reis wrote:
> > I'm just trying to follow the pattern-- does this TODO apply to
> > SelectionBoundsChanged?
> 
> Yes. I actually moved the method down here with the rest of them. Added the
TODO
> as well although it is starting to look very redundant.

Feel free to create a separate section with the TODO for these 3 methods (and
any others as you migrate them).  Then you don't need the TODO on each one.

> 
> So basically these information are to be collected and used again by tab's
view
> for IME purposes. Similar to TextInputStateChanged.

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

https://codereview.chromium.org/2057803002/diff/120001/content/public/test/te...
content/public/test/text_input_test_utils.cc:33:
base::Bind(&DefaultTextInputManagerObserverCallback);
On 2016/06/30 00:24:45, EhsanK wrote:
> On 2016/06/29 20:18:37, Charlie Reis wrote:
> > We should not be adding new statically allocated objects.  There's
> > LAZY_INSTANCE_INITIALIZER if necessary, but can we avoid this?
> I understand.
> 
> In initial patches of TextInputManager we had std::unique_ptr to callbacks
which
> would allow us to do:
> 
> if (callback_) callback_->Run()
> 
> and avoid the use of a static default fallback value. But we later changed
that
> along the review process. Would you suggest we use that idea?

I don't remember the objections to that approach, but it sounds reasonable if we
don't always have callbacks to run.  (It's also possible to keep a no-op
callback within InternalObserver rather than making it a global, so you can
decide which one looks nicer.)

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

https://codereview.chromium.org/2057803002/diff/120001/content/public/test/te...
content/public/test/text_input_test_utils.h:71: // i.e., selection bounds or
TextInputState, with the TextInputManager that is
On 2016/06/30 00:24:45, EhsanK wrote:
> On 2016/06/29 20:18:37, Charlie Reis wrote:
> > Are these the only things that will trigger it, or do we need to keep adding
> > reasons?
> 
> We might keep adding for similar additions:
> RenderWidgetHostViewBase::SelectionChanged
> RenderWidgetHostViewBase::ImeCompositionRangeChanged
> are similar changes for their application in IME.

Updating the value on any of those seems reasonable.  That said, I'm not sure if
it's useful to have an exhaustive list in this comment, since it's likely to get
stale.  Changing this to "most recently updated any of its state (e.g.,
TextInputState or otherwise)" might be sufficient.

https://codereview.chromium.org/2057803002/diff/160001/chrome/browser/rendere...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2057803002/diff/160001/chrome/browser/rendere...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:333:
LOG(INFO) << "View type created";
Did you mean to leave these in?

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

https://codereview.chromium.org/2057803002/diff/160001/content/browser/render...
content/browser/renderer_host/text_input_manager.h:147: // is selection anchor
and the second is selection focus.
nit: Stale comment.

[EhsanK, 2016-06-30 21:01:15]

Thanks Charlie!

PTAL.

https://codereview.chromium.org/2057803002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2057803002/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:271: //
(https://crbug.com/578168).
On 2016/06/30 20:27:21, Charlie Reis wrote:
> On 2016/06/30 00:24:45, EhsanK wrote:
> > On 2016/06/29 20:18:37, Charlie Reis wrote:
> > > I'm just trying to follow the pattern-- does this TODO apply to
> > > SelectionBoundsChanged?
> > 
> > Yes. I actually moved the method down here with the rest of them. Added the
> TODO
> > as well although it is starting to look very redundant.
> 
> Feel free to create a separate section with the TODO for these 3 methods (and
> any others as you migrate them).  Then you don't need the TODO on each one.
> 
> > 
> > So basically these information are to be collected and used again by tab's
> view
> > for IME purposes. Similar to TextInputStateChanged.

Acknowledged.

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

https://codereview.chromium.org/2057803002/diff/120001/content/public/test/te...
content/public/test/text_input_test_utils.cc:33:
base::Bind(&DefaultTextInputManagerObserverCallback);
On 2016/06/30 20:27:21, Charlie Reis wrote:
> On 2016/06/30 00:24:45, EhsanK wrote:
> > On 2016/06/29 20:18:37, Charlie Reis wrote:
> > > We should not be adding new statically allocated objects.  There's
> > > LAZY_INSTANCE_INITIALIZER if necessary, but can we avoid this?
> > I understand.
> > 
> > In initial patches of TextInputManager we had std::unique_ptr to callbacks
> which
> > would allow us to do:
> > 
> > if (callback_) callback_->Run()
> > 
> > and avoid the use of a static default fallback value. But we later changed
> that
> > along the review process. Would you suggest we use that idea?
> 
> I don't remember the objections to that approach, but it sounds reasonable if
we
> don't always have callbacks to run.  (It's also possible to keep a no-op
> callback within InternalObserver rather than making it a global, so you can
> decide which one looks nicer.)

In that sense, I think it might look more readable to keep unique_ptr and avoid
the no-op method altogether.

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

https://codereview.chromium.org/2057803002/diff/120001/content/public/test/te...
content/public/test/text_input_test_utils.h:71: // i.e., selection bounds or
TextInputState, with the TextInputManager that is
On 2016/06/30 20:27:21, Charlie Reis wrote:
> On 2016/06/30 00:24:45, EhsanK wrote:
> > On 2016/06/29 20:18:37, Charlie Reis wrote:
> > > Are these the only things that will trigger it, or do we need to keep
adding
> > > reasons?
> > 
> > We might keep adding for similar additions:
> > RenderWidgetHostViewBase::SelectionChanged
> > RenderWidgetHostViewBase::ImeCompositionRangeChanged
> > are similar changes for their application in IME.
> 
> Updating the value on any of those seems reasonable.  That said, I'm not sure
if
> it's useful to have an exhaustive list in this comment, since it's likely to
get
> stale.  Changing this to "most recently updated any of its state (e.g.,
> TextInputState or otherwise)" might be sufficient.

Acknowledged.

https://codereview.chromium.org/2057803002/diff/160001/chrome/browser/rendere...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2057803002/diff/160001/chrome/browser/rendere...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:333:
LOG(INFO) << "View type created";
On 2016/06/30 20:27:21, Charlie Reis wrote:
> Did you mean to leave these in?

No sorry for these. I will remove them all.

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

https://codereview.chromium.org/2057803002/diff/160001/content/browser/render...
content/browser/renderer_host/text_input_manager.h:147: // is selection anchor
and the second is selection focus.
On 2016/06/30 20:27:21, Charlie Reis wrote:
> nit: Stale comment.

Acknowledged.

[Charlie Reis, 2016-06-30 21:16:14]

LGTM once Ken approves.  (Did you test the device-scale-factor thing he
mentioned?)

https://codereview.chromium.org/2057803002/diff/180001/chrome/browser/rendere...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(left):

https://codereview.chromium.org/2057803002/diff/180001/chrome/browser/rendere...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:322: 
nit: Better to leave this blank line in place.

[EhsanK, 2016-06-30 21:50:36]

ekaramad@chromium.org changed reviewers:
+ sky@chromium.org

[EhsanK, 2016-06-30 21:50:37]

Thank you, Charlie!


I tested it locally and it looked fine. Also, this might be worth checking:
https://cs.chromium.org/chromium/src/content/renderer/render_widget.cc?rcl=0&...
where apparently in the conversion we take into account the "original device
scale factor".

kenrb@: Could you please take another look.

Also, adding sky@ for owner reviews on the chrome/* test file. Thanks!

[sky, 2016-07-01 00:03:49]

I believe you mean content/public/test not chrome/test.

https://codereview.chromium.org/2057803002/diff/200001/content/public/test/te...
File content/public/test/text_input_test_utils.cc (right):

https://codereview.chromium.org/2057803002/diff/200001/content/public/test/te...
content/public/test/text_input_test_utils.cc:97:
std::unique_ptr<TextInputManagerTester::Callback>
You shouldn't need unique_ptrs for these. Instead use is_null to test if valid.

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

https://codereview.chromium.org/2057803002/diff/200001/content/public/test/te...
content/public/test/text_input_test_utils.h:45: using Callback = base::Closure;
nit: this using is a bit overkill now, just use base::Closure, it isn't much
longer.

[EhsanK, 2016-07-01 00:29:57]

sky@: Thank you very much for the reviews! I actually meant tests including
chrome/browser/renderer_host/site_per_process_text_input_manager_browsertest.cc.

PTAL.

https://codereview.chromium.org/2057803002/diff/200001/content/public/test/te...
File content/public/test/text_input_test_utils.cc (right):

https://codereview.chromium.org/2057803002/diff/200001/content/public/test/te...
content/public/test/text_input_test_utils.cc:97:
std::unique_ptr<TextInputManagerTester::Callback>
On 2016/07/01 00:03:49, sky wrote:
> You shouldn't need unique_ptrs for these. Instead use is_null to test if
valid.

Thanks! This is neat.

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

https://codereview.chromium.org/2057803002/diff/200001/content/public/test/te...
content/public/test/text_input_test_utils.h:45: using Callback = base::Closure;
On 2016/07/01 00:03:49, sky wrote:
> nit: this using is a bit overkill now, just use base::Closure, it isn't much
> longer.

Acknowledged.

[sky, 2016-07-01 16:13:40]

Charlie should be an owner of
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc . Please
update OWNERS in this patch and I'll approve it.

[kenrb, 2016-07-04 18:51:30]

lgtm

[EhsanK, 2016-07-04 18:59:15]

Thanks for the reviews!

sky@: Added creis@ as the owner of this file.

[EhsanK, 2016-07-04 20:13:58]

Small refactoring change to RWHVCF and TextInputManager. PTAL.

[sky, 2016-07-06 15:55:00]

OWNERS change LGTM

[Charlie Reis, 2016-07-06 18:42:36]

Thanks, LGTM with nits.

https://codereview.chromium.org/2057803002/diff/300001/chrome/browser/rendere...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2057803002/diff/300001/chrome/browser/rendere...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:188:
explicit ViewSelectionBoundsChangedObserver(
nit: No explicit unless you have exactly one argument.

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

https://codereview.chromium.org/2057803002/diff/300001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4287: //
Calculate the bounds
nit: Blank line before, and end with period.

https://codereview.chromium.org/2057803002/diff/300001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4297:
EXPECT_EQ(text_input_client()->GetCaretBounds(), measured_rect);
I'm not sure which one is the expected value and which one is the actual, but it
should be EXPECT_EQ(expected, actual) to have legible test output when it fails.
 Is this backwards?

https://codereview.chromium.org/2057803002/diff/300001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2057803002/diff/300001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:266: // Notifies
the View that the renderer selection bounds has changed.
nit: view

https://codereview.chromium.org/2057803002/diff/300001/content/browser/render...
File content/browser/renderer_host/text_input_manager.cc (right):

https://codereview.chromium.org/2057803002/diff/300001/content/browser/render...
content/browser/renderer_host/text_input_manager.cc:101: // TODO(ekaramad):
Implement the logic for other platforms (crbug.com/578168).
nit: Blank line before.

[EhsanK, 2016-07-06 19:15:35]

Thank you all for reviewing this patch! FYI, I will commit after successful
dry-run.

https://codereview.chromium.org/2057803002/diff/300001/chrome/browser/rendere...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2057803002/diff/300001/chrome/browser/rendere...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:188:
explicit ViewSelectionBoundsChangedObserver(
On 2016/07/06 18:42:36, Charlie Reis wrote:
> nit: No explicit unless you have exactly one argument.

Acknowledged.

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

https://codereview.chromium.org/2057803002/diff/300001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4287: //
Calculate the bounds
On 2016/07/06 18:42:36, Charlie Reis wrote:
> nit: Blank line before, and end with period.

Done.

https://codereview.chromium.org/2057803002/diff/300001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4297:
EXPECT_EQ(text_input_client()->GetCaretBounds(), measured_rect);
On 2016/07/06 18:42:36, Charlie Reis wrote:
> I'm not sure which one is the expected value and which one is the actual, but
it
> should be EXPECT_EQ(expected, actual) to have legible test output when it
fails.
>  Is this backwards?

Hmm. I think it is fair to say that |measured_rect| is expected so yes, this is
backwards. Thanks!

https://codereview.chromium.org/2057803002/diff/300001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2057803002/diff/300001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:266: // Notifies
the View that the renderer selection bounds has changed.
On 2016/07/06 18:42:36, Charlie Reis wrote:
> nit: view

Done. I believe this is some older style as I inherited it from original
comment. This happens on some other comments as well.

https://codereview.chromium.org/2057803002/diff/300001/content/browser/render...
File content/browser/renderer_host/text_input_manager.cc (right):

https://codereview.chromium.org/2057803002/diff/300001/content/browser/render...
content/browser/renderer_host/text_input_manager.cc:101: // TODO(ekaramad):
Implement the logic for other platforms (crbug.com/578168).
On 2016/07/06 18:42:36, Charlie Reis wrote:
> nit: Blank line before.

Done.

[EhsanK, 2016-07-06 19:15:43]

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

[commit-bot: I haz the power, 2016-07-06 19:16:21]

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-06 23:28:56]

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

[commit-bot: I haz the power, 2016-07-06 23:28:57]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[EhsanK, 2016-07-07 00:53:21]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2016-07-07 00:53:22]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
creis@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2057803002/#ps320001
(title: "Addressing creis@'s comments")

[commit-bot: I haz the power, 2016-07-07 00:54:02]

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

[EhsanK, 2016-07-07 01:23:02]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2016-07-07 01:23:24]

Description was changed from

==========
Tracking SelectionBounds for all RenderWidgets on the browser side.

This patch implements the RenderWidgetHostViewBase::SelectionBoundsChanged as
well as
RenderWidgetHostViewChildFrame::SelectionBoundsChanged and moves the logic
inside the
RenderWidgetHostViewAura::SelectionBoundsChanged to the TextInputManager owned
by the
WebContentsImpl. This patch also adds a test to verify that TextInputManager
tracks
all the RWHVs selection bounds.

The same implementation will be required for Mac; but perhaps it requires slight
changes
on the logic to store the state from ViewHostMsg_SelectionBounds_Params.

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

to

==========
Tracking SelectionBounds for all RenderWidgets on the browser side.

This patch implements the RenderWidgetHostViewBase::SelectionBoundsChanged as
well as
RenderWidgetHostViewChildFrame::SelectionBoundsChanged and moves the logic
inside the
RenderWidgetHostViewAura::SelectionBoundsChanged to the TextInputManager owned
by the
WebContentsImpl. This patch also adds a test to verify that TextInputManager
tracks
all the RWHVs selection bounds.

The same implementation will be required for Mac; but perhaps it requires slight
changes
on the logic to store the state from ViewHostMsg_SelectionBounds_Params.

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

[EhsanK, 2016-07-07 01:23:34]

The CQ bit was checked by ekaramad@chromium.org

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

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-07 02:45:06]

Description was changed from

==========
Tracking SelectionBounds for all RenderWidgets on the browser side.

This patch implements the RenderWidgetHostViewBase::SelectionBoundsChanged as
well as
RenderWidgetHostViewChildFrame::SelectionBoundsChanged and moves the logic
inside the
RenderWidgetHostViewAura::SelectionBoundsChanged to the TextInputManager owned
by the
WebContentsImpl. This patch also adds a test to verify that TextInputManager
tracks
all the RWHVs selection bounds.

The same implementation will be required for Mac; but perhaps it requires slight
changes
on the logic to store the state from ViewHostMsg_SelectionBounds_Params.

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

to

==========
Tracking SelectionBounds for all RenderWidgets on the browser side.

This patch implements the RenderWidgetHostViewBase::SelectionBoundsChanged as
well as
RenderWidgetHostViewChildFrame::SelectionBoundsChanged and moves the logic
inside the
RenderWidgetHostViewAura::SelectionBoundsChanged to the TextInputManager owned
by the
WebContentsImpl. This patch also adds a test to verify that TextInputManager
tracks
all the RWHVs selection bounds.

The same implementation will be required for Mac; but perhaps it requires slight
changes
on the logic to store the state from ViewHostMsg_SelectionBounds_Params.

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

[commit-bot: I haz the power, 2016-07-07 02:45:07]

Committed patchset #16 (id:320001)

[commit-bot: I haz the power, 2016-07-07 02:48:14]

Description was changed from

==========
Tracking SelectionBounds for all RenderWidgets on the browser side.

This patch implements the RenderWidgetHostViewBase::SelectionBoundsChanged as
well as
RenderWidgetHostViewChildFrame::SelectionBoundsChanged and moves the logic
inside the
RenderWidgetHostViewAura::SelectionBoundsChanged to the TextInputManager owned
by the
WebContentsImpl. This patch also adds a test to verify that TextInputManager
tracks
all the RWHVs selection bounds.

The same implementation will be required for Mac; but perhaps it requires slight
changes
on the logic to store the state from ViewHostMsg_SelectionBounds_Params.

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

to

==========
Tracking SelectionBounds for all RenderWidgets on the browser side.

This patch implements the RenderWidgetHostViewBase::SelectionBoundsChanged as
well as
RenderWidgetHostViewChildFrame::SelectionBoundsChanged and moves the logic
inside the
RenderWidgetHostViewAura::SelectionBoundsChanged to the TextInputManager owned
by the
WebContentsImpl. This patch also adds a test to verify that TextInputManager
tracks
all the RWHVs selection bounds.

The same implementation will be required for Mac; but perhaps it requires slight
changes
on the logic to store the state from ViewHostMsg_SelectionBounds_Params.

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

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

[commit-bot: I haz the power, 2016-07-07 02:48:16]

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