Add unit tests for webrtc_media_stream_adapter.cc

content/renderer/media/webrtc/media_stream_video_webrtc_sink.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_RENDERER_MEDIA_WEBRTC_WEBRTC_VIDEO_TRACK_ADAPTER_H_
 #define CONTENT_RENDERER_MEDIA_WEBRTC_WEBRTC_VIDEO_TRACK_ADAPTER_H_
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/threading/thread_checker.h"
 #include "content/public/renderer/media_stream_video_sink.h"
 #include "content/renderer/media/webrtc/webrtc_video_capturer_adapter.h"
 #include "third_party/WebKit/public/platform/WebMediaStreamTrack.h"
 #include "third_party/webrtc/api/mediastreaminterface.h"
 
 namespace content {
 
 class MediaStreamVideoTrack;
 class PeerConnectionDependencyFactory;
 
 // MediaStreamVideoWebRtcSink is an adapter between a
 // content::MediaStreamVideoTrack object and a webrtc VideoTrack that is
 // currently sent on a PeerConnection.
 // The responsibility of the class is to create and own a representation of a
 // webrtc VideoTrack that can be added and removed from a RTCPeerConnection. An
 // instance of MediaStreamVideoWebRtcSink is created when a VideoTrack is added
 // 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);
   ~MediaStreamVideoWebRtcSink() override;
 
   webrtc::VideoTrackInterface* webrtc_video_track() {
     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;
 
  private:
   // Helper to request a refresh frame from the source. Called via the callback
   // passed to WebRtcVideoSourceAdapter.
   void RequestRefreshFrame();
 
   // Used to DCHECK that we are called on the correct thread.
@@ skipping 14 matching lines @@
   // MediaStreamVideoWebRtcSink in order to allow it to request refresh frames.
   // See comments in media_stream_video_webrtc_sink.cc.
   base::WeakPtrFactory<MediaStreamVideoWebRtcSink> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(MediaStreamVideoWebRtcSink);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_RENDERER_MEDIA_WEBRTC_WEBRTC_VIDEO_TRACK_ADAPTER_H_