Host extensions

Host extensions

This CL introduces HostExtension, an interface for classes that
extend the host with non-core functionality.

Extensions are added to the ChromotingHost. They are used to compile
the list of capabilities reported to the client, which can be used by
the client to determine the availability of the extension.

When a client connects, a HostExtension has the opportunity to create
an HostExtensionSession to hold client/extension state, and to handle
extension messages from that client.

BUG=

NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273901

[Wez, 2014-05-28 01:05:58]

https://codereview.chromium.org/301453003/diff/40001/remoting/host/chromoting...
File remoting/host/chromoting_host.cc (right):

https://codereview.chromium.org/301453003/diff/40001/remoting/host/chromoting...
remoting/host/chromoting_host.cc:327: &extensions_.get());
This looks weird; does ScopedVector.get() return the bare vector, then? It would
look a lot less weird if you pass it to ClientSession as a |const Foo&| rather
than |const Foo *|

https://codereview.chromium.org/301453003/diff/40001/remoting/host/chromoting...
File remoting/host/chromoting_host.h (right):

https://codereview.chromium.org/301453003/diff/40001/remoting/host/chromoting...
remoting/host/chromoting_host.h:11: #include "base/memory/ref_counted.h"
Did you mean to add this dependency?

https://codereview.chromium.org/301453003/diff/40001/remoting/host/client_ses...
File remoting/host/client_session.cc (right):

https://codereview.chromium.org/301453003/diff/40001/remoting/host/client_ses...
remoting/host/client_session.cc:175: CreateExtensionSessions();
Why are we doing this here *and* in OnConnectionAuthenticated?

https://codereview.chromium.org/301453003/diff/40001/remoting/host/client_ses...
remoting/host/client_session.cc:288: CreateExtensionSessions();
Could you have the ChromotingHost add the extension in OnSessionAuthenticated(),
which is called shortly before this?

That way you can avoid passing the list of Extensions to ClientSession;
ChromotingHost would call ClientSession::AddExtension() for each extension, and
then once OnSessionAuthenticated() returns the ClientSession can compile the
list of capabilities from the ExtensionSessions.

https://codereview.chromium.org/301453003/diff/40001/remoting/host/client_ses...
File remoting/host/client_session.h (right):

https://codereview.chromium.org/301453003/diff/40001/remoting/host/client_ses...
remoting/host/client_session.h:108: const ExtensionList* extensions);
Ouch. It's a shame to have to keep a bare pointer to the list of extensions for
so long. ;(

https://codereview.chromium.org/301453003/diff/40001/remoting/host/client_ses...
File remoting/host/client_session_unittest.cc (right):

https://codereview.chromium.org/301453003/diff/40001/remoting/host/client_ses...
remoting/host/client_session_unittest.cc:723: InvokeWithoutArgs(this,
&ClientSessionTest::StopClientSession)));
This chunk of work is unrelated to the test, aside from the DeliverClientMessage
call; could it be factored out from the three tests to make them more readable?
e.g. into a ConnectClientAndSendExtensionMessage(...)

https://codereview.chromium.org/301453003/diff/40001/remoting/host/client_ses...
remoting/host/client_session_unittest.cc:760: // Verifies that host factors in
extension capabilties when reporting
typo:

https://codereview.chromium.org/301453003/diff/40001/remoting/host/client_ses...
remoting/host/client_session_unittest.cc:761: // capabilities to the client.
These lines look to be wrongly indented?

https://codereview.chromium.org/301453003/diff/40001/remoting/host/extension.h
File remoting/host/extension.h (right):

https://codereview.chromium.org/301453003/diff/40001/remoting/host/extension....
remoting/host/extension.h:12: namespace remoting {
Should this belong under remoting::protocol, since it's protocol-specific?

https://codereview.chromium.org/301453003/diff/40001/remoting/host/extension....
remoting/host/extension.h:19: class Extension {
Since this is in the remoting namespace, but host-specific, should it be
HostExtension?

https://codereview.chromium.org/301453003/diff/40001/remoting/host/extension....
remoting/host/extension.h:23: // extension.
How? Are they e.g. merged into the capabilities the host reports to the client
at startup?

https://codereview.chromium.org/301453003/diff/40001/remoting/host/extension....
remoting/host/extension.h:28: // It may return NULL.
Why would it do that? What happens if it does? Suggest e.g. "If NULL is returned
then the extension will not be available to the client session."

https://codereview.chromium.org/301453003/diff/40001/remoting/host/extension....
remoting/host/extension.h:31: ClientSession* client_session) = 0;
Why do we need to pass the ClientSession in here?

https://codereview.chromium.org/301453003/diff/40001/remoting/remoting_host.gypi
File remoting/remoting_host.gypi (right):

https://codereview.chromium.org/301453003/diff/40001/remoting/remoting_host.g...
remoting/remoting_host.gypi:123: 'host/extension_manager.h',
There's no extension_manager.h in this CL.

[dcaiafa, 2014-05-28 22:44:57]

ptal

https://codereview.chromium.org/301453003/diff/40001/remoting/host/chromoting...
File remoting/host/chromoting_host.cc (right):

https://codereview.chromium.org/301453003/diff/40001/remoting/host/chromoting...
remoting/host/chromoting_host.cc:327: &extensions_.get());
Removed.

https://codereview.chromium.org/301453003/diff/40001/remoting/host/chromoting...
File remoting/host/chromoting_host.h (right):

https://codereview.chromium.org/301453003/diff/40001/remoting/host/chromoting...
remoting/host/chromoting_host.h:11: #include "base/memory/ref_counted.h"
On 2014/05/28 01:05:59, Wez wrote:
> Did you mean to add this dependency?

It was already there. I just fixed the sort order :)

https://codereview.chromium.org/301453003/diff/40001/remoting/host/client_ses...
File remoting/host/client_session.cc (right):

https://codereview.chromium.org/301453003/diff/40001/remoting/host/client_ses...
remoting/host/client_session.cc:175: CreateExtensionSessions();
On 2014/05/28 01:05:59, Wez wrote:
> Why are we doing this here *and* in OnConnectionAuthenticated?

They are the two sites where client_capabilities_ are established and set in
desktop_environment_. In OnConnectionAuthenticated's case, the client doesn't
support capabilities.

https://codereview.chromium.org/301453003/diff/40001/remoting/host/client_ses...
remoting/host/client_session.cc:288: CreateExtensionSessions();
On 2014/05/28 01:05:59, Wez wrote:
> Could you have the ChromotingHost add the extension in
OnSessionAuthenticated(),
> which is called shortly before this?
> 
> That way you can avoid passing the list of Extensions to ClientSession;
> ChromotingHost would call ClientSession::AddExtension() for each extension,
and
> then once OnSessionAuthenticated() returns the ClientSession can compile the
> list of capabilities from the ExtensionSessions.

Done.

https://codereview.chromium.org/301453003/diff/40001/remoting/host/client_ses...
File remoting/host/client_session.h (right):

https://codereview.chromium.org/301453003/diff/40001/remoting/host/client_ses...
remoting/host/client_session.h:108: const ExtensionList* extensions);
On 2014/05/28 01:05:59, Wez wrote:
> Ouch. It's a shame to have to keep a bare pointer to the list of extensions
for
> so long. ;(

Removed.

https://codereview.chromium.org/301453003/diff/40001/remoting/host/client_ses...
File remoting/host/client_session_unittest.cc (right):

https://codereview.chromium.org/301453003/diff/40001/remoting/host/client_ses...
remoting/host/client_session_unittest.cc:723: InvokeWithoutArgs(this,
&ClientSessionTest::StopClientSession)));
On 2014/05/28 01:05:59, Wez wrote:
> This chunk of work is unrelated to the test, aside from the
DeliverClientMessage
> call; could it be factored out from the three tests to make them more
readable?
> e.g. into a ConnectClientAndSendExtensionMessage(...)

Done.

https://codereview.chromium.org/301453003/diff/40001/remoting/host/client_ses...
remoting/host/client_session_unittest.cc:760: // Verifies that host factors in
extension capabilties when reporting
On 2014/05/28 01:05:59, Wez wrote:
> typo:

Done.

https://codereview.chromium.org/301453003/diff/40001/remoting/host/client_ses...
remoting/host/client_session_unittest.cc:761: // capabilities to the client.
On 2014/05/28 01:05:59, Wez wrote:
> These lines look to be wrongly indented?

Done.

https://codereview.chromium.org/301453003/diff/40001/remoting/host/extension.h
File remoting/host/extension.h (right):

https://codereview.chromium.org/301453003/diff/40001/remoting/host/extension....
remoting/host/extension.h:12: namespace remoting {
On 2014/05/28 01:05:59, Wez wrote:
> Should this belong under remoting::protocol, since it's protocol-specific?

Per our offline conversation: it should be under remoting only because there is
no client analog (except in javascript).

https://codereview.chromium.org/301453003/diff/40001/remoting/host/extension....
remoting/host/extension.h:19: class Extension {
On 2014/05/28 01:05:59, Wez wrote:
> Since this is in the remoting namespace, but host-specific, should it be
> HostExtension?

Done.

https://codereview.chromium.org/301453003/diff/40001/remoting/host/extension....
remoting/host/extension.h:23: // extension.
On 2014/05/28 01:05:59, Wez wrote:
> How? Are they e.g. merged into the capabilities the host reports to the client
> at startup?

Done.

https://codereview.chromium.org/301453003/diff/40001/remoting/host/extension....
remoting/host/extension.h:28: // It may return NULL.
On 2014/05/28 01:05:59, Wez wrote:
> Why would it do that? What happens if it does? Suggest e.g. "If NULL is
returned
> then the extension will not be available to the client session."

Done.

https://codereview.chromium.org/301453003/diff/40001/remoting/host/extension....
remoting/host/extension.h:31: ClientSession* client_session) = 0;
On 2014/05/28 01:05:59, Wez wrote:
> Why do we need to pass the ClientSession in here?

So the extension can (among others):
- Query client capabilities
- Send extension messages
- Disconnect session on error

https://codereview.chromium.org/301453003/diff/40001/remoting/remoting_host.gypi
File remoting/remoting_host.gypi (right):

https://codereview.chromium.org/301453003/diff/40001/remoting/remoting_host.g...
remoting/remoting_host.gypi:123: 'host/extension_manager.h',
On 2014/05/28 01:05:59, Wez wrote:
> There's no extension_manager.h in this CL.

Done.

[Wez, 2014-05-28 23:03:41]

LGTM

It looks like AddHostCapabilities() is used for both HostExtension and
DesktopEnvironment capabilities; that wasn't obvious as I was reviewing the code
(the name implies it's capabilities of the ChromotingHost ;) so perhaps clarify
that in the method's comment, e.g.

"Adds extended capabilities to advertise to the client, e.g. those implemented
by DesktopEnvironment or HostExtensions." features."

https://codereview.chromium.org/301453003/diff/80001/remoting/host/chromoting...
File remoting/host/chromoting_host.cc (right):

https://codereview.chromium.org/301453003/diff/80001/remoting/host/chromoting...
remoting/host/chromoting_host.cc:219:
client->AddHostCapabilities((*extension)->GetCapabilities());
Do you really need to do this here? Could it wait until OnClientCapabilities, so
that you can derive the capabilities list from the list of extensions that you
have actually added to the ClientSession?

https://codereview.chromium.org/301453003/diff/80001/remoting/host/chromoting...
remoting/host/chromoting_host.cc:219:
client->AddHostCapabilities((*extension)->GetCapabilities());
If not then it would seem cleaner to have SetHostCapabilities(), and have the
host collate the list and pass it once, rather than having the ClientSession
accumulate the list.

https://codereview.chromium.org/301453003/diff/80001/remoting/host/client_ses...
File remoting/host/client_session.cc (right):

https://codereview.chromium.org/301453003/diff/80001/remoting/host/client_ses...
remoting/host/client_session.cc:116: host_capabilities_.append(" ");
See comment in ChromotingHost re doing this accumulation of capabilities in the
ChromotingHost code and just SetHostCapabilities() here.

https://codereview.chromium.org/301453003/diff/80001/remoting/host/client_ses...
File remoting/host/client_session.h (right):

https://codereview.chromium.org/301453003/diff/80001/remoting/host/client_ses...
remoting/host/client_session.h:71: virtual void
OnClientCapabilities(ClientSession* client) = 0;
All the other handlers are OnSession<Foo>; OnSessionCapabilities or
OnSessionClientCapabilities, for consistency?

https://codereview.chromium.org/301453003/diff/80001/remoting/host/client_ses...
remoting/host/client_session.h:97: // |event_handler|,
|desktop_environment_factory| must outlive |this|.
nit: , -> and

https://codereview.chromium.org/301453003/diff/80001/remoting/host/client_ses...
File remoting/host/client_session_unittest.cc (right):

https://codereview.chromium.org/301453003/diff/80001/remoting/host/client_ses...
remoting/host/client_session_unittest.cc:358: void
ClientSessionTest::SendExtensionMessageAndDisconnect(
Sorry, this was not a good name for me to suggest... this call actually sets an
expectation that does the work - call it
SetSendMessageAndDisconnectExpectation() (phew)?

https://codereview.chromium.org/301453003/diff/80001/remoting/host/host_exten...
File remoting/host/host_extension.h (right):

https://codereview.chromium.org/301453003/diff/80001/remoting/host/host_exten...
remoting/host/host_extension.h:24: // Capabilities are used to inform the client
of the availability of an
nit: "... may be used ..." - we don't require this; it's just a tool for
extensions that need to be checked for?

https://codereview.chromium.org/301453003/diff/80001/remoting/host/host_exten...
remoting/host/host_extension.h:32: // session.
nit: I'd suggest "NULL may be returned if |client_session| cannot support this
extension."

[dcaiafa, 2014-05-29 00:03:15]

Thanks!

https://codereview.chromium.org/301453003/diff/80001/remoting/host/chromoting...
File remoting/host/chromoting_host.cc (right):

https://codereview.chromium.org/301453003/diff/80001/remoting/host/chromoting...
remoting/host/chromoting_host.cc:219:
client->AddHostCapabilities((*extension)->GetCapabilities());
On 2014/05/28 23:03:41, Wez wrote:
> Do you really need to do this here? Could it wait until OnClientCapabilities,
so
> that you can derive the capabilities list from the list of extensions that you
> have actually added to the ClientSession?

We need to send the host capabilities to the client before it can reply with its
capabilities - so we need to compile the host capabilities before
OnClientCapabilities is called.

https://codereview.chromium.org/301453003/diff/80001/remoting/host/chromoting...
remoting/host/chromoting_host.cc:219:
client->AddHostCapabilities((*extension)->GetCapabilities());
On 2014/05/28 23:03:41, Wez wrote:
> If not then it would seem cleaner to have SetHostCapabilities(), and have the
> host collate the list and pass it once, rather than having the ClientSession
> accumulate the list.

As you mentioned in the comment's message, AddHostCapabilities is also used to
compile capabilities from DesktopEnvironment.

https://codereview.chromium.org/301453003/diff/80001/remoting/host/client_ses...
File remoting/host/client_session.cc (right):

https://codereview.chromium.org/301453003/diff/80001/remoting/host/client_ses...
remoting/host/client_session.cc:116: host_capabilities_.append(" ");
On 2014/05/28 23:03:41, Wez wrote:
> See comment in ChromotingHost re doing this accumulation of capabilities in
the
> ChromotingHost code and just SetHostCapabilities() here.

As you mentioned in the comment's message, AddHostCapabilities is also used to
compile capabilities from DesktopEnvironment.

https://codereview.chromium.org/301453003/diff/80001/remoting/host/client_ses...
File remoting/host/client_session.h (right):

https://codereview.chromium.org/301453003/diff/80001/remoting/host/client_ses...
remoting/host/client_session.h:71: virtual void
OnClientCapabilities(ClientSession* client) = 0;
On 2014/05/28 23:03:41, Wez wrote:
> All the other handlers are OnSession<Foo>; OnSessionCapabilities or
> OnSessionClientCapabilities, for consistency?

Done.

https://codereview.chromium.org/301453003/diff/80001/remoting/host/client_ses...
remoting/host/client_session.h:97: // |event_handler|,
|desktop_environment_factory| must outlive |this|.
On 2014/05/28 23:03:41, Wez wrote:
> nit: , -> and

Done.

https://codereview.chromium.org/301453003/diff/80001/remoting/host/client_ses...
File remoting/host/client_session_unittest.cc (right):

https://codereview.chromium.org/301453003/diff/80001/remoting/host/client_ses...
remoting/host/client_session_unittest.cc:358: void
ClientSessionTest::SendExtensionMessageAndDisconnect(
On 2014/05/28 23:03:41, Wez wrote:
> Sorry, this was not a good name for me to suggest... this call actually sets
an
> expectation that does the work - call it
> SetSendMessageAndDisconnectExpectation() (phew)?

Done.

https://codereview.chromium.org/301453003/diff/80001/remoting/host/host_exten...
File remoting/host/host_extension.h (right):

https://codereview.chromium.org/301453003/diff/80001/remoting/host/host_exten...
remoting/host/host_extension.h:24: // Capabilities are used to inform the client
of the availability of an
On 2014/05/28 23:03:41, Wez wrote:
> nit: "... may be used ..." - we don't require this; it's just a tool for
> extensions that need to be checked for?

Done.

https://codereview.chromium.org/301453003/diff/80001/remoting/host/host_exten...
remoting/host/host_extension.h:32: // session.
On 2014/05/28 23:03:41, Wez wrote:
> nit: I'd suggest "NULL may be returned if |client_session| cannot support this
> extension."

Done.

[dcaiafa, 2014-05-29 16:37:56]

The CQ bit was checked by dcaiafa@chromium.org

[commit-bot: I haz the power, 2014-05-29 16:40:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dcaiafa@chromium.org/301453003/140001

[commit-bot: I haz the power, 2014-05-29 20:15:47]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-29 23:25:09]

Change committed as 273652

[dcaiafa, 2014-05-30 14:19:50]

The CQ bit was checked by dcaiafa@chromium.org

[commit-bot: I haz the power, 2014-05-30 14:20:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dcaiafa@chromium.org/301453003/160001

[commit-bot: I haz the power, 2014-05-30 16:47:34]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-30 19:12:49]

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

[commit-bot: I haz the power, 2014-05-30 19:12:50]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[dcaiafa, 2014-05-30 20:06:12]

The CQ bit was checked by dcaiafa@chromium.org

[commit-bot: I haz the power, 2014-05-30 20:08:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dcaiafa@chromium.org/301453003/160001

[commit-bot: I haz the power, 2014-05-30 20:13:19]

Change committed as 273901