[DevTools] Implement Target.setDiscoverTargets method.

[DevTools] Implement Target.setDiscoverTargets method.

When discovery is enabled, we force agents hosts for all web contents
and report all available agent hosts to the client.

BUG=648775
Committed: https://crrev.com/f00c4f7bc478b2f9ce9ba2dec7f18737ee461d70
Cr-Commit-Position: refs/heads/master@{#426239}

[dgozman, 2016-10-12 17:23:30]

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

[dgozman, 2016-10-12 17:23:30]

Could you please take a look?

[dgozman, 2016-10-12 17:23:42]

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

[commit-bot: I haz the power, 2016-10-12 17:24:04]

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-10-12 17:34:44]

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

[commit-bot: I haz the power, 2016-10-12 17:34:45]

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

[dgozman, 2016-10-12 21:10:23]

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

[commit-bot: I haz the power, 2016-10-12 21:10:52]

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-10-12 22:41:01]

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

[commit-bot: I haz the power, 2016-10-12 22:41:02]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2016-10-12 23:11:19]

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

https://codereview.chromium.org/2408133004/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_manager.cc:54: for (DevToolsAgentHostImpl*
agent_host : agent_hosts_) {
How can this happen?

https://codereview.chromium.org/2408133004/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_manager.cc:63: if (!delegate_initialized_) {
What is wrong with !delegate_ ?

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

https://codereview.chromium.org/2408133004/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_manager.h:47: void
WebContentsCreated(WebContentsImpl* web_contents);
These seem random / hacky.

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

https://codereview.chromium.org/2408133004/diff/20001/content/browser/devtool...
content/browser/devtools/forwarding_agent_host.cc:18:
DevToolsManager::GetInstance()->AgentHostCreated(this);
Why not the DevToolsAgentHostImpl?

[dgozman, 2016-10-12 23:23:12]

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

https://codereview.chromium.org/2408133004/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_manager.cc:54: for (DevToolsAgentHostImpl*
agent_host : agent_hosts_) {
On 2016/10/12 23:11:18, pfeldman wrote:
> How can this happen?

Service workers shutdown later (I think on IO) than this singleton is destroyed
on UI thread, keeping their agent hosts alive longer.

https://codereview.chromium.org/2408133004/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_manager.cc:63: if (!delegate_initialized_) {
On 2016/10/12 23:11:18, pfeldman wrote:
> What is wrong with !delegate_ ?

Client can pass a null one, I don't want to ask for it again.

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

https://codereview.chromium.org/2408133004/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_manager.h:47: void
WebContentsCreated(WebContentsImpl* web_contents);
On 2016/10/12 23:11:18, pfeldman wrote:
> These seem random / hacky.

Any ideas?

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

https://codereview.chromium.org/2408133004/diff/20001/content/browser/devtool...
content/browser/devtools/forwarding_agent_host.cc:18:
DevToolsManager::GetInstance()->AgentHostCreated(this);
On 2016/10/12 23:11:18, pfeldman wrote:
> Why not the DevToolsAgentHostImpl?

Subclasses are not yet fully initialized in
DevToolsAgentHostImpl::DevToolsAgentHostImpl, and we can query GetType() or
something as a result of this call.

[pfeldman, 2016-10-13 17:58:11]

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

https://codereview.chromium.org/2408133004/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_manager.cc:54: for (DevToolsAgentHostImpl*
agent_host : agent_hosts_) {
Could you mention it the comments here? Also, what if their destructors access
the manager? Btw, we already have g_instances in the DevToolsAgentHostImpl.

I'd rather make sure that devtoolsmanager outlives the hosts and stored pointers
to them in the manager, hence removing the g_instances. If that is not feasible,
I'd at least reuse the g_instances here.

[dgozman, 2016-10-14 23:30:22]

Please take a look at the new version!

[pfeldman, 2016-10-14 23:58:52]

Let's try splitting discovery vs autoattach entirely for the sake of code
sanity.

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

https://codereview.chromium.org/2408133004/diff/60001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.cc:299:
DevToolsAgentHost::GetOrCreateAll();
DevToolsAgentHost::GetOrCreateAll

DCHECK notified.

https://codereview.chromium.org/2408133004/diff/60001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.cc:359:
g_instances.Get().erase(g_instances.Get().find(id_));
Erase by id.

[dgozman, 2016-10-17 23:42:14]

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

[commit-bot: I haz the power, 2016-10-17 23:42:42]

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

[dgozman, 2016-10-17 23:45:11]

PTAL, all comments addressed.

[commit-bot: I haz the power, 2016-10-17 23:55:08]

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

[commit-bot: I haz the power, 2016-10-17 23:55:09]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dgozman, 2016-10-18 00:37:42]

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

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

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-10-18 01:07:33]

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

[commit-bot: I haz the power, 2016-10-18 01:07:34]

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_...)

[dgozman, 2016-10-19 01:49:13]

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

[commit-bot: I haz the power, 2016-10-19 01:49:37]

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-10-19 03:13:47]

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

[commit-bot: I haz the power, 2016-10-19 03:13:48]

Dry run: Exceeded global retry quota

[dgozman, 2016-10-19 05:45:38]

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

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

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-10-19 13:07:10]

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

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

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

[pfeldman, 2016-10-19 17:38:43]

lgtm

[dgozman, 2016-10-19 17:39:33]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2016-10-19 17:39:54]

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-19 17:45:40]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-10-21 13:10:00]

Description was changed from

==========
[DevTools] Implement Target.setDiscoverTargets method.

When discovery is enabled, we force agents hosts for all web contents
and report all available agent hosts to the client.

BUG=648775
==========

to

==========
[DevTools] Implement Target.setDiscoverTargets method.

When discovery is enabled, we force agents hosts for all web contents
and report all available agent hosts to the client.

BUG=648775

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

[commit-bot: I haz the power, 2016-10-21 13:10:02]

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