Add HeadlessTabSocket

Add HeadlessTabSocket

This lets a C++ embedder send messages to and from JS on a HeadlessWebContents.
Note for this to work the headless browser context must have been created with
AddTabSocketMojoBindings and the headless web contents must have been created
with CreateTabSocket(true).

This will not affect chrome.exe --headless because only a C++ embedder can set
the above options.

Note that C++ embedders can already do this and more with the current headless API.
At a later date we will remove the more general headless mojo interface in favor of
TabSockets, unless it turns out somebody is actually using them.

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

[alex clarke (OOO till 29th), 2017-04-11 18:45:49]

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

[commit-bot: I haz the power, 2017-04-11 18:46:19]

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-11 18:50:39]

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

[commit-bot: I haz the power, 2017-04-11 18:50:39]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[alex clarke (OOO till 29th), 2017-04-11 18:52:23]

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

[alex clarke (OOO till 29th), 2017-04-11 18:52:24]

mkwst@ please review headless/lib/tab_socket.mojom

This is only for use by C++ embedders (they can do this and more already with
our API).   Happy to provide more context over chat or vc.

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

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

[commit-bot: I haz the power, 2017-04-11 19:07:40]

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

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

Description was changed from

==========
Add HeadlessTabSocket

This lets a C++ embedder send messages to and from JS on a HeadlessWebContents.
Note for this to work the headless browser context must have been created with
AddTabSocketMojoBindings and the headless web contents must have been created
with CreateTabSocket(true).

This will not affect chrome.exe --headless because only a C++ embedder can set
the above options.

Later we will remove the more general headless mojo interface.  I'm unaware of
anyone using this outside of google.

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

to

==========
Add HeadlessTabSocket

This lets a C++ embedder send messages to and from JS on a HeadlessWebContents.
Note for this to work the headless browser context must have been created with
AddTabSocketMojoBindings and the headless web contents must have been created
with CreateTabSocket(true).

This will not affect chrome.exe --headless because only a C++ embedder can set
the above options.

Note that C++ embedders can already do this and more with the current headless
API.
Later we will remove the more general headless mojo interface in favor of these
TabSockets, unless it turns out somebody is actually using them.

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

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

Description was changed from

==========
Add HeadlessTabSocket

This lets a C++ embedder send messages to and from JS on a HeadlessWebContents.
Note for this to work the headless browser context must have been created with
AddTabSocketMojoBindings and the headless web contents must have been created
with CreateTabSocket(true).

This will not affect chrome.exe --headless because only a C++ embedder can set
the above options.

Note that C++ embedders can already do this and more with the current headless
API.
Later we will remove the more general headless mojo interface in favor of these
TabSockets, unless it turns out somebody is actually using them.

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

to

==========
Add HeadlessTabSocket

This lets a C++ embedder send messages to and from JS on a HeadlessWebContents.
Note for this to work the headless browser context must have been created with
AddTabSocketMojoBindings and the headless web contents must have been created
with CreateTabSocket(true).

This will not affect chrome.exe --headless because only a C++ embedder can set
the above options.

Note that C++ embedders can already do this and more with the current headless
API.
Later we will remove the more general headless mojo interface in favor of these
TabSockets, unless it turns out somebody is actually using them.

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

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

Description was changed from

==========
Add HeadlessTabSocket

This lets a C++ embedder send messages to and from JS on a HeadlessWebContents.
Note for this to work the headless browser context must have been created with
AddTabSocketMojoBindings and the headless web contents must have been created
with CreateTabSocket(true).

This will not affect chrome.exe --headless because only a C++ embedder can set
the above options.

Note that C++ embedders can already do this and more with the current headless
API.
Later we will remove the more general headless mojo interface in favor of these
TabSockets, unless it turns out somebody is actually using them.

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

to

==========
Add HeadlessTabSocket

This lets a C++ embedder send messages to and from JS on a HeadlessWebContents.
Note for this to work the headless browser context must have been created with
AddTabSocketMojoBindings and the headless web contents must have been created
with CreateTabSocket(true).

This will not affect chrome.exe --headless because only a C++ embedder can set
the above options.

Note that C++ embedders can already do this and more with the current headless
API.
At a later date we will remove the more general headless mojo interface in favor
of
TabSockets, unless it turns out somebody is actually using them.

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

[commit-bot: I haz the power, 2017-04-11 19:22:16]

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

[commit-bot: I haz the power, 2017-04-11 19:22:17]

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

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

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

[commit-bot: I haz the power, 2017-04-11 19:42:26]

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-11 21:09:22]

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

[commit-bot: I haz the power, 2017-04-11 21:09:23]

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

[alex clarke (OOO till 29th), 2017-04-11 22:17:33]

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

[commit-bot: I haz the power, 2017-04-11 22:18:42]

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-11 23:35:21]

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

[commit-bot: I haz the power, 2017-04-11 23:35:21]

Dry run: This issue passed the CQ dry run.

[Sami, 2017-04-12 14:18:46]

Looks great! A couple of comments below.

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/browser/he...
File headless/lib/browser/headless_tab_socket_impl.cc (right):

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/browser/he...
headless/lib/browser/headless_tab_socket_impl.cc:18:
waiting_for_message_cb_.Run(message);
Ditto about locking.

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/browser/he...
headless/lib/browser/headless_tab_socket_impl.cc:31:
listener_->OnMessageFromTab(message);
Could we do this outside the lock? Otherwise you can't always reply to a message
from the callback.

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/browser/he...
headless/lib/browser/headless_tab_socket_impl.cc:52:
callback.Run(out_going_message_queue_.front());
Ditto about locking.

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/browser/he...
File headless/lib/browser/headless_tab_socket_impl.h (right):

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/browser/he...
headless/lib/browser/headless_tab_socket_impl.h:38: std::list<std::string>
out_going_message_queue_;
nit: outgoing_message_queue_

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/browser/he...
headless/lib/browser/headless_tab_socket_impl.h:39: std::list<std::string>
incomming_message_queue_;
typo: incoming

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/renderer/h...
File headless/lib/renderer/headless_content_renderer_client.cc (right):

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/renderer/h...
headless/lib/renderer/headless_content_renderer_client.cc:34: if
(render_frame->GetEnabledBindings() &
nit: Could be an early out to reduce the indentation

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/renderer/h...
headless/lib/renderer/headless_content_renderer_client.cc:39: let
messages_to_send_ = [];
nit: messagesToSend

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/renderer/h...
headless/lib/renderer/headless_content_renderer_client.cc:51:
window.TabSocket.SendMessageToEmbedder = function(message) {
Maybe this should just also be send() to match WebSocket.

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/renderer/h...
headless/lib/renderer/headless_content_renderer_client.cc:59:
window.TabSocket.SetListener = function(listener) {
Could we make the caller set "TabSocket.onmessage = function..." to match the
WebSocket API?

https://codereview.chromium.org/2813953002/diff/60001/headless/public/headles...
File headless/public/headless_tab_socket.h (right):

https://codereview.chromium.org/2813953002/diff/60001/headless/public/headles...
headless/public/headless_tab_socket.h:23: virtual void OnMessageFromTab(const
std::string& message) = 0;
Maybe add a note here reminding that the message may be potentially sent by
untrusted web content so it should be validated carefully.

https://codereview.chromium.org/2813953002/diff/60001/headless/public/headles...
File headless/public/headless_web_contents.h (right):

https://codereview.chromium.org/2813953002/diff/60001/headless/public/headles...
headless/public/headless_web_contents.h:102: // TODO(alexclarke): Remove
AddMojoService.
Please also mention that this is deprecated.

[alex clarke (OOO till 29th), 2017-04-12 15:47:40]

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

[alex clarke (OOO till 29th), 2017-04-12 15:47:46]

PTAL

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/browser/he...
File headless/lib/browser/headless_tab_socket_impl.cc (right):

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/browser/he...
headless/lib/browser/headless_tab_socket_impl.cc:18:
waiting_for_message_cb_.Run(message);
On 2017/04/12 14:18:45, Sami wrote:
> Ditto about locking.

Done.

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/browser/he...
headless/lib/browser/headless_tab_socket_impl.cc:31:
listener_->OnMessageFromTab(message);
On 2017/04/12 14:18:45, Sami wrote:
> Could we do this outside the lock? Otherwise you can't always reply to a
message
> from the callback.

Done.

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/browser/he...
headless/lib/browser/headless_tab_socket_impl.cc:52:
callback.Run(out_going_message_queue_.front());
On 2017/04/12 14:18:45, Sami wrote:
> Ditto about locking.

Done.

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/browser/he...
File headless/lib/browser/headless_tab_socket_impl.h (right):

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/browser/he...
headless/lib/browser/headless_tab_socket_impl.h:38: std::list<std::string>
out_going_message_queue_;
On 2017/04/12 14:18:45, Sami wrote:
> nit: outgoing_message_queue_

Done.

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/browser/he...
headless/lib/browser/headless_tab_socket_impl.h:39: std::list<std::string>
incomming_message_queue_;
On 2017/04/12 14:18:45, Sami wrote:
> typo: incoming

Done.

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/renderer/h...
File headless/lib/renderer/headless_content_renderer_client.cc (right):

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/renderer/h...
headless/lib/renderer/headless_content_renderer_client.cc:34: if
(render_frame->GetEnabledBindings() &
On 2017/04/12 14:18:46, Sami wrote:
> nit: Could be an early out to reduce the indentation

Done.

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/renderer/h...
headless/lib/renderer/headless_content_renderer_client.cc:39: let
messages_to_send_ = [];
On 2017/04/12 14:18:46, Sami wrote:
> nit: messagesToSend

Done.

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/renderer/h...
headless/lib/renderer/headless_content_renderer_client.cc:51:
window.TabSocket.SendMessageToEmbedder = function(message) {
On 2017/04/12 14:18:46, Sami wrote:
> Maybe this should just also be send() to match WebSocket.

Done.

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/renderer/h...
headless/lib/renderer/headless_content_renderer_client.cc:59:
window.TabSocket.SetListener = function(listener) {
On 2017/04/12 14:18:46, Sami wrote:
> Could we make the caller set "TabSocket.onmessage = function..." to match the
> WebSocket API?

Done.

https://codereview.chromium.org/2813953002/diff/60001/headless/public/headles...
File headless/public/headless_tab_socket.h (right):

https://codereview.chromium.org/2813953002/diff/60001/headless/public/headles...
headless/public/headless_tab_socket.h:23: virtual void OnMessageFromTab(const
std::string& message) = 0;
On 2017/04/12 14:18:46, Sami wrote:
> Maybe add a note here reminding that the message may be potentially sent by
> untrusted web content so it should be validated carefully.

Done.

https://codereview.chromium.org/2813953002/diff/60001/headless/public/headles...
File headless/public/headless_web_contents.h (right):

https://codereview.chromium.org/2813953002/diff/60001/headless/public/headles...
headless/public/headless_web_contents.h:102: // TODO(alexclarke): Remove
AddMojoService.
On 2017/04/12 14:18:46, Sami wrote:
> Please also mention that this is deprecated.

Done.

[commit-bot: I haz the power, 2017-04-12 15:48:00]

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

[Sami, 2017-04-12 16:09:06]

lgtm, thanks!

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/browser/he...
File headless/lib/browser/headless_tab_socket_impl.cc (right):

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/browser/he...
headless/lib/browser/headless_tab_socket_impl.cc:31:
listener_->OnMessageFromTab(message);
On 2017/04/12 15:47:46, alex clarke wrote:
> On 2017/04/12 14:18:45, Sami wrote:
> > Could we do this outside the lock? Otherwise you can't always reply to a
> message
> > from the callback.
> 
> Done.

Looks like we're still holding the lock here?

[commit-bot: I haz the power, 2017-04-12 17:01:44]

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

[commit-bot: I haz the power, 2017-04-12 17:01:47]

Dry run: This issue passed the CQ dry run.

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

Mike can you tae a look please? :)

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/browser/he...
File headless/lib/browser/headless_tab_socket_impl.cc (right):

https://codereview.chromium.org/2813953002/diff/60001/headless/lib/browser/he...
headless/lib/browser/headless_tab_socket_impl.cc:31:
listener_->OnMessageFromTab(message);
On 2017/04/12 16:09:06, Sami wrote:
> On 2017/04/12 15:47:46, alex clarke wrote:
> > On 2017/04/12 14:18:45, Sami wrote:
> > > Could we do this outside the lock? Otherwise you can't always reply to a
> > message
> > > from the callback.
> > 
> > Done.
> 
> Looks like we're still holding the lock here?

Oops done.

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

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

[commit-bot: I haz the power, 2017-04-12 19:43:25]

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 20:50:44]

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

[commit-bot: I haz the power, 2017-04-12 20:50:47]

Dry run: This issue passed the CQ dry run.

[Mike West, 2017-04-13 06:29:16]

LGTM.

[alex clarke (OOO till 29th), 2017-04-13 07:05:42]

Thanks!

[alex clarke (OOO till 29th), 2017-04-13 07:05:48]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2017-04-13 07:05:49]

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/2813953002/#ps100001
(title: "Change HeadlessTabSocketImpl::SetListener")

[commit-bot: I haz the power, 2017-04-13 07:06:18]

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 07:11:41]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492067148412410,
"parent_rev": "2a8fcff1676460930aab885766445035aa451625", "commit_rev":
"8196d431989e75a2fdbf19ddb42822b5d080da0e"}

[commit-bot: I haz the power, 2017-04-13 07:12:41]

Description was changed from

==========
Add HeadlessTabSocket

This lets a C++ embedder send messages to and from JS on a HeadlessWebContents.
Note for this to work the headless browser context must have been created with
AddTabSocketMojoBindings and the headless web contents must have been created
with CreateTabSocket(true).

This will not affect chrome.exe --headless because only a C++ embedder can set
the above options.

Note that C++ embedders can already do this and more with the current headless
API.
At a later date we will remove the more general headless mojo interface in favor
of
TabSockets, unless it turns out somebody is actually using them.

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

to

==========
Add HeadlessTabSocket

This lets a C++ embedder send messages to and from JS on a HeadlessWebContents.
Note for this to work the headless browser context must have been created with
AddTabSocketMojoBindings and the headless web contents must have been created
with CreateTabSocket(true).

This will not affect chrome.exe --headless because only a C++ embedder can set
the above options.

Note that C++ embedders can already do this and more with the current headless
API.
At a later date we will remove the more general headless mojo interface in favor
of
TabSockets, unless it turns out somebody is actually using them.

BUG=546953

Review-Url: https://codereview.chromium.org/2813953002
Cr-Commit-Position: refs/heads/master@{#464327}
Committed:
https://chromium.googlesource.com/chromium/src/+/8196d431989e75a2fdbf19ddb428...
==========

[commit-bot: I haz the power, 2017-04-13 07:12:42]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/8196d431989e75a2fdbf19ddb428...