Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3002663002:
Make it possible for tests to set up trace event handlers. (Closed)
|
Created:
3 years, 4 months ago by ehmaldonado_webrtc Modified:
3 years, 4 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionMake it possible for tests to set up trace event handlers.
As it is now, the first time a TRACE_EVENT... is called, the result from
the current handler is stored in a static const variable, and subsequent
calls will use that value regardless of changes to the handler.
This is a problem if a test wants to use another handler.
BUG=None
Review-Url: https://codereview.webrtc.org/3002663002
Cr-Commit-Position: refs/heads/master@{#19382}
Committed: https://chromium.googlesource.com/external/webrtc/+/a9732656221e0033ab93ecf01cf07f71817da1e8
Patch Set 1 #Patch Set 2 : Actually fix it. #
Total comments: 8
Patch Set 3 : Address comments. #
Total comments: 2
Patch Set 4 : Nit. #Messages
Total messages: 24 (9 generated)
Description was changed from ========== Make it possible for tests to set up trace event handlers. As it is now, the first time a TRACE_EVENT... is called, the result from the current handler is stored in a static const variable, and subsequent calls will use that value regardless of changes to the handler. This is a problem if a test wants to use another handler. BUG=None ========== to ========== Make it possible for tests to set up trace event handlers. As it is now, the first time a TRACE_EVENT... is called, the result from the current handler is stored in a static const variable, and subsequent calls will use that value regardless of changes to the handler. This is a problem if a test wants to use another handler. BUG=None ==========
ehmaldonado@webrtc.org changed reviewers: + hbos@webrtc.org, kwiberg@webrtc.org, mbonadei@webrtc.org
What do you think?
ehmaldonado@webrtc.org changed reviewers: + kjellander@webrtc.org
I think hbos/kwiberg is better to review here than me. https://codereview.webrtc.org/3002663002/diff/20001/webrtc/rtc_base/trace_eve... File webrtc/rtc_base/trace_event.h (right): https://codereview.webrtc.org/3002663002/diff/20001/webrtc/rtc_base/trace_eve... webrtc/rtc_base/trace_event.h:584: #endif // WEBRTC_IS_TEST // WEBRTC_NON_CONST_TRACE_EVENT_HANDLERS ?
Ok, wanted to have your opinion on defining the define there under rtc_include_tests. https://codereview.webrtc.org/3002663002/diff/20001/webrtc/rtc_base/trace_eve... File webrtc/rtc_base/trace_event.h (right): https://codereview.webrtc.org/3002663002/diff/20001/webrtc/rtc_base/trace_eve... webrtc/rtc_base/trace_event.h:584: #endif // WEBRTC_IS_TEST On 2017/08/16 11:58:57, kjellander_webrtc wrote: > // WEBRTC_NON_CONST_TRACE_EVENT_HANDLERS > ? Right, sorry.
https://codereview.webrtc.org/3002663002/diff/20001/webrtc/BUILD.gn File webrtc/BUILD.gn (right): https://codereview.webrtc.org/3002663002/diff/20001/webrtc/BUILD.gn#newcode30 webrtc/BUILD.gn:30: defines += [ "WEBRTC_NON_CONST_TRACE_EVENT_HANDLERS" ] Please add a comment explaining why this is needed and what effect it has.
https://codereview.webrtc.org/3002663002/diff/20001/webrtc/BUILD.gn File webrtc/BUILD.gn (right): https://codereview.webrtc.org/3002663002/diff/20001/webrtc/BUILD.gn#newcode30 webrtc/BUILD.gn:30: defines += [ "WEBRTC_NON_CONST_TRACE_EVENT_HANDLERS" ] Please consider always defining this (to 0 or 1, for off/on), and testing it with #if instead of #ifdef. See https://groups.google.com/d/msg/discuss-webrtc/tz0Qre1j2QU/iMTSv2lQCAAJ https://codereview.webrtc.org/3002663002/diff/20001/webrtc/rtc_base/trace_eve... File webrtc/rtc_base/trace_event.h (right): https://codereview.webrtc.org/3002663002/diff/20001/webrtc/rtc_base/trace_eve... webrtc/rtc_base/trace_event.h:578: #ifdef WEBRTC_NON_CONST_TRACE_EVENT_HANDLERS Should this be named _NON_STATIC_ instead of _NON_CONST_?
PTAL https://codereview.webrtc.org/3002663002/diff/20001/webrtc/BUILD.gn File webrtc/BUILD.gn (right): https://codereview.webrtc.org/3002663002/diff/20001/webrtc/BUILD.gn#newcode30 webrtc/BUILD.gn:30: defines += [ "WEBRTC_NON_CONST_TRACE_EVENT_HANDLERS" ] On 2017/08/16 13:11:40, kwiberg-webrtc wrote: > Please consider always defining this (to 0 or 1, for off/on), and testing it > with #if instead of #ifdef. See > https://groups.google.com/d/msg/discuss-webrtc/tz0Qre1j2QU/iMTSv2lQCAAJ Done. https://codereview.webrtc.org/3002663002/diff/20001/webrtc/rtc_base/trace_eve... File webrtc/rtc_base/trace_event.h (right): https://codereview.webrtc.org/3002663002/diff/20001/webrtc/rtc_base/trace_eve... webrtc/rtc_base/trace_event.h:578: #ifdef WEBRTC_NON_CONST_TRACE_EVENT_HANDLERS On 2017/08/16 13:11:41, kwiberg-webrtc wrote: > Should this be named _NON_STATIC_ instead of _NON_CONST_? Done. https://codereview.webrtc.org/3002663002/diff/20001/webrtc/rtc_base/trace_eve... webrtc/rtc_base/trace_event.h:584: #endif // WEBRTC_IS_TEST On 2017/08/16 12:00:45, ehmaldonado_webrtc wrote: > On 2017/08/16 11:58:57, kjellander_webrtc wrote: > > // WEBRTC_NON_CONST_TRACE_EVENT_HANDLERS > > ? > > Right, sorry. Done.
lgtm with nit https://codereview.webrtc.org/3002663002/diff/40001/webrtc/rtc_base/trace_eve... File webrtc/rtc_base/trace_event.h (right): https://codereview.webrtc.org/3002663002/diff/40001/webrtc/rtc_base/trace_eve... webrtc/rtc_base/trace_event.h:579: #define INTERNAL_TRACE_EVENT_INFO_TYPE \ nit: I think you can put this on one line. Same below.
https://codereview.webrtc.org/3002663002/diff/40001/webrtc/rtc_base/trace_eve... File webrtc/rtc_base/trace_event.h (right): https://codereview.webrtc.org/3002663002/diff/40001/webrtc/rtc_base/trace_eve... webrtc/rtc_base/trace_event.h:579: #define INTERNAL_TRACE_EVENT_INFO_TYPE \ On 2017/08/16 14:54:59, kjellander_webrtc wrote: > nit: I think you can put this on one line. Same below. Done.
lgtm
The CQ bit was checked by ehmaldonado@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/3002663002/#ps60001 (title: "Nit.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_arm64_rel/builds/...)
The CQ bit was checked by ehmaldonado@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1502971831267160,
"parent_rev": "6ff045f097529ab71fbe9233a712c4bdce3f440d", "commit_rev":
"a9732656221e0033ab93ecf01cf07f71817da1e8"}
Message was sent while issue was closed.
Description was changed from ========== Make it possible for tests to set up trace event handlers. As it is now, the first time a TRACE_EVENT... is called, the result from the current handler is stored in a static const variable, and subsequent calls will use that value regardless of changes to the handler. This is a problem if a test wants to use another handler. BUG=None ========== to ========== Make it possible for tests to set up trace event handlers. As it is now, the first time a TRACE_EVENT... is called, the result from the current handler is stored in a static const variable, and subsequent calls will use that value regardless of changes to the handler. This is a problem if a test wants to use another handler. BUG=None Review-Url: https://codereview.webrtc.org/3002663002 Cr-Commit-Position: refs/heads/master@{#19382} Committed: https://chromium.googlesource.com/external/webrtc/+/a9732656221e0033ab93ecf01... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/a9732656221e0033ab93ecf01...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.webrtc.org/2997963002/ by ehmaldonado@webrtc.org. The reason for reverting is: Breaks Chromium bots. https://build.chromium.org/p/chromium.webrtc.fyi/builders/Mac%20Builder/build.... |
