Etienne Pierre-Doray (Gerrit)

unread,

Aug 26, 2025, 9:22:20 AMAug 26

to Mikhail Khokhlov, Sadrul Chowdhury, Alexis Menard, Jiewei Qian, ningxin hu, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, jophba...@chromium.org, devtools-re...@chromium.org, net-r...@chromium.org, cc-...@chromium.org, mfoltz...@chromium.org, kinuko...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, core-timi...@chromium.org, bnc+...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, core-web-vita...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org, speed-metrics...@chromium.org, chrome-gr...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Mikhail Khokhlov

Etienne Pierre-Doray added 1 comment

Patchset-level comments

File-level comment, Patchset 20 (Latest):

Etienne Pierre-Doray . resolved

PTAL

Open in Gerrit

Related details

Attention is currently required from:

Mikhail Khokhlov

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Mikhail Khokhlov (Gerrit)

unread,

Aug 26, 2025, 11:49:17 AMAug 26

to Etienne Pierre-Doray, Sadrul Chowdhury, Alexis Menard, Jiewei Qian, ningxin hu, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, jophba...@chromium.org, devtools-re...@chromium.org, net-r...@chromium.org, cc-...@chromium.org, mfoltz...@chromium.org, kinuko...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, core-timi...@chromium.org, bnc+...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, core-web-vita...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org, speed-metrics...@chromium.org, chrome-gr...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Etienne Pierre-Doray

Mikhail Khokhlov voted and added 1 comment

Votes added by Mikhail Khokhlov

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 20:

Mikhail Khokhlov . resolved

LGTM

Open in Gerrit

Related details

Attention is currently required from:

Etienne Pierre-Doray

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Etienne Pierre-Doray (Gerrit)

unread,

Aug 26, 2025, 12:02:05 PMAug 26

to Dave Tapuska, Gabriel Charette, Mikhail Khokhlov, Sadrul Chowdhury, Alexis Menard, Jiewei Qian, ningxin hu, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, jophba...@chromium.org, devtools-re...@chromium.org, net-r...@chromium.org, cc-...@chromium.org, mfoltz...@chromium.org, kinuko...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, core-timi...@chromium.org, bnc+...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, core-web-vita...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org, speed-metrics...@chromium.org, chrome-gr...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Dave Tapuska and Gabriel Charette

Etienne Pierre-Doray added 1 comment

Patchset-level comments

File-level comment, Patchset 21 (Latest):

Etienne Pierre-Doray . resolved

+dtapuska@ for third_party/blink
+gab@ for base/

Open in Gerrit

Related details

Attention is currently required from:

Dave Tapuska
Gabriel Charette

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dave Tapuska (Gerrit)

unread,

Aug 26, 2025, 12:06:17 PMAug 26

to Etienne Pierre-Doray, Gabriel Charette, Mikhail Khokhlov, Sadrul Chowdhury, Alexis Menard, Jiewei Qian, ningxin hu, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, jophba...@chromium.org, devtools-re...@chromium.org, net-r...@chromium.org, cc-...@chromium.org, mfoltz...@chromium.org, kinuko...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, core-timi...@chromium.org, bnc+...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, core-web-vita...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org, speed-metrics...@chromium.org, chrome-gr...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Etienne Pierre-Doray and Gabriel Charette

Dave Tapuska voted and added 5 comments

Votes added by Dave Tapuska

Code-Review

+1

5 comments

Patchset-level comments

File-level comment, Patchset 21 (Latest):

Dave Tapuska . resolved

blink lgtm % nits

File third_party/blink/renderer/core/css/element_rule_collector_test.cc

Line 739, Patchset 21 (Latest): base::test::TracingEnvironment tracing_environment_;

Dave Tapuska . unresolved

No trailing underscore

File third_party/blink/renderer/core/paint/paint_and_raster_invalidation_test.cc

Line 96, Patchset 21 (Latest): base::test::TracingEnvironment tracing_environment;

Dave Tapuska . unresolved

private:

trailing underscore on variable name too since it is a member variable.

File third_party/blink/renderer/core/paint/paint_layer_painter_test.cc

Line 832, Patchset 21 (Latest): base::test::TracingEnvironment tracing_environment_;

Dave Tapuska . unresolved

No trailing underscore for variable in this scope.

File third_party/blink/renderer/core/paint/timing/image_paint_timing_detector_test.cc

Line 290, Patchset 21 (Latest): base::test::TracingEnvironment tracing_environment_;

Dave Tapuska . unresolved

Can this be moved into the private section?

Open in Gerrit

Related details

Attention is currently required from:

Etienne Pierre-Doray
Gabriel Charette

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Gabriel Charette (Gerrit)

unread,

Aug 26, 2025, 12:58:41 PMAug 26

to Etienne Pierre-Doray, Gabriel Charette, Dave Tapuska, Mikhail Khokhlov, Sadrul Chowdhury, Alexis Menard, Jiewei Qian, ningxin hu, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, jophba...@chromium.org, devtools-re...@chromium.org, net-r...@chromium.org, cc-...@chromium.org, mfoltz...@chromium.org, kinuko...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, core-timi...@chromium.org, bnc+...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, core-web-vita...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org, speed-metrics...@chromium.org, chrome-gr...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Etienne Pierre-Doray

Gabriel Charette voted and added 2 comments

Votes added by Gabriel Charette

Code-Review	+1
Owners-Override	+1

2 comments

Patchset-level comments

File-level comment, Patchset 21 (Latest):

Gabriel Charette . resolved

LGTM w/ comment, and O-O for trivial things outside //base

File third_party/blink/renderer/core/paint/paint_and_raster_invalidation_test.cc

Line 99, Patchset 21 (Latest):TEST_P(PaintAndRasterInvalidationTest, TrackingForTracing) {

Gabriel Charette . unresolved

Doesn't this fixture already have a `TracingEnvironment` added in its .h? Why does the `ScopedEnablePaintInvalidationTracing` also need one?

Open in Gerrit

Related details

Attention is currently required from:

Etienne Pierre-Doray

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Etienne Pierre-Doray (Gerrit)

unread,

Aug 26, 2025, 3:15:07 PMAug 26

to Gabriel Charette, Dave Tapuska, Mikhail Khokhlov, Sadrul Chowdhury, Alexis Menard, Jiewei Qian, ningxin hu, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, jophba...@chromium.org, devtools-re...@chromium.org, net-r...@chromium.org, cc-...@chromium.org, mfoltz...@chromium.org, kinuko...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, core-timi...@chromium.org, bnc+...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, core-web-vita...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org, speed-metrics...@chromium.org, chrome-gr...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Etienne Pierre-Doray voted and added 5 comments

Votes added by Etienne Pierre-Doray

Commit-Queue

+1

5 comments

File third_party/blink/renderer/core/css/element_rule_collector_test.cc

Line 739, Patchset 21: base::test::TracingEnvironment tracing_environment_;

Dave Tapuska . resolved

No trailing underscore

Etienne Pierre-Doray

Done

File third_party/blink/renderer/core/paint/paint_and_raster_invalidation_test.cc

Line 96, Patchset 21: base::test::TracingEnvironment tracing_environment;

Dave Tapuska . resolved

private:
trailing underscore on variable name too since it is a member variable.

Etienne Pierre-Doray

N/A

Line 99, Patchset 21:TEST_P(PaintAndRasterInvalidationTest, TrackingForTracing) {

Gabriel Charette . resolved

Doesn't this fixture already have a `TracingEnvironment` added in its .h? Why does the `ScopedEnablePaintInvalidationTracing` also need one?

Etienne Pierre-Doray

Correct, I removed the one in ScopedEnablePaintInvalidationTracing.

File third_party/blink/renderer/core/paint/paint_layer_painter_test.cc

Line 832, Patchset 21: base::test::TracingEnvironment tracing_environment_;

Dave Tapuska . resolved

No trailing underscore for variable in this scope.

Etienne Pierre-Doray

Done

File third_party/blink/renderer/core/paint/timing/image_paint_timing_detector_test.cc

Line 290, Patchset 21: base::test::TracingEnvironment tracing_environment_;

Dave Tapuska . resolved

Can this be moved into the private section?

Etienne Pierre-Doray

Done

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Etienne Pierre-Doray (Gerrit)

unread,

Aug 26, 2025, 3:15:22 PMAug 26

to Gabriel Charette, Dave Tapuska, Mikhail Khokhlov, Sadrul Chowdhury, Alexis Menard, Jiewei Qian, ningxin hu, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, jophba...@chromium.org, devtools-re...@chromium.org, net-r...@chromium.org, cc-...@chromium.org, mfoltz...@chromium.org, kinuko...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, core-timi...@chromium.org, bnc+...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, core-web-vita...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org, speed-metrics...@chromium.org, chrome-gr...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Etienne Pierre-Doray voted Commit-Queue+2

Commit-Queue

+2

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Aug 26, 2025, 3:18:52 PMAug 26

to Etienne Pierre-Doray, Gabriel Charette, Dave Tapuska, Mikhail Khokhlov, Sadrul Chowdhury, Alexis Menard, Jiewei Qian, ningxin hu, AyeAye, chromium...@chromium.org, jophba...@chromium.org, devtools-re...@chromium.org, net-r...@chromium.org, cc-...@chromium.org, mfoltz...@chromium.org, kinuko...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, core-timi...@chromium.org, bnc+...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, core-web-vita...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org, speed-metrics...@chromium.org, chrome-gr...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

21 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: third_party/blink/renderer/core/paint/paint_and_raster_invalidation_test.cc
Insertions: 0, Deletions: 2.

@@ -92,8 +92,6 @@
     trace_event::EnableTracing(TRACE_DISABLED_BY_DEFAULT("blink.invalidation"));
   }
   ~ScopedEnablePaintInvalidationTracing() { trace_event::DisableTracing(); }
-
-  base::test::TracingEnvironment tracing_environment;
 };
 
 TEST_P(PaintAndRasterInvalidationTest, TrackingForTracing) {
```
```
The name of the file: third_party/blink/renderer/core/paint/paint_layer_painter_test.cc
Insertions: 1, Deletions: 1.

@@ -829,7 +829,7 @@
 }
 
 TEST_P(PaintLayerPainterTest, DevtoolsPaintTraceEvents) {
-  base::test::TracingEnvironment tracing_environment_;
+  base::test::TracingEnvironment tracing_environment;
   SetBodyInnerHTML(R"HTML(
     <div id=scroller style="width: 400px; height: 400px; overflow-y: scroll;
                             position: relative">
```
```
The name of the file: third_party/blink/renderer/core/css/element_rule_collector_test.cc
Insertions: 1, Deletions: 1.

@@ -736,7 +736,7 @@
     const StyleRule* rule);
 
 TEST_F(ElementRuleCollectorTest, FindStyleSheet) {
-  base::test::TracingEnvironment tracing_environment_;
+  base::test::TracingEnvironment tracing_environment;
   trace_analyzer::Start(
       TRACE_DISABLED_BY_DEFAULT("devtools.timeline.invalidationTracking"));
   InvalidationSetToSelectorMap::StartOrStopTrackingIfNeeded(
```
```
The name of the file: third_party/blink/renderer/core/paint/timing/image_paint_timing_detector_test.cc
Insertions: 1, Deletions: 1.

@@ -287,7 +287,6 @@
   }
 
   test::TaskEnvironment task_environment_;
-  base::test::TracingEnvironment tracing_environment_;
   scoped_refptr<base::TestMockTimeTaskRunner> test_task_runner_;
   frame_test_helpers::WebViewHelper web_view_helper_;
 
@@ -315,6 +314,7 @@
     return original_image_content;
   }
 
+  base::test::TracingEnvironment tracing_environment_;
   MockPaintTimingCallbackManager::CallbackQueue callback_queue_;
   Persistent<MockPaintTimingCallbackManager> mock_callback_manager_;
   Persistent<MockPaintTimingCallbackManager> child_mock_callback_manager_;
```

Change information

Commit message:

[tracing] Improve test::TracingEnvironment

This CL makes test::TracingEnvironment better isolated.
TraceLog::SetEnabled is now stricter about being test only,
and doesn't leave tracing initialized, forcing TracingEnvironment
to be used in all tests that run tracing, thus a big
part of this CL is adding missing base::test::TracingEnvironment.

Change-Id: I222c06729f1de4089097f349b19ecafe49b0d730

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6875667

Reviewed-by: Mikhail Khokhlov <khok...@google.com>

Reviewed-by: Dave Tapuska <dtap...@chromium.org>

Owners-Override: Gabriel Charette <g...@chromium.org>

Commit-Queue: Etienne Pierre-Doray <etie...@chromium.org>

Reviewed-by: Gabriel Charette <g...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1506696}

Files:

M base/message_loop/message_pump_glib_unittest.cc
M base/test/test_trace_processor.cc
M base/test/trace_event_analyzer_unittest.cc
M base/test/trace_test_utils.cc
M base/test/trace_test_utils.h
M base/test/trace_to_file.cc
M base/trace_event/memory_dump_manager_unittest.cc
M base/trace_event/trace_event_impl.cc
M base/trace_event/trace_event_impl.h
M base/trace_event/trace_event_unittest.cc
M base/trace_event/trace_log.cc
M base/trace_event/trace_log.h
M chrome/browser/page_load_metrics/observers/core/ukm_page_load_metrics_observer_unittest.cc
M components/tracing/test/trace_event_perftest.cc
M components/viz/common/yuv_readback_unittest.cc
M gin/v8_isolate_memory_dump_provider_unittest.cc
M net/log/trace_net_log_observer_unittest.cc
M services/resource_coordinator/public/cpp/memory_instrumentation/tracing_integration_unittest.cc
M services/tracing/public/cpp/perfetto/producer_test_utils.h
M services/webnn/public/cpp/webnn_trace_unittest.cc
M third_party/blink/renderer/core/css/element_rule_collector_test.cc
M third_party/blink/renderer/core/inspector/invalidation_set_to_selector_map_test.cc
M third_party/blink/renderer/core/paint/DEPS
M third_party/blink/renderer/core/paint/paint_and_raster_invalidation_test.cc
M third_party/blink/renderer/core/paint/paint_and_raster_invalidation_test.h
M third_party/blink/renderer/core/paint/paint_layer_painter_test.cc
M third_party/blink/renderer/core/paint/timing/image_paint_timing_detector_test.cc
M third_party/blink/renderer/core/paint/timing/largest_contentful_paint_calculator_test.cc
M third_party/blink/renderer/core/paint/timing/text_paint_timing_detector_test.cc
M third_party/blink/renderer/core/timing/window_performance_test.cc
M third_party/blink/renderer/core/timing/worker_performance_test.cc
M third_party/blink/renderer/platform/widget/input/input_handler_proxy_unittest.cc

Change size: M

Delta: 32 files changed, 126 insertions(+), 65 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Dave Tapuska, +1 by Mikhail Khokhlov, +1 by Gabriel Charette

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[tracing] Improve test::TracingEnvironment [chromium/src : main]

Etienne Pierre-Doray (Gerrit)

Etienne Pierre-Doray added 1 comment

Related details

Mikhail Khokhlov (Gerrit)

Mikhail Khokhlov voted and added 1 comment

Votes added by Mikhail Khokhlov

1 comment

Related details

Etienne Pierre-Doray (Gerrit)

Etienne Pierre-Doray added 1 comment

Related details

Dave Tapuska (Gerrit)

Dave Tapuska voted and added 5 comments

Votes added by Dave Tapuska

5 comments

Related details

Gabriel Charette (Gerrit)

Gabriel Charette voted and added 2 comments

Votes added by Gabriel Charette

2 comments

Related details

Etienne Pierre-Doray (Gerrit)

Etienne Pierre-Doray voted and added 5 comments

Votes added by Etienne Pierre-Doray

5 comments

Related details

Etienne Pierre-Doray (Gerrit)

Etienne Pierre-Doray voted Commit-Queue+2

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information