[webkit-reviews] review granted: [Bug 109015] <hr> should expose AXRole/AXSubrole, etc : [Attachment 227360] patch

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 109015: <hr> should expose AXRole/AXSubrole, etc
https://bugs.webkit.org/show_bug.cgi?id=109015

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

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=227360&action=review


Besides a couple of nits mentioned below, the patch looks good to me. It is
very weird, though, that you got those failures in the mac EWS bots, since the
it seemed to me (after looking through the actual results zipped in previous
comments) that the EWS were not picking, for whatever reason, the new expected
results you are providing with your patch (which are identical to the diff
provided by the EWS as "proof of failure")

Thus, would you mind addressing those nits and also making sure there's nothing
wrong with the new expectations for mac before landing? Thanks!

> Source/WebCore/accessibility/atk/AccessibilityObjectAtk.cpp:-46
> -    if (role == HorizontalRuleRole)
> -	   return IncludeObject;

Nit. Could you move the declaration above down right before the first use of
role?

> LayoutTests/ChangeLog:12
> +	   * platform/gtk/accessibility/roles-exposed-expected.txt: Added.

You don't need to add a expectation for GTK. I believe that the one in
accessibility/roles-exposed-expected.txt should still work for GTK and EFL

> LayoutTests/accessibility/lists.html:28
> +    <BR><BR><BR><BR>

I guess this change will probably mean a rebaseline for GTK/EFL later on. I'll
keep an eye on that