Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
597283006:
Updated the MockWebRtcPeerConnectionHandler used in blink Layouttests. (Closed)
|
Created:
6 years, 2 months ago by perkj_chrome Modified:
6 years, 1 month ago CC:
chromium-reviews, darin-cc_chromium.org, jam, jochen+watch_chromium.org, mkwst+moarreviews-shell_chromium.org, mkwst+moarreviews-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionUpdated the MockWebRtcPeerConnectionHandler object used in blink Layouttests.
This cl change the mock to create remote mediastreams based on the mediastreams that have been added as local mediastreams. The remote streams are updated once RTCPeerConnection::setRemoteDescription is called. The purpose is to be able to test remote mediastreams in blink layouttests.
BUG=417245
Committed: https://crrev.com/4002a79ff025a89a95670c66b04d407ee980534b
Cr-Commit-Position: refs/heads/master@{#299069}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Addressed comments from hta. #
Total comments: 4
Patch Set 3 : Addressed comments. #
Messages
Total messages: 19 (6 generated)
perkj@chromium.org changed reviewers: + hta@chromium.org
Can you please take a look?
A few comments - it's been a while since I did blink stuff, so not sure about what's a pointer and what's an object. Worried about landing this separate from any tests that exercise the new functionality - we're landing untested code. But whatever works with the Chromium/Blink submit sequence.... Nit in description: "This cl change the mock" -> "This CL changes the mock". https://codereview.chromium.org/597283006/diff/1/content/shell/renderer/test_... File content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.cc (right): https://codereview.chromium.org/597283006/diff/1/content/shell/renderer/test_... content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.cc:291: local_audio_tracks[i].id()); Shouldn't you be using the new flags here? this seems to be calling the deprecated initialize function (but does permit the CL to be landed independently of the other one). https://codereview.chromium.org/597283006/diff/1/content/shell/renderer/test_... content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.cc:303: local_audio_tracks[i].id()); The occurence of "audio" in this line looks strange... https://codereview.chromium.org/597283006/diff/1/content/shell/renderer/test_... content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.cc:349: DCHECK(local_streams_.find(stream.id().utf8()) == local_streams_.end()); This is actually not according to spec. Spec says "If stream is already in connection's local streams set, then abort these steps". (It doesn't say that an error is returned, so it seems to be ignorable.) https://codereview.chromium.org/597283006/diff/1/content/shell/renderer/test_... File content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.h (left): https://codereview.chromium.org/597283006/diff/1/content/shell/renderer/test_... content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.h:76: MockWebRTCPeerConnectionHandler(); Hmm.... I think there was a reason for this existing once upon a time, but I can't remember what it was .... its existence ensures that there can't be a compiler-generated version, but I don't think there is one generated by default. Suspicious about deleting it anyway. https://codereview.chromium.org/597283006/diff/1/content/shell/renderer/test_... File content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.h (right): https://codereview.chromium.org/597283006/diff/1/content/shell/renderer/test_... content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.h:81: // remote MediaStreams with the same amount of tracks and notifies |client_| Nit: amount of tracks -> number of tracks https://codereview.chromium.org/597283006/diff/1/content/shell/renderer/test_... content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.h:82: // about added and removed streams. Its triggered when setRemoteDescription Nit: Its -> It's https://codereview.chromium.org/597283006/diff/1/content/shell/renderer/test_... content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.h:93: typedef std::map<std::string, blink::WebMediaStream> StreamMap; Strange question: What is the type of blink::WebMediaStream - is it a pointer, a wrapper around a pointer, or a full fledged object? The way this is written, it looks as if you're taking copies of the objects.
https://codereview.chromium.org/597283006/diff/1/content/shell/renderer/test_... File content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.cc (right): https://codereview.chromium.org/597283006/diff/1/content/shell/renderer/test_... content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.cc:291: local_audio_tracks[i].id()); On 2014/09/30 09:43:49, hta - Chromium wrote: > Shouldn't you be using the new flags here? this seems to be calling the > deprecated initialize function (but does permit the CL to be landed > independently of the other one). Yes, but I would prefer to do that when I change all other calls in Chromium. I would like to land this first to add the test for removing remote remote tracks. https://codereview.chromium.org/597283006/diff/1/content/shell/renderer/test_... content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.cc:303: local_audio_tracks[i].id()); On 2014/09/30 09:43:49, hta - Chromium wrote: > The occurence of "audio" in this line looks strange... Yes, wrong. https://codereview.chromium.org/597283006/diff/1/content/shell/renderer/test_... content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.cc:349: DCHECK(local_streams_.find(stream.id().utf8()) == local_streams_.end()); On 2014/09/30 09:43:49, hta - Chromium wrote: > This is actually not according to spec. Spec says "If stream is already in > connection's local streams set, then abort these steps". (It doesn't say that an > error is returned, so it seems to be ignorable.) > ok- this was to help debugging tests. It looks like if I return false here, a syntax error exception will be thrown. RTCPeerConnection.cpp. https://codereview.chromium.org/597283006/diff/1/content/shell/renderer/test_... File content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.h (right): https://codereview.chromium.org/597283006/diff/1/content/shell/renderer/test_... content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.h:81: // remote MediaStreams with the same amount of tracks and notifies |client_| On 2014/09/30 09:43:49, hta - Chromium wrote: > Nit: amount of tracks -> number of tracks Done. https://codereview.chromium.org/597283006/diff/1/content/shell/renderer/test_... content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.h:82: // about added and removed streams. Its triggered when setRemoteDescription On 2014/09/30 09:43:50, hta - Chromium wrote: > Nit: Its -> It's Done. https://codereview.chromium.org/597283006/diff/1/content/shell/renderer/test_... content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.h:93: typedef std::map<std::string, blink::WebMediaStream> StreamMap; On 2014/09/30 09:43:50, hta - Chromium wrote: > Strange question: What is the type of blink::WebMediaStream - is it a pointer, a > wrapper around a pointer, or a full fledged object? > > The way this is written, it looks as if you're taking copies of the objects. They are simple objects around a smart pointers.
ping?
lgtm
perkj@chromium.org changed reviewers: + jochen@chromium.org
jochen, can you help with this? Does it make sence? Harald have reviewed but we have no owners in Stockholm.
lgtm https://codereview.chromium.org/597283006/diff/20001/content/shell/renderer/t... File content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.h (left): https://codereview.chromium.org/597283006/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.h:76: MockWebRTCPeerConnectionHandler(); plz don't delete the ctor, even if its empty https://codereview.chromium.org/597283006/diff/20001/content/shell/renderer/t... File content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.h (right): https://codereview.chromium.org/597283006/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.h:16: #include "third_party/WebKit/public/platform/WebString.h" should not be needed, no?
https://codereview.chromium.org/597283006/diff/20001/content/shell/renderer/t... File content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.h (left): https://codereview.chromium.org/597283006/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.h:76: MockWebRTCPeerConnectionHandler(); On 2014/10/09 11:49:34, jochen wrote: > plz don't delete the ctor, even if its empty Done. https://codereview.chromium.org/597283006/diff/20001/content/shell/renderer/t... File content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.h (right): https://codereview.chromium.org/597283006/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/mock_webrtc_peer_connection_handler.h:16: #include "third_party/WebKit/public/platform/WebString.h" On 2014/10/09 11:49:34, jochen wrote: > should not be needed, no? Acknowledged.
The CQ bit was checked by perkj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/597283006/40001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to commit the patch.
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by perkj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/597283006/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4002a79ff025a89a95670c66b04d407ee980534b Cr-Commit-Position: refs/heads/master@{#299069} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
