Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2909653003:
Re-enable the multiple_peerconnections story and disable webrtc traces and metrics. (Closed)
|
Created:
3 years, 7 months ago by ehmaldonado_chromium Modified:
3 years, 7 months ago CC:
chromium-reviews, telemetry-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRe-enable the multiple_peerconnections story and disable webrtc traces and metrics.
BUG=chromium:725502
Review-Url: https://codereview.chromium.org/2909653003
Cr-Commit-Position: refs/heads/master@{#475190}
Committed: https://chromium.googlesource.com/chromium/src/+/09f367096ac4724bd0a14d86a346317e4cc68500
Patch Set 1 #
Total comments: 3
Patch Set 2 : Rebase. Disable audio the nice way. Reduce the load of the multiple_peerconnections story. #
Total comments: 2
Patch Set 3 : Address comment. #
Messages
Total messages: 20 (6 generated)
ehmaldonado@chromium.org changed reviewers: + charliea@chromium.org, kjellander@chromium.org, nednguyen@google.com, rnephew@chromium.org
PTAL
On 2017/05/26 17:03:36, ehmaldonado_chromium wrote: > PTAL non-owner LGTM.
https://codereview.chromium.org/2909653003/diff/1/tools/perf/page_sets/webrtc... File tools/perf/page_sets/webrtc_cases.py (right): https://codereview.chromium.org/2909653003/diff/1/tools/perf/page_sets/webrtc... tools/perf/page_sets/webrtc_cases.py:143: pass this is wrong since it gonna return None. Randy: what should we do here?
https://codereview.chromium.org/2909653003/diff/1/tools/perf/page_sets/webrtc... File tools/perf/page_sets/webrtc_cases.py (right): https://codereview.chromium.org/2909653003/diff/1/tools/perf/page_sets/webrtc... tools/perf/page_sets/webrtc_cases.py:135: # self.AddStory(AudioCall(self, 'OPUS')) These disabling seem should be converted to the SetExpectations below as well.
https://codereview.chromium.org/2909653003/diff/1/tools/perf/page_sets/webrtc... File tools/perf/page_sets/webrtc_cases.py (right): https://codereview.chromium.org/2909653003/diff/1/tools/perf/page_sets/webrtc... tools/perf/page_sets/webrtc_cases.py:143: pass On 2017/05/26 18:30:52, nednguyen wrote: > this is wrong since it gonna return None. Randy: what should we do here? Its fine. This is the default for this method: https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... Here is where SetExpecation is used: https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... I think you have this confused with GetExpectations which is in benchmark.py: https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...
On 2017/05/26 18:51:52, rnephew (Reviews Here) wrote: > https://codereview.chromium.org/2909653003/diff/1/tools/perf/page_sets/webrtc... > File tools/perf/page_sets/webrtc_cases.py (right): > > https://codereview.chromium.org/2909653003/diff/1/tools/perf/page_sets/webrtc... > tools/perf/page_sets/webrtc_cases.py:143: pass > On 2017/05/26 18:30:52, nednguyen wrote: > > this is wrong since it gonna return None. Randy: what should we do here? > > Its fine. This is the default for this method: > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > Here is where SetExpecation is used: > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > I think you have this confused with GetExpectations which is in benchmark.py: > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... Ah great. :-)
lgtm but you will need to rebase. Please keep Charlie's disabling
On 2017/05/26 20:11:20, nednguyen wrote: > lgtm but you will need to rebase. Please keep Charlie's disabling Not sure if you saw the last patch.
https://codereview.chromium.org/2909653003/diff/20001/tools/perf/page_sets/we... File tools/perf/page_sets/webrtc_cases.py (right): https://codereview.chromium.org/2909653003/diff/20001/tools/perf/page_sets/we... tools/perf/page_sets/webrtc_cases.py:144: for codec in CODECS: No, please don't do this as it makes sheriffs super confused about which stories are disabled & which are not. You should just manually list out all the story that are disabled.
https://codereview.chromium.org/2909653003/diff/20001/tools/perf/page_sets/we... File tools/perf/page_sets/webrtc_cases.py (right): https://codereview.chromium.org/2909653003/diff/20001/tools/perf/page_sets/we... tools/perf/page_sets/webrtc_cases.py:135: for codec in CODECS: I also don't think code saving here is a good idea.
Patchset #3 (id:40001) has been deleted
On 2017/05/26 20:17:07, nednguyen wrote: > https://codereview.chromium.org/2909653003/diff/20001/tools/perf/page_sets/we... > File tools/perf/page_sets/webrtc_cases.py (right): > > https://codereview.chromium.org/2909653003/diff/20001/tools/perf/page_sets/we... > tools/perf/page_sets/webrtc_cases.py:135: for codec in CODECS: > I also don't think code saving here is a good idea. OK, PTAL
lgtm
The CQ bit was checked by ehmaldonado@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rnephew@chromium.org Link to the patchset: https://codereview.chromium.org/2909653003/#ps60001 (title: "Address comment.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495837543714130,
"parent_rev": "2862c87c6a08b3f8943ea6291ed5c37640c03aac", "commit_rev":
"09f367096ac4724bd0a14d86a346317e4cc68500"}
Message was sent while issue was closed.
Description was changed from ========== Re-enable the multiple_peerconnections story and disable webrtc traces and metrics. BUG=chromium:725502 ========== to ========== Re-enable the multiple_peerconnections story and disable webrtc traces and metrics. BUG=chromium:725502 Review-Url: https://codereview.chromium.org/2909653003 Cr-Commit-Position: refs/heads/master@{#475190} Committed: https://chromium.googlesource.com/chromium/src/+/09f367096ac4724bd0a14d86a346... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/09f367096ac4724bd0a14d86a346... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
