Bug 255076

<rdar://problem/107694492>

Created attachment 465792 [details] Patch

Created attachment 465793 [details] Patch

Created attachment 465795 [details] Patch

Comment on attachment 465795 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=465795&action=review The approach is good. Let’s get the naming right. > Source/WebCore/page/LocalFrameView.cpp:4686 > + traverseForPaintInvalidation(NullGraphicsContext::PaintInvalidationReasons::InvalidatingAccessibilityObjectRects, &accessibilityPaintContext); There’s a naming conflict here, and I think you’ve adopted the “invalidating” term too literally. This isn’t a “invalidate the accessibility rects” paint pass, it’s a “generate the accessibility rects”. So calling “traverseForPaintInvalidation” doesn’t make sense here, suggesting we rename “traverseForPaintInvalidation()” to something like “traverseForUtilityPaint”. PaintInvalidationReasons should also be renamed. “Utility paint” is not a great name. We should think of a better one. > Source/WebCore/rendering/PaintContext.h:36 > +class PaintContext : public CanMakeCheckedPtr { This would be a UtilityPaintContext or something (it’s explicitly *not* a paint context). > Source/WebCore/rendering/RenderLayer.cpp:3176 > + bool isCollectingAccessibilityRects = context.invalidatingAccessibilityObjectRects(); This is where the mis-naming is obvious.

I see that the callers of LocalFrameView::traverseForPaintInvalidation() are LocalFrameView::invalidateControlTints() and LocalFrameView::invalidateImagesWithAsyncDecodes(). The non-invalidation one is ContentfulPaintChecker, which just calls paint() directly: NullGraphicsContext checkerContext(NullGraphicsContext::PaintInvalidationReasons::DetectingContentfulPaint); frameView.paint(checkerContext, frameView.renderView()->documentRect()); Maybe that’s the way to go for accessibility rects.

Created attachment 465945 [details] Patch

> The non-invalidation one is ContentfulPaintChecker, which just calls paint() > directly: > > NullGraphicsContext > checkerContext(NullGraphicsContext::PaintInvalidationReasons:: > DetectingContentfulPaint); > frameView.paint(checkerContext, frameView.renderView()->documentRect()); > > Maybe that’s the way to go for accessibility rects. Thanks, implemented this. As far as the name for what is currently called "PaintContext". There are two consumers of this context — event regions and accessibility. And both do very similar things: use the paint traversal to get final geometry for use in a non-directly-graphical way. So maybe this type of paint is a "geometry traversal paint", making this a GeometryTraversalContext? 

Thoughts or other ideas? 

Latest patch uses GeometryTraversalContext in case seeing the name in context helps — happy to change further if needed.

(In reply to Tyler Wilcock from comment #8) > > The non-invalidation one is ContentfulPaintChecker, which just calls paint() > > directly: > > > > NullGraphicsContext > > checkerContext(NullGraphicsContext::PaintInvalidationReasons:: > > DetectingContentfulPaint); > > frameView.paint(checkerContext, frameView.renderView()->documentRect()); > > > > Maybe that’s the way to go for accessibility rects. > Thanks, implemented this. > > As far as the name for what is currently called "PaintContext". There are > two consumers of this context — event regions and accessibility. And both do > very similar things: use the paint traversal to get final geometry for use > in a non-directly-graphical way. > > So maybe this type of paint is a "geometry traversal paint", making this a > GeometryTraversalContext? > > 

Thoughts or other ideas? > > 

Latest patch uses GeometryTraversalContext in case seeing the name in > context helps — happy to change further if needed. Wonder if a better name would be RegionContext, which then can be either the existing EventRegionContext object or a new AccessibilityRegionContext.

Created attachment 466114 [details] Patch

(In reply to chris fleizach from comment #9) > (In reply to Tyler Wilcock from comment #8) > > > The non-invalidation one is ContentfulPaintChecker, which just calls paint() > > > directly: > > > > > > NullGraphicsContext > > > checkerContext(NullGraphicsContext::PaintInvalidationReasons:: > > > DetectingContentfulPaint); > > > frameView.paint(checkerContext, frameView.renderView()->documentRect()); > > > > > > Maybe that’s the way to go for accessibility rects. > > Thanks, implemented this. > > > > As far as the name for what is currently called "PaintContext". There are > > two consumers of this context — event regions and accessibility. And both do > > very similar things: use the paint traversal to get final geometry for use > > in a non-directly-graphical way. > > > > So maybe this type of paint is a "geometry traversal paint", making this a > > GeometryTraversalContext? > > > > 

Thoughts or other ideas? > > > > 

Latest patch uses GeometryTraversalContext in case seeing the name in > > context helps — happy to change further if needed. > > Wonder if a better name would be > > RegionContext, > > which then can be either the existing EventRegionContext object or a new > AccessibilityRegionContext. OK, made this change, so should be ready to review again.

(In reply to Tyler Wilcock from comment #11) > (In reply to chris fleizach from comment #9) > > (In reply to Tyler Wilcock from comment #8) > > > > The non-invalidation one is ContentfulPaintChecker, which just calls paint() > > > > directly: > > > > > > > > NullGraphicsContext > > > > checkerContext(NullGraphicsContext::PaintInvalidationReasons:: > > > > DetectingContentfulPaint); > > > > frameView.paint(checkerContext, frameView.renderView()->documentRect()); > > > > > > > > Maybe that’s the way to go for accessibility rects. > > > Thanks, implemented this. > > > > > > As far as the name for what is currently called "PaintContext". There are > > > two consumers of this context — event regions and accessibility. And both do > > > very similar things: use the paint traversal to get final geometry for use > > > in a non-directly-graphical way. > > > > > > So maybe this type of paint is a "geometry traversal paint", making this a > > > GeometryTraversalContext? > > > > > > 

Thoughts or other ideas? > > > > > > 

Latest patch uses GeometryTraversalContext in case seeing the name in > > > context helps — happy to change further if needed. > > > > Wonder if a better name would be > > > > RegionContext, > > > > which then can be either the existing EventRegionContext object or a new > > AccessibilityRegionContext. > OK, made this change, so should be ready to review again. Looks pretty good to me

Committed 263493@main (6fa5a22eeca9): <https://commits.webkit.org/263493@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 466114 [details].