Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(163)

Issue 10703095: New PeerConnection handler in Chrome to support latest PeerConnection draft (Closed)

Created:
8 years, 5 months ago by perkj_chrome
Modified:
8 years, 3 months ago
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org
Visibility:
Public.

Description

This cl adds a new PeerConnection handler in Chrome to support latest W3C PeerConnection draft. http://dev.w3.org/2011/webrtc/editor/webrtc.html - Some of the functions are are overriden by the proposal in https://docs.google.com/a/google.com/document/d/1nfA1ElWed5PPR4nqenEkxNMMLRfzAsh1yMLe8yRcns8/edit# The implementation is in rtc_peer_connection_handler.cc and is called RTCPeerConnectionHandler. It implements the new WebKit interface WebKit::WebRTCPeerConnectionHandler. The idea is to keep the previous PeerConnection version around until web developers have had time to upgrade to the new API. The Chrome glue code for the old version is in peer_connection_handler_jsep.cc. The implementation is called PeerConnectionHandlerJsep and implements WebKit::WebPeerConnection00Handler BUG=142829 TEST= Tested with the included unit test. A PyAutoTest is also beeing uploaded for review. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156608

Patch Set 1 #

Patch Set 2 : Updated with latest patch. #

Patch Set 3 : Updated glue code. #

Patch Set 4 : Updated to latest webkit change #

Patch Set 5 : Update to latest WebKit and Libjingle. #

Patch Set 6 : Update to latest WebKit and libjingle. #

Patch Set 7 : Updated to latest WebKit + added onicecomplete. #

Patch Set 8 : Removed unnecessary files #

Patch Set 9 : Added unit test and mock. Removed old bad mock. #

Total comments: 32

Patch Set 10 : Fix code review issues found by Wei. #

Total comments: 63

Patch Set 11 : Fixed code review comments by Ronghua. #

Total comments: 4

Patch Set 12 : Fix code review comments found Ronghua. #

Total comments: 2

Patch Set 13 : Updated to latest code base. #

Patch Set 14 : Rebased to latest code base. #

Patch Set 15 : Fixed broken unittes. #

Total comments: 22

Patch Set 16 : Fix build error on clang - mock_webrtc_peer_connection_client #

Patch Set 17 : Addressed code review issues found by Tommi. #

Total comments: 1

Patch Set 18 : Readd the UMA histogram for Deprecated PeerConnection to not screw up the stats. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1145 lines, -162 lines) Patch
M content/content_renderer.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -0 lines 0 comments Download
M content/content_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +3 lines, -2 lines 0 comments Download
M content/renderer/media/media_stream_dependency_factory.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 5 chunks +26 lines, -7 lines 0 comments Download
M content/renderer/media/media_stream_dependency_factory.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 10 chunks +45 lines, -17 lines 0 comments Download
M content/renderer/media/media_stream_dependency_factory_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +15 lines, -9 lines 0 comments Download
M content/renderer/media/mock_media_stream_dependency_factory.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +17 lines, -13 lines 0 comments Download
M content/renderer/media/mock_media_stream_dependency_factory.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 8 chunks +34 lines, -17 lines 0 comments Download
M content/renderer/media/mock_peer_connection_impl.h View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +7 lines, -1 line 0 comments Download
M content/renderer/media/mock_peer_connection_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +33 lines, -15 lines 0 comments Download
D content/renderer/media/mock_web_peer_connection_handler_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +0 lines, -40 lines 0 comments Download
D content/renderer/media/mock_web_peer_connection_handler_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +0 lines, -37 lines 0 comments Download
A content/renderer/media/mock_web_rtc_peer_connection_handler_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +59 lines, -0 lines 0 comments Download
A content/renderer/media/mock_web_rtc_peer_connection_handler_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +62 lines, -0 lines 0 comments Download
M content/renderer/media/peer_connection_handler_base.h View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M content/renderer/media/peer_connection_handler_base.cc View 1 2 3 4 1 chunk +10 lines, -0 lines 0 comments Download
A content/renderer/media/rtc_peer_connection_handler.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +86 lines, -0 lines 0 comments Download
A content/renderer/media/rtc_peer_connection_handler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +392 lines, -0 lines 0 comments Download
A content/renderer/media/rtc_peer_connection_handler_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +330 lines, -0 lines 0 comments Download
M content/renderer/media/webrtc_uma_histograms.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +2 lines, -0 lines 0 comments Download
M content/renderer/renderer_webkitplatformsupport_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
M content/renderer/renderer_webkitplatformsupport_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +18 lines, -4 lines 0 comments Download

Messages

Total messages: 25 (0 generated)
perkj_chrome
Hi, here is a Chrome patch that matches your libjingle patch + (most of) my ...
8 years, 5 months ago (2012-07-05 15:17:12 UTC) #1
perkj_chrome
Adding Wei. Can you guys please take a look and start reviewing? Please skip the ...
8 years, 4 months ago (2012-08-09 14:28:19 UTC) #2
perkj_chrome
Unit test added. Now I think this is as good as it gets. Please help ...
8 years, 4 months ago (2012-08-10 14:35:38 UTC) #3
wjia(left Chromium)
http://codereview.chromium.org/10703095/diff/17009/content/content_tests.gypi File content/content_tests.gypi (right): http://codereview.chromium.org/10703095/diff/17009/content/content_tests.gypi#newcode156 content/content_tests.gypi:156: 'renderer/media/mock_web_peer_connection_00_handler_client.h', remove extra spaces at the end. http://codereview.chromium.org/10703095/diff/17009/content/renderer/media/media_stream_impl.cc File ...
8 years, 4 months ago (2012-08-12 17:06:52 UTC) #4
perkj_chrome
http://codereview.chromium.org/10703095/diff/17009/content/content_tests.gypi File content/content_tests.gypi (right): http://codereview.chromium.org/10703095/diff/17009/content/content_tests.gypi#newcode156 content/content_tests.gypi:156: 'renderer/media/mock_web_peer_connection_00_handler_client.h', On 2012/08/12 17:06:52, wjia wrote: > remove extra ...
8 years, 4 months ago (2012-08-13 07:35:46 UTC) #5
Ronghua Wu (Left Chromium)
http://codereview.chromium.org/10703095/diff/24025/content/renderer/media/media_stream_impl.h File content/renderer/media/media_stream_impl.h (right): http://codereview.chromium.org/10703095/diff/24025/content/renderer/media/media_stream_impl.h#newcode54 content/renderer/media/media_stream_impl.h:54: class WebRTCPeerConnectionHandlerClient; Let's keep in order http://codereview.chromium.org/10703095/diff/24025/content/renderer/media/media_stream_impl_unittest.cc File content/renderer/media/media_stream_impl_unittest.cc ...
8 years, 4 months ago (2012-08-14 00:59:02 UTC) #6
perkj_chrome
Please take another look. http://codereview.chromium.org/10703095/diff/24025/content/renderer/media/media_stream_impl.h File content/renderer/media/media_stream_impl.h (right): http://codereview.chromium.org/10703095/diff/24025/content/renderer/media/media_stream_impl.h#newcode54 content/renderer/media/media_stream_impl.h:54: class WebRTCPeerConnectionHandlerClient; On 2012/08/14 00:59:03, ...
8 years, 4 months ago (2012-08-14 09:15:40 UTC) #7
Ronghua Wu (Left Chromium)
LGTM after few more minor comments. http://codereview.chromium.org/10703095/diff/24025/content/renderer/renderer_webkitplatformsupport_impl.cc File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10703095/diff/24025/content/renderer/renderer_webkitplatformsupport_impl.cc#newcode722 content/renderer/renderer_webkitplatformsupport_impl.cc:722: if (web_frame) { ...
8 years, 4 months ago (2012-08-15 00:57:32 UTC) #8
wjia(left Chromium)
On 2012/08/15 00:57:32, Ronghua Wu wrote: > LGTM after few more minor comments. > > ...
8 years, 4 months ago (2012-08-15 01:05:13 UTC) #9
perkj_chrome
http://codereview.chromium.org/10703095/diff/25009/content/renderer/media/mock_peer_connection_impl.cc File content/renderer/media/mock_peer_connection_impl.cc (right): http://codereview.chromium.org/10703095/diff/25009/content/renderer/media/mock_peer_connection_impl.cc#newcode174 content/renderer/media/mock_peer_connection_impl.cc:174: dependency_factory_->CreateSessionDescription(kDummyOffer)); On 2012/08/15 00:57:32, Ronghua Wu wrote: > Using ...
8 years, 4 months ago (2012-08-15 09:12:30 UTC) #10
perkj_chrome
piman, would mind helping us again to review changes in content/renderer? This cl is dependent ...
8 years, 4 months ago (2012-08-15 09:32:53 UTC) #11
piman
http://codereview.chromium.org/10703095/diff/13017/content/renderer/renderer_webkitplatformsupport_impl.cc File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10703095/diff/13017/content/renderer/renderer_webkitplatformsupport_impl.cc#newcode717 content/renderer/renderer_webkitplatformsupport_impl.cc:717: WebFrame* web_frame = WebFrame::frameForCurrentContext(); frameForCurrentContext is not what you ...
8 years, 4 months ago (2012-08-15 16:51:50 UTC) #12
perkj_chrome
Adding Tommy W who is working on the WebKit parts. Tommy - can createRTCPeerConnectionHandler be ...
8 years, 4 months ago (2012-08-16 07:34:51 UTC) #13
Tommy Widenflycht
http://codereview.chromium.org/10703095/diff/13017/content/renderer/renderer_webkitplatformsupport_impl.cc File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10703095/diff/13017/content/renderer/renderer_webkitplatformsupport_impl.cc#newcode717 content/renderer/renderer_webkitplatformsupport_impl.cc:717: WebFrame* web_frame = WebFrame::frameForCurrentContext(); On 2012/08/15 16:51:50, piman wrote: ...
8 years, 4 months ago (2012-08-22 10:35:20 UTC) #14
piman
+darin. Grabbing the correct view from the Javascript context that's on top of the stack ...
8 years, 4 months ago (2012-08-22 16:27:48 UTC) #15
Tommy Widenflycht
On 2012/08/22 16:27:48, piman wrote: > +darin. Grabbing the correct view from the Javascript context ...
8 years, 3 months ago (2012-08-28 09:13:41 UTC) #16
piman
On 2012/08/28 09:13:41, tommy wrote: > On 2012/08/22 16:27:48, piman wrote: > > +darin. Grabbing ...
8 years, 3 months ago (2012-08-28 18:36:44 UTC) #17
Tommy Widenflycht
On 2012/08/28 18:36:44, piman wrote: > On 2012/08/28 09:13:41, tommy wrote: > > On 2012/08/22 ...
8 years, 3 months ago (2012-09-03 13:09:17 UTC) #18
perkj_chrome
ok- so I have refactored how P2PSocketDispatcher and PeerConnections are created. So I hope it ...
8 years, 3 months ago (2012-09-13 11:43:59 UTC) #19
tommi (sloooow) - chröme
https://chromiumcodereview.appspot.com/10703095/diff/33023/content/renderer/media/media_stream_dependency_factory.h File content/renderer/media/media_stream_dependency_factory.h (right): https://chromiumcodereview.appspot.com/10703095/diff/33023/content/renderer/media/media_stream_dependency_factory.h#newcode78 content/renderer/media/media_stream_dependency_factory.h:78: virtual talk_base::scoped_refptr<webrtc::PeerConnectionInterface> does this need to be talk_base::scoped_refptr or ...
8 years, 3 months ago (2012-09-13 12:20:13 UTC) #20
perkj_chrome
PTAL https://chromiumcodereview.appspot.com/10703095/diff/33023/content/renderer/media/media_stream_dependency_factory.h File content/renderer/media/media_stream_dependency_factory.h (right): https://chromiumcodereview.appspot.com/10703095/diff/33023/content/renderer/media/media_stream_dependency_factory.h#newcode78 content/renderer/media/media_stream_dependency_factory.h:78: virtual talk_base::scoped_refptr<webrtc::PeerConnectionInterface> On 2012/09/13 12:20:14, tommi wrote: > ...
8 years, 3 months ago (2012-09-13 13:41:34 UTC) #21
tommi (sloooow) - chröme
lgtm https://chromiumcodereview.appspot.com/10703095/diff/42002/content/renderer/media/rtc_peer_connection_handler.cc File content/renderer/media/rtc_peer_connection_handler.cc (right): https://chromiumcodereview.appspot.com/10703095/diff/42002/content/renderer/media/rtc_peer_connection_handler.cc#newcode357 content/renderer/media/rtc_peer_connection_handler.cc:357: DCHECK(candidate); nit: can skip this since it's dereferenced ...
8 years, 3 months ago (2012-09-13 14:48:44 UTC) #22
piman
content/renderer LGTM (I didn't look at content/renderer/media where you have other owners)
8 years, 3 months ago (2012-09-13 17:09:46 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/perkj@chromium.org/10703095/42006
8 years, 3 months ago (2012-09-13 17:29:00 UTC) #24
commit-bot: I haz the power
8 years, 3 months ago (2012-09-13 19:46:20 UTC) #25
Change committed as 156608

Powered by Google App Engine
This is Rietveld 408576698