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

Issue 1161403002: Relanding WebRTC peerconnection iframe test. (Closed)

Created:
5 years, 6 months ago by phoglund_chromium
Modified:
5 years, 6 months ago
CC:
chromium-reviews, feature-media-reviews_chromium.org, jam, darin-cc_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Relanding WebRTC peerconnection iframe test. This is a version of perkj's test in codereview.chromium.org/1073783003 which also covers a full peer connection call. It does not attempt to verify the case where we nuke the iframe though since we don't know what will happen past the gUM call in that call. The WebRTC call in the iframe will just get torn down so there's not much we can test. This fixes a bug in the test from the original patch, where we would negotiate the call twice because of an iframe.onload quirk. This actually worked except on Windows, but it's a degenerate use case we should not test (except maybe in a fuzzer). BUG=479093 Committed: https://crrev.com/98b51d8ead31a00a78f59c6bc7c187ee10f7a43e Cr-Commit-Position: refs/heads/master@{#332586}

Patch Set 1 #

Patch Set 2 : Fixing onload bug (no longer running two calls in parallel) #

Total comments: 2

Patch Set 3 : Fixing indent #

Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -0 lines) Patch
M content/browser/media/webrtc_browsertest.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M content/test/data/media/peerconnection-call.html View 1 2 1 chunk +27 lines, -0 lines 0 comments Download

Messages

Total messages: 9 (3 generated)
phoglund_chromium
5 years, 6 months ago (2015-06-02 12:28:14 UTC) #2
kjellander_chromium
nice! lgtm https://codereview.chromium.org/1161403002/diff/20001/content/test/data/media/peerconnection-call.html File content/test/data/media/peerconnection-call.html (right): https://codereview.chromium.org/1161403002/diff/20001/content/test/data/media/peerconnection-call.html#newcode575 content/test/data/media/peerconnection-call.html:575: window.parent.reportTestSuccess); nit: +2 spaces indent
5 years, 6 months ago (2015-06-03 07:57:37 UTC) #3
phoglund_chromium
https://codereview.chromium.org/1161403002/diff/20001/content/test/data/media/peerconnection-call.html File content/test/data/media/peerconnection-call.html (right): https://codereview.chromium.org/1161403002/diff/20001/content/test/data/media/peerconnection-call.html#newcode575 content/test/data/media/peerconnection-call.html:575: window.parent.reportTestSuccess); On 2015/06/03 07:57:36, kjellander wrote: > nit: +2 ...
5 years, 6 months ago (2015-06-03 08:31:54 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1161403002/40001
5 years, 6 months ago (2015-06-03 08:32:35 UTC) #7
commit-bot: I haz the power
Committed patchset #3 (id:40001)
5 years, 6 months ago (2015-06-03 10:49:01 UTC) #8
commit-bot: I haz the power
5 years, 6 months ago (2015-06-03 10:49:51 UTC) #9
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/98b51d8ead31a00a78f59c6bc7c187ee10f7a43e
Cr-Commit-Position: refs/heads/master@{#332586}

Powered by Google App Engine
This is Rietveld 408576698