Adding RTCErrorOr class to be used by ORTC APIs.

Adding RTCErrorOr class to be used by ORTC APIs.

This utility class can be used to represent either an error or a
successful return value. Follows the pattern of StatusOr in the protobuf
library.

This will be used by ORTC factory methods; for instance, CreateRtpSender
will either return an RtpSender or an error if the parameters are
invalid or some other failure occurs.

This CL also moves RTCError classes to a separate file, and adds tests
that were missing before.

BUG=webrtc:7013
Review-Url: https://codereview.webrtc.org/2692723002
Cr-Commit-Position: refs/heads/master@{#16659}
Committed: https://chromium.googlesource.com/external/webrtc/+/6038e97e048bebf763f7149ce0d84d0798c65f8b

[Taylor Brandstetter, 2017-02-12 02:36:37]

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

[Taylor Brandstetter, 2017-02-12 02:36:38]

Tommi: Peter thought it would be a good idea for you to look at this.

Here's an example of how it will be used:
https://codereview.webrtc.org/2675173003/diff/230001/webrtc/api/ortc/ortcfact...

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h
File webrtc/api/rtcerror.h (right):

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode94
webrtc/api/rtcerror.h:94: static const RTCError& OK();
This is the only thing other than RTCErrorOr and CreateAndLogError that's new.

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode123
webrtc/api/rtcerror.h:123: rtc::LoggingSeverity severity);
Can be used like:

return CreateAndLogError(RTCErrorType::INVALID_PARAMETER, "Foo must be between 1
and 10.");

[tommi, 2017-02-12 12:11:22]

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc
File webrtc/api/rtcerror.cc (right):

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc#newcode41
webrtc/api/rtcerror.cc:41: static RTCError ok;
have you checked if this is thread safe? I don't think it is on Windows at
least.
I think there is some singleton code elsewhere that you could take a look at. 
Alterternatively, if it is guaranteed that this method is always called on the
same thread or if it is always called once on a known thread before any other
thread can call it, it can be considered thread safe and it would be good to
document that here.

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h
File webrtc/api/rtcerror.h (right):

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode87
webrtc/api/rtcerror.h:87: RTCError(RTCErrorType type, std::string message)
is |message| expected to be std::move()d?  also, use std::move in the
initializer list?

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode103
webrtc/api/rtcerror.h:103: std::string message() const { return message_; }
return const&?

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode117
webrtc/api/rtcerror.h:117: std::ostream& operator<<(std::ostream& stream,
RTCErrorType error);
Streams are costly in terms of code size and cpu usage. Is this only for for
debug logging and test diagnostics?  If not, I prefer we figure out a way to do
this that does not involve iostream. We don't need all the bloat that comes with
it such as localization.

Some more information here:
https://google.github.io/styleguide/cppguide.html#Streams

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode184
webrtc/api/rtcerror.h:184: RTCErrorOr(const RTCError& error) : error_(error) {
RTC_DCHECK(!error.ok()); }
could this be |RTCError&& error| and move ownership to RTCErrorOr?  Since
RTCError is a complex object, it would be good if the API is designed to avoid
creating copies whenever we use RTCErrorOr as the documentation suggests.

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode195
webrtc/api/rtcerror.h:195: // Copy constructor.
do we need one? (comment?)
if we could instead live with being only movable, that would be nice.

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode229
webrtc/api/rtcerror.h:229: T& value() {
can you add a comment that explains when this is needed?

[Taylor Brandstetter, 2017-02-12 16:19:23]

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc
File webrtc/api/rtcerror.cc (right):

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc#newcode41
webrtc/api/rtcerror.cc:41: static RTCError ok;
On 2017/02/12 12:11:21, tommi (webrtc) wrote:
> have you checked if this is thread safe? I don't think it is on Windows at
> least.
> I think there is some singleton code elsewhere that you could take a look at. 
> Alterternatively, if it is guaranteed that this method is always called on the
> same thread or if it is always called once on a known thread before any other
> thread can call it, it can be considered thread safe and it would be good to
> document that here.

It's thread-safe according to the standard, though Microsoft didn't implement
this until VS2015. Does webrtc have a minimum version of VS it targets? chromium
builds with VS2015.

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h
File webrtc/api/rtcerror.h (right):

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode87
webrtc/api/rtcerror.h:87: RTCError(RTCErrorType type, std::string message)
On 2017/02/12 12:11:22, tommi (webrtc) wrote:
> is |message| expected to be std::move()d?  also, use std::move in the
> initializer list?

Just meant to use a const ref, fixed.

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode103
webrtc/api/rtcerror.h:103: std::string message() const { return message_; }
On 2017/02/12 12:11:21, tommi (webrtc) wrote:
> return const&?

Done.

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode117
webrtc/api/rtcerror.h:117: std::ostream& operator<<(std::ostream& stream,
RTCErrorType error);
On 2017/02/12 12:11:22, tommi (webrtc) wrote:
> Streams are costly in terms of code size and cpu usage. Is this only for for
> debug logging and test diagnostics?  If not, I prefer we figure out a way to
do
> this that does not involve iostream. We don't need all the bloat that comes
with
> it such as localization.
> 
> Some more information here:
> https://google.github.io/styleguide/cppguide.html#Streams

Yes, it's only intended for logging. I'll add a comment.

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode184
webrtc/api/rtcerror.h:184: RTCErrorOr(const RTCError& error) : error_(error) {
RTC_DCHECK(!error.ok()); }
On 2017/02/12 12:11:21, tommi (webrtc) wrote:
> could this be |RTCError&& error| and move ownership to RTCErrorOr?  Since
> RTCError is a complex object, it would be good if the API is designed to avoid
> creating copies whenever we use RTCErrorOr as the documentation suggests.

Done.

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode195
webrtc/api/rtcerror.h:195: // Copy constructor.
On 2017/02/12 12:11:21, tommi (webrtc) wrote:
> do we need one? (comment?)
> if we could instead live with being only movable, that would be nice.

I can't think of where it would be useful. I'll remove it for now.

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode229
webrtc/api/rtcerror.h:229: T& value() {
On 2017/02/12 12:11:21, tommi (webrtc) wrote:
> can you add a comment that explains when this is needed?

It could be useful if the implementation, say, wants to check something about
the value of an RTCErrorOr before passing it further up the stack.

RTCErrorOr<Foo> MakeFooInternal(Params p) {
  // stuff
  return Foo(p);
}

RTCErrorOr<Foo> MakeFoo(Params p) {
  auto result = MakeFooInternal(p);
  if (result.ok()) {
    LOG(LS_INFO) << "Created Foo with ID: " << result.value().id();
  }
  return result;
}

Bad example, since that logging could be done in MakeFooInternal, but that's the
idea.

[tommi, 2017-02-12 16:27:28]

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h
File webrtc/api/rtcerror.h (right):

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode229
webrtc/api/rtcerror.h:229: T& value() {
On 2017/02/12 16:19:23, Taylor Brandstetter wrote:
> On 2017/02/12 12:11:21, tommi (webrtc) wrote:
> > can you add a comment that explains when this is needed?
> 
> It could be useful if the implementation, say, wants to check something about
> the value of an RTCErrorOr before passing it further up the stack.
> 
> RTCErrorOr<Foo> MakeFooInternal(Params p) {
>   // stuff
>   return Foo(p);
> }
> 
> RTCErrorOr<Foo> MakeFoo(Params p) {
>   auto result = MakeFooInternal(p);
>   if (result.ok()) {
>     LOG(LS_INFO) << "Created Foo with ID: " << result.value().id();
>   }
>   return result;
> }
> 
> Bad example, since that logging could be done in MakeFooInternal, but that's
the
> idea.

Would the |const T& value()| accessor cover that case?
I'm specifically wondering about the ability to be able to change |value|
outside of the RTCErrorOr class.

[Taylor Brandstetter, 2017-02-12 16:44:12]

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h
File webrtc/api/rtcerror.h (right):

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode229
webrtc/api/rtcerror.h:229: T& value() {
On 2017/02/12 16:27:28, tommi (webrtc) wrote:
> On 2017/02/12 16:19:23, Taylor Brandstetter wrote:
> > On 2017/02/12 12:11:21, tommi (webrtc) wrote:
> > > can you add a comment that explains when this is needed?
> > 
> > It could be useful if the implementation, say, wants to check something
about
> > the value of an RTCErrorOr before passing it further up the stack.
> > 
> > RTCErrorOr<Foo> MakeFooInternal(Params p) {
> >   // stuff
> >   return Foo(p);
> > }
> > 
> > RTCErrorOr<Foo> MakeFoo(Params p) {
> >   auto result = MakeFooInternal(p);
> >   if (result.ok()) {
> >     LOG(LS_INFO) << "Created Foo with ID: " << result.value().id();
> >   }
> >   return result;
> > }
> > 
> > Bad example, since that logging could be done in MakeFooInternal, but that's
> the
> > idea.
> 
> Would the |const T& value()| accessor cover that case?
> I'm specifically wondering about the ability to be able to change |value|
> outside of the RTCErrorOr class.

There are still some cases where the non-const-ref accessor is convenient.

RTCErrorOr<Foo> MakeGeneralFoo(...);

RTCErrorOr<Foo> MakeVideoFoo(...) {
  auto result = MakeGeneralFoo(...);
  if (!result.ok()) {
    return result;
  }
  result.value().DoVideoSpecificThing();
  return result;
}

RTCErrorOr<Foo> MakeAudioFoo(...) {
  // ...
}

[tommi, 2017-02-12 18:19:32]

I'm heading out, so just sending the two drafts I had. Will take a look at the
latest patch later

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc
File webrtc/api/rtcerror.cc (right):

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc#newcode41
webrtc/api/rtcerror.cc:41: static RTCError ok;
On 2017/02/12 16:19:23, Taylor Brandstetter wrote:
> On 2017/02/12 12:11:21, tommi (webrtc) wrote:
> > have you checked if this is thread safe? I don't think it is on Windows at
> > least.
> > I think there is some singleton code elsewhere that you could take a look
at. 
> > Alterternatively, if it is guaranteed that this method is always called on
the
> > same thread or if it is always called once on a known thread before any
other
> > thread can call it, it can be considered thread safe and it would be good to
> > document that here.
> 
> It's thread-safe according to the standard, though Microsoft didn't implement
> this until VS2015. Does webrtc have a minimum version of VS it targets?
chromium
> builds with VS2015.

Looks like the change in chromium was made a couple of weeks ago:
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Static...

We're following that so this should be fine.  Since it's such a recent change
though, I would like to confirm with kjellander@ before landing.

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h
File webrtc/api/rtcerror.h (right):

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode229
webrtc/api/rtcerror.h:229: T& value() {
On 2017/02/12 16:44:12, Taylor Brandstetter wrote:
> On 2017/02/12 16:27:28, tommi (webrtc) wrote:
> > On 2017/02/12 16:19:23, Taylor Brandstetter wrote:
> > > On 2017/02/12 12:11:21, tommi (webrtc) wrote:
> > > > can you add a comment that explains when this is needed?
> > > 
> > > It could be useful if the implementation, say, wants to check something
> about
> > > the value of an RTCErrorOr before passing it further up the stack.
> > > 
> > > RTCErrorOr<Foo> MakeFooInternal(Params p) {
> > >   // stuff
> > >   return Foo(p);
> > > }
> > > 
> > > RTCErrorOr<Foo> MakeFoo(Params p) {
> > >   auto result = MakeFooInternal(p);
> > >   if (result.ok()) {
> > >     LOG(LS_INFO) << "Created Foo with ID: " << result.value().id();
> > >   }
> > >   return result;
> > > }
> > > 
> > > Bad example, since that logging could be done in MakeFooInternal, but
that's
> > the
> > > idea.
> > 
> > Would the |const T& value()| accessor cover that case?
> > I'm specifically wondering about the ability to be able to change |value|
> > outside of the RTCErrorOr class.
> 
> There are still some cases where the non-const-ref accessor is convenient.
> 
> RTCErrorOr<Foo> MakeGeneralFoo(...);
> 
> RTCErrorOr<Foo> MakeVideoFoo(...) {
>   auto result = MakeGeneralFoo(...);
>   if (!result.ok()) {
>     return result;
>   }
>   result.value().DoVideoSpecificThing();
>   return result;
> }
> 
> RTCErrorOr<Foo> MakeAudioFoo(...) {
>   // ...
> }

OK, if it's necessary, I'm not against it. What I was thinking was that if we
don't actually need it now, it's easier to add it later if we need it than
remove it after usage gets added without us noticing (e.g. if a method should
have been made const but wasn't made so since things compile without it).

[the sun, 2017-02-13 07:35:02]

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

[the sun, 2017-02-13 07:35:03]

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc
File webrtc/api/rtcerror.cc (right):

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc#newcode41
webrtc/api/rtcerror.cc:41: static RTCError ok;
On 2017/02/12 18:19:32, tommi (webrtc) wrote:
> On 2017/02/12 16:19:23, Taylor Brandstetter wrote:
> > On 2017/02/12 12:11:21, tommi (webrtc) wrote:
> > > have you checked if this is thread safe? I don't think it is on Windows at
> > > least.
> > > I think there is some singleton code elsewhere that you could take a look
> at. 
> > > Alterternatively, if it is guaranteed that this method is always called on
> the
> > > same thread or if it is always called once on a known thread before any
> other
> > > thread can call it, it can be considered thread safe and it would be good
to
> > > document that here.
> > 
> > It's thread-safe according to the standard, though Microsoft didn't
implement
> > this until VS2015. Does webrtc have a minimum version of VS it targets?
> chromium
> > builds with VS2015.
> 
> Looks like the change in chromium was made a couple of weeks ago:
>
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Static...
> 
> We're following that so this should be fine.  Since it's such a recent change
> though, I would like to confirm with kjellander@ before landing.

I'd rather avoid singletons as much as possible. It doesn't scale well; i.e. bad
for backend scenario.

[tommi, 2017-02-13 12:10:01]

https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h
File webrtc/api/rtcerror.h (right):

https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new...
webrtc/api/rtcerror.h:112: std::string message_;
Can we avoid allocating a string for every error?  Won't some be either empty or
a static string?

https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new...
webrtc/api/rtcerror.h:251: RTCError error_;
this also means that every return value that uses RTCErrorOr, creates a string,
which seems unnecessary.

[the sun, 2017-02-13 13:10:23]

Description was changed from

==========
Adding RTCErrorOr class to be used by ORTC APIs.

This utility class can be used to represent either an error or a
successful return value. Follows the pattern of StatusOr in the protobuf
library.

This will be used by ORTC factory methods; for instance, CreateRtpSender
will either return an RtpSender or an error if the parameters are
invalid or some other failure occurs.

This CL also moves RTCError classes to a separate file, and adds tests
that were missing before.

BUG=webrtc:7013
==========

to

==========
Adding RTCErrorOr class to be used by ORTC APIs.

This utility class can be used to represent either an error or a
successful return value. Follows the pattern of StatusOr in the protobuf
library.

This will be used by ORTC factory methods; for instance, CreateRtpSender
will either return an RtpSender or an error if the parameters are
invalid or some other failure occurs.

This CL also moves RTCError classes to a separate file, and adds tests
that were missing before.

BUG=webrtc:7013
==========

[the sun, 2017-02-13 13:10:23]

solenberg@webrtc.org changed reviewers:
- solenberg@webrtc.org

[Taylor Brandstetter, 2017-02-13 18:03:53]

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc
File webrtc/api/rtcerror.cc (right):

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc#newcode41
webrtc/api/rtcerror.cc:41: static RTCError ok;
On 2017/02/13 07:35:03, the sun wrote:
> 
> I'd rather avoid singletons as much as possible. It doesn't scale well; i.e.
bad
> for backend scenario.

I'm curious what the downside here is. But would you prefer it returning a newly
constructed RTCError?

https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h
File webrtc/api/rtcerror.h (right):

https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new...
webrtc/api/rtcerror.h:112: std::string message_;
On 2017/02/13 12:10:01, tommi (webrtc) wrote:
> Can we avoid allocating a string for every error?  Won't some be either empty
or
> a static string?

No, there are cases where I want to build the string.

https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new...
webrtc/api/rtcerror.h:251: RTCError error_;
On 2017/02/13 12:10:01, tommi (webrtc) wrote:
> this also means that every return value that uses RTCErrorOr, creates a
string,
> which seems unnecessary.

I'd hoped that, by using move constructors/assignment, most the overhead of
strings would be reduced.

But I don't expect "RTCError"s to be used in any code that's a performance
bottleneck, since this is intended for just the API layer.

[tommi, 2017-02-13 18:17:14]

https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h
File webrtc/api/rtcerror.h (right):

https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new...
webrtc/api/rtcerror.h:104: void set_message(const std::string& message) {
message_ = message; }
is this method necessary? It could have benefits to require that the message
only be set at construction time. The message would be const, doesn't have to be
a complex object like a std::string or even set at all.

https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new...
webrtc/api/rtcerror.h:112: std::string message_;
On 2017/02/13 18:03:53, Taylor Brandstetter wrote:
> On 2017/02/13 12:10:01, tommi (webrtc) wrote:
> > Can we avoid allocating a string for every error?  Won't some be either
empty
> or
> > a static string?
> 
> No, there are cases where I want to build the string.

I think the answer to "Won't some be either empty or a static string?" will
still be Yes.
If that's the case, the interface to reading the stream (the message() method),
could return const char*. That would allow the implementation to return a static
string pointer or a std::string::c_str() when a more complex string has to be
built.

https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new...
webrtc/api/rtcerror.h:251: RTCError error_;
On 2017/02/13 18:03:53, Taylor Brandstetter wrote:
> On 2017/02/13 12:10:01, tommi (webrtc) wrote:
> > this also means that every return value that uses RTCErrorOr, creates a
> string,
> > which seems unnecessary.
> 
> I'd hoped that, by using move constructors/assignment, most the overhead of
> strings would be reduced.
> 
> But I don't expect "RTCError"s to be used in any code that's a performance
> bottleneck, since this is intended for just the API layer.

I understand. The overhead can still be reduced further and if this class will
be a part of the public API, I think it's worth considering if we're over
promising in the interface in a way that prevents the implementation from being
optimized.

[Taylor Brandstetter, 2017-02-13 18:44:18]

https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h
File webrtc/api/rtcerror.h (right):

https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new...
webrtc/api/rtcerror.h:104: void set_message(const std::string& message) {
message_ = message; }
On 2017/02/13 18:17:14, tommi (webrtc) wrote:
> is this method necessary? It could have benefits to require that the message
> only be set at construction time. The message would be const, doesn't have to
be
> a complex object like a std::string or even set at all.

The PeerConnection methods that take an "RTCError*" as an out parameter for
backwards compatibility need it. That may be the only reason.

https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new...
webrtc/api/rtcerror.h:112: std::string message_;
On 2017/02/13 18:17:14, tommi (webrtc) wrote:
> On 2017/02/13 18:03:53, Taylor Brandstetter wrote:
> > On 2017/02/13 12:10:01, tommi (webrtc) wrote:
> > > Can we avoid allocating a string for every error?  Won't some be either
> empty
> > or
> > > a static string?
> > 
> > No, there are cases where I want to build the string.
> 
> I think the answer to "Won't some be either empty or a static string?" will
> still be Yes.
> If that's the case, the interface to reading the stream (the message()
method),
> could return const char*. That would allow the implementation to return a
static
> string pointer or a std::string::c_str() when a more complex string has to be
> built.

Oh, I didn't see "some", sorry.

Hmm. But if "message" returns a "const char*", then the code above webrtc will
still have to convert it to a string again which will result in a copy, unless
it uses "const char*" everywhere the message is passed along.

[tommi, 2017-02-13 19:04:06]

On 2017/02/13 18:44:18, Taylor Brandstetter wrote:
> https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h
> File webrtc/api/rtcerror.h (right):
> 
>
https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new...
> webrtc/api/rtcerror.h:104: void set_message(const std::string& message) {
> message_ = message; }
> On 2017/02/13 18:17:14, tommi (webrtc) wrote:
> > is this method necessary? It could have benefits to require that the message
> > only be set at construction time. The message would be const, doesn't have
to
> be
> > a complex object like a std::string or even set at all.
> 
> The PeerConnection methods that take an "RTCError*" as an out parameter for
> backwards compatibility need it. That may be the only reason.
> 
>
https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new...
> webrtc/api/rtcerror.h:112: std::string message_;
> On 2017/02/13 18:17:14, tommi (webrtc) wrote:
> > On 2017/02/13 18:03:53, Taylor Brandstetter wrote:
> > > On 2017/02/13 12:10:01, tommi (webrtc) wrote:
> > > > Can we avoid allocating a string for every error?  Won't some be either
> > empty
> > > or
> > > > a static string?
> > > 
> > > No, there are cases where I want to build the string.
> > 
> > I think the answer to "Won't some be either empty or a static string?" will
> > still be Yes.
> > If that's the case, the interface to reading the stream (the message()
> method),
> > could return const char*. That would allow the implementation to return a
> static
> > string pointer or a std::string::c_str() when a more complex string has to
be
> > built.
> 
> Oh, I didn't see "some", sorry.
> 
> Hmm. But if "message" returns a "const char*", then the code above webrtc will
> still have to convert it to a string again which will result in a copy, unless
> it uses "const char*" everywhere the message is passed along.

Well, I was thinking if we had something like:

union {
  const char* static_;
  std::string* str_;
} message_;

then we could return either message_.static_ or message.str_->c_str() depending
on which was needed.
In Chromium, a lot of the error strings are just static strings and all that
needs to be passed around, is a pointer.  In rare cases, a string needs to be
constructed, so that's also supported, but there has been a lot of work done to
back out of designs that require string copies, so I'm thinking that here, we
can optimize for (presumably) the most common case (no error), support the
second most common case in an efficient case (static error string) and still
support the rich string case, which I'm assuming will be the rarest.

sorry for being difficult with this btw :) I'm trying to make up for it by
replying quickly.

[the sun, 2017-02-13 19:28:53]

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc
File webrtc/api/rtcerror.cc (right):

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc#newcode41
webrtc/api/rtcerror.cc:41: static RTCError ok;
On 2017/02/13 18:03:53, Taylor Brandstetter wrote:
> On 2017/02/13 07:35:03, the sun wrote:
> > 
> > I'd rather avoid singletons as much as possible. It doesn't scale well; i.e.
> bad
> > for backend scenario.
> 
> I'm curious what the downside here is. But would you prefer it returning a
newly
> constructed RTCError?

Sorry, I misread the code in the morning haze. Looks fine, but shouldn't it be
"static const"?

[Taylor Brandstetter, 2017-02-13 22:22:05]

On 2017/02/13 19:04:06, tommi (webrtc) wrote:
> 
> Well, I was thinking if we had something like:
> 
> union {
>   const char* static_;
>   std::string* str_;
> } message_;
> 
> then we could return either message_.static_ or message.str_->c_str()
depending
> on which was needed.
> In Chromium, a lot of the error strings are just static strings and all that
> needs to be passed around, is a pointer.  In rare cases, a string needs to be
> constructed, so that's also supported, but there has been a lot of work done
to
> back out of designs that require string copies, so I'm thinking that here, we
> can optimize for (presumably) the most common case (no error), support the
> second most common case in an efficient case (static error string) and still
> support the rich string case, which I'm assuming will be the rarest.
> 
> sorry for being difficult with this btw :) I'm trying to make up for it by
> replying quickly.

Ok, if this is the approach chromium is taking, you convinced me. See updated
patchset.

[Taylor Brandstetter, 2017-02-13 22:24:15]

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc
File webrtc/api/rtcerror.cc (right):

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc#newcode41
webrtc/api/rtcerror.cc:41: static RTCError ok;
On 2017/02/13 19:28:52, the sun wrote:
> On 2017/02/13 18:03:53, Taylor Brandstetter wrote:
> > On 2017/02/13 07:35:03, the sun wrote:
> > > 
> > > I'd rather avoid singletons as much as possible. It doesn't scale well;
i.e.
> > bad
> > > for backend scenario.
> > 
> > I'm curious what the downside here is. But would you prefer it returning a
> newly
> > constructed RTCError?
> 
> Sorry, I misread the code in the morning haze. Looks fine, but shouldn't it be
> "static const"?

Done. By the way, when I did this, I got a compile warning that was only fixed
by changing the constructor from "RTCError() = default" to "RTCError() {}".
Would you happen to know why? Aren't these doing the same thing?

[the sun, 2017-02-13 23:02:24]

On 2017/02/13 22:24:15, Taylor Brandstetter wrote:
> https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc
> File webrtc/api/rtcerror.cc (right):
> 
>
https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc#newcode41
> webrtc/api/rtcerror.cc:41: static RTCError ok;
> On 2017/02/13 19:28:52, the sun wrote:
> > On 2017/02/13 18:03:53, Taylor Brandstetter wrote:
> > > On 2017/02/13 07:35:03, the sun wrote:
> > > > 
> > > > I'd rather avoid singletons as much as possible. It doesn't scale well;
> i.e.
> > > bad
> > > > for backend scenario.
> > > 
> > > I'm curious what the downside here is. But would you prefer it returning a
> > newly
> > > constructed RTCError?
> > 
> > Sorry, I misread the code in the morning haze. Looks fine, but shouldn't it
be
> > "static const"?
> 
> Done. By the way, when I did this, I got a compile warning that was only fixed
> by changing the constructor from "RTCError() = default" to "RTCError() {}".
> Would you happen to know why? Aren't these doing the same thing?

That's interesting! No, I don't know. If anything I'd have guessed "= default"
would possess more magic powers. :)

Do you have any idea kwiberg@?

[kwiberg-webrtc, 2017-02-14 09:51:49]

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

[kwiberg-webrtc, 2017-02-14 09:51:49]

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc
File webrtc/api/rtcerror.cc (right):

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc#newcode41
webrtc/api/rtcerror.cc:41: static RTCError ok;
On 2017/02/13 22:24:15, Taylor Brandstetter wrote:
> On 2017/02/13 19:28:52, the sun wrote:
> > On 2017/02/13 18:03:53, Taylor Brandstetter wrote:
> > > On 2017/02/13 07:35:03, the sun wrote:
> > > > 
> > > > I'd rather avoid singletons as much as possible. It doesn't scale well;
> i.e.
> > > bad
> > > > for backend scenario.
> > > 
> > > I'm curious what the downside here is. But would you prefer it returning a
> > newly
> > > constructed RTCError?
> > 
> > Sorry, I misread the code in the morning haze. Looks fine, but shouldn't it
be
> > "static const"?
> 
> Done. By the way, when I did this, I got a compile warning that was only fixed
> by changing the constructor from "RTCError() = default" to "RTCError() {}".
> Would you happen to know why? Aren't these doing the same thing?

Interesting. What did the message say?

This page (http://en.cppreference.com/w/cpp/language/default_constructor) has a
section "Deleted implicitly-declared default constructor" on circumstances under
which the implicitly-declared default constructor is deleted. I can't see that
any of them would apply here, though.

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h
File webrtc/api/rtcerror.h (right):

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode243
webrtc/api/rtcerror.h:243: T value_;
Hmm. So an RTCErrorOr always contains a T. This may be inefficient, and may be
problematic if T isn't default-constructable.

https://codereview.webrtc.org/2692723002/diff/40001/webrtc/api/rtcerror.h
File webrtc/api/rtcerror.h (right):

https://codereview.webrtc.org/2692723002/diff/40001/webrtc/api/rtcerror.h#new...
webrtc/api/rtcerror.h:88: // the message is a static string.
If this sort of thing is a consideration, you may want to take the std::string
by value, so that the caller can std::move it. Or add a third constructor that
takes a std::string&&.

[tommi, 2017-02-14 09:55:56]

https://codereview.webrtc.org/2692723002/diff/40001/webrtc/api/rtcerror.h
File webrtc/api/rtcerror.h (right):

https://codereview.webrtc.org/2692723002/diff/40001/webrtc/api/rtcerror.h#new...
webrtc/api/rtcerror.h:88: // the message is a static string.
On 2017/02/14 09:51:49, kwiberg-webrtc wrote:
> If this sort of thing is a consideration, you may want to take the std::string
> by value, so that the caller can std::move it. Or add a third constructor that
> takes a std::string&&.

this is a consideration because there's no need to allocate memory + copy for a
static string

[kwiberg-webrtc, 2017-02-14 11:37:25]

https://codereview.webrtc.org/2692723002/diff/40001/webrtc/api/rtcerror.h
File webrtc/api/rtcerror.h (right):

https://codereview.webrtc.org/2692723002/diff/40001/webrtc/api/rtcerror.h#new...
webrtc/api/rtcerror.h:88: // the message is a static string.
On 2017/02/14 09:55:56, tommi (webrtc) wrote:
> On 2017/02/14 09:51:49, kwiberg-webrtc wrote:
> > If this sort of thing is a consideration, you may want to take the
std::string
> > by value, so that the caller can std::move it. Or add a third constructor
that
> > takes a std::string&&.
> 
> this is a consideration because there's no need to allocate memory + copy for
a
> static string

Then I'd suggest having *only* the const char* and std::string&& variants, so
that callers are forced to either pass a static string or pass an rvalue
std::string. That makes it hard for them to make performance-degrading mistakes.

[tommi, 2017-02-14 11:58:58]

https://codereview.webrtc.org/2692723002/diff/40001/webrtc/api/rtcerror.h
File webrtc/api/rtcerror.h (right):

https://codereview.webrtc.org/2692723002/diff/40001/webrtc/api/rtcerror.h#new...
webrtc/api/rtcerror.h:88: // the message is a static string.
On 2017/02/14 11:37:25, kwiberg-webrtc wrote:
> On 2017/02/14 09:55:56, tommi (webrtc) wrote:
> > On 2017/02/14 09:51:49, kwiberg-webrtc wrote:
> > > If this sort of thing is a consideration, you may want to take the
> std::string
> > > by value, so that the caller can std::move it. Or add a third constructor
> that
> > > takes a std::string&&.
> > 
> > this is a consideration because there's no need to allocate memory + copy
for
> a
> > static string
> 
> Then I'd suggest having *only* the const char* and std::string&& variants, so
> that callers are forced to either pass a static string or pass an rvalue
> std::string. That makes it hard for them to make performance-degrading
mistakes.

That sounds good to me. I may have mentioned it a couple of patch sets ago,
maybe there's some reason that Taylor knows about.

[Taylor Brandstetter, 2017-02-15 02:22:23]

Another weird thing I ran into: this doesn't work:

str_.~string();

But this does:

str_.~basic_string();

It sounds like this is a bug?
http://stackoverflow.com/questions/24593942/how-to-explicitly-call-a-namespac...

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc
File webrtc/api/rtcerror.cc (right):

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc#newcode41
webrtc/api/rtcerror.cc:41: static RTCError ok;
On 2017/02/14 09:51:49, kwiberg-webrtc wrote:
> On 2017/02/13 22:24:15, Taylor Brandstetter wrote:
> > On 2017/02/13 19:28:52, the sun wrote:
> > > On 2017/02/13 18:03:53, Taylor Brandstetter wrote:
> > > > On 2017/02/13 07:35:03, the sun wrote:
> > > > > 
> > > > > I'd rather avoid singletons as much as possible. It doesn't scale
well;
> > i.e.
> > > > bad
> > > > > for backend scenario.
> > > > 
> > > > I'm curious what the downside here is. But would you prefer it returning
a
> > > newly
> > > > constructed RTCError?
> > > 
> > > Sorry, I misread the code in the morning haze. Looks fine, but shouldn't
it
> be
> > > "static const"?
> > 
> > Done. By the way, when I did this, I got a compile warning that was only
fixed
> > by changing the constructor from "RTCError() = default" to "RTCError() {}".
> > Would you happen to know why? Aren't these doing the same thing?
> 
> Interesting. What did the message say?
> 
> This page (http://en.cppreference.com/w/cpp/language/default_constructor) has
a
> section "Deleted implicitly-declared default constructor" on circumstances
under
> which the implicitly-declared default constructor is deleted. I can't see that
> any of them would apply here, though.

"default initialization of an object of const type 'const webrtc::RTCError'
without a user-provided default constructor". It seems it's the union it's
unhappy with. Maybe the union default constructor doesn't work how I thought it
would, or the compiler has a bug. I changed the union anyway though.

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h
File webrtc/api/rtcerror.h (right):

https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode243
webrtc/api/rtcerror.h:243: T value_;
On 2017/02/14 09:51:49, kwiberg-webrtc wrote:
> Hmm. So an RTCErrorOr always contains a T. This may be inefficient, and may be
> problematic if T isn't default-constructable.

I plan to use this for things that are either small or unique_ptrs, so I think
this should be fine.

https://codereview.webrtc.org/2692723002/diff/40001/webrtc/api/rtcerror.h
File webrtc/api/rtcerror.h (right):

https://codereview.webrtc.org/2692723002/diff/40001/webrtc/api/rtcerror.h#new...
webrtc/api/rtcerror.h:88: // the message is a static string.
On 2017/02/14 11:58:58, tommi (webrtc) wrote:
> On 2017/02/14 11:37:25, kwiberg-webrtc wrote:
> > On 2017/02/14 09:55:56, tommi (webrtc) wrote:
> > > On 2017/02/14 09:51:49, kwiberg-webrtc wrote:
> > > > If this sort of thing is a consideration, you may want to take the
> > std::string
> > > > by value, so that the caller can std::move it. Or add a third
constructor
> > that
> > > > takes a std::string&&.
> > > 
> > > this is a consideration because there's no need to allocate memory + copy
> for
> > a
> > > static string
> > 
> > Then I'd suggest having *only* the const char* and std::string&& variants,
so
> > that callers are forced to either pass a static string or pass an rvalue
> > std::string. That makes it hard for them to make performance-degrading
> mistakes.
> 
> That sounds good to me. I may have mentioned it a couple of patch sets ago,
> maybe there's some reason that Taylor knows about.

Done.

[tommi, 2017-02-15 16:27:07]

Thanks for jumping through all the hoops Taylor.  lgtm assuming the destructor
strangeness isn't a problem in practice.  Might be good to document it though.

https://codereview.webrtc.org/2692723002/diff/120001/webrtc/api/rtcerror.cc
File webrtc/api/rtcerror.cc (right):

https://codereview.webrtc.org/2692723002/diff/120001/webrtc/api/rtcerror.cc#n...
webrtc/api/rtcerror.cc:62: string_message_.~basic_string();
interesting - did you hit this problem with all tool chains?

[tommi, 2017-02-15 16:27:17]

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

[commit-bot: I haz the power, 2017-02-15 16:27:27]

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-02-15 16:29:15]

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

[commit-bot: I haz the power, 2017-02-15 16:29:16]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/17350)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22785)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/21553)
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/18787)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/22802)

[Taylor Brandstetter, 2017-02-15 21:14:21]

https://codereview.webrtc.org/2692723002/diff/120001/webrtc/api/rtcerror.cc
File webrtc/api/rtcerror.cc (right):

https://codereview.webrtc.org/2692723002/diff/120001/webrtc/api/rtcerror.cc#n...
webrtc/api/rtcerror.cc:62: string_message_.~basic_string();
On 2017/02/15 16:27:07, tommi (webrtc) wrote:
> interesting - did you hit this problem with all tool chains?

I'd only tried with gcc. This might be expected behavior though, since string is
a typedef.

[tommi, 2017-02-15 21:32:46]

On 2017/02/15 21:14:21, Taylor Brandstetter wrote:
> https://codereview.webrtc.org/2692723002/diff/120001/webrtc/api/rtcerror.cc
> File webrtc/api/rtcerror.cc (right):
> 
>
https://codereview.webrtc.org/2692723002/diff/120001/webrtc/api/rtcerror.cc#n...
> webrtc/api/rtcerror.cc:62: string_message_.~basic_string();
> On 2017/02/15 16:27:07, tommi (webrtc) wrote:
> > interesting - did you hit this problem with all tool chains?
> 
> I'd only tried with gcc. This might be expected behavior though, since string
is
> a typedef.

Ah yeah, could be. Wonder if you could do the "technically" correct thing by
using a template? something like
template<typename T>
void CallDestructor(T* t) { t->~T(); }

[tommi, 2017-02-15 21:33:15]

On 2017/02/15 21:32:46, tommi (webrtc) wrote:
> On 2017/02/15 21:14:21, Taylor Brandstetter wrote:
> > https://codereview.webrtc.org/2692723002/diff/120001/webrtc/api/rtcerror.cc
> > File webrtc/api/rtcerror.cc (right):
> > 
> >
>
https://codereview.webrtc.org/2692723002/diff/120001/webrtc/api/rtcerror.cc#n...
> > webrtc/api/rtcerror.cc:62: string_message_.~basic_string();
> > On 2017/02/15 16:27:07, tommi (webrtc) wrote:
> > > interesting - did you hit this problem with all tool chains?
> > 
> > I'd only tried with gcc. This might be expected behavior though, since
string
> is
> > a typedef.
> 
> Ah yeah, could be. Wonder if you could do the "technically" correct thing by
> using a template? something like
> template<typename T>
> void CallDestructor(T* t) { t->~T(); }

bah, nevermind, if basic_string works, that's probably more readable and at
least as correct :)

[Taylor Brandstetter, 2017-02-15 21:37:57]

On 2017/02/15 21:32:46, tommi (webrtc) wrote:
> Wonder if you could do the "technically" correct thing by
> using a template? something like
> template<typename T>
> void CallDestructor(T* t) { t->~T(); }

Oh, that must be why rtc::Optional<std::string> works. Templates do so much
magic I don't understand.

[Taylor Brandstetter, 2017-02-16 01:55:53]

I changed "CreateAndLogError" to a macro, for a couple reasons:

1. It doesn't work with chromium's "logging.h" override.
2. It doesn't log the line of code that called it, which makes it much less
useful.

[Taylor Brandstetter, 2017-02-17 06:34:59]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2017-02-17 06:35:00]

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/2692723002/#ps160001 (title:
"Changing "CreateAndLogError" to a macro.")

[commit-bot: I haz the power, 2017-02-17 06:35:08]

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-02-17 07:31:34]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1487313299614570,
"parent_rev": "74520c6718fc8ebc6da9fec1ce28b1b49112a921", "commit_rev":
"6038e97e048bebf763f7149ce0d84d0798c65f8b"}

[commit-bot: I haz the power, 2017-02-17 07:31:37]

Description was changed from

==========
Adding RTCErrorOr class to be used by ORTC APIs.

This utility class can be used to represent either an error or a
successful return value. Follows the pattern of StatusOr in the protobuf
library.

This will be used by ORTC factory methods; for instance, CreateRtpSender
will either return an RtpSender or an error if the parameters are
invalid or some other failure occurs.

This CL also moves RTCError classes to a separate file, and adds tests
that were missing before.

BUG=webrtc:7013
==========

to

==========
Adding RTCErrorOr class to be used by ORTC APIs.

This utility class can be used to represent either an error or a
successful return value. Follows the pattern of StatusOr in the protobuf
library.

This will be used by ORTC factory methods; for instance, CreateRtpSender
will either return an RtpSender or an error if the parameters are
invalid or some other failure occurs.

This CL also moves RTCError classes to a separate file, and adds tests
that were missing before.

BUG=webrtc:7013

Review-Url: https://codereview.webrtc.org/2692723002
Cr-Commit-Position: refs/heads/master@{#16659}
Committed:
https://chromium.googlesource.com/external/webrtc/+/6038e97e048bebf763f7149ce...
==========

[commit-bot: I haz the power, 2017-02-17 07:31:39]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/external/webrtc/+/6038e97e048bebf763f7149ce...

[kjellander_webrtc, 2017-02-21 10:48:02]

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

[kjellander_webrtc, 2017-02-21 10:48:04]

https://codereview.webrtc.org/2692723002/diff/160001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2692723002/diff/160001/webrtc/api/BUILD.gn#newc...
webrtc/api/BUILD.gn:206: rtc_test("rtc_api_unittests") {
Can you make this a rtc_source_set and add it as a dependency in rtc_unittests
instead?
https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?rcl=0bdacd8a...

I'd prefer not adding another test binary if possible. I don't see webrtc/api
will have that many tests long-term.

[kwiberg-webrtc, 2017-02-21 12:00:26]

https://codereview.webrtc.org/2692723002/diff/160001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2692723002/diff/160001/webrtc/api/BUILD.gn#newc...
webrtc/api/BUILD.gn:206: rtc_test("rtc_api_unittests") {
On 2017/02/21 10:48:03, kjellander_webrtc wrote:
> Can you make this a rtc_source_set and add it as a dependency in rtc_unittests
> instead?
>
https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?rcl=0bdacd8a...
> 
> I'd prefer not adding another test binary if possible. I don't see webrtc/api
> will have that many tests long-term.

I'm neutral on the issue of having more test binaries or not, but webrtc/api/
will probably end up with a bunch of unit tests. Stuff like ArrayView, Optional,
Buffer, checks.h, etc. are going to have to move to api/ sooner or later.

[kjellander_webrtc, 2017-02-21 12:01:44]

https://codereview.webrtc.org/2692723002/diff/160001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2692723002/diff/160001/webrtc/api/BUILD.gn#newc...
webrtc/api/BUILD.gn:206: rtc_test("rtc_api_unittests") {
On 2017/02/21 12:00:26, kwiberg-webrtc wrote:
> On 2017/02/21 10:48:03, kjellander_webrtc wrote:
> > Can you make this a rtc_source_set and add it as a dependency in
rtc_unittests
> > instead?
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?rcl=0bdacd8a...
> > 
> > I'd prefer not adding another test binary if possible. I don't see
webrtc/api
> > will have that many tests long-term.
> 
> I'm neutral on the issue of having more test binaries or not, but webrtc/api/
> will probably end up with a bunch of unit tests. Stuff like ArrayView,
Optional,
> Buffer, checks.h, etc. are going to have to move to api/ sooner or later.

Right, but at least I expect them all to be fast. Not stuff like ramping up
bitrates and things like that.
Please let's try to not introduce a new one here.

[Taylor Brandstetter, 2017-02-21 17:59:32]

https://codereview.webrtc.org/2692723002/diff/160001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2692723002/diff/160001/webrtc/api/BUILD.gn#newc...
webrtc/api/BUILD.gn:206: rtc_test("rtc_api_unittests") {
On 2017/02/21 12:01:44, kjellander_webrtc wrote:
> On 2017/02/21 12:00:26, kwiberg-webrtc wrote:
> > On 2017/02/21 10:48:03, kjellander_webrtc wrote:
> > > Can you make this a rtc_source_set and add it as a dependency in
> rtc_unittests
> > > instead?
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?rcl=0bdacd8a...
> > > 
> > > I'd prefer not adding another test binary if possible. I don't see
> webrtc/api
> > > will have that many tests long-term.
> > 
> > I'm neutral on the issue of having more test binaries or not, but
webrtc/api/
> > will probably end up with a bunch of unit tests. Stuff like ArrayView,
> Optional,
> > Buffer, checks.h, etc. are going to have to move to api/ sooner or later.
> 
> Right, but at least I expect them all to be fast. Not stuff like ramping up
> bitrates and things like that.
> Please let's try to not introduce a new one here.

Here: https://codereview.webrtc.org/2709573003

It was on my to-do list to do something about this anyway, since
rtc_api_unittests isn't being run yet.