Implement the "needs-ice-restart" logic for SetConfiguration.

Implement the "needs-ice-restart" logic for SetConfiguration.

Changing the configuration will cause subsequently generated offers to change
the ufrag/pwd as necessary, so that a new round of gathering is started that
uses the new configuration.

This CL also makes some minor unrelated changes: changing the reference SDP in
the PC tests to more match what we generate, and relaxing the network thread
requirement for JsepTransport (since there's no reason the "needs-ice-restart"
flag can't be accessed from the signaling thread).

BUG=webrtc:6714
Committed: https://crrev.com/d1a38b591d85ba144d5d53298fdd1e453ab43ead
Cr-Commit-Position: refs/heads/master@{#15527}

[Taylor Brandstetter, 2016-12-09 22:15:53]

deadbeef@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[Taylor Brandstetter, 2016-12-09 22:15:53]

PTAL Peter. Some Blink people were reluctant to ship setConfiguration without
this implemented.

https://codereview.webrtc.org/2563153002/diff/1/webrtc/api/peerconnectioninte...
File webrtc/api/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/2563153002/diff/1/webrtc/api/peerconnectioninte...
webrtc/api/peerconnectioninterface_unittest.cc:2889: }
Note that this test was passing even before I implemented the needs-ice-restart
logic, due to this bug: https://bugs.chromium.org/p/webrtc/issues/detail?id=6351

So maybe we should fix that bug first, since otherwise this test is useless...

[pthatcher1, 2016-12-10 02:26:32]

lgtm, with nits

https://codereview.webrtc.org/2563153002/diff/20001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/2563153002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface_unittest.cc:2773: }
Unimportant idea: if you enabled max-bundle, couldn't you just have one ufrag? 
Would make this code slightly shorter.

https://codereview.webrtc.org/2563153002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface_unittest.cc:2809:
CreateAnswerAsRemoteDescription(kSdpStringWithStream1);
Would it make sense to stick all of the above in some setup method?

https://codereview.webrtc.org/2563153002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface_unittest.cc:2817: }
Would it make sense to have helper method to grab ufrags?  Would make the actual
thing being tested a larger percentage of the code.

https://codereview.webrtc.org/2563153002/diff/20001/webrtc/api/webrtcsession.h
File webrtc/api/webrtcsession.h (right):

https://codereview.webrtc.org/2563153002/diff/20001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.h:289: bool NeedsIceRestart(const std::string&
transport_name) const;
Can we just call these MIDs?  If not, why is it content_name in one place and
transport_name in the other?

https://codereview.webrtc.org/2563153002/diff/20001/webrtc/p2p/base/jseptrans...
File webrtc/p2p/base/jseptransport.h (right):

https://codereview.webrtc.org/2563153002/diff/20001/webrtc/p2p/base/jseptrans...
webrtc/p2p/base/jseptransport.h:293: // SetXTransportDescription is not in
progress.
And if SetXTransportDescription is in progress, what happens?

https://codereview.webrtc.org/2563153002/diff/20001/webrtc/p2p/base/jseptrans...
webrtc/p2p/base/jseptransport.h:296: // occurred yet for this transport.
"no ICE restart has occured", meaning that an ICE restart has been proposed (by
local or remote) or finalized (by both)?  This may not matter now, but it will
when we add rollback.

https://codereview.webrtc.org/2563153002/diff/20001/webrtc/p2p/base/jseptrans...
webrtc/p2p/base/jseptransport.h:359: // needs-ice-restart bit as described in
JSEP.
A summary of what it means would be nice, even if just one sentence.

[Taylor Brandstetter, 2016-12-10 19:00:35]

https://codereview.webrtc.org/2563153002/diff/20001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/2563153002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface_unittest.cc:2773: }
On 2016/12/10 02:26:31, pthatcher1 wrote:
> Unimportant idea: if you enabled max-bundle, couldn't you just have one ufrag?

> Would make this code slightly shorter.

I'd prefer not to change what's tested just to shorten the code.

https://codereview.webrtc.org/2563153002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface_unittest.cc:2809:
CreateAnswerAsRemoteDescription(kSdpStringWithStream1);
On 2016/12/10 02:26:31, pthatcher1 wrote:
> Would it make sense to stick all of the above in some setup method?

I don't think so, we already have way too many helper methods that make it hard
to see what tests are testing.

https://codereview.webrtc.org/2563153002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface_unittest.cc:2817: }
On 2016/12/10 02:26:31, pthatcher1 wrote:
> Would it make sense to have helper method to grab ufrags?  Would make the
actual
> thing being tested a larger percentage of the code.

In this case I agree; done.

https://codereview.webrtc.org/2563153002/diff/20001/webrtc/api/webrtcsession.h
File webrtc/api/webrtcsession.h (right):

https://codereview.webrtc.org/2563153002/diff/20001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.h:289: bool NeedsIceRestart(const std::string&
transport_name) const;
On 2016/12/10 02:26:31, pthatcher1 wrote:
> Can we just call these MIDs?  If not, why is it content_name in one place and
> transport_name in the other?

I'll call it content_name here too for consistency. We can change it to "mid"
later, doing it everywhere at once if we do it.

https://codereview.webrtc.org/2563153002/diff/20001/webrtc/p2p/base/jseptrans...
File webrtc/p2p/base/jseptransport.h (right):

https://codereview.webrtc.org/2563153002/diff/20001/webrtc/p2p/base/jseptrans...
webrtc/p2p/base/jseptransport.h:293: // SetXTransportDescription is not in
progress.
On 2016/12/10 02:26:31, pthatcher1 wrote:
> And if SetXTransportDescription is in progress, what happens?

Undefined behavior. So don't do that.

https://codereview.webrtc.org/2563153002/diff/20001/webrtc/p2p/base/jseptrans...
webrtc/p2p/base/jseptransport.h:296: // occurred yet for this transport.
On 2016/12/10 02:26:31, pthatcher1 wrote:
> "no ICE restart has occured", meaning that an ICE restart has been proposed
(by
> local or remote) or finalized (by both)?  This may not matter now, but it will
> when we add rollback.

I clarified the comments. As for rollback, there are tons of things we'll need
to figure out when we start supporting that. My understanding is that a rollback
reverts *all* state unless otherwise stated (such as the addTrack weirdness),
which would include the needs-ice-restart flag.

https://codereview.webrtc.org/2563153002/diff/20001/webrtc/p2p/base/jseptrans...
webrtc/p2p/base/jseptransport.h:359: // needs-ice-restart bit as described in
JSEP.
On 2016/12/10 02:26:31, pthatcher1 wrote:
> A summary of what it means would be nice, even if just one sentence.

Doesn't need to be here, since this is a private variable where the comment is
just a small reminder about what it's for. But I added more comments for
SetNeedsIceRestartFlag.

[Taylor Brandstetter, 2016-12-10 19:01:30]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-12-10 19:01:30]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2563153002/#ps40001 (title:
"Addressing comments (adding helper method, adding comments, renaming)")

[commit-bot: I haz the power, 2016-12-10 19:01:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-12-10 19:21:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 19:21:18]

Try jobs failed on following builders:
  win_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/14268)

[Taylor Brandstetter, 2016-12-10 20:57:43]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-12-10 20:57:43]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2563153002/#ps60001 (title:
"Fixing signed/unsigned comparison warning.")

[commit-bot: I haz the power, 2016-12-10 20:57:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-12-10 21:15:34]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481403463360450,
"parent_rev": "624745e79f8d03452b132d33b339502d196a33d8", "commit_rev":
"8c4d53d7523c7127a6f38e9ce4ff5a09bb79c870"}

[commit-bot: I haz the power, 2016-12-10 21:15:37]

Description was changed from

==========
Implement the "needs-ice-restart" logic for SetConfiguration.

Changing the configuration will cause subsequently generated offers to change
the ufrag/pwd as necessary, so that a new round of gathering is started that
uses the new configuration.

This CL also makes some minor unrelated changes: changing the reference SDP in
the PC tests to more match what we generate, and relaxing the network thread
requirement for JsepTransport (since there's no reason the "needs-ice-restart"
flag can't be accessed from the signaling thread).

BUG=webrtc:6714
==========

to

==========
Implement the "needs-ice-restart" logic for SetConfiguration.

Changing the configuration will cause subsequently generated offers to change
the ufrag/pwd as necessary, so that a new round of gathering is started that
uses the new configuration.

This CL also makes some minor unrelated changes: changing the reference SDP in
the PC tests to more match what we generate, and relaxing the network thread
requirement for JsepTransport (since there's no reason the "needs-ice-restart"
flag can't be accessed from the signaling thread).

BUG=webrtc:6714

Review-Url: https://codereview.webrtc.org/2563153002
==========

[commit-bot: I haz the power, 2016-12-10 21:15:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-10 21:15:45]

Description was changed from

==========
Implement the "needs-ice-restart" logic for SetConfiguration.

Changing the configuration will cause subsequently generated offers to change
the ufrag/pwd as necessary, so that a new round of gathering is started that
uses the new configuration.

This CL also makes some minor unrelated changes: changing the reference SDP in
the PC tests to more match what we generate, and relaxing the network thread
requirement for JsepTransport (since there's no reason the "needs-ice-restart"
flag can't be accessed from the signaling thread).

BUG=webrtc:6714

Review-Url: https://codereview.webrtc.org/2563153002
==========

to

==========
Implement the "needs-ice-restart" logic for SetConfiguration.

Changing the configuration will cause subsequently generated offers to change
the ufrag/pwd as necessary, so that a new round of gathering is started that
uses the new configuration.

This CL also makes some minor unrelated changes: changing the reference SDP in
the PC tests to more match what we generate, and relaxing the network thread
requirement for JsepTransport (since there's no reason the "needs-ice-restart"
flag can't be accessed from the signaling thread).

BUG=webrtc:6714

Committed: https://crrev.com/d1a38b591d85ba144d5d53298fdd1e453ab43ead
Cr-Commit-Position: refs/heads/master@{#15527}
==========

[commit-bot: I haz the power, 2016-12-10 21:15:46]

Patchset 4 (id:??) landed as
https://crrev.com/d1a38b591d85ba144d5d53298fdd1e453ab43ead
Cr-Commit-Position: refs/heads/master@{#15527}