Add the URL attribute to cricket::Candiate.

Add the URL attribute to cricket::Candiate.

The URL of the ICE server will be reconstructed by the Port and the URL
attribute is added to the cricket::Candidate struct so that we can tell
which ICE server the candidate was gathered from.

This CL only changes the native C++ code. The Java and Objc wrapper will
be created in separate CLs.

BUG=webrtc::7128

Review-Url: https://codereview.webrtc.org/2685053004
Cr-Commit-Position: refs/heads/master@{#16591}
Committed: https://chromium.googlesource.com/external/webrtc/+/26d99c2e280e3e0cf3db50623b5ab18990f080e7

[Zhi Huang, 2017-02-10 01:35:36]

Patchset #1 (id:1) has been deleted

[Taylor Brandstetter, 2017-02-10 01:58:45]

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

[Taylor Brandstetter, 2017-02-10 01:58:46]

Looks good, just some minor suggestions

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/candidate.h
File webrtc/p2p/base/candidate.h (right):

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/candidate...
webrtc/p2p/base/candidate.h:186: // The URL of ICE server which this candidate
is gathered from.
nit: Add "the" before "ICE server"

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:255: std::string url,
nit: const ref parameter

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:335: std::string url,
nit: const ref parameter

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/stunport_...
File webrtc/p2p/base/stunport_unittest.cc (right):

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/stunport_...
webrtc/p2p/base/stunport_unittest.cc:204: std::string expected_server_url =
"stun:" + kStunAddr1.ToString();
nit: This test relies on SocketAddress::ToString()'s implementation not
changing. It may be better to do:

CreateStunPort(rtc::SocketAddress("127.0.0.1", 5000));
...
std::string expected_server_url = "stun:127.0.0.1:5000";

So that the test expectation is more explicit.

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:1093: }
nit: A switch statement would be better here, so that if a new protocol is
added, there will be a compile warning and someone will know to update this
function.

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/turnport_...
File webrtc/p2p/base/turnport_unittest.cc (right):

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/turnport_...
webrtc/p2p/base/turnport_unittest.cc:658: "turn:" +
kTurnUdpProtoAddr.address.ToString() + "?transport=udp";
See my comment on stunport_unittest.cc

[Zhi Huang, 2017-02-10 06:45:50]

Please take another look. Thanks.

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/candidate.h
File webrtc/p2p/base/candidate.h (right):

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/candidate...
webrtc/p2p/base/candidate.h:186: // The URL of ICE server which this candidate
is gathered from.
On 2017/02/10 01:58:45, Taylor Brandstetter wrote:
> nit: Add "the" before "ICE server"

Done.

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:255: std::string url,
On 2017/02/10 01:58:46, Taylor Brandstetter wrote:
> nit: const ref parameter

Done.

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:335: std::string url,
On 2017/02/10 01:58:46, Taylor Brandstetter wrote:
> nit: const ref parameter

Done.
I thought the "std::string url" is created locally so it might need to be copied
but I noticed that I was wrong.

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/stunport_...
File webrtc/p2p/base/stunport_unittest.cc (right):

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/stunport_...
webrtc/p2p/base/stunport_unittest.cc:204: std::string expected_server_url =
"stun:" + kStunAddr1.ToString();
On 2017/02/10 01:58:46, Taylor Brandstetter wrote:
> nit: This test relies on SocketAddress::ToString()'s implementation not
> changing. It may be better to do:
> 
> CreateStunPort(rtc::SocketAddress("127.0.0.1", 5000));
> ...
> std::string expected_server_url = "stun:127.0.0.1:5000";
> 
> So that the test expectation is more explicit.

Agreed.
If we expect that the ToString() might change, then I'll change the code that
re-constructs the url as well because it also use the ToString() method.

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:1093: }
On 2017/02/10 01:58:46, Taylor Brandstetter wrote:
> nit: A switch statement would be better here, so that if a new protocol is
> added, there will be a compile warning and someone will know to update this
> function.

Agreed. This would be better!

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/turnport_...
File webrtc/p2p/base/turnport_unittest.cc (right):

https://codereview.webrtc.org/2685053004/diff/20001/webrtc/p2p/base/turnport_...
webrtc/p2p/base/turnport_unittest.cc:658: "turn:" +
kTurnUdpProtoAddr.address.ToString() + "?transport=udp";
On 2017/02/10 01:58:46, Taylor Brandstetter wrote:
> See my comment on stunport_unittest.cc

Done.

[Taylor Brandstetter, 2017-02-10 16:59:50]

https://codereview.webrtc.org/2685053004/diff/40001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/2685053004/diff/40001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:1097: default:
nit: Actually, you don't need the default case here. This is something I learned
recently: we have compile warnings if there is an unhandled enum in a switch
statement (-Wswitch), so by leaving out "default", you have a compile-time check
that all the enum's values are handled.

[Zhi Huang, 2017-02-10 18:57:14]

Please take another look. Thanks.

https://codereview.webrtc.org/2685053004/diff/40001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/2685053004/diff/40001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:1097: default:
On 2017/02/10 16:59:50, Taylor Brandstetter wrote:
> nit: Actually, you don't need the default case here. This is something I
learned
> recently: we have compile warnings if there is an unhandled enum in a switch
> statement (-Wswitch), so by leaving out "default", you have a compile-time
check
> that all the enum's values are handled.

Oh I see. Good to know that! Thanks.
Done.

[Taylor Brandstetter, 2017-02-11 00:44:21]

lgtm

[Zhi Huang, 2017-02-13 18:37:29]

The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 18:37:35]

Dry run: 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, 2017-02-13 20:30:11]

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

[commit-bot: I haz the power, 2017-02-13 20:30:12]

Dry run: This issue passed the CQ dry run.

[Zhi Huang, 2017-02-13 20:44:04]

The CQ bit was checked by zhihuang@webrtc.org

[Zhi Huang, 2017-02-13 20:44:05]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2685053004/#ps80001 (title:
"Merge")

[commit-bot: I haz the power, 2017-02-13 20:44:10]

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, 2017-02-13 20:47:28]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487018644997930,
"parent_rev": "9c9d95b2dcf6007de99ad640e2f0f48e69dcb9db", "commit_rev":
"26d99c2e280e3e0cf3db50623b5ab18990f080e7"}

[commit-bot: I haz the power, 2017-02-13 20:47:31]

Description was changed from

==========
Add the URL attribute to cricket::Candiate.

The URL of the ICE server will be reconstructed by the Port and the URL
attribute is added to the cricket::Candidate struct so that we can tell
which ICE server the candidate was gathered from.

This CL only changes the native C++ code. The Java and Objc wrapper will
be created in separate CLs.

BUG=webrtc::7128
==========

to

==========
Add the URL attribute to cricket::Candiate.

The URL of the ICE server will be reconstructed by the Port and the URL
attribute is added to the cricket::Candidate struct so that we can tell
which ICE server the candidate was gathered from.

This CL only changes the native C++ code. The Java and Objc wrapper will
be created in separate CLs.

BUG=webrtc::7128

Review-Url: https://codereview.webrtc.org/2685053004
Cr-Commit-Position: refs/heads/master@{#16591}
Committed:
https://chromium.googlesource.com/external/webrtc/+/26d99c2e280e3e0cf3db50623...
==========

[commit-bot: I haz the power, 2017-02-13 20:47:32]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/26d99c2e280e3e0cf3db50623...