Respect the order of input messages from browser to renderer

Respect the order of input messages from browser to renderer

When we call InputConnection#setSelection and other methods together,
the order sometimes does not get kept. The reason is that
SetEditableSelectionOffsets is a frame message while all the others are
input messages, and they go to different message queues.

In addition, RenderViewImpl is the only who registers its routing ID
through InputHandlerManager::AddInputHandler() call. RenderFrameImpl,
on the other hand, does not register itself.

Once routing ID is registered, InputEventFilter will post input messages to
compositor impl thread first (such that scrolling can happen inside
compositor impl thread and keep in line with other input message handling
order).

We fix this by
1) Changing SetEditableSelectionOffsets from FrameMsg to InputMsg.
  (Now there is no remaining frame messages in ime_adapter_android.cc.)
2) Change DidAddInputHandler() and DidRemoveInputHandler() names to
  RegisterRoutingID() and UnregisterRoutingID(), respectively.
3) Register RenderFrame's routing ID at InputEventFilter.

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

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

[Changwan Ryu, 2016-04-15 00:21:53]

Description was changed from

==========
Respect the order of input messages from browser to renderer

When we call InputConnection#setSelection and other methods together,
the order sometimes does not get kept. The reason is that
SetEditableSelectionOffsets is a frame message while all the others are
input messages, and they go to different message queues.

In addition, RenderViewImpl is the only who registers its routing ID
through InputHandlerManager::AddInputHandler() call. RenderFrameImpl,
on the other hand, does not register itself.

Once routing ID is registered, InputEventFilter will post input messages to
compositor impl thread first (such that scrolling can happen inside
compositor impl thread and keep in line with other input message handling
order).

We fix this by
1) Changing SetEditableSelectionOffsets from FrameMsg to InputMsg.
(Now there is no remaining frame messages in ime_adapter_android.cc.)
2) Lifting the routing ID check from InputEventFilter.

BUG=601707
==========

to

==========
Respect the order of input messages from browser to renderer

When we call InputConnection#setSelection and other methods together,
the order sometimes does not get kept. The reason is that
SetEditableSelectionOffsets is a frame message while all the others are
input messages, and they go to different message queues.

In addition, RenderViewImpl is the only who registers its routing ID
through InputHandlerManager::AddInputHandler() call. RenderFrameImpl,
on the other hand, does not register itself.

Once routing ID is registered, InputEventFilter will post input messages to
compositor impl thread first (such that scrolling can happen inside
compositor impl thread and keep in line with other input message handling
order).

We fix this by
1) Changing SetEditableSelectionOffsets from FrameMsg to InputMsg.
(Now there is no remaining frame messages in ime_adapter_android.cc.)
2) Lifting the routing ID check from InputEventFilter.

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

[Changwan Ryu, 2016-04-15 00:27:03]

Description was changed from

==========
Respect the order of input messages from browser to renderer

When we call InputConnection#setSelection and other methods together,
the order sometimes does not get kept. The reason is that
SetEditableSelectionOffsets is a frame message while all the others are
input messages, and they go to different message queues.

In addition, RenderViewImpl is the only who registers its routing ID
through InputHandlerManager::AddInputHandler() call. RenderFrameImpl,
on the other hand, does not register itself.

Once routing ID is registered, InputEventFilter will post input messages to
compositor impl thread first (such that scrolling can happen inside
compositor impl thread and keep in line with other input message handling
order).

We fix this by
1) Changing SetEditableSelectionOffsets from FrameMsg to InputMsg.
(Now there is no remaining frame messages in ime_adapter_android.cc.)
2) Lifting the routing ID check from InputEventFilter.

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

to

==========
Respect the order of input messages from browser to renderer

When we call InputConnection#setSelection and other methods together,
the order sometimes does not get kept. The reason is that
SetEditableSelectionOffsets is a frame message while all the others are
input messages, and they go to different message queues.

In addition, RenderViewImpl is the only who registers its routing ID
through InputHandlerManager::AddInputHandler() call. RenderFrameImpl,
on the other hand, does not register itself.

Once routing ID is registered, InputEventFilter will post input messages to
compositor impl thread first (such that scrolling can happen inside
compositor impl thread and keep in line with other input message handling
order).

We fix this by
1) Changing SetEditableSelectionOffsets from FrameMsg to InputMsg.
  (Now there is no remaining frame messages in ime_adapter_android.cc.)
2) Change DidAddInputHandler() and DidRemoveInputHandler() names to
  RegisterRoutingID() and UnregisterRoutingID(), respectively.
3) Register RenderFrame's routing ID at InputEventFilter.

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

[Changwan Ryu, 2016-04-15 00:27:03]

changwan@chromium.org changed reviewers:
+ aelias@chromium.org

[aelias_OOO_until_Jul13, 2016-04-15 00:41:52]

aelias@chromium.org changed reviewers:
+ tdresser@chromium.org

[aelias_OOO_until_Jul13, 2016-04-15 00:42:49]

lgtm, but please wait for tdresser@ to double-check the routing ID change in
content/renderer/input/input_handler_manager.cc before landing.

[tdresser, 2016-04-18 14:12:25]

content/renderer/input/input_handler_manager.cc LGTM

[Changwan Ryu, 2016-04-19 04:58:08]

changwan@chromium.org changed reviewers:
+ sievers@chromium.org

[Changwan Ryu, 2016-04-19 04:58:09]

sievers@, could you take a look?

[Changwan Ryu, 2016-04-19 05:00:14]

changwan@chromium.org changed reviewers:
+ palmer@chromium.org

[Changwan Ryu, 2016-04-19 05:00:14]

palmer@chromium.org: Please review changes in content/common/.

[no sievers, 2016-04-19 21:16:16]

sievers@chromium.org changed reviewers:
+ dcheng@chromium.org, nasko@chromium.org, rockot@chromium.org

[no sievers, 2016-04-19 21:27:50]

This is subtle, and overall it looks like we are (and probably have been) using
the input event filtering code not quite along the lines of its original
intention.

So to order Android's SetEditableSelectionOffsets with other input messages sent
to RenderViewImpl and RenderFrame, you are
- making it an InputMsg (so the InputEventFilter filters it on the IO thread at
all)
- bouncing InputMsg_* handled by RenderFrameImpl through the compositor thread,
although they are never handled there

That means InputMsg_* are now all making the same thread hops, and maybe that
guarantees ordering. And maybe that also seems reasonable that all messages in
this class are ordered, not sure, there's a lot of them. Also if we wanted to
achieve that, I'm wondering if it's better to tweak the InputEventFilter to
simply consider the msg class only (InputMsg) instead of needing to register
route ids. Because code might move around again and things will break in subtle
ways.

Also at the same time your patch changes ordering of other messages sent to
RenderFrame vs. InputMsgs going there, and there's a lot of them (most of them
are going there).

+nasko, dcheng to comment if that's safe.

+rockot to comment on ordering between messages for RenderFrame and RenderView
and input in the Mojo world, while we're at it. To make sure this is the right
direction.

[nasko, 2016-04-20 17:31:46]

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

[nasko, 2016-04-20 17:31:47]

kenrb@ is our expert on input events, so I'll defer to him for reviewing this.

[kenrb, 2016-04-20 20:47:07]

On 2016/04/19 21:27:50, sievers wrote:
> This is subtle, and overall it looks like we are (and probably have been)
using
> the input event filtering code not quite along the lines of its original
> intention.
> 
> So to order Android's SetEditableSelectionOffsets with other input messages
sent
> to RenderViewImpl and RenderFrame, you are
> - making it an InputMsg (so the InputEventFilter filters it on the IO thread
at
> all)
> - bouncing InputMsg_* handled by RenderFrameImpl through the compositor
thread,
> although they are never handled there
> 
> That means InputMsg_* are now all making the same thread hops, and maybe that
> guarantees ordering. And maybe that also seems reasonable that all messages in
> this class are ordered, not sure, there's a lot of them. Also if we wanted to
> achieve that, I'm wondering if it's better to tweak the InputEventFilter to
> simply consider the msg class only (InputMsg) instead of needing to register
> route ids. Because code might move around again and things will break in
subtle
> ways.
> 
> Also at the same time your patch changes ordering of other messages sent to
> RenderFrame vs. InputMsgs going there, and there's a lot of them (most of them
> are going there).
> 
> +nasko, dcheng to comment if that's safe.
> 

That shouldn't cause any problems. I wonder, though, is it necessary to have all
RenderFrameImpls registered with the InputEventFilter? For frames that don't
have RenderWidgets, are there any messages that need to hop to the compositor
thread?

[kenrb, 2016-04-20 20:59:21]

Hmm, to fill that thought out a bit more...

Registering RenderFrameImpl's routing ID might just be the wrong thing. When
RenderFrameImpl owns a RenderWidget, some InputMsgs are sent directly to the
RenderWidget (including, notable, InputMsg_HandleInputEvent). With the current
patch, this will not get checked by the filter. It seems like at least we would
want to register the RenderWidget's routing ID, and possibly the
RenderFrameImpl's is unnecessary.

[Changwan Ryu, 2016-04-25 08:21:51]

On 2016/04/20 20:59:21, kenrb wrote:
> Hmm, to fill that thought out a bit more...
> 
> Registering RenderFrameImpl's routing ID might just be the wrong thing. When
> RenderFrameImpl owns a RenderWidget, some InputMsgs are sent directly to the
> RenderWidget (including, notable, InputMsg_HandleInputEvent). With the current
> patch, this will not get checked by the filter. It seems like at least we
would
> want to register the RenderWidget's routing ID, and possibly the
> RenderFrameImpl's is unnecessary.

Thanks for pointing out InputMsg_HandleInputEvent case. When RenderWidget is
created separately (when it's not RenderViewImpl), we aren't adding its input
handler, so only RenderFrameImpl will try to register it's routing ID.

But it's not clear how to avoid it. Are you going to add input handler for
RenderWidget anyways?
If so, how about moving AddInputHandler() logic from RenderViewImpl to
RenderWidget and register RenderFrameImpl's ID separately?

Otherwise, how about adding RenderFrameImpl's routing ID only when
RenderViewImpl::initializeLayerTreeView() gets called?

[kenrb, 2016-04-25 15:56:17]

On 2016/04/25 08:21:51, Changwan Ryu wrote:
> On 2016/04/20 20:59:21, kenrb wrote:
> > Hmm, to fill that thought out a bit more...
> > 
> > Registering RenderFrameImpl's routing ID might just be the wrong thing. When
> > RenderFrameImpl owns a RenderWidget, some InputMsgs are sent directly to the
> > RenderWidget (including, notable, InputMsg_HandleInputEvent). With the
current
> > patch, this will not get checked by the filter. It seems like at least we
> would
> > want to register the RenderWidget's routing ID, and possibly the
> > RenderFrameImpl's is unnecessary.
> 
> Thanks for pointing out InputMsg_HandleInputEvent case. When RenderWidget is
> created separately (when it's not RenderViewImpl), we aren't adding its input
> handler, so only RenderFrameImpl will try to register it's routing ID.
> 
> But it's not clear how to avoid it. Are you going to add input handler for
> RenderWidget anyways?
> If so, how about moving AddInputHandler() logic from RenderViewImpl to
> RenderWidget and register RenderFrameImpl's ID separately?
> 

We should have done that already but had missed it in earlier CLs. Moving
AddInputHandler() from RenderViewImpl to RenderWidget sounds right.

> Otherwise, how about adding RenderFrameImpl's routing ID only when
> RenderViewImpl::initializeLayerTreeView() gets called?

Is it necessary to add RenderFrameImpl's routing ID? Can any of the input
messages that it receives be processed on the compositor thread?

An input message sent to RenderWidget vs one sent to RenderFrame are already not
guaranteed to be in order, is that correct?

[Changwan Ryu, 2016-04-25 16:44:52]

On 2016/04/25 15:56:17, kenrb wrote:
> On 2016/04/25 08:21:51, Changwan Ryu wrote:
> > On 2016/04/20 20:59:21, kenrb wrote:
> > > Hmm, to fill that thought out a bit more...
> > > 
> > > Registering RenderFrameImpl's routing ID might just be the wrong thing.
When
> > > RenderFrameImpl owns a RenderWidget, some InputMsgs are sent directly to
the
> > > RenderWidget (including, notable, InputMsg_HandleInputEvent). With the
> current
> > > patch, this will not get checked by the filter. It seems like at least we
> > would
> > > want to register the RenderWidget's routing ID, and possibly the
> > > RenderFrameImpl's is unnecessary.
> > 
> > Thanks for pointing out InputMsg_HandleInputEvent case. When RenderWidget is
> > created separately (when it's not RenderViewImpl), we aren't adding its
input
> > handler, so only RenderFrameImpl will try to register it's routing ID.
> > 
> > But it's not clear how to avoid it. Are you going to add input handler for
> > RenderWidget anyways?
> > If so, how about moving AddInputHandler() logic from RenderViewImpl to
> > RenderWidget and register RenderFrameImpl's ID separately?
> > 
> 
> We should have done that already but had missed it in earlier CLs. Moving
> AddInputHandler() from RenderViewImpl to RenderWidget sounds right.
> 
> > Otherwise, how about adding RenderFrameImpl's routing ID only when
> > RenderViewImpl::initializeLayerTreeView() gets called?
> 
> Is it necessary to add RenderFrameImpl's routing ID? Can any of the input
> messages that it receives be processed on the compositor thread?
> 
> An input message sent to RenderWidget vs one sent to RenderFrame are already
not
> guaranteed to be in order, is that correct?

The purpose of registering the routing ID (as in the current patch) is such that
we can hop RenderFrame's input messages on the compositor thread and come back
to main thread immediately.
RenderFrame does not handle InputMsg_HandleInputEvent anyways, so registering
the routing ID should do the trick.
I could not observe the reordering issue for RenderFrameImpl + RenderViewImpl
case with the current patch.

Does this approach make sense once we move the AddInputHandler logic to
RenderWidget?

Also, I'd like to ask one more question in order to save one day from timezone
difference...
I already tried moving RenderViewImpl's AddInputHandler logic to RenderWiget
here:
https://codereview.chromium.org/1904753003/#ps160001
(please ignore the title... and also note that RenderFrameImpl's
AddInputHandler() isn't effective because of my typo in
RenderWidget::AddInputHandler()))

Basically, when input handler does not process it (NO_CONSUMER_EXISTS), then it
forwards the message to the renderer thread.
And there are small side effects, such as what's fixed in
touch_input_browsertest.cc.

Is this roughly going in the right direction? If not, I can wait for your change
and work on top of that.

[kenrb, 2016-04-26 18:29:24]

On 2016/04/25 16:44:52, Changwan Ryu wrote:
> The purpose of registering the routing ID (as in the current patch) is such
that
> we can hop RenderFrame's input messages on the compositor thread and come back
> to main thread immediately.
> RenderFrame does not handle InputMsg_HandleInputEvent anyways, so registering
> the routing ID should do the trick.
> I could not observe the reordering issue for RenderFrameImpl + RenderViewImpl
> case with the current patch.

Okay, I think I understand. The suggestion is that, even though no messages sent
to a RenderFrame will be processed on the compositor thread, we want to jump
threads anyway because that serves to keep RenderFrame input messages properly
ordered with RenderWidget input messages (which can be processed on the
compositor thread)?

> 
> Does this approach make sense once we move the AddInputHandler logic to
> RenderWidget?
> 
> Also, I'd like to ask one more question in order to save one day from timezone
> difference...
> I already tried moving RenderViewImpl's AddInputHandler logic to RenderWiget
> here:
> https://codereview.chromium.org/1904753003/#ps160001
> (please ignore the title... and also note that RenderFrameImpl's
> AddInputHandler() isn't effective because of my typo in
> RenderWidget::AddInputHandler()))
> 
> Basically, when input handler does not process it (NO_CONSUMER_EXISTS), then
it
> forwards the message to the renderer thread.
> And there are small side effects, such as what's fixed in
> touch_input_browsertest.cc.
> 
> Is this roughly going in the right direction? If not, I can wait for your
change
> and work on top of that.

This looks right to me.

[Changwan Ryu, 2016-04-28 04:43:18]

On 2016/04/26 18:29:24, kenrb wrote:
> On 2016/04/25 16:44:52, Changwan Ryu wrote:
> > The purpose of registering the routing ID (as in the current patch) is such
> that
> > we can hop RenderFrame's input messages on the compositor thread and come
back
> > to main thread immediately.
> > RenderFrame does not handle InputMsg_HandleInputEvent anyways, so
registering
> > the routing ID should do the trick.
> > I could not observe the reordering issue for RenderFrameImpl +
RenderViewImpl
> > case with the current patch.
> 
> Okay, I think I understand. The suggestion is that, even though no messages
sent
> to a RenderFrame will be processed on the compositor thread, we want to jump
> threads anyway because that serves to keep RenderFrame input messages properly
> ordered with RenderWidget input messages (which can be processed on the
> compositor thread)?
Correct.

> 
> > 
> > Does this approach make sense once we move the AddInputHandler logic to
> > RenderWidget?
> > 
> > Also, I'd like to ask one more question in order to save one day from
timezone
> > difference...
> > I already tried moving RenderViewImpl's AddInputHandler logic to RenderWiget
> > here:
> > https://codereview.chromium.org/1904753003/#ps160001
> > (please ignore the title... and also note that RenderFrameImpl's
> > AddInputHandler() isn't effective because of my typo in
> > RenderWidget::AddInputHandler()))
> > 
> > Basically, when input handler does not process it (NO_CONSUMER_EXISTS), then
> it
> > forwards the message to the renderer thread.
> > And there are small side effects, such as what's fixed in
> > touch_input_browsertest.cc.
> > 
> > Is this roughly going in the right direction? If not, I can wait for your
> change
> > and work on top of that.
> 
> This looks right to me.

Hmm... Actually I think adding input handler to renderwidget may be needed, but
is orthogonal to this fix.
(I can upload a separate patch to follow through if you want.)

I've uploaded a new patch just to fix OOPIF case. Could you take another look?

[kenrb, 2016-04-28 15:07:45]

lgtm (overall, and as owner for IPC files)

On 2016/04/28 04:43:18, Changwan Ryu wrote:
> Hmm... Actually I think adding input handler to renderwidget may be needed,
but
> is orthogonal to this fix.
> (I can upload a separate patch to follow through if you want.)
> 

I would really appreciate that, thanks.

[Changwan Ryu, 2016-04-29 00:11:34]

On 2016/04/28 15:07:45, kenrb wrote:
> lgtm (overall, and as owner for IPC files)
> 
> On 2016/04/28 04:43:18, Changwan Ryu wrote:
> > Hmm... Actually I think adding input handler to renderwidget may be needed,
> but
> > is orthogonal to this fix.
> > (I can upload a separate patch to follow through if you want.)
> > 
> 
> I would really appreciate that, thanks.

sievers@, could you take another look?

[Changwan Ryu, 2016-05-03 01:37:36]

On 2016/04/29 00:11:34, Changwan Ryu wrote:
> On 2016/04/28 15:07:45, kenrb wrote:
> > lgtm (overall, and as owner for IPC files)
> > 
> > On 2016/04/28 04:43:18, Changwan Ryu wrote:
> > > Hmm... Actually I think adding input handler to renderwidget may be
needed,
> > but
> > > is orthogonal to this fix.
> > > (I can upload a separate patch to follow through if you want.)
> > > 
> > 
> > I would really appreciate that, thanks.
> 
> sievers@, could you take another look?

rockot@, per comment #13, could you check if this is going in the right
direction?

[Ken Rockot(use gerrit already), 2016-05-03 01:46:58]

On 2016/05/03 at 01:37:36, changwan wrote:
> On 2016/04/29 00:11:34, Changwan Ryu wrote:
> > On 2016/04/28 15:07:45, kenrb wrote:
> > > lgtm (overall, and as owner for IPC files)
> > > 
> > > On 2016/04/28 04:43:18, Changwan Ryu wrote:
> > > > Hmm... Actually I think adding input handler to renderwidget may be
needed,
> > > but
> > > > is orthogonal to this fix.
> > > > (I can upload a separate patch to follow through if you want.)
> > > > 
> > > 
> > > I would really appreciate that, thanks.
> > 
> > sievers@, could you take another look?
> 
> rockot@, per comment #13, could you check if this is going in the right
direction?

Seems sane to me, rs lgtm. I'm still not *too* familiar with this area of the
code so I can't provide much useful commentary on the general direction, but the
change is small enough that I wouldn't be worried either way. I'll be looking at
this stuff more in depth in the coming week or two, so if anything relevant
comes out of that I'll ping you with better feedback.

[Changwan Ryu, 2016-05-23 03:07:37]

On 2016/05/03 01:46:58, Ken Rockot wrote:
> On 2016/05/03 at 01:37:36, changwan wrote:
> > On 2016/04/29 00:11:34, Changwan Ryu wrote:
> > > On 2016/04/28 15:07:45, kenrb wrote:
> > > > lgtm (overall, and as owner for IPC files)
> > > > 
> > > > On 2016/04/28 04:43:18, Changwan Ryu wrote:
> > > > > Hmm... Actually I think adding input handler to renderwidget may be
> needed,
> > > > but
> > > > > is orthogonal to this fix.
> > > > > (I can upload a separate patch to follow through if you want.)
> > > > > 
> > > > 
> > > > I would really appreciate that, thanks.
> > > 
> > > sievers@, could you take another look?
> > 
> > rockot@, per comment #13, could you check if this is going in the right
> direction?
> 
> Seems sane to me, rs lgtm. I'm still not *too* familiar with this area of the
> code so I can't provide much useful commentary on the general direction, but
the
> change is small enough that I wouldn't be worried either way. I'll be looking
at
> this stuff more in depth in the coming week or two, so if anything relevant
> comes out of that I'll ping you with better feedback.

ping to sievers@: could you take another look? thanks.

[Changwan Ryu, 2016-05-26 00:25:12]

On 2016/05/23 03:07:37, Changwan Ryu wrote:
> On 2016/05/03 01:46:58, Ken Rockot wrote:
> > On 2016/05/03 at 01:37:36, changwan wrote:
> > > On 2016/04/29 00:11:34, Changwan Ryu wrote:
> > > > On 2016/04/28 15:07:45, kenrb wrote:
> > > > > lgtm (overall, and as owner for IPC files)
> > > > > 
> > > > > On 2016/04/28 04:43:18, Changwan Ryu wrote:
> > > > > > Hmm... Actually I think adding input handler to renderwidget may be
> > needed,
> > > > > but
> > > > > > is orthogonal to this fix.
> > > > > > (I can upload a separate patch to follow through if you want.)
> > > > > > 
> > > > > 
> > > > > I would really appreciate that, thanks.
> > > > 
> > > > sievers@, could you take another look?
> > > 
> > > rockot@, per comment #13, could you check if this is going in the right
> > direction?
> > 
> > Seems sane to me, rs lgtm. I'm still not *too* familiar with this area of
the
> > code so I can't provide much useful commentary on the general direction, but
> the
> > change is small enough that I wouldn't be worried either way. I'll be
looking
> at
> > this stuff more in depth in the coming week or two, so if anything relevant
> > comes out of that I'll ping you with better feedback.
> 
> ping to sievers@: could you take another look? thanks.

sievers@, let me know if you have any other concern about this CL. Thanks.

[Changwan Ryu, 2016-06-01 06:17:33]

On 2016/05/26 00:25:12, Changwan Ryu wrote:
> On 2016/05/23 03:07:37, Changwan Ryu wrote:
> > On 2016/05/03 01:46:58, Ken Rockot wrote:
> > > On 2016/05/03 at 01:37:36, changwan wrote:
> > > > On 2016/04/29 00:11:34, Changwan Ryu wrote:
> > > > > On 2016/04/28 15:07:45, kenrb wrote:
> > > > > > lgtm (overall, and as owner for IPC files)
> > > > > > 
> > > > > > On 2016/04/28 04:43:18, Changwan Ryu wrote:
> > > > > > > Hmm... Actually I think adding input handler to renderwidget may
be
> > > needed,
> > > > > > but
> > > > > > > is orthogonal to this fix.
> > > > > > > (I can upload a separate patch to follow through if you want.)
> > > > > > > 
> > > > > > 
> > > > > > I would really appreciate that, thanks.
> > > > > 
> > > > > sievers@, could you take another look?
> > > > 
> > > > rockot@, per comment #13, could you check if this is going in the right
> > > direction?
> > > 
> > > Seems sane to me, rs lgtm. I'm still not *too* familiar with this area of
> the
> > > code so I can't provide much useful commentary on the general direction,
but
> > the
> > > change is small enough that I wouldn't be worried either way. I'll be
> looking
> > at
> > > this stuff more in depth in the coming week or two, so if anything
relevant
> > > comes out of that I'll ping you with better feedback.
> > 
> > ping to sievers@: could you take another look? thanks.
> 
> sievers@, let me know if you have any other concern about this CL. Thanks.

FYI, this blocks yabinh@'s CL (https://codereview.chromium.org/2010803005/), as
it may introduce flakiness in the new tests.

[no sievers, 2016-06-01 22:14:47]

https://codereview.chromium.org/1877073003/diff/120001/content/public/rendere...
File content/public/renderer/render_view.h (right):

https://codereview.chromium.org/1877073003/diff/120001/content/public/rendere...
content/public/renderer/render_view.h:154: // Whether we have added input
handler or not.
nit: the comment does not add any information that's not already in the method
name (I'd just remove it)

https://codereview.chromium.org/1877073003/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1877073003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1226: // TODO(changwan): apply this logic
to RenderWidget once we add input handler
nit: i'd remove this TODO here since IIUC you intend to leave the code here as
is

https://codereview.chromium.org/1877073003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1228: if (render_view_ &&
render_view_->HasAddedInputHandler()) {
This looks fragile. Do you expect HasAddedInputHandler() to be always true?
It just seems like call sequences could change somewhere, and your change would
silently become nonfunctional.

https://codereview.chromium.org/1877073003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1234:
input_handler_manager->RegisterRoutingID(GetRoutingID());
How does it get unregistered?

[Changwan Ryu, 2016-06-02 10:49:06]

https://codereview.chromium.org/1877073003/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1877073003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1228: if (render_view_ &&
render_view_->HasAddedInputHandler()) {
On 2016/06/01 22:14:47, sievers wrote:
> This looks fragile. Do you expect HasAddedInputHandler() to be always true?
> It just seems like call sequences could change somewhere, and your change
would
> silently become nonfunctional.

Two hidden assumptions here:
1) initializeLayerTreeView() is part of RenderViewImpl::Initialize().
2) RenderViewImpl::Initialize() is created before RenderFrameImpl::Initialize().

I think as long as those two are met, RegisterRoutingID() should be called. I am
not sure if those assumptions are easily breakable.

But there is a chance that this fix can break in the future because there is no
rigorous test around it. I'll try a bit more but I found it challenging to add a
test that instantiates a real RenderThreadImpl object and test it against
RenderViewImpl / RenderFrameImpl / RenderWidget while pausing and resuming the
impl thread.

[no sievers, 2016-06-02 17:16:09]

On 2016/06/02 10:49:06, Changwan Ryu wrote:
>
https://codereview.chromium.org/1877073003/diff/120001/content/renderer/rende...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/1877073003/diff/120001/content/renderer/rende...
> content/renderer/render_frame_impl.cc:1228: if (render_view_ &&
> render_view_->HasAddedInputHandler()) {
> On 2016/06/01 22:14:47, sievers wrote:
> > This looks fragile. Do you expect HasAddedInputHandler() to be always true?
> > It just seems like call sequences could change somewhere, and your change
> would
> > silently become nonfunctional.
> 
> Two hidden assumptions here:
> 1) initializeLayerTreeView() is part of RenderViewImpl::Initialize().
> 2) RenderViewImpl::Initialize() is created before
RenderFrameImpl::Initialize().
> 
> I think as long as those two are met, RegisterRoutingID() should be called. I
am
> not sure if those assumptions are easily breakable.
> 

That's what DCHECK() is for :)

And not 'if (' because that'd mean we are supporting the bad sequence (which we
are not, since things would be broken).


> But there is a chance that this fix can break in the future because there is
no
> rigorous test around it. I'll try a bit more but I found it challenging to add
a
> test that instantiates a real RenderThreadImpl object and test it against
> RenderViewImpl / RenderFrameImpl / RenderWidget while pausing and resuming the
> impl thread.

[Changwan Ryu, 2016-06-03 06:48:29]

https://codereview.chromium.org/1877073003/diff/120001/content/public/rendere...
File content/public/renderer/render_view.h (right):

https://codereview.chromium.org/1877073003/diff/120001/content/public/rendere...
content/public/renderer/render_view.h:154: // Whether we have added input
handler or not.
On 2016/06/01 22:14:47, sievers wrote:
> nit: the comment does not add any information that's not already in the method
> name (I'd just remove it)

Removed.

https://codereview.chromium.org/1877073003/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1877073003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1226: // TODO(changwan): apply this logic
to RenderWidget once we add input handler
On 2016/06/01 22:14:47, sievers wrote:
> nit: i'd remove this TODO here since IIUC you intend to leave the code here as
> is

Removed.

https://codereview.chromium.org/1877073003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1228: if (render_view_ &&
render_view_->HasAddedInputHandler()) {
On 2016/06/02 10:49:06, Changwan Ryu wrote:
> On 2016/06/01 22:14:47, sievers wrote:
> > This looks fragile. Do you expect HasAddedInputHandler() to be always true?
> > It just seems like call sequences could change somewhere, and your change
> would
> > silently become nonfunctional.
> 
> Two hidden assumptions here:
> 1) initializeLayerTreeView() is part of RenderViewImpl::Initialize().
> 2) RenderViewImpl::Initialize() is created before
RenderFrameImpl::Initialize().
> 
> I think as long as those two are met, RegisterRoutingID() should be called. I
am
> not sure if those assumptions are easily breakable.
> 
> But there is a chance that this fix can break in the future because there is
no
> rigorous test around it. I'll try a bit more but I found it challenging to add
a
> test that instantiates a real RenderThreadImpl object and test it against
> RenderViewImpl / RenderFrameImpl / RenderWidget while pausing and resuming the
> impl thread.

Removed render_view check and changed to DCHECK(). Let me know if there is more
to do to.

https://codereview.chromium.org/1877073003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1234:
input_handler_manager->RegisterRoutingID(GetRoutingID());
On 2016/06/01 22:14:47, sievers wrote:
> How does it get unregistered?

Now it's unregistered in ~RenderFrameImpl().

[Changwan Ryu, 2016-06-09 04:26:08]

On 2016/06/03 06:48:29, Changwan Ryu wrote:
>
https://codereview.chromium.org/1877073003/diff/120001/content/public/rendere...
> File content/public/renderer/render_view.h (right):
> 
>
https://codereview.chromium.org/1877073003/diff/120001/content/public/rendere...
> content/public/renderer/render_view.h:154: // Whether we have added input
> handler or not.
> On 2016/06/01 22:14:47, sievers wrote:
> > nit: the comment does not add any information that's not already in the
method
> > name (I'd just remove it)
> 
> Removed.
> 
>
https://codereview.chromium.org/1877073003/diff/120001/content/renderer/rende...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/1877073003/diff/120001/content/renderer/rende...
> content/renderer/render_frame_impl.cc:1226: // TODO(changwan): apply this
logic
> to RenderWidget once we add input handler
> On 2016/06/01 22:14:47, sievers wrote:
> > nit: i'd remove this TODO here since IIUC you intend to leave the code here
as
> > is
> 
> Removed.
> 
>
https://codereview.chromium.org/1877073003/diff/120001/content/renderer/rende...
> content/renderer/render_frame_impl.cc:1228: if (render_view_ &&
> render_view_->HasAddedInputHandler()) {
> On 2016/06/02 10:49:06, Changwan Ryu wrote:
> > On 2016/06/01 22:14:47, sievers wrote:
> > > This looks fragile. Do you expect HasAddedInputHandler() to be always
true?
> > > It just seems like call sequences could change somewhere, and your change
> > would
> > > silently become nonfunctional.
> > 
> > Two hidden assumptions here:
> > 1) initializeLayerTreeView() is part of RenderViewImpl::Initialize().
> > 2) RenderViewImpl::Initialize() is created before
> RenderFrameImpl::Initialize().
> > 
> > I think as long as those two are met, RegisterRoutingID() should be called.
I
> am
> > not sure if those assumptions are easily breakable.
> > 
> > But there is a chance that this fix can break in the future because there is
> no
> > rigorous test around it. I'll try a bit more but I found it challenging to
add
> a
> > test that instantiates a real RenderThreadImpl object and test it against
> > RenderViewImpl / RenderFrameImpl / RenderWidget while pausing and resuming
the
> > impl thread.
> 
> Removed render_view check and changed to DCHECK(). Let me know if there is
more
> to do to.
> 
>
https://codereview.chromium.org/1877073003/diff/120001/content/renderer/rende...
> content/renderer/render_frame_impl.cc:1234:
> input_handler_manager->RegisterRoutingID(GetRoutingID());
> On 2016/06/01 22:14:47, sievers wrote:
> > How does it get unregistered?
> 
> Now it's unregistered in ~RenderFrameImpl().

sievers@, please take another look. Thanks.

[no sievers, 2016-06-10 19:58:30]

lgtm

https://codereview.chromium.org/1877073003/diff/180001/content/renderer/input...
File content/renderer/input/input_handler_manager.cc (right):

https://codereview.chromium.org/1877073003/diff/180001/content/renderer/input...
content/renderer/input/input_handler_manager.cc:143: void
InputHandlerManager::RegisterRoutingIDOnCompositorThread(int routing_id) {
nit:  DCHECK(task_runner_->BelongsToCurrentThread());

https://codereview.chromium.org/1877073003/diff/180001/content/renderer/input...
content/renderer/input/input_handler_manager.cc:160:
client_->UnregisterRoutingID(routing_id);
nit:  DCHECK(task_runner_->BelongsToCurrentThread());

[palmer, 2016-06-10 20:44:24]

lgtm

[Changwan Ryu, 2016-06-13 01:20:59]

https://codereview.chromium.org/1877073003/diff/180001/content/renderer/input...
File content/renderer/input/input_handler_manager.cc (right):

https://codereview.chromium.org/1877073003/diff/180001/content/renderer/input...
content/renderer/input/input_handler_manager.cc:143: void
InputHandlerManager::RegisterRoutingIDOnCompositorThread(int routing_id) {
On 2016/06/10 19:58:30, sievers wrote:
> nit:  DCHECK(task_runner_->BelongsToCurrentThread());

Done.

https://codereview.chromium.org/1877073003/diff/180001/content/renderer/input...
content/renderer/input/input_handler_manager.cc:160:
client_->UnregisterRoutingID(routing_id);
On 2016/06/10 19:58:30, sievers wrote:
> nit:  DCHECK(task_runner_->BelongsToCurrentThread());

Done.

[Changwan Ryu, 2016-06-13 01:21:07]

The CQ bit was checked by changwan@chromium.org

[Changwan Ryu, 2016-06-13 01:21:08]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
tdresser@chromium.org, rockot@chromium.org, kenrb@chromium.org,
sievers@chromium.org, palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1877073003/#ps200001
(title: "add DCHECKs for thread")

[commit-bot: I haz the power, 2016-06-13 01:21:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1877073003/200001

[commit-bot: I haz the power, 2016-06-13 02:39:27]

Description was changed from

==========
Respect the order of input messages from browser to renderer

When we call InputConnection#setSelection and other methods together,
the order sometimes does not get kept. The reason is that
SetEditableSelectionOffsets is a frame message while all the others are
input messages, and they go to different message queues.

In addition, RenderViewImpl is the only who registers its routing ID
through InputHandlerManager::AddInputHandler() call. RenderFrameImpl,
on the other hand, does not register itself.

Once routing ID is registered, InputEventFilter will post input messages to
compositor impl thread first (such that scrolling can happen inside
compositor impl thread and keep in line with other input message handling
order).

We fix this by
1) Changing SetEditableSelectionOffsets from FrameMsg to InputMsg.
  (Now there is no remaining frame messages in ime_adapter_android.cc.)
2) Change DidAddInputHandler() and DidRemoveInputHandler() names to
  RegisterRoutingID() and UnregisterRoutingID(), respectively.
3) Register RenderFrame's routing ID at InputEventFilter.

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

to

==========
Respect the order of input messages from browser to renderer

When we call InputConnection#setSelection and other methods together,
the order sometimes does not get kept. The reason is that
SetEditableSelectionOffsets is a frame message while all the others are
input messages, and they go to different message queues.

In addition, RenderViewImpl is the only who registers its routing ID
through InputHandlerManager::AddInputHandler() call. RenderFrameImpl,
on the other hand, does not register itself.

Once routing ID is registered, InputEventFilter will post input messages to
compositor impl thread first (such that scrolling can happen inside
compositor impl thread and keep in line with other input message handling
order).

We fix this by
1) Changing SetEditableSelectionOffsets from FrameMsg to InputMsg.
  (Now there is no remaining frame messages in ime_adapter_android.cc.)
2) Change DidAddInputHandler() and DidRemoveInputHandler() names to
  RegisterRoutingID() and UnregisterRoutingID(), respectively.
3) Register RenderFrame's routing ID at InputEventFilter.

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

[commit-bot: I haz the power, 2016-06-13 02:39:29]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-06-13 02:40:46]

Description was changed from

==========
Respect the order of input messages from browser to renderer

When we call InputConnection#setSelection and other methods together,
the order sometimes does not get kept. The reason is that
SetEditableSelectionOffsets is a frame message while all the others are
input messages, and they go to different message queues.

In addition, RenderViewImpl is the only who registers its routing ID
through InputHandlerManager::AddInputHandler() call. RenderFrameImpl,
on the other hand, does not register itself.

Once routing ID is registered, InputEventFilter will post input messages to
compositor impl thread first (such that scrolling can happen inside
compositor impl thread and keep in line with other input message handling
order).

We fix this by
1) Changing SetEditableSelectionOffsets from FrameMsg to InputMsg.
  (Now there is no remaining frame messages in ime_adapter_android.cc.)
2) Change DidAddInputHandler() and DidRemoveInputHandler() names to
  RegisterRoutingID() and UnregisterRoutingID(), respectively.
3) Register RenderFrame's routing ID at InputEventFilter.

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

to

==========
Respect the order of input messages from browser to renderer

When we call InputConnection#setSelection and other methods together,
the order sometimes does not get kept. The reason is that
SetEditableSelectionOffsets is a frame message while all the others are
input messages, and they go to different message queues.

In addition, RenderViewImpl is the only who registers its routing ID
through InputHandlerManager::AddInputHandler() call. RenderFrameImpl,
on the other hand, does not register itself.

Once routing ID is registered, InputEventFilter will post input messages to
compositor impl thread first (such that scrolling can happen inside
compositor impl thread and keep in line with other input message handling
order).

We fix this by
1) Changing SetEditableSelectionOffsets from FrameMsg to InputMsg.
  (Now there is no remaining frame messages in ime_adapter_android.cc.)
2) Change DidAddInputHandler() and DidRemoveInputHandler() names to
  RegisterRoutingID() and UnregisterRoutingID(), respectively.
3) Register RenderFrame's routing ID at InputEventFilter.

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

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

[commit-bot: I haz the power, 2016-06-13 02:40:48]

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