Fix the flaky WebRtcSessionTest.TestRtxRemovedByCreateAnswer.

Fix the flaky WebRtcSessionTest.TestRtxRemovedByCreateAnswer.

Use the attribute in MediaContentDescription to test whether Rtx is removed in the answer instead of searching the substring "rtx" in the whole answer sdp.

BUG=webrtc:4943
Committed: https://crrev.com/3a334656de92eb2ab3dd688a1bd70f0db3535f85
Cr-Commit-Position: refs/heads/master@{#12639}

[Zhi Huang, 2016-04-22 20:16:21]

Description was changed from

==========
Fix the flaky WebRtcSessionTest.TestRtxRemovedByCreateAnswer.

BUG=
==========

to

==========
Fix the flaky WebRtcSessionTest.TestRtxRemovedByCreateAnswer.

Use the attribute in MediaContentDescription to test whether Rtx is removed in
the answer instead of searching the substring "rtx" in the whole answer sdp.

BUG=webrtc:4943
==========

[Zhi Huang, 2016-04-22 22:53:30]

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

[Zhi Huang, 2016-04-22 22:53:31]

Tested 2000 times without failure.

[Taylor Brandstetter, 2016-04-23 00:05:14]

https://codereview.webrtc.org/1919523002/diff/1/webrtc/api/webrtcsession_unit...
File webrtc/api/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1919523002/diff/1/webrtc/api/webrtcsession_unit...
webrtc/api/webrtcsession_unittest.cc:4299: EXPECT_TRUE(offer_sdp.find("rtx") !=
std::string::npos);
We might as well use the same technique here to check if the offer contains an
rtx codec. So maybe make a helper method, "DescriptionContainsRtxCodec", that
returns a bool.

https://codereview.webrtc.org/1919523002/diff/1/webrtc/api/webrtcsession_unit...
webrtc/api/webrtcsession_unittest.cc:4310: for (auto iter =
mdesc->codecs().begin(); iter != mdesc->codecs().end();
nit: We can use C++11 range-based for loops in new code. So:

for (const VideoCodec& codec : mdesc->codecs()) {
...
}

https://codereview.webrtc.org/1919523002/diff/1/webrtc/api/webrtcsession_unit...
webrtc/api/webrtcsession_unittest.cc:4312:
EXPECT_TRUE(iter->ToString().find("rtx") == std::string::npos);
I think you can just check that the codec's name is not "rtx". So:

EXPECT_NE("rtx", codec.name);

[Zhi Huang, 2016-04-23 00:51:57]

https://codereview.webrtc.org/1919523002/diff/1/webrtc/api/webrtcsession_unit...
File webrtc/api/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1919523002/diff/1/webrtc/api/webrtcsession_unit...
webrtc/api/webrtcsession_unittest.cc:4299: EXPECT_TRUE(offer_sdp.find("rtx") !=
std::string::npos);
On 2016/04/23 00:05:14, Taylor Brandstetter wrote:
> We might as well use the same technique here to check if the offer contains an
> rtx codec. So maybe make a helper method, "DescriptionContainsRtxCodec", that
> returns a bool. 

Acknowledged.

https://codereview.webrtc.org/1919523002/diff/1/webrtc/api/webrtcsession_unit...
webrtc/api/webrtcsession_unittest.cc:4299: EXPECT_TRUE(offer_sdp.find("rtx") !=
std::string::npos);
On 2016/04/23 00:05:14, Taylor Brandstetter wrote:
> We might as well use the same technique here to check if the offer contains an
> rtx codec. So maybe make a helper method, "DescriptionContainsRtxCodec", that
> returns a bool. 

Done.

https://codereview.webrtc.org/1919523002/diff/1/webrtc/api/webrtcsession_unit...
webrtc/api/webrtcsession_unittest.cc:4310: for (auto iter =
mdesc->codecs().begin(); iter != mdesc->codecs().end();
On 2016/04/23 00:05:14, Taylor Brandstetter wrote:
> nit: We can use C++11 range-based for loops in new code. So:
> 
> for (const VideoCodec& codec : mdesc->codecs()) {
> ...
> } 

Done.

https://codereview.webrtc.org/1919523002/diff/1/webrtc/api/webrtcsession_unit...
webrtc/api/webrtcsession_unittest.cc:4312:
EXPECT_TRUE(iter->ToString().find("rtx") == std::string::npos);
On 2016/04/23 00:05:14, Taylor Brandstetter wrote:
> I think you can just check that the codec's name is not "rtx". So:
> 
> EXPECT_NE("rtx", codec.name);

Acknowledged.

[Taylor Brandstetter, 2016-04-23 01:25:40]

lgtm with just one small suggestion

https://codereview.webrtc.org/1919523002/diff/20001/webrtc/api/webrtcsession_...
File webrtc/api/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1919523002/diff/20001/webrtc/api/webrtcsession_...
webrtc/api/webrtcsession_unittest.cc:4320:
EXPECT_FALSE(DescriptionContainsRtxCodec(answer));
nit: Can you move this back to before "SetLocalDescriptionWithoutError" for
consistency? Also, "SetLocalDescription" takes ownership of the
SessionDescription passed into it, so technically it's not safe to use
afterwards.

[pthatcher1, 2016-05-05 20:28:34]

lgtm, although I would prefer the small improvement made before you submit.

https://codereview.webrtc.org/1919523002/diff/40001/webrtc/api/webrtcsession_...
File webrtc/api/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1919523002/diff/40001/webrtc/api/webrtcsession_...
webrtc/api/webrtcsession_unittest.cc:1116: bool
DescriptionContainsRtxCodec(const SessionDescriptionInterface* desc) {
This should have "video" in the name since it only checks video codecs.

And you could generalize it to just ContainsVideoCodecWithName(desc, "rtx").

[Zhi Huang, 2016-05-06 01:33:47]

The CQ bit was checked by zhihuang@webrtc.org

[Zhi Huang, 2016-05-06 01:33:48]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1919523002/#ps60001 (title:
"Rename the functions.")

[commit-bot: I haz the power, 2016-05-06 01:33:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1919523002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1919523002/60001

[commit-bot: I haz the power, 2016-05-06 01:37:52]

Description was changed from

==========
Fix the flaky WebRtcSessionTest.TestRtxRemovedByCreateAnswer.

Use the attribute in MediaContentDescription to test whether Rtx is removed in
the answer instead of searching the substring "rtx" in the whole answer sdp.

BUG=webrtc:4943
==========

to

==========
Fix the flaky WebRtcSessionTest.TestRtxRemovedByCreateAnswer.

Use the attribute in MediaContentDescription to test whether Rtx is removed in
the answer instead of searching the substring "rtx" in the whole answer sdp.

BUG=webrtc:4943
==========

[commit-bot: I haz the power, 2016-05-06 01:37:53]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-06 01:38:00]

Description was changed from

==========
Fix the flaky WebRtcSessionTest.TestRtxRemovedByCreateAnswer.

Use the attribute in MediaContentDescription to test whether Rtx is removed in
the answer instead of searching the substring "rtx" in the whole answer sdp.

BUG=webrtc:4943
==========

to

==========
Fix the flaky WebRtcSessionTest.TestRtxRemovedByCreateAnswer.

Use the attribute in MediaContentDescription to test whether Rtx is removed in
the answer instead of searching the substring "rtx" in the whole answer sdp.

BUG=webrtc:4943

Committed: https://crrev.com/3a334656de92eb2ab3dd688a1bd70f0db3535f85
Cr-Commit-Position: refs/heads/master@{#12639}
==========

[commit-bot: I haz the power, 2016-05-06 01:38:01]

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