[Chromoting] LogToServer correctly handles multiple simultaneous connections.

[Chromoting] LogToServer correctly handles multiple simultaneous connections.

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

[simonmorris, 2012-05-21 20:27:39]

ptal

[Wez, 2012-05-21 20:40:13]

http://codereview.chromium.org/10413035/diff/1/remoting/host/log_to_server.h
File remoting/host/log_to_server.h (right):

http://codereview.chromium.org/10413035/diff/1/remoting/host/log_to_server.h#...
remoting/host/log_to_server.h:73: bool route_type_set_;
Can you use the presence of an entry for the client connection JID in the
|connection_info/type_| table as indicating that the type is set, and thereby
avoid the need for this class?

[simonmorris, 2012-05-21 20:45:33]

On 2012/05/21 20:40:13, Wez wrote:
> http://codereview.chromium.org/10413035/diff/1/remoting/host/log_to_server.h
> File remoting/host/log_to_server.h (right):
> 
>
http://codereview.chromium.org/10413035/diff/1/remoting/host/log_to_server.h#...
> remoting/host/log_to_server.h:73: bool route_type_set_;
> Can you use the presence of an entry for the client connection JID in the
> |connection_info/type_| table as indicating that the type is set, and thereby
> avoid the need for this class?

That would break if we ever added another connection info field, which could be
set independently of the route type.

If we believe we'll never add such a field, or would prefer to restore this
class when we do, we could remove this class and just store a map from client
JID to route type.

[Wez, 2012-05-21 20:58:50]

On 2012/05/21 20:45:33, simonmorris wrote:
> On 2012/05/21 20:40:13, Wez wrote:
> > http://codereview.chromium.org/10413035/diff/1/remoting/host/log_to_server.h
> > File remoting/host/log_to_server.h (right):
> > 
> >
>
http://codereview.chromium.org/10413035/diff/1/remoting/host/log_to_server.h#...
> > remoting/host/log_to_server.h:73: bool route_type_set_;
> > Can you use the presence of an entry for the client connection JID in the
> > |connection_info/type_| table as indicating that the type is set, and
thereby
> > avoid the need for this class?
> 
> That would break if we ever added another connection info field, which could
be
> set independently of the route type.
> 
> If we believe we'll never add such a field, or would prefer to restore this
> class when we do, we could remove this class and just store a map from client
> JID to route type.

Personally, I think it's fine to go with the map for now and add the class
if/when we need it.

[simonmorris, 2012-05-21 22:34:36]

http://codereview.chromium.org/10413035/diff/1/remoting/host/log_to_server.h
File remoting/host/log_to_server.h (right):

http://codereview.chromium.org/10413035/diff/1/remoting/host/log_to_server.h#...
remoting/host/log_to_server.h:73: bool route_type_set_;
On 2012/05/21 20:40:13, Wez wrote:
> Can you use the presence of an entry for the client connection JID in the
> |connection_info/type_| table as indicating that the type is set, and thereby
> avoid the need for this class?

Done.

[Wez, 2012-05-21 22:47:31]

lgtm

http://codereview.chromium.org/10413035/diff/5001/remoting/host/log_to_server...
File remoting/host/log_to_server_unittest.cc (right):

http://codereview.chromium.org/10413035/diff/5001/remoting/host/log_to_server...
remoting/host/log_to_server_unittest.cc:65: MATCHER_P2(IsTwoClientsConnected,
connection_type1, connection_type2, "") {
nit: AreTwoClientsConnected.

[simonmorris, 2012-05-24 22:15:21]

fyi

http://codereview.chromium.org/10413035/diff/5001/remoting/host/log_to_server...
File remoting/host/log_to_server_unittest.cc (right):

http://codereview.chromium.org/10413035/diff/5001/remoting/host/log_to_server...
remoting/host/log_to_server_unittest.cc:65: MATCHER_P2(IsTwoClientsConnected,
connection_type1, connection_type2, "") {
On 2012/05/21 22:47:31, Wez wrote:
> nit: AreTwoClientsConnected.

The matcher is a function of a single parameter, so "is" is more accurate. It's
just that the parameter may describe two clients having connected.

[commit-bot: I haz the power, 2012-05-24 22:17:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10413035/9001

[commit-bot: I haz the power, 2012-05-25 00:44:01]

Change committed as 138952