Bug 230558 – AX: Refactoring of TextMarkers and TextMarkerRanges support on Mac.

• Component: New Bugs
• Status: RESOLVED FIXED

[Andres Gonzalez]

Refactoring of AXTextMarker and AXTextMarkerRange support on Mac.

[Andres Gonzalez]

Created attachment 438839 [details]
Patch

[chris fleizach]

Comment on attachment 438839 [details]
Patch

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

> Source/WebCore/accessibility/AXTextMarker.cpp:29
> +#include "HTmLInputElement.h"

this casing is wrong
HTmLInputElement.h

> Source/WebCore/accessibility/AXTextMarker.h:76
> +    AXObjectCache* m_axObjectCache { nullptr };

should this be WeakPtr?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4043
> +            return (id)AXTextMarkerRange(AXTextMarker((AXTextMarkerRef)[array objectAtIndex:0]), AXTextMarker((AXTextMarkerRef)[array objectAtIndex:1])).platformObject();

is there a way to overload casting for AXTextMarker so the casting and init methods are called automatically?

[Radar WebKit Bug Importer]

<rdar://problem/83625620>

[Andres Gonzalez]

Created attachment 463912 [details]
Patch

[chris fleizach]

Comment on attachment 463912 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:2530
> +    textMarkerData.node = node;

it seems like we should be able to only store axID. then when we want the axObject again we can look it up. no need to store node or objectWrapper

[Tyler Wilcock]

Comment on attachment 463912 [details]
Patch

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

> Source/WebCore/SourcesCocoa.txt:129
> +accessibility/cocoa/AXTextMarkerCocoa.mm @no-unify

Why no-unify?

> Source/WebCore/accessibility/AXObjectCache.cpp:2516
> +    auto* object = getOrCreate(node);

Is it OK to downgrade this RefPtr to a raw pointer? Can any of the code below cause side effects that would destroy this object while we have a pointer to it?

> Source/WebCore/accessibility/AXTextMarker.cpp:29
> +#include "HTmLInputElement.h"

The capitalization seems wrong here — HTmLInputElement

> Source/WebCore/accessibility/AXTextMarker.cpp:48
> +    if (is<HTMLInputElement>(*domNode) && downcast<HTMLInputElement>(*domNode).isPasswordField())

I think both derefs of domNode on this line aren't necessary, but if you prefer this style then feel free to keep them.

> Source/WebCore/accessibility/AXTextMarker.cpp:53
> +    auto characterOffset = m_axObjectCache->characterOffsetFromVisiblePosition(visiblePosition);

Do we need to null-check m_axObjectCache?

> Source/WebCore/accessibility/AXTextMarker.h:67
> +    AXObjectCache* m_axObjectCache { nullptr };

Should this be a WeakPtr?

[Andres Gonzalez]

Created attachment 463978 [details]
Patch

[Andres Gonzalez]

Created attachment 464016 [details]
Patch

[Andres Gonzalez]

Created attachment 464017 [details]
Patch

[Tyler Wilcock]

Comment on attachment 464017 [details]
Patch

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

> Source/WebCore/SourcesCocoa.txt:129
> +accessibility/cocoa/AXTextMarkerCocoa.mm @no-unify

Why no-unify?

> Source/WebCore/accessibility/AXObjectCache.cpp:2526
> +    auto object = getOrCreate(node);

AXObjectCache::getOrCreate returns an AccessibilityObject*, so I think we either want auto* or RefPtr. If you intended auto*, is it safe to downgrade this RefPtr to a raw pointer?

> Source/WebCore/accessibility/AXObjectCache.cpp:2545
> +    // Since TextMarkerData.ignored is not assigned here, it allows to return valid TextMarkers for ignored AX objects.

I think you're missing a word here: "it allows to return"

Maybe:

// TextMarkerData.ignored is deliberately not assigned here so we can return valid TextMarkers for ignored AX objects.

> Source/WebCore/accessibility/AXObjectCache.cpp:2861
> +    auto object = cache->getOrCreate(domNode);

AXObjectCache::getOrCreate returns an AccessibilityObject*, so I think we either want auto* or RefPtr. If you intended auto*, is it safe to downgrade this RefPtr to a raw pointer?

> Source/WebCore/accessibility/AXTextMarker.cpp:64
> +/*
> + if (deepPosition.anchorType() == Position::PositionIsAfterAnchor
> +        || deepPosition.anchorType() == Position::PositionIsAfterChildren) {
> +        TextMarkerData textMarkerData;
> +        textMarkerDataForCharacterOffset(textMarkerData, characterOffset);
> +        return;
> +    }
> +*/

Do we need this code? Or is this still a WIP patch?

> Source/WebCore/accessibility/AXTextMarker.cpp:66
> +    m_axObjectCache->setNodeInUse(node);

Is it OK not to null-check the cache?

> Source/WebCore/accessibility/AXTextMarker.cpp:78
> +    if (is<HTMLInputElement>(*domNode) && downcast<HTMLInputElement>(*domNode).isPasswordField()) {

The derefs on this line aren't necessary and could even cause a crash if domNode is null.

> Source/WebCore/accessibility/AXTextMarker.cpp:84
> +    auto visiblePosition = m_axObjectCache->visiblePositionFromCharacterOffset(characterOffset);

Is it OK not to null-check the cache?

> Source/WebCore/accessibility/AXTextMarker.cpp:85
> +    unsigned vpOffset = 0;

I think WebKit style guidelines recommend spelling out all variables names, i.e. visiblePositionOffset vs. vpOffset.

> Source/WebCore/accessibility/AXTextMarker.h:70
> +    AXObjectCache* m_axObjectCache { nullptr };

Should this be a WeakPtr?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3364
> +//        auto visiblePosition = visiblePositionForTextMarker(cache, textMarker);

Can this be removed? Or this still a WIP patch?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3691
> +//            auto* cache = uiElement.get()->axObjectCache();
> +//            if (!cache)
> +//                return nil;
> +//            return AXTextMarkerRange(cache, uiElement.get()->elementRange()).platformObject();

Can this be removed?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3978
> +//        return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([&textMarkerRange, protectedSelf = retainPtr(self)] () -> RetainPtr<id> {
> +//            auto* backingObject = protectedSelf.get().axBackingObject;
> +//            if (!backingObject)
> +//                return nil;
> +//
> +//            auto range = rangeForTextMarkerRange(backingObject->axObjectCache(), textMarkerRange);
> +//
> +//            return (id)startOrEndTextMarkerForRange(backingObject->axObjectCache(), range, true);
> +//        });

Can this be removed?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3991
> +//        return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([&textMarkerRange, protectedSelf = retainPtr(self)] () -> RetainPtr<id> {
> +//            auto* backingObject = protectedSelf.get().axBackingObject;
> +//            if (!backingObject)
> +//                return nil;
> +//
> +//            auto range = rangeForTextMarkerRange(backingObject->axObjectCache(), textMarkerRange);
> +//            return (id)startOrEndTextMarkerForRange(backingObject->axObjectCache(), range, false);
> +//        });

Can this be removed?

> LayoutTests/accessibility/mac/visible-position-crash-for-text-node.html:24
> +//        output += expect("element.domIdentifier", "'content'");

Is this `expect` still valid?

[Andres Gonzalez]

Created attachment 464031 [details]
Patch

Thanks Tyler. Addressed most of your comments, but this is still a WIP. Stay tuned...

[Andres Gonzalez]

Created attachment 464045 [details]
Patch

[Andres Gonzalez]

Created attachment 464049 [details]
Patch

[chris fleizach]

Comment on attachment 464049 [details]
Patch

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

we need to do something to clean up the AccessibilityObjectWrappers that we're storing in the TextMarkerData. this is the reason we are calling setNodeInUse(), -- so that it knows to validate the node before using it again

Basically I think we need to update this to check for the validity of the object wrapper

inline bool AXTextMarker::isNull() const
 102{
 103    return !m_axObjectCache || !node() || !m_axObjectCache->isNodeInUse(node());
 104}

> Source/WebCore/accessibility/AXTextMarker.cpp:99
> +    m_data.node = node;

is it possible to structure these initializers so that we only need to write this code once, and that we have the common initialization handle this work

> Source/WebCore/accessibility/AXTextMarker.cpp:181
> +    if (isIgnored() || isNull())

do we really want to check isIgnored here?that will prevent us from using markers for ignored nodes

> Source/WebCore/accessibility/cocoa/AXTextMarkerCocoa.mm:69
> +    return (AXTextMarkerRef)&m_data;

this seems like it can't work. we're expecting to return an object that is created and retained. but here' we are just casting it.

feel like this code should be the same on Mac and iOS and probably do what macOS is doing

> Source/WebCore/accessibility/cocoa/AXTextMarkerCocoa.mm:89
> +        , adoptCF(AXTextMarkerCreate(kCFAllocatorDefault, (const UInt8*)&start.m_data, sizeof(start.m_data))).get()

can we just call platformObject() on start and end, which looks like returns the right data for us

[chris fleizach]

(In reply to chris fleizach from comment #14)
> Comment on attachment 464049 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=464049&action=review
> 
> we need to do something to clean up the AccessibilityObjectWrappers that
> we're storing in the TextMarkerData. this is the reason we are calling
> setNodeInUse(), -- so that it knows to validate the node before using it
> again
> 
> Basically I think we need to update this to check for the validity of the
> object wrapper
> 
> inline bool AXTextMarker::isNull() const
>  102{
>  103    return !m_axObjectCache || !node() ||
> !m_axObjectCache->isNodeInUse(node());
>  104}
> 
> > Source/WebCore/accessibility/AXTextMarker.cpp:99
> > +    m_data.node = node;
> 
> is it possible to structure these initializers so that we only need to write
> this code once, and that we have the common initialization handle this work
> 
> > Source/WebCore/accessibility/AXTextMarker.cpp:181
> > +    if (isIgnored() || isNull())
> 
> do we really want to check isIgnored here?that will prevent us from using
> markers for ignored nodes
> 
> > Source/WebCore/accessibility/cocoa/AXTextMarkerCocoa.mm:69
> > +    return (AXTextMarkerRef)&m_data;
> 
> this seems like it can't work. we're expecting to return an object that is
> created and retained. but here' we are just casting it.
> 
> feel like this code should be the same on Mac and iOS and probably do what
> macOS is doing
> 
> > Source/WebCore/accessibility/cocoa/AXTextMarkerCocoa.mm:89
> > +        , adoptCF(AXTextMarkerCreate(kCFAllocatorDefault, (const UInt8*)&start.m_data, sizeof(start.m_data))).get()
> 
> can we just call platformObject() on start and end, which looks like returns
> the right data for us

With us storing the objectWrapper, I wonder whether we need to also store axID() now... it seems like we could just use the wrapper for that

[Andres Gonzalez]

(In reply to chris fleizach from comment #15)
> (In reply to chris fleizach from comment #14)
> > Comment on attachment 464049 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=464049&action=review
> > 
> > we need to do something to clean up the AccessibilityObjectWrappers that
> > we're storing in the TextMarkerData. this is the reason we are calling
> > setNodeInUse(), -- so that it knows to validate the node before using it
> > again
> > 
> > Basically I think we need to update this to check for the validity of the
> > object wrapper
> > 
> > inline bool AXTextMarker::isNull() const
> >  102{
> >  103    return !m_axObjectCache || !node() ||
> > !m_axObjectCache->isNodeInUse(node());
> >  104}
> > 
> > > Source/WebCore/accessibility/AXTextMarker.cpp:99
> > > +    m_data.node = node;
> > 
> > is it possible to structure these initializers so that we only need to write
> > this code once, and that we have the common initialization handle this work
> > 
> > > Source/WebCore/accessibility/AXTextMarker.cpp:181
> > > +    if (isIgnored() || isNull())
> > 
> > do we really want to check isIgnored here?that will prevent us from using
> > markers for ignored nodes
> > 
> > > Source/WebCore/accessibility/cocoa/AXTextMarkerCocoa.mm:69
> > > +    return (AXTextMarkerRef)&m_data;
> > 
> > this seems like it can't work. we're expecting to return an object that is
> > created and retained. but here' we are just casting it.
> > 
> > feel like this code should be the same on Mac and iOS and probably do what
> > macOS is doing
> > 
> > > Source/WebCore/accessibility/cocoa/AXTextMarkerCocoa.mm:89
> > > +        , adoptCF(AXTextMarkerCreate(kCFAllocatorDefault, (const UInt8*)&start.m_data, sizeof(start.m_data))).get()
> > 
> > can we just call platformObject() on start and end, which looks like returns
> > the right data for us
> 
> With us storing the objectWrapper, I wonder whether we need to also store
> axID() now... it seems like we could just use the wrapper for that

We may be able to remove that duplication. Now it is complicated because you have things like the following in a cpp file:

VisiblePosition AXObjectCache::visiblePositionForTextMarkerData(TextMarkerData& textMarkerData)
{
...
    if (cache && !cache->m_idsInUse.contains(textMarkerData.axID))
...
and I'm not sure how to retrieve a property from an ObjC object in a cpp. We can come back to this once we figure out how to better validate the TextMarkers.

[chris fleizach]

> VisiblePosition
> AXObjectCache::visiblePositionForTextMarkerData(TextMarkerData&
> textMarkerData)
> {
> ...
>     if (cache && !cache->m_idsInUse.contains(textMarkerData.axID))
> ...
> and I'm not sure how to retrieve a property from an ObjC object in a cpp. We
> can come back to this once we figure out how to better validate the
> TextMarkers.

We would change this to

if (cache && !cache->objectWrappersInUse(textMarkerData.axObject))

[Andres Gonzalez]

Created attachment 464461 [details]
Patch

[chris fleizach]

Comment on attachment 464461 [details]
Patch

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

> COMMIT_MESSAGE:10
> +AXTextMarker encapsulates the AXIDs of the backing AXObjectCache and AXIsolatedTree, as well as the AXID of the corresponding AXCoreObject. This allow for the retrieval of the TextMarker properties without having to resource to the underlying DOM and RenderTree objects.

This allows

> Source/WebCore/accessibility/AXObjectCache.cpp:-2533
> -    textMarkerData.axID = obj->objectID();

It looks like we have a constructor for AXTextMarker that would take one of these parameters and fill in all the data here.

> Source/WebCore/accessibility/AXObjectCache.cpp:2724
> +    if (data.node != marker.m_data.node) {

this usage makes me thing textMarkerDataForPreviousCharacterOffset() should return a TextMarkerData, instead of us passing in data that we need to replace

> Source/WebCore/accessibility/AXObjectCache.cpp:-2826
> -    textMarkerData.axID = obj->objectID();

It looks like we have a constructor for AXTextMarker that would take one of these parameters and fill in all the data here.

> Source/WebCore/accessibility/AXObjectCache.cpp:2897
>      TextMarkerData textMarkerData;

can we use a TextMarker constructor here and then do

return TextMarker(object).data();

> Source/WebCore/accessibility/AXObjectCache.h:63
> +    AXID treeID;

this parameter seems like it's being used for the cacheID rather than the treeID. is that correct?

> Source/WebCore/accessibility/AXTextMarker.cpp:41
> +    Position deepPosition = visiblePosition.deepEquivalent();

auto deepPosition

> Source/WebCore/accessibility/AXTextMarker.cpp:117
> +    if (!cache)

can we combine some of this code in the constructors into helper methods? I feel like there's a lot of duplication and a lot of things to get right. for example, you have to remember to call setNodeInUse each time, rather than just having one call site

> Source/WebCore/accessibility/cocoa/AXTextMarkerCocoa.mm:57
> +RetainPtr<AXTextMarkerRef> AXTextMarker::platformObject() const

wonder if we should call this copyPlatformObject() so callers know to release it

> Source/WebCore/accessibility/cocoa/AXTextMarkerCocoa.mm:62
> +    return (AXTextMarkerRef)&m_data;

I'm worried this does not return an allocated object. I think we're going to overrelease this on iOS

> Source/WebCore/accessibility/cocoa/AXTextMarkerCocoa.mm:69
> +    , m_end(AXTextMarkerRangeCopyEndMarker(textMarkerRangeRef))

I think we need to release these start and end markers, because we copy them here, but we're not storing or releasing them

> Source/WebCore/accessibility/cocoa/AXTextMarkerCocoa.mm:77
> +        , adoptCF(AXTextMarkerCreate(kCFAllocatorDefault, (const UInt8*)&m_start.m_data, sizeof(m_start.m_data))).get()

I think we need to autorelease the start and end markers here, otherwise no one else will release them right?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3740
> +            return (id)AXTextMarker(backingObject->visiblePositionForPoint(webCorePoint)).platformObject().autorelease();

I think we can use bridgingAutorelease() and then we don't have to cast to (id)

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3830
> +        return (id)AXTextMarkerRange { { (AXTextMarkerRef)[array objectAtIndex:0] }, { (AXTextMarkerRef)[array objectAtIndex:1] } }.platformObject().autorelease();

I think we can use bridgingAutorelease() and then we don't have to cast to (id)

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3967
> +        return (id)axTextMarkerRange.start().platformObject().autorelease();

I think we can use bridgingAutorelease() and then we don't have to cast to (id)

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3972
> +        return (id)axTextMarkerRange.end().platformObject().autorelease();

I think we can use bridgingAutorelease() and then we don't have to cast to (id)

[Andres Gonzalez]

Created attachment 464516 [details]
Patch

[Andres Gonzalez]

Created attachment 464525 [details]
Patch

[Andres Gonzalez]

Created attachment 464537 [details]
Patch

[Andres Gonzalez]

Created attachment 464538 [details]
Patch

[Andres Gonzalez]

Created attachment 464545 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #19)
> Comment on attachment 464461 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=464461&action=review
> 
> > COMMIT_MESSAGE:10
> > +AXTextMarker encapsulates the AXIDs of the backing AXObjectCache and AXIsolatedTree, as well as the AXID of the corresponding AXCoreObject. This allow for the retrieval of the TextMarker properties without having to resource to the underlying DOM and RenderTree objects.
> 
> This allows

Fixed.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:-2533
> > -    textMarkerData.axID = obj->objectID();
> 
> It looks like we have a constructor for AXTextMarker that would take one of
> these parameters and fill in all the data here.

Added constructors for the TextMarkerData struct that make the code cleaner.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:2724
> > +    if (data.node != marker.m_data.node) {
> 
> this usage makes me thing textMarkerDataForPreviousCharacterOffset() should
> return a TextMarkerData, instead of us passing in data that we need to
> replace

Made several methods return an TextMarkerData instead of taking an out param.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:-2826
> > -    textMarkerData.axID = obj->objectID();
> 
> It looks like we have a constructor for AXTextMarker that would take one of
> these parameters and fill in all the data here.

Yes, same as above.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:2897
> >      TextMarkerData textMarkerData;
> 
> can we use a TextMarker constructor here and then do
> 
> return TextMarker(object).data();

Not sure that I follow this suggestion, but I did rework this code using the new constructors for TextMarkerData and methods that return TextMarkerData
> 
> > Source/WebCore/accessibility/AXObjectCache.h:63
> > +    AXID treeID;
> 
> this parameter seems like it's being used for the cacheID rather than the
> treeID. is that correct?

I use treeID for both the ID of the Cache and the ID of the IsolatedTree.
> 
> > Source/WebCore/accessibility/AXTextMarker.cpp:41
> > +    Position deepPosition = visiblePosition.deepEquivalent();
> 
> auto deepPosition
> 
> > Source/WebCore/accessibility/AXTextMarker.cpp:117
> > +    if (!cache)
> 
> can we combine some of this code in the constructors into helper methods? I
> feel like there's a lot of duplication and a lot of things to get right. for
> example, you have to remember to call setNodeInUse each time, rather than
> just having one call site

Yes,, reduced the duplication with the TextMarkerData constructors.
> 
> > Source/WebCore/accessibility/cocoa/AXTextMarkerCocoa.mm:57
> > +RetainPtr<AXTextMarkerRef> AXTextMarker::platformObject() const
> 
> wonder if we should call this copyPlatformObject() so callers know to
> release it

Renamed to platformData, since that's what it returns.
> 
> > Source/WebCore/accessibility/cocoa/AXTextMarkerCocoa.mm:62
> > +    return (AXTextMarkerRef)&m_data;

> 
> I'm worried this does not return an allocated object. I think we're going to
> overrelease this on iOS

Fixed by introducing PlatformTextMarkerData and serialization/deserialization of TextMarkerData for the specific platform.
> 
> > Source/WebCore/accessibility/cocoa/AXTextMarkerCocoa.mm:69
> > +    , m_end(AXTextMarkerRangeCopyEndMarker(textMarkerRangeRef))
> 
> I think we need to release these start and end markers, because we copy them
> here, but we're not storing or releasing them

Fixed.
> 
> > Source/WebCore/accessibility/cocoa/AXTextMarkerCocoa.mm:77
> > +        , adoptCF(AXTextMarkerCreate(kCFAllocatorDefault, (const UInt8*)&m_start.m_data, sizeof(m_start.m_data))).get()
> 
> I think we need to autorelease the start and end markers here, otherwise no
> one else will release them right?

Fixed.
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3740
> > +            return (id)AXTextMarker(backingObject->visiblePositionForPoint(webCorePoint)).platformObject().autorelease();
> 
> I think we can use bridgingAutorelease() and then we don't have to cast to
> (id)
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3830
> > +        return (id)AXTextMarkerRange { { (AXTextMarkerRef)[array objectAtIndex:0] }, { (AXTextMarkerRef)[array objectAtIndex:1] } }.platformObject().autorelease();
> 
> I think we can use bridgingAutorelease() and then we don't have to cast to
> (id)
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3967
> > +        return (id)axTextMarkerRange.start().platformObject().autorelease();
> 
> I think we can use bridgingAutorelease() and then we don't have to cast to
> (id)
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3972
> > +        return (id)axTextMarkerRange.end().platformObject().autorelease();
> 
> I think we can use bridgingAutorelease() and then we don't have to cast to
> (id)

Fixed, bridgingRelease() all of the above.

[Andres Gonzalez]

Created attachment 464546 [details]
Patch

[chris fleizach]

Comment on attachment 464546 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:2794
> +    cache->setNodeInUse(node);

should we put the nodeInUse in the contractor for the Marker? it will at least reduce chance someone forgets that step

> Source/WebCore/accessibility/AXTextMarker.cpp:41
> +    Node* node = visiblePosition.deepEquivalent().anchorNode();

auto* node

> Source/WebCore/accessibility/AXTextMarker.h:47
> +    TextMarkerData()

should we allow this to be made without arguments? aka - do we need this constructor?

> Source/WebCore/accessibility/AXTextMarker.h:58
> +        memset(static_cast<void*>(this), 0, sizeof(*this));

should we have this constructor call into the other one to reduce duplicated code?

> Source/WebCore/accessibility/AXTextMarker.h:74
> +        if (cache) {

should we assert and return error if cache is nil?

[Andres Gonzalez]

Created attachment 464563 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #27)
> Comment on attachment 464546 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=464546&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:2794
> > +    cache->setNodeInUse(node);
> 
> should we put the nodeInUse in the contractor for the Marker? it will at
> least reduce chance someone forgets that step

This is a private method of the cache, and I think it belongs there. I don't anticipate that we need to do this any where else.
> 
> > Source/WebCore/accessibility/AXTextMarker.cpp:41
> > +    Node* node = visiblePosition.deepEquivalent().anchorNode();
> 
> auto* node

Fixed.
> 
> > Source/WebCore/accessibility/AXTextMarker.h:47
> > +    TextMarkerData()
> 
> should we allow this to be made without arguments? aka - do we need this
> constructor?

We do need this default constructor because the AXTextMarker and AXTextMarkerRange classes have default constructors and thus the m_data member has to have one.
> 
> > Source/WebCore/accessibility/AXTextMarker.h:58
> > +        memset(static_cast<void*>(this), 0, sizeof(*this));
> 
> should we have this constructor call into the other one to reduce duplicated
> code?

Calling other constructor will create a different object. An init function could be added but since it is just a function call that is shared, didn't think it was worthy.
> 
> > Source/WebCore/accessibility/AXTextMarker.h:74
> > +        if (cache) {
> 
> should we assert and return error if cache is nil?

Changed it to take a cache by reference, so we don't have to check in here.

[Andres Gonzalez]

Created attachment 464582 [details]
Patch

[EWS]

Committed 259169@main (a805e7d61cce): <https://commits.webkit.org/259169@main>

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