Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2706563003:
Create the API that returns the RTCConfiguration of the PeerConnection.
|
Created:
3 years, 10 months ago by zhihuang1 Modified:
3 years, 6 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, mlamouri+watch-test-runner_chromium.org, posciak+watch_chromium.org, blink-reviews, jam, feature-media-reviews_chromium.org, dglazkov+blink, darin-cc_chromium.org, haraken, einbinder+watch-test-runner_chromium.org, kinuko+watch, blink-reviews-api_chromium.org, jochen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate the API that returns the RTCConfiguration of the PeerConnection.
Returns the last configuration applied via setConfiguration, or if
setConfiguration hasn't been called, the configuration the
RTCPeerConnection was constructed with.
Intent to ship thread:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/R_NKBRbDzhE/8gaX7HCjFgAJ
BUG=chromium:587453
Patch Set 1 : Wire up the getConfiguration() #Patch Set 2 : Fix the build. #Patch Set 3 : rebaselining #
Total comments: 7
Patch Set 4 : Verify the certificate. #Patch Set 5 : rebaselining again #
Total comments: 35
Patch Set 6 : Response to the CR comments. #
Total comments: 6
Patch Set 7 : Fix the issues in data type conversion. #
Total comments: 3
Patch Set 8 : Merge. #Patch Set 9 : rebaselining #
Total comments: 3
Messages
Total messages: 63 (39 generated)
The CQ bit was checked by zhihuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by zhihuang@chromium.org
The CQ bit was checked by zhihuang@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zhihuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by zhihuang@chromium.org
The CQ bit was checked by zhihuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:60001) has been deleted
zhihuang@chromium.org changed reviewers: + deadbeef@chromium.org
Please take a look. Thanks!
lgtm; just needs chromium/blink reviewers. https://codereview.chromium.org/2706563003/diff/80001/content/renderer/media/... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2706563003/diff/80001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:311: webrtc::PeerConnectionInterface::IceServer ice_server = Can use a const ref here instead of making a copy. https://codereview.chromium.org/2706563003/diff/80001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:351: NOTREACHED(); nit: Don't need default case here. https://codereview.chromium.org/2706563003/diff/80001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:361: default: nit: Don't need default case here. https://codereview.chromium.org/2706563003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2706563003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:356: default: nit: Don't need any of the default cases in this function.
The CQ bit was checked by zhihuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Create the API that returns the RTCConfiguration of the PeerConnection. Returns the last configuration applied via setConfiguration, or if setConfiguration hasn't been called, the configuration the RTCPeerConnection was constructed with Intent to ship thread: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/R_NKBRbDzhE/8g... BUG=chromium:587453 ========== to ========== Create the API that returns the RTCConfiguration of the PeerConnection. Returns the last configuration applied via setConfiguration, or if setConfiguration hasn't been called, the configuration the RTCPeerConnection was constructed with. Intent to ship thread: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/R_NKBRbDzhE/8g... BUG=chromium:587453 ==========
zhihuang@chromium.org changed reviewers: + dcheng@chromium.org, dmazzoni@chromium.org, tommi@chromium.org
Hi, Please take a look. Thanks. https://codereview.chromium.org/2706563003/diff/80001/content/renderer/media/... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2706563003/diff/80001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:311: webrtc::PeerConnectionInterface::IceServer ice_server = On 2017/03/02 22:30:45, Taylor_Brandstetter wrote: > Can use a const ref here instead of making a copy. Done. https://codereview.chromium.org/2706563003/diff/80001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:351: NOTREACHED(); On 2017/03/02 22:30:45, Taylor_Brandstetter wrote: > nit: Don't need default case here. Done. https://codereview.chromium.org/2706563003/diff/80001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:361: default: On 2017/03/02 22:30:45, Taylor_Brandstetter wrote: > nit: Don't need default case here. Done.
nice - I've got only minor questions/suggestions https://codereview.chromium.org/2706563003/diff/120001/content/renderer/media... File content/renderer/media/rtc_peer_connection_handler.cc (left): https://codereview.chromium.org/2706563003/diff/120001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:260: default: just checking - will compilation catch it for us if we somehow don't handle an enumerated value? https://codereview.chromium.org/2706563003/diff/120001/content/renderer/media... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2706563003/diff/120001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:304: for (size_t i = 0; i < webrtc_config.servers.size(); i++) { ++i https://codereview.chromium.org/2706563003/diff/120001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:309: for (size_t j = 0; j < ice_server.urls.size(); j++) { ++j Also, this code tends to not use {} for single line bodies of conditionals or loops. https://codereview.chromium.org/2706563003/diff/120001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:330: default: needed? (since similar cases were deleted above). ditto for the below. https://codereview.chromium.org/2706563003/diff/120001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:361: for (size_t i = 0; i < webrtc_config.certificates.size(); i++) { ++i https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:382: for (WebRTCIceServer blink_server : blinkConfig.iceServers) { nit: const WebRTCIceServer& as is, you're creating a copy. WebRTCIceServer might under the hood be implemented to do a shallow copy, but that feels like an implementation detail that you don't need to rely on here. https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:386: for (WebURL blink_url : blink_server.urls) { const WebURL&
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2706563003/diff/120001/content/renderer/media... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2706563003/diff/120001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:299: void GetBlinkRtcConfiguration( Nit: might be more clearly named ToBlinkRtcConfiguration, since this is a conversion helper. https://codereview.chromium.org/2706563003/diff/120001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:310: blink_urls[j] = (GURL(ice_server.urls[j])); Nit: no () around GURL() https://codereview.chromium.org/2706563003/diff/120001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:362: certificates[i] = std::unique_ptr<blink::WebRTCCertificate>( Nit: base::MakeUnique<WebRTCCertificate>(...) https://codereview.chromium.org/2706563003/diff/120001/content/shell/test_run... File content/shell/test_runner/mock_webrtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2706563003/diff/120001/content/shell/test_run... content/shell/test_runner/mock_webrtc_peer_connection_handler.cc:448: return blink_config; Nit: return blink::WebRTCConfiguration() https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:326: iceServers.push_back(WebRTCIceServer{urls, username, credential}); Nit: () rather than {}. We only use uniform initialization syntax when the alternatives don't work well: https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:345: RTCConfiguration getWebKitRtcConfiguration(WebRTCConfiguration& blinkConfig) { Pass argument by const ref. The naming is also a bit confusing here, IMO (it's not a getter, it's a conversion helper). We're also converting between two WebKit types so my earlier naming suggestion doesn't really work here either. https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:401: for (std::unique_ptr<blink::WebRTCCertificate>& blink_certificate : auto& for conciseness https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:867: rtcConfig = getWebKitRtcConfiguration(blinkConfig); Nit: reads a bit more naturally to just return the value directly, rather than assigning to a mutable ref (as this doesn't have any other return values) https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/ActiveConnectionThrottlingTest.cpp (right): https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ActiveConnectionThrottlingTest.cpp:78: return config; Nit: return blink::WebRTCConfiguration()
https://codereview.chromium.org/2706563003/diff/120001/content/renderer/media... File content/renderer/media/rtc_peer_connection_handler.cc (left): https://codereview.chromium.org/2706563003/diff/120001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:260: default: On 2017/03/09 19:37:07, tommi - chröme wrote: > just checking - will compilation catch it for us if we somehow don't handle an > enumerated value? Yes, from the "Wswitch" warning. That way it's a compile-time error rather than run-time.
Patchset #6 (id:140001) has been deleted
Patchset #6 (id:160001) has been deleted
The CQ bit was checked by zhihuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi, Please take another look. Thanks! https://codereview.chromium.org/2706563003/diff/120001/content/renderer/media... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2706563003/diff/120001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:299: void GetBlinkRtcConfiguration( On 2017/03/09 21:36:54, dcheng wrote: > Nit: might be more clearly named ToBlinkRtcConfiguration, since this is a > conversion helper. Done. https://codereview.chromium.org/2706563003/diff/120001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:304: for (size_t i = 0; i < webrtc_config.servers.size(); i++) { On 2017/03/09 19:37:07, tommi - chröme wrote: > ++i Done. https://codereview.chromium.org/2706563003/diff/120001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:309: for (size_t j = 0; j < ice_server.urls.size(); j++) { On 2017/03/09 19:37:07, tommi - chröme wrote: > ++j > > Also, this code tends to not use {} for single line bodies of conditionals or > loops. Done. https://codereview.chromium.org/2706563003/diff/120001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:310: blink_urls[j] = (GURL(ice_server.urls[j])); On 2017/03/09 21:36:54, dcheng wrote: > Nit: no () around GURL() Done. https://codereview.chromium.org/2706563003/diff/120001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:330: default: On 2017/03/09 19:37:07, tommi - chröme wrote: > needed? (since similar cases were deleted above). ditto for the below. I'll remove these. https://codereview.chromium.org/2706563003/diff/120001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:361: for (size_t i = 0; i < webrtc_config.certificates.size(); i++) { On 2017/03/09 19:37:07, tommi - chröme wrote: > ++i Done. https://codereview.chromium.org/2706563003/diff/120001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:362: certificates[i] = std::unique_ptr<blink::WebRTCCertificate>( On 2017/03/09 21:36:54, dcheng wrote: > Nit: base::MakeUnique<WebRTCCertificate>(...) Done. https://codereview.chromium.org/2706563003/diff/120001/content/shell/test_run... File content/shell/test_runner/mock_webrtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2706563003/diff/120001/content/shell/test_run... content/shell/test_runner/mock_webrtc_peer_connection_handler.cc:448: return blink_config; On 2017/03/09 21:36:54, dcheng wrote: > Nit: return blink::WebRTCConfiguration() Done. https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:326: iceServers.push_back(WebRTCIceServer{urls, username, credential}); On 2017/03/09 21:36:54, dcheng wrote: > Nit: () rather than {}. We only use uniform initialization syntax when the > alternatives don't work well: > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... Thanks for the link. Good to learn that. https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:345: RTCConfiguration getWebKitRtcConfiguration(WebRTCConfiguration& blinkConfig) { On 2017/03/09 21:36:54, dcheng wrote: > Pass argument by const ref. It is not const because the ownership of blinkConfig.certificates will be changed. > The naming is also a bit confusing here, IMO (it's not a getter, it's a > conversion helper). We're also converting between two WebKit types so my earlier > naming suggestion doesn't really work here either. What about just merging this with the getConfiguration() since this is only used once. https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:382: for (WebRTCIceServer blink_server : blinkConfig.iceServers) { On 2017/03/09 19:37:08, tommi - chröme wrote: > nit: const WebRTCIceServer& > as is, you're creating a copy. WebRTCIceServer might under the hood be > implemented to do a shallow copy, but that feels like an implementation detail > that you don't need to rely on here. Done. https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:386: for (WebURL blink_url : blink_server.urls) { On 2017/03/09 19:37:08, tommi - chröme wrote: > const WebURL& Done. https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:401: for (std::unique_ptr<blink::WebRTCCertificate>& blink_certificate : On 2017/03/09 21:36:54, dcheng wrote: > auto& for conciseness Done. https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:867: rtcConfig = getWebKitRtcConfiguration(blinkConfig); On 2017/03/09 21:36:54, dcheng wrote: > Nit: reads a bit more naturally to just return the value directly, rather than > assigning to a mutable ref (as this doesn't have any other return values) I agree that the direct return is more nature but it seems that I cannot do that. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/scrip... The RTCConfiguration is a dictionary so the generated file in V8 will expect an output parameter. https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/ActiveConnectionThrottlingTest.cpp (right): https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ActiveConnectionThrottlingTest.cpp:78: return config; On 2017/03/09 21:36:54, dcheng wrote: > Nit: return blink::WebRTCConfiguration() Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
rubberstamp lgtm for test_runner
https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:326: iceServers.push_back(WebRTCIceServer{urls, username, credential}); On 2017/03/10 03:20:43, zhihuang1 wrote: > On 2017/03/09 21:36:54, dcheng wrote: > > Nit: () rather than {}. We only use uniform initialization syntax when the > > alternatives don't work well: > > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... > > Thanks for the link. Good to learn that. Ah... I missed that this was a struct. It's OK to inline it into push back and just write {urls, username, credential} then https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:867: rtcConfig = getWebKitRtcConfiguration(blinkConfig); On 2017/03/10 03:20:43, zhihuang1 wrote: > On 2017/03/09 21:36:54, dcheng wrote: > > Nit: reads a bit more naturally to just return the value directly, rather than > > assigning to a mutable ref (as this doesn't have any other return values) > > I agree that the direct return is more nature but it seems that I cannot do > that. > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/scrip... > The RTCConfiguration is a dictionary so the generated file in V8 will expect an > output parameter. Acknowledged. https://codereview.chromium.org/2706563003/diff/170001/content/renderer/media... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2706563003/diff/170001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:1523: ToBlinkRtcConfiguration(webrtc_config, &blink_config); For this one, it's more straight to just return ToBlinkRtcConfiguration(), since there are no API constraints. https://codereview.chromium.org/2706563003/diff/170001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2706563003/diff/170001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:843: String urlString(blink_url.string().utf8().c_str()); Nit: omit utf8().c_str(), otherwise this turns a relatively cheap Blink string copy (which is just a refcount bump) into copying and creating a new string. In fact, the local here isn't necessary, and you can just wite urlStrings.push_back(blink_url.string()); https://codereview.chromium.org/2706563003/diff/170001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:848: String username(blink_server.username.utf8().c_str()); Same here and below: no local, and just pass the WebString to setUsername directly (there's a conversion operator)
https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:326: iceServers.push_back(WebRTCIceServer{urls, username, credential}); On 2017/03/10 19:54:00, dcheng wrote: > On 2017/03/10 03:20:43, zhihuang1 wrote: > > On 2017/03/09 21:36:54, dcheng wrote: > > > Nit: () rather than {}. We only use uniform initialization syntax when the > > > alternatives don't work well: > > > > > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... > > > > Thanks for the link. Good to learn that. > > Ah... I missed that this was a struct. It's OK to inline it into push back and > just write {urls, username, credential} then Actually, isn't emplace_back preferable then? iceServers.emplace_back(urls, username, credential);
On 2017/03/10 20:06:03, Taylor_Brandstetter wrote: > https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp > (right): > > https://codereview.chromium.org/2706563003/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:326: > iceServers.push_back(WebRTCIceServer{urls, username, credential}); > On 2017/03/10 19:54:00, dcheng wrote: > > On 2017/03/10 03:20:43, zhihuang1 wrote: > > > On 2017/03/09 21:36:54, dcheng wrote: > > > > Nit: () rather than {}. We only use uniform initialization syntax when the > > > > alternatives don't work well: > > > > > > > > > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... > > > > > > Thanks for the link. Good to learn that. > > > > Ah... I missed that this was a struct. It's OK to inline it into push back and > > just write {urls, username, credential} then > > Actually, isn't emplace_back preferable then? > > iceServers.emplace_back(urls, username, credential); There's no explicit constructor, so I don't think emplace_back will work here.
Please take another look. Thanks! https://codereview.chromium.org/2706563003/diff/170001/content/renderer/media... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2706563003/diff/170001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:1523: ToBlinkRtcConfiguration(webrtc_config, &blink_config); On 2017/03/10 19:54:00, dcheng wrote: > For this one, it's more straight to just return ToBlinkRtcConfiguration(), since > there are no API constraints. Done. https://codereview.chromium.org/2706563003/diff/170001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2706563003/diff/170001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:843: String urlString(blink_url.string().utf8().c_str()); On 2017/03/10 19:54:00, dcheng wrote: > Nit: omit utf8().c_str(), otherwise this turns a relatively cheap Blink string > copy (which is just a refcount bump) into copying and creating a new string. > > In fact, the local here isn't necessary, and you can just wite > urlStrings.push_back(blink_url.string()); Oh I see. https://codereview.chromium.org/2706563003/diff/170001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:848: String username(blink_server.username.utf8().c_str()); On 2017/03/10 19:54:00, dcheng wrote: > Same here and below: no local, and just pass the WebString to setUsername > directly (there's a conversion operator) Done.
lgtm with nit addressed https://codereview.chromium.org/2706563003/diff/190001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2706563003/diff/190001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:326: iceServers.push_back(WebRTCIceServer{urls, username, credential}); Nit: omit WebRTCIceServer, and just use braces, since this is just simple braced initialization of a struct. i.e. iceServers.push_back({urls, username, credential});
https://codereview.chromium.org/2706563003/diff/190001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2706563003/diff/190001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:326: iceServers.push_back(WebRTCIceServer{urls, username, credential}); On 2017/03/11 07:14:46, dcheng wrote: > Nit: omit WebRTCIceServer, and just use braces, since this is just simple braced > initialization of a struct. > > i.e. > > iceServers.push_back({urls, username, credential}); I tried this but the compiler didn't like it. third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:326:18: error: no matching member function for call to 'push_back' iceServers.push_back({urls, username, credential}); ~~~~~~~~~~~^~~~~~~~~ ../../third_party/WebKit/Source/wtf/Vector.h:1113:8: note: candidate template ignored: couldn't infer template argument 'U' void push_back(U&&);
https://codereview.chromium.org/2706563003/diff/190001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2706563003/diff/190001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:326: iceServers.push_back(WebRTCIceServer{urls, username, credential}); On 2017/03/13 18:50:53, zhihuang1 wrote: > On 2017/03/11 07:14:46, dcheng wrote: > > Nit: omit WebRTCIceServer, and just use braces, since this is just simple > braced > > initialization of a struct. > > > > i.e. > > > > iceServers.push_back({urls, username, credential}); > > I tried this but the compiler didn't like it. > > third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:326:18: > error: no matching member function for call to 'push_back' > iceServers.push_back({urls, username, credential}); > ~~~~~~~~~~~^~~~~~~~~ > ../../third_party/WebKit/Source/wtf/Vector.h:1113:8: note: candidate template > ignored: couldn't infer template argument 'U' > void push_back(U&&); Ah... it's because the types here don't exactly match the fields in the struct. I guess this is fine.
Hi Tommi, Please take another look. Thanks!
lgtm
The CQ bit was checked by zhihuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by zhihuang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@chromium.org, dmazzoni@chromium.org, tommi@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2706563003/#ps230001 (title: "rebaselining")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
zhihuang@chromium.org changed reviewers: + foolip@chromium.org
Hi Philip, Please take a look. The LayoutTests part needs to be reviewed. Thanks!
zhihuang@chromium.org changed reviewers: + dglazkov@chromium.org - foolip@chromium.org
Please take a look. Thanks!
foolip@chromium.org changed reviewers: + foolip@chromium.org
I think that more test coverage around various kinds of normalization would be good. The spec has an internal "[[ configuration]]" slot which is updated in two places and used in the definition of the getConfiguration() getter: "When this method is called, the user agent must return the RTCConfiguration object stored in the [[ configuration]] internal slot." The way that Web IDL works, that returned RTCConfiguration object would then get converted into an ECMAScript object, so that a new object is returned each time. That's what this implementation will end up doing, but it would be easy to misinterpret the spec to mean returning the same ECMAScript object every time, so guarding against that with a test would be good. For the RTCConfiguration dictionary members that have default values, can you test that if they are not passed at all to setConfiguration(), the object returned from getConfiguration() still has them and that their value is the default value? For the dictionary members that don't have default values, can you test that not passing them to setConfiguration() results in them not existing at all on the object returned from getConfiguration()? That would fail for iceServers currently I believe. If you can't tell what the spec requires, spec fixes are in order. https://codereview.chromium.org/2706563003/diff/230001/content/test/data/medi... File content/test/data/media/peerconnection-setAndGetConfiguration.html (right): https://codereview.chromium.org/2706563003/diff/230001/content/test/data/medi... content/test/data/media/peerconnection-setAndGetConfiguration.html:24: function testSetAndGetConfiguration() { It looks like much of this could be tested in web-platform-tests and thus benefit the second implementer. It's not yet a requirement to use web-platform-tests just because it's possible, but if you'd like to write as much of the tests in LayoutTetst/external/wpt/webrtc as possible, I'd be happy to review. (Changes will be automatically exported.) https://codereview.chromium.org/2706563003/diff/230001/content/test/data/medi... content/test/data/media/peerconnection-setAndGetConfiguration.html:207: assertEquals(server1['urls'].length, server2['urls'].length); I think this will fail due to normalization if the input isn't a list of URLs but a single URL. https://codereview.chromium.org/2706563003/diff/230001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2706563003/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:844: urls.setStringSequence(urlStrings); There's some normalization here that isn't spelled out in the spec. The configuration `{ "iceServers": [{ "urls": "stuns:stun.example.org" }] }` would become `{ "iceServers": [{ "urls": ["stuns:stun.example.org"] }] }` (array of 1 URL) and actually so would `{ "iceServers": [{ "url": "stuns:stun.example.org" }] }` (url in singular, not plural). Can you update the spec to clearly define what should happen here and make sure the tests cover that?
On 2017/03/21 08:37:15, foolip OOO until May 29 wrote: > I think that more test coverage around various kinds of normalization would be > good. > > The spec has an internal "[[ configuration]]" slot which is updated in two > places and used in the definition of the getConfiguration() getter: "When this > method is called, the user agent must return the RTCConfiguration object stored > in the [[ configuration]] internal slot." > > The way that Web IDL works, that returned RTCConfiguration object would then get > converted into an ECMAScript object, so that a new object is returned each time. > That's what this implementation will end up doing, but it would be easy to > misinterpret the spec to mean returning the same ECMAScript object every time, > so guarding against that with a test would be good. > > For the RTCConfiguration dictionary members that have default values, can you > test that if they are not passed at all to setConfiguration(), the object > returned from getConfiguration() still has them and that their value is the > default value? > > For the dictionary members that don't have default values, can you test that not > passing them to setConfiguration() results in them not existing at all on the > object returned from getConfiguration()? That would fail for iceServers > currently I believe. If you can't tell what the spec requires, spec fixes are in > order. > > https://codereview.chromium.org/2706563003/diff/230001/content/test/data/medi... > File content/test/data/media/peerconnection-setAndGetConfiguration.html (right): > > https://codereview.chromium.org/2706563003/diff/230001/content/test/data/medi... > content/test/data/media/peerconnection-setAndGetConfiguration.html:24: function > testSetAndGetConfiguration() { > It looks like much of this could be tested in web-platform-tests and thus > benefit the second implementer. It's not yet a requirement to use > web-platform-tests just because it's possible, but if you'd like to write as > much of the tests in LayoutTetst/external/wpt/webrtc as possible, I'd be happy > to review. (Changes will be automatically exported.) > > https://codereview.chromium.org/2706563003/diff/230001/content/test/data/medi... > content/test/data/media/peerconnection-setAndGetConfiguration.html:207: > assertEquals(server1['urls'].length, server2['urls'].length); > I think this will fail due to normalization if the input isn't a list of URLs > but a single URL. > > https://codereview.chromium.org/2706563003/diff/230001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp > (right): > > https://codereview.chromium.org/2706563003/diff/230001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:844: > urls.setStringSequence(urlStrings); > There's some normalization here that isn't spelled out in the spec. The > configuration `{ "iceServers": [{ "urls": "stuns:stun.example.org" }] }` would > become `{ "iceServers": [{ "urls": ["stuns:stun.example.org"] }] }` (array of 1 > URL) and actually so would `{ "iceServers": [{ "url": "stuns:stun.example.org" > }] }` (url in singular, not plural). > > Can you update the spec to clearly define what should happen here and make sure > the tests cover that? Sorry for responding so late, but I don't think this is an issue from a standards perspective. The singular "url" parameter isn't part of the standard; it's just something we support for backwards compatibility. And the spec already has a normalization step: "If server.urls is a string, let server.urls be a list consisting of just that string." So, if setConfiguration is called with { "urls": "stuns:stun.example.org" }, I'd expect getConfiguration to return { "urls": ["stuns:stun.example.org"] }.
On 2017/05/25 21:13:59, Taylor_Brandstetter wrote: > On 2017/03/21 08:37:15, foolip OOO until May 29 wrote: > > I think that more test coverage around various kinds of normalization would be > > good. > > > > The spec has an internal "[[ configuration]]" slot which is updated in two > > places and used in the definition of the getConfiguration() getter: "When this > > method is called, the user agent must return the RTCConfiguration object > stored > > in the [[ configuration]] internal slot." > > > > The way that Web IDL works, that returned RTCConfiguration object would then > get > > converted into an ECMAScript object, so that a new object is returned each > time. > > That's what this implementation will end up doing, but it would be easy to > > misinterpret the spec to mean returning the same ECMAScript object every time, > > so guarding against that with a test would be good. > > > > For the RTCConfiguration dictionary members that have default values, can you > > test that if they are not passed at all to setConfiguration(), the object > > returned from getConfiguration() still has them and that their value is the > > default value? > > > > For the dictionary members that don't have default values, can you test that > not > > passing them to setConfiguration() results in them not existing at all on the > > object returned from getConfiguration()? That would fail for iceServers > > currently I believe. If you can't tell what the spec requires, spec fixes are > in > > order. > > > > > https://codereview.chromium.org/2706563003/diff/230001/content/test/data/medi... > > File content/test/data/media/peerconnection-setAndGetConfiguration.html > (right): > > > > > https://codereview.chromium.org/2706563003/diff/230001/content/test/data/medi... > > content/test/data/media/peerconnection-setAndGetConfiguration.html:24: > function > > testSetAndGetConfiguration() { > > It looks like much of this could be tested in web-platform-tests and thus > > benefit the second implementer. It's not yet a requirement to use > > web-platform-tests just because it's possible, but if you'd like to write as > > much of the tests in LayoutTetst/external/wpt/webrtc as possible, I'd be happy > > to review. (Changes will be automatically exported.) > > > > > https://codereview.chromium.org/2706563003/diff/230001/content/test/data/medi... > > content/test/data/media/peerconnection-setAndGetConfiguration.html:207: > > assertEquals(server1['urls'].length, server2['urls'].length); > > I think this will fail due to normalization if the input isn't a list of URLs > > but a single URL. > > > > > https://codereview.chromium.org/2706563003/diff/230001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp > > (right): > > > > > https://codereview.chromium.org/2706563003/diff/230001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:844: > > urls.setStringSequence(urlStrings); > > There's some normalization here that isn't spelled out in the spec. The > > configuration `{ "iceServers": [{ "urls": "stuns:stun.example.org" }] }` would > > become `{ "iceServers": [{ "urls": ["stuns:stun.example.org"] }] }` (array of > 1 > > URL) and actually so would `{ "iceServers": [{ "url": "stuns:stun.example.org" > > }] }` (url in singular, not plural). > > > > Can you update the spec to clearly define what should happen here and make > sure > > the tests cover that? > > Sorry for responding so late, but I don't think this is an issue from a > standards perspective. > > The singular "url" parameter isn't part of the standard; it's just something we > support for backwards compatibility. > > And the spec already has a normalization step: > > "If server.urls is a string, let server.urls be a list consisting of just that > string." > > So, if setConfiguration is called with { "urls": "stuns:stun.example.org" }, I'd > expect getConfiguration to return { "urls": ["stuns:stun.example.org"] }. OK, good, that's step 11.2 of https://w3c.github.io/webrtc-pc/#set-pc-configuration Can you add a test for that case? Maybe in LayoutTests/external/wpt/RTCPeerConnection-getConfiguration.html? :) |
