Refactor IT2Me-specific functions into a HostObserver subclass.

Refactor IT2Me-specific functions into a HostObserver subclass.

This also removes the curtain-mode hooks from DesktopEnvironment and ChromotingHost.

Me2Me hosts will no longer show the Disconnect or Continue windows.  It2Me hosts should be unaffected, except for the removal of curtain-mode (which will be added back in a later CL).


BUG=104377
TEST=Manual and unittests


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

[Sergey Ulanov, 2011-11-29 20:17:59]

Looks mostly good. The main problem is that I think that Curtain doesn't belong
to DesktopEnvironment. I think for now we should just remove curtain-related
code from DesktopEnvironment and ChromotingHost.

http://codereview.chromium.org/8725016/diff/1001/remoting/host/desktop_enviro...
File remoting/host/desktop_environment.h (right):

http://codereview.chromium.org/8725016/diff/1001/remoting/host/desktop_enviro...
remoting/host/desktop_environment.h:39: Curtain* curtain() const { return
curtain_.get(); }
Do we want curtain to be part of DesktopEnvironment or should it also be moved
to the new class?

http://codereview.chromium.org/8725016/diff/1001/remoting/host/it2me_observer.cc
File remoting/host/it2me_observer.cc (right):

http://codereview.chromium.org/8725016/diff/1001/remoting/host/it2me_observer...
remoting/host/it2me_observer.cc:63: authenticated_jid_ = jid;
add DCHECK(authenticated_jid_.empty()) here because we are not expecting more
than one concurrent connection.

http://codereview.chromium.org/8725016/diff/1001/remoting/host/it2me_observer...
remoting/host/it2me_observer.cc:176: base::Unretained(this)),
nit: can this fit on the previous line?

http://codereview.chromium.org/8725016/diff/1001/remoting/host/it2me_observer...
remoting/host/it2me_observer.cc:178: } else if (!start) {
if(!start) is redundant here.

http://codereview.chromium.org/8725016/diff/1001/remoting/host/it2me_observer...
remoting/host/it2me_observer.cc:192: base::Unretained(this)),
nit: move to the previous line.

http://codereview.chromium.org/8725016/diff/1001/remoting/host/it2me_observer.h
File remoting/host/it2me_observer.h (right):

http://codereview.chromium.org/8725016/diff/1001/remoting/host/it2me_observer...
remoting/host/it2me_observer.h:36: class It2MeObserver : public
HostStatusObserver {
I don't think this is a good name for this class - it doesn't reflect what this
class does. This class implements UI for It2Me host, so I think it should be
called  SupportHostController or It2MeHostUserInterface, or something like it.

[Lambros, 2011-11-29 23:46:25]

On 2011/11/29 20:17:59, sergeyu wrote:
> Looks mostly good. The main problem is that I think that Curtain doesn't
belong
> to DesktopEnvironment. I think for now we should just remove curtain-related
> code from DesktopEnvironment and ChromotingHost.

True, but it also doesn't belong to the IT2MeObserver class, as curtain-mode is
generally a Me2Me function :)  That's why I've left that code alone in this CL,
we can deal with it separately.

[Sergey Ulanov, 2011-11-29 23:59:29]

On Tue, Nov 29, 2011 at 3:46 PM, <lambroslambrou@chromium.org> wrote:

> On 2011/11/29 20:17:59, sergeyu wrote:
>
>> Looks mostly good. The main problem is that I think that Curtain doesn't
>>
> belong
>
>> to DesktopEnvironment. I think for now we should just remove
>> curtain-related
>> code from DesktopEnvironment and ChromotingHost.
>>
>
> True, but it also doesn't belong to the IT2MeObserver class, as
> curtain-mode is
> generally a Me2Me function :)  That's why I've left that code alone in
> this CL,
> we can deal with it separately.
>

You are right, it doesn't belong to IT2MeObserver, that's why I didn't
suggest to move it to IT2MeObserver :-).  I just think we can remove it
from DesktopEnvironment now and add it later to an analogue
of IT2MeObserver for Me2Me.


>
>
>
http://codereview.chromium.**org/8725016/<http://codereview.chromium.org/8725...
>

[Lambros, 2011-11-30 20:47:02]

http://codereview.chromium.org/8725016/diff/1001/remoting/host/desktop_enviro...
File remoting/host/desktop_environment.h (right):

http://codereview.chromium.org/8725016/diff/1001/remoting/host/desktop_enviro...
remoting/host/desktop_environment.h:39: Curtain* curtain() const { return
curtain_.get(); }
On 2011/11/29 20:18:00, sergeyu wrote:
> Do we want curtain to be part of DesktopEnvironment or should it also be moved
> to the new class?
Will eventually move to a Me2Me-specific HostObserver class.  For now, removed
curtain from DesktopEnv + ChromotingHost.

http://codereview.chromium.org/8725016/diff/1001/remoting/host/it2me_observer.cc
File remoting/host/it2me_observer.cc (right):

http://codereview.chromium.org/8725016/diff/1001/remoting/host/it2me_observer...
remoting/host/it2me_observer.cc:63: authenticated_jid_ = jid;
On 2011/11/29 20:18:00, sergeyu wrote:
> add DCHECK(authenticated_jid_.empty()) here because we are not expecting more
> than one concurrent connection.

Done.

http://codereview.chromium.org/8725016/diff/1001/remoting/host/it2me_observer...
remoting/host/it2me_observer.cc:176: base::Unretained(this)),
On 2011/11/29 20:18:00, sergeyu wrote:
> nit: can this fit on the previous line?
Not any more :)

http://codereview.chromium.org/8725016/diff/1001/remoting/host/it2me_observer...
remoting/host/it2me_observer.cc:178: } else if (!start) {
On 2011/11/29 20:18:00, sergeyu wrote:
> if(!start) is redundant here.

Done.

http://codereview.chromium.org/8725016/diff/1001/remoting/host/it2me_observer...
remoting/host/it2me_observer.cc:192: base::Unretained(this)),
On 2011/11/29 20:18:00, sergeyu wrote:
> nit: move to the previous line.
Won't fit.

http://codereview.chromium.org/8725016/diff/1001/remoting/host/it2me_observer.h
File remoting/host/it2me_observer.h (right):

http://codereview.chromium.org/8725016/diff/1001/remoting/host/it2me_observer...
remoting/host/it2me_observer.h:36: class It2MeObserver : public
HostStatusObserver {
On 2011/11/29 20:18:00, sergeyu wrote:
> I don't think this is a good name for this class - it doesn't reflect what
this
> class does. This class implements UI for It2Me host, so I think it should be
> called  SupportHostController or It2MeHostUserInterface, or something like it.

Done.

[Sergey Ulanov, 2011-11-30 21:24:37]

LGTM,
I think we can now remove is_it2me flag from ChromotingHost.
Also, please update CL description regarding curtain removal.

http://codereview.chromium.org/8725016/diff/12001/remoting/host/chromoting_ho...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/8725016/diff/12001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.cc:311: if (is_it2me_ &&
AuthenticatedClientsCount() > 0) {
I think we can remove is_it2me_ flag now. Here we should always reject second
connection, for Me2Me too - it will not work properly anyway.

http://codereview.chromium.org/8725016/diff/12001/remoting/host/it2me_host_us...
File remoting/host/it2me_host_user_interface.cc (right):

http://codereview.chromium.org/8725016/diff/12001/remoting/host/it2me_host_us...
remoting/host/it2me_host_user_interface.cc:31: ChromotingHostContext* context)
nit: align this with (

http://codereview.chromium.org/8725016/diff/12001/remoting/host/it2me_host_us...
remoting/host/it2me_host_user_interface.cc:48: ContinueWindow* continue_window,
same here

[Lambros, 2011-11-30 22:09:41]

Updated CL description.

http://codereview.chromium.org/8725016/diff/12001/remoting/host/chromoting_ho...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/8725016/diff/12001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.cc:311: if (is_it2me_ &&
AuthenticatedClientsCount() > 0) {
On 2011/11/30 21:24:37, sergeyu wrote:
> I think we can remove is_it2me_ flag now. Here we should always reject second
> connection, for Me2Me too - it will not work properly anyway.
I will address this in a followup CL.  Dealing with a second connection should
be different between IT2Me (shutdown the Host), and Me2Me (disconnect the first
connection and allow the new connection to proceed).  So the logic needs to be
moved to It2Me- and Me2Me-specific classes.

http://codereview.chromium.org/8725016/diff/12001/remoting/host/it2me_host_us...
File remoting/host/it2me_host_user_interface.cc (right):

http://codereview.chromium.org/8725016/diff/12001/remoting/host/it2me_host_us...
remoting/host/it2me_host_user_interface.cc:31: ChromotingHostContext* context)
On 2011/11/30 21:24:37, sergeyu wrote:
> nit: align this with (

Done.

http://codereview.chromium.org/8725016/diff/12001/remoting/host/it2me_host_us...
remoting/host/it2me_host_user_interface.cc:48: ContinueWindow* continue_window,
On 2011/11/30 21:24:37, sergeyu wrote:
> same here

Done.

[commit-bot: I haz the power, 2011-12-01 01:07:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/8725016/24001

[commit-bot: I haz the power, 2011-12-01 04:25:27]

Change committed as 112392