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.