[DevTools] Rework DevToolsSession interaction with domain handlers.

[DevTools] Rework DevToolsSession interaction with domain handlers.

- Introduced base class DevToolsDomainHandler.
- Agent host registers handlers in DevToolsSession.
- Passing DevToolsSession instance around.

This is to:
- reduce boilerplate;
- prepare for multi-client (there will be multiple sessions).

BUG=590878
TBR=pfeldman

Committed: https://crrev.com/39d9d9ab10964aa9864273742dccc848b9c514bd
Cr-Commit-Position: refs/heads/master@{#440874}

[dgozman, 2016-12-20 19:57:07]

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

[dgozman, 2016-12-20 19:57:22]

dgozman@chromium.org changed reviewers:
+ caseq@chromium.org, pfeldman@chromium.org

[dgozman, 2016-12-20 19:57:23]

Could you please take a look?

[commit-bot: I haz the power, 2016-12-20 19:58: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-12-20 22:55:09]

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

[commit-bot: I haz the power, 2016-12-20 22:55:09]

Dry run: This issue passed the CQ dry run.

[caseq, 2016-12-20 23:13:20]

https://codereview.chromium.org/2590293003/diff/1/content/browser/devtools/de...
File content/browser/devtools/devtools_agent_host_impl.cc (right):

https://codereview.chromium.org/2590293003/diff/1/content/browser/devtools/de...
content/browser/devtools/devtools_agent_host_impl.cc:139: bool
DevToolsAgentHostImpl::InnerAttach(DevToolsAgentHostClient* client,
Let's rename to InnerAttachClient

https://codereview.chromium.org/2590293003/diff/1/content/browser/devtools/de...
content/browser/devtools/devtools_agent_host_impl.cc:150:
Attach(session_.get());
AttachSession?

https://codereview.chromium.org/2590293003/diff/1/content/browser/devtools/de...
File content/browser/devtools/devtools_session.h (right):

https://codereview.chromium.org/2590293003/diff/1/content/browser/devtools/de...
content/browser/devtools/devtools_session.h:70: bool attached_;
Let's try to get rid of that, as well as of Attach()/Detach(). It does not make
sense for a session to exist when it's not attached.

https://codereview.chromium.org/2590293003/diff/1/content/browser/devtools/pr...
File content/browser/devtools/protocol/devtools_domain_handler.h (right):

https://codereview.chromium.org/2590293003/diff/1/content/browser/devtools/pr...
content/browser/devtools/protocol/devtools_domain_handler.h:23: virtual Response
Disable();
I wonder if we can get rid of that. This is not the protocol method, so why
would it return Response? Should we just generate a generic protocol wrapper
around that?

[dgozman, 2016-12-21 00:29:40]

PTAL

https://codereview.chromium.org/2590293003/diff/1/content/browser/devtools/de...
File content/browser/devtools/devtools_agent_host_impl.cc (right):

https://codereview.chromium.org/2590293003/diff/1/content/browser/devtools/de...
content/browser/devtools/devtools_agent_host_impl.cc:139: bool
DevToolsAgentHostImpl::InnerAttach(DevToolsAgentHostClient* client,
On 2016/12/20 23:13:19, caseq wrote:
> Let's rename to InnerAttachClient

Done.

https://codereview.chromium.org/2590293003/diff/1/content/browser/devtools/de...
content/browser/devtools/devtools_agent_host_impl.cc:150:
Attach(session_.get());
On 2016/12/20 23:13:19, caseq wrote:
> AttachSession?

Done.

https://codereview.chromium.org/2590293003/diff/1/content/browser/devtools/de...
File content/browser/devtools/devtools_session.h (right):

https://codereview.chromium.org/2590293003/diff/1/content/browser/devtools/de...
content/browser/devtools/devtools_session.h:70: bool attached_;
On 2016/12/20 23:13:20, caseq wrote:
> Let's try to get rid of that, as well as of Attach()/Detach(). It does not
make
> sense for a session to exist when it's not attached.

Done.

https://codereview.chromium.org/2590293003/diff/1/content/browser/devtools/pr...
File content/browser/devtools/protocol/devtools_domain_handler.h (right):

https://codereview.chromium.org/2590293003/diff/1/content/browser/devtools/pr...
content/browser/devtools/protocol/devtools_domain_handler.h:23: virtual Response
Disable();
On 2016/12/20 23:13:20, caseq wrote:
> I wonder if we can get rid of that. This is not the protocol method, so why
> would it return Response? Should we just generate a generic protocol wrapper
> around that?

Per offline discussion: we didn't find a clear solution which is definitely a
win over this.

[dgozman, 2016-12-21 00:29:46]

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

[commit-bot: I haz the power, 2016-12-21 00:30:18]

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-12-21 02:29:05]

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

[commit-bot: I haz the power, 2016-12-21 02:29:05]

Dry run: This issue passed the CQ dry run.

[caseq, 2016-12-21 23:17:28]

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
File content/browser/devtools/devtools_agent_host_impl.cc (right):

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.cc:283: if
(agent_host->session_) {
nit: parametrize HostClosed() with bool and call it here.

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
File content/browser/devtools/devtools_session.cc (right):

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_session.cc:59: bool offer_to_delegate,
remove this?

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
File content/browser/devtools/devtools_session.h (right):

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_session.h:30: int session_id() { return
session_id_; }
nit: could be const.

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_session.h:41: protocol::InputHandler*
GetInputHandler();
Let's move these somewhere else, it would be nice for session to not know about
concrete handler types.

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/dom_handler.h (right):

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/dom_handler.h:18: class DOMHandler : public
DevToolsDomainHandler,
Should we perhaps inherit resp. Foo::backend classes from this?

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/memory_handler.h (right):

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/memory_handler.h:22: void
SetRenderFrameHost(RenderFrameHostImpl* host) override;
Here and elsewhere, nuke the override, the base is not pure anyway.

[dgozman, 2016-12-22 06:26:13]

Please take another look.

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
File content/browser/devtools/devtools_agent_host_impl.cc (right):

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.cc:283: if
(agent_host->session_) {
On 2016/12/21 23:17:27, caseq wrote:
> nit: parametrize HostClosed() with bool and call it here.

Done.

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
File content/browser/devtools/devtools_session.cc (right):

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_session.cc:59: bool offer_to_delegate,
On 2016/12/21 23:17:27, caseq wrote:
> remove this?

Ops, I meant to use it actually.

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
File content/browser/devtools/devtools_session.h (right):

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_session.h:30: int session_id() { return
session_id_; }
On 2016/12/21 23:17:27, caseq wrote:
> nit: could be const.

Done.

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_session.h:41: protocol::InputHandler*
GetInputHandler();
On 2016/12/21 23:17:27, caseq wrote:
> Let's move these somewhere else, it would be nice for session to not know
about
> concrete handler types.

Done.

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/dom_handler.h (right):

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/dom_handler.h:18: class DOMHandler : public
DevToolsDomainHandler,
On 2016/12/21 23:17:27, caseq wrote:
> Should we perhaps inherit resp. Foo::backend classes from this?

Not really.

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/memory_handler.h (right):

https://codereview.chromium.org/2590293003/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/memory_handler.h:22: void
SetRenderFrameHost(RenderFrameHostImpl* host) override;
On 2016/12/21 23:17:27, caseq wrote:
> Here and elsewhere, nuke the override, the base is not pure anyway.

Done.

[dgozman, 2016-12-22 06:26:20]

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

[commit-bot: I haz the power, 2016-12-22 06:26:28]

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-12-22 07:23:39]

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

[commit-bot: I haz the power, 2016-12-22 07:23:40]

Dry run: This issue passed the CQ dry run.

[caseq, 2016-12-23 00:38:41]

lgtm

https://codereview.chromium.org/2590293003/diff/40001/content/browser/devtool...
File content/browser/devtools/devtools_session.cc (right):

https://codereview.chromium.org/2590293003/diff/40001/content/browser/devtool...
content/browser/devtools/devtools_session.cc:11: #include
"content/browser/devtools/protocol/input_handler.h"
These should hopefully no longer be necessary :-)

[dgozman, 2016-12-27 20:01:52]

https://codereview.chromium.org/2590293003/diff/40001/content/browser/devtool...
File content/browser/devtools/devtools_session.cc (right):

https://codereview.chromium.org/2590293003/diff/40001/content/browser/devtool...
content/browser/devtools/devtools_session.cc:11: #include
"content/browser/devtools/protocol/input_handler.h"
On 2016/12/23 00:38:41, caseq wrote:
> These should hopefully no longer be necessary :-)

Done.

[dgozman, 2016-12-27 20:04:38]

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

[commit-bot: I haz the power, 2016-12-27 20:04:47]

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-12-27 21:00:54]

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

[commit-bot: I haz the power, 2016-12-27 21:00:55]

Dry run: This issue passed the CQ dry run.

[dgozman, 2016-12-28 18:51:18]

Description was changed from

==========
[DevTools] Rework DevToolsSession interaction with domain handlers.

- Introduced base class DevToolsDomainHandler.
- Agent host registers handlers in DevToolsSession.
- Passing DevToolsSession instance around.

This is to:
- reduce boilerplate;
- prepare for multi-client (there will be multiple sessions).

BUG=590878
==========

to

==========
[DevTools] Rework DevToolsSession interaction with domain handlers.

- Introduced base class DevToolsDomainHandler.
- Agent host registers handlers in DevToolsSession.
- Passing DevToolsSession instance around.

This is to:
- reduce boilerplate;
- prepare for multi-client (there will be multiple sessions).

BUG=590878
TBR=pfeldman
==========

[dgozman, 2016-12-28 18:51:35]

TBR'ing pfeldman for BUILD.gn

[dgozman, 2016-12-28 18:51:41]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2016-12-28 18:51:41]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org
Link to the patchset: https://codereview.chromium.org/2590293003/#ps60001
(title: "includes")

[commit-bot: I haz the power, 2016-12-28 18:51:49]

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-12-28 18:56:35]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482951101279930,
"parent_rev": "633c28a4d1b7ed7e564feefa74a94f050300f6b3", "commit_rev":
"ca3c04b5c69dc6198d888825c1d337471114d9a9"}

[commit-bot: I haz the power, 2016-12-28 18:57:03]

Description was changed from

==========
[DevTools] Rework DevToolsSession interaction with domain handlers.

- Introduced base class DevToolsDomainHandler.
- Agent host registers handlers in DevToolsSession.
- Passing DevToolsSession instance around.

This is to:
- reduce boilerplate;
- prepare for multi-client (there will be multiple sessions).

BUG=590878
TBR=pfeldman
==========

to

==========
[DevTools] Rework DevToolsSession interaction with domain handlers.

- Introduced base class DevToolsDomainHandler.
- Agent host registers handlers in DevToolsSession.
- Passing DevToolsSession instance around.

This is to:
- reduce boilerplate;
- prepare for multi-client (there will be multiple sessions).

BUG=590878
TBR=pfeldman

Review-Url: https://codereview.chromium.org/2590293003
==========

[commit-bot: I haz the power, 2016-12-28 18:57:04]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2017-01-02 15:49:45]

Description was changed from

==========
[DevTools] Rework DevToolsSession interaction with domain handlers.

- Introduced base class DevToolsDomainHandler.
- Agent host registers handlers in DevToolsSession.
- Passing DevToolsSession instance around.

This is to:
- reduce boilerplate;
- prepare for multi-client (there will be multiple sessions).

BUG=590878
TBR=pfeldman

Review-Url: https://codereview.chromium.org/2590293003
==========

to

==========
[DevTools] Rework DevToolsSession interaction with domain handlers.

- Introduced base class DevToolsDomainHandler.
- Agent host registers handlers in DevToolsSession.
- Passing DevToolsSession instance around.

This is to:
- reduce boilerplate;
- prepare for multi-client (there will be multiple sessions).

BUG=590878
TBR=pfeldman

Committed: https://crrev.com/39d9d9ab10964aa9864273742dccc848b9c514bd
Cr-Commit-Position: refs/heads/master@{#440874}
==========

[commit-bot: I haz the power, 2017-01-02 15:49:46]

Patchset 4 (id:??) landed as
https://crrev.com/39d9d9ab10964aa9864273742dccc848b9c514bd
Cr-Commit-Position: refs/heads/master@{#440874}