Cleanup naming issue with content::VideoFrameProvider

Cleanup naming issue with content::VideoFrameProvider

The name content::VideoFrameProvider was easy to cause confusion with
cc:VideoFrameProvider. Furthermore, the methods provided in the
content::VideoFrameProvider are not specific to Video, which makes the
name somewhat unsuitable.

List of changes:
* Moved the typedef content::VideoFrameProvider::RepaintCB out to a new
  file and renamed it to VideoFrameCallback.
* Renamed interface VideoFrameProvider to PausableStream
* Renamed method Play() to Resume()

BUG=526835
Committed: https://crrev.com/bcfff02130887b36724a2cc6874cbcc792ce985d
Cr-Commit-Position: refs/heads/master@{#398071}

[chfremer, 2016-05-25 19:25:33]

Description was changed from

==========
Rename Play() to Resume()


* Renamed video_frame_provider.h to pausable_stream.h. * Moved
VideoFrameCallback to a new file.


Renamed VideoFrameProvider to PausableStream in code.

BUG=
==========

to

==========
Cleanup naming issue with content::VideoFrameProvider

The name content::VideoFrameProvider was easy to cause confusion with
cc:VideoFrameProvider. Furthermore, the methods provided in the
content::VideoFrameProvider are not specific to Video, which makes the
name somewhat unsuitable.

List of changes:
* Moved the typedef content::VideoFrameProvider::RepaintCB out to a new
  file and renamed it to VideoFrameCallback.
* Renamed interface VideoFrameProvider to PausableStream
* Renamed method Play() to Resume()

BUG=526835
==========

[chfremer, 2016-05-25 20:55:36]

chfremer@chromium.org changed reviewers:
+ emircan@chromium.org, sievers@chromium.org, tommi@chromium.org

[emircan, 2016-05-25 22:18:05]

https://codereview.chromium.org/2005053006/diff/1/content/public/renderer/pau...
File content/public/renderer/pausable_stream.h (right):

https://codereview.chromium.org/2005053006/diff/1/content/public/renderer/pau...
content/public/renderer/pausable_stream.h:13: class PausableStream
I understand that you tried to name the class based on the methods it provides.
However, I still don't think it is a good name:
- We usually use W3C defined MediaStream instead of Stream. See
content/public/renderer file names.
- This name doesn't indicate any information about what the methods do. Judging
by the what the overrides do, we can call it MediaStreamRenderer. WDYT?

Moreover, we can make this class the base for AudioRendererSink[0] in a followup
patch.
[0]
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_r...

https://codereview.chromium.org/2005053006/diff/1/content/public/renderer/vid...
File content/public/renderer/video_frame_callback.h (right):

https://codereview.chromium.org/2005053006/diff/1/content/public/renderer/vid...
content/public/renderer/video_frame_callback.h:15: VideoFrameCallback;
Instead of making a new file for it, we can keep this typedef in where it is
used.

https://codereview.chromium.org/2005053006/diff/1/content/renderer/media/webm...
File content/renderer/media/webmediaplayer_ms.h (right):

https://codereview.chromium.org/2005053006/diff/1/content/renderer/media/webm...
content/renderer/media/webmediaplayer_ms.h:60: // PausableStream
video_frame_provider_:
Rename the variable accordingly. When referencing members, use
|video_frame_provider_|.

https://codereview.chromium.org/2005053006/diff/1/content/renderer/media/webm...
content/renderer/media/webmediaplayer_ms.h:63: // blink::WebMediaPlayerClient
client_:
Ditto

https://codereview.chromium.org/2005053006/diff/1/content/renderer/media/webm...
content/renderer/media/webmediaplayer_ms.h:162: // The callback for
video_frame_provider_ to signal a new frame is available.
Ditto.

[chfremer, 2016-05-26 00:10:11]

https://codereview.chromium.org/2005053006/diff/1/content/public/renderer/pau...
File content/public/renderer/pausable_stream.h (right):

https://codereview.chromium.org/2005053006/diff/1/content/public/renderer/pau...
content/public/renderer/pausable_stream.h:13: class PausableStream
On 2016/05/25 22:18:04, emircan wrote:
> I understand that you tried to name the class based on the methods it
provides.
> However, I still don't think it is a good name:
> - We usually use W3C defined MediaStream instead of Stream. See
> content/public/renderer file names.
> - This name doesn't indicate any information about what the methods do.
Judging
> by the what the overrides do, we can call it MediaStreamRenderer. WDYT?
> 
> Moreover, we can make this class the base for AudioRendererSink[0] in a
followup
> patch.
> [0]
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_r...

I realize that there are two possible perspectives when it comes to interfaces.
One perspective is to define an interface from the perspective of the client
that calls it and is not concerned with the actual behavior of concrete
implementations (let us call this the "required interface" perspective). The
other perspective is to define an interface as a specification for behavior
offered by its implementations (let us call this the "provided interface"
perspective).

The "provided interface" perspective has the advantage of reducing the coupling
between the interface and the calling client. In our example, the calling client
should only have to care about calling Start/Stop/Resume/Pause at the right
time, and there is no need for it to know anything about callbacks with
VideoFrames being made to someone else. From this perspective, it is intentional
that the name does not suggest any more specifics on what the methods do. The
information on what the expected behavior is should then go to the factory that
creates instances exposing this interface (in our case that is
MediaStreamRendererFactory[1]). Components implementing the callback interface
to receive VideoFrames should not be forced to know anything about either this
interface or the factory interface MediaStreamRendererFactory[1]. That is why I
though it would be a good idea to move the typedef for the callback to a
separate file.

Please let me know if you disagree with using the "provided interface"
perspective in this case or with any of the reasoning behind it. It would be
very valuable to me to understand your (and other people's) point of view on
this.

I am struggling to find a proper name for and abstraction of something that can
be started, paused, resumed and stopped. I agree that the name |PausableStream|
may indeed not be great. Using the suffix "Stream" was not supposed to indicate
any connection to MediaStream or other defined terms.
Please let me know if you have any good suggestions for a better name.

Agreed to making this a base class for AudioRendereSink[0] in a folloup patch.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...

https://codereview.chromium.org/2005053006/diff/1/content/public/renderer/vid...
File content/public/renderer/video_frame_callback.h (right):

https://codereview.chromium.org/2005053006/diff/1/content/public/renderer/vid...
content/public/renderer/video_frame_callback.h:15: VideoFrameCallback;
On 2016/05/25 22:18:05, emircan wrote:
> Instead of making a new file for it, we can keep this typedef in where it is
> used.

Please see my other reply for an explanation for why I thought it makes sense to
move it to a separate file.
Components implementing this callback interface are not necessarily the same
components that create or use the component making the callbacks. Therefore,
there is no need for them to depend on anything other than the callback
interface.

https://codereview.chromium.org/2005053006/diff/1/content/renderer/media/webm...
File content/renderer/media/webmediaplayer_ms.h (right):

https://codereview.chromium.org/2005053006/diff/1/content/renderer/media/webm...
content/renderer/media/webmediaplayer_ms.h:60: // PausableStream
video_frame_provider_:
On 2016/05/25 22:18:05, emircan wrote:
> Rename the variable accordingly. When referencing members, use
> |video_frame_provider_|.

Thanks. I will fix it here and at the other places using the || notation.

Not sure about renaming the variable, though. I think it is good that the
variable name carries information about the role that this concrete
|PausableStream| instance plays in this class, so I intentionally left the
variable name as is. What would be your suggestion for a better name?

https://codereview.chromium.org/2005053006/diff/1/content/renderer/media/webm...
content/renderer/media/webmediaplayer_ms.h:63: // blink::WebMediaPlayerClient
client_:
On 2016/05/25 22:18:05, emircan wrote:
> Ditto

Acknowledged.

https://codereview.chromium.org/2005053006/diff/1/content/renderer/media/webm...
content/renderer/media/webmediaplayer_ms.h:162: // The callback for
video_frame_provider_ to signal a new frame is available.
On 2016/05/25 22:18:05, emircan wrote:
> Ditto.

Acknowledged.

[emircan, 2016-05-27 19:37:37]

https://codereview.chromium.org/2005053006/diff/1/content/public/renderer/pau...
File content/public/renderer/pausable_stream.h (right):

https://codereview.chromium.org/2005053006/diff/1/content/public/renderer/pau...
content/public/renderer/pausable_stream.h:13: class PausableStream
On 2016/05/26 00:10:11, chfremer wrote:
> On 2016/05/25 22:18:04, emircan wrote:
> > I understand that you tried to name the class based on the methods it
> provides.
> > However, I still don't think it is a good name:
> > - We usually use W3C defined MediaStream instead of Stream. See
> > content/public/renderer file names.
> > - This name doesn't indicate any information about what the methods do.
> Judging
> > by the what the overrides do, we can call it MediaStreamRenderer. WDYT?
> > 
> > Moreover, we can make this class the base for AudioRendererSink[0] in a
> followup
> > patch.
> > [0]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_r...
> 
> I realize that there are two possible perspectives when it comes to
interfaces.
> One perspective is to define an interface from the perspective of the client
> that calls it and is not concerned with the actual behavior of concrete
> implementations (let us call this the "required interface" perspective). The
> other perspective is to define an interface as a specification for behavior
> offered by its implementations (let us call this the "provided interface"
> perspective).
> 
> The "provided interface" perspective has the advantage of reducing the
coupling
> between the interface and the calling client. In our example, the calling
client
> should only have to care about calling Start/Stop/Resume/Pause at the right
> time, and there is no need for it to know anything about callbacks with
> VideoFrames being made to someone else. From this perspective, it is
intentional
> that the name does not suggest any more specifics on what the methods do. The
> information on what the expected behavior is should then go to the factory
that
> creates instances exposing this interface (in our case that is
> MediaStreamRendererFactory[1]). Components implementing the callback interface
> to receive VideoFrames should not be forced to know anything about either this
> interface or the factory interface MediaStreamRendererFactory[1]. That is why
I
> though it would be a good idea to move the typedef for the callback to a
> separate file.
> 
> Please let me know if you disagree with using the "provided interface"
> perspective in this case or with any of the reasoning behind it. It would be
> very valuable to me to understand your (and other people's) point of view on
> this.
> 
> I am struggling to find a proper name for and abstraction of something that
can
> be started, paused, resumed and stopped. I agree that the name
|PausableStream|
> may indeed not be great. Using the suffix "Stream" was not supposed to
indicate
> any connection to MediaStream or other defined terms.
> Please let me know if you have any good suggestions for a better name.
> 
> Agreed to making this a base class for AudioRendereSink[0] in a folloup patch.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...

Most of the Chrome/WebRTC designs are based on W3C draft. When naming these
classes, we usually try to be as similar as possible to the given W3C spec
description[0]. "MediaStream" definition comes from there. "Video" and "Audio"
names suggest if they correspond to the <audio> and <video> tags in HTML.
"Render" is term borrowed from there as well. We ideally would like people
reading this code to relate to that document as much as possible.

[0]
https://www.w3.org/TR/mediacapture-streams/#the-model-sources-sinks-constrain...

https://codereview.chromium.org/2005053006/diff/1/content/public/renderer/vid...
File content/public/renderer/video_frame_callback.h (right):

https://codereview.chromium.org/2005053006/diff/1/content/public/renderer/vid...
content/public/renderer/video_frame_callback.h:15: VideoFrameCallback;
On 2016/05/26 00:10:11, chfremer wrote:
> On 2016/05/25 22:18:05, emircan wrote:
> > Instead of making a new file for it, we can keep this typedef in where it is
> > used.
> 
> Please see my other reply for an explanation for why I thought it makes sense
to
> move it to a separate file.
> Components implementing this callback interface are not necessarily the same
> components that create or use the component making the callbacks. Therefore,
> there is no need for them to depend on anything other than the callback
> interface.

I agree with your understanding and actually made the note myself in the bug to
move this to a new file. However, the convention has been to include these
callbacks as class members so far. We can look at moving them in a separate
patch?

https://codereview.chromium.org/2005053006/diff/1/content/renderer/media/webm...
File content/renderer/media/webmediaplayer_ms.h (right):

https://codereview.chromium.org/2005053006/diff/1/content/renderer/media/webm...
content/renderer/media/webmediaplayer_ms.h:60: // PausableStream
video_frame_provider_:
On 2016/05/26 00:10:11, chfremer wrote:
> On 2016/05/25 22:18:05, emircan wrote:
> > Rename the variable accordingly. When referencing members, use
> > |video_frame_provider_|.
> 
> Thanks. I will fix it here and at the other places using the || notation.
> 
> Not sure about renaming the variable, though. I think it is good that the
> variable name carries information about the role that this concrete
> |PausableStream| instance plays in this class, so I intentionally left the
> variable name as is. What would be your suggestion for a better name?

I see your point about the name. However, you should still put || around class
members.

[chfremer, 2016-05-27 23:27:42]

Patchset #2 (id:20001) has been deleted

[chfremer, 2016-05-27 23:36:56]

I started from scratch in order to stay more consistent with the existing style.

Here is what I changed:
* In media_stream_renderer_fatory.h: Renamed GetVideoFrameProvider() ->
GetVideoRenderer() and renamed VideoFrameProvider -> MediaStreamVideoRenderer.
This makes it consistent with the corresponding names for the audio part.
* In media_stream_video_renderer.h: Renamed Play() -> Resume()

[emircan, 2016-05-31 18:13:13]

https://codereview.chromium.org/2005053006/diff/40001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms.h (right):

https://codereview.chromium.org/2005053006/diff/40001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms.h:201:
scoped_refptr<MediaStreamAudioRenderer> audio_renderer_;  // Weak
Can you make this one space before comment as well?

https://codereview.chromium.org/2005053006/diff/40001/content/shell/renderer/...
File content/shell/renderer/layout_test/test_video_frame_provider.h (right):

https://codereview.chromium.org/2005053006/diff/40001/content/shell/renderer/...
content/shell/renderer/layout_test/test_video_frame_provider.h:24: class
TestVideoFrameProvider : public MediaStreamVideoRenderer {
We should change this test class's name accoringly, i.w.
TestMediaStreamVideoRenderer.

[chfremer, 2016-05-31 21:05:54]

Patchset #3 (id:60001) has been deleted

[chfremer, 2016-05-31 22:09:26]

* Addressed comments.
* Added missing file name change in gypi file.

https://codereview.chromium.org/2005053006/diff/40001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms.h (right):

https://codereview.chromium.org/2005053006/diff/40001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms.h:201:
scoped_refptr<MediaStreamAudioRenderer> audio_renderer_;  // Weak
On 2016/05/31 18:13:13, emircan wrote:
> Can you make this one space before comment as well?

Done.

https://codereview.chromium.org/2005053006/diff/40001/content/shell/renderer/...
File content/shell/renderer/layout_test/test_video_frame_provider.h (right):

https://codereview.chromium.org/2005053006/diff/40001/content/shell/renderer/...
content/shell/renderer/layout_test/test_video_frame_provider.h:24: class
TestVideoFrameProvider : public MediaStreamVideoRenderer {
On 2016/05/31 18:13:13, emircan wrote:
> We should change this test class's name accoringly, i.w.
> TestMediaStreamVideoRenderer.

Done.

[emircan, 2016-05-31 22:24:10]

lgtm

[chfremer, 2016-05-31 22:24:45]

chfremer@chromium.org changed reviewers:
- sievers@chromium.org, tommi@chromium.org

[chfremer, 2016-05-31 22:27:26]

chfremer@chromium.org changed reviewers:
+ sievers@chromium.org

[chfremer, 2016-05-31 22:27:26]

sievers@chromium.org: Please do an owner's review.

[no sievers, 2016-06-01 22:35:37]

On 2016/05/31 22:27:26, chfremer wrote:
> mailto:sievers@chromium.org: Please do an owner's review.

lgtm

[chfremer, 2016-06-01 22:37:04]

The CQ bit was checked by chfremer@chromium.org

[commit-bot: I haz the power, 2016-06-01 22:38:21]

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

[commit-bot: I haz the power, 2016-06-02 01:59:31]

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

[commit-bot: I haz the power, 2016-06-02 01:59:32]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chfremer, 2016-06-02 18:33:59]

The CQ bit was checked by chfremer@chromium.org

[commit-bot: I haz the power, 2016-06-02 18:34:44]

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

[commit-bot: I haz the power, 2016-06-03 00:34:38]

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

[commit-bot: I haz the power, 2016-06-03 00:34:40]

Try jobs failed on following builders:
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[chfremer, 2016-06-06 16:16:03]

The CQ bit was checked by chfremer@chromium.org

[commit-bot: I haz the power, 2016-06-06 16:16:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2005053006/80001

[commit-bot: I haz the power, 2016-06-06 18:30:01]

Description was changed from

==========
Cleanup naming issue with content::VideoFrameProvider

The name content::VideoFrameProvider was easy to cause confusion with
cc:VideoFrameProvider. Furthermore, the methods provided in the
content::VideoFrameProvider are not specific to Video, which makes the
name somewhat unsuitable.

List of changes:
* Moved the typedef content::VideoFrameProvider::RepaintCB out to a new
  file and renamed it to VideoFrameCallback.
* Renamed interface VideoFrameProvider to PausableStream
* Renamed method Play() to Resume()

BUG=526835
==========

to

==========
Cleanup naming issue with content::VideoFrameProvider

The name content::VideoFrameProvider was easy to cause confusion with
cc:VideoFrameProvider. Furthermore, the methods provided in the
content::VideoFrameProvider are not specific to Video, which makes the
name somewhat unsuitable.

List of changes:
* Moved the typedef content::VideoFrameProvider::RepaintCB out to a new
  file and renamed it to VideoFrameCallback.
* Renamed interface VideoFrameProvider to PausableStream
* Renamed method Play() to Resume()

BUG=526835
==========

[commit-bot: I haz the power, 2016-06-06 18:30:03]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-06-06 18:31:11]

Description was changed from

==========
Cleanup naming issue with content::VideoFrameProvider

The name content::VideoFrameProvider was easy to cause confusion with
cc:VideoFrameProvider. Furthermore, the methods provided in the
content::VideoFrameProvider are not specific to Video, which makes the
name somewhat unsuitable.

List of changes:
* Moved the typedef content::VideoFrameProvider::RepaintCB out to a new
  file and renamed it to VideoFrameCallback.
* Renamed interface VideoFrameProvider to PausableStream
* Renamed method Play() to Resume()

BUG=526835
==========

to

==========
Cleanup naming issue with content::VideoFrameProvider

The name content::VideoFrameProvider was easy to cause confusion with
cc:VideoFrameProvider. Furthermore, the methods provided in the
content::VideoFrameProvider are not specific to Video, which makes the
name somewhat unsuitable.

List of changes:
* Moved the typedef content::VideoFrameProvider::RepaintCB out to a new
  file and renamed it to VideoFrameCallback.
* Renamed interface VideoFrameProvider to PausableStream
* Renamed method Play() to Resume()

BUG=526835

Committed: https://crrev.com/bcfff02130887b36724a2cc6874cbcc792ce985d
Cr-Commit-Position: refs/heads/master@{#398071}
==========

[commit-bot: I haz the power, 2016-06-06 18:31:12]

Patchset 3 (id:??) landed as
https://crrev.com/bcfff02130887b36724a2cc6874cbcc792ce985d
Cr-Commit-Position: refs/heads/master@{#398071}