Closed Bug 858135 Opened 9 years ago Closed 9 years ago

Update LocalMediaStreamPlayback inheritance to use Object.create()

Categories

(Core :: WebRTC, defect)

18 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: whimboo, Assigned: whimboo)

Details

(Whiteboard: [WebRTC][blocking-webrtc-][tests][qa-])

Attachments

(1 file)

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.
Attached patch Patch v1Splinter Review
Attachment #733422 - Flags: feedback?(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...
Attachment #733422 - Flags: feedback?(jsmith) → feedback+
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
Attachment #733422 - Flags: review?(rjesup)
Whiteboard: [WebRTC][tests] → [WebRTC][blocking-webrtc-][tests]
Attachment #733422 - Flags: review?(rjesup) → review+
Asking for checkin given that inbound is currently closed.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/41f71920a83b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Whiteboard: [WebRTC][blocking-webrtc-][tests] → [WebRTC][blocking-webrtc-][tests][qa-]
Jason, I don't think we need this for 22.0, but if you think it will be good for you, please nominated.
(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.
You need to log in before you can comment on or make changes to this bug.