Propagate connection state from networking layer to UI.

Show connection state in the Chromoting client UI.

Now screen is desaturated when we get notification that
video channel is not working.

BUG=131411
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147830

[Jamie, 2012-07-12 01:46:42]

http://codereview.chromium.org/10692179/diff/1/remoting/webapp/client_session.js
File remoting/webapp/client_session.js (right):

http://codereview.chromium.org/10692179/diff/1/remoting/webapp/client_session...
remoting/webapp/client_session.js:485: * @param {number} error The plugin's
error state, if any.
Add @private if this is a private method, or remove the underscore if it isn't.

http://codereview.chromium.org/10692179/diff/1/remoting/webapp/client_session...
remoting/webapp/client_session.js:501: * @param {boolean} inactive True if the
connection is inactive.
Add @private or remove the underscore.

http://codereview.chromium.org/10692179/diff/1/remoting/webapp/main.css
File remoting/webapp/main.css (right):

http://codereview.chromium.org/10692179/diff/1/remoting/webapp/main.css#newco...
remoting/webapp/main.css:566: -webkit-filter: grayscale(70%) blur(1px);
Nit: indentation.

http://codereview.chromium.org/10692179/diff/1/remoting/webapp/main.css#newco...
remoting/webapp/main.css:567: -webkit-transition: -webkit-filter 0.5s;
I think the transition property needs to be set regardless of whether or not the
session-client-inactive class is applied, otherwise it will fade in but not out
(or vice versa, I forget which).

Also, we've used "0.15s ease" for the tool-bar; 0.5s seems a bit long. Maybe run
it by Ken to get his opinion?

[Sergey Ulanov, 2012-07-13 20:42:50]

http://codereview.chromium.org/10692179/diff/1/remoting/webapp/client_session.js
File remoting/webapp/client_session.js (right):

http://codereview.chromium.org/10692179/diff/1/remoting/webapp/client_session...
remoting/webapp/client_session.js:485: * @param {number} error The plugin's
error state, if any.
On 2012/07/12 01:46:42, Jamie wrote:
> Add @private if this is a private method, or remove the underscore if it
isn't.

Done.

http://codereview.chromium.org/10692179/diff/1/remoting/webapp/client_session...
remoting/webapp/client_session.js:501: * @param {boolean} inactive True if the
connection is inactive.
On 2012/07/12 01:46:42, Jamie wrote:
> Add @private or remove the underscore.

Done.

http://codereview.chromium.org/10692179/diff/1/remoting/webapp/main.css
File remoting/webapp/main.css (right):

http://codereview.chromium.org/10692179/diff/1/remoting/webapp/main.css#newco...
remoting/webapp/main.css:566: -webkit-filter: grayscale(70%) blur(1px);
On 2012/07/12 01:46:42, Jamie wrote:
> Nit: indentation.

Done.

http://codereview.chromium.org/10692179/diff/1/remoting/webapp/main.css#newco...
remoting/webapp/main.css:567: -webkit-transition: -webkit-filter 0.5s;
On 2012/07/12 01:46:42, Jamie wrote:
> I think the transition property needs to be set regardless of whether or not
the
> session-client-inactive class is applied, otherwise it will fade in but not
out
> (or vice versa, I forget which).

That was intentional. It will fade with transition, but returns back instantly
when the connection returns.

> 
> Also, we've used "0.15s ease" for the tool-bar; 0.5s seems a bit long. Maybe
run
> it by Ken to get his opinion?

I saw 0.218s without ease for button color transition. Not sure if it makes to
use the same transition mode in different places, mainly because they are
different transition. IMHO linear transitions work better than ease when for
colors (vs position/size). Anyway, changed it to 0.2s here.

[Wez, 2012-07-16 18:51:23]

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/connectio...
File remoting/protocol/connection_to_host.cc (right):

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/connectio...
remoting/protocol/connection_to_host.cc:215:
event_callback_->OnConnectionInactive(inactive);
You're basing the connection's "active" state purely on whether the video
channel is active; although it's the highest throughput channel, it's equally
disruptive for the event channel not to be "active", and if the web-app knew
that the event channel were inactive it could provide some transient indicator
of the fact in the UI. If you expose the list of "active" channels to the app in
each notification then you can provide more fine-grained indicators; otherwise,
include the states of all channels in the single session-active indication?

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/connectio...
File remoting/protocol/connection_to_host.h (right):

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/connectio...
remoting/protocol/connection_to_host.h:68: // Called to indicate inactive
connection.
What does it mean for a connection to be "inactive"? I'd assume it means that
the client has not sent any input events for a while, or the host has sent no
video updates, but in the context of this CL it means that one or more channels
are not "connected"?

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/connectio...
remoting/protocol/connection_to_host.h:69: virtual void
OnConnectionInactive(bool inactive) = 0;
Why not expose this as a new connection state?

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/libjingle...
File remoting/protocol/libjingle_transport_factory.cc (right):

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/libjingle...
remoting/protocol/libjingle_transport_factory.cc:246:
event_handler_->OnTransportInactive(this, !channel->writable());
You're picking up whether a channel is "active" from whether or not it's
writable; that will work for UDP channels but if we ever re-enable TCP channels
then the channel will become un-writable when the TCP buffer is full, surely?

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/session.h
File remoting/protocol/session.h (right):

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/session.h...
remoting/protocol/session.h:70: // Called when inactive state on one of the
channels changes.
nit: Clarify what it means for a channel to be "inactive".

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/transport.h
File remoting/protocol/transport.h (right):

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/transport...
remoting/protocol/transport.h:89: // connections.
nit: Re-express this as the transport being "ready", rather than
"active/inactive"?

http://codereview.chromium.org/10692179/diff/7001/remoting/webapp/client_plug...
File remoting/webapp/client_plugin_async.js (right):

http://codereview.chromium.org/10692179/diff/7001/remoting/webapp/client_plug...
remoting/webapp/client_plugin_async.js:41: this.onDesktopSizeUpdateHandler =
function () {};
nit: Add a comment for the desktop size update handler, for consistency.

[Wez, 2012-07-16 18:52:39]

http://codereview.chromium.org/10692179/diff/7001/remoting/webapp/main.css
File remoting/webapp/main.css (right):

http://codereview.chromium.org/10692179/diff/7001/remoting/webapp/main.css#ne...
remoting/webapp/main.css:566: -webkit-filter: grayscale(70%) blur(1px);
nit: Do we really want to blue the desktop at this point? That seems a little
more disruptive than is necessary.

[Wez, 2012-07-16 18:54:47]

Please update the CL title e.g. to "Desaturate the desktop when the video
channel becomes unavailable".

We should consider changing our Disconnect / gone-offline behaviour to switch to
a desaturated image of the desktop rather than the Reconnect/OK dialog, as well.

[Sergey Ulanov, 2012-07-17 01:17:17]

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/connectio...
File remoting/protocol/connection_to_host.cc (right):

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/connectio...
remoting/protocol/connection_to_host.cc:215:
event_callback_->OnConnectionInactive(inactive);
On 2012/07/16 18:51:23, Wez wrote:
> You're basing the connection's "active" state purely on whether the video
> channel is active; although it's the highest throughput channel, it's equally
> disruptive for the event channel not to be "active", and if the web-app knew
> that the event channel were inactive it could provide some transient indicator
> of the fact in the UI. If you expose the list of "active" channels to the app
in
> each notification then you can provide more fine-grained indicators;
otherwise,
> include the states of all channels in the single session-active indication?

Changed it to set ready=false when any of the channels is not ready.

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/connectio...
File remoting/protocol/connection_to_host.h (right):

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/connectio...
remoting/protocol/connection_to_host.h:68: // Called to indicate inactive
connection.
On 2012/07/16 18:51:23, Wez wrote:
> What does it mean for a connection to be "inactive"? I'd assume it means that
> the client has not sent any input events for a while, or the host has sent no
> video updates, but in the context of this CL it means that one or more
channels
> are not "connected"?

Done.

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/connectio...
remoting/protocol/connection_to_host.h:69: virtual void
OnConnectionInactive(bool inactive) = 0;
On 2012/07/16 18:51:23, Wez wrote:
> Why not expose this as a new connection state?

I considered it, but I don't think it would be useful to expose it as part of
State:
 1. It's not really a part of the connection state machine implemented in this
class,
 2. Older version of the webapp may not know how to handle the new state state,
 3. Not all client will care about not-ready state,
 4. Later we may want to replace it with something that exposes more information
about connection health (e.g. delays, etc.).

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/libjingle...
File remoting/protocol/libjingle_transport_factory.cc (right):

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/libjingle...
remoting/protocol/libjingle_transport_factory.cc:246:
event_handler_->OnTransportInactive(this, !channel->writable());
On 2012/07/16 18:51:23, Wez wrote:
> You're picking up whether a channel is "active" from whether or not it's
> writable; that will work for UDP channels but if we ever re-enable TCP
channels
> then the channel will become un-writable when the TCP buffer is full, surely?

Libjingle marks a channel as not-writable when it failed to receive pings on
that channel for more than five seconds. pings are sent every second. It works
the same for both UDP and TCP-based connections.

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/session.h
File remoting/protocol/session.h (right):

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/session.h...
remoting/protocol/session.h:70: // Called when inactive state on one of the
channels changes.
On 2012/07/16 18:51:23, Wez wrote:
> nit: Clarify what it means for a channel to be "inactive".

Done.

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/transport.h
File remoting/protocol/transport.h (right):

http://codereview.chromium.org/10692179/diff/7001/remoting/protocol/transport...
remoting/protocol/transport.h:89: // connections.
On 2012/07/16 18:51:23, Wez wrote:
> nit: Re-express this as the transport being "ready", rather than
> "active/inactive"?

Done.

http://codereview.chromium.org/10692179/diff/7001/remoting/webapp/client_plug...
File remoting/webapp/client_plugin_async.js (right):

http://codereview.chromium.org/10692179/diff/7001/remoting/webapp/client_plug...
remoting/webapp/client_plugin_async.js:41: this.onDesktopSizeUpdateHandler =
function () {};
On 2012/07/16 18:51:23, Wez wrote:
> nit: Add a comment for the desktop size update handler, for consistency.

onDesktopSizeUpdateHandler() doesn't have any parameters, and we need the
comments for other event handler just to declare types for JS compiler. There
are comments for this callback in client_plugin.js.

http://codereview.chromium.org/10692179/diff/7001/remoting/webapp/main.css
File remoting/webapp/main.css (right):

http://codereview.chromium.org/10692179/diff/7001/remoting/webapp/main.css#ne...
remoting/webapp/main.css:566: -webkit-filter: grayscale(70%) blur(1px);
On 2012/07/16 18:52:39, Wez wrote:
> nit: Do we really want to blue the desktop at this point? That seems a little
> more disruptive than is necessary.

removed blur filter for now, but we may need to reevaluate effects that we use
here in the future if we get some users feedback about it. E.g. not all users
may notice that screen is being desaturated.

[Wez, 2012-07-17 01:47:50]

http://codereview.chromium.org/10692179/diff/7001/remoting/webapp/main.css
File remoting/webapp/main.css (right):

http://codereview.chromium.org/10692179/diff/7001/remoting/webapp/main.css#ne...
remoting/webapp/main.css:566: -webkit-filter: grayscale(70%) blur(1px);
On 2012/07/17 01:17:18, sergeyu wrote:
> On 2012/07/16 18:52:39, Wez wrote:
> > nit: Do we really want to blue the desktop at this point? That seems a
little
> > more disruptive than is necessary.
> 
> removed blur filter for now, but we may need to reevaluate effects that we use
> here in the future if we get some users feedback about it. E.g. not all users
> may notice that screen is being desaturated.

Let's take a look at the blur effect tomorrow and decide - without seeing it
it's hard to judge the impact!

http://codereview.chromium.org/10692179/diff/16001/remoting/protocol/connecti...
File remoting/protocol/connection_to_host.cc (right):

http://codereview.chromium.org/10692179/diff/16001/remoting/protocol/connecti...
remoting/protocol/connection_to_host.cc:215:
not_ready_channels_.erase(channel_name);
Should we set |not_ready_channels_| to contain all the channels, initially, so
that the "ready" state is accurate at start of day?

http://codereview.chromium.org/10692179/diff/16001/remoting/protocol/connecti...
remoting/protocol/connection_to_host.cc:220: if (ready &&
not_ready_channels_.size() == 0U) {
Can't this entire block be
event_callback_.OnConnectionReady(not_ready_channels.empty())? The only downside
is that you might see multiple callbacks with the same state, but so long as we
state that callbacks must cope with that, I think that's fine.

http://codereview.chromium.org/10692179/diff/16001/remoting/protocol/connecti...
File remoting/protocol/connection_to_host.h (right):

http://codereview.chromium.org/10692179/diff/16001/remoting/protocol/connecti...
remoting/protocol/connection_to_host.h:71: // delayed. This is used byt the UI
to indicate when connection is
typo: byt

http://codereview.chromium.org/10692179/diff/16001/remoting/protocol/connecti...
remoting/protocol/connection_to_host.h:71: // delayed. This is used byt the UI
to indicate when connection is
suggest: This is used to indicate in the UI that the connection is temporarily
broken.

http://codereview.chromium.org/10692179/diff/16001/remoting/protocol/connecti...
remoting/protocol/connection_to_host.h:162: // List of channels that are
currenly in not-ready state.
typo: currenly

http://codereview.chromium.org/10692179/diff/16001/remoting/protocol/connecti...
remoting/protocol/connection_to_host.h:162: // List of channels that are
currenly in not-ready state.
nit: ... that are not currently ready.

[Jamie, 2012-07-17 02:58:10]

http://codereview.chromium.org/10692179/diff/7001/remoting/webapp/main.css
File remoting/webapp/main.css (right):

http://codereview.chromium.org/10692179/diff/7001/remoting/webapp/main.css#ne...
remoting/webapp/main.css:566: -webkit-filter: grayscale(70%) blur(1px);
On 2012/07/17 01:47:50, Wez wrote:
> On 2012/07/17 01:17:18, sergeyu wrote:
> > On 2012/07/16 18:52:39, Wez wrote:
> > > nit: Do we really want to blue the desktop at this point? That seems a
> little
> > > more disruptive than is necessary.
> > 
> > removed blur filter for now, but we may need to reevaluate effects that we
use
> > here in the future if we get some users feedback about it. E.g. not all
users
> > may notice that screen is being desaturated.
> 
> Let's take a look at the blur effect tomorrow and decide - without seeing it
> it's hard to judge the impact!

We should get UX sign-off on this sort of thing, but FWIW, I like the blur.

[Sergey Ulanov, 2012-07-18 00:52:07]

http://codereview.chromium.org/10692179/diff/7001/remoting/webapp/main.css
File remoting/webapp/main.css (right):

http://codereview.chromium.org/10692179/diff/7001/remoting/webapp/main.css#ne...
remoting/webapp/main.css:566: -webkit-filter: grayscale(70%) blur(1px);
On 2012/07/17 01:47:50, Wez wrote:
> On 2012/07/17 01:17:18, sergeyu wrote:
> > On 2012/07/16 18:52:39, Wez wrote:
> > > nit: Do we really want to blue the desktop at this point? That seems a
> little
> > > more disruptive than is necessary.
> > 
> > removed blur filter for now, but we may need to reevaluate effects that we
use
> > here in the future if we get some users feedback about it. E.g. not all
users
> > may notice that screen is being desaturated.
> 
> Let's take a look at the blur effect tomorrow and decide - without seeing it
> it's hard to judge the impact!

Can we do it later? it's not the most important part of this CL

http://codereview.chromium.org/10692179/diff/16001/remoting/protocol/connecti...
File remoting/protocol/connection_to_host.cc (right):

http://codereview.chromium.org/10692179/diff/16001/remoting/protocol/connecti...
remoting/protocol/connection_to_host.cc:215:
not_ready_channels_.erase(channel_name);
On 2012/07/17 01:47:50, Wez wrote:
> Should we set |not_ready_channels_| to contain all the channels, initially, so
> that the "ready" state is accurate at start of day?

I don't think it's necessary because we know that each channels is ready at some
point before state is set to CONNECTED.

http://codereview.chromium.org/10692179/diff/16001/remoting/protocol/connecti...
remoting/protocol/connection_to_host.cc:220: if (ready &&
not_ready_channels_.size() == 0U) {
On 2012/07/17 01:47:50, Wez wrote:
> Can't this entire block be
> event_callback_.OnConnectionReady(not_ready_channels.empty())? The only
downside
> is that you might see multiple callbacks with the same state, but so long as
we
> state that callbacks must cope with that, I think that's fine.

Done.

http://codereview.chromium.org/10692179/diff/16001/remoting/protocol/connecti...
File remoting/protocol/connection_to_host.h (right):

http://codereview.chromium.org/10692179/diff/16001/remoting/protocol/connecti...
remoting/protocol/connection_to_host.h:71: // delayed. This is used byt the UI
to indicate when connection is
On 2012/07/17 01:47:50, Wez wrote:
> typo: byt

Done.

http://codereview.chromium.org/10692179/diff/16001/remoting/protocol/connecti...
remoting/protocol/connection_to_host.h:71: // delayed. This is used byt the UI
to indicate when connection is
On 2012/07/17 01:47:50, Wez wrote:
> suggest: This is used to indicate in the UI that the connection is temporarily
> broken.

Done.

http://codereview.chromium.org/10692179/diff/16001/remoting/protocol/connecti...
remoting/protocol/connection_to_host.h:162: // List of channels that are
currenly in not-ready state.
On 2012/07/17 01:47:50, Wez wrote:
> typo: currenly

Done.

http://codereview.chromium.org/10692179/diff/16001/remoting/protocol/connecti...
remoting/protocol/connection_to_host.h:162: // List of channels that are
currenly in not-ready state.
On 2012/07/17 01:47:50, Wez wrote:
> nit: ... that are not currently ready.

Done.

[Wez, 2012-07-19 00:53:08]

lgtm

[commit-bot: I haz the power, 2012-07-20 01:38:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10692179/27003

[commit-bot: I haz the power, 2012-07-23 03:40:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10692179/28008

[commit-bot: I haz the power, 2012-07-23 05:00:34]

Change committed as 147830