Bug 219491

Created attachment 415303 [details]
Patch

Comment on attachment 415303 [details]
Patch

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

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:613
> +static id AXTextMarkerRangeStart(id textMarkerRange)

should these methods return the actual types rather than id (since we're refactoring anyway)

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:627
> +static bool getBytesFromAXTextMarker(CFTypeRef textMarker, void* bytes, size_t length)

I think we can use a 
TextMarkerData*

instead of a void* and then avoid the size parameter

Created attachment 415430 [details]
Patch

(In reply to chris fleizach from comment #2)
> Comment on attachment 415303 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415303&action=review
> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:613
> > +static id AXTextMarkerRangeStart(id textMarkerRange)
> 
> should these methods return the actual types rather than id (since we're
> refactoring anyway)

Done. More work than I anticipated, but done and tested now.
> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:627
> > +static bool getBytesFromAXTextMarker(CFTypeRef textMarker, void* bytes, size_t length)
> 
> I think we can use a 
> TextMarkerData*
> 
> instead of a void* and then avoid the size parameter

Done, good catch.

Created attachment 415491 [details]
Patch

Committed r270476: <https://trac.webkit.org/changeset/270476>

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

<rdar://problem/72012226>

Comment on attachment 415491 [details]
Patch

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

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:637
> +    memcpy(&data, AXTextMarkerGetBytePtr(textMarker), sizeof(data));

How do we know that the binary layout of the result of AXTextMarkerGetBytePtr matches TextMarkerData?

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:704
> +AXTextMarkerRef textMarkerForCharacterOffset(AXObjectCache* cache, const CharacterOffset& characterOffset)

Should functions like this return RetainPtr<AXTextMarkerRef>?

(In reply to Simon Fraser (smfr) from comment #8)
> Comment on attachment 415491 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415491&action=review
> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:637
> > +    memcpy(&data, AXTextMarkerGetBytePtr(textMarker), sizeof(data));
> 
> How do we know that the binary layout of the result of
> AXTextMarkerGetBytePtr matches TextMarkerData?

Please, see https://bugs.webkit.org/show_bug.cgi?id=219601. Added a size check. We should eventually make the definition of TextMarkerData common to both the system APIs and Webkit, but in the meantime this check has served us well for a few years already.
> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:704
> > +AXTextMarkerRef textMarkerForCharacterOffset(AXObjectCache* cache, const CharacterOffset& characterOffset)
> 
> Should functions like this return RetainPtr<AXTextMarkerRef>?

The idea is to add a class AXTextMarker that wraps and retains the system AXTextMarkerRef. This class will expose the conversions to the WebCore types, VisiblePosition, CharacterOffset, etc. For now, I don't see any downside in keeping the return values to be the raw AXTextMarkerRefs as before.