Robert, thanks for your review. Justin, thanks for your response. I have entered a Yes ballot.
Alissa > On Aug 26, 2017, at 2:10 PM, Justin Uberti <[email protected]> wrote: > > Thanks for this careful review. A new version of the document (-22) has been > posted which addresses almost all of these comments, with a few exceptions > noted below. > > Diff: > https://www.ietf.org/rfcdiff?url1=draft-ietf-rtcweb-jsep-21&url2=draft-ietf-rtcweb-jsep-22&difftype=--html > > <https://www.ietf.org/rfcdiff?url1=draft-ietf-rtcweb-jsep-21&url2=draft-ietf-rtcweb-jsep-22&difftype=--html> > > On Tue, Aug 8, 2017 at 12:31 PM, Robert Sparks <[email protected] > <mailto:[email protected]>> wrote: > Reviewer: Robert Sparks > Review result: Almost Ready > > I am the assigned Gen-ART reviewer for this draft. The General Area > Review Team (Gen-ART) reviews all IETF documents being processed > by the IESG for the IETF Chair. Please treat these comments just > like any other last call comments. > > For more information, please see the FAQ at > > <https://trac.ietf.org/trac/gen/wiki/GenArtfaq > <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>>. > > Document: draft-ietf-rtcweb-jsep-21 > Reviewer: Robert Sparks > Review Date: 2017-08-08 > IETF LC End Date: 2017-08-11 > IESG Telechat date: Not scheduled for a telechat > > Summary: Almost ready for publication as a Proposed Standard RFC, but with > issues that need to be addressed before publishing > > Major issues: > > The last paragraph of 5.1 needs more context. Why woudn't you expect the > re-offer to fail, given that you already know the peer is using the older > profile strings? It's not clear from the email list and proceedings how the > document ended up with this particular text. > > Minor issues: > > Second paragraph of 3.2, last sentence: at "these parameters or reject > them" - "these" and "them" have ambiguous antecedents. It's not clear if you > are trying to point to the parameters that don't follow the earlier stated > rule, or all parameters. The sentence is complex. Splitting it up and > finding a way to avoid the need for the semicolon will likely make it > easier to say what you mean. > > Last paragraph on page 5 - first sentence: at "One way to mitigate this" - > "this" has an unclear antecedent. Did you mean "One way to reduce the > complexity of the application's responsibilities"? > > Last paragraph of 3.5.2.1 - "all of these fields" is ambiguous. Please > state precisely which fields you mean. > > Odd edge: The description createAnswer (in 4.1.7) leaves open the > possibility that the application could call createAnswer twice > (back-to-back with no intervening calls to other APIs) with different > values in the options parameter. Is this ok? If not, should there be > language that says not to do that somewhere? > > Slightly more than minor maybe: The first paragraph of 5.1.2 has at least a > reference error. UDP/TLS/RTP/SAVPF is not specified in RFC7850. It's in > RFC5764. Assuming that this is only a reference error, then the last > sentence seems like a stray. It doesn't seem to be in the right context > ("this advertisement" doesn't cast back to the discussion of the two > profiles above it, which are both UDP only). Please clarify the sentence, > giving it the right context, or remove it. > > The fourth sentence of the second bullet of section 5.2.1 is confusing. (It > starts "It is RECOMMENDED that the <sess-id>".) RFC3265 requires the > initial sess-id high-order bit to be zero. As written, this can be read to > say that setting that bit to zero is only RECOMMENDED, and that it's > possible to have sess-ids that are something other than 64 bits. You mean > to say "Do what 3265 says to do and make the lower 63 bits random". Here's > one possible replacement for the sentence: "RFC 3265 requires that the > high-order bit of the initial <sess-id> be zero. It is RECOMMENDED that the > remaining 63 bits be initially set to a cryptographically random value." > > The last paragraph of 5.3.1 prohibits the same attributes from generated > answers that are prohibited in generated offers. Consider adding text for > what to do if an offer shows up from a peer with some of those prohibited > attributes. > > This is covered by the last paragraph on page 43: > > Note that these requirements are in some cases stricter than those of > SDP. Implementations MUST be prepared to accept compliant SDP even > if it would not conform to the requirements for generating SDP in > this specification. > > > The second paragraph of section 5.4 is confusing. A slight restructuring of > the sentences would remove much of the potential confusion, I think. I > suggest replacing the first sentence (which is very complex) with this: > "After calling setLocalDescription, the application MAY modify the SDP to > reduce the capabilities in the offer before sending it to the far side, as > long as it remains a valid SDP offer and specifies a subset of what was in > the original offer. Likewise, an application that has received an offer > from a peer MAY modify the received SDP, subject to those same constraints, > before calling setRemoteDescription." > > The first paragraph of section 5.7.1 is simply restating requirements from > RFC4566. Restating normative requirements like that has led to interop > problems through unintended introduced ambiguity and spec maintenance > problems. Please consider rewriting the paragraph to say "RFC4566 says" and > avoid the 2119 keywords. > > We considered this, but found that a) RFC4566 is clearly cited, and b) the > actual restatement was very minor, really just affecting the discussion of > the v= and o= lines. As a result we concluded that pointing the reader > elsewhere just for those two lines did not seem like an improvement. > > > On page 69, at "If the application attempt to transmit DTMF when using a > media format that does not have a corresponding telephone-event format, > this MUST result in an error". This sentence is out-of-place. The section > is about applying a remote description. If the document needs to retain the > sentence, it belongs off in some "processing media" section. In particular, > the current placement of text is confusing about what and when an error > could be returned, and it could be misread to say that an error should be > returned to the setRemoteDescription call. > > Nits: > > Section 1.2 second paragraph - "This was rejected based on a feeling". Can > you write something that speaks to consensus here? "Based on a feeling" > isn't quite right. Maybe "using the argument"? > > PeerConnection is used first at the next to last paragraph of section 3. > Please add a reference at that point. > > "LS" (lipsync) is first used at 4.1.2, with no context for what it is. > Please add a reference. > > The concept of a "pending local description" is used in sections 3.5.1, > 4.1.6, and 4.1.7 but isn't really explained until much later in the > document. Please provide forward references, or move a description of what > a pending description is earlier in the document. > > In 4.1.7 (create Answer) why would calling setLocalDescription cause the > answer to be a _pending_ local description. I think it's the full on local > description at that point, not a pending one? > > At 4.1.17's first paragraph: Why is "if parsed successfully" necessary to > call out? I suggest removing the phrase. > > 4.2.3 talks about setting direction properties, but the values that can be > set aren't called out (and even then only by inference from a list in an > example) in section 4.2.5. Should there be explicit pointers to the api doc > here? (And in section 4.2.6?) > > There are two paragraphs on page 50 that start "The next step is to". They > are both referring to the same step. I suggest combining the paragraphs. > > The 4th bullet on page 64 is ambiguous at "this m= section". It looks like > this bullet should just be merged with the previous bullet. > > The second paragraph of 6.8 starts "Next, ". I think it should start > "First, ". > > It's not clear what the "Or," in the first bullet on page 65 is referring > to. Are you trying to say "If the m= section is not new, and the ICE ufrag > and password values have changed" or something else? > > The intent here is as stated, "if the m= section is not new and the values > have changed". We discussed this but concluded that the meaning of the > current text was clear. > > > In the last paragraph of section 8 at "programmer MUST recognize". That is > not an appropriate use of 2119. I suggest "needs to be aware" instead of > "MUST recognize". > > Micro Nits: > > Suggested change to the first sentence of 1.1 > > OLD: > > The thinking behind WebRTC call setup has been to fully specify and > control the media plane, but to leave the signaling plane up to the > application as much as possible. > > New: > > WebRTC call setup has been designed to focus on controlling the media > plane, leaving signaling plane behavior up to the application as much as > possible. > > The first full bullet on page 18 has a sentence that starts "Note that when > considering". Please change that to simply "When considering". As written, > there is an implication that this is a consequence of something specified > somewhere else. > > The verb "surfaced" appears three times in the document without support or > definition. Can you replace them with simpler language? > > At the second to last sub-bullet at the end of page 66, please remove "note > that". The sentence is stronger without it. > > I noted these sections as particularly hard to read when going through the > document linearly. I don't have concrete suggestions for improvement, but > perhaps other eyes (if they agree the sections could use improvement) can > come up with something: > - First sentence of the first paragraph on page 19. > - 2nd sentence of the 4th paragraph on page 19. > > > _______________________________________________ > rtcweb mailing list > [email protected] <mailto:[email protected]> > https://www.ietf.org/mailman/listinfo/rtcweb > <https://www.ietf.org/mailman/listinfo/rtcweb> > > _______________________________________________ > Gen-art mailing list > [email protected] > https://www.ietf.org/mailman/listinfo/gen-art
_______________________________________________ Gen-art mailing list [email protected] https://www.ietf.org/mailman/listinfo/gen-art
