858135 - Update LocalMediaStreamPlayback inheritance to use Object.create()

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

While working on the datachannel tests and the inheritance for PeerConnectionTest -> DataChannelTest I have encountered problems because when using the prototype inheritance we call the constructor twice. As result we get 4 peer connection objects. To circumvent that we have to use the new way of inheritance (with ECMAScript 5) via Object.create() which does NOT call the constructor of the base class.

While this will be part of my patch for bug 796894 I thought that we should handle the mediaplayback module separately. That gives us consistency across our framework classes.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v1

Comment 2

[Jason Smith [:jsmith]]

Comment on attachment 733422 [details] [diff] [review]
Patch v1

Review of attachment 733422 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine.

::: dom/media/tests/mochitest/mediaStreamPlayback.js
@@ +191,3 @@
>     */
> +  playMediaWithStreamStop : {
> +    value: function (isResume, onSuccess, onError) {

We really have to define value here on each of these functions? Syntax feels a bit odd...

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 733422 [details] [diff] [review]
Patch v1

Review of attachment 733422 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/mochitest/mediaStreamPlayback.js
@@ +191,3 @@
>     */
> +  playMediaWithStreamStop : {
> +    value: function (isResume, onSuccess, onError) {

Yes, additional attributes to define the scope of attached properties are enumerable, configurable, and writable. But all of them we don't need here. See https://developer.mozilla.org/en-US/docs/JavaScript/Guide/Inheritance_Revisited

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Asking for checkin given that inbound is currently closed.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41f71920a83b

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/41f71920a83b

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Jason, I don't think we need this for 22.0, but if you think it will be good for you, please nominated.

Comment 8

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #7)
> Jason, I don't think we need this for 22.0, but if you think it will be good
> for you, please nominated.

Ah okay. I wasn't planning on uplifting this one actually.