Allow MainThreadEventQueue to call the RenderWidget directly.

Allow MainThreadEventQueue to call the RenderWidget directly.

Allocate the MainThreadEventQueue from the render widget allowing
a binding between then. This allows us to avoid
serialization/deserialization of events to IPC messages inside the
renderer. It also allows us to avoid jumping through a number of hoops
looking the input handler in a table. For example requesting a begin
main frame from the main thread doesn't require a hop to the compositor
and back to the main thread any more.

BUG=703344
Review-Url: https://codereview.chromium.org/2813683002
Cr-Commit-Position: refs/heads/master@{#466340}
Committed: https://chromium.googlesource.com/chromium/src/+/9ec1a91b89f0dd7393a1c38fa84a7064d5984c1f

[dtapuska, 2017-04-10 15:26:55]

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

[commit-bot: I haz the power, 2017-04-10 15:27:13]

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

[dtapuska, 2017-04-10 15:27:27]

dtapuska@chromium.org changed reviewers:
+ mustaq@chromium.org, tdresser@chromium.org

[dtapuska, 2017-04-10 15:27:28]

PTAL

[tdresser, 2017-04-10 15:54:17]

A quick first pass - there are a few high'ish level things I want to make sure I
understand correctly.

https://codereview.chromium.org/2813683002/diff/1/content/renderer/idle_user_...
File content/renderer/idle_user_detector.cc (left):

https://codereview.chromium.org/2813683002/diff/1/content/renderer/idle_user_...
content/renderer/idle_user_detector.cc:42: delete this;
Out of curiosity, why this change? It seems to be a common pattern here.

https://codereview.chromium.org/2813683002/diff/1/content/renderer/input/main...
File content/renderer/input/main_thread_event_queue.h (right):

https://codereview.chromium.org/2813683002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.h:88: uint32_t touch_event_id) =
0;
Why is SendInputEventAck separate from NotifySchedulerOfEventHandled?

https://codereview.chromium.org/2813683002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.h:141: class
MainThreadEventQueueWithRenderWidget : public MainThreadEventQueue {
Why doesn't the MainThreadEventQueue just have a weakptr to a RenderWidget?

https://codereview.chromium.org/2813683002/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2813683002/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2665: if (!input_event || dispatch_type !=
DISPATCH_TYPE_BLOCKING) {
== DISPATCH_TYPE_NONBLOCKING?

[dtapuska, 2017-04-10 16:08:06]

https://codereview.chromium.org/2813683002/diff/1/content/renderer/idle_user_...
File content/renderer/idle_user_detector.cc (left):

https://codereview.chromium.org/2813683002/diff/1/content/renderer/idle_user_...
content/renderer/idle_user_detector.cc:42: delete this;
On 2017/04/10 15:54:17, tdresser wrote:
> Out of curiosity, why this change? It seems to be a common pattern here.

Because this is a IPC message filter that self deletes but it listens for IPC
message events which don't normally go through that flow anymore. So I need to
call it directly. So I need a handle in the RenderView to it.. since I have a
handle, I'll control deletion via a unique_ptr...

https://codereview.chromium.org/2813683002/diff/1/content/renderer/input/main...
File content/renderer/input/main_thread_event_queue.h (right):

https://codereview.chromium.org/2813683002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.h:88: uint32_t touch_event_id) =
0;
On 2017/04/10 15:54:17, tdresser wrote:
> Why is SendInputEventAck separate from NotifySchedulerOfEventHandled?

Mainly for the tests. There is a mock renderer scheduler which doesn't create a
render widget. If you have a better idea I'm happy to hear it.

https://codereview.chromium.org/2813683002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.h:141: class
MainThreadEventQueueWithRenderWidget : public MainThreadEventQueue {
On 2017/04/10 15:54:17, tdresser wrote:
> Why doesn't the MainThreadEventQueue just have a weakptr to a RenderWidget?

I could do it that way but the abstraction is for tests. I don't believe there
is a MockRenderWidget

[commit-bot: I haz the power, 2017-04-10 16:09:35]

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

[commit-bot: I haz the power, 2017-04-10 16:09:36]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[tdresser, 2017-04-10 16:25:01]

https://codereview.chromium.org/2813683002/diff/1/content/renderer/idle_user_...
File content/renderer/idle_user_detector.cc (left):

https://codereview.chromium.org/2813683002/diff/1/content/renderer/idle_user_...
content/renderer/idle_user_detector.cc:42: delete this;
On 2017/04/10 16:08:05, dtapuska wrote:
> On 2017/04/10 15:54:17, tdresser wrote:
> > Out of curiosity, why this change? It seems to be a common pattern here.
> 
> Because this is a IPC message filter that self deletes but it listens for IPC
> message events which don't normally go through that flow anymore. So I need to
> call it directly. So I need a handle in the RenderView to it.. since I have a
> handle, I'll control deletion via a unique_ptr...

Acknowledged.

https://codereview.chromium.org/2813683002/diff/1/content/renderer/input/main...
File content/renderer/input/main_thread_event_queue.h (right):

https://codereview.chromium.org/2813683002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.h:141: class
MainThreadEventQueueWithRenderWidget : public MainThreadEventQueue {
On 2017/04/10 16:08:05, dtapuska wrote:
> On 2017/04/10 15:54:17, tdresser wrote:
> > Why doesn't the MainThreadEventQueue just have a weakptr to a RenderWidget?
> 
> I could do it that way but the abstraction is for tests. I don't believe there
> is a MockRenderWidget

Could we add a new interface which RenderWidget implements, which includes all
the functionality the MTEQ requires? Mocking that would be pretty easy.

[mustaq, 2017-04-12 19:27:49]

Two comments for now...

https://codereview.chromium.org/2813683002/diff/1/content/renderer/idle_user_...
File content/renderer/idle_user_detector.cc (right):

https://codereview.chromium.org/2813683002/diff/1/content/renderer/idle_user_...
content/renderer/idle_user_detector.cc:28: void
IdleUserDetector::HandleInputMessageReceived() {
This has nothing to do with handling input. Let's call it |ActivityDetected|
instead.

https://codereview.chromium.org/2813683002/diff/1/content/renderer/input/inpu...
File content/renderer/input/input_handler_manager.h (right):

https://codereview.chromium.org/2813683002/diff/1/content/renderer/input/inpu...
content/renderer/input/input_handler_manager.h:70: void UnregisterRoutingID(int
routing_id);
The UnregisterXYZ w/o a corresponding RegisterXYZ seems a bit too odd. CS seems
to suggest that there was no caller to RegisterXYZ before, but ~RenderFrameImpl
calls UnregisterXYZ. Is it real?

[dtapuska, 2017-04-13 16:56:18]

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

[dtapuska, 2017-04-13 16:56:24]

PTAL

https://codereview.chromium.org/2813683002/diff/1/content/renderer/idle_user_...
File content/renderer/idle_user_detector.cc (right):

https://codereview.chromium.org/2813683002/diff/1/content/renderer/idle_user_...
content/renderer/idle_user_detector.cc:28: void
IdleUserDetector::HandleInputMessageReceived() {
On 2017/04/12 19:27:49, mustaq wrote:
> This has nothing to do with handling input. Let's call it |ActivityDetected|
> instead.

Done.

https://codereview.chromium.org/2813683002/diff/1/content/renderer/input/inpu...
File content/renderer/input/input_handler_manager.h (right):

https://codereview.chromium.org/2813683002/diff/1/content/renderer/input/inpu...
content/renderer/input/input_handler_manager.h:70: void UnregisterRoutingID(int
routing_id);
On 2017/04/12 19:27:49, mustaq wrote:
> The UnregisterXYZ w/o a corresponding RegisterXYZ seems a bit too odd. CS
seems
> to suggest that there was no caller to RegisterXYZ before, but
~RenderFrameImpl
> calls UnregisterXYZ. Is it real?

Yes; AddInputHandler and RegisterAssociatedRenderFrameRoutingID are the
"register" functions.

https://codereview.chromium.org/2813683002/diff/1/content/renderer/input/main...
File content/renderer/input/main_thread_event_queue.h (right):

https://codereview.chromium.org/2813683002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.h:141: class
MainThreadEventQueueWithRenderWidget : public MainThreadEventQueue {
On 2017/04/10 16:25:01, tdresser wrote:
> On 2017/04/10 16:08:05, dtapuska wrote:
> > On 2017/04/10 15:54:17, tdresser wrote:
> > > Why doesn't the MainThreadEventQueue just have a weakptr to a
RenderWidget?
> > 
> > I could do it that way but the abstraction is for tests. I don't believe
there
> > is a MockRenderWidget
> 
> Could we add a new interface which RenderWidget implements, which includes all
> the functionality the MTEQ requires? Mocking that would be pretty easy.

Done.

https://codereview.chromium.org/2813683002/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2813683002/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2665: if (!input_event || dispatch_type !=
DISPATCH_TYPE_BLOCKING) {
On 2017/04/10 15:54:17, tdresser wrote:
> == DISPATCH_TYPE_NONBLOCKING?

Done.

[commit-bot: I haz the power, 2017-04-13 16:56:51]

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

[tdresser, 2017-04-13 17:19:59]

https://codereview.chromium.org/2813683002/diff/20001/content/renderer/input/...
File content/renderer/input/input_event_filter_unittest.cc (right):

https://codereview.chromium.org/2813683002/diff/20001/content/renderer/input/...
content/renderer/input/input_event_filter_unittest.cc:437: for (size_t i = 1; i
< messages.size() - 1; ++i) {
What changed here?

https://codereview.chromium.org/2813683002/diff/20001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue.cc (right):

https://codereview.chromium.org/2813683002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:400: const
std::unique_ptr<MainThreadEventQueueTask>& queued_item) {
Why did this move out of the anonymous namespace?

https://codereview.chromium.org/2813683002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:404: static_cast<const
QueuedWebInputEvent*>(queued_item.get());
Why cast to a pointer here, and a ref below? Could you use a ref here as well?

https://codereview.chromium.org/2813683002/diff/20001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue.h (right):

https://codereview.chromium.org/2813683002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.h:105: void NeedsMainFrame();
Should this be called SetNeedsMainFrame, to be consistent with the method on the
client?

[commit-bot: I haz the power, 2017-04-13 17:46:59]

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

[commit-bot: I haz the power, 2017-04-13 17:47:00]

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

[dtapuska, 2017-04-18 12:59:28]

https://codereview.chromium.org/2813683002/diff/20001/content/renderer/input/...
File content/renderer/input/input_event_filter_unittest.cc (right):

https://codereview.chromium.org/2813683002/diff/20001/content/renderer/input/...
content/renderer/input/input_event_filter_unittest.cc:437: for (size_t i = 1; i
< messages.size() - 1; ++i) {
On 2017/04/13 17:19:59, tdresser wrote:
> What changed here?

This is showing that the first event is an event and then next things in the
queue are still IPC messages. Since we aren't storing the event as an IPC
message anymore we need to explicitly call it out. ie.
message_recorder.event_at(0) is an event. and message_recorder.message_at(0)
would fail because it isn't a message.

https://codereview.chromium.org/2813683002/diff/20001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue.cc (right):

https://codereview.chromium.org/2813683002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:404: static_cast<const
QueuedWebInputEvent*>(queued_item.get());
On 2017/04/13 17:19:59, tdresser wrote:
> Why cast to a pointer here, and a ref below? Could you use a ref here as well?

THe queued_item is a unique_ptr... yet the code below is a cast of a member that
returns a const reference.

https://codereview.chromium.org/2813683002/diff/20001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue.h (right):

https://codereview.chromium.org/2813683002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.h:105: void NeedsMainFrame();
On 2017/04/13 17:19:59, tdresser wrote:
> Should this be called SetNeedsMainFrame, to be consistent with the method on
the
> client?

Done.

[tdresser, 2017-04-18 13:45:05]

LGTM with nits.

https://codereview.chromium.org/2813683002/diff/20001/content/renderer/input/...
File content/renderer/input/input_event_filter_unittest.cc (right):

https://codereview.chromium.org/2813683002/diff/20001/content/renderer/input/...
content/renderer/input/input_event_filter_unittest.cc:437: for (size_t i = 1; i
< messages.size() - 1; ++i) {
On 2017/04/18 12:59:28, dtapuska wrote:
> On 2017/04/13 17:19:59, tdresser wrote:
> > What changed here?
> 
> This is showing that the first event is an event and then next things in the
> queue are still IPC messages. Since we aren't storing the event as an IPC
> message anymore we need to explicitly call it out. ie.
> message_recorder.event_at(0) is an event. and message_recorder.message_at(0)
> would fail because it isn't a message.

Gotcha, thanks for the explanation. Might be good to have a comment the first
time this happens to clarify this a bit?

https://codereview.chromium.org/2813683002/diff/20001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue.cc (right):

https://codereview.chromium.org/2813683002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:404: static_cast<const
QueuedWebInputEvent*>(queued_item.get());
On 2017/04/18 12:59:28, dtapuska wrote:
> On 2017/04/13 17:19:59, tdresser wrote:
> > Why cast to a pointer here, and a ref below? Could you use a ref here as
well?
> 
> THe queued_item is a unique_ptr... yet the code below is a cast of a member
that
> returns a const reference.

You could deref it though, right?

https://codereview.chromium.org/2813683002/diff/40001/content/renderer/input/...
File content/renderer/input/input_event_filter_unittest.cc (right):

https://codereview.chromium.org/2813683002/diff/40001/content/renderer/input/...
content/renderer/input/input_event_filter_unittest.cc:133: CHECK(false);
NOTREACHED?

https://codereview.chromium.org/2813683002/diff/40001/content/renderer/input/...
content/renderer/input/input_event_filter_unittest.cc:188: return
messages_[i]->itemAsMessage();
Could these CHECK for non-null, deref, and return const references? It looks
like we're assuming these are non-null, but still passing a pointer.

https://codereview.chromium.org/2813683002/diff/40001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue.cc (right):

https://codereview.chromium.org/2813683002/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:40: // Time interval at which
touchmove events will be skipped during rAF signal.
Should this say "touchmove events during scroll will be"... ?

[dtapuska, 2017-04-18 14:16:03]

https://codereview.chromium.org/2813683002/diff/40001/content/renderer/input/...
File content/renderer/input/input_event_filter_unittest.cc (right):

https://codereview.chromium.org/2813683002/diff/40001/content/renderer/input/...
content/renderer/input/input_event_filter_unittest.cc:133: CHECK(false);
On 2017/04/18 13:45:05, tdresser wrote:
> NOTREACHED?

Done.

https://codereview.chromium.org/2813683002/diff/40001/content/renderer/input/...
content/renderer/input/input_event_filter_unittest.cc:188: return
messages_[i]->itemAsMessage();
On 2017/04/18 13:45:05, tdresser wrote:
> Could these CHECK for non-null, deref, and return const references? It looks
> like we're assuming these are non-null, but still passing a pointer.

Done.

https://codereview.chromium.org/2813683002/diff/40001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue.cc (right):

https://codereview.chromium.org/2813683002/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:40: // Time interval at which
touchmove events will be skipped during rAF signal.
On 2017/04/18 13:45:05, tdresser wrote:
> Should this say "touchmove events during scroll will be"... ?

Done.

[dtapuska, 2017-04-18 14:18:30]

dtapuska@chromium.org changed reviewers:
+ clamy@chromium.org

[dtapuska, 2017-04-18 14:18:30]

clamy@chromium.org: Please review changes in 

PTAL; input/* have already been reviewed.

[dtapuska, 2017-04-18 14:18:43]

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

[commit-bot: I haz the power, 2017-04-18 14:19:09]

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, 2017-04-18 15:11:36]

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

[commit-bot: I haz the power, 2017-04-18 15:11:37]

Dry run: This issue passed the CQ dry run.

[dtapuska, 2017-04-19 12:39:57]

On 2017/04/18 14:18:30, dtapuska wrote:
> mailto:clamy@chromium.org: Please review changes in 
> 
> PTAL; input/* have already been reviewed.

clamy@ ping; or should I choose a different reviewer?

[clamy, 2017-04-19 15:34:04]

Description was changed from

==========
Allow MainThreadEventQueue to call the RenderWidget directly.

Allocate the MainThreadEventQueue from the render widget allowing
a binding between then. This allows us to avoid
serialization/deserialization of events to IPC messages inside the
renderer. It also allows us to avoid jumping through a number of hoops
looking the input handler in a table. For example requesting a begin
main frame from the main thread doesn't require a hop to the compositor
and back to the main thread any more.

BUG=703344
==========

to

==========
Allow MainThreadEventQueue to call the RenderWidget directly.

Allocate the MainThreadEventQueue from the render widget allowing
a binding between then. This allows us to avoid
serialization/deserialization of events to IPC messages inside the
renderer. It also allows us to avoid jumping through a number of hoops
looking the input handler in a table. For example requesting a begin
main frame from the main thread doesn't require a hop to the compositor
and back to the main thread any more.

BUG=703344
==========

[clamy, 2017-04-19 15:34:04]

clamy@chromium.org changed reviewers:
+ nick@chromium.org

[clamy, 2017-04-19 15:34:46]

Sorry for the delay. I'm less familiar with RenderWidget, nick@ could you take a
look? Thanks!

[ncarter (slow), 2017-04-20 22:04:44]

lgtm from a content/ OWNER perspective, with two style nits. Though, the input
implications of this are mostly over my head, so I'm trusting tdresser's lgtm.

https://codereview.chromium.org/2813683002/diff/60001/content/renderer/idle_u...
File content/renderer/idle_user_detector.cc (right):

https://codereview.chromium.org/2813683002/diff/60001/content/renderer/idle_u...
content/renderer/idle_user_detector.cc:15: : RenderViewObserver(render_view){
clang-format this file

https://codereview.chromium.org/2813683002/diff/60001/content/renderer/input/...
File content/renderer/input/input_event_filter_unittest.cc (right):

https://codereview.chromium.org/2813683002/diff/60001/content/renderer/input/...
content/renderer/input/input_event_filter_unittest.cc:132: virtual const
ReceivedMessage* itemAsMessage() const {
These method names aren't in Chromium C++ style.

[dtapuska, 2017-04-21 14:27:30]

https://codereview.chromium.org/2813683002/diff/60001/content/renderer/idle_u...
File content/renderer/idle_user_detector.cc (right):

https://codereview.chromium.org/2813683002/diff/60001/content/renderer/idle_u...
content/renderer/idle_user_detector.cc:15: : RenderViewObserver(render_view){
On 2017/04/20 22:04:44, ncarter wrote:
> clang-format this file

Done.

https://codereview.chromium.org/2813683002/diff/60001/content/renderer/input/...
File content/renderer/input/input_event_filter_unittest.cc (right):

https://codereview.chromium.org/2813683002/diff/60001/content/renderer/input/...
content/renderer/input/input_event_filter_unittest.cc:132: virtual const
ReceivedMessage* itemAsMessage() const {
On 2017/04/20 22:04:44, ncarter wrote:
> These method names aren't in Chromium C++ style.

ugh.. yes you are correct!

[dtapuska, 2017-04-21 14:27:40]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2017-04-21 14:27:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org, nick@chromium.org
Link to the patchset: https://codereview.chromium.org/2813683002/#ps80001
(title: "Fix style nits")

[commit-bot: I haz the power, 2017-04-21 14:27:57]

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, 2017-04-21 15:18:35]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492784860086260,
"parent_rev": "2900e15403dca980062e107fc87b4d84cd07b49f", "commit_rev":
"9ec1a91b89f0dd7393a1c38fa84a7064d5984c1f"}

[commit-bot: I haz the power, 2017-04-21 15:18:49]

Description was changed from

==========
Allow MainThreadEventQueue to call the RenderWidget directly.

Allocate the MainThreadEventQueue from the render widget allowing
a binding between then. This allows us to avoid
serialization/deserialization of events to IPC messages inside the
renderer. It also allows us to avoid jumping through a number of hoops
looking the input handler in a table. For example requesting a begin
main frame from the main thread doesn't require a hop to the compositor
and back to the main thread any more.

BUG=703344
==========

to

==========
Allow MainThreadEventQueue to call the RenderWidget directly.

Allocate the MainThreadEventQueue from the render widget allowing
a binding between then. This allows us to avoid
serialization/deserialization of events to IPC messages inside the
renderer. It also allows us to avoid jumping through a number of hoops
looking the input handler in a table. For example requesting a begin
main frame from the main thread doesn't require a hop to the compositor
and back to the main thread any more.

BUG=703344

Review-Url: https://codereview.chromium.org/2813683002
Cr-Commit-Position: refs/heads/master@{#466340}
Committed:
https://chromium.googlesource.com/chromium/src/+/9ec1a91b89f0dd7393a1c38fa84a...
==========

[commit-bot: I haz the power, 2017-04-21 15:18:50]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9ec1a91b89f0dd7393a1c38fa84a...