[webkit-reviews] review granted: [Bug 221875] AX: Image should report the embedded accessibility description if available : [Attachment 420756] Patch

[bugzilla-daemon at webkit.org]

Jer Noble <jer.noble at apple.com> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 221875: AX: Image should report the embedded accessibility description if
available
https://bugs.webkit.org/show_bug.cgi?id=221875

Attachment 420756: Patch

https://bugs.webkit.org/attachment.cgi?id=420756&action=review




--- Comment #23 from Jer Noble <jer.noble at apple.com> ---
Comment on attachment 420756
  --> https://bugs.webkit.org/attachment.cgi?id=420756
Patch

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

r=me with nits.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1596
> +    if (imageAttrs == nil) {

if (!imageAttrs)

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1601
> +	   auto tempArray = adoptNS([[NSMutableArray alloc]
initWithArray:attributes]);
> +	   [tempArray
addObject:NSAccessibilityEmbeddedImageDescriptionAttribute];
> +	   [tempArray addObject:NSAccessibilityURLAttribute];
> +	   imageAttrs = tempArray.leakRef();
> +    }

adoptNS(...).leakRef() seems to be a no-op, so I'm not sure what it's doing
here. This applies to all the other initializers in this function as well.

Additionally, it's a little weird that every attribute dictionary is
initialized the first time this method is called, rather than have a separate
static/NeverDestroyed method for each, so the cost is only paid when at item of
the specific type is passed in. But since this is the existing pattern, maybe
it's not a big deal.

> Source/WebCore/platform/cf/MediaAccessibilitySoftLink.h:76
> +SOFT_LINK_FUNCTION_FOR_HEADER(WebCore, MediaAccessibility,
MAImageCaptioningCopyCaptionWithSource, CFStringRef, (CGImageSourceRef
imageSource, CFErrorRef * CF_RETURNS_RETAINED error), (imageSource, error))
> +#define MAImageCaptioningCopyCaptionWithSource
softLink_MediaAccessibility_MAImageCaptioningCopyCaptionWithSource

Should this be the _MAY_FAIL variant?