Headless: Support sending and receiving of raw protocol messages

Headless: Support sending and receiving of raw protocol messages

Also adds support for finding the render tree node id, if any, for a given devtools
agent host id.

BUG=546953
Review-Url: https://codereview.chromium.org/2812253002
Cr-Commit-Position: refs/heads/master@{#465658}
Committed: https://chromium.googlesource.com/chromium/src/+/a936ad274d8d2f8967fb39dd8150488e0383ab8f

[alex clarke (OOO till 29th), 2017-04-12 09:27:53]

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

[alex clarke (OOO till 29th), 2017-04-12 09:28:05]

alexclarke@chromium.org changed reviewers:
+ skyostil@chromium.org

[alex clarke (OOO till 29th), 2017-04-12 09:28:06]

PTAL

[commit-bot: I haz the power, 2017-04-12 09:28:14]

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-12 10:09:46]

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

[commit-bot: I haz the power, 2017-04-12 10:09:46]

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

[Sami, 2017-04-12 14:54:24]

https://codereview.chromium.org/2812253002/diff/1/headless/lib/browser/headle...
File headless/lib/browser/headless_devtools_client_impl.cc (right):

https://codereview.chromium.org/2812253002/diff/1/headless/lib/browser/headle...
headless/lib/browser/headless_devtools_client_impl.cc:116: DCHECK_EQ((id % 2),
1) << "Raw devtools messages must have an odd ID.";
Could we instead add a generator getter for next_message_id_? That way this is
less error prone on the embedder side.

https://codereview.chromium.org/2812253002/diff/1/headless/public/headless_de...
File headless/public/headless_devtools_client.h (right):

https://codereview.chromium.org/2812253002/diff/1/headless/public/headless_de...
headless/public/headless_devtools_client.h:154: virtual void OnProtocolMessage(
This feels a little asymmetric since you send a string but get back both a
string and a dict. How about we make call this as the first thing with just the
raw string and have it return true/false to choose whether the built-in callback
gets called or not?

https://codereview.chromium.org/2812253002/diff/1/headless/public/headless_de...
headless/public/headless_devtools_client.h:166: virtual void
SendRawDevtoolsMessage(const std::string& json_message) = 0;
s/Devtools/DevTools/

[alex clarke (OOO till 29th), 2017-04-13 10:46:28]

PTAL

https://codereview.chromium.org/2812253002/diff/1/headless/lib/browser/headle...
File headless/lib/browser/headless_devtools_client_impl.cc (right):

https://codereview.chromium.org/2812253002/diff/1/headless/lib/browser/headle...
headless/lib/browser/headless_devtools_client_impl.cc:116: DCHECK_EQ((id % 2),
1) << "Raw devtools messages must have an odd ID.";
On 2017/04/12 14:54:24, Sami wrote:
> Could we instead add a generator getter for next_message_id_? That way this is
> less error prone on the embedder side.

Added a function as discussed offline.

https://codereview.chromium.org/2812253002/diff/1/headless/public/headless_de...
File headless/public/headless_devtools_client.h (right):

https://codereview.chromium.org/2812253002/diff/1/headless/public/headless_de...
headless/public/headless_devtools_client.h:154: virtual void OnProtocolMessage(
On 2017/04/12 14:54:24, Sami wrote:
> This feels a little asymmetric since you send a string but get back both a
> string and a dict. How about we make call this as the first thing with just
the
> raw string and have it return true/false to choose whether the built-in
callback
> gets called or not?

As discussed offline we added void SendRawDevToolsMessage(const
base::DictionaryValue* message).

https://codereview.chromium.org/2812253002/diff/1/headless/public/headless_de...
headless/public/headless_devtools_client.h:166: virtual void
SendRawDevtoolsMessage(const std::string& json_message) = 0;
On 2017/04/12 14:54:24, Sami wrote:
> s/Devtools/DevTools/

Done.

[alex clarke (OOO till 29th), 2017-04-13 10:46:36]

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

[commit-bot: I haz the power, 2017-04-13 10:46:49]

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-13 11:12:30]

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

[commit-bot: I haz the power, 2017-04-13 11:12:31]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2017-04-19 13:16:50]

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

[commit-bot: I haz the power, 2017-04-19 13:17: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, 2017-04-19 13:48:19]

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

[commit-bot: I haz the power, 2017-04-19 13:48:20]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2017-04-19 14:21:42]

Description was changed from

==========
Headless: Support sending and receiving of raw protocol messages

BUG=546953
==========

to

==========
Headless: Support sending and receiving of raw protocol messages

Also adds support for finding the render tree node id, if any, for a given
devtools
agent host id.

BUG=546953
==========

[Sami, 2017-04-19 15:24:43]

lgtm with a few nits, thanks!

https://codereview.chromium.org/2812253002/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_devtools_client_impl.h (right):

https://codereview.chromium.org/2812253002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_devtools_client_impl.h:100: void
SendRawDevToolsMessage(const base::DictionaryValue* message) override;
nit: const ref to make it obvious this can't be null?

https://codereview.chromium.org/2812253002/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_web_contents_impl.h (right):

https://codereview.chromium.org/2812253002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_web_contents_impl.h:58: bool
GetRenderTreeNodeIdForDevToolsAgentHostId(
GetFrameTreeNode... I think is more accurate?

https://codereview.chromium.org/2812253002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_web_contents_impl.h:60: int* render_tree_node_id)
const override;
frame_tree_node_id

https://codereview.chromium.org/2812253002/diff/40001/headless/public/headles...
File headless/public/headless_devtools_client.h (right):

https://codereview.chromium.org/2812253002/diff/40001/headless/public/headles...
headless/public/headless_devtools_client.h:158: const base::DictionaryValue*
parsed_message) = 0;
nit: should |parsed_message| also be a const ref since it's never going to be
null?

[alex clarke (OOO till 29th), 2017-04-19 16:30:33]

Thanks

https://codereview.chromium.org/2812253002/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_devtools_client_impl.h (right):

https://codereview.chromium.org/2812253002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_devtools_client_impl.h:100: void
SendRawDevToolsMessage(const base::DictionaryValue* message) override;
On 2017/04/19 15:24:43, Sami wrote:
> nit: const ref to make it obvious this can't be null?

Done.

https://codereview.chromium.org/2812253002/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_web_contents_impl.h (right):

https://codereview.chromium.org/2812253002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_web_contents_impl.h:58: bool
GetRenderTreeNodeIdForDevToolsAgentHostId(
On 2017/04/19 15:24:43, Sami wrote:
> GetFrameTreeNode... I think is more accurate?

Done.

https://codereview.chromium.org/2812253002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_web_contents_impl.h:60: int* render_tree_node_id)
const override;
On 2017/04/19 15:24:43, Sami wrote:
> frame_tree_node_id

Done.

https://codereview.chromium.org/2812253002/diff/40001/headless/public/headles...
File headless/public/headless_devtools_client.h (right):

https://codereview.chromium.org/2812253002/diff/40001/headless/public/headles...
headless/public/headless_devtools_client.h:158: const base::DictionaryValue*
parsed_message) = 0;
On 2017/04/19 15:24:43, Sami wrote:
> nit: should |parsed_message| also be a const ref since it's never going to be
> null?

Done.

[alex clarke (OOO till 29th), 2017-04-19 16:30:40]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2017-04-19 16:30:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2812253002/#ps60001
(title: "Changes for Sami")

[commit-bot: I haz the power, 2017-04-19 16:31:13]

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-19 16:44:45]

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

[commit-bot: I haz the power, 2017-04-19 16:44:46]

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

[alex clarke (OOO till 29th), 2017-04-19 17:09:05]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2017-04-19 17:09:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2812253002/#ps80001
(title: "Fix tests")

[commit-bot: I haz the power, 2017-04-19 17:09:40]

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

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492621745936530,
"parent_rev": "4774072a7a9b4b64adb17234eaadc922d1409439", "commit_rev":
"a936ad274d8d2f8967fb39dd8150488e0383ab8f"}

[commit-bot: I haz the power, 2017-04-19 17:36:50]

Description was changed from

==========
Headless: Support sending and receiving of raw protocol messages

Also adds support for finding the render tree node id, if any, for a given
devtools
agent host id.

BUG=546953
==========

to

==========
Headless: Support sending and receiving of raw protocol messages

Also adds support for finding the render tree node id, if any, for a given
devtools
agent host id.

BUG=546953

Review-Url: https://codereview.chromium.org/2812253002
Cr-Commit-Position: refs/heads/master@{#465658}
Committed:
https://chromium.googlesource.com/chromium/src/+/a936ad274d8d2f8967fb39dd8150...
==========

[commit-bot: I haz the power, 2017-04-19 17:36:52]

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