Now launching WebRTC-AppRTC test with Collider.

Now launching WebRTC-AppRTC test with Collider.

AppRTC is about to switch to an architecture where there is an
additional server component called the collider. This patch brings up
the collider binary prior to running the test.

Land this when merging the apprtc-collider branch into the apprtc master branch.

BUG=438217
Committed: https://crrev.com/93f5e0dd5328abeb894b4ed006949bf8dc5c475c
Cr-Commit-Position: refs/heads/master@{#308776}

[phoglund_chromium, 2014-12-08 12:19:50]

phoglund@chromium.org changed reviewers:
+ kjellander@chromium.org, tommi@chromium.org

[phoglund_chromium, 2014-12-08 12:19:51]

phoglund@chromium.org changed required reviewers:
+ tommi@chromium.org

[phoglund_chromium, 2014-12-08 12:19:52]

Henrik: main review
Tommi: owner stamp

[kjellander_chromium, 2014-12-08 14:57:43]

lgtm, although I'd like to see some code duplication reduction by introducing a
few constants.

https://codereview.chromium.org/751333004/diff/20001/chrome/browser/media/chr...
File chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc (right):

https://codereview.chromium.org/751333004/diff/20001/chrome/browser/media/chr...
chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:247:
ASSERT_TRUE(LaunchColliderOnLocalHost("http://localhost:9999", "8089"));
I'd suggest extracting a few constants here...

https://codereview.chromium.org/751333004/diff/20001/chrome/browser/media/chr...
chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:251: GURL room_url =
GURL("http://localhost:9999/r/some_room"
Create a constant? Then you could just append the firefox parameter
(&firefox_fake_device=1) to that one.

[phoglund_chromium, 2014-12-08 15:28:32]

https://codereview.chromium.org/751333004/diff/20001/chrome/browser/media/chr...
File chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc (right):

https://codereview.chromium.org/751333004/diff/20001/chrome/browser/media/chr...
chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:247:
ASSERT_TRUE(LaunchColliderOnLocalHost("http://localhost:9999", "8089"));
On 2014/12/08 14:57:43, kjellander wrote:
> I'd suggest extracting a few constants here...

Not sure if that makes stuff better. Sure, I mention 9999 in several places but
it makes it very readable what port 9999 is for. I could extract a apprtc_port
variable but then I'd have to stringprintf stuff together.

https://codereview.chromium.org/751333004/diff/20001/chrome/browser/media/chr...
chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc:251: GURL room_url =
GURL("http://localhost:9999/r/some_room"
On 2014/12/08 14:57:43, kjellander wrote:
> Create a constant? Then you could just append the firefox parameter
> (&firefox_fake_device=1) to that one.

Yeah, but then the juicy parts about how stuff is wired together kind move away
from the test itself and into constants at the top of the file. I think it's
worth the small amounts of duplication for the clarity.

[tommi (sloooow) - chröme, 2014-12-08 21:04:32]

rs lgtm

[jiayl, 2014-12-16 18:54:24]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-12-16 18:55:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/751333004/80001

[commit-bot: I haz the power, 2014-12-16 19:02:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-16 19:02:22]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/99593)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[phoglund_chromium, 2014-12-17 09:48:20]

Rebased and tried out locally with latest the apprtc-collider branch. It appears
to still work, so I'm submitting.

[phoglund_chromium, 2014-12-17 09:48:27]

The CQ bit was checked by phoglund@chromium.org

[commit-bot: I haz the power, 2014-12-17 09:49:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/751333004/100001

[commit-bot: I haz the power, 2014-12-17 10:35:46]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-17 10:36:37]

Patchset 6 (id:??) landed as
https://crrev.com/93f5e0dd5328abeb894b4ed006949bf8dc5c475c
Cr-Commit-Position: refs/heads/master@{#308776}