Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 2982743002: webrtc: Fix missing port forwarding on CrOS

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 3 days ago by cywang
Modified:
2 days, 4 hours ago
CC:
catapult-reviews_chromium.org, telemetry-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
catapult
Visibility:
Public.

Description

webrtc: Fix missing port forwarding on CrOS There is a local streaming server started in webrtc benchmark for CrOS browser to connect to, however, the http server is not on the CrOS device and is not reachable from the device without remote port forwarding. The patch addresses this issue by creating a remote port forwarder if the local server and device are not on the same machine. BUG=chromium:740900 TEST=run_benchmark --browser=cros-chrome --output-format=chartjson --remote=DUT_IP webrtc Signed-off-by: Chung-yih Wang <cywang@chromium.org>;

Patch Set 1 #

Total comments: 4

Patch Set 2 : webrtc: Fix missing port forwarding on CrOS #

Total comments: 2

Patch Set 3 : webrtc: Fix missing port forwarding on CrOS #

Patch Set 4 : webrtc: Fix missing port forwarding on CrOS #

Patch Set 5 : webrtc: Fix missing port forwarding on CrOS and android #

Total comments: 4

Patch Set 6 : webrtc: Fix missing port forwarding on CrOS and android #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+84 lines, -5 lines) Patch
M telemetry/telemetry/core/cros_interface.py View 1 2 chunks +7 lines, -3 lines 0 comments Download
M telemetry/telemetry/core/platform.py View 1 2 3 4 5 3 chunks +20 lines, -0 lines 0 comments Download
M telemetry/telemetry/internal/forwarders/cros_forwarder.py View 3 chunks +27 lines, -2 lines 0 comments Download
M telemetry/telemetry/internal/platform/android_platform_backend.py View 1 2 3 4 5 1 chunk +9 lines, -0 lines 0 comments Download
M telemetry/telemetry/internal/platform/cros_platform_backend.py View 1 2 3 4 5 1 chunk +7 lines, -0 lines 0 comments Download
M telemetry/telemetry/internal/platform/platform_backend.py View 1 2 3 1 chunk +14 lines, -0 lines 2 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 35 (12 generated)
cywang
1 week, 3 days ago (2017-07-13 15:15:18 UTC) #2
malmnas
https://codereview.chromium.org/2982743002/diff/1/telemetry/telemetry/core/cros_interface.py File telemetry/telemetry/core/cros_interface.py (right): https://codereview.chromium.org/2982743002/diff/1/telemetry/telemetry/core/cros_interface.py#newcode156 telemetry/telemetry/core/cros_interface.py:156: # As remote port forwarding might conflicts with the ...
1 week, 2 days ago (2017-07-13 19:30:00 UTC) #3
cywang
Thanks! https://codereview.chromium.org/2982743002/diff/1/telemetry/telemetry/core/cros_interface.py File telemetry/telemetry/core/cros_interface.py (right): https://codereview.chromium.org/2982743002/diff/1/telemetry/telemetry/core/cros_interface.py#newcode156 telemetry/telemetry/core/cros_interface.py:156: # As remote port forwarding might conflicts with ...
1 week, 2 days ago (2017-07-14 02:42:07 UTC) #4
malmnas
lgtm
1 week, 2 days ago (2017-07-14 08:14:54 UTC) #5
dtosic
lgtm Thanks!
1 week, 2 days ago (2017-07-14 08:15:34 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2982743002/20001
1 week, 2 days ago (2017-07-14 08:15:36 UTC) #9
commit-bot: I haz the power
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the ...
1 week, 2 days ago (2017-07-14 08:15:42 UTC) #11
cywang
Hi Wu-cheng, Please help to review the patch, thanks!
6 days, 8 hours ago (2017-07-17 10:30:57 UTC) #13
achuithb
On 2017/07/17 10:30:57, cywang wrote: > Hi Wu-cheng, > > Please help to review the ...
5 days, 23 hours ago (2017-07-17 19:09:21 UTC) #14
nednguyen
https://codereview.chromium.org/2982743002/diff/20001/telemetry/telemetry/core/platform.py File telemetry/telemetry/core/platform.py (right): https://codereview.chromium.org/2982743002/diff/20001/telemetry/telemetry/core/platform.py#newcode418 telemetry/telemetry/core/platform.py:418: if not self._platform_backend._cri.local: This seems very wrong. Not all ...
5 days, 17 hours ago (2017-07-18 00:44:58 UTC) #15
cywang
https://codereview.chromium.org/2982743002/diff/20001/telemetry/telemetry/core/platform.py File telemetry/telemetry/core/platform.py (right): https://codereview.chromium.org/2982743002/diff/20001/telemetry/telemetry/core/platform.py#newcode418 telemetry/telemetry/core/platform.py:418: if not self._platform_backend._cri.local: On 2017/07/18 00:44:58, nednguyen wrote: > ...
5 days, 9 hours ago (2017-07-18 09:03:32 UTC) #16
cywang
On 2017/07/17 19:09:21, achuithb wrote: > lgtm. Have you verified that this does not break ...
5 days, 9 hours ago (2017-07-18 09:19:13 UTC) #17
nednguyen
On 2017/07/18 09:19:13, cywang wrote: > On 2017/07/17 19:09:21, achuithb wrote: > > lgtm. Have ...
5 days, 5 hours ago (2017-07-18 12:47:31 UTC) #18
cywang
On 2017/07/18 12:47:31, nednguyen wrote: > On 2017/07/18 09:19:13, cywang wrote: > > On 2017/07/17 ...
4 days, 8 hours ago (2017-07-19 10:09:19 UTC) #19
cywang
Please help to test more android devices/browsers as I do not have enough platform to ...
4 days, 8 hours ago (2017-07-19 10:13:15 UTC) #20
achuithb
unit tests? https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/internal/platform/android_platform_backend.py File telemetry/telemetry/internal/platform/android_platform_backend.py (right): https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/internal/platform/android_platform_backend.py#newcode162 telemetry/telemetry/internal/platform/android_platform_backend.py:162: # Android device is connected via adb ...
4 days ago (2017-07-19 17:50:32 UTC) #23
nednguyen
On 2017/07/19 17:50:32, achuithb wrote: > unit tests? > > https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/internal/platform/android_platform_backend.py > File telemetry/telemetry/internal/platform/android_platform_backend.py (right): ...
3 days, 16 hours ago (2017-07-20 02:28:07 UTC) #26
cywang
On 2017/07/20 02:28:07, nednguyen wrote: > On 2017/07/19 17:50:32, achuithb wrote: > > unit tests? ...
3 days, 15 hours ago (2017-07-20 03:30:17 UTC) #27
nednguyen
On 2017/07/20 03:30:17, cywang wrote: > On 2017/07/20 02:28:07, nednguyen wrote: > > On 2017/07/19 ...
3 days, 13 hours ago (2017-07-20 05:33:47 UTC) #28
cywang
I have run the unit test locally against android browser: 1021 tests passed in 4704.2s, ...
2 days, 9 hours ago (2017-07-21 09:21:23 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2982743002/100001
2 days, 8 hours ago (2017-07-21 09:45:49 UTC) #32
commit-bot: I haz the power
Try jobs failed on following builders: Catapult Linux Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Linux%20Tryserver/builds/8363)
2 days, 7 hours ago (2017-07-21 10:46:39 UTC) #34
nednguyen
2 days, 4 hours ago (2017-07-21 14:30:00 UTC) #35
lg2me overall. Though can you add a test case that would fail without this fix?
I imagine the test case would be similar to whatever is breaking webrtc
benchmark on remote CrOs device.

You can look into BrowserTest in browser_unittest.py

https://codereview.chromium.org/2982743002/diff/100001/telemetry/telemetry/in...
File telemetry/telemetry/internal/platform/platform_backend.py (right):

https://codereview.chromium.org/2982743002/diff/100001/telemetry/telemetry/in...
telemetry/telemetry/internal/platform/platform_backend.py:116: def
IsDeviceRemote(self):
s/IsDeviceRemote/IsRemoteDevice

https://codereview.chromium.org/2982743002/diff/100001/telemetry/telemetry/in...
telemetry/telemetry/internal/platform/platform_backend.py:117: """Check if
target browser is on remote device.
The platform concept in Telemetry is agnostic about browser. So the comment here
should be "Check if this platform is on a remote device"
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 25c286973