layout: Add statistics for accessibility updates. - #46385
Conversation
|
7c690da to
5f22825
Compare
Co-authored-by: delan azabani <dazabani@igalia.com> Signed-off-by: Alice Boxhall <alice@igalia.com>
Co-authored-by: Alice Boxhall <alice@igalia.com> Signed-off-by: delan azabani <dazabani@igalia.com>
Unit test wasn't expecting a tuple returned from AccessibilityUpdate::finalize(). Signed-off-by: Alice Boxhall <alice@igalia.com>
5f22825 to
05e9319
Compare
mrobinson
left a comment
There was a problem hiding this comment.
Looks good with one change request:
| pub update_node_and_descendants_from_dom_node: u32, | ||
| pub update_node_local: u32, | ||
| pub nodes_in_tree_update: u32, |
There was a problem hiding this comment.
I think using usize for these will avoid the use of try_into in this change.
There was a problem hiding this comment.
Unfortunately it needs to be a u32 at some point, because it gets written into an IDL value which is unsigned long.
There was a problem hiding this comment.
I think conversion from usize to u32 can be done without try_into() so in that direction we can avoid the uncertainty.
There was a problem hiding this comment.
I think we somehow got our wires crossed, because I'm very confused.
This value (nodes_in_tree_update) is the only one assigned with a try_into(), which is specifically necessary because it's converting from a usize (i.e. the result of changed_nodes.len()) to a u32.
That conversion itself has to happen somewhere because the value of AccessibilityUpdateResult::accessibility_nodes_in_tree_update is a u32, because it's compiled from an IDL value with a type of unsigned long.
So even if nodes_in_tree_update was a usize, and the corresponding value in ReflowStatistics was also a usize, we'd still need to do the conversion eventually when creating the AccessibilityUpdateResult in servotestutils.rs, right?
There was a problem hiding this comment.
Ah, you're right! Sorry. I'll send this to the MQ now.
There was a problem hiding this comment.
Honestly, if we managed to get 2^32 nodes in a WPT test I'd imagine we'd have bigger problems than a try_into().
For each accessibility update, track the number of times we call a couple of key methods:
update_node_and_descendants_from_dom_node()update_node_local()Also expose the number of nodes added to the
TreeUpdatewhich was produced as the output of the update, for comparison.These are made available to
ServoTestUtilsvia a new method,forceAccessibilityUpdate(), which works like the existingforceLayout()but returns anAccessibilityResultcontaining these three values.Testing: Adds new "WPT" (not really) tests using the new counters
Fixes: Part of #46346