Adds clearer function to create rtc::Thread without Physical SocketServer

Adds clearer function to create rtc::Thread without Physical SocketServer
Moves logic of default SocketServer from MessageQueue to SocketServer

Committed: https://crrev.com/bebf54cad1b8c5156652a869305c7be9dc2e9ddc
Cr-Commit-Position: refs/heads/master@{#12541}

[danilchap, 2016-04-15 17:49:15]

danilchap@webrtc.org changed reviewers:
+ pthatcher@webrtc.org, stefan@webrtc.org

[danilchap, 2016-04-15 17:49:15]

Most threads do not need PhysicalSocketServer. This CL adds function that allows
creating Thread with NullSocketServer without mentioning any SocketServer.

This CL is part of introducing Network Thread to improve bwe prober.
(https://codereview.webrtc.org/1888903003/).
Expirements shows that posting from network thread to worker thread with
PhysicalSocketServer do not improve prober much, but posing to worker thread
with NullSocketServer make the difference.

[stefan-webrtc, 2016-04-19 08:29:33]

https://codereview.webrtc.org/1891293002/diff/40001/webrtc/base/messagequeue.cc
File webrtc/base/messagequeue.cc (right):

https://codereview.webrtc.org/1891293002/diff/40001/webrtc/base/messagequeue....
webrtc/base/messagequeue.cc:115: // messagequeue_unittest to depend on network
libraries... yuck.
Is this now fixed?

https://codereview.webrtc.org/1891293002/diff/40001/webrtc/base/messagequeue.h
File webrtc/base/messagequeue.h (right):

https://codereview.webrtc.org/1891293002/diff/40001/webrtc/base/messagequeue....
webrtc/base/messagequeue.h:280: // If caller pass socketserver with ownship.
s/ownship/ownership

https://codereview.webrtc.org/1891293002/diff/40001/webrtc/base/nullsocketser...
File webrtc/base/nullsocketserver.cc (right):

https://codereview.webrtc.org/1891293002/diff/40001/webrtc/base/nullsocketser...
webrtc/base/nullsocketserver.cc:11: #ifndef WEBRTC_BASE_NULLSOCKETSERVER_H__
No need for these in a .cc file, right?

[danilchap, 2016-04-19 09:43:10]

https://codereview.webrtc.org/1891293002/diff/40001/webrtc/base/messagequeue.cc
File webrtc/base/messagequeue.cc (right):

https://codereview.webrtc.org/1891293002/diff/40001/webrtc/base/messagequeue....
webrtc/base/messagequeue.cc:115: // messagequeue_unittest to depend on network
libraries... yuck.
On 2016/04/19 08:29:33, stefan-webrtc (holmer) wrote:
> Is this now fixed?

MessageQueue still holds a socket server.
This CL though a step towards removing dependency on network libraries (message
queue doesn't create socketserver itself).
Removing this dependency is not trivial (tried) and better be done in another
CL.

https://codereview.webrtc.org/1891293002/diff/40001/webrtc/base/messagequeue.h
File webrtc/base/messagequeue.h (right):

https://codereview.webrtc.org/1891293002/diff/40001/webrtc/base/messagequeue....
webrtc/base/messagequeue.h:280: // If caller pass socketserver with ownship.
On 2016/04/19 08:29:33, stefan-webrtc (holmer) wrote:
> s/ownship/ownership

Done.

https://codereview.webrtc.org/1891293002/diff/40001/webrtc/base/nullsocketser...
File webrtc/base/nullsocketserver.cc (right):

https://codereview.webrtc.org/1891293002/diff/40001/webrtc/base/nullsocketser...
webrtc/base/nullsocketserver.cc:11: #ifndef WEBRTC_BASE_NULLSOCKETSERVER_H__
On 2016/04/19 08:29:33, stefan-webrtc (holmer) wrote:
> No need for these in a .cc file, right?

Oops, removed.

[stefan-webrtc, 2016-04-19 10:24:32]

lgtm

[danilchap, 2016-04-25 09:45:02]

danilchap@webrtc.org changed reviewers:
+ perkj@webrtc.org

[perkj_webrtc, 2016-04-27 10:54:01]

tommi might have comments on this class as well if you want.

https://codereview.webrtc.org/1891293002/diff/80001/webrtc/base/thread.h
File webrtc/base/thread.h (right):

https://codereview.webrtc.org/1891293002/diff/80001/webrtc/base/thread.h#newc...
webrtc/base/thread.h:103: explicit Thread(SocketServer* ss);
Do we really need all these for versions in production code? Can test only
versions be removed? 

or move all ctors to be protected and use only your Create methods?

[tommi, 2016-04-27 12:45:25]

tommi@webrtc.org changed reviewers:
+ tommi@webrtc.org

[tommi, 2016-04-27 12:45:25]

I'm really liking this cl!  Have a couple of questions but lgtm.

https://codereview.webrtc.org/1891293002/diff/100001/webrtc/base/messagequeue.h
File webrtc/base/messagequeue.h (right):

https://codereview.webrtc.org/1891293002/diff/100001/webrtc/base/messagequeue...
webrtc/base/messagequeue.h:178: MessageQueue(SocketServer* ss, bool init_queue);
Should this ctor be marked as deprecated?
Would it make sense at some point to have a derived MessageQueue class own the
SocketServer integration?

https://codereview.webrtc.org/1891293002/diff/100001/webrtc/base/messagequeue...
webrtc/base/messagequeue.h:281: // If caller pass socketserver with ownership.
nit: passes
or "Used if socketserver ownership lies with |this|."

https://codereview.webrtc.org/1891293002/diff/100001/webrtc/base/thread.h
File webrtc/base/thread.h (right):

https://codereview.webrtc.org/1891293002/diff/100001/webrtc/base/thread.h#new...
webrtc/base/thread.h:103: explicit Thread(SocketServer* ss);
Down the line, I think it would be good if the socket server dependency could be
moved to a derived class, e.g. SocketServerThread.

[danilchap, 2016-04-27 15:08:55]

https://codereview.webrtc.org/1891293002/diff/80001/webrtc/base/thread.h
File webrtc/base/thread.h (right):

https://codereview.webrtc.org/1891293002/diff/80001/webrtc/base/thread.h#newc...
webrtc/base/thread.h:103: explicit Thread(SocketServer* ss);
On 2016/04/27 10:54:01, perkj_webrtc wrote:
> Do we really need all these for versions in production code? Can test only
> versions be removed? 
> 
> or move all ctors to be protected and use only your Create methods? 

Didn't found any use of init_queue = false, so removing it from constructors,
reducing their number to three.
Test only constructor Thread(SocketServer*) can't be easily removed: need to
update a lot of tests first.

https://codereview.webrtc.org/1891293002/diff/100001/webrtc/base/messagequeue.h
File webrtc/base/messagequeue.h (right):

https://codereview.webrtc.org/1891293002/diff/100001/webrtc/base/messagequeue...
webrtc/base/messagequeue.h:178: MessageQueue(SocketServer* ss, bool init_queue);
On 2016/04/27 12:45:25, tommi-webrtc wrote:
> Should this ctor be marked as deprecated?
> Would it make sense at some point to have a derived MessageQueue class own the
> SocketServer integration?

It would make sense to remove MessageQueue dependency on SocketServer.
Which means both constructors should be changed.
While I have some ideas how to do it, would prefer to leave it to another cl.

https://codereview.webrtc.org/1891293002/diff/100001/webrtc/base/messagequeue...
webrtc/base/messagequeue.h:281: // If caller pass socketserver with ownership.
On 2016/04/27 12:45:25, tommi-webrtc wrote:
> nit: passes
> or "Used if socketserver ownership lies with |this|."

Done.

https://codereview.webrtc.org/1891293002/diff/100001/webrtc/base/thread.h
File webrtc/base/thread.h (right):

https://codereview.webrtc.org/1891293002/diff/100001/webrtc/base/thread.h#new...
webrtc/base/thread.h:103: explicit Thread(SocketServer* ss);
On 2016/04/27 12:45:25, tommi-webrtc wrote:
> Down the line, I think it would be good if the socket server dependency could
be
> moved to a derived class, e.g. SocketServerThread.

I see this cl as a small step towards that goal.

[danilchap, 2016-04-28 08:25:42]

The CQ bit was checked by danilchap@webrtc.org

[danilchap, 2016-04-28 08:25:42]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org,
tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1891293002/#ps140001 (title:
" ")

[commit-bot: I haz the power, 2016-04-28 08:25:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1891293002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1891293002/140001

[commit-bot: I haz the power, 2016-04-28 08:32:53]

Committed patchset #8 (id:140001)

[pthatcher1, 2016-04-29 22:56:35]

lgtm

[commit-bot: I haz the power, 2016-05-01 22:01:08]

Description was changed from

==========
Adds clearer function to create rtc::Thread without Physical SocketServer
Moves logic of default SocketServer from MessageQueue to SocketServer
==========

to

==========
Adds clearer function to create rtc::Thread without Physical SocketServer
Moves logic of default SocketServer from MessageQueue to SocketServer

Committed: https://crrev.com/bebf54cad1b8c5156652a869305c7be9dc2e9ddc
Cr-Commit-Position: refs/heads/master@{#12541}
==========

[commit-bot: I haz the power, 2016-05-01 22:01:09]

Patchset 8 (id:??) landed as
https://crrev.com/bebf54cad1b8c5156652a869305c7be9dc2e9ddc
Cr-Commit-Position: refs/heads/master@{#12541}