Bug 257406

<rdar://problem/109912633>

Created attachment 466509 [details] Patch

Created attachment 466510 [details] Patch

(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.

(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.

Created attachment 466550 [details] Patch

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].

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