DevTools: introduce browser domain for basic target discovery.

DevTools: introduce browser domain for basic target discovery.

TBR=reillyg trivial change to internal function signature.
NOTRY=true

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

[pfeldman, 2016-06-07 21:34:36]

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

[commit-bot: I haz the power, 2016-06-07 21:35:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2047833002/1

[commit-bot: I haz the power, 2016-06-07 22:14:35]

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

[commit-bot: I haz the power, 2016-06-07 22:14:36]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[pfeldman, 2016-06-07 23:43:48]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2016-06-07 23:44:32]

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

[commit-bot: I haz the power, 2016-06-07 23:44:33]

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

[commit-bot: I haz the power, 2016-06-07 23:44:34]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[pfeldman, 2016-06-08 00:13:29]

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

[commit-bot: I haz the power, 2016-06-08 00:14:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2047833002/20001

[commit-bot: I haz the power, 2016-06-08 02:48:43]

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

[commit-bot: I haz the power, 2016-06-08 02:48:44]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2016-06-08 03:26:59]

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

[dgozman, 2016-06-08 03:53:29]

https://codereview.chromium.org/2047833002/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/browser_handler.cc (right):

https://codereview.chromium.org/2047833002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/browser_handler.cc:68:
Response::ServerError("Host is already being debugged");
s/Host/Target in error messages

https://codereview.chromium.org/2047833002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/browser_handler.cc:100: }
Should we send "detached" event to the target? But then, we should also send
"closed" event.

https://codereview.chromium.org/2047833002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/browser_handler.cc:102: 
nit: extra blank line

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

https://codereview.chromium.org/2047833002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/browser_handler.h:26: class BrowserHandler :
DevToolsAgentHostClient {
style: missing public/private before super-class.

https://codereview.chromium.org/2047833002/diff/20001/content/public/browser/...
File content/public/browser/devtools_agent_host.h (right):

https://codereview.chromium.org/2047833002/diff/20001/content/public/browser/...
content/public/browser/devtools_agent_host.h:111: // Attaches |client| to this
agent host to start debugging.
Let's comment when this may return false.

https://codereview.chromium.org/2047833002/diff/20001/content/public/browser/...
content/public/browser/devtools_agent_host.h:116: virtual bool
ForceAttachClient(DevToolsAgentHostClient* client) = 0;
Can this ever return false?

https://codereview.chromium.org/2047833002/diff/20001/content/public/browser/...
content/public/browser/devtools_agent_host.h:125: virtual bool
DispatchProtocolMessage(const std::string& message) = 0;
Let's add client parameter here as well, to be sure we handle messages from the
current client.
It will also help later with multi-client.

https://codereview.chromium.org/2047833002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2047833002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4218: "domain":
"Browser",
Will we migrate targets from ServiceWorker domain to this one?

https://codereview.chromium.org/2047833002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4263: "name":
"sendMessage",
Oh-oh. Sounds like a duplication of worker domain. Should we merge them? If so,
maybe Browser is not that good of a name.

[pfeldman, 2016-06-08 15:54:53]

https://codereview.chromium.org/2047833002/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/browser_handler.cc (right):

https://codereview.chromium.org/2047833002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/browser_handler.cc:68:
Response::ServerError("Host is already being debugged");
On 2016/06/08 03:53:29, dgozman_slow wrote:
> s/Host/Target in error messages

Done.

https://codereview.chromium.org/2047833002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/browser_handler.cc:100: }
On 2016/06/08 03:53:29, dgozman_slow wrote:
> Should we send "detached" event to the target? But then, we should also send
> "closed" event.

Don't we deliver detach reason already or should I do this right here?

https://codereview.chromium.org/2047833002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/browser_handler.cc:102: 
On 2016/06/08 03:53:29, dgozman_slow wrote:
> nit: extra blank line

Done.

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

https://codereview.chromium.org/2047833002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/browser_handler.h:26: class BrowserHandler :
DevToolsAgentHostClient {
On 2016/06/08 03:53:29, dgozman_slow wrote:
> style: missing public/private before super-class.

Done.

https://codereview.chromium.org/2047833002/diff/20001/content/public/browser/...
File content/public/browser/devtools_agent_host.h (right):

https://codereview.chromium.org/2047833002/diff/20001/content/public/browser/...
content/public/browser/devtools_agent_host.h:111: // Attaches |client| to this
agent host to start debugging.
On 2016/06/08 03:53:29, dgozman_slow wrote:
> Let's comment when this may return false.

Done.

https://codereview.chromium.org/2047833002/diff/20001/content/public/browser/...
content/public/browser/devtools_agent_host.h:116: virtual bool
ForceAttachClient(DevToolsAgentHostClient* client) = 0;
On 2016/06/08 03:53:29, dgozman_slow wrote:
> Can this ever return false?

Done.

https://codereview.chromium.org/2047833002/diff/20001/content/public/browser/...
content/public/browser/devtools_agent_host.h:125: virtual bool
DispatchProtocolMessage(const std::string& message) = 0;
On 2016/06/08 03:53:29, dgozman_slow wrote:
> Let's add client parameter here as well, to be sure we handle messages from
the
> current client.
> It will also help later with multi-client.

Done.

https://codereview.chromium.org/2047833002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2047833002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4218: "domain":
"Browser",
On 2016/06/08 03:53:29, dgozman_slow wrote:
> Will we migrate targets from ServiceWorker domain to this one?

Yes

https://codereview.chromium.org/2047833002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4263: "name":
"sendMessage",
On 2016/06/08 03:53:29, dgozman_slow wrote:
> Oh-oh. Sounds like a duplication of worker domain. Should we merge them? If
so,
> maybe Browser is not that good of a name.

I don't plan on merging them atm. Are you suggesting to rename it to Subtargets?
I can think of it in a follow-up.

[dgozman, 2016-06-08 15:59:09]

lgtm

https://codereview.chromium.org/2047833002/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/browser_handler.cc (right):

https://codereview.chromium.org/2047833002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/browser_handler.cc:100: }
On 2016/06/08 15:54:52, pfeldman wrote:
> On 2016/06/08 03:53:29, dgozman_slow wrote:
> > Should we send "detached" event to the target? But then, we should also send
> > "closed" event.
> 
> Don't we deliver detach reason already or should I do this right here?

We already do. I think it may be better to deliver if we go down the discovery
way, but let's wait with it.

https://codereview.chromium.org/2047833002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2047833002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4263: "name":
"sendMessage",
On 2016/06/08 15:54:53, pfeldman wrote:
> On 2016/06/08 03:53:29, dgozman_slow wrote:
> > Oh-oh. Sounds like a duplication of worker domain. Should we merge them? If
> so,
> > maybe Browser is not that good of a name.
> 
> I don't plan on merging them atm. Are you suggesting to rename it to
Subtargets?
> I can think of it in a follow-up.

Yeah, something along this lines.

[pfeldman, 2016-06-08 17:42:07]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-06-08 17:42:07]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2047833002/#ps40001
(title: "review comments")

[commit-bot: I haz the power, 2016-06-08 17:42:21]

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

[pfeldman, 2016-06-08 17:43:22]

The CQ bit was unchecked by pfeldman@chromium.org

[pfeldman, 2016-06-08 18:09:28]

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

[commit-bot: I haz the power, 2016-06-08 18:09:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2047833002/60001

[pfeldman, 2016-06-08 18:54:44]

Description was changed from

==========
DevTools: introduce browser domain for basic target discovery.
==========

to

==========
DevTools: introduce browser domain for basic target discovery.

TBR=reillyg trivial change to internal function signature.
==========

[pfeldman, 2016-06-08 18:54:44]

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

[commit-bot: I haz the power, 2016-06-08 20:35:50]

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

[commit-bot: I haz the power, 2016-06-08 20:35:51]

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

[pfeldman, 2016-06-08 21:06:23]

Description was changed from

==========
DevTools: introduce browser domain for basic target discovery.

TBR=reillyg trivial change to internal function signature.
==========

to

==========
DevTools: introduce browser domain for basic target discovery.

TBR=reillyg trivial change to internal function signature.
NOTRY=true
==========

[pfeldman, 2016-06-08 21:06:29]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-06-08 21:06:29]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2047833002/#ps60001
(title: "review comments addressed")

[commit-bot: I haz the power, 2016-06-08 21:07:14]

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

[commit-bot: I haz the power, 2016-06-08 21:13:54]

Description was changed from

==========
DevTools: introduce browser domain for basic target discovery.

TBR=reillyg trivial change to internal function signature.
NOTRY=true
==========

to

==========
DevTools: introduce browser domain for basic target discovery.

TBR=reillyg trivial change to internal function signature.
NOTRY=true
==========

[commit-bot: I haz the power, 2016-06-08 21:13:57]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-08 21:16:32]

Description was changed from

==========
DevTools: introduce browser domain for basic target discovery.

TBR=reillyg trivial change to internal function signature.
NOTRY=true
==========

to

==========
DevTools: introduce browser domain for basic target discovery.

TBR=reillyg trivial change to internal function signature.
NOTRY=true

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

[commit-bot: I haz the power, 2016-06-08 21:16:34]

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