Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
111383003:
Creates a basic Firefox - Chrome WebRTC interop test. (Closed)
|
Created:
7 years ago by phoglund_chromium Modified:
7 years ago CC:
chromium-reviews, fischman+watch_chromium.org, feature-media-reviews_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCreates a basic Firefox - Chrome WebRTC interop test.
The test brings up a local AppRTC instance and tells both browser to go
to the same apprtc room. We then verify that the call gets up from the
chrome side. If the call doesn't get up we can assume something went
wrong.
Depends on https://codereview.chromium.org/114293002/ for proper execution
on the bots.
BUG=272077
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240947
Patch Set 1 #Patch Set 2 : Now using the proper firefox launcher. #Patch Set 3 : Cleanup #Patch Set 4 : Correcting launcher check #
Total comments: 30
Patch Set 5 : Focusing the patch on Linux-only for now, make shutdown more reliable. #Patch Set 6 : Rebased #Messages
Total messages: 15 (0 generated)
kjellander: main review tommi: owner stamp
https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... File chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc (right): https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:119: FILE_PATH_LITERAL("../firefox-nightly/firefox/firefox")); Add each path component separately for cross platform compatibility. Same below. https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:204: ASSERT_TRUE(WaitForCallToComeUp(chrome_tab)); EXPECT_TRUE so that the next line will be run. Fix this elsewhere too?
https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... File chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc (right): https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:138: VLOG(0) << "Running " << command_line.GetCommandLineString(); forgot one thing - VLOG(0) is apparently frowned upon (see chromium-dev). Use VLOG(1) for truly 'verbose only' logging. https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:187: VLOG(0) << "Waiting for AppRTC to come up..."; would be nice to change all the VLOG(0)'s to VLOG(1)'s in this file.
https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... File chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc (right): https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:36: // WebRTC-AppRTC integration test. Requires a real webcam and microphone Please clarify that for the Firefox-Chrome test, the real webcam will be used by Firefox and Chrome will fall back on using its fake device, in order to have a webcam available for Firefox. https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:139: return base::LaunchProcess(command_line, base::LaunchOptions(), What happens if there's no webcam available for Firefox? It would be nice if it's easy to find out from logs but since we have a webcam check it's unlikely to happen unless we make mistakes in additional tests involving Firefox in the future. https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:146: return base::KillProcess(firefox_, 0, false); Is there some way we can check if firefox crashed and fail the test in such a case? I.e. the process no longer exists at this step. https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:185: ASSERT_TRUE(LaunchApprtcInstanceOnLocalhost()); I think we want to skip this on Vista as well, as in the WorksOnApprtc test above. https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:185: ASSERT_TRUE(LaunchApprtcInstanceOnLocalhost()); Add DetectErrorsInJavaScript(); since communicating with firefox might trigger other JS code paths than the Chrome-Chrome AppRTC test. https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:195: base::RandInt(0, 65536))); Do we really need to randomize this? I would be surprised if the room i already taken as I assume the state of the local AppRTC instance is reset between each run? For eventual parallel test execution, I guess we would run into a port conflict on 9999 before hitting an occupied room due to multiple AppRTC instances. https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:205: ASSERT_TRUE(StopFirefox()); Add ASSERT_TRUE(StopApprtcInstance()); after stopping firefox? (changing that one to EXPECT_TRUE too).
https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... File chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc (right): https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:146: return base::KillProcess(firefox_, 0, false); On 2013/12/13 10:19:11, Henrik Kjellander wrote: > Is there some way we can check if firefox crashed and fail the test in such a > case? I.e. the process no longer exists at this step. Can we find a better way to close Firefox? Killing the process like this is a good way to end up with a corrupt profile. A corrupt profile will break FF on the next startup, cause the test to fail and someone (sheriff/trooper) will have to manually go in and fix the profile on the bot.
https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... File chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc (right): https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:36: // WebRTC-AppRTC integration test. Requires a real webcam and microphone On 2013/12/13 10:19:11, Henrik Kjellander wrote: > Please clarify that for the Firefox-Chrome test, the real webcam will be used by > Firefox and Chrome will fall back on using its fake device, in order to have a > webcam available for Firefox. Done. https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:119: FILE_PATH_LITERAL("../firefox-nightly/firefox/firefox")); On 2013/12/12 18:41:33, tommi wrote: > Add each path component separately for cross platform compatibility. > > Same below. I can do that, but it will _work_ like this. For instance we're doing that in the other browser tests which run on all platforms (e.g. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med...). I think this is the more readable alternative than adding a ton of .Append and FILE_PATH_LITERAL calls. https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:138: VLOG(0) << "Running " << command_line.GetCommandLineString(); On 2013/12/12 19:52:05, tommi wrote: > forgot one thing - VLOG(0) is apparently frowned upon (see chromium-dev). Use > VLOG(1) for truly 'verbose only' logging. Done. https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:139: return base::LaunchProcess(command_line, base::LaunchOptions(), On 2013/12/13 10:19:11, Henrik Kjellander wrote: > What happens if there's no webcam available for Firefox? It would be nice if > it's easy to find out from logs but since we have a webcam check it's unlikely > to happen unless we make mistakes in additional tests involving Firefox in the > future. Don't know, I guess gUM will fail there and the call will not get up. I can see if I can enable a suitable level of logging for FF for now so that we can see something from the test logs when the call doesn't get up. https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:146: return base::KillProcess(firefox_, 0, false); On 2013/12/13 10:19:11, Henrik Kjellander wrote: > Is there some way we can check if firefox crashed and fail the test in such a > case? I.e. the process no longer exists at this step. I can't see any easy portable way of doing that unfortunately. Perhaps if we manage to redesign the firefox run script to terminate itself cleanly somehow. https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:146: return base::KillProcess(firefox_, 0, false); On 2013/12/13 12:21:31, tommi wrote: > On 2013/12/13 10:19:11, Henrik Kjellander wrote: > > Is there some way we can check if firefox crashed and fail the test in such a > > case? I.e. the process no longer exists at this step. > > Can we find a better way to close Firefox? > Killing the process like this is a good way to end up with a corrupt profile. A > corrupt profile will break FF on the next startup, cause the test to fail and > someone (sheriff/trooper) will have to manually go in and fix the profile on the > bot. It will shut down cleanly on Linux, because I hooked the runner script up to listen to SIGTERM (which is what kill sends on POSIX). That signal handler will ask Firefox to shut itself down cleanly. See line 57 of the runner script in the sibling patch: https://codereview.chromium.org/114293002/diff/60001/run_firefox_webrtc.py We probably need to do something else on Windows. Kill does TerminateProcess (http://msdn.microsoft.com/en-us/library/windows/desktop/ms686714(v=vs.85).aspx) on Windows, which seems to be a pretty hard kill. The docs for the python signal API (http://docs.python.org/2/library/signal.html) hints that it maps the signals on Windows somehow - question is just if it maps TerminateProcess to SIGTERM or SIGKILL, or none of them. If we can catch SIGTERM or its equivalent on Windows, I suggest we go with this KillProcess strategy. I kind of like the idea that a process cleans itself up nicely when sent the TERM signal. If not, we could redesign the runner script so that it figures out by itself when it can die. Then we just wait here for it to die. If it doesn't die, something presumably went wrong. https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:185: ASSERT_TRUE(LaunchApprtcInstanceOnLocalhost()); On 2013/12/13 10:19:11, Henrik Kjellander wrote: > I think we want to skip this on Vista as well, as in the WorksOnApprtc test > above. You mean skip it on XP? Yeah, probably. Actually I probably need to realign this patch to just be for Linux for now anyway. It needs more work for Win and Mac. https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:185: ASSERT_TRUE(LaunchApprtcInstanceOnLocalhost()); On 2013/12/13 10:19:11, Henrik Kjellander wrote: > Add DetectErrorsInJavaScript(); since communicating with firefox might trigger > other JS code paths than the Chrome-Chrome AppRTC test. Done. https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:187: VLOG(0) << "Waiting for AppRTC to come up..."; On 2013/12/12 19:52:05, tommi wrote: > would be nice to change all the VLOG(0)'s to VLOG(1)'s in this file. Done. https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:195: base::RandInt(0, 65536))); On 2013/12/13 10:19:11, Henrik Kjellander wrote: > Do we really need to randomize this? I would be surprised if the room i already > taken as I assume the state of the local AppRTC instance is reset between each > run? > For eventual parallel test execution, I guess we would run into a port conflict > on 9999 before hitting an occupied room due to multiple AppRTC instances. Yeah, this test can't execute in parallel, but it presumably could if we randomized the ports. And no, randomizing the ports seems unnecessary since we restart the server instance each time. https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:204: ASSERT_TRUE(WaitForCallToComeUp(chrome_tab)); On 2013/12/12 18:41:33, tommi wrote: > EXPECT_TRUE so that the next line will be run. > > Fix this elsewhere too? I think it's good to return early if we have already failed, in general, but perhaps it would be nice to ensure we clean up Firefox. I guess I could add a TearDown that checks "if we opened firefox, make sure it dies". https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:205: ASSERT_TRUE(StopFirefox()); On 2013/12/13 10:19:11, Henrik Kjellander wrote: > Add > ASSERT_TRUE(StopApprtcInstance()); > after stopping firefox? (changing that one to EXPECT_TRUE too). Done.
https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... File chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc (right): https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:119: FILE_PATH_LITERAL("../firefox-nightly/firefox/firefox")); On 2013/12/13 14:35:10, phoglund wrote: > On 2013/12/12 18:41:33, tommi wrote: > > Add each path component separately for cross platform compatibility. > > > > Same below. > > I can do that, but it will _work_ like this. For instance we're doing that in > the other browser tests which run on all platforms (e.g. > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med...). > I think this is the more readable alternative than adding a ton of .Append and > FILE_PATH_LITERAL calls. OK, if Append takes care of this then it's fine as is. https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:146: return base::KillProcess(firefox_, 0, false); On 2013/12/13 14:35:10, phoglund wrote: > On 2013/12/13 12:21:31, tommi wrote: > > On 2013/12/13 10:19:11, Henrik Kjellander wrote: > > > Is there some way we can check if firefox crashed and fail the test in such > a > > > case? I.e. the process no longer exists at this step. > > > > Can we find a better way to close Firefox? > > Killing the process like this is a good way to end up with a corrupt profile. > A > > corrupt profile will break FF on the next startup, cause the test to fail and > > someone (sheriff/trooper) will have to manually go in and fix the profile on > the > > bot. > It will shut down cleanly on Linux, because I hooked the runner script up to > listen to SIGTERM (which is what kill sends on POSIX). That signal handler will > ask Firefox to shut itself down cleanly. See line 57 of the runner script in the > sibling patch: > https://codereview.chromium.org/114293002/diff/60001/run_firefox_webrtc.py > > We probably need to do something else on Windows. Kill does TerminateProcess > (http://msdn.microsoft.com/en-us/library/windows/desktop/ms686714%28v=vs.85%29...) > on Windows, which seems to be a pretty hard kill. The docs for the python signal > API (http://docs.python.org/2/library/signal.html) hints that it maps the > signals on Windows somehow - question is just if it maps TerminateProcess to > SIGTERM or SIGKILL, or none of them. > > If we can catch SIGTERM or its equivalent on Windows, I suggest we go with this > KillProcess strategy. I kind of like the idea that a process cleans itself up > nicely when sent the TERM signal. If not, we could redesign the runner script so > that it figures out by itself when it can die. Then we just wait here for it to > die. If it doesn't die, something presumably went wrong. Signals like that don't exist on Windows unfortunately. Instead WM_CLOSE or WM_SYSCOMMAND(SC_CLOSE) can be posted to the main window of the application. Then wait for the application to terminate and only if it doesn't quit within a specific timeout, use KillProcess. In the case of the launcher, maybe there isn't a window? In that case you could try posting a WM_QUIT message to the main thread (PostThreadMessage) and do the same. In any case, on Windows, what will happen with the code as is, is that the launcher will be killed and Firefox will actually be left running. https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:195: base::RandInt(0, 65536))); On 2013/12/13 14:35:10, phoglund wrote: > On 2013/12/13 10:19:11, Henrik Kjellander wrote: > > Do we really need to randomize this? I would be surprised if the room i > already > > taken as I assume the state of the local AppRTC instance is reset between each > > run? > > For eventual parallel test execution, I guess we would run into a port > conflict > > on 9999 before hitting an occupied room due to multiple AppRTC instances. > Yeah, this test can't execute in parallel, but it presumably could if we > randomized the ports. > > And no, randomizing the ports seems unnecessary since we restart the server > instance each time. The room number could also be the thread id. As is the test can still run into the occasional flakiness.
nice work. lgtm assuming you remove the redundant room randomization. https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... File chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc (right): https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:195: base::RandInt(0, 65536))); On 2013/12/13 14:35:10, phoglund wrote: > On 2013/12/13 10:19:11, Henrik Kjellander wrote: > > Do we really need to randomize this? I would be surprised if the room i > already > > taken as I assume the state of the local AppRTC instance is reset between each > > run? > > For eventual parallel test execution, I guess we would run into a port > conflict > > on 9999 before hitting an occupied room due to multiple AppRTC instances. > Yeah, this test can't execute in parallel, but it presumably could if we > randomized the ports. > > And no, randomizing the ports seems unnecessary since we restart the server > instance each time. Fair enough, as we don't run these tests in parallel. But please remove the unnecessary randomization of the room name, since it doesn't seem to be needed.
https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... File chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc (right): https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:146: return base::KillProcess(firefox_, 0, false); Nope, you are right, I actually tried it and registering a signal handler and TerminateProcess'ing the python instance and it doesn't work on Windows. There is no window in the launcher itself, so we need to redesign the launcher so it can figure out when to die and clean up by itself. I suggest we punt that to a later patch and get this Linux-only patch running first.
lgtm https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... File chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc (right): https://codereview.chromium.org/111383003/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:146: return base::KillProcess(firefox_, 0, false); On 2013/12/13 15:13:35, phoglund wrote: > Nope, you are right, I actually tried it and registering a signal handler and > TerminateProcess'ing the python instance and it doesn't work on Windows. > > There is no window in the launcher itself, so we need to redesign the launcher > so it can figure out when to die and clean up by itself. I suggest we punt that > to a later patch and get this Linux-only patch running first. Sounds good. Can you add a note explaining that and perhaps a check that makes sure this doesn't run on windows until we've fixed it?
> Sounds good. Can you add a note explaining that and perhaps a check that makes > sure this doesn't run on windows until we've fixed it? Adding a note. We don't need a check since there's no way this test can run in its current state on Windows (we have no idea how to deal with the downloaded installer yet).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/phoglund@chromium.org/111383003/80001
Failed to apply patch for
chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc
Hunk #1 FAILED at 35.
Hunk #2 succeeded at 84 (offset 5 lines).
Hunk #3 succeeded at 104 (offset 5 lines).
Hunk #4 succeeded at 117 (offset 5 lines).
Hunk #5 succeeded at 161 (offset 5 lines).
Hunk #6 succeeded at 176 (offset 5 lines).
1 out of 6 hunks FAILED -- saving rejects to file
chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc.rej
Patch: chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc
Index: chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc
diff --git a/chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc
b/chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc
index
9669957841cdf627fe27093bdc96bddf365a2ccf..1fe24eca0ee326858e0cbf7ad7a146f6d989a927
100644
--- a/chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc
+++ b/chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc
@@ -35,22 +35,34 @@ const char kTitlePageOfAppEngineAdminPage[] = "Instances";
// WebRTC-AppRTC integration test. Requires a real webcam and microphone
// on the running system. This test is not meant to run in the main browser
-// test suite since normal tester machines do not have webcams.
+// test suite since normal tester machines do not have webcams. Chrome will use
+// this camera for the regular AppRTC test whereas Firefox will use it in the
+// Firefox interop test (where case Chrome will use its built-in fake device).
//
// This test will bring up a AppRTC instance on localhost and verify that the
// call gets up when connecting to the same room from two tabs in a browser.
class WebrtcApprtcBrowserTest : public WebRtcTestBase {
public:
+ WebrtcApprtcBrowserTest()
+ : dev_appserver_(base::kNullProcessHandle),
+ firefox_(base::kNullProcessHandle) {
+ }
+
virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
- EXPECT_FALSE(command_line->HasSwitch(
- switches::kUseFakeDeviceForMediaStream));
- EXPECT_FALSE(command_line->HasSwitch(
- switches::kUseFakeUIForMediaStream));
+ EXPECT_FALSE(command_line->HasSwitch(switches::kUseFakeUIForMediaStream));
// The video playback will not work without a GPU, so force its use here.
command_line->AppendSwitch(switches::kUseGpuInTests);
}
+ virtual void TearDown() OVERRIDE {
+ // Kill any processes we may have brought up.
+ if (dev_appserver_ != base::kNullProcessHandle)
+ base::KillProcess(dev_appserver_, 0, false);
+ if (firefox_ != base::kNullProcessHandle)
+ base::KillProcess(firefox_, 0, false);
+ }
+
protected:
bool LaunchApprtcInstanceOnLocalhost() {
base::FilePath appengine_dev_appserver =
@@ -79,7 +91,7 @@ class WebrtcApprtcBrowserTest : public WebRtcTestBase {
command_line.AppendArg("--admin_port=9998");
command_line.AppendArg("--skip_sdk_update_check");
- VLOG(0) << "Running " << command_line.GetCommandLineString();
+ VLOG(1) << "Running " << command_line.GetCommandLineString();
return base::LaunchProcess(command_line, base::LaunchOptions(),
&dev_appserver_);
}
@@ -99,10 +111,6 @@ class WebrtcApprtcBrowserTest : public WebRtcTestBase {
return result == kTitlePageOfAppEngineAdminPage;
}
- bool StopApprtcInstance() {
- return base::KillProcess(dev_appserver_, 0, false);
- }
-
bool WaitForCallToComeUp(content::WebContents* tab_contents) {
// Apprtc will set remoteVideo.style.opacity to 1 when the call comes up.
std::string javascript =
@@ -116,8 +124,38 @@ class WebrtcApprtcBrowserTest : public WebRtcTestBase {
return source_dir;
}
+ bool LaunchFirefoxWithUrl(const GURL& url) {
+ base::FilePath firefox_binary =
+ GetSourceDir().Append(
+ FILE_PATH_LITERAL("../firefox-nightly/firefox/firefox"));
+ if (!base::PathExists(firefox_binary)) {
+ LOG(ERROR) << "Missing firefox binary at " <<
+ firefox_binary.value() << ". " << kAdviseOnGclientSolution;
+ return false;
+ }
+ base::FilePath firefox_launcher =
+ GetSourceDir().Append(
+ FILE_PATH_LITERAL("../webrtc.DEPS/run_firefox_webrtc.py"));
+ if (!base::PathExists(firefox_launcher)) {
+ LOG(ERROR) << "Missing firefox launcher at " <<
+ firefox_launcher.value() << ". " << kAdviseOnGclientSolution;
+ return false;
+ }
+
+ CommandLine command_line(firefox_launcher);
+ command_line.AppendSwitchPath("--binary", firefox_binary);
+ command_line.AppendSwitchASCII("--webpage", url.spec());
+
+ VLOG(1) << "Running " << command_line.GetCommandLineString();
+ return base::LaunchProcess(command_line, base::LaunchOptions(),
+ &firefox_);
+
+ return true;
+ }
+
private:
base::ProcessHandle dev_appserver_;
+ base::ProcessHandle firefox_;
};
IN_PROC_BROWSER_TEST_F(WebrtcApprtcBrowserTest, MANUAL_WorksOnApprtc) {
@@ -130,7 +168,7 @@ IN_PROC_BROWSER_TEST_F(WebrtcApprtcBrowserTest,
MANUAL_WorksOnApprtc) {
DetectErrorsInJavaScript();
ASSERT_TRUE(LaunchApprtcInstanceOnLocalhost());
while (!LocalApprtcInstanceIsUp())
- VLOG(0) << "Waiting for AppRTC to come up...";
+ VLOG(1) << "Waiting for AppRTC to come up...";
GURL room_url = GURL(base::StringPrintf("localhost:9999?r=room_%d",
base::RandInt(0, 65536)));
@@ -145,6 +183,43 @@ IN_PROC_BROWSER_TEST_F(WebrtcApprtcBrowserTest,
MANUAL_WorksOnApprtc) {
ASSERT_TRUE(WaitForCallToComeUp(left_tab));
ASSERT_TRUE(WaitForCallToComeUp(right_tab));
+}
+
+#if defined(OS_LINUX)
+#define MAYBE_MANUAL_FirefoxApprtcInteropTest MANUAL_FirefoxApprtcInteropTest
+#else
+// Not implemented yed on Windows and Mac.
+#define MAYBE_MANUAL_FirefoxApprtcInteropTest
DISABLED_MANUAL_FirefoxApprtcInteropTest
+#endif
+
+IN_PROC_BROWSER_TEST_F(WebrtcApprtcBrowserTest,
+ MAYBE_MANUAL_FirefoxApprtcInteropTest) {
+ // TODO(mcasas): Remove Win version filtering when this bug gets fixed:
+ // http://code.google.com/p/webrtc/issues/detail?id=2703
+#if defined(OS_WIN)
+ if (base::win::GetVersion() < base::win::VERSION_VISTA)
+ return;
+#endif
+
+ DetectErrorsInJavaScript();
+ ASSERT_TRUE(LaunchApprtcInstanceOnLocalhost());
+ while (!LocalApprtcInstanceIsUp())
+ VLOG(1) << "Waiting for AppRTC to come up...";
+
+ // Run Chrome with a fake device to avoid having the browsers fight over the
+ // camera (we'll just give that to firefox here).
+ CommandLine::ForCurrentProcess()->AppendSwitch(
+ switches::kUseFakeDeviceForMediaStream);
+
+ GURL room_url = GURL(base::StringPrintf("http://localhost:9999?r=room_%d",
+ base::RandInt(0, 65536)));
+ content::WebContents* chrome_tab = OpenPageAndAcceptUserMedia(room_url);
+
+ // TODO(phoglund): Remove when this bug gets fixed:
+ // http://code.google.com/p/webrtc/issues/detail?id=1742
+ SleepInJavascript(chrome_tab, 5000);
+
+ ASSERT_TRUE(LaunchFirefoxWithUrl(room_url));
- ASSERT_TRUE(StopApprtcInstance());
+ ASSERT_TRUE(WaitForCallToComeUp(chrome_tab));
}
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/phoglund@chromium.org/111383003/90001
Message was sent while issue was closed.
Change committed as 240947 |
