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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by cywang
Modified:
3 weeks, 6 days 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
Commit queue not available (can’t edit this change).

Messages

Total messages: 44 (18 generated)
cywang
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week ago (2017-07-14 02:42:07 UTC) #4
malmnas
lgtm
2 months, 1 week ago (2017-07-14 08:14:54 UTC) #5
dtosic
lgtm Thanks!
2 months, 1 week 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
2 months, 1 week 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 ...
2 months, 1 week ago (2017-07-14 08:15:42 UTC) #11
cywang
Hi Wu-cheng, Please help to review the patch, thanks!
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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: > ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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): ...
2 months, 1 week 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? ...
2 months, 1 week 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 ...
2 months, 1 week 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 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
2 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)
2 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? ...
2 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 ...
4 weeks 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
4 weeks ago (2017-08-28 03:38:38 UTC) #39
commit-bot: I haz the power
4 weeks 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...
Sign in to reply to this message.

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