Rename BEGIN_PROXY_MAP --> BEGIN_SIGNALLING_PROXY_MAP.

Rename BEGIN_PROXY_MAP --> BEGIN_SIGNALLING_PROXY_MAP.

And BEGIN_WORKER_PROXY_MAP --> BEGIN_PROXY_MAP.

This rename was suggested by Tommi, with the idea that a proxy
invoking methods on the worker thread should be the common case.
It's a followup to https://codereview.webrtc.org/1861633002/

This cl also adds unittests for proxy calls to the
worker thread.

BUG=webrtc:5426
Committed: https://crrev.com/72c8d2b708d442d16700229fe22918d187534430
Cr-Commit-Position: refs/heads/master@{#12374}

[nisse-webrtc, 2016-04-08 08:52:22]

nisse@webrtc.org changed reviewers:
+ perkj@webrtc.org, pthatcher@webrtc.org, tommi@webrtc.org

[nisse-webrtc, 2016-04-08 08:52:23]

This is a renaming cl, there's no urgency, but I'd like to know if you agree
it's an improvement.

[tommi, 2016-04-08 09:00:22]

lgtm for the macro name rename, but I'm kind of changing my mind about the class
rename that I suggested :-/

https://codereview.webrtc.org/1871833002/diff/1/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1871833002/diff/1/webrtc/api/peerconnection.cc#...
webrtc/api/peerconnection.cc:691: new_sender = RtpSenderSignallingProxy::Create(
ah, I didn't think through my class-renaming suggestion.  Maybe that change
isn't worth it.

[nisse-webrtc, 2016-04-08 09:28:23]

On 2016/04/08 09:00:22, tommi-webrtc wrote:
> lgtm for the macro name rename, but I'm kind of changing my mind about the
class
> rename that I suggested :-/
> 
> https://codereview.webrtc.org/1871833002/diff/1/webrtc/api/peerconnection.cc
> File webrtc/api/peerconnection.cc (right):
> 
>
https://codereview.webrtc.org/1871833002/diff/1/webrtc/api/peerconnection.cc#...
> webrtc/api/peerconnection.cc:691: new_sender =
RtpSenderSignallingProxy::Create(
> ah, I didn't think through my class-renaming suggestion.  Maybe that change
> isn't worth it.

I think I'll revert that part then (but I'll wait for the other's opinions
before doing anything).

[perkj_webrtc, 2016-04-08 11:05:36]

https://codereview.webrtc.org/1871833002/diff/1/webrtc/api/proxy.h
File webrtc/api/proxy.h (right):

https://codereview.webrtc.org/1871833002/diff/1/webrtc/api/proxy.h#newcode308
webrtc/api/proxy.h:308: class c##SignallingProxy : public c##Interface {        
               \
- I dont like the Signalling part here either. Please revert that.

[nisse-webrtc, 2016-04-11 09:21:20]

https://codereview.webrtc.org/1871833002/diff/1/webrtc/api/proxy.h
File webrtc/api/proxy.h (right):

https://codereview.webrtc.org/1871833002/diff/1/webrtc/api/proxy.h#newcode308
webrtc/api/proxy.h:308: class c##SignallingProxy : public c##Interface {        
               \
On 2016/04/08 11:05:35, perkj_webrtc wrote:
> - I dont like the Signalling part here either. Please revert that.

Reverted now.

[perkj_webrtc, 2016-04-11 10:41:55]

looks good. Can you please add a test for the new methods as well.

https://codereview.webrtc.org/1871833002/diff/20001/webrtc/api/proxy.h
File webrtc/api/proxy.h (right):

https://codereview.webrtc.org/1871833002/diff/20001/webrtc/api/proxy.h#newcod...
webrtc/api/proxy.h:308: class c##Proxy : public c##Interface {                  
     \
nit: please fix the formatting.

https://codereview.webrtc.org/1871833002/diff/20001/webrtc/api/proxy.h#newcod...
webrtc/api/proxy.h:314: MethodCall0<c##Proxy, void> call(                       
 \
dito

https://codereview.webrtc.org/1871833002/diff/20001/webrtc/api/proxy_unittest.cc
File webrtc/api/proxy_unittest.cc (right):

https://codereview.webrtc.org/1871833002/diff/20001/webrtc/api/proxy_unittest...
webrtc/api/proxy_unittest.cc:74: class ProxyTest: public testing::Test {
Add test for the worker thread and the new worker proxy as well please.

[nisse-webrtc, 2016-04-11 13:39:37]

Description was changed from

==========
Rename BEGIN_PROXY_MAP --> BEGIN_SIGNALLING_PROXY_MAP.

And BEGIN_WORKER_PROXY_MAP --> BEGIN_PROXY_MAP.

This rename was suggested by Tommi, with the idea that a proxy
invoking methods on the worker thread should be the common case.
It's a followup to https://codereview.webrtc.org/1861633002/

The cl also renames the corresponding classes, FooProxy -->
FooSignallingProxy. I'm not entirely sure this is an improvement, it
changes all the ::Create calls, making lines longer for a quite small
gain in clarity.

BUG=webrtc:5426
==========

to

==========
Rename BEGIN_PROXY_MAP --> BEGIN_SIGNALLING_PROXY_MAP.

And BEGIN_WORKER_PROXY_MAP --> BEGIN_PROXY_MAP.

This rename was suggested by Tommi, with the idea that a proxy
invoking methods on the worker thread should be the common case.
It's a followup to https://codereview.webrtc.org/1861633002/

This cl also adds unittests for proxy calls to the
worker thread. 

BUG=webrtc:5426
==========

[nisse-webrtc, 2016-04-11 13:40:51]

Now including unittests.

[nisse-webrtc, 2016-04-12 06:01:23]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-12 06:01:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1871833002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1871833002/40001

[commit-bot: I haz the power, 2016-04-12 06:10:44]

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

[commit-bot: I haz the power, 2016-04-12 06:10:45]

Dry run: Try jobs failed on following builders:
  win_clang_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/2413)

[perkj_webrtc, 2016-04-12 07:12:14]

https://codereview.webrtc.org/1871833002/diff/40001/webrtc/api/proxy_unittest.cc
File webrtc/api/proxy_unittest.cc (right):

https://codereview.webrtc.org/1871833002/diff/40001/webrtc/api/proxy_unittest...
webrtc/api/proxy_unittest.cc:76: BEGIN_SIGNALLING_PROXY_MAP(Fake)
Sorry for late discovery. /s Signalling -> Signaling

https://codereview.webrtc.org/1871833002/diff/40001/webrtc/api/proxy_unittest...
webrtc/api/proxy_unittest.cc:102: rtc::scoped_refptr<FakeInterface>
fake_signalling_proxy_;
fake_signaling_....

https://codereview.webrtc.org/1871833002/diff/40001/webrtc/api/proxy_unittest...
webrtc/api/proxy_unittest.cc:163: class ProxyTest : public SignallingProxyTest {
nit: I prefer a totally separate test that does not inherit from
SignallingProxyTest just because its easier to read.

[nisse-webrtc, 2016-04-12 07:38:55]

https://codereview.webrtc.org/1871833002/diff/40001/webrtc/api/proxy_unittest.cc
File webrtc/api/proxy_unittest.cc (right):

https://codereview.webrtc.org/1871833002/diff/40001/webrtc/api/proxy_unittest...
webrtc/api/proxy_unittest.cc:76: BEGIN_SIGNALLING_PROXY_MAP(Fake)
On 2016/04/12 07:12:14, perkj_webrtc wrote:
> Sorry for late discovery. /s Signalling -> Signaling

Fixed. Still 17 occurences elsewhere in the code, mostly in comments, but also
rtc::HttpServer::Connection::signalling_.

https://codereview.webrtc.org/1871833002/diff/40001/webrtc/api/proxy_unittest...
webrtc/api/proxy_unittest.cc:163: class ProxyTest : public SignallingProxyTest {
On 2016/04/12 07:12:14, perkj_webrtc wrote:
> nit: I prefer a totally separate test that does not inherit from
> SignallingProxyTest just because its easier to read.

I think this is a simple enough case of inheritance. I was first going to use
the same class for all the tests. But decided to create a new derived class
mainly because I prefer 

  ProxyTest.Method1
  ProxySignalingTest.Method1

over

  ProxyTest.Method1
  ProxyTest.SignalingMethod1

in the test log. I'll eliminate the inheritance if you insist.

[pthatcher1, 2016-04-12 23:33:26]

Can we have it always be explicit, such as BEGIN_SIGNALING_PROXY_MAP *and*
BEGIN_WORKER_PROXY_MAP?

That way no one will possibly make a BEGIN_PROXY_MAP and get the wrong one (and
on any situation that we missed in this refactoring will not be broken).

By the way "signaling" is shorter than "signalling" and equally correct.

[perkj_webrtc, 2016-04-13 05:36:26]

On 2016/04/12 23:33:26, pthatcher1 wrote:
> Can we have it always be explicit, such as BEGIN_SIGNALING_PROXY_MAP *and*
> BEGIN_WORKER_PROXY_MAP?
> 
> That way no one will possibly make a BEGIN_PROXY_MAP and get the wrong one
(and
> on any situation that we missed in this refactoring will not be broken).
> 
> By the way "signaling" is shorter than "signalling" and equally correct.

lgtm

Peter- I think the idea was that BEGIN_PROXY_MAP can proxy to both the worker or
the signaling thread- depending on the need.  But the BEGIN_SIGNALING_PROXY_MAP
can only proxy to the signaling thread.  BEGIN_WORKER_PROXY_MAP feels strange if
it can proxy to both the signaling and the worker thread. 
(We can problably also remove BEGIN_SIGNALING_PROXY_MAP and force all proxies to
inject both the worker thread and the signaling thread).  I don't have a strong
opinion though.

[nisse-webrtc, 2016-04-13 07:09:47]

On 2016/04/12 23:33:26, pthatcher1 wrote:

> That way no one will possibly make a BEGIN_PROXY_MAP and get the wrong one
(and
> on any situation that we missed in this refactoring will not be broken).

I think I prefer the shorter name for the more general proxy. Mistakes should
hopefully be obvious when compiling the first call to the proxy constructor,
since BEGIN_PROXY_MAP produces a constructor which *requires* two Thread*
arguments.
  
> By the way "signaling" is shorter than "signalling" and equally correct.

See http://www.googlefight.com/signalling-vs-signaling.php. The shorter form
seems more popular.

[nisse-webrtc, 2016-04-13 07:13:04]

On 2016/04/13 05:36:26, perkj_webrtc wrote:

> (We can problably also remove BEGIN_SIGNALING_PROXY_MAP and force all proxies
to
> inject both the worker thread and the signaling thread).  I don't have a
strong
> opinion though.

I tried that earlier. One small but annoying problem with that approach is that
if no method using the worker thread is defined, we get compilation warnings
about the worker_thread_ instance variable being unused. Easy to silence (just
add a (void) worker_thread_ expression anywhere), but it doesn't feel right. So
I prefer a separate class without any knowledge of the worker thread.

[pthatcher1, 2016-04-13 17:33:41]

On 2016/04/13 05:36:26, perkj_webrtc wrote:
> On 2016/04/12 23:33:26, pthatcher1 wrote:
> > Can we have it always be explicit, such as BEGIN_SIGNALING_PROXY_MAP *and*
> > BEGIN_WORKER_PROXY_MAP?
> > 
> > That way no one will possibly make a BEGIN_PROXY_MAP and get the wrong one
> (and
> > on any situation that we missed in this refactoring will not be broken).
> > 
> > By the way "signaling" is shorter than "signalling" and equally correct.
> 
> lgtm
> 
> Peter- I think the idea was that BEGIN_PROXY_MAP can proxy to both the worker
or
> the signaling thread- depending on the need.  But the
BEGIN_SIGNALING_PROXY_MAP
> can only proxy to the signaling thread.  BEGIN_WORKER_PROXY_MAP feels strange
if
> it can proxy to both the signaling and the worker thread. 
> (We can problably also remove BEGIN_SIGNALING_PROXY_MAP and force all proxies
to
> inject both the worker thread and the signaling thread).  I don't have a
strong
> opinion though.

Ah, that makes sense to me now.  I didn't realize that the general one took two
threads as an argument.  I didn't do a full code review yet.  Sorry for the
premature comment.

[pthatcher1, 2016-04-13 17:35:03]

On 2016/04/13 07:09:47, nisse-webrtc wrote:
> On 2016/04/12 23:33:26, pthatcher1 wrote:
> 
> > That way no one will possibly make a BEGIN_PROXY_MAP and get the wrong one
> (and
> > on any situation that we missed in this refactoring will not be broken).
> 
> I think I prefer the shorter name for the more general proxy. Mistakes should
> hopefully be obvious when compiling the first call to the proxy constructor,
> since BEGIN_PROXY_MAP produces a constructor which *requires* two Thread*
> arguments.
>   
> > By the way "signaling" is shorter than "signalling" and equally correct.
> 
> See http://www.googlefight.com/signalling-vs-signaling.php. The shorter form
> seems more popular.

In our code base it's also more common:

https://code.google.com/p/chromium/codesearch#search/&q=signaling&sq=package:...

vs.  

https://code.google.com/p/chromium/codesearch#search/&q=signalling&sq=package...

(463 vs 101)

[tommi, 2016-04-14 13:49:02]

I think that we're spending too much time bikeshedding. I propose we land the cl
now and address further concerns in separate cls.

[pthatcher1, 2016-04-14 18:49:57]

lgtm

Sorry, I didn't mean to imply a minor name change was blocking my approval.  It
wasn't.  I hadn't LGed because I hadn't yet found the time to read through the
whole thing.  The naming this was just a small aside.

[nisse-webrtc, 2016-04-15 06:52:43]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-04-15 06:52:44]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1871833002/#ps60001 (title:
"Spelling fix: signalling --> signaling.")

[commit-bot: I haz the power, 2016-04-15 06:52:51]

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

[commit-bot: I haz the power, 2016-04-15 09:48:09]

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

[commit-bot: I haz the power, 2016-04-15 09:48:10]

Try jobs failed on following builders:
  win_drmemory_light on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...)

[nisse-webrtc, 2016-04-15 10:08:18]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2016-04-15 10:08:31]

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

[commit-bot: I haz the power, 2016-04-15 10:49:09]

Description was changed from

==========
Rename BEGIN_PROXY_MAP --> BEGIN_SIGNALLING_PROXY_MAP.

And BEGIN_WORKER_PROXY_MAP --> BEGIN_PROXY_MAP.

This rename was suggested by Tommi, with the idea that a proxy
invoking methods on the worker thread should be the common case.
It's a followup to https://codereview.webrtc.org/1861633002/

This cl also adds unittests for proxy calls to the
worker thread. 

BUG=webrtc:5426
==========

to

==========
Rename BEGIN_PROXY_MAP --> BEGIN_SIGNALLING_PROXY_MAP.

And BEGIN_WORKER_PROXY_MAP --> BEGIN_PROXY_MAP.

This rename was suggested by Tommi, with the idea that a proxy
invoking methods on the worker thread should be the common case.
It's a followup to https://codereview.webrtc.org/1861633002/

This cl also adds unittests for proxy calls to the
worker thread. 

BUG=webrtc:5426
==========

[commit-bot: I haz the power, 2016-04-15 10:49:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-15 10:49:20]

Description was changed from

==========
Rename BEGIN_PROXY_MAP --> BEGIN_SIGNALLING_PROXY_MAP.

And BEGIN_WORKER_PROXY_MAP --> BEGIN_PROXY_MAP.

This rename was suggested by Tommi, with the idea that a proxy
invoking methods on the worker thread should be the common case.
It's a followup to https://codereview.webrtc.org/1861633002/

This cl also adds unittests for proxy calls to the
worker thread. 

BUG=webrtc:5426
==========

to

==========
Rename BEGIN_PROXY_MAP --> BEGIN_SIGNALLING_PROXY_MAP.

And BEGIN_WORKER_PROXY_MAP --> BEGIN_PROXY_MAP.

This rename was suggested by Tommi, with the idea that a proxy
invoking methods on the worker thread should be the common case.
It's a followup to https://codereview.webrtc.org/1861633002/

This cl also adds unittests for proxy calls to the
worker thread.

BUG=webrtc:5426

Committed: https://crrev.com/72c8d2b708d442d16700229fe22918d187534430
Cr-Commit-Position: refs/heads/master@{#12374}
==========

[commit-bot: I haz the power, 2016-04-15 10:49:22]

Patchset 4 (id:??) landed as
https://crrev.com/72c8d2b708d442d16700229fe22918d187534430
Cr-Commit-Position: refs/heads/master@{#12374}