1133866 - Some light refactoring would be useful in JsepSessionImpl

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P3, major)

Description

[Byron Campen [:bwc]]

There are some functions in JsepSessionImpl that should be broken down into smaller pieces.

Comment 1

[Byron Campen [:bwc]]

Attachment: Some refactoring and simplification in JsepSessionImpl

Comment 2

[Byron Campen [:bwc]]

Attachment: Some refactoring and simplification in JsepSessionImpl

Comment 3

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c1ec6a179f2

Comment 4

[Byron Campen [:bwc]]

Attachment: Some refactoring and simplification in JsepSessionImpl

Comment 5

[Byron Campen [:bwc]]

Attachment: Some refactoring and simplification in JsepSessionImpl

Comment 6

[Byron Campen [:bwc]]

Attachment: MozReview Request: bz://1133866/bwc

/r/4311 - Bug 1133866: Some refactoring and simplification in JsepSessionImpl

Pull down this commit:

hg pull review -r 231b1da46ba4332d2de318c5e6210e6f616b2914

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8569250 [details]
MozReview Request: bz://1133866/bwc

https://reviewboard.mozilla.org/r/4309/#review3513

Looks like a strict improvement.

Looking at that header file, I'm thinking that JsepSessionImpl knows and does far too much.  Maybe that's something to look at next.  I don't know what the best thing to cut out would be though: SDP construction, negotiation pieces, and transport handling all seem like candidates.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> -    ++numRecv;
> +    if (offerToReceive->isSome() && **offerToReceive) {
> +      *offerToReceive = Some(**offerToReceive - 1);
> +    }

This doesn't even scare you a little? :)

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> -        if (streamIdEnd == std::string::npos) {
> +  if (streamIdEnd == std::string::npos) {
> -          streamIdEnd = i->attribute.size();
> +    streamIdEnd = msidAttribute.size();
> -        }
> +  }
>  
> -        size_t trackIdStart =
> +  size_t trackIdStart =
> -          i->attribute.find_first_not_of(" \t", streamIdEnd);
> +    msidAttribute.find_first_not_of(" \t", streamIdEnd);
> -        if (trackIdStart == std::string::npos) {
> +  if (trackIdStart == std::string::npos) {
> -          trackIdStart = i->attribute.size();
> +    trackIdStart = msidAttribute.size();
> -        }
> +  }

No point looking for a trackId if the streamId consumes the whole string.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
>    for (auto i = mLocalTracks.begin(); i != mLocalTracks.end(); ++i) {
>      if (!i->mAssignedMLine.isSome()) {
>        continue;
>      }
>  
>      // Get rid of all m-line assignments that have not been executed by a call
>      // to SetLocalDescription.
>      if (!i->mSetInLocalDescription) {
>        i->mAssignedMLine.reset();
> +      continue;
>      }
>  
>      if (!offer.GetMediaSection(*i->mAssignedMLine).IsReceiving()) {
>        i->mAssignedMLine.reset();
>      }
>    }

I'd change this differently.

    for (auto i = mLocalTracks.begin(); i != mLocalTracks.end(); ++i) {
      if (TrackIsNotNegotiated(offer, *i)) {
        i->mAssignedMLine.reset();
      }
    }

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> -        NS_ASSERTION(false, "Failed to find remote track for level");
> +      NS_ASSERTION(false, "Failed to find remote track for level");

Are we using NS_ASSERTION and MOZ_ASSERT in the same file?

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> +JsepSessionImpl::CopyStickyParams(const SdpMediaSection& source,
> +                                            SdpMediaSection* dest)

nit: align

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> -        size_t streamIdStart = i->attribute.find_first_not_of(" \t", 5);
> +  size_t streamIdStart = msidAttribute.find_first_not_of(" \t", 5);

I didn't notice this before, but we should note the divergence here from the spec.  There is no space permitted after the colon.  (Also the general acceptance of tab instead of space, which I would prefer we didn't do).

Comment 8

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/4309/#review3571

The tricky bit when breaking this up is going to be handling the error reporting in a graceful way. I'll have to think about it.

> Are we using NS_ASSERTION and MOZ_ASSERT in the same file?

Yeah, that was something jesup asked for.

> No point looking for a trackId if the streamId consumes the whole string.

I guess I could move the setting of the outparams earlier so we can bail out here.

> This doesn't even scare you a little? :)

Most of the readability problem here is the API of Maybe. It is profoundly unintuitive. I might be able to pass a size_t* (which is null if not set), but that would make the code at the callsite slightly harder to read. Let me see.

> I'd change this differently.
> 
>     for (auto i = mLocalTracks.begin(); i != mLocalTracks.end(); ++i) {
>       if (TrackIsNotNegotiated(offer, *i)) {
>         i->mAssignedMLine.reset();
>       }
>     }

Hmm. That name isn't quite right though. We could call it ShouldUnbindTrackGivenRemoteOffer, but that's getting a little wordy, and still provides little insight into when it might return true.

Comment 9

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/4309/#review3625

> I guess I could move the setting of the outparams earlier so we can bail out here.

I tried this, and it seemed less readable to me, and was more code.

Comment 10

[Byron Campen [:bwc]]

Final try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf8adbc30bf4

Comment 11

[Byron Campen [:bwc]]

Check back on try

Comment 12

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8150bce392cf

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8150bce392cf

Comment 14

[Byron Campen [:bwc]]

Comment on attachment 8569250 [details]
MozReview Request: bz://1133866/bwc

Approval Request Comment

See approval request in bug 1144432, as this is a prerequisite.

Comment 15

[Byron Campen [:bwc]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/27402eebc291

Comment 17

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1133866: Some refactoring and simplification in JsepSessionImpl