[webkit-reviews] review requested: [Bug 118179] [ATK] Refactor code for translating offsets between WebCore a11y and ATK : [Attachment 205897] Patch proposal

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has asked  for review:
Bug 118179: [ATK] Refactor code for translating offsets between WebCore a11y
and ATK
https://bugs.webkit.org/show_bug.cgi?id=118179

Attachment 205897: Patch proposal
https://bugs.webkit.org/attachment.cgi?id=205897&action=review

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
Thanks for the review, Chris. Now attaching a new patch addressing the issues
you pointed out, see my comments below...

(In reply to comment #2)
> (From update of attachment 205700 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=205700&action=review
> 
> > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:473
> > +static int listItemMarkerLengthForObject(const AccessibilityObject*
object)
> 
> this only applies to LTR, maybe the method should be named to indicate that?

I already thought of that, but in the end I found it better, or at least more
clear, not to do it since it's an internal matter from the point of view of
that function. From the caller you just care about "normalizing" the offset for
list items since it's an special element from the point of view of ATK (as we
flatten it out together with the marker).

> > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:475
> > +	 if (!object->isAccessibilityRenderObject())
> 
> you shouldn't have to check isAXRenderObject() -- renderer() is on every
AXObject, so you just need to check if the renderer is 0 or not (which you're
already doing)

Ok.

> > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:482
> > +	     String markerText =
toRenderListItem(renderer)->markerTextWithSuffix();
> 
> this can be made into one line of code

Done.

> > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:491
> > +	 if (!object->isAccessibilityRenderObject() || !object->isListItem())
> 
> AXrenderObject check is not necessary

Ok.

> > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:500
> > +	 if (!coreObject || !coreObject->isAccessibilityRenderObject() ||
!coreObject->isListItem())
> 
> AXRenderObject check not necessary

Ok.