Move BlimpInputManager to BlimpDocument from BlimpCompositor.

Move BlimpInputManager to BlimpDocument from BlimpCompositor.

Previously BlimpCompositor also routes the input events,
And we have the following call chain:

UI->BlimpInputManager->BlimpCompositor->BlimpDocument->
BlimpDocumentManager->RenderWidgetFeature.

This CL moves the ownership of BlimpInputManager to BlimpDocument, the
call chain of sending input event to engine becomes:

UI->BlimpInputManager->BlimpDocument->BlimpDocumentManager->
RenderWidgetFeature.

BUG=644326
Committed: https://crrev.com/f899472e1c9bf5487449603df5a6584f22b8068d
Cr-Commit-Position: refs/heads/master@{#424343}

[xingliu, 2016-10-07 02:22:34]

Description was changed from

==========
MoveBlimpInputManager to BlimpDocument from BlimpCompositor.

Previously BlimpCompositor also routes the input events,
And we have the following call chain:

UI->BlimpInputManager->BlimpCompositor->BlimpDocument->
BlimpDocumentManager->RenderWidgetFeature.


This CL moves the ownership of input manager to BlimpDocument, the
call chain of sending input event to engine becomes:

UI->BlimpInputManager->BlimpDocument->BlimpDocumentManager->
RenderWidgetFeature.


BUG=644326
==========

to

==========
Move BlimpInputManager to BlimpDocument from BlimpCompositor.

Previously BlimpCompositor also routes the input events,
And we have the following call chain:

UI->BlimpInputManager->BlimpCompositor->BlimpDocument->
BlimpDocumentManager->RenderWidgetFeature.


This CL moves the ownership of input manager to BlimpDocument, the
call chain of sending input event to engine becomes:

UI->BlimpInputManager->BlimpDocument->BlimpDocumentManager->
RenderWidgetFeature.


BUG=644326
==========

[xingliu, 2016-10-07 02:22:59]

Description was changed from

==========
Move BlimpInputManager to BlimpDocument from BlimpCompositor.

Previously BlimpCompositor also routes the input events,
And we have the following call chain:

UI->BlimpInputManager->BlimpCompositor->BlimpDocument->
BlimpDocumentManager->RenderWidgetFeature.


This CL moves the ownership of input manager to BlimpDocument, the
call chain of sending input event to engine becomes:

UI->BlimpInputManager->BlimpDocument->BlimpDocumentManager->
RenderWidgetFeature.


BUG=644326
==========

to

==========
Move BlimpInputManager to BlimpDocument from BlimpCompositor.

Previously BlimpCompositor also routes the input events,
And we have the following call chain:

UI->BlimpInputManager->BlimpCompositor->BlimpDocument->
BlimpDocumentManager->RenderWidgetFeature.


This CL moves the ownership of BlimpInputManager to BlimpDocument, the
call chain of sending input event to engine becomes:

UI->BlimpInputManager->BlimpDocument->BlimpDocumentManager->
RenderWidgetFeature.


BUG=644326
==========

[xingliu, 2016-10-07 02:33:51]

xingliu@chromium.org changed reviewers:
+ dtrainor@chromium.org, khushalsagar@chromium.org

[xingliu, 2016-10-07 02:33:52]

Pulled out BlimpInputManager from BlimpCompositor. Please help take a look,
thanks.

[Khushal, 2016-10-07 03:40:15]

https://codereview.chromium.org/2400923002/diff/20001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2400923002/diff/20001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor.cc:165: // Returns a reference to
the InputHandler owned by layer tree host, which is
Don't need the comment in the definition.

https://codereview.chromium.org/2400923002/diff/20001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor.cc:168: DCHECK(host_);
Don't need to DCHECK if we're de-referencing it.

https://codereview.chromium.org/2400923002/diff/20001/blimp/client/core/rende...
File blimp/client/core/render_widget/blimp_document.h (right):

https://codereview.chromium.org/2400923002/diff/20001/blimp/client/core/rende...
blimp/client/core/render_widget/blimp_document.h:42: protected:
How about adding a protected ctor for tests here, that takes in a compositor.
Then in the public ctor you can create a compositor and pass it to the protected
one, and the test can directly use the protected one? So all the input setup is
in one place.

https://codereview.chromium.org/2400923002/diff/20001/blimp/client/core/rende...
File blimp/client/core/render_widget/blimp_document_manager_unittest.cc (right):

https://codereview.chromium.org/2400923002/diff/20001/blimp/client/core/rende...
blimp/client/core/render_widget/blimp_document_manager_unittest.cc:22: using
testing::Return;
Not used?

https://codereview.chromium.org/2400923002/diff/20001/blimp/client/core/rende...
File blimp/client/core/render_widget/blimp_document_unittest.cc (left):

https://codereview.chromium.org/2400923002/diff/20001/blimp/client/core/rende...
blimp/client/core/render_widget/blimp_document_unittest.cc:24: const int
kDefaultDocumentId = 1;
Why remove this?

[David Trainor- moved to gerrit, 2016-10-07 07:07:08]

https://codereview.chromium.org/2400923002/diff/20001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2400923002/diff/20001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor.cc:168: DCHECK(host_);
On 2016/10/07 03:40:15, Khushal wrote:
> Don't need to DCHECK if we're de-referencing it.

Yeah std::unique_ptr should check for you, but just for reference, IIUC if it's
a raw pointer it won't break unless you try to access memory on the class
(vtable, member variables).

[xingliu, 2016-10-10 18:58:50]

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

[commit-bot: I haz the power, 2016-10-10 18:59:06]

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

[xingliu, 2016-10-10 19:00:17]

Thanks for the review, please help to take a look.

Major change: added a protected ctor in blimp_document for testing according to
feedback, which is safer than swapping the whole compositor.

https://codereview.chromium.org/2400923002/diff/20001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2400923002/diff/20001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor.cc:165: // Returns a reference to
the InputHandler owned by layer tree host, which is
On 2016/10/07 03:40:15, Khushal wrote:
> Don't need the comment in the definition.

Done.

https://codereview.chromium.org/2400923002/diff/20001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor.cc:168: DCHECK(host_);
On 2016/10/07 07:07:08, David Trainor wrote:
> On 2016/10/07 03:40:15, Khushal wrote:
> > Don't need to DCHECK if we're de-referencing it.
> 
> Yeah std::unique_ptr should check for you, but just for reference, IIUC if
it's
> a raw pointer it won't break unless you try to access memory on the class
> (vtable, member variables).

Done.

https://codereview.chromium.org/2400923002/diff/20001/blimp/client/core/rende...
File blimp/client/core/render_widget/blimp_document.h (right):

https://codereview.chromium.org/2400923002/diff/20001/blimp/client/core/rende...
blimp/client/core/render_widget/blimp_document.h:42: protected:
On 2016/10/07 03:40:15, Khushal wrote:
> How about adding a protected ctor for tests here, that takes in a compositor.
> Then in the public ctor you can create a compositor and pass it to the
protected
> one, and the test can directly use the protected one? So all the input setup
is
> in one place.

Good idea.

https://codereview.chromium.org/2400923002/diff/20001/blimp/client/core/rende...
File blimp/client/core/render_widget/blimp_document_manager_unittest.cc (right):

https://codereview.chromium.org/2400923002/diff/20001/blimp/client/core/rende...
blimp/client/core/render_widget/blimp_document_manager_unittest.cc:22: using
testing::Return;
On 2016/10/07 03:40:15, Khushal wrote:
> Not used?

Done.

https://codereview.chromium.org/2400923002/diff/20001/blimp/client/core/rende...
File blimp/client/core/render_widget/blimp_document_unittest.cc (left):

https://codereview.chromium.org/2400923002/diff/20001/blimp/client/core/rende...
blimp/client/core/render_widget/blimp_document_unittest.cc:24: const int
kDefaultDocumentId = 1;
On 2016/10/07 03:40:15, Khushal wrote:
> Why remove this?

Moved it back, this is added in the previous patch, thought this was not very
useful.

[commit-bot: I haz the power, 2016-10-10 19:57:19]

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

[commit-bot: I haz the power, 2016-10-10 19:57:20]

Dry run: This issue passed the CQ dry run.

[David Trainor- moved to gerrit, 2016-10-10 21:12:20]

lgtm % nit and khushal's +2.  Thanks!

https://codereview.chromium.org/2400923002/diff/40001/blimp/client/core/rende...
File blimp/client/core/render_widget/blimp_document.cc (right):

https://codereview.chromium.org/2400923002/diff/40001/blimp/client/core/rende...
blimp/client/core/render_widget/blimp_document.cc:18: BlimpDocumentManager*
document_manager)
Can you just call the other constructor with the additional compositor object?

[xingliu, 2016-10-10 21:43:27]

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

[commit-bot: I haz the power, 2016-10-10 21:43:44]

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

[xingliu, 2016-10-10 21:48:00]

Thanks for the review, start to land this patch.

https://codereview.chromium.org/2400923002/diff/40001/blimp/client/core/rende...
File blimp/client/core/render_widget/blimp_document.cc (right):

https://codereview.chromium.org/2400923002/diff/40001/blimp/client/core/rende...
blimp/client/core/render_widget/blimp_document.cc:18: BlimpDocumentManager*
document_manager)
On 2016/10/10 21:12:20, David Trainor wrote:
> Can you just call the other constructor with the additional compositor object?

Done.

[Khushal, 2016-10-10 21:54:54]

https://codereview.chromium.org/2400923002/diff/60001/blimp/client/core/rende...
File blimp/client/core/render_widget/blimp_document.cc (right):

https://codereview.chromium.org/2400923002/diff/60001/blimp/client/core/rende...
blimp/client/core/render_widget/blimp_document.cc:31: DCHECK(!input_manager_);
You shouldn't DCHECK. The |input_manager_| has not been initialized here.

[commit-bot: I haz the power, 2016-10-10 22:40:55]

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

[commit-bot: I haz the power, 2016-10-10 22:40:56]

Dry run: This issue passed the CQ dry run.

[xingliu, 2016-10-10 23:00:17]

Removed the DCHECK in the ctor.

https://codereview.chromium.org/2400923002/diff/60001/blimp/client/core/rende...
File blimp/client/core/render_widget/blimp_document.cc (right):

https://codereview.chromium.org/2400923002/diff/60001/blimp/client/core/rende...
blimp/client/core/render_widget/blimp_document.cc:31: DCHECK(!input_manager_);
On 2016/10/10 21:54:54, Khushal wrote:
> You shouldn't DCHECK. The |input_manager_| has not been initialized here.

Done, yeah, seems DCHECK previously is not in ctor.

[xingliu, 2016-10-11 01:02:21]

The CQ bit was checked by xingliu@chromium.org

[xingliu, 2016-10-11 01:02:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2400923002/#ps80001
(title: "Removed unnecessary DCHECK.")

[commit-bot: I haz the power, 2016-10-11 01:02:55]

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-10-11 02:11:27]

Description was changed from

==========
Move BlimpInputManager to BlimpDocument from BlimpCompositor.

Previously BlimpCompositor also routes the input events,
And we have the following call chain:

UI->BlimpInputManager->BlimpCompositor->BlimpDocument->
BlimpDocumentManager->RenderWidgetFeature.


This CL moves the ownership of BlimpInputManager to BlimpDocument, the
call chain of sending input event to engine becomes:

UI->BlimpInputManager->BlimpDocument->BlimpDocumentManager->
RenderWidgetFeature.


BUG=644326
==========

to

==========
Move BlimpInputManager to BlimpDocument from BlimpCompositor.

Previously BlimpCompositor also routes the input events,
And we have the following call chain:

UI->BlimpInputManager->BlimpCompositor->BlimpDocument->
BlimpDocumentManager->RenderWidgetFeature.


This CL moves the ownership of BlimpInputManager to BlimpDocument, the
call chain of sending input event to engine becomes:

UI->BlimpInputManager->BlimpDocument->BlimpDocumentManager->
RenderWidgetFeature.


BUG=644326
==========

[commit-bot: I haz the power, 2016-10-11 02:11:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-11 02:19:02]

Description was changed from

==========
Move BlimpInputManager to BlimpDocument from BlimpCompositor.

Previously BlimpCompositor also routes the input events,
And we have the following call chain:

UI->BlimpInputManager->BlimpCompositor->BlimpDocument->
BlimpDocumentManager->RenderWidgetFeature.


This CL moves the ownership of BlimpInputManager to BlimpDocument, the
call chain of sending input event to engine becomes:

UI->BlimpInputManager->BlimpDocument->BlimpDocumentManager->
RenderWidgetFeature.


BUG=644326
==========

to

==========
Move BlimpInputManager to BlimpDocument from BlimpCompositor.

Previously BlimpCompositor also routes the input events,
And we have the following call chain:

UI->BlimpInputManager->BlimpCompositor->BlimpDocument->
BlimpDocumentManager->RenderWidgetFeature.

This CL moves the ownership of BlimpInputManager to BlimpDocument, the
call chain of sending input event to engine becomes:

UI->BlimpInputManager->BlimpDocument->BlimpDocumentManager->
RenderWidgetFeature.

BUG=644326

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

[commit-bot: I haz the power, 2016-10-11 02:19:04]

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