[webkit-reviews] review canceled: [Bug 73819] [Gtk] Dojo toggle buttons should expose ROLE_TOGGLE_BUTTON not ROLE_PUSH_BUTTON : [Attachment 159484] Fixes the bug

[bugzilla-daemon at webkit.org]

Alejandro Piñeiro <apinheiro at igalia.com> has canceled Alejandro Piñeiro
<apinheiro at igalia.com>'s request for review:
Bug 73819: [Gtk] Dojo toggle buttons should expose ROLE_TOGGLE_BUTTON not
ROLE_PUSH_BUTTON
https://bugs.webkit.org/show_bug.cgi?id=73819

Attachment 159484: Fixes the bug
https://bugs.webkit.org/attachment.cgi?id=159484&action=review

------- Additional Comments from Alejandro Piñeiro <apinheiro at igalia.com>
(In reply to comment #29)
> (From update of attachment 159484 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=159484&action=review
> 
> looking good, besides minor nits. i think you might want to wait for someone
on the google CC list to review the chromium change, although I'm sure it will
be fine

They were automatically added on comment 24, and I added Dominic Mazzoni to the
CC list

Dominic could you review the patch to check if it is ok with you?

> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:1743
> > +// This method assumes that the object is already a button-related
> 
> I think this comment is probably not needed. it just explains what the code
does, which is pretty easy to read.

Done.

> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:1750
> > +	 // If aria-pressed is present, then it should be exposed as a toggle
button.
> 
> you could ASSERT(isButton()) and return UnknownRole if not a button

I think that I can't. We have a chicken-egg problem here. isButton() just
checks if roleValue() == ButtonRole. roleValue just returns m_role. m_role is
computed at determineAccessibilityRole. And this new function (buttonRoleType)
is used as an auxiliar function at determineAccessibilityRole. So, when
buttonRoleType is called you are still computing m_role, so isButton() return
value could be wrong.

This is the reason I added that comment "This method assumes that the object is
already a button-related role object". You should already know that it is a
button role, for example that the object has the tag name buttonTag.

Taking into account that this is an auxiliary method, and that you need to take
that into account, I don't see this method really suitable for a public method.
I have moved it to the protected section.