Bug 257406 – AX: Cache AXPropertyName::RelativeFrame for AccessibilityScrollbars

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Tyler Wilcock]

...

[Radar WebKit Bug Importer]

<rdar://problem/109912633>

[Tyler Wilcock]

Created attachment 466509 [details]
Patch

[Tyler Wilcock]

Created attachment 466510 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Created attachment 466510 [details]
> Patch

Looks good, just a couple of minor comments.
--- a/Source/WebCore/accessibility/AXGeometryManager.cpp
+++ b/Source/WebCore/accessibility/AXGeometryManager.cpp

-std::optional<IntRect> AXGeometryManager::paintRectForID(AXID axID)
+std::optional<IntRect> AXGeometryManager::cachedRectForID(AXID axID)

Maybe just rectForID ?

-    if (!paintRectChanged)
+    if (!rectChanged)
         return;

     RefPtr tree = AXIsolatedTree::treeForPageID(*m_cache->pageID());

m_cache->pageID() can be nullopt, couldn't it?

+void AXObjectCache::onScrollbarFrameRectChange(const Scrollbar& scrollbar)

I guess we haven't standardized on using "Change" (as a noun) or Changed (as a past tense verb) in these functions. I can go either way but would be good to be consistent.

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #4)
> (In reply to Tyler Wilcock from comment #3)
> > Created attachment 466510 [details]
> > Patch
> 
> Looks good, just a couple of minor comments.
> --- a/Source/WebCore/accessibility/AXGeometryManager.cpp
> +++ b/Source/WebCore/accessibility/AXGeometryManager.cpp
> 
> -std::optional<IntRect> AXGeometryManager::paintRectForID(AXID axID)
> +std::optional<IntRect> AXGeometryManager::cachedRectForID(AXID axID)
> 
> Maybe just rectForID ?
I did consider rectForID, but decided against it because in my opinion that name implies that it is the way to get geometry for any object, and in reality that is not the case (only things with cacheable geometry). Plus, having "cached" in the name lends more credence to the fact that the function returns an optional (std::nullopt logically means that there was no "cached" rect).

> -    if (!paintRectChanged)
> +    if (!rectChanged)
>          return;
> 
>      RefPtr tree = AXIsolatedTree::treeForPageID(*m_cache->pageID());
> 
> m_cache->pageID() can be nullopt, couldn't it?
This method should never be called (and is not called) on a cache with no page ID -- we null check m_pageID before calling this in all places. There is an ASSERT at the top of the method verifying / documenting this expectation.

> +void AXObjectCache::onScrollbarFrameRectChange(const Scrollbar& scrollbar)
> 
> I guess we haven't standardized on using "Change" (as a noun) or Changed (as
> a past tense verb) in these functions. I can go either way but would be good
> to be consistent.
Agreed. I prefer onFooChange vs. onFooChanged. Let's do that cleanup in a separate patch.

[Tyler Wilcock]

Created attachment 466550 [details]
Patch

[EWS]

Committed 264762@main (1a3a54995b6f): <https://commits.webkit.org/264762@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 466550 [details].

[Ross Kirsling]

Fixed !ENABLE(ACCESSIBILITY) build break in https://github.com/WebKit/WebKit/pull/14582.