Simplify class FakeVideoTrackRenderer.

FakeVideoTrackRenderer is a thin wrapper over
FakeVideoRenderer, only registering itself on a
VideoSourceInterface on construction and removing itself on
destruction. Let it inherit FakeVideoRenderer, instead of
proxying all methods.

BUG=webrtc:5426
Committed: https://crrev.com/3858477d4b751f85cbbcc6aec3570eebe4bf8e55
Cr-Commit-Position: refs/heads/master@{#12313}

[nisse-webrtc, 2016-03-24 09:20:23]

nisse@webrtc.org changed reviewers:
+ perkj@webrtc.org, pthatcher@webrtc.org

[nisse-webrtc, 2016-03-24 09:20:24]

I noticed that the FakeVideoTrackRenderer class no longer does anything
interesting, it delegates everything to FakeVideoRenderer, and only adds the
convenience of providing a video source to the constructor.

One subtle difference when moving that convenience feature to FakeVideoRenderer
is that it uses a raw VideoSourceInterface pointer, not a reference counter
VideoTrackSource. I believe that is ok with the test code, the videotrack
objects are kept alive during the tests by other means.

[nisse-webrtc, 2016-03-24 09:20:33]

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

[commit-bot: I haz the power, 2016-03-24 09:20:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828173002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828173002/1

[commit-bot: I haz the power, 2016-03-24 09:29:48]

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

[commit-bot: I haz the power, 2016-03-24 09:29:49]

Dry run: Try jobs failed on following builders:
  mac_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/13791)
  win_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/13773)

[perkj_webrtc, 2016-03-24 10:27:09]

On 2016/03/24 09:29:49, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   mac_rel on tryserver.webrtc (JOB_FAILED,
> http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/13791)
>   win_rel on tryserver.webrtc (JOB_FAILED,
> http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/13773)

Needs some more work as discussed.

[nisse-webrtc, 2016-03-24 10:34:32]

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

[commit-bot: I haz the power, 2016-03-24 10:34:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828173002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828173002/20001

[commit-bot: I haz the power, 2016-03-24 10:42:19]

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

[commit-bot: I haz the power, 2016-03-24 10:42:19]

Dry run: Try jobs failed on following builders:
  win_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/13778)

[nisse-webrtc, 2016-03-24 10:42:34]

Ok, I'm having some second thoughts on this cl. The raw pointer actually broke
the tests. Not due to anything fundamental, but objects getting destructed in a
bad order. (Using reference counts just to solve that problem is overkill, but I
don't care about that in testcode...).

The current cl (#2) keeps FakeVideoTrackRenderer, but lets it inherit
FakeVideoRenderer. In my opinion, that's a nice simplification and removes
delegation boilerplatecode.

In addition, it switches from using FakeVideoTrackRenderer to FakeVideoRenderer
+ explicit calls to AddOrUpdateSink and RemoveSink at the places where it was
easy to do. THat change is a bit more questionable (but I think it would make
sense if FakeVideoTrackRenderer was eliminated).

FakeVideoTrackRenderer is still used in peerconnection_unittest.cc and
peerconnectiontestwrapper.cc, and my current understanding is that it actually
adds some value there.

[perkj_webrtc, 2016-03-29 15:27:31]

On 2016/03/24 10:42:34, nisse-webrtc wrote:
> Ok, I'm having some second thoughts on this cl. The raw pointer actually broke
> the tests. Not due to anything fundamental, but objects getting destructed in
a
> bad order. (Using reference counts just to solve that problem is overkill, but
I
> don't care about that in testcode...).
> 
> The current cl (#2) keeps FakeVideoTrackRenderer, but lets it inherit
> FakeVideoRenderer. In my opinion, that's a nice simplification and removes
> delegation boilerplatecode.
> 
> In addition, it switches from using FakeVideoTrackRenderer to
FakeVideoRenderer
> + explicit calls to AddOrUpdateSink and RemoveSink at the places where it was
> easy to do. THat change is a bit more questionable (but I think it would make
> sense if FakeVideoTrackRenderer was eliminated).
> 
> FakeVideoTrackRenderer is still used in peerconnection_unittest.cc and
> peerconnectiontestwrapper.cc, and my current understanding is that it actually
> adds some value there.

Looks like you still have a problem to resolve.

[nisse-webrtc, 2016-03-30 08:21:11]

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

[commit-bot: I haz the power, 2016-03-30 08:21:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828173002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828173002/40001

[nisse-webrtc, 2016-03-30 09:13:24]

Fixed the use-after-free bug in previous version.

Question is, what should we do? Do you think all of this cl makes sense, or is
it better to only do the simplification to FakeVideTrackRenderer, i.e., only the
change to fakevideotrackrenderer.h?

[commit-bot: I haz the power, 2016-03-30 09:21:46]

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

[commit-bot: I haz the power, 2016-03-30 09:21:47]

Dry run: This issue passed the CQ dry run.

[perkj_webrtc, 2016-04-04 06:20:53]

On 2016/03/30 09:21:47, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

I think only fakevideotrackrenderer.h.

[nisse-webrtc, 2016-04-04 06:40:53]

Description was changed from

==========
Deleted class FakeVideoTrackRenderer.

It was wrapper over FakeVideoRenderer, only registering itself on a
VideoSourceInterface on construction and removing itself on
destruction. Instead, extended FakeVideoRenderer to optionally take a
VideoSourceInterface as a constructor argument.

BUG=webrtc:5426
==========

to

==========
FakeVideoTrackRenderer is a thin wrapper over
FakeVideoRenderer, only registering itself on a
VideoSourceInterface on construction and removing itself on
destruction. Let it inherit FakeVideoRenderer, instead of
proxying all methods.

BUG=webrtc:5426
==========

[nisse-webrtc, 2016-04-04 06:41:52]

Reverted all changes but fakevideotrackrenderer.h.

[perkj_webrtc, 2016-04-07 11:18:44]

lgtm

[nisse-webrtc, 2016-04-11 10:42:11]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2016-04-11 10:42:15]

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

[commit-bot: I haz the power, 2016-04-11 11:38:34]

Description was changed from

==========
FakeVideoTrackRenderer is a thin wrapper over
FakeVideoRenderer, only registering itself on a
VideoSourceInterface on construction and removing itself on
destruction. Let it inherit FakeVideoRenderer, instead of
proxying all methods.

BUG=webrtc:5426
==========

to

==========
FakeVideoTrackRenderer is a thin wrapper over
FakeVideoRenderer, only registering itself on a
VideoSourceInterface on construction and removing itself on
destruction. Let it inherit FakeVideoRenderer, instead of
proxying all methods.

BUG=webrtc:5426
==========

[commit-bot: I haz the power, 2016-04-11 11:38:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-11 11:38:44]

Description was changed from

==========
FakeVideoTrackRenderer is a thin wrapper over
FakeVideoRenderer, only registering itself on a
VideoSourceInterface on construction and removing itself on
destruction. Let it inherit FakeVideoRenderer, instead of
proxying all methods.

BUG=webrtc:5426
==========

to

==========
FakeVideoTrackRenderer is a thin wrapper over
FakeVideoRenderer, only registering itself on a
VideoSourceInterface on construction and removing itself on
destruction. Let it inherit FakeVideoRenderer, instead of
proxying all methods.

BUG=webrtc:5426

Committed: https://crrev.com/3858477d4b751f85cbbcc6aec3570eebe4bf8e55
Cr-Commit-Position: refs/heads/master@{#12313}
==========

[commit-bot: I haz the power, 2016-04-11 11:38:45]

Patchset 4 (id:??) landed as
https://crrev.com/3858477d4b751f85cbbcc6aec3570eebe4bf8e55
Cr-Commit-Position: refs/heads/master@{#12313}