Add unit tests for webrtc_media_stream_adapter.cc

content/renderer/media/webrtc/media_stream_video_webrtc_sink.h

Index: content/renderer/media/webrtc/media_stream_video_webrtc_sink.h
diff --git a/content/renderer/media/webrtc/media_stream_video_webrtc_sink.h b/content/renderer/media/webrtc/media_stream_video_webrtc_sink.h
index fdd6233d4a39ff9dc6c7771d265ac03afc4b79d2..a4a48b38ece280a250cc68ee00baf19cd903f420 100644
--- a/content/renderer/media/webrtc/media_stream_video_webrtc_sink.h
+++ b/content/renderer/media/webrtc/media_stream_video_webrtc_sink.h
@@ -27,7 +27,7 @@ class PeerConnectionDependencyFactory;
 // to an RTCPeerConnection object.
 // Instances of this class is owned by the WebRtcMediaStreamAdapter object that
 // created it.
-class MediaStreamVideoWebRtcSink : public MediaStreamVideoSink {
+class CONTENT_EXPORT MediaStreamVideoWebRtcSink : public MediaStreamVideoSink {
  public:
   MediaStreamVideoWebRtcSink(const blink::WebMediaStreamTrack& track,
                              PeerConnectionDependencyFactory* factory);
@@ -37,6 +37,9 @@ class MediaStreamVideoWebRtcSink : public MediaStreamVideoSink {
     return video_track_.get();
   }
 
+  // Exposed for testing.
+  rtc::Optional<bool> SourceNeedsDenoising();

[tommi (sloooow) - chröme, 2016/10/10 09:53:35]

SourceNeedsDenoisingForTesting()

Should it also be const?

Actually, instead of adding this method, would it work to inherit from
MediaStreamVideoWebRtcSink in a test only class (in the cc file of the test that
needs it) and have this method only there?

[hta - Chromium, 2016/10/10 13:15:02]

I don't like having derived classes as the implementation under test in the
tests - it's a bit too easy to introduce behavior in the derived class that
doesn't match 100% the behavior of the class we are trying to test.

Renamed it to SourceNeedsDenoisingTestHelper.

[tommi (sloooow) - chröme, 2016/10/10 13:33:37]

The convention is to have the suffix "ForTesting", so I suggest we stick to that
if we need the functionality here.

The counter argument for the second comment is that we don't want to ship test
code.  There's a balance to strike there and from what I've seen, we use
ForTesting methods only in classes where it's inhibitively complex to test an
implementation without modifying it directly.

[mcasas, 2016/10/10 15:58:22]

On that: 
"Functions used only for testing should be restricted 
to test-only scenarios either by #ifdefing them 
appropriately (e.g. #if defined(UNIT_TEST)) or by 
naming them with a ForTesting suffix. The latter will 
be checked at presubmit time to ensure they're only 
called by test files."

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md

[hta - Chromium, 2016/10/10 18:59:01]

Ack - if the "ForTesting suffix is magical, it makes sense to use it all the
time. The accessor is an accessor that calls an accessor to a sub-object that is
not exposed in the .h file at all - I count exposing the sub-object in the .h
file just so that we can write a derived class that accesses it as "complex to
test an implementation without modifying it".

(I also looked at modifying the class under test to use an injected class
instead of the WebRTC class, but it's a class hierarchy, not an encapsulation,
so this would involve rewriting the entire logic of the class.)

[tommi (sloooow) - chröme, 2016/10/10 19:10:36]

That's OK with me.

Just to clarify my comment, that is not what I mean. If the complexity is to
make a member variable protected instead of private, that does not qualify as
complex.  :)

What I mean is more inline with e.g. the startup path of the browser, whereby
mocking the entire browser is too complex and we instead add "ForTesting"
methods to that harness that allow for testing.

However, for this CL, the implementation is trivial and if it's inline, will not
cost us bytes of shipping dead code.

+
  protected:
   // Implementation of MediaStreamSink.
   void OnEnabledChanged(bool enabled) override;