Update Bind to match its comments and always capture by value.

Update Bind to match its comments and always capture by value. Also update the generated count to 9 args.

The existing comment is wrong, and the test even ensures it: Bind will capture reference values by reference. That makes it hard to use with AsyncInvoker, because you can't safely Bind to a function that takes (const) reference params.

The new version of this code strips references in the bound object, so it captures by value, but can bind against functions that take const references, they'll just be references to the copy.

As the class comment implies, actual by-reference args should be passed as pointers or things that safely share (e.g. scoped_refptr) and not references directly. A new test case ensures the pointer reference works. The new code will also give a compiler error if you try to bind
to a non-const reference.

BUG=

Committed: https://crrev.com/5d9b92b53daf4db78fd090be4e210e07f786120d
Cr-Commit-Position: refs/heads/master@{#10397}

[noahric, 2015-08-19 01:05:10]

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

[noahric, 2015-08-19 01:05:10]

Hey Peter, let me know what you think about this. I was trying to write some
stuff that used AsyncInvoker, and I realized (1) that the implementation of Bind
differed from the class comment, and (2) that using it's capture-by-ref behavior
makes it unsafe to use with AsyncInvoker. I've updated it to always capture by
value. I also bumped the pump arg count to 9, since I have some larger methods
to Bind :)

[noahric, 2015-08-19 02:46:07]

Also, the linux failures appear to be happening in ToT, so I'm not sure what to
do there.

[pthatcher1, 2015-08-20 23:53:03]

This makes sense to me, and the code looks good.

But I'm a little worried that there are uses of Bind out there that now assume
the old behavior (to not copy the value).  I see about 89 uses in go/crcs.  How
could we be sure that they are all safe switching?

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

[pthatcher1, 2015-08-20 23:54:24]

They are almost all in Channel/ChannelManager, with a little in
Session/Transport code.

I don't see how it's at all related to the build failures you saw.  There aren't
any uses that I can see in that area of the code.

[noahric, 2015-10-22 00:12:32]

noahric@chromium.org changed reviewers:
+ glaznev@webrtc.org, magjed@webrtc.org

[noahric, 2015-10-22 00:12:33]

message: +glaznev, since I updated some android files and +magjed because I
merged it with the scoped ref removal struct.

I stepped through all the references to rtc::Bind:
https://code.google.com/p/chromium/codesearch#search/&q=rtc::Bind&sq=package:...

Here's what I did/learned:
1) webrtcvideocapturer has a function that was already shallow copying in the
sig, so I updated the sig to just take a const webrtc::VideoFrame&. That removes
a shallow webrtc::VideoFrame copy in the Chrome path (which was doing a by-val
capture originally), and leaves the shallow copy in the wrong-thread path.
Either doesn't matter, because the frame's buffer is pulled out of the frame
anyways, though I could also update this for the method to take a pointer and
not copy it in either case. Let me know which you prefer.
2) androidmedia(encoder|decoder)_jni.cc were passing frames by reference. Those
would also be fine to turn into copies and leave the files unchanged (the frames
themselves have a default copy constructor and just shallow copy pointers), but
I replaced them with passing explicit pointers for clarity that it is an
expected "reference" capture, because it's frowned upon to be creating implicit
copies of types with raw pointers. I could put these back to refs (with the
implicit copy) if you want.
3) androivideocapturer_jni.cc is also now doing an extra shallow copy of a
NativeTextureHandleImpl&, but it was already doing an implicit one when creating
the AndroidTextureBuffer, so I left this alone. It's also slightly confusing
because it creates the object for the param inline, and taking the address of
that object looks scary, even though it is semantically equivalent.
4) some other places will now pass copies of structs, which means they'll be
less efficient, but still correct. If we're worried about performance, I can
update them to pass pointers. An example of this is
ChannelManager::CreateVideoChannel, which takes a set of VideoOptions and now
copies them in the thread hop.
5) The remaining places that wanted to pass non-const "reference"s already pass
pointers or pointers-to-pointers.

From #1 and #2, though, it also seems like someone should consider adding
DISALLOW_COPY_AND_ASSIGN to various frame types. It would have forced me to make
the usage more explicit, but these types contain raw pointers to buffers and
generally shouldn't be copied.

[magjed_webrtc, 2015-10-22 23:54:31]

I like this change because it makes rtc::Bind() more similar to std::Bind().
However, I don't like the change from 'const EncodedImage& inputImage' to 'const
EncodedImage* inputImage' just for avoiding capturing by-value. How difficult
would it be to implement rtc::Ref() similar to std::Ref()?

https://codereview.webrtc.org/1291543006/diff/100001/webrtc/base/bind.h.pump
File webrtc/base/bind.h.pump (right):

https://codereview.webrtc.org/1291543006/diff/100001/webrtc/base/bind.h.pump#...
webrtc/base/bind.h.pump:65: #include <type_traits>
I'm surprised this compiles on all webrtc bots, but I still think all C++11
library features are banned, no? Are you sure it will be possible to roll this
change into Chromium?
You could also implement this yourself with just one more template
specialization:
template <class T&> struct RemoveAllRef { typedef T type; };

[noahric, 2015-10-23 00:34:04]

On 2015/10/22 23:54:31, magjed_webrtc wrote:
> I like this change because it makes rtc::Bind() more similar to std::Bind().
> However, I don't like the change from 'const EncodedImage& inputImage' to
'const
> EncodedImage* inputImage' just for avoiding capturing by-value. How difficult
> would it be to implement rtc::Ref() similar to std::Ref()?
> 
> https://codereview.webrtc.org/1291543006/diff/100001/webrtc/base/bind.h.pump
> File webrtc/base/bind.h.pump (right):
> 
>
https://codereview.webrtc.org/1291543006/diff/100001/webrtc/base/bind.h.pump#...
> webrtc/base/bind.h.pump:65: #include <type_traits>
> I'm surprised this compiles on all webrtc bots, but I still think all C++11
> library features are banned, no? Are you sure it will be possible to roll this
> change into Chromium?
> You could also implement this yourself with just one more template
> specialization:
> template <class T&> struct RemoveAllRef { typedef T type; };

Dunno about the std::Ref, though there's also a nice-to-have about correctly
capturing && types (I ran into that as well).

I could put the "const EncodedImage& inputImage" back, and have an added shallow
copy. I just didn't like it because it's non-obvious, and it probably violates
the style guide guide (EncodedImage should have DISALLOW_COPY_AND_ASSIGN because
of the bare pointers). So I guess my preference is to submit this as is and
leave the std::Ref as an exercise for someone else :)

Let me know what you think. Thanks!

[noahric, 2015-10-23 00:34:27]

Thanks!

https://codereview.webrtc.org/1291543006/diff/100001/webrtc/base/bind.h.pump
File webrtc/base/bind.h.pump (right):

https://codereview.webrtc.org/1291543006/diff/100001/webrtc/base/bind.h.pump#...
webrtc/base/bind.h.pump:65: #include <type_traits>
Ah, yeah, good point. And I realized I was being dumb and don't actually need a
specialization anymore for scoped_refptr specifically. For some reason, in my
head, I thought it was removing references inside the scoped_ptr, which doesn't
make sense.

Though this was also removing const-ness. Is that still desirable? It shouldn't
actually make a difference if the field is const or not, so I updated the
bind_unittest's static_assert to expect ::type's const-ness to match the
inferred template type.

Let me know if you want me to put it back and add specializations for also
stripping out const.

Thanks!

[magjed_webrtc, 2015-10-23 05:17:01]

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

[magjed_webrtc, 2015-10-23 05:17:01]

+tommi - I remember we had a short discussion if we should make rtc::Bind()
capture all refs by value, and came to the conclusion to only fix
scoped_refptrs. Was it only out of laziness and because we can hopefully use
std::bind in ~1 year, or some other reason?

noahric - Just to make sure - have you searched for calls to rtc::Bind() inside
the rtc namespace without the rtc:: prefix?

https://codereview.webrtc.org/1291543006/diff/100001/webrtc/base/bind.h.pump
File webrtc/base/bind.h.pump (right):

https://codereview.webrtc.org/1291543006/diff/100001/webrtc/base/bind.h.pump#...
webrtc/base/bind.h.pump:65: #include <type_traits>
On 2015/10/23 00:34:27, noahric wrote:
> Ah, yeah, good point. And I realized I was being dumb and don't actually need
a
> specialization anymore for scoped_refptr specifically. For some reason, in my
> head, I thought it was removing references inside the scoped_ptr, which
doesn't
> make sense.
> 
> Though this was also removing const-ness. Is that still desirable? It
shouldn't
> actually make a difference if the field is const or not, so I updated the
> bind_unittest's static_assert to expect ::type's const-ness to match the
> inferred template type.
> 
> Let me know if you want me to put it back and add specializations for also
> stripping out const.
> 
> Thanks!

I don't remember why I stripped const-ness, but I think I got some compiler
error because AddRef() and Release() used to be non-const methods in WebRTC, and
this was a workaround.

If it compiles without stripping const-ness, it's not desirable :)

https://codereview.webrtc.org/1291543006/diff/120001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right):

https://codereview.webrtc.org/1291543006/diff/120001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:122: const
webrtc::VideoFrame* input_image,
Leave this unchanged - implicit copies of webrtc::VideoFrame is fine and we do
it elsewhere.

https://codereview.webrtc.org/1291543006/diff/120001/webrtc/base/bind.h.pump
File webrtc/base/bind.h.pump (right):

https://codereview.webrtc.org/1291543006/diff/120001/webrtc/base/bind.h.pump#...
webrtc/base/bind.h.pump:127: // RemoveAllRef implementations are equivalent to
std::remove_reference make
Missing some word or punctuation between 'std::remove_reference' and 'make'.

https://codereview.webrtc.org/1291543006/diff/120001/webrtc/base/bind.h.pump#...
webrtc/base/bind.h.pump:130: struct RemoveAllRef {
Can you rename this to rtc::remove_reference and move to
webrtc/base/template_util.h instead? We have e.g. rtc::is_same and other std
duplicates in that file.

https://codereview.webrtc.org/1291543006/diff/120001/webrtc/base/bind_unittes...
File webrtc/base/bind_unittest.cc (right):

https://codereview.webrtc.org/1291543006/diff/120001/webrtc/base/bind_unittes...
webrtc/base/bind_unittest.cc:233: // Bind() should make a copy of |x|, i.e. the
pointers should be the same.
s/i.e. the pointers should be the same/i.e. the pointers should not be the
same/g.

[noahric, 2015-10-23 06:02:15]

noahric@chromium.org changed reviewers:
- glaznev@webrtc.org

[noahric, 2015-10-23 06:02:16]

Thanks!

Yup, I did check webrtc/base. asyncinvoker has a few uses and a unit test, I
believe, but they were all safe (pointers or primitives).

Let me know what you guys decide. I'll say at least that this change lets me
remove ~300 lines of code from an implementation that had a bunch of duplication
because it couldn't use bind.h, so I'd be really happy to have this in :)

Also: it's the 2 year + 4 day anniversary of the initial checkin of this code,
which has a code review bearing this comment from me:
"Explicitly state that it captures arguments by value and not by reference,
since this may have practical implications."

Ryan intended it to do that, but I guess I could have actually reviewed it a bit
closer :)

https://codereview.webrtc.org/1291543006/diff/100001/webrtc/base/bind.h.pump
File webrtc/base/bind.h.pump (right):

https://codereview.webrtc.org/1291543006/diff/100001/webrtc/base/bind.h.pump#...
webrtc/base/bind.h.pump:65: #include <type_traits>
On 2015/10/23 05:17:01, magjed_webrtc wrote:
> On 2015/10/23 00:34:27, noahric wrote:
> > Ah, yeah, good point. And I realized I was being dumb and don't actually
need
> a
> > specialization anymore for scoped_refptr specifically. For some reason, in
my
> > head, I thought it was removing references inside the scoped_ptr, which
> doesn't
> > make sense.
> > 
> > Though this was also removing const-ness. Is that still desirable? It
> shouldn't
> > actually make a difference if the field is const or not, so I updated the
> > bind_unittest's static_assert to expect ::type's const-ness to match the
> > inferred template type.
> > 
> > Let me know if you want me to put it back and add specializations for also
> > stripping out const.
> > 
> > Thanks!
> 
> I don't remember why I stripped const-ness, but I think I got some compiler
> error because AddRef() and Release() used to be non-const methods in WebRTC,
and
> this was a workaround.
> 
> If it compiles without stripping const-ness, it's not desirable :)

Gotcha. Seems to be fine now that they are const and the ref field is mutable.

https://codereview.webrtc.org/1291543006/diff/120001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right):

https://codereview.webrtc.org/1291543006/diff/120001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:122: const
webrtc::VideoFrame* input_image,
On 2015/10/23 05:17:01, magjed_webrtc wrote:
> Leave this unchanged - implicit copies of webrtc::VideoFrame is fine and we do
> it elsewhere.

SGTM. Done.

https://codereview.webrtc.org/1291543006/diff/120001/webrtc/base/bind.h.pump
File webrtc/base/bind.h.pump (right):

https://codereview.webrtc.org/1291543006/diff/120001/webrtc/base/bind.h.pump#...
webrtc/base/bind.h.pump:127: // RemoveAllRef implementations are equivalent to
std::remove_reference make
On 2015/10/23 05:17:01, magjed_webrtc wrote:
> Missing some word or punctuation between 'std::remove_reference' and 'make'.

Done (no more comment in template_util.h).

https://codereview.webrtc.org/1291543006/diff/120001/webrtc/base/bind.h.pump#...
webrtc/base/bind.h.pump:130: struct RemoveAllRef {
On 2015/10/23 05:17:01, magjed_webrtc wrote:
> Can you rename this to rtc::remove_reference and move to
> webrtc/base/template_util.h instead? We have e.g. rtc::is_same and other std
> duplicates in that file.

Done.

https://codereview.webrtc.org/1291543006/diff/120001/webrtc/base/bind_unittes...
File webrtc/base/bind_unittest.cc (right):

https://codereview.webrtc.org/1291543006/diff/120001/webrtc/base/bind_unittes...
webrtc/base/bind_unittest.cc:233: // Bind() should make a copy of |x|, i.e. the
pointers should be the same.
On 2015/10/23 05:17:01, magjed_webrtc wrote:
> s/i.e. the pointers should be the same/i.e. the pointers should not be the
> same/g.

Done.

https://codereview.webrtc.org/1291543006/diff/140001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvideocapturer.cc (right):

https://codereview.webrtc.org/1291543006/diff/140001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideocapturer.cc:386: start_thread_,
FYI, this is from git cl format.

[magjed_webrtc, 2015-10-23 14:33:08]

lgtm

[tommi, 2015-10-23 16:11:37]

On 2015/10/23 05:17:01, magjed_webrtc wrote:
> +tommi - I remember we had a short discussion if we should make rtc::Bind()
> capture all refs by value, and came to the conclusion to only fix
> scoped_refptrs. Was it only out of laziness and because we can hopefully use
> std::bind in ~1 year, or some other reason?
> 

I don't remember the specifics but we wanted the change to be conservative and
address the specific problem we had at that time.  Capture refs by value could
have had unwanted side effects and for some reason I feel like we found at least
one place where that could have been a bad thing but I don't remember what it
was.

[tommi, 2015-10-24 17:23:07]

The CQ bit was checked by tommi@webrtc.org

[tommi, 2015-10-24 17:23:08]

lgtm

[commit-bot: I haz the power, 2015-10-24 17:23:18]

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

[commit-bot: I haz the power, 2015-10-24 18:14:51]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-10-24 18:14:58]

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