[webkit-reviews] review denied: [Bug 121977] Regression: AX: <table><caption> no longer exposed as AXTitle. : [Attachment 213217] Updated patch.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Samuel White
<samuel_white at apple.com>'s request for review:
Bug 121977: Regression: AX: <table><caption> no longer exposed as AXTitle.
https://bugs.webkit.org/show_bug.cgi?id=121977

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=213217&action=review


Looks generally good, but with a lot of “style changes” we probably don’t want.


> Source/WebCore/accessibility/AccessibilityImageMapLink.h:64
> +    virtual void accessibilityText(Vector<AccessibilityText>&) OVERRIDE;

Why does this need to be public? It is good to have overriding functions be
private, even if the function they are overriding is public. It helps us detect
people calling functions directly on derived classes but paying virtual
dispatch overhead. In general, most virtual function overrides should be
private.

> Source/WebCore/accessibility/AccessibilityMediaControls.h:50
> +    virtual void accessibilityText(Vector<AccessibilityText>&) OVERRIDE;

Same comment.

> Source/WebCore/accessibility/AccessibilityNodeObject.h:125
> +    virtual void accessibilityText(Vector<AccessibilityText>&) OVERRIDE;

Why does this function need to be public now? It was private before.

Strange that this function is non-const, but all the other nearly identical
ones are const. Not new to this patch, though.

Also, as an aside, WebKit coding style would name this function
getAccessibilityText. We use the prefix "get" to mean "returns a value through
an out argument". Since you didn’t add this function, that doesn’t need to be
fixed in this patch.

> Source/WebCore/accessibility/AccessibilityNodeObject.h:196
> +    virtual void titleElementText(Vector<AccessibilityText>&) const;
> +    virtual void alternativeText(Vector<AccessibilityText>&) const;
> +    virtual void visibleText(Vector<AccessibilityText>&) const;
> +    virtual void helpText(Vector<AccessibilityText>&) const;

Why do these functions need to be public now? They were private before.

Why do all of these functions need to be virtual now? Only titleElementText is
now being overridden, so I would suggest making only it be virtual.

> Source/WebCore/accessibility/AccessibilityTable.h:96
> +    virtual void titleElementText(Vector<AccessibilityText>&) const
OVERRIDE;

Should be private, not protected.