Pull out common code from client and host versions of ServerLogEntry.

Pull out common code from client and host versions of ServerLogEntry.

This also adds unittests for ServerLogEntry client code, with common code
pulled out to a separate file.

This doesn't affect LogToServer classes - they will be refactored in a
separate CL. Also fix some IWYU issues uncovered by the refactor.

TEST=unittests

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

[Lambros, 2014-05-16 23:41:45]

Sorry it's a biggie - can't see any way to split this up.

[rmsousa, 2014-05-20 00:54:41]

lgtm w/ nits
(but I don't know how to resolve the first one, and am not sure if the second
one would look cleaner, so it's your call on both of them)

https://codereview.chromium.org/282063005/diff/60001/remoting/client/jni/chro...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/282063005/diff/60001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.cc:22: #include
"remoting/protocol/server_log_entry.h"
Nit: Not sure if protocol is the right long-term home for it - protocol/ mostly
contains the the client-host protocol.
On the other hand, there isn't really any other good place for code that is
common between client and host but not part of the protocol... (base/ can't
depend on jingle_glue, and jingle_glue/ itself mostly contains low-level stuff).

https://codereview.chromium.org/282063005/diff/60001/remoting/client/server_l...
File remoting/client/server_log_entry_client.h (right):

https://codereview.chromium.org/282063005/diff/60001/remoting/client/server_l...
remoting/client/server_log_entry_client.h:20: // Constructs a log entry for a
session state change.
Nit: I think you could inherit from ServerLogEntry, and make these MakeLogEntry*
statics and the Add* member methods. Not sure if it would look cleaner.

(same for the host side)

[Lambros, 2014-05-20 01:13:34]

Adding sergeyu@ for thoughts on this.

https://codereview.chromium.org/282063005/diff/60001/remoting/client/jni/chro...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/282063005/diff/60001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.cc:22: #include
"remoting/protocol/server_log_entry.h"
On 2014/05/20 00:54:41, rmsousa wrote:
> Nit: Not sure if protocol is the right long-term home for it - protocol/
mostly
> contains the the client-host protocol.
> On the other hand, there isn't really any other good place for code that is
> common between client and host but not part of the protocol... (base/ can't
> depend on jingle_glue, and jingle_glue/ itself mostly contains low-level
stuff).

It would be slightly cleaner without the protocol:: namespace, certainly. But
I'm not sure what else to do with this. @Sergey, any thoughts?

https://codereview.chromium.org/282063005/diff/60001/remoting/client/server_l...
File remoting/client/server_log_entry_client.h (right):

https://codereview.chromium.org/282063005/diff/60001/remoting/client/server_l...
remoting/client/server_log_entry_client.h:20: // Constructs a log entry for a
session state change.
On 2014/05/20 00:54:41, rmsousa wrote:
> Nit: I think you could inherit from ServerLogEntry, and make these
MakeLogEntry*
> statics and the Add* member methods. Not sure if it would look cleaner.
> 
> (same for the host side)

I thought about that, but I'm not sure. The problem is that this class is a
wrapper around a std::map, and is passed by value and stored in a std::vector.
People might be tempted to add extra fields to the derived class, which might
cause problems with casting between base/derived types.

[Sergey Ulanov, 2014-05-21 04:02:36]

https://codereview.chromium.org/282063005/diff/60001/remoting/client/jni/chro...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/282063005/diff/60001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.cc:22: #include
"remoting/protocol/server_log_entry.h"
On 2014/05/20 01:13:35, Lambros wrote:
> On 2014/05/20 00:54:41, rmsousa wrote:
> > Nit: Not sure if protocol is the right long-term home for it - protocol/
> mostly
> > contains the the client-host protocol.
> > On the other hand, there isn't really any other good place for code that is
> > common between client and host but not part of the protocol... (base/ can't
> > depend on jingle_glue, and jingle_glue/ itself mostly contains low-level
> stuff).
> 
> It would be slightly cleaner without the protocol:: namespace, certainly. But
> I'm not sure what else to do with this. @Sergey, any thoughts?

Hm. Good question. I suggest putting this stuff in jingle_glue for now. In the
past jingle_glue was used for libjingle adapters and such, but currently it
contains mostly signaling code. Server-side logging looks like it belongs to
signaling layer (but it's arguable). Later we should rename
'jingle_glue'->'signaling' and move chromium_port_allocator and
chromium_socket_factory to remoting/protocol. jingle_glue is a crappy name
anyway, and it's easily confused with jingle/glue. WDYT?

[Lambros, 2014-05-21 15:27:28]

SGTM


On Tue, May 20, 2014 at 9:02 PM, <sergeyu@chromium.org> wrote:

>
> https://codereview.chromium.org/282063005/diff/60001/remoting/client/jni/
> chromoting_jni_instance.cc
> File remoting/client/jni/chromoting_jni_instance.cc (right):
>
> https://codereview.chromium.org/282063005/diff/60001/remoting/client/jni/
> chromoting_jni_instance.cc#newcode22
> remoting/client/jni/chromoting_jni_instance.cc:22: #include
> "remoting/protocol/server_log_entry.h"
> On 2014/05/20 01:13:35, Lambros wrote:
>
>> On 2014/05/20 00:54:41, rmsousa wrote:
>> > Nit: Not sure if protocol is the right long-term home for it -
>>
> protocol/
>
>> mostly
>> > contains the the client-host protocol.
>> > On the other hand, there isn't really any other good place for code
>>
> that is
>
>> > common between client and host but not part of the protocol...
>>
> (base/ can't
>
>> > depend on jingle_glue, and jingle_glue/ itself mostly contains
>>
> low-level
>
>> stuff).
>>
>
>  It would be slightly cleaner without the protocol:: namespace,
>>
> certainly. But
>
>> I'm not sure what else to do with this. @Sergey, any thoughts?
>>
>
> Hm. Good question. I suggest putting this stuff in jingle_glue for now.
> In the past jingle_glue was used for libjingle adapters and such, but
> currently it contains mostly signaling code. Server-side logging looks
> like it belongs to signaling layer (but it's arguable). Later we should
> rename 'jingle_glue'->'signaling' and move chromium_port_allocator and
> chromium_socket_factory to remoting/protocol. jingle_glue is a crappy
> name anyway, and it's easily confused with jingle/glue. WDYT?
>
> https://codereview.chromium.org/282063005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Lambros, 2014-05-22 00:10:48]

ptal. The only change I've made is to move ServerLogEntry (+unittests) from
protocol/ to jingle_glue/

[rmsousa, 2014-05-22 00:13:36]

lgtm

[Lambros, 2014-05-22 01:45:20]

The CQ bit was checked by lambroslambrou@chromium.org

[commit-bot: I haz the power, 2014-05-22 01:49:22]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/282063005/...

[commit-bot: I haz the power, 2014-05-22 11:47:07]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-05-22 11:48:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-22 11:48:58]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Lambros, 2014-05-22 22:17:57]

The CQ bit was checked by lambroslambrou@chromium.org

[commit-bot: I haz the power, 2014-05-22 22:19:09]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/282063005/...

[commit-bot: I haz the power, 2014-05-22 22:21:25]

Change committed as 272346