[webkit-reviews] review granted: [Bug 97571] AX: Support embedded SVG objects in AX tree : [Attachment 166614] patch

[bugzilla-daemon at webkit.org]

Tim Horton <timothy_horton at apple.com> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 97571: AX: Support embedded SVG objects in AX tree
https://bugs.webkit.org/show_bug.cgi?id=97571

Attachment 166614: patch
https://bugs.webkit.org/attachment.cgi?id=166614&action=review

------- Additional Comments from Tim Horton <timothy_horton at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=166614&action=review


> Source/WebCore/ChangeLog:67
> +	   (WebCore::toSVGImageChromeClient):

This is the best place for per-function documentation, it’s a shame not to use
it for a relatively big patch like this!

> Source/WebCore/accessibility/AccessibilityRenderObject.h:252
>      // This returns true if it's focusable but it's not content editable and
it's not a control or ARIA control.

Did this comment at some point get detached from the line it refers to?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:839
> +    Document* doc = document();

Would prefer to spell out document, but there seem to be lots of cases of this
abbreviation.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2744
> +    RenderImage* renderImage = toRenderImage(m_renderer);

Can we fold this into the next line?

> Source/WebCore/accessibility/AccessibilitySVGRoot.h:29
> +#ifndef AccessibilitySVGRoot_h

This file, and the rest of the Accessibility files (yay, consistency, I
guess?), is in a weird order. The majority of WebCore classes go
public/protected/private and put members below methods. Maybe there’s no real
rule about this.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1229
> +	   return String();

Does it really not want a title at all?

> Source/WebCore/svg/graphics/SVGImageChromeClient.h:72
> +

If you had per-function comments in your change log, it would be more clear
what changed here :)

> LayoutTests/accessibility/svg-bounds.html:8
> -<svg>
> +<svg tabindex=0>

This change is not mentioned in your change log and I don’t understand it.

> LayoutTests/accessibility/resources/svg-face.svg:1
> +<svg xmlns:cc="http://web.resource.org/cc/"
xmlns:dc="http://purl.org/dc/elements/1.1/"
xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
xmlns="http://www.w3.org/2000/svg" xmlns:svg="http://www.w3.org/2000/svg"

So many unnecessary namespaces?

> LayoutTests/ChangeLog:8
> +	   Test skipped on chromium until clickPoint() is implemented in DRT.

Probably add one more line here about what your new test tests! Though that’s
pretty clear, I guess.