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