This test is currently timing out in ITM.
<rdar://problem/96315434>
Created attachment 460625 [details] Patch
Comment on attachment 460625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=460625&action=review > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:-401 > -- (NSString *)baseAccessibilityDescription are we using this on iOS? generally we want to move in direction where iOS and Mac can share same implementations
Comment on attachment 460625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=460625&action=review > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:383 > + AXLOG(makeString("property: ", property)); Do we want to keep this logging? If so, could we make it more succinct by replacing these two log lines with something like: AXLOG(makeString("updateNodeProperty of ", property, " for objectID ", axObject.objectID().loggingString()); > Source/WebCore/html/HTMLInputElement.cpp:1399 > + if (auto* renderer = this->renderer()) { Do we need a renderer here, or can we just use the node to get the document().existingAXObjectCache()? Sometimes we don't have a renderer (e.g. display:contents, which I doubt is valid for input elements...but still).
(In reply to chris fleizach from comment #3) > Comment on attachment 460625 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=460625&action=review > > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:-401 > > -- (NSString *)baseAccessibilityDescription > > are we using this on iOS? > generally we want to move in direction where iOS and Mac can share same > implementations Not used on iOS.
Created attachment 460760 [details] Patch
(In reply to Tyler Wilcock from comment #4) > Comment on attachment 460625 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=460625&action=review > > > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:383 > > + AXLOG(makeString("property: ", property)); > > Do we want to keep this logging? If so, could we make it more succinct by > replacing these two log lines with something like: > > AXLOG(makeString("updateNodeProperty of ", property, " for objectID ", > axObject.objectID().loggingString()); Done in latest revision. > > > Source/WebCore/html/HTMLInputElement.cpp:1399 > > + if (auto* renderer = this->renderer()) { > > Do we need a renderer here, or can we just use the node to get the > document().existingAXObjectCache()? Sometimes we don't have a renderer (e.g. > display:contents, which I doubt is valid for input elements...but still). Done.
Comment on attachment 460760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=460760&action=review > Source/WebCore/accessibility/AXObjectCache.cpp:1899 > + AXLOG("Deferred handling"); If we want to keep this log, could it include more information? I know it's nested inside an AXTRACE, but I find sometimes a lot of logs happen after an AXTRACE opening, making it hard to reference again because it's far away. Maybe we could include the same makeString from above? makeString("Deferring the handling of attribute ", attrName.localName().string(), " for element ", element->debugDescription()) What do you think? > Source/WebCore/accessibility/AXObjectCache.cpp:3405 > + AXLOG(makeString("ChildrenChangedList", m_deferredChildrenChangedList.size())); Is this log missing a space between the size and ChildrenChangedList string? Maybe we could also include the word "size" to indicate what the number means. > Source/WebCore/accessibility/AXObjectCache.cpp:3415 > + AXLOG("Baling out due to reentrant call."); Baling misspelled, should be bailing. > Source/WebCore/accessibility/AXObjectCache.cpp:3420 > + AXLOG(makeString("RecomputeTableIsExposedList ", m_deferredRecomputeTableIsExposedList.computeSize())); Should this include the word "size"? Same for the other logs in this function.
Created attachment 460767 [details] Patch
(In reply to Tyler Wilcock from comment #8) > Comment on attachment 460760 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=460760&action=review > > > Source/WebCore/accessibility/AXObjectCache.cpp:1899 > > + AXLOG("Deferred handling"); > > If we want to keep this log, could it include more information? I know it's > nested inside an AXTRACE, but I find sometimes a lot of logs happen after an > AXTRACE opening, making it hard to reference again because it's far away. > Maybe we could include the same makeString from above? > > makeString("Deferring the handling of attribute ", > attrName.localName().string(), " for element ", element->debugDescription()) > > What do you think? Replaced with suggestion and removed the one below AXTRACE to avoid duplication. > > > Source/WebCore/accessibility/AXObjectCache.cpp:3405 > > + AXLOG(makeString("ChildrenChangedList", m_deferredChildrenChangedList.size())); > > Is this log missing a space between the size and ChildrenChangedList string? > Maybe we could also include the word "size" to indicate what the number > means. > Done. > > Source/WebCore/accessibility/AXObjectCache.cpp:3415 > > + AXLOG("Baling out due to reentrant call."); > > Baling misspelled, should be bailing. > Fixed. > > Source/WebCore/accessibility/AXObjectCache.cpp:3420 > > + AXLOG(makeString("RecomputeTableIsExposedList ", m_deferredRecomputeTableIsExposedList.computeSize())); > > Should this include the word "size"? Same for the other logs in this > function. Done.
Committed 252332@main (755db3c9b12c): <https://commits.webkit.org/252332@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 460767 [details].