From 25b182c372427e798954b814b0f1a0875ab43f98 Mon Sep 17 00:00:00 2001 From: Nolan Lawson Date: Mon, 22 Apr 2024 02:16:05 -0700 Subject: [PATCH] fix(user-timing): fix clearing marks/measures by name (#32120) This fixes several tests in [wpt/user-timing](https://wpt.fyi/results/user-timing?label=master&product=chrome%5Bexperimental%5D&product=firefox%5Bexperimental%5D&product=safari%5Bexperimental%5D&product=servo&aligned) by fixing some logic errors in how marks/measures are cleared (via [`clearMarks`](https://developer.mozilla.org/en-US/docs/Web/API/Performance/clearMarks) and [`clearMeasures`](https://developer.mozilla.org/en-US/docs/Web/API/Performance/clearMeasures)). There are two changes: 1. Fix the boolean logic in `clear_entries_by_name_and_type` so that, when `clearMarks('foo')` or `clearMeasures('foo')` is called, the presence of the entry name correctly filters based on existing entry names. 2. Make the `entry_name` param a `DOMString` rather than an `Option` since every API call has it as `Some` anyway, and I'm not aware of any [Performance APIs](https://developer.mozilla.org/en-US/docs/Web/API/Performance) where you can clear all entries regardless of type. (This is not strictly required for the fix, but I think it makes the code easier to read.) ~~I also considered adding the expected WPT results using `mach update-wpt`. But I'm not sure if you want these changes, since the expectations are currently missing (i.e. `tests/wpt/meta/user-timing` does not exist).~~ (_Update: added!_) For the record, this PR fixes the following tests: - `clearMarks.html.ini` - `clearMeasures.html.ini` - `clear_non_existent_mark.any.js.ini` - `clear_non_existent_measure.any.js.ini` - `clear_one_mark.any.js.ini` - `clear_one_measure.any.js.ini` ~~In case you do want these meta files, here they are: https://github.com/nolanlawson/servo/commit/510e6146ba01bae4f4aafda8754718e24ce6e868~~ --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable) - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___ --- components/script/dom/performance.rs | 14 ++++++-------- .../user-timing/clearMarks.html.ini | 12 ------------ .../user-timing/clearMeasures.html.ini | 15 --------------- .../clear_non_existent_mark.any.js.ini | 8 -------- .../clear_non_existent_measure.any.js.ini | 8 -------- .../user-timing/clear_one_mark.any.js.ini | 8 -------- .../user-timing/clear_one_measure.any.js.ini | 8 -------- tests/wpt/meta/user-timing/clearMarks.html.ini | 12 ------------ tests/wpt/meta/user-timing/clearMeasures.html.ini | 15 --------------- .../clear_non_existent_mark.any.js.ini | 8 -------- .../clear_non_existent_measure.any.js.ini | 8 -------- .../meta/user-timing/clear_one_mark.any.js.ini | 8 -------- .../meta/user-timing/clear_one_measure.any.js.ini | 8 -------- 13 files changed, 6 insertions(+), 126 deletions(-) delete mode 100644 tests/wpt/meta-legacy-layout/user-timing/clearMarks.html.ini delete mode 100644 tests/wpt/meta-legacy-layout/user-timing/clearMeasures.html.ini delete mode 100644 tests/wpt/meta-legacy-layout/user-timing/clear_non_existent_mark.any.js.ini delete mode 100644 tests/wpt/meta-legacy-layout/user-timing/clear_non_existent_measure.any.js.ini delete mode 100644 tests/wpt/meta-legacy-layout/user-timing/clear_one_mark.any.js.ini delete mode 100644 tests/wpt/meta-legacy-layout/user-timing/clear_one_measure.any.js.ini delete mode 100644 tests/wpt/meta/user-timing/clearMarks.html.ini delete mode 100644 tests/wpt/meta/user-timing/clearMeasures.html.ini delete mode 100644 tests/wpt/meta/user-timing/clear_non_existent_mark.any.js.ini delete mode 100644 tests/wpt/meta/user-timing/clear_non_existent_measure.any.js.ini delete mode 100644 tests/wpt/meta/user-timing/clear_one_mark.any.js.ini delete mode 100644 tests/wpt/meta/user-timing/clear_one_measure.any.js.ini diff --git a/components/script/dom/performance.rs b/components/script/dom/performance.rs index e04d623346c..277662ccb42 100644 --- a/components/script/dom/performance.rs +++ b/components/script/dom/performance.rs @@ -93,13 +93,11 @@ impl PerformanceEntryList { pub fn clear_entries_by_name_and_type( &mut self, name: Option, - entry_type: Option, + entry_type: DOMString, ) { self.entries.retain(|e| { - name.as_ref().map_or(true, |name_| *e.name() != *name_) && - entry_type - .as_ref() - .map_or(true, |type_| *e.entry_type() != *type_) + *e.entry_type() != *entry_type || + name.as_ref().map_or(false, |name_| *e.name() != *name_) }); } @@ -479,7 +477,7 @@ impl PerformanceMethods for Performance { fn ClearMarks(&self, mark_name: Option) { self.buffer .borrow_mut() - .clear_entries_by_name_and_type(mark_name, Some(DOMString::from("mark"))); + .clear_entries_by_name_and_type(mark_name, DOMString::from("mark")); } // https://w3c.github.io/user-timing/#dom-performance-measure @@ -526,13 +524,13 @@ impl PerformanceMethods for Performance { fn ClearMeasures(&self, measure_name: Option) { self.buffer .borrow_mut() - .clear_entries_by_name_and_type(measure_name, Some(DOMString::from("measure"))); + .clear_entries_by_name_and_type(measure_name, DOMString::from("measure")); } // https://w3c.github.io/resource-timing/#dom-performance-clearresourcetimings fn ClearResourceTimings(&self) { self.buffer .borrow_mut() - .clear_entries_by_name_and_type(None, Some(DOMString::from("resource"))); + .clear_entries_by_name_and_type(None, DOMString::from("resource")); self.resource_timing_buffer_current_size.set(0); } diff --git a/tests/wpt/meta-legacy-layout/user-timing/clearMarks.html.ini b/tests/wpt/meta-legacy-layout/user-timing/clearMarks.html.ini deleted file mode 100644 index 3a91bd4357c..00000000000 --- a/tests/wpt/meta-legacy-layout/user-timing/clearMarks.html.ini +++ /dev/null @@ -1,12 +0,0 @@ -[clearMarks.html] - [First loop: checking entries after removing "". There should be 2 entries.] - expected: FAIL - - [First loop: checking entries after removing "1". There should be 1 entries.] - expected: FAIL - - [Second loop: checking entries after removing "". There should be 4 entries.] - expected: FAIL - - [Second loop: checking entries after removing "1". There should be 2 entries.] - expected: FAIL diff --git a/tests/wpt/meta-legacy-layout/user-timing/clearMeasures.html.ini b/tests/wpt/meta-legacy-layout/user-timing/clearMeasures.html.ini deleted file mode 100644 index 837d6a09f17..00000000000 --- a/tests/wpt/meta-legacy-layout/user-timing/clearMeasures.html.ini +++ /dev/null @@ -1,15 +0,0 @@ -[clearMeasures.html] - [First loop: checking entries after removing "". There should be 2 entries.] - expected: FAIL - - [First loop: checking entries after removing "2". There should be 1 entries.] - expected: FAIL - - [Second loop: checking entries after removing "". There should be 4 entries.] - expected: FAIL - - [Second loop: checking entries after removing "2". There should be 2 entries.] - expected: FAIL - - [Nothing should happen if we clear a non-exist measure] - expected: FAIL diff --git a/tests/wpt/meta-legacy-layout/user-timing/clear_non_existent_mark.any.js.ini b/tests/wpt/meta-legacy-layout/user-timing/clear_non_existent_mark.any.js.ini deleted file mode 100644 index 18034af5534..00000000000 --- a/tests/wpt/meta-legacy-layout/user-timing/clear_non_existent_mark.any.js.ini +++ /dev/null @@ -1,8 +0,0 @@ -[clear_non_existent_mark.any.html] - [Clearing a non-existent mark doesn't affect existing marks] - expected: FAIL - - -[clear_non_existent_mark.any.worker.html] - [Clearing a non-existent mark doesn't affect existing marks] - expected: FAIL diff --git a/tests/wpt/meta-legacy-layout/user-timing/clear_non_existent_measure.any.js.ini b/tests/wpt/meta-legacy-layout/user-timing/clear_non_existent_measure.any.js.ini deleted file mode 100644 index 79eebf30387..00000000000 --- a/tests/wpt/meta-legacy-layout/user-timing/clear_non_existent_measure.any.js.ini +++ /dev/null @@ -1,8 +0,0 @@ -[clear_non_existent_measure.any.worker.html] - [Clearing a non-existent measure doesn't affect existing measures] - expected: FAIL - - -[clear_non_existent_measure.any.html] - [Clearing a non-existent measure doesn't affect existing measures] - expected: FAIL diff --git a/tests/wpt/meta-legacy-layout/user-timing/clear_one_mark.any.js.ini b/tests/wpt/meta-legacy-layout/user-timing/clear_one_mark.any.js.ini deleted file mode 100644 index 602b6742a38..00000000000 --- a/tests/wpt/meta-legacy-layout/user-timing/clear_one_mark.any.js.ini +++ /dev/null @@ -1,8 +0,0 @@ -[clear_one_mark.any.html] - [Clearing an existent mark doesn't affect other existing marks] - expected: FAIL - - -[clear_one_mark.any.worker.html] - [Clearing an existent mark doesn't affect other existing marks] - expected: FAIL diff --git a/tests/wpt/meta-legacy-layout/user-timing/clear_one_measure.any.js.ini b/tests/wpt/meta-legacy-layout/user-timing/clear_one_measure.any.js.ini deleted file mode 100644 index d1b2d3f1212..00000000000 --- a/tests/wpt/meta-legacy-layout/user-timing/clear_one_measure.any.js.ini +++ /dev/null @@ -1,8 +0,0 @@ -[clear_one_measure.any.worker.html] - [Clearing an existent measure doesn't affect other existing measures] - expected: FAIL - - -[clear_one_measure.any.html] - [Clearing an existent measure doesn't affect other existing measures] - expected: FAIL diff --git a/tests/wpt/meta/user-timing/clearMarks.html.ini b/tests/wpt/meta/user-timing/clearMarks.html.ini deleted file mode 100644 index 3a91bd4357c..00000000000 --- a/tests/wpt/meta/user-timing/clearMarks.html.ini +++ /dev/null @@ -1,12 +0,0 @@ -[clearMarks.html] - [First loop: checking entries after removing "". There should be 2 entries.] - expected: FAIL - - [First loop: checking entries after removing "1". There should be 1 entries.] - expected: FAIL - - [Second loop: checking entries after removing "". There should be 4 entries.] - expected: FAIL - - [Second loop: checking entries after removing "1". There should be 2 entries.] - expected: FAIL diff --git a/tests/wpt/meta/user-timing/clearMeasures.html.ini b/tests/wpt/meta/user-timing/clearMeasures.html.ini deleted file mode 100644 index 837d6a09f17..00000000000 --- a/tests/wpt/meta/user-timing/clearMeasures.html.ini +++ /dev/null @@ -1,15 +0,0 @@ -[clearMeasures.html] - [First loop: checking entries after removing "". There should be 2 entries.] - expected: FAIL - - [First loop: checking entries after removing "2". There should be 1 entries.] - expected: FAIL - - [Second loop: checking entries after removing "". There should be 4 entries.] - expected: FAIL - - [Second loop: checking entries after removing "2". There should be 2 entries.] - expected: FAIL - - [Nothing should happen if we clear a non-exist measure] - expected: FAIL diff --git a/tests/wpt/meta/user-timing/clear_non_existent_mark.any.js.ini b/tests/wpt/meta/user-timing/clear_non_existent_mark.any.js.ini deleted file mode 100644 index 18034af5534..00000000000 --- a/tests/wpt/meta/user-timing/clear_non_existent_mark.any.js.ini +++ /dev/null @@ -1,8 +0,0 @@ -[clear_non_existent_mark.any.html] - [Clearing a non-existent mark doesn't affect existing marks] - expected: FAIL - - -[clear_non_existent_mark.any.worker.html] - [Clearing a non-existent mark doesn't affect existing marks] - expected: FAIL diff --git a/tests/wpt/meta/user-timing/clear_non_existent_measure.any.js.ini b/tests/wpt/meta/user-timing/clear_non_existent_measure.any.js.ini deleted file mode 100644 index 79eebf30387..00000000000 --- a/tests/wpt/meta/user-timing/clear_non_existent_measure.any.js.ini +++ /dev/null @@ -1,8 +0,0 @@ -[clear_non_existent_measure.any.worker.html] - [Clearing a non-existent measure doesn't affect existing measures] - expected: FAIL - - -[clear_non_existent_measure.any.html] - [Clearing a non-existent measure doesn't affect existing measures] - expected: FAIL diff --git a/tests/wpt/meta/user-timing/clear_one_mark.any.js.ini b/tests/wpt/meta/user-timing/clear_one_mark.any.js.ini deleted file mode 100644 index 602b6742a38..00000000000 --- a/tests/wpt/meta/user-timing/clear_one_mark.any.js.ini +++ /dev/null @@ -1,8 +0,0 @@ -[clear_one_mark.any.html] - [Clearing an existent mark doesn't affect other existing marks] - expected: FAIL - - -[clear_one_mark.any.worker.html] - [Clearing an existent mark doesn't affect other existing marks] - expected: FAIL diff --git a/tests/wpt/meta/user-timing/clear_one_measure.any.js.ini b/tests/wpt/meta/user-timing/clear_one_measure.any.js.ini deleted file mode 100644 index d1b2d3f1212..00000000000 --- a/tests/wpt/meta/user-timing/clear_one_measure.any.js.ini +++ /dev/null @@ -1,8 +0,0 @@ -[clear_one_measure.any.worker.html] - [Clearing an existent measure doesn't affect other existing measures] - expected: FAIL - - -[clear_one_measure.any.html] - [Clearing an existent measure doesn't affect other existing measures] - expected: FAIL