1174274 - make WrapRunnable* use nsRunnableMethodArgs<> as its underlying storage

Status: NEW

(Core :: WebRTC, defect)

Description

[Nathan Froyd [:froydnj]]

It's a little silly that we have multiple implementations of this functionality in the tree.  As a step towards merging them, runnable methods should both use nsRunnableMethodArgs as their underlying storage.

Doing this is straightforward, the only wrinkle I ran into was this error:

In file included from /home/froydnj/src/gecko-dev.git/media/mtransport/test/transport_unittests.cpp:24:
../../../dist/include/nsThreadUtils.h:613:31: error: non-const lvalue reference to type 'vector<[2 * ...]>' cannot bind to a
      temporary of type 'vector<[2 * ...]>'
    return ((*aObj).*aMethod)(args.PassAsParameter()...);
                              ^~~~~~~~~~~~~~~~~~~~~~
../../../dist/include/nsThreadUtils.h:656:45: note: in instantiation of function template specialization
      'detail::RunnableMethodCall<nsresult>::apply<mozilla::NrIceMediaStream, nsresult
      (mozilla::NrIceMediaStream::*)(std::vector<std::basic_string<char>, std::allocator<std::basic_string<char> > > &),
      StoreCopyPassByValue<std::vector<std::basic_string<char>, std::allocator<std::basic_string<char> > > > >' requested here
    return ::detail::RunnableMethodCall<R>::apply(o, m, m0);
                                            ^
/home/froydnj/src/gecko-dev.git/media/mtransport/runnable_utils.h:225:31: note: in instantiation of function template
      specialization 'nsRunnableMethodArguments<std::vector<std::basic_string<char>, std::allocator<std::basic_string<char> > >
      >::apply<nsresult, mozilla::NrIceMediaStream, nsresult (mozilla::NrIceMediaStream::*)(std::vector<std::basic_string<char>,
      std::allocator<std::basic_string<char> > > &)>' requested here
    *mReturn = mArgs.template apply<Ret>(detail::ToPointer<Class>::convert(mObj), mMethod);
                              ^
/home/froydnj/src/gecko-dev.git/media/mtransport/runnable_utils.h:220:3: note: in instantiation of member function
      'mozilla::runnable_args_memfn_ret<nsresult, mozilla::RefPtr<mozilla::NrIceMediaStream>, nsresult
      (mozilla::NrIceMediaStream::*)(std::vector<std::basic_string<char>, std::allocator<std::basic_string<char> > > &),
      std::vector<std::basic_string<char>, std::allocator<std::basic_string<char> > > >::Run' requested here
  runnable_args_memfn_ret(Ret* ret, Class obj, M method, Args... args)
  ^
/home/froydnj/src/gecko-dev.git/media/mtransport/runnable_utils.h:240:14: note: in instantiation of member function
      'mozilla::runnable_args_memfn_ret<nsresult, mozilla::RefPtr<mozilla::NrIceMediaStream>, nsresult
      (mozilla::NrIceMediaStream::*)(std::vector<std::basic_string<char>, std::allocator<std::basic_string<char> > > &),
      std::vector<std::basic_string<char>, std::allocator<std::basic_string<char> > > >::runnable_args_memfn_ret' requested here
  return new runnable_args_memfn_ret<R, Class, M, Args...>(ret, obj, method, args...);
             ^
/home/froydnj/src/gecko-dev.git/media/mtransport/test/transport_unittests.cpp:695:9: note: in instantiation of function template
      specialization 'mozilla::WrapRunnableRet<nsresult, mozilla::RefPtr<mozilla::NrIceMediaStream>, nsresult
      (mozilla::NrIceMediaStream::*)(std::vector<std::basic_string<char>, std::allocator<std::basic_string<char> > > &),
      std::vector<std::basic_string<char>, std::allocator<std::basic_string<char> > > >' requested here
        WrapRunnableRet(&res, peer_->streams_[i], &NrIceMediaStream::ParseAttributes,
        ^
1 error generated.

This can be fixed easily enough, but I'm unsure of why:

        WrapRunnableRet(&res, peer_->streams_[i], &NrIceMediaStream::ParseAttributes,
                        candidates_[streams_[i]->name()]), NS_DISPATCH_SYNC);

where candidates_[streams_[i]->name()] is a std::vector<std::string>& loses the reference qualifier during template instantation.  I think that is the intended behavior, since we're sending the attributes across threads, but it's still a bit puzzling.  We can make NrIceMediaStream::ParseAttributes take a |const std::vector<std::string>&|, which fixes the error, but it'd be nice to know why.

There's also a bit of an impedence mismatch: WrapRunnable* just expects implicit template instantiation to work, while nsRunnableMethodArgs works off of explicitly-specified template arguments for greater control.  I could see this mismatch causing problems...

Comment 1

[Eric Rescorla (:ekr)]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #0)
> It's a little silly that we have multiple implementations of this
> functionality in the tree.  As a step towards merging them, runnable methods
> should both use nsRunnableMethodArgs as their underlying storage.
> 
> Doing this is straightforward, the only wrinkle I ran into was this error:
> 
> In file included from
> /home/froydnj/src/gecko-dev.git/media/mtransport/test/transport_unittests.
> cpp:24:
> ../../../dist/include/nsThreadUtils.h:613:31: error: non-const lvalue
> reference to type 'vector<[2 * ...]>' cannot bind to a
>       temporary of type 'vector<[2 * ...]>'
>     return ((*aObj).*aMethod)(args.PassAsParameter()...);
>                               ^~~~~~~~~~~~~~~~~~~~~~
> ../../../dist/include/nsThreadUtils.h:656:45: note: in instantiation of
> function template specialization
>      
> 'detail::RunnableMethodCall<nsresult>::apply<mozilla::NrIceMediaStream,
> nsresult
>       (mozilla::NrIceMediaStream::*)(std::vector<std::basic_string<char>,
> std::allocator<std::basic_string<char> > > &),
>       StoreCopyPassByValue<std::vector<std::basic_string<char>,
> std::allocator<std::basic_string<char> > > > >' requested here
>     return ::detail::RunnableMethodCall<R>::apply(o, m, m0);
>                                             ^
> /home/froydnj/src/gecko-dev.git/media/mtransport/runnable_utils.h:225:31:
> note: in instantiation of function template
>       specialization
> 'nsRunnableMethodArguments<std::vector<std::basic_string<char>,
> std::allocator<std::basic_string<char> > >
>       >::apply<nsresult, mozilla::NrIceMediaStream, nsresult
> (mozilla::NrIceMediaStream::*)(std::vector<std::basic_string<char>,
>       std::allocator<std::basic_string<char> > > &)>' requested here
>     *mReturn = mArgs.template
> apply<Ret>(detail::ToPointer<Class>::convert(mObj), mMethod);
>                               ^
> /home/froydnj/src/gecko-dev.git/media/mtransport/runnable_utils.h:220:3:
> note: in instantiation of member function
>       'mozilla::runnable_args_memfn_ret<nsresult,
> mozilla::RefPtr<mozilla::NrIceMediaStream>, nsresult
>       (mozilla::NrIceMediaStream::*)(std::vector<std::basic_string<char>,
> std::allocator<std::basic_string<char> > > &),
>       std::vector<std::basic_string<char>,
> std::allocator<std::basic_string<char> > > >::Run' requested here
>   runnable_args_memfn_ret(Ret* ret, Class obj, M method, Args... args)
>   ^
> /home/froydnj/src/gecko-dev.git/media/mtransport/runnable_utils.h:240:14:
> note: in instantiation of member function
>       'mozilla::runnable_args_memfn_ret<nsresult,
> mozilla::RefPtr<mozilla::NrIceMediaStream>, nsresult
>       (mozilla::NrIceMediaStream::*)(std::vector<std::basic_string<char>,
> std::allocator<std::basic_string<char> > > &),
>       std::vector<std::basic_string<char>,
> std::allocator<std::basic_string<char> > > >::runnable_args_memfn_ret'
> requested here
>   return new runnable_args_memfn_ret<R, Class, M, Args...>(ret, obj, method,
> args...);
>              ^
> /home/froydnj/src/gecko-dev.git/media/mtransport/test/transport_unittests.
> cpp:695:9: note: in instantiation of function template
>       specialization 'mozilla::WrapRunnableRet<nsresult,
> mozilla::RefPtr<mozilla::NrIceMediaStream>, nsresult
>       (mozilla::NrIceMediaStream::*)(std::vector<std::basic_string<char>,
> std::allocator<std::basic_string<char> > > &),
>       std::vector<std::basic_string<char>,
> std::allocator<std::basic_string<char> > > >' requested here
>         WrapRunnableRet(&res, peer_->streams_[i],
> &NrIceMediaStream::ParseAttributes,
>         ^
> 1 error generated.
> 
> This can be fixed easily enough, but I'm unsure of why:
> 
>         WrapRunnableRet(&res, peer_->streams_[i],
> &NrIceMediaStream::ParseAttributes,
>                         candidates_[streams_[i]->name()]), NS_DISPATCH_SYNC);
> 
> where candidates_[streams_[i]->name()] is a std::vector<std::string>& loses
> the reference qualifier during template instantation.  I think that is the
> intended behavior, since we're sending the attributes across threads, but
> it's still a bit puzzling.  We can make NrIceMediaStream::ParseAttributes
> take a |const std::vector<std::string>&|, which fixes the error, but it'd be
> nice to know why.
> 
> There's also a bit of an impedence mismatch: WrapRunnable* just expects
> implicit template instantiation to work, while nsRunnableMethodArgs works
> off of explicitly-specified template arguments for greater control.  I could
> see this mismatch causing problems...

I think this is an important feature of WrapRunnable, and one that is more
in line with how std::bind() works when we eventually have it (though obviously
std::bind() is much better).

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Eric Rescorla (:ekr) from comment #1)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #0)
> > There's also a bit of an impedence mismatch: WrapRunnable* just expects
> > implicit template instantiation to work, while nsRunnableMethodArgs works
> > off of explicitly-specified template arguments for greater control.  I could
> > see this mismatch causing problems...
> 
> I think this is an important feature of WrapRunnable, and one that is more
> in line with how std::bind() works when we eventually have it (though
> obviously
> std::bind() is much better).

For avoidance of doubt, you think the implicit instantiation style of WrapRunnable is preferable?

Comment 3

[Eric Rescorla (:ekr)]

Yes, I think it's better.