1112382 - Remove the duplicated code for data channel tests

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Nils Ohlmeier [:drno]]

It is very painful to maintain the two test queues in templates.js as they both do pretty much the same up to the point when the PC's are connected.

We should remove the DataChannelTest Wrapper.

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: refactor_data_channel_tests.patch

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a7d322f770cb

Comment 2

[Byron Campen [:bwc]]

I'll try to get to this tomorrow.

Comment 3

[Byron Campen [:bwc]]

Comment on attachment 8537520 [details] [diff] [review]
refactor_data_channel_tests.patch

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

You'll have to rebase, since there's a NoBundle DataChannel test now. Also, I'm assuming that this patch is mostly moving stuff around, but I've noticed some problems in the moved code, a couple of which are relatively serious. At least fix the if (_allChannelsAreClosed) bug, and see what happens to the tests.

::: dom/media/tests/mochitest/dataChannel.js
@@ +183,5 @@
> +          }
> +          else {
> +            options.id = sourceChannel2.id;
> +          }
> +          var reliable = !options.ordered ? false : (options.maxRetransmits || options.maxRetransmitTime);

Uh, ordered delivery and reliability are orthogonal... is this from some old version of the spec, or something we've just added on our own?

::: dom/media/tests/mochitest/pc.js
@@ +743,5 @@
> +
> +  function _allChannelsAreClosed() {
> +    var ret = null;
> +    if (localChannel) {
> +      ret = (localChannel.readyState === "closed");

I think this would be much more readable with early returns (eg; if (localChannel && localChannel.readyState !== "closed") { return false; } )

@@ +761,5 @@
> +    if (everythingClosed) {
> +      // safety protection against events firing late
> +      return;
> +    }
> +    if (_allChannelsAreClosed) {

This should be if (_allChannelsAreClosed()); right now this will always be true, since functions themselves are "truthy".

@@ +780,5 @@
> +
> +  if ((localChannel) && (localChannel.readyState !== "closed")) {
> +    // in case of steeplechase there is no far end, so we can only poll
> +    if (remoteChannel) {
> +      remoteChannel.onclose = function () {

I'm not sure I understand why we don't set this if localChannel is already closed. Shouldn't we set both remoteChannel.onclose and localChannel.onclose regardless? Seems simpler that way, and less fragile. (For example, if localChannel is already closed, but remoteChannel is not, remoteChannel.onclose is never set, which is surely not right)

Comment 4

[Nils Ohlmeier [:drno]]

Attachment: refactor_data_channel_tests.patch

I fixed the missing function call in the if statement. Thx!

For the rest: on purpose I wanted to change as little as possible to not cause any side effects. I would prefer to open new/separate tickets for the issues you found. I think that would make it easier to track them. Would you be OK with that approach?

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bf6b4701eb55

Comment 5

[Byron Campen [:bwc]]

I'm ok with that, just don't forget the NoBundle test.

Comment 6

[Nils Ohlmeier [:drno]]

(In reply to Byron Campen [:bwc] (PTO through Jan 1) from comment #5)
> I'm ok with that, just don't forget the NoBundle test.

I sure did :-( Thanks for the reminder :-)

Comment 7

[Nils Ohlmeier [:drno]]

Attachment: refactor_data_channel_tests.patch

Fixed the nobundle data channel test as pointed out by Byron.

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8819a710a821

Follow up bug for fixing the issues pointed out in comment #3: 1115769

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8541785 [details] [diff] [review]
refactor_data_channel_tests.patch

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

lgtm

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0dfb8c9e7e64

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0dfb8c9e7e64