RESOLVED FIXED258998
AX: Several classes never reset m_subtreeDirty to false 
https://bugs.webkit.org/show_bug.cgi?id=258998
Summary AX: Several classes never reset m_subtreeDirty to false
Tyler Wilcock
Reported 2023-07-07 15:11:11 PDT
This causes several performance issues.
Attachments
Patch (14.26 KB, patch)
2023-07-07 15:24 PDT, Tyler Wilcock 		no flags
DetailsFormatted DiffDiff
Patch (14.27 KB, patch)
2023-07-07 20:52 PDT, Tyler Wilcock 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2023-07-07 15:11:24 PDT
<rdar://problem/111929312>
Tyler Wilcock
Comment 2 2023-07-07 15:24:41 PDT
Created attachment 466985 [details] Patch
chris fleizach
Comment 3 2023-07-07 15:29:26 PDT
Comment on attachment 466985 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=466985&action=review > Source/WebCore/accessibility/AccessibilityTable.cpp:683 > + m_subtreeDirty = false; do we need the clearDirtySubtree scopeExit and this one here? > Source/WebCore/accessibility/AccessibilityTableRow.cpp:147 > + m_subtreeDirty = false; should we have a scope exit here?
Tyler Wilcock
Comment 4 2023-07-07 18:54:47 PDT
(In reply to chris fleizach from comment #3) > Comment on attachment 466985 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=466985&action=review > > > Source/WebCore/accessibility/AccessibilityTable.cpp:683 > > + m_subtreeDirty = false; > > do we need the clearDirtySubtree scopeExit and this one here? This function does have a clearDirtySubtree-scopeExit, but I intentionally want to make sure to set `m_subtreeDirty = false` before updateChildrenRoles() to avoid unnecessarily passing a dirty subtree flag around if child roles do get updated (since that kicks off a lot of other operations). > > Source/WebCore/accessibility/AccessibilityTableRow.cpp:147 > > + m_subtreeDirty = false; > > should we have a scope exit here? In my opinion, it's unnecessary for this one since both branches in this if-statement now cover modifying the flag (with the `else` modifying it through AccessibilityRenderObject::addChildren()). I don't feel overly strong on either of these points if you have a different preference, though.
Tyler Wilcock
Comment 5 2023-07-07 20:52:51 PDT
Created attachment 466987 [details] Patch
EWS
Comment 6 2023-07-08 00:23:07 PDT
Committed 265878@main (2517a540e6f5): <https://commits.webkit.org/265878@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 466987 [details].
Note You need to log in before you can comment on or make changes to this bug.