Decouple AXObjectCache handleChildrenChanged and postNotification.
<rdar://problem/86247404>
Created attachment 446484 [details] Patch
Comment on attachment 446484 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446484&action=review > Source/WebCore/ChangeLog:12 > + notification (postNotification), which causes a doble deferral of these doble > double > Source/WebCore/ChangeLog:16 > + added or removed, before properties in the rsulting objects may be rsulting > resulting > Source/WebCore/ChangeLog:17 > + updated. This patch fixes this problem by making handleChildrenchanged handleChildrenchanged > handleChildrenChanged > Source/WebCore/accessibility/AXObjectCache.cpp:995 > + childrenChanged(parent->parentObject()); question here - if we're going up the parent chain telling things we need to update, do we need to process this subtree any further? if a parent has changed won't it recreate the entire subtree? > Source/WebCore/accessibility/AXObjectCache.cpp:1801 > + handleChildrenChanged(*get(element->parentNode())); we probably want to defer this along with the modal change or maybe deferModalChange should also implicitly handle children changed on the paren
Created attachment 446647 [details] Patch
(In reply to chris fleizach from comment #3) > Comment on attachment 446484 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=446484&action=review > > > Source/WebCore/ChangeLog:12 > > + notification (postNotification), which causes a doble deferral of these > > doble > double Fixed. > > > Source/WebCore/ChangeLog:16 > > + added or removed, before properties in the rsulting objects may be > > rsulting > resulting Fixed. > > > Source/WebCore/ChangeLog:17 > > + updated. This patch fixes this problem by making handleChildrenchanged > > handleChildrenchanged > handleChildrenChanged Fixed. > > > Source/WebCore/accessibility/AXObjectCache.cpp:995 > > + childrenChanged(parent->parentObject()); > > question here - if we're going up the parent chain telling things we need to > update, do we need to process this subtree any further? if a parent has > changed won't it recreate the entire subtree? You're right. I thought we might need to go up the entire chain checking whether isIgnored had change, but it doesn't seem to be necessary, at least for the test cases we have. > > > Source/WebCore/accessibility/AXObjectCache.cpp:1801 > > + handleChildrenChanged(*get(element->parentNode())); > > we probably want to defer this along with the modal change > or maybe deferModalChange should also implicitly handle children changed on > the paren Done. thanks!
Created attachment 447090 [details] Patch
Created attachment 447314 [details] Patch
Created attachment 451417 [details] Patch
Comment on attachment 451417 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451417&action=review > Source/WebCore/accessibility/AXObjectCache.cpp:1049 > + postPlatformNotification(&object, AXChildrenChanged); Do we still need to post this notification since we now immediately handle this children changed event by calling AXObjectCache::childrenChanged and AXObjectCache::updateIsolatedTree?
(In reply to Tyler Wilcock from comment #9) > Comment on attachment 451417 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=451417&action=review > > > Source/WebCore/accessibility/AXObjectCache.cpp:1049 > > + postPlatformNotification(&object, AXChildrenChanged); > > Do we still need to post this notification since we now immediately handle > this children changed event by calling AXObjectCache::childrenChanged and > AXObjectCache::updateIsolatedTree? The naming isn't great here, but postPlatformNotification actually just fires the platform notification, as opposed to postNotification which is the one that causes changes in the state of the live tree and consequently in the isolated tree. That's why I see this as a decoupling between the handling the notification which means updating the trees state, and once that is done, notifying clients that something changed, which is postPlatformNotification.
(In reply to Andres Gonzalez from comment #10) > (In reply to Tyler Wilcock from comment #9) > > Comment on attachment 451417 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=451417&action=review > > > > > Source/WebCore/accessibility/AXObjectCache.cpp:1049 > > > + postPlatformNotification(&object, AXChildrenChanged); > > > > Do we still need to post this notification since we now immediately handle > > this children changed event by calling AXObjectCache::childrenChanged and > > AXObjectCache::updateIsolatedTree? > > The naming isn't great here, but postPlatformNotification actually just > fires the platform notification, as opposed to postNotification which is the > one that causes changes in the state of the live tree and consequently in > the isolated tree. That's why I see this as a decoupling between the > handling the notification which means updating the trees state, and once > that is done, notifying clients that something changed, which is > postPlatformNotification. Ahh, got it. Nice, yeah this seems like a good change.
Comment on attachment 451417 [details] Patch looks good assuming tests pass now
I think somebody clicked the "Retry failed tests" button, but that won't include the latest changes from trunk that speed up the test. A new patch might have to be uploaded.
Created attachment 451687 [details] Patch
Created attachment 451840 [details] Patch
Committed r289723 (247208@main): <https://commits.webkit.org/247208@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 451840 [details].