Set the initial resolution of an RDP session to the client screen resolution if it is available.

Set the initial resolution of an RDP session to the client screen resolution if it is available.

Changes in this CL:
 - The version of the control channel is increased to 3. This allows the host and client to tell if the peer supports capabilities negotiation or not.
 - The client and host negotiate supported capabilities by sending each other a list of the supported capabilities. Capabilities supported by both client and host are assumed to be enabled.
 - The client plugin and webapp negotiate the list of capabilities supported by the client. The webapp has the final word.
 - The DesktopEnvironment interface was extended to provide the list of all supported capabilities and receive the results of negotiation with the client.
 - Added the 'sendInitialResolution' capability. When it is enabled the client sends its screen resolution to the host once the connection has been established.
 - DesktopSessionProxy now waits for the client screen resolution when the 'sendInitialResolution' capability is enabled.

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

[alexeypa (please no reviews), 2013-04-12 17:58:38]

PTAL.

Jamie: *.js
Sergey: the rest.

[Wez, 2013-04-15 19:33:14]

On 2013/04/12 17:58:38, alexeypa wrote:
> PTAL.
> 
> Jamie: *.js
> Sergey: the rest.

Can we tick & "disable" the resize-to-client menu item, or hide it, so it's
clear that it can't be changed for the host?

[alexeypa (please no reviews), 2013-04-15 19:56:29]

> Can we tick & "disable" the resize-to-client menu item, or hide it, so it's
> clear that it can't be changed for the host?

I thought I could split it into two separate capabilities:
  - 'sendInitialClientResolution' that will request the client to send its
resolution once the connection is established.
  - 'forceResizeToClient' to send resize-to-client notifications
unconditionally.

This CL will implement 'sendInitialClientResolution' only with no changes to
resize-to-client logic. A later CL will add support of 'forceResizeToClient' if
needed. WDYT?

[Jamie, 2013-04-15 22:14:11]

As discussed, I don't think having an 'always resize to client' capability is
the way to do this. A 'send initial size' is a better fit as it provides the
same benefit (hosts that have no concept of a native resolution will have a
sensible size on connection) without override the user's preference regarding
the behaviour when they resize the client window.

https://chromiumcodereview.appspot.com/13932020/diff/4001/remoting/webapp/cli...
File remoting/webapp/client_plugin_async.js (right):

https://chromiumcodereview.appspot.com/13932020/diff/4001/remoting/webapp/cli...
remoting/webapp/client_plugin_async.js:150: // Let the host know that the webapp
can be requested to send always send
s/to send/to/

https://chromiumcodereview.appspot.com/13932020/diff/4001/remoting/webapp/cli...
remoting/webapp/client_plugin_async.js:152:
this.capabilities_.push('alwaysResizeToClient');
Use ALWAYS_RESIZE_TO_CLIENT variable to avoid duplication?

https://chromiumcodereview.appspot.com/13932020/diff/4001/remoting/webapp/cli...
File remoting/webapp/client_session.js (right):

https://chromiumcodereview.appspot.com/13932020/diff/4001/remoting/webapp/cli...
remoting/webapp/client_session.js:241: * @type {!Array.<string>}
You've declared this type as being never null, then defaulted it to null. You
also have null checks elsewhere. I think defaulting to [] would be better, in
which case you can probably lose the null checks.

[alexeypa (please no reviews), 2013-04-15 23:18:53]

> As discussed, I don't think having an 'always resize to client'
> capability is the way to do this. A 'send initial size' is
> a better fit as it provides the same benefit (hosts that have no
> concept of a native resolution will have a sensible size on
> connection) without override the user's preference regarding
> the behaviour when they resize the client window.

The capability is now called 'sendInitialResoultion'. It will cause the client
to sent its  resolution once. Resize-to-client logic is not affected.

https://chromiumcodereview.appspot.com/13932020/diff/4001/remoting/webapp/cli...
File remoting/webapp/client_plugin_async.js (right):

https://chromiumcodereview.appspot.com/13932020/diff/4001/remoting/webapp/cli...
remoting/webapp/client_plugin_async.js:150: // Let the host know that the webapp
can be requested to send always send
On 2013/04/15 22:14:11, Jamie wrote:
> s/to send/to/

Done.

https://chromiumcodereview.appspot.com/13932020/diff/4001/remoting/webapp/cli...
remoting/webapp/client_plugin_async.js:152:
this.capabilities_.push('alwaysResizeToClient');
On 2013/04/15 22:14:11, Jamie wrote:
> Use ALWAYS_RESIZE_TO_CLIENT variable to avoid duplication?

Done.

https://chromiumcodereview.appspot.com/13932020/diff/4001/remoting/webapp/cli...
File remoting/webapp/client_session.js (right):

https://chromiumcodereview.appspot.com/13932020/diff/4001/remoting/webapp/cli...
remoting/webapp/client_session.js:241: * @type {!Array.<string>}
On 2013/04/15 22:14:11, Jamie wrote:
> You've declared this type as being never null, then defaulted it to null. You
> also have null checks elsewhere. I think defaulting to [] would be better, in
> which case you can probably lose the null checks.

Done.

[alexeypa (please no reviews), 2013-04-15 23:19:53]

... jscompiler passes as well.

[Jamie, 2013-04-15 23:40:23]

I don't see any code to implement the sendInitialSize feature. Am I missing
something? Also, please update the CL description to match the latest Cl.

https://chromiumcodereview.appspot.com/13932020/diff/13001/remoting/webapp/cl...
File remoting/webapp/client_plugin.js (right):

https://chromiumcodereview.appspot.com/13932020/diff/13001/remoting/webapp/cl...
remoting/webapp/client_plugin.js:34: /** @type {function(!Array.<string>): void}
Capabilities negotiated callback. */
Use of the ! operator is not common. Unless it won't compile without it, I
suggest removing it, here and elsewhere.

https://chromiumcodereview.appspot.com/13932020/diff/13001/remoting/webapp/cl...
File remoting/webapp/client_session.js (right):

https://chromiumcodereview.appspot.com/13932020/diff/13001/remoting/webapp/cl...
remoting/webapp/client_session.js:236: SEND_INITIAL_RESOLUTION:
'sendInitialResolution'
These capabilities should be commented somewhere. Here seems as good a place as
any.

https://chromiumcodereview.appspot.com/13932020/diff/13001/remoting/webapp/cl...
remoting/webapp/client_session.js:797: * negotiated with the host.
This could be more simply phrased: "Called when the client-host capabilities
negotiation is complete."

https://chromiumcodereview.appspot.com/13932020/diff/13001/remoting/webapp/cl...
remoting/webapp/client_session.js:806: console.error('ERROR:
onSetCapabilities_() is called more than once');
Nit: No need for ERROR--the console shows errors in red to highlight them.

[alexeypa (please no reviews), 2013-04-15 23:56:31]

> I don't see any code to implement the sendInitialSize feature. Am I missing
something?

See client_session.js, remoting.ClientSession.prototype.onSetCapabilities_.

> Also, please update the CL description to match the latest Cl.

Done.

https://chromiumcodereview.appspot.com/13932020/diff/13001/remoting/webapp/cl...
File remoting/webapp/client_plugin.js (right):

https://chromiumcodereview.appspot.com/13932020/diff/13001/remoting/webapp/cl...
remoting/webapp/client_plugin.js:34: /** @type {function(!Array.<string>): void}
Capabilities negotiated callback. */
On 2013/04/15 23:40:23, Jamie wrote:
> Use of the ! operator is not common. Unless it won't compile without it, I
> suggest removing it, here and elsewhere.

I just verified that jscompiler checks for "!". I guess it is a good thing to
use ! because it puts more restrictions on the type.

https://chromiumcodereview.appspot.com/13932020/diff/13001/remoting/webapp/cl...
File remoting/webapp/client_session.js (right):

https://chromiumcodereview.appspot.com/13932020/diff/13001/remoting/webapp/cl...
remoting/webapp/client_session.js:236: SEND_INITIAL_RESOLUTION:
'sendInitialResolution'
On 2013/04/15 23:40:23, Jamie wrote:
> These capabilities should be commented somewhere. Here seems as good a place
as
> any.

Done.

https://chromiumcodereview.appspot.com/13932020/diff/13001/remoting/webapp/cl...
remoting/webapp/client_session.js:797: * negotiated with the host.
On 2013/04/15 23:40:23, Jamie wrote:
> This could be more simply phrased: "Called when the client-host capabilities
> negotiation is complete."

Done.

https://chromiumcodereview.appspot.com/13932020/diff/13001/remoting/webapp/cl...
remoting/webapp/client_session.js:806: console.error('ERROR:
onSetCapabilities_() is called more than once');
On 2013/04/15 23:40:23, Jamie wrote:
> Nit: No need for ERROR--the console shows errors in red to highlight them.

Done.

[Jamie, 2013-04-16 00:15:25]

JS LGTM

[wez1, 2013-04-16 05:19:41]

On 15 Apr 2013 15:14, <jamiewalch@chromium.org> wrote:
>
> As discussed, I don't think having an 'always resize to client'
capability is
> the way to do this. A 'send initial size' is a better fit as it provides
the
> same benefit (hosts that have no concept of a native resolution will have
a
> sensible size on connection) without override the user's preference
regarding
> the behaviour when they resize the client window.

When is that distinction ever going to be useful, though? And useful enough
to warrant a more confusing UX?

>
>
>
https://chromiumcodereview.appspot.com/13932020/diff/4001/remoting/webapp/cli...
> File remoting/webapp/client_plugin_async.js (right):
>
>
https://chromiumcodereview.appspot.com/13932020/diff/4001/remoting/webapp/cli...
> remoting/webapp/client_plugin_async.js:150: // Let the host know that
> the webapp can be requested to send always send
> s/to send/to/
>
>
https://chromiumcodereview.appspot.com/13932020/diff/4001/remoting/webapp/cli...
> remoting/webapp/client_plugin_async.js:152:
> this.capabilities_.push('alwaysResizeToClient');
> Use ALWAYS_RESIZE_TO_CLIENT variable to avoid duplication?
>
>
https://chromiumcodereview.appspot.com/13932020/diff/4001/remoting/webapp/cli...
> File remoting/webapp/client_session.js (right):
>
>
https://chromiumcodereview.appspot.com/13932020/diff/4001/remoting/webapp/cli...
> remoting/webapp/client_session.js:241: * @type {!Array.<string>}
> You've declared this type as being never null, then defaulted it to
> null. You also have null checks elsewhere. I think defaulting to []
> would be better, in which case you can probably lose the null checks.
>
> https://chromiumcodereview.appspot.com/13932020/

[Sergey Ulanov, 2013-04-16 08:38:53]

https://codereview.chromium.org/13932020/diff/14040/remoting/base/capabilitie...
File remoting/base/capabilities.cc (right):

https://codereview.chromium.org/13932020/diff/14040/remoting/base/capabilitie...
remoting/base/capabilities.cc:18: bool isValidCapability(const std::string&
capability) {
CamelCase

https://codereview.chromium.org/13932020/diff/14040/remoting/base/capabilitie...
remoting/base/capabilities.cc:66: bool Capabilities::FromProtocolMessage(const
protocol::Capabilities& message) {
nit: Not sure if we really need this given that list of capabilities can be
serialized into a string. Protocol::Capabilities message can contain just one
string that would hold all capabilities. Also base shouldn't depend on proto
code. I think checkdeps should detect it.

https://codereview.chromium.org/13932020/diff/14040/remoting/base/capabilitie...
remoting/base/capabilities.cc:88: scoped_ptr<base::ListValue>
Capabilities::ToListValue() const {
I'm not sure if we really need this - capabilities list can be represented as a
single string in JSON.

https://codereview.chromium.org/13932020/diff/14040/remoting/base/capabilitie...
remoting/base/capabilities.cc:92: while (i != capabilities_.end()) {
can this be for loop?

https://codereview.chromium.org/13932020/diff/14040/remoting/client/chromotin...
File remoting/client/chromoting_client.cc (right):

https://codereview.chromium.org/13932020/diff/14040/remoting/client/chromotin...
remoting/client/chromoting_client.cc:157:
*config_.capabilities.ToProtocolMessage());
Should we send intersection of capabilities supported by webapp and the client?

https://codereview.chromium.org/13932020/diff/14040/remoting/client/chromotin...
File remoting/client/chromoting_client.h (right):

https://codereview.chromium.org/13932020/diff/14040/remoting/client/chromotin...
remoting/client/chromoting_client.h:67: OVERRIDE;
nit: I think the right way to format this code is as follows:
  virtual void SetCapabilities(
      const protocol::Capabilities& capabilities) OVERRIDE;
That was bikeshedded in chromium-dev long time ago.

https://codereview.chromium.org/13932020/diff/14040/remoting/client/plugin/ch...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/13932020/diff/14040/remoting/client/plugin/ch...
remoting/client/plugin/chromoting_instance.cc:193:
data->SetString("requestedCapabilities", kRequestedCapabilities);
It's not clear to me what's the difference between these two attributes, and
client code doesn't make it clearer. Given that requestedCapabilities is ignored
by the webapp, do we need it at all?

https://codereview.chromium.org/13932020/diff/14040/remoting/client/plugin/ch...
remoting/client/plugin/chromoting_instance.cc:305: if
(data->GetList("capabilities", &capabilities)) {
Here capablities are represented as list, while requestedCapabilities and
supportedCapabilites are passed as strings.

https://codereview.chromium.org/13932020/diff/14040/remoting/client/plugin/ch...
File remoting/client/plugin/chromoting_instance.h (right):

https://codereview.chromium.org/13932020/diff/14040/remoting/client/plugin/ch...
remoting/client/plugin/chromoting_instance.h:78: static const int kApiVersion =
8;
No need to change this version. see my comments in JS.

https://codereview.chromium.org/13932020/diff/14040/remoting/host/client_sess...
File remoting/host/client_session.cc (right):

https://codereview.chromium.org/13932020/diff/14040/remoting/host/client_sess...
remoting/host/client_session.cc:156: VLOG(1) << "Client capabilities: " <<
client_capabilities_->ToString();
nit: this will convert the capabilities to string that will almost never will be
useful. maybe better remove that VLOG

https://codereview.chromium.org/13932020/diff/14040/remoting/host/client_sess...
remoting/host/client_session.cc:161: if (desktop_environment_) {
It sucks that we need to worry about the case when
OnConnectionChannelsConnected() is called after this method. Is there any way we
can avoid it?  is it possible to move desktop environment creation to
OnConnectionAuthenticated()?

https://codereview.chromium.org/13932020/diff/14040/remoting/host/desktop_ses...
File remoting/host/desktop_session_proxy.cc (right):

https://codereview.chromium.org/13932020/diff/14040/remoting/host/desktop_ses...
remoting/host/desktop_session_proxy.cc:99: is_desktop_session_created_ = true;
is_desktop_session_connected_?

https://codereview.chromium.org/13932020/diff/14040/remoting/protocol/session...
File remoting/protocol/session_config.h (right):

https://codereview.chromium.org/13932020/diff/14040/remoting/protocol/session...
remoting/protocol/session_config.h:18: extern const int
kCapabilitiesControlStreamVersion;
This value would not need to be in the header if supports_capabilities() wasn't
declined inline in the header. kDefaultStreamVersion can also be removed from
here.

https://codereview.chromium.org/13932020/diff/14040/remoting/webapp/client_pl...
File remoting/webapp/client_plugin_async.js (right):

https://codereview.chromium.org/13932020/diff/14040/remoting/webapp/client_pl...
remoting/webapp/client_plugin_async.js:112: var tokens = str.match(/\S+/g);
would it be better to use .split(" ") here? at least it would be more readable?

https://codereview.chromium.org/13932020/diff/14040/remoting/webapp/client_pl...
remoting/webapp/client_plugin_async.js:128: if (this.pluginApiVersion_ >= 8) {
For new versions we normally use apiFeatures property, so there is no need to
increase API version here (We should actually remove API version - it doesn't
make sense anymore). But in this case you don't even need new apiFeature - you
can check the presence of the attributes to detect support for capabilities.

[alexeypa (please no reviews), 2013-04-16 22:06:11]

On 2013/04/16 05:19:41, wez1 wrote:
> On 15 Apr 2013 15:14, <mailto:jamiewalch@chromium.org> wrote:
> > As discussed, I don't think having an 'always resize to client'
> > capability is the way to do this. A 'send initial size' is a
> > better fit as it provides the same benefit (hosts that have no
> > concept of a native resolution will have a sensible size on
> > connection) without override the user's preference regarding
> > the behaviour when they resize the client window.
> 
> When is that distinction ever going to be useful, though? And useful enough
> to warrant a more confusing UX?

The UI is not going to change. Not with this CL at least.

Most of the user would probably want 'resize-to-client' to be ON. Those who
disable it however know better why it should be disabled. For instance they
don't want the icons on the desktop to move. We should not override the user's
choice.

By adding 'sendInitialResolution' we don't change 'resize-to-client' but allow
the host to guess a better initial resolution than a hardcoded constant.

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/base/capa...
File remoting/base/capabilities.cc (right):

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/base/capa...
remoting/base/capabilities.cc:18: bool isValidCapability(const std::string&
capability) {
On 2013/04/16 08:38:53, sergeyu wrote:
> CamelCase

Removed.

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/base/capa...
remoting/base/capabilities.cc:66: bool Capabilities::FromProtocolMessage(const
protocol::Capabilities& message) {
On 2013/04/16 08:38:53, sergeyu wrote:
> nit: Not sure if we really need this given that list of capabilities can be
> serialized into a string. Protocol::Capabilities message can contain just one
> string that would hold all capabilities. Also base shouldn't depend on proto
> code. I think checkdeps should detect it.  

Removed.

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/base/capa...
remoting/base/capabilities.cc:88: scoped_ptr<base::ListValue>
Capabilities::ToListValue() const {
On 2013/04/16 08:38:53, sergeyu wrote:
> I'm not sure if we really need this - capabilities list can be represented as
a
> single string in JSON.

Removed.

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/base/capa...
remoting/base/capabilities.cc:92: while (i != capabilities_.end()) {
On 2013/04/16 08:38:53, sergeyu wrote:
> can this be for loop?

A for loop would be less readable because line wrapping. Anyway this code was
removed.

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/client/ch...
File remoting/client/chromoting_client.cc (right):

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/client/ch...
remoting/client/chromoting_client.cc:157:
*config_.capabilities.ToProtocolMessage());
On 2013/04/16 08:38:53, sergeyu wrote:
> Should we send intersection of capabilities supported by webapp and the
client?

We do. |config_.capabilities| is a combination of capabilities supported by the
webapp and plugin. The plugin's capabilities are sent to the webapp in the
"hello" message. The webapp combines them with capabilities that it supports and
return the result to the plugin in the "connect" message.

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/client/ch...
File remoting/client/chromoting_client.h (right):

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/client/ch...
remoting/client/chromoting_client.h:67: OVERRIDE;
On 2013/04/16 08:38:53, sergeyu wrote:
> nit: I think the right way to format this code is as follows:
>   virtual void SetCapabilities(
>       const protocol::Capabilities& capabilities) OVERRIDE;
> That was bikeshedded in chromium-dev long time ago.

Done.

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/client/pl...
File remoting/client/plugin/chromoting_instance.cc (right):

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/client/pl...
remoting/client/plugin/chromoting_instance.cc:193:
data->SetString("requestedCapabilities", kRequestedCapabilities);
On 2013/04/16 08:38:53, sergeyu wrote:
> It's not clear to me what's the difference between these two attributes,

"requested" capabilities are the ones that need to be supported by both plugin
and webapp to work. I.e. the intersection of "requested" capabilities supported
by the plugin and webapp will be sent to the host.

"supported" capabilities are the ones that do not require any special handling
in the webapp. The webapp will pass them through in most of the cases. The
reason they are passed to the webapp at all is that the latter should be able to
"veto" them in case if we need to fix anything after the client plugin has been
released. 

> and client code doesn't make it clearer.

Declarations of both constants are documented. See chromoting_instance.h. 

> Given that requestedCapabilities is ignored
> by the webapp, do we need it at all?

We still need the webapp to be ready to handle them. We could remove them from
the plugin but it is just around five lines of code and it serves as
documentation/guidelines how the "requested" capabilities should be handled.

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/client/pl...
remoting/client/plugin/chromoting_instance.cc:305: if
(data->GetList("capabilities", &capabilities)) {
On 2013/04/16 08:38:53, sergeyu wrote:
> Here capablities are represented as list, while requestedCapabilities and
> supportedCapabilites are passed as strings.

Done.

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/client/pl...
File remoting/client/plugin/chromoting_instance.h (right):

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/client/pl...
remoting/client/plugin/chromoting_instance.h:78: static const int kApiVersion =
8;
On 2013/04/16 08:38:53, sergeyu wrote:
> No need to change this version. see my comments in JS.

Done.

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/host/clie...
File remoting/host/client_session.cc (right):

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/host/clie...
remoting/host/client_session.cc:156: VLOG(1) << "Client capabilities: " <<
client_capabilities_->ToString();
On 2013/04/16 08:38:53, sergeyu wrote:
> nit: this will convert the capabilities to string that will almost never will
be
> useful. maybe better remove that VLOG 

It is actually useful. I used it when I tested various combinations of the
client and host. It makes it possible to figure out  what exactly was negotiated
and why.

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/host/clie...
remoting/host/client_session.cc:161: if (desktop_environment_) {
On 2013/04/16 08:38:53, sergeyu wrote:
> It sucks that we need to worry about the case when
> OnConnectionChannelsConnected() is called after this method. Is there any way
we
> can avoid it?  is it possible to move desktop environment creation to
> OnConnectionAuthenticated()?

Done.

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/host/desk...
File remoting/host/desktop_session_proxy.cc (right):

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/host/desk...
remoting/host/desktop_session_proxy.cc:99: is_desktop_session_created_ = true;
On 2013/04/16 08:38:53, sergeyu wrote:
> is_desktop_session_connected_?

Done.

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/protocol/...
File remoting/protocol/session_config.h (right):

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/protocol/...
remoting/protocol/session_config.h:18: extern const int
kCapabilitiesControlStreamVersion;
On 2013/04/16 08:38:53, sergeyu wrote:
> This value would not need to be in the header if supports_capabilities()
wasn't
> declined inline in the header. kDefaultStreamVersion can also be removed from
> here.

Done.

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/webapp/cl...
File remoting/webapp/client_plugin_async.js (right):

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/webapp/cl...
remoting/webapp/client_plugin_async.js:112: var tokens = str.match(/\S+/g);
On 2013/04/16 08:38:53, sergeyu wrote:
> would it be better to use .split(" ") here? at least it would be more
readable?

Nope. "".split(" ") returns [""] while we want []. It does not handle well
multiple consecutive spaces too.

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/webapp/cl...
remoting/webapp/client_plugin_async.js:128: if (this.pluginApiVersion_ >= 8) {
On 2013/04/16 08:38:53, sergeyu wrote:
> For new versions we normally use apiFeatures property, so there is no need to
> increase API version here (We should actually remove API version - it doesn't
> make sense anymore). But in this case you don't even need new apiFeature - you
> can check the presence of the attributes to detect support for capabilities.

Done.

[alexeypa (please no reviews), 2013-04-17 23:23:28]

Sergey, ping.

[Sergey Ulanov, 2013-04-18 00:34:53]

lgtm, but please see my comments

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/host/clie...
File remoting/host/client_session.cc (right):

https://chromiumcodereview.appspot.com/13932020/diff/14040/remoting/host/clie...
remoting/host/client_session.cc:161: if (desktop_environment_) {
On 2013/04/16 22:06:11, alexeypa wrote:
> On 2013/04/16 08:38:53, sergeyu wrote:
> > It sucks that we need to worry about the case when
> > OnConnectionChannelsConnected() is called after this method. Is there any
way
> we
> > can avoid it?  is it possible to move desktop environment creation to
> > OnConnectionAuthenticated()?
> 
> Done.

I don't see it in the last patchset.

https://chromiumcodereview.appspot.com/13932020/diff/36001/remoting/base/capa...
File remoting/base/capabilities.cc (right):

https://chromiumcodereview.appspot.com/13932020/diff/36001/remoting/base/capa...
remoting/base/capabilities.cc:39: result.erase(end);
shouldn't this be result.erase(end, result.end)?
erase(end) will remove a single element.

https://chromiumcodereview.appspot.com/13932020/diff/36001/remoting/base/capa...
File remoting/base/capabilities_unittest.cc (right):

https://chromiumcodereview.appspot.com/13932020/diff/36001/remoting/base/capa...
remoting/base/capabilities_unittest.cc:45: EXPECT_EQ(IntersectCapabilities("a
b", "b"), "b");
Please add more complex tests, e.g. when the elements are not sorted and when
there is more than 2 of them.

https://chromiumcodereview.appspot.com/13932020/diff/36001/remoting/client/ch...
File remoting/client/chromoting_client.cc (right):

https://chromiumcodereview.appspot.com/13932020/diff/36001/remoting/client/ch...
remoting/client/chromoting_client.cc:103: host_capabilities_ =
make_scoped_ptr(new std::string());
host_capabilities_ is never used except to check that it's set, so maybe replace
it with a single boolean value |capabilities_received_|?

https://chromiumcodereview.appspot.com/13932020/diff/36001/remoting/client/ch...
File remoting/client/chromoting_client.h (right):

https://chromiumcodereview.appspot.com/13932020/diff/36001/remoting/client/ch...
remoting/client/chromoting_client.h:101: scoped_ptr<std::string>
host_capabilities_;
why not just std::string instead of scoped_ptr?

https://chromiumcodereview.appspot.com/13932020/diff/36001/remoting/client/pl...
File remoting/client/plugin/chromoting_instance.cc (right):

https://chromiumcodereview.appspot.com/13932020/diff/36001/remoting/client/pl...
remoting/client/plugin/chromoting_instance.cc:304: if
(data->HasKey("capabilities")) {
Don't need this. GetString() will return false if the value is not present. You
can write this chunk of code as follows:
  if (!data->GetString("capabilities", &config.capabilities))
    config.capabilities.clear();

https://chromiumcodereview.appspot.com/13932020/diff/36001/remoting/protocol/...
File remoting/protocol/session_config.cc (right):

https://chromiumcodereview.appspot.com/13932020/diff/36001/remoting/protocol/...
remoting/protocol/session_config.cc:15: const int
kCapabilitiesControlStreamVersion = 3;
I think it's better to have kControlStreamVersion=3 and
kControlStreamVersionNoCapabilities=2.

https://chromiumcodereview.appspot.com/13932020/diff/36001/remoting/webapp/cl...
File remoting/webapp/client_plugin_async.js (right):

https://chromiumcodereview.appspot.com/13932020/diff/36001/remoting/webapp/cl...
remoting/webapp/client_plugin_async.js:138: var requestedCapabilities = []
add semicolon:
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...

https://chromiumcodereview.appspot.com/13932020/diff/36001/remoting/webapp/cl...
remoting/webapp/client_plugin_async.js:148: var supportedCapabilities = []
semicolon

[alexeypa (please no reviews), 2013-04-18 18:56:36]

https://codereview.chromium.org/13932020/diff/14040/remoting/host/client_sess...
File remoting/host/client_session.cc (right):

https://codereview.chromium.org/13932020/diff/14040/remoting/host/client_sess...
remoting/host/client_session.cc:161: if (desktop_environment_) {
On 2013/04/18 00:34:53, sergeyu wrote:
> I don't see it in the last patchset.

It turned out that |event_handler_->OnSessionAuthenticated(this);| must be the
last line in ClientSession::OnConnectionAuthenticated() because it can delete
the client session. At the same time the desktop environment should be created
after |event_handler_->OnSessionAuthenticated(this)| has been called. So I moved
created on the desktop environment back to
ClientSession::OnConnectionChannelsConnected().

This issue affects not only SetCapabilities but other control messages. I'm
going to address it properly in a separate CL.

https://codereview.chromium.org/13932020/diff/36001/remoting/base/capabilitie...
File remoting/base/capabilities.cc (right):

https://codereview.chromium.org/13932020/diff/36001/remoting/base/capabilitie...
remoting/base/capabilities.cc:39: result.erase(end);
On 2013/04/18 00:34:53, sergeyu wrote:
> shouldn't this be result.erase(end, result.end)?
> erase(end) will remove a single element.

Yes, it should.

https://codereview.chromium.org/13932020/diff/36001/remoting/base/capabilitie...
File remoting/base/capabilities_unittest.cc (right):

https://codereview.chromium.org/13932020/diff/36001/remoting/base/capabilitie...
remoting/base/capabilities_unittest.cc:45: EXPECT_EQ(IntersectCapabilities("a
b", "b"), "b");
On 2013/04/18 00:34:53, sergeyu wrote:
> Please add more complex tests, e.g. when the elements are not sorted and when
> there is more than 2 of them.

Done.

https://codereview.chromium.org/13932020/diff/36001/remoting/client/chromotin...
File remoting/client/chromoting_client.cc (right):

https://codereview.chromium.org/13932020/diff/36001/remoting/client/chromotin...
remoting/client/chromoting_client.cc:103: host_capabilities_ =
make_scoped_ptr(new std::string());
On 2013/04/18 00:34:53, sergeyu wrote:
> host_capabilities_ is never used except to check that it's set, so maybe
replace
> it with a single boolean value |capabilities_received_|?

Done.

https://codereview.chromium.org/13932020/diff/36001/remoting/client/chromotin...
File remoting/client/chromoting_client.h (right):

https://codereview.chromium.org/13932020/diff/36001/remoting/client/chromotin...
remoting/client/chromoting_client.h:101: scoped_ptr<std::string>
host_capabilities_;
On 2013/04/18 00:34:53, sergeyu wrote:
> why not just std::string instead of scoped_ptr?

Done.

https://codereview.chromium.org/13932020/diff/36001/remoting/client/plugin/ch...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/13932020/diff/36001/remoting/client/plugin/ch...
remoting/client/plugin/chromoting_instance.cc:304: if
(data->HasKey("capabilities")) {
On 2013/04/18 00:34:53, sergeyu wrote:
> Don't need this. GetString() will return false if the value is not present.
You
> can write this chunk of code as follows:
>   if (!data->GetString("capabilities", &config.capabilities))
>     config.capabilities.clear();

This is needed to detect the case when "capabilities" is present but it is not a
string.

https://codereview.chromium.org/13932020/diff/36001/remoting/protocol/session...
File remoting/protocol/session_config.cc (right):

https://codereview.chromium.org/13932020/diff/36001/remoting/protocol/session...
remoting/protocol/session_config.cc:15: const int
kCapabilitiesControlStreamVersion = 3;
On 2013/04/18 00:34:53, sergeyu wrote:
> I think it's better to have kControlStreamVersion=3 and
> kControlStreamVersionNoCapabilities=2.

Done.

https://codereview.chromium.org/13932020/diff/36001/remoting/webapp/client_pl...
File remoting/webapp/client_plugin_async.js (right):

https://codereview.chromium.org/13932020/diff/36001/remoting/webapp/client_pl...
remoting/webapp/client_plugin_async.js:138: var requestedCapabilities = []
On 2013/04/18 00:34:53, sergeyu wrote:
> add semicolon:
>
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...

Done.

https://codereview.chromium.org/13932020/diff/36001/remoting/webapp/client_pl...
remoting/webapp/client_plugin_async.js:148: var supportedCapabilities = []
On 2013/04/18 00:34:53, sergeyu wrote:
> semicolon

Done.

[commit-bot: I haz the power, 2013-04-18 19:00:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/13932020/48001

[commit-bot: I haz the power, 2013-04-18 19:27:39]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-18 21:36:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/13932020/63001

[commit-bot: I haz the power, 2013-04-18 21:36:40]

Presubmit check for 13932020-63001 failed and returned exit status 1.

INFO:root:Found 38 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Traceback (most recent call last):
  File "/b/commit-queue/verification/presubmit_shim.py", line 33, in <module>
    sys.exit(presubmit_support.Main(argv))
  File "/b/depot_tools/presubmit_support.py", line 1305, in Main
    rietveld_obj)
  File "/b/depot_tools/presubmit_support.py", line 1129, in DoPresubmitChecks
    results += executer.ExecPresubmitScript(presubmit_script, filename)
  File "/b/depot_tools/presubmit_support.py", line 1046, in ExecPresubmitScript
    result = eval(function_name + '(*__args)', context)
  File "<string>", line 1, in <module>
  File "<string>", line 865, in CheckChangeOnCommit
  File "<string>", line 700, in _CommonChecks
  File "<string>", line 258, in _CheckNoUNIT_TESTInSourceFiles
  File "/b/depot_tools/presubmit_support.py", line 549, in ChangedContents
    for line in self.GenerateScmDiff().splitlines():
  File "/b/depot_tools/presubmit_support.py", line 619, in GenerateScmDiff
    [self.LocalPath()], self._local_root, False, None)
  File "/b/depot_tools/scm.py", line 913, in GenerateDiff
    full_move, revision))
  File "/b/depot_tools/scm.py", line 837, in _DiffItemInternal
    data = SVN.Capture(command, cwd)
  File "/b/depot_tools/scm.py", line 479, in Capture
    ['svn'] + args, stderr=subprocess2.PIPE, cwd=cwd, **kwargs)
  File "/b/depot_tools/subprocess2.py", line 449, in check_output
    return check_call_out(args, stdout=PIPE, **kwargs)[0]
  File "/b/depot_tools/subprocess2.py", line 412, in check_call_out
    returncode, args, kwargs.get('cwd'), out[0], out[1])
subprocess2.CalledProcessError: Command svn diff --internal-diff
remoting/base/capabilities.cc returned non-zero exit status 1 in
/b/commit-queue/workdir/chromium
svn: invalid option: --internal-diff
Type 'svn help' for usage.

[commit-bot: I haz the power, 2013-04-18 22:12:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/13932020/63001

[commit-bot: I haz the power, 2013-04-19 09:29:31]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-04-19 13:54:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/13932020/63001

[commit-bot: I haz the power, 2013-04-19 14:55:42]

Change committed as 195192