Adding some features to proxy.h, and restructuring the macros.

Adding some features to proxy.h, and restructuring the macros.

New features are:
- Invoke a destructor on the worker thread.
- Make proxy wrapper for a non-refcounted object.
- Ability to use unique_ptrs (as arguments or return values).

These features are needed by this CL:
https://codereview.webrtc.org/2632613002/

BUG=None

Review-Url: https://codereview.webrtc.org/2628343003
Cr-Commit-Position: refs/heads/master@{#16151}
Committed: https://chromium.googlesource.com/external/webrtc/+/d99a200fad07a23e4e31daab50b7b55bf364ad06

[pthatcher1, 2017-01-17 19:43:59]

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

[pthatcher1, 2017-01-17 19:44:00]

https://codereview.webrtc.org/2628343003/diff/1/webrtc/api/peerconnectionfact...
File webrtc/api/peerconnectionfactoryproxy.h (right):

https://codereview.webrtc.org/2628343003/diff/1/webrtc/api/peerconnectionfact...
webrtc/api/peerconnectionfactoryproxy.h:103: END_PROXY_MAP()
It seems like the end should match the begin.  So would END_SIGNALING_PROXY_MAP
be a better name, even if it's just an alias of END_PROXY_MAP?

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

https://codereview.webrtc.org/2628343003/diff/1/webrtc/api/proxy.h#newcode31
webrtc/api/proxy.h:31: //   PROXY_SIGNALING_THREAD_DESTRUCTOR()
What happens if one doesn't include this line?  Would it be too easy to forget
and cause something bad to happen?  Would it make sense to keep the default to
be as if the line were there?

https://codereview.webrtc.org/2628343003/diff/1/webrtc/api/proxy.h#newcode48
webrtc/api/proxy.h:48: // The variant defined with
BEGIN_NON_REFCOUNTED_PROXY_MAP does not use
Would BEGIN_OWNED_PROXY_MAP be a better name?

https://codereview.webrtc.org/2628343003/diff/1/webrtc/api/proxy_unittest.cc
File webrtc/api/proxy_unittest.cc (right):

https://codereview.webrtc.org/2628343003/diff/1/webrtc/api/proxy_unittest.cc#...
webrtc/api/proxy_unittest.cc:89: class SignalingProxyTest : public testing::Test
{
Is there a test of the new functionality you are adding?

[Taylor Brandstetter, 2017-01-18 01:17:14]

https://codereview.webrtc.org/2628343003/diff/1/webrtc/api/peerconnectionfact...
File webrtc/api/peerconnectionfactoryproxy.h (right):

https://codereview.webrtc.org/2628343003/diff/1/webrtc/api/peerconnectionfact...
webrtc/api/peerconnectionfactoryproxy.h:103: END_PROXY_MAP()
On 2017/01/17 19:44:00, pthatcher1 wrote:
> It seems like the end should match the begin.  So would
END_SIGNALING_PROXY_MAP
> be a better name, even if it's just an alias of END_PROXY_MAP?

I don't see much benefit to that. Plus, code would then compile if you match
"BEGIN_SIGNALING_PROXY_MAP" with "END_WORKER_PROXY_MAP".

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

https://codereview.webrtc.org/2628343003/diff/1/webrtc/api/proxy.h#newcode31
webrtc/api/proxy.h:31: //   PROXY_SIGNALING_THREAD_DESTRUCTOR()
On 2017/01/17 19:44:00, pthatcher1 wrote:
> What happens if one doesn't include this line?  Would it be too easy to forget
> and cause something bad to happen?  Would it make sense to keep the default to
> be as if the line were there?

You'd get a compile error: "use of undeclared identifier 'destructor_thread'"

https://codereview.webrtc.org/2628343003/diff/1/webrtc/api/proxy.h#newcode48
webrtc/api/proxy.h:48: // The variant defined with
BEGIN_NON_REFCOUNTED_PROXY_MAP does not use
On 2017/01/17 19:44:00, pthatcher1 wrote:
> Would BEGIN_OWNED_PROXY_MAP be a better name?

Done.

https://codereview.webrtc.org/2628343003/diff/1/webrtc/api/proxy_unittest.cc
File webrtc/api/proxy_unittest.cc (right):

https://codereview.webrtc.org/2628343003/diff/1/webrtc/api/proxy_unittest.cc#...
webrtc/api/proxy_unittest.cc:89: class SignalingProxyTest : public testing::Test
{
On 2017/01/17 19:44:00, pthatcher1 wrote:
> Is there a test of the new functionality you are adding?

Done. I only added a smoke test for the "owned proxy", since it can be assumed
that if one type of method works the others will work the same way.

[pthatcher1, 2017-01-18 01:19:38]

LGTM

This looks good to me, but the macro fun could sure you another set of eyes.  I
recommend finding another reviewer.

[Taylor Brandstetter, 2017-01-18 02:25:55]

I also added support for unique_ptrs (I realized I needed this too, plus it was
a TODO elsewhere)

[Taylor Brandstetter, 2017-01-18 03:46:07]

Description was changed from

==========
Adding some features to proxy.h, and restructuring the macros.

New features are:
- Invoke a destructor on the worker thread.
- Make proxy wrapper for a non-refcounted object.

These features are needed by this CL:
https://codereview.webrtc.org/2632613002/

BUG=None
==========

to

==========
Adding some features to proxy.h, and restructuring the macros.

New features are:
- Invoke a destructor on the worker thread.
- Make proxy wrapper for a non-refcounted object.
- Ability to use unique_ptrs (as arguments or return values).

These features are needed by this CL:
https://codereview.webrtc.org/2632613002/

BUG=None
==========

[Taylor Brandstetter, 2017-01-18 03:48:39]

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

[Taylor Brandstetter, 2017-01-18 03:48:39]

tommi: Would you like to take a look as well?

[tommi, 2017-01-18 12:18:02]

nice improvement. lgtm.  Thanks for taking care of the todo as well :)

https://codereview.webrtc.org/2628343003/diff/80001/webrtc/api/peerconnection...
File webrtc/api/peerconnectionfactoryproxy.h (right):

https://codereview.webrtc.org/2628343003/diff/80001/webrtc/api/peerconnection...
webrtc/api/peerconnectionfactoryproxy.h:27:
PROXY_METHOD5(rtc::scoped_refptr<PeerConnectionInterface>,
nice to get rid of the other hack

[tommi, 2017-01-18 12:18:22]

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

[commit-bot: I haz the power, 2017-01-18 12:18:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-18 13:03:46]

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

[commit-bot: I haz the power, 2017-01-18 13:03:47]

Dry run: This issue passed the CQ dry run.

[Taylor Brandstetter, 2017-01-18 16:52:45]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2017-01-18 16:52:45]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2628343003/#ps80001 (title:
"Fixing "depends on patchset..."")

[commit-bot: I haz the power, 2017-01-18 16:52:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-18 16:55:23]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484758365243260,
"parent_rev": "c8ee882753f5240018445a1ce5761fd1c3196b4e", "commit_rev":
"d99a200fad07a23e4e31daab50b7b55bf364ad06"}

[commit-bot: I haz the power, 2017-01-18 16:55:26]

Description was changed from

==========
Adding some features to proxy.h, and restructuring the macros.

New features are:
- Invoke a destructor on the worker thread.
- Make proxy wrapper for a non-refcounted object.
- Ability to use unique_ptrs (as arguments or return values).

These features are needed by this CL:
https://codereview.webrtc.org/2632613002/

BUG=None
==========

to

==========
Adding some features to proxy.h, and restructuring the macros.

New features are:
- Invoke a destructor on the worker thread.
- Make proxy wrapper for a non-refcounted object.
- Ability to use unique_ptrs (as arguments or return values).

These features are needed by this CL:
https://codereview.webrtc.org/2632613002/

BUG=None

Review-Url: https://codereview.webrtc.org/2628343003
Cr-Commit-Position: refs/heads/master@{#16151}
Committed:
https://chromium.googlesource.com/external/webrtc/+/d99a200fad07a23e4e31daab5...
==========

[commit-bot: I haz the power, 2017-01-18 16:55:27]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/d99a200fad07a23e4e31daab5...