<rdar://problem/101804648>

Created attachment 463343 [details]
Patch

Comment on attachment 463343 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=463343&action=review

Would objects outside the main-document for other reasons (e.g. they're in an iframe) have the same problem that SVG objects do?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:336
> +        AXLOG("Bail out here, we can't build an isolated tree branch rooted at a detached object.");

If we're changing this comment to a log, the "Bail out here" portion doesn't have the same context. Maybe it should instead saying something like:

"collectNodeChangesForSubtree bailing for object ID %s because it is detached"

Created attachment 463356 [details]
Patch

(In reply to Tyler Wilcock from comment #3)
> Comment on attachment 463343 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=463343&action=review
> 
> Would objects outside the main-document for other reasons (e.g. they're in
> an iframe) have the same problem that SVG objects do?

I think iframes documents share the same top document with the main document, so it wouldn't have the same problem.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:336
> > +        AXLOG("Bail out here, we can't build an isolated tree branch rooted at a detached object.");
> 
> If we're changing this comment to a log, the "Bail out here" portion doesn't
> have the same context. Maybe it should instead saying something like:
> 
> "collectNodeChangesForSubtree bailing for object ID %s because it is
> detached"

Removed that part of the log message. It is clear in the log that this message is in the scope of this method.

Created attachment 463360 [details]
Patch

Comment on attachment 463360 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=463360&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:799
> +    RefPtr object = createObjectFromRenderer(renderer);

we don't need the type of the RefPtr anymore?

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:51
> +    ASSERT(renderer);

can we ASSERT(cache) here too

Created attachment 463367 [details]
Patch

(In reply to chris fleizach from comment #7)
> Comment on attachment 463360 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=463360&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:799
> > +    RefPtr object = createObjectFromRenderer(renderer);
> 
> we don't need the type of the RefPtr anymore?

No, compiler magic inference.
> 
> > Source/WebCore/accessibility/AccessibilitySVGElement.cpp:51
> > +    ASSERT(renderer);
> 
> can we ASSERT(cache) here too

Done.

Committed 256261@main (c98a2ded1d00): <https://commits.webkit.org/256261@main>

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