Closed
Bug 1133866
Opened 8 years ago
Closed 8 years ago
Some light refactoring would be useful in JsepSessionImpl
Categories
(Core :: WebRTC: Signaling, defect, P3)
Core
WebRTC: Signaling
Tracking
()
RESOLVED
FIXED
mozilla39
People
(Reporter: bwc, Assigned: bwc)
Details
Attachments
(2 files, 4 obsolete files)
49.36 KB,
patch
|
Details | Diff | Splinter Review | |
39 bytes,
text/x-review-board-request
|
mt
:
review+
|
Details |
There are some functions in JsepSessionImpl that should be broken down into smaller pieces.
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8565713 -
Attachment is obsolete: true
Assignee | ||
Comment 3•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c1ec6a179f2
Assignee | ||
Comment 4•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8565764 -
Attachment is obsolete: true
Updated•8 years ago
|
Severity: normal → major
Flags: firefox-backlog+
Priority: -- → P3
Assignee | ||
Comment 5•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8568278 -
Attachment is obsolete: true
Assignee | ||
Comment 6•8 years ago
|
||
/r/4311 - Bug 1133866: Some refactoring and simplification in JsepSessionImpl Pull down this commit: hg pull review -r 231b1da46ba4332d2de318c5e6210e6f616b2914
Assignee | ||
Updated•8 years ago
|
Attachment #8569250 -
Flags: review?(martin.thomson)
Updated•8 years ago
|
Attachment #8569250 -
Flags: review?(martin.thomson) → review+
Comment 7•8 years ago
|
||
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).
Assignee | ||
Comment 8•8 years ago
|
||
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.
Assignee | ||
Comment 9•8 years ago
|
||
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.
Assignee | ||
Comment 10•8 years ago
|
||
Final try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf8adbc30bf4
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(docfaraday)
Keywords: checkin-needed
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 12•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8150bce392cf
Comment 13•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8150bce392cf
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8569250 [details] MozReview Request: bz://1133866/bwc Approval Request Comment See approval request in bug 1144432, as this is a prerequisite.
Attachment #8569250 -
Flags: approval-mozilla-aurora?
Updated•8 years ago
|
status-firefox38:
--- → affected
Updated•8 years ago
|
Attachment #8569250 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 15•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/27402eebc291
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 16•8 years ago
|
||
Attachment #8569250 -
Attachment is obsolete: true
Attachment #8619499 -
Flags: review+
Assignee | ||
Comment 17•8 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•