Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2534633002:
Preparation CL for WebRTC performance test using promise-based getStats (Closed)
|
Created:
4 years ago by hbos_chromium Modified:
4 years ago CC:
chromium-reviews, feature-media-reviews_chromium.org, phoglund+watch_chromium.org, mcasas+watch+vc_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPreparation CL for WebRTC performance test using promise-based getStats
In order to have a perf test looking at stats of interest we need to be
able to get the results of a JavaScript getStats call. These can be
returned as JSON-stringified JavaScript dictionaries.
In this CL:
- [g|G]etStatsReportDictionary added to webrtc_browsertest_base.cc and
peerconnection_getstats.js, which produce/read a JSON-stringified
version of the stats.
- RTCStatsReportDictionary and RTCStatsDictionary, helper classes for
reading stats from the base::Dictionary representation of the stats.
- Unittests for the helper classes.
In a follow-up CL these methods/classes will be used to obtain the
stats in the to-be-added performance test. This will yield pretty
graphs.
BUG=670306, 627816
Committed: https://crrev.com/c6ab14b0aa877c3e8e885934608d95849ff9a1a8
Cr-Commit-Position: refs/heads/master@{#435894}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Addressed nits. Report /w ForEach, Filter, Get, GetAll, GetByType (not just Filter). #Patch Set 3 : Rebase with master #Patch Set 4 : Rename RTCStats[Report]Dictionary to TestStats[Report]Dictionary #
Messages
Total messages: 44 (24 generated)
The CQ bit was checked by hbos@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== WebRtcPerformanceBrowserTest added using promise-based getStats. WIP BUG= ========== to ========== Preparation CL for WebRTC performance test using promise-based getStats In order to have a perf test looking at stats of interest we need to be able to get the results of a JavaScript getStats call. These can be returned as JSON-stringified JavaScript dictionaries. In this CL: - [g|G]etStatsReportDictionary added to webrtc_browsertest_base.cc and peerconnection_getstats.js, which produce/read a JSON-stringified version of the stats. - RTCStatsReportDictionary and RTCStatsDictionary, helper classes for reading stats from the base::Dictionary representation of the stats. - Unittests for the helper classes. In a follow-up CL these methods/classes will be used to obtain the stats in the to-be-added performance test. This will yield pretty graphs. BUG=627816 ==========
hbos@chromium.org changed reviewers: + hta@chromium.org, phoglund@chromium.org
Please take a look, phoglund and hta. https://codereview.chromium.org/2534633002/diff/60001/chrome/browser/media/we... File chrome/browser/media/webrtc/webrtc_browsertest_base.cc (right): https://codereview.chromium.org/2534633002/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc/webrtc_browsertest_base.cc:499: std::string result = ExecuteJavascript("getStatsAsDictionary()", tab); nit: I'll rename this to getStatsReportDictionary so the .cc and .js layers have matching names. https://codereview.chromium.org/2534633002/diff/60001/chrome/browser/media/we... File chrome/browser/media/webrtc/webrtc_browsertest_base.h (right): https://codereview.chromium.org/2534633002/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc/webrtc_browsertest_base.h:13: #include "base/values.h" nits: The include for values.h should be moved to the .cc file, <memory> is not needed and <vector> should be included.
Why do you need the new dictionary helper class? To make a more convenient way to get vector<bool> out of lists?
https://codereview.chromium.org/2534633002/diff/60001/chrome/test/data/webrtc... File chrome/test/data/webrtc/peerconnection_getstats.js (right): https://codereview.chromium.org/2534633002/diff/60001/chrome/test/data/webrtc... chrome/test/data/webrtc/peerconnection_getstats.js:279: returnToTest('ok-' + JSON.stringify(reportDictionary, null, ' ')); nit: "null, ' '" does pretty indented JSON, I did this for debugging and forgot about it, will remove params before landing for compact JSON.
On 2016/11/30 12:37:22, phoglund_chrome wrote: > Why do you need the new dictionary helper class? To make a more convenient way > to get vector<bool> out of lists? To make it easier to get the information, turning "Gets" into one-liners that crash if it fails, hopefully making the test code cleaner. You think it is over-kill? The same thing could be achieved with anonymous namespace helper functions operating on base::DictionaryValue, could do that instead.
On 2016/11/30 12:48:15, hbos_chromium wrote: > On 2016/11/30 12:37:22, phoglund_chrome wrote: > > Why do you need the new dictionary helper class? To make a more convenient way > > to get vector<bool> out of lists? > > To make it easier to get the information, turning "Gets" into one-liners that > crash if it fails, hopefully making the test code cleaner. > You think it is over-kill? The same thing could be achieved with anonymous > namespace helper functions operating on base::DictionaryValue, could do that > instead. It's a bit hard to tell until I see how you actually use it, but let's go with it for now :) lgtm
Please take a look, hta.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:80001) has been deleted
Hm. Adding another set of C++ classes named RTCStatsDictionary is a Bad Idea. Can you rename the new RTCStatsDictionary to TestStatsDictionary or something like that (and the same for RTCStatsReportDictionary)?
PTAL hta. On 2016/12/01 14:52:38, hta - Chromium wrote: > Can you rename the new RTCStatsDictionary to TestStatsDictionary or something > like that (and the same for RTCStatsReportDictionary)? Done. Renamed.
Patchset #4 (id:140001) has been deleted
Description was changed from ========== Preparation CL for WebRTC performance test using promise-based getStats In order to have a perf test looking at stats of interest we need to be able to get the results of a JavaScript getStats call. These can be returned as JSON-stringified JavaScript dictionaries. In this CL: - [g|G]etStatsReportDictionary added to webrtc_browsertest_base.cc and peerconnection_getstats.js, which produce/read a JSON-stringified version of the stats. - RTCStatsReportDictionary and RTCStatsDictionary, helper classes for reading stats from the base::Dictionary representation of the stats. - Unittests for the helper classes. In a follow-up CL these methods/classes will be used to obtain the stats in the to-be-added performance test. This will yield pretty graphs. BUG=627816 ========== to ========== Preparation CL for WebRTC performance test using promise-based getStats In order to have a perf test looking at stats of interest we need to be able to get the results of a JavaScript getStats call. These can be returned as JSON-stringified JavaScript dictionaries. In this CL: - [g|G]etStatsReportDictionary added to webrtc_browsertest_base.cc and peerconnection_getstats.js, which produce/read a JSON-stringified version of the stats. - RTCStatsReportDictionary and RTCStatsDictionary, helper classes for reading stats from the base::Dictionary representation of the stats. - Unittests for the helper classes. In a follow-up CL these methods/classes will be used to obtain the stats in the to-be-added performance test. This will yield pretty graphs. BUG=670306, 627816 ==========
The CQ bit was checked by hbos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by hbos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from phoglund@chromium.org Link to the patchset: https://codereview.chromium.org/2534633002/#ps160001 (title: "Rename RTCStats[Report]Dictionary to TestStats[Report]Dictionary")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
hbos@chromium.org changed reviewers: + tommi@chromium.org
Please take a look tommi, for the adding of these files: chrome/browser/media/webrtc/test_stats_dictionary.cc chrome/browser/media/webrtc/test_stats_dictionary.h chrome/browser/media/webrtc/test_stats_dictionary_unittest.cc Used by WebRtcBrowserTestBase and in follow-ups by browser tests.
lgtm
The CQ bit was checked by hbos@chromium.org
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": 160001, "attempt_start_ts": 1480670411557890,
"parent_rev": "3b34ba3ddfdb70c585b5543081393f5f72e55d9d", "commit_rev":
"8d92448b226699e67088c17ece7660bf71d9f7e0"}
Message was sent while issue was closed.
Description was changed from ========== Preparation CL for WebRTC performance test using promise-based getStats In order to have a perf test looking at stats of interest we need to be able to get the results of a JavaScript getStats call. These can be returned as JSON-stringified JavaScript dictionaries. In this CL: - [g|G]etStatsReportDictionary added to webrtc_browsertest_base.cc and peerconnection_getstats.js, which produce/read a JSON-stringified version of the stats. - RTCStatsReportDictionary and RTCStatsDictionary, helper classes for reading stats from the base::Dictionary representation of the stats. - Unittests for the helper classes. In a follow-up CL these methods/classes will be used to obtain the stats in the to-be-added performance test. This will yield pretty graphs. BUG=670306, 627816 ========== to ========== Preparation CL for WebRTC performance test using promise-based getStats In order to have a perf test looking at stats of interest we need to be able to get the results of a JavaScript getStats call. These can be returned as JSON-stringified JavaScript dictionaries. In this CL: - [g|G]etStatsReportDictionary added to webrtc_browsertest_base.cc and peerconnection_getstats.js, which produce/read a JSON-stringified version of the stats. - RTCStatsReportDictionary and RTCStatsDictionary, helper classes for reading stats from the base::Dictionary representation of the stats. - Unittests for the helper classes. In a follow-up CL these methods/classes will be used to obtain the stats in the to-be-added performance test. This will yield pretty graphs. BUG=670306, 627816 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Preparation CL for WebRTC performance test using promise-based getStats In order to have a perf test looking at stats of interest we need to be able to get the results of a JavaScript getStats call. These can be returned as JSON-stringified JavaScript dictionaries. In this CL: - [g|G]etStatsReportDictionary added to webrtc_browsertest_base.cc and peerconnection_getstats.js, which produce/read a JSON-stringified version of the stats. - RTCStatsReportDictionary and RTCStatsDictionary, helper classes for reading stats from the base::Dictionary representation of the stats. - Unittests for the helper classes. In a follow-up CL these methods/classes will be used to obtain the stats in the to-be-added performance test. This will yield pretty graphs. BUG=670306, 627816 ========== to ========== Preparation CL for WebRTC performance test using promise-based getStats In order to have a perf test looking at stats of interest we need to be able to get the results of a JavaScript getStats call. These can be returned as JSON-stringified JavaScript dictionaries. In this CL: - [g|G]etStatsReportDictionary added to webrtc_browsertest_base.cc and peerconnection_getstats.js, which produce/read a JSON-stringified version of the stats. - RTCStatsReportDictionary and RTCStatsDictionary, helper classes for reading stats from the base::Dictionary representation of the stats. - Unittests for the helper classes. In a follow-up CL these methods/classes will be used to obtain the stats in the to-be-added performance test. This will yield pretty graphs. BUG=670306, 627816 Committed: https://crrev.com/c6ab14b0aa877c3e8e885934608d95849ff9a1a8 Cr-Commit-Position: refs/heads/master@{#435894} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c6ab14b0aa877c3e8e885934608d95849ff9a1a8 Cr-Commit-Position: refs/heads/master@{#435894}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:160001) has been created in https://codereview.chromium.org/2544033003/ by hbos@chromium.org. The reason for reverting is: Compile error on http://build.chromium.org/p/chromium.webrtc/builders/Linux%20Builder/builds/8... due to uninitialized variables (warning treated as an error). False alarm warning but I'll have to init and reland.
Message was sent while issue was closed.
FYI: Findit identified this CL at revision 435894 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
On 2016/12/02 09:51:16, findit-for-me wrote: > FYI: Findit identified this CL at revision 435894 as the culprit for > failures in the build cycles as shown on: > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... There's a mismatch between CQ and waterfall compiler settings. I'll file a bug.
Message was sent while issue was closed.
On 2016/12/02 10:09:34, hbos_chromium wrote: > On 2016/12/02 09:51:16, findit-for-me wrote: > > FYI: Findit identified this CL at revision 435894 as the culprit for > > failures in the build cycles as shown on: > > > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... > > There's a mismatch between CQ and waterfall compiler settings. I'll file a bug. Oh, I used DCHECKs instead of CHECKs, so code that was doing the initialization could be ignored in optimized builds. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
