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

Issue 2982743002: webrtc: Fix missing port forwarding on CrOS (Closed)

Created:
3 years, 5 months ago by cywang
Modified:
3 years, 3 months 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>; Review-Url: https://codereview.chromium.org/2982743002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/1075e5dc8bc2ef383e67391639b0a23eed9f3546

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

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+78 lines, -5 lines) Patch
M telemetry/telemetry/core/cros_interface.py View 1 2 3 4 5 6 2 chunks +7 lines, -3 lines 0 comments Download
M telemetry/telemetry/core/platform.py View 1 2 3 4 5 6 3 chunks +14 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 6 1 chunk +9 lines, -0 lines 0 comments Download
M telemetry/telemetry/internal/platform/cros_platform_backend.py View 1 2 3 4 5 6 1 chunk +7 lines, -0 lines 0 comments Download
M telemetry/telemetry/internal/platform/platform_backend.py View 1 2 3 4 5 6 1 chunk +14 lines, -0 lines 0 comments Download

Messages

Total messages: 44 (18 generated)
cywang
3 years, 5 months 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 ...
3 years, 5 months 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 ...
3 years, 5 months ago (2017-07-14 02:42:07 UTC) #4
malmnas
lgtm
3 years, 5 months ago (2017-07-14 08:14:54 UTC) #5
dtosic
lgtm Thanks!
3 years, 5 months 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
3 years, 5 months 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 ...
3 years, 5 months ago (2017-07-14 08:15:42 UTC) #11
cywang
Hi Wu-cheng, Please help to review the patch, thanks!
3 years, 5 months 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 ...
3 years, 5 months 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 ...
3 years, 5 months 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: > ...
3 years, 5 months 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 ...
3 years, 5 months 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 ...
3 years, 5 months 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 ...
3 years, 5 months 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 ...
3 years, 5 months 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 ...
3 years, 5 months 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 years, 5 months 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 years, 5 months 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 years, 5 months 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, ...
3 years, 5 months 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
3 years, 5 months 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)
3 years, 5 months ago (2017-07-21 10:46:39 UTC) #34
nednguyen
lg2me overall. Though can you add a test case that would fail without this fix? ...
3 years, 5 months ago (2017-07-21 14:30:00 UTC) #35
cywang
Okay, let's focus on ChromeOS first. For unit tests, I probably don't need to add ...
3 years, 3 months ago (2017-08-28 03:31:44 UTC) #36
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/120001
3 years, 3 months ago (2017-08-28 03:38:38 UTC) #39
commit-bot: I haz the power
3 years, 3 months ago (2017-08-28 04:05:36 UTC) #42
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...

Powered by Google App Engine
This is Rietveld 408576698