Remove authenticated_ fields from stubs.

The authenticated_ fields are moved out of stubs and into
ClientSession. Messages to the stubs are dispatched via
ClientSession, and the stub classes are pure virtual.

BUG=none
TEST=none

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

[Alpha Left Google, 2011-03-28 16:22:26]

Looking very good. I have some comments about coding style and commenting.

http://codereview.chromium.org/6724033/diff/3001/remoting/host/chromoting_hos...
File remoting/host/chromoting_host_unittest.cc (right):

http://codereview.chromium.org/6724033/diff/3001/remoting/host/chromoting_hos...
remoting/host/chromoting_host_unittest.cc:63: class DummyTask : public Task {
Alternatively you can also do:

void DummyTask() {}
and then NewRunnableFunction(&DummyTask);

but that just saves you some typing, works the same :)

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session.cc
File remoting/host/client_session.cc (right):

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session...
remoting/host/client_session.cc:9: #include "media/base/callback.h"
do you mean base/callback.h?

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session.h
File remoting/host/client_session.h (right):

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session...
remoting/host/client_session.h:38: const
base::Callback<UserAuthenticator*(void)>& auth_factory,
I'm a but confused looking at this, can you do some typedef?
Usually for complex type like this we use typedefs so as to make code more
readable.

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session...
remoting/host/client_session.h:55: protocol::ConnectionToClient* connection()
const;
nit: this getter can be inline.

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session...
remoting/host/client_session.h:56: bool authenticated() const;
nit: for simple getter like this you can simply do inline.

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session...
remoting/host/client_session.h:58: protected:
nit: Is there other class that inherit this? I think we can remove this
protected section. In general the coding style is we should have only public and
private, and no sharing members between subclasses. In this case it seems
unnecessary to have protected.

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session...
remoting/host/client_session.h:60: ~ClientSession();
virtual ~ClientSession();

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session...
File remoting/host/client_session_unittest.cc (right):

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session...
remoting/host/client_session_unittest.cc:21: class DummyTask : public Task {
nit: this can be a function, just to save you typing.

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session...
remoting/host/client_session_unittest.cc:104:
client_session_->InjectKeyEvent(&key_event1, new DummyTask());
This is a very good test! I'd like to see a bit of comments to explain why we
only expect certain actions.

http://codereview.chromium.org/6724033/diff/3001/remoting/protocol/connection...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/6724033/diff/3001/remoting/protocol/connection...
remoting/protocol/connection_to_client.h:74: virtual void
set_input_stub(InputStub* input_stub);
do we need a getter like host stub?

http://codereview.chromium.org/6724033/diff/3001/remoting/protocol/connection...
remoting/protocol/connection_to_client.h:74: virtual void
set_input_stub(InputStub* input_stub);
Since we have these setters now for stubs, need to add comments to explain when
we can call this setter, or should we set them before calling Init()?

http://codereview.chromium.org/6724033/diff/3001/remoting/protocol/connection...
remoting/protocol/connection_to_client.h:76: protected:
Can we not have protected here?

[simonmorris, 2011-03-29 17:07:51]

http://codereview.chromium.org/6724033/diff/3001/remoting/host/chromoting_hos...
File remoting/host/chromoting_host_unittest.cc (right):

http://codereview.chromium.org/6724033/diff/3001/remoting/host/chromoting_hos...
remoting/host/chromoting_host_unittest.cc:63: class DummyTask : public Task {
On 2011/03/28 16:22:26, Alpha wrote:
> Alternatively you can also do:
> 
> void DummyTask() {}
> and then NewRunnableFunction(&DummyTask);
> 
> but that just saves you some typing, works the same :)

Done.

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session.cc
File remoting/host/client_session.cc (right):

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session...
remoting/host/client_session.cc:9: #include "media/base/callback.h"
On 2011/03/28 16:22:26, Alpha wrote:
> do you mean base/callback.h?

I wanted media/base/callback.h for AutoTaskRunner.

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session.h
File remoting/host/client_session.h (right):

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session...
remoting/host/client_session.h:38: const
base::Callback<UserAuthenticator*(void)>& auth_factory,
On 2011/03/28 16:22:26, Alpha wrote:
> I'm a but confused looking at this, can you do some typedef?
> Usually for complex type like this we use typedefs so as to make code more
> readable.

Done.

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session...
remoting/host/client_session.h:55: protocol::ConnectionToClient* connection()
const;
On 2011/03/28 16:22:26, Alpha wrote:
> nit: this getter can be inline.

Done.

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session...
remoting/host/client_session.h:56: bool authenticated() const;
On 2011/03/28 16:22:26, Alpha wrote:
> nit: for simple getter like this you can simply do inline.

Done.

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session...
remoting/host/client_session.h:58: protected:
On 2011/03/28 16:22:26, Alpha wrote:
> nit: Is there other class that inherit this? I think we can remove this
> protected section. In general the coding style is we should have only public
and
> private, and no sharing members between subclasses. In this case it seems
> unnecessary to have protected.

Done.

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session...
remoting/host/client_session.h:60: ~ClientSession();
On 2011/03/28 16:22:26, Alpha wrote:
> virtual ~ClientSession();

Done.

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session...
File remoting/host/client_session_unittest.cc (right):

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session...
remoting/host/client_session_unittest.cc:21: class DummyTask : public Task {
On 2011/03/28 16:22:26, Alpha wrote:
> nit: this can be a function, just to save you typing.

It turns out to be more typing overall, when you allow for
"new DummyTask()" being concise.

http://codereview.chromium.org/6724033/diff/3001/remoting/host/client_session...
remoting/host/client_session_unittest.cc:104:
client_session_->InjectKeyEvent(&key_event1, new DummyTask());
On 2011/03/28 16:22:26, Alpha wrote:
> This is a very good test! I'd like to see a bit of comments to explain why we
> only expect certain actions.

Done.

http://codereview.chromium.org/6724033/diff/3001/remoting/protocol/connection...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/6724033/diff/3001/remoting/protocol/connection...
remoting/protocol/connection_to_client.h:74: virtual void
set_input_stub(InputStub* input_stub);
On 2011/03/28 16:22:26, Alpha wrote:
> do we need a getter like host stub?
> 

It turns out we don't need a host_stub_ getter, either,
so I've removed it.

http://codereview.chromium.org/6724033/diff/3001/remoting/protocol/connection...
remoting/protocol/connection_to_client.h:74: virtual void
set_input_stub(InputStub* input_stub);
On 2011/03/28 16:22:26, Alpha wrote:
> Since we have these setters now for stubs, need to add comments to explain
when
> we can call this setter, or should we set them before calling Init()?

Added comments.

http://codereview.chromium.org/6724033/diff/3001/remoting/protocol/connection...
remoting/protocol/connection_to_client.h:76: protected:
On 2011/03/28 16:22:26, Alpha wrote:
> Can we not have protected here?

MockConnectionToClient inherits from ConnectionToClient.
I'm not sure whether that's a good thing, but it means
this dtor can't be private.

[Alpha Left Google, 2011-03-29 18:11:06]

LGTM.

[garykac, 2011-03-30 20:04:03]

LGTM.

Curiously, this looks similar to my first stab at this. I recall getting
comments that we wanted to cut off processing the message ASAP if we weren't
authenticated, so I pushed it down into the dispatcher.

However, I like this approach *much* better.

Some nits that (if you agree) you can put in a separate CL.

http://codereview.chromium.org/6724033/diff/12001/remoting/host/client_sessio...
File remoting/host/client_session.cc (right):

http://codereview.chromium.org/6724033/diff/12001/remoting/host/client_sessio...
remoting/host/client_session.cc:76: done->Run();
nit: The
  media::AutoTaskRunner task_runner(done);
pattern is nicer and consistent with what you did earlier.

http://codereview.chromium.org/6724033/diff/12001/remoting/host/client_sessio...
remoting/host/client_session.cc:86: done->Run();
nit: AutoTaskRunner

[Wez, 2011-03-30 20:24:57]

Drive-by!

LGTM for what it's worth, although see comments regarding the behaviour when we
see unexpected messages while not authenticated.

http://codereview.chromium.org/6724033/diff/12001/remoting/host/client_sessio...
File remoting/host/client_session.cc (right):

http://codereview.chromium.org/6724033/diff/12001/remoting/host/client_sessio...
remoting/host/client_session.cc:38: }
If the client sends unexpected messages before having authenticated, we should
really abort the connection.

http://codereview.chromium.org/6724033/diff/12001/remoting/host/client_sessio...
remoting/host/client_session.cc:86: done->Run();
As above, it's not valid for the client to send input before it has successfully
authenticated.

http://codereview.chromium.org/6724033/diff/12001/remoting/protocol/client_me...
File remoting/protocol/client_message_dispatcher.cc (right):

http://codereview.chromium.org/6724033/diff/12001/remoting/protocol/client_me...
remoting/protocol/client_message_dispatcher.cc:52: LOG(WARNING) << "Invalid
control message received.";
If not authenticated, disconnect?

http://codereview.chromium.org/6724033/diff/12001/remoting/protocol/host_mess...
File remoting/protocol/host_message_dispatcher.cc (right):

http://codereview.chromium.org/6724033/diff/12001/remoting/protocol/host_mess...
remoting/protocol/host_message_dispatcher.cc:62: LOG(WARNING) << "Invalid
control message received.";
If not authenticated, disconnect?

http://codereview.chromium.org/6724033/diff/12001/remoting/protocol/host_mess...
remoting/protocol/host_message_dispatcher.cc:77: delete done_task;
If not authenticated, disconnect?

nit: This block is equivalent to the one in OnControlMessageReceived; could they
be laid out in the same style as one another?

[simonmorris, 2011-03-31 11:14:00]

Jamie: could you check my changes to your changes to
chromoting_host_unittest.cc, and the corresponding changes
to UserAuthenticatorFake?

http://codereview.chromium.org/6724033/diff/12001/remoting/host/client_sessio...
File remoting/host/client_session.cc (right):

http://codereview.chromium.org/6724033/diff/12001/remoting/host/client_sessio...
remoting/host/client_session.cc:38: }
On 2011/03/30 20:24:57, Wez wrote:
> If the client sends unexpected messages before having authenticated, we should
> really abort the connection.

Could be good, in another CL.

http://codereview.chromium.org/6724033/diff/12001/remoting/host/client_sessio...
remoting/host/client_session.cc:76: done->Run();
On 2011/03/30 20:04:03, garykac wrote:
> nit: The
>   media::AutoTaskRunner task_runner(done);
> pattern is nicer and consistent with what you did earlier.

Done.

http://codereview.chromium.org/6724033/diff/12001/remoting/host/client_sessio...
remoting/host/client_session.cc:86: done->Run();
On 2011/03/30 20:04:03, garykac wrote:
> nit: AutoTaskRunner

Done.

http://codereview.chromium.org/6724033/diff/12001/remoting/host/client_sessio...
remoting/host/client_session.cc:86: done->Run();
On 2011/03/30 20:24:57, Wez wrote:
> As above, it's not valid for the client to send input before it has
successfully
> authenticated.

Could be good, in another CL.

http://codereview.chromium.org/6724033/diff/12001/remoting/protocol/client_me...
File remoting/protocol/client_message_dispatcher.cc (right):

http://codereview.chromium.org/6724033/diff/12001/remoting/protocol/client_me...
remoting/protocol/client_message_dispatcher.cc:52: LOG(WARNING) << "Invalid
control message received.";
On 2011/03/30 20:24:57, Wez wrote:
> If not authenticated, disconnect?

Could be good, in another CL.

http://codereview.chromium.org/6724033/diff/12001/remoting/protocol/host_mess...
File remoting/protocol/host_message_dispatcher.cc (right):

http://codereview.chromium.org/6724033/diff/12001/remoting/protocol/host_mess...
remoting/protocol/host_message_dispatcher.cc:62: LOG(WARNING) << "Invalid
control message received.";
On 2011/03/30 20:24:57, Wez wrote:
> If not authenticated, disconnect?

Could be good, in another CL.

http://codereview.chromium.org/6724033/diff/12001/remoting/protocol/host_mess...
remoting/protocol/host_message_dispatcher.cc:77: delete done_task;
On 2011/03/30 20:24:57, Wez wrote:
> If not authenticated, disconnect?
> 
> nit: This block is equivalent to the one in OnControlMessageReceived; could
they
> be laid out in the same style as one another?

Done.

[Jamie, 2011-03-31 11:31:25]

LGTM, but see if you agree with my comments.

http://codereview.chromium.org/6724033/diff/19001/remoting/host/chromoting_ho...
File remoting/host/chromoting_host_unittest.cc (right):

http://codereview.chromium.org/6724033/diff/19001/remoting/host/chromoting_ho...
remoting/host/chromoting_host_unittest.cc:65: void DummyFunction() {}
Can we rename this |DummyDoneTask| or something that better reflects why it's
needed? I had to look at the |BeginSessionRequest| definition to work out what
it was for.

Nit: Other empty function in this file use two lines, not one.

[simonmorris, 2011-03-31 12:23:43]

http://codereview.chromium.org/6724033/diff/19001/remoting/host/chromoting_ho...
File remoting/host/chromoting_host_unittest.cc (right):

http://codereview.chromium.org/6724033/diff/19001/remoting/host/chromoting_ho...
remoting/host/chromoting_host_unittest.cc:65: void DummyFunction() {}
On 2011/03/31 11:31:25, Jamie wrote:
> Can we rename this |DummyDoneTask| or something that better reflects why it's
> needed? I had to look at the |BeginSessionRequest| definition to work out what
> it was for.
> 
> Nit: Other empty function in this file use two lines, not one.

Done.