Bug 57854 – [GTK] Implement AccessibilityUIElement::{row|column}IndexRange in DRT

• Component: Accessibility
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

We need this feature to be able to unskip the following test:

 accessibility/table-cell-spans.html

Prior to this we need to get bug 57463 and bug 57829 fixed, so setting a dependency there.

[Mario Sanchez Prada]

Created attachment 88250 [details]
Patch proposal + unskipped layout test

Attaching patch proposal

[Alexander Pavlov (apavlov)]

I don't think the bug 57829 can influence this issue, as the former is related purely to the Web Inspector internals (unless you need to debug live textual node changes in the Elements panel).

[Xan Lopez]

Comment on attachment 88250 [details]
Patch proposal + unskipped layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=88250&action=review

> Tools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:510
> +    GOwnPtr<gchar> rangeString(g_strdup_printf("{0, 0}"));

This can just be g_strdup.

> Tools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:551
> +    return JSStringCreateWithUTF8CString(rangeString.get());

These two methods are almost exactly identical (only difference seems to be to use column or row in the printf). I think you can refactor it to make both call a common method that does either one depending on a parameter, for instance?

[Mario Sanchez Prada]

Created attachment 88259 [details]
Patch proposal + unskipped layout test

(In reply to comment #3)
> (From update of attachment 88250 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88250&action=review
> 
> > Tools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:510
> > +    GOwnPtr<gchar> rangeString(g_strdup_printf("{0, 0}"));
> 
> This can just be g_strdup.

Sure thing.

> > Tools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:551
> > +    return JSStringCreateWithUTF8CString(rangeString.get());
> 
> These two methods are almost exactly identical (only difference seems to be to use column or row in the printf). I think you can refactor it to make both call a common method that does either one depending on a parameter, for instance?

Done. I've to say I don't usually like writing helper functions with a boolean parameter to distinguish between two possible situations (row or column range?), but I guess in this case, being a _private_ helper function only used from there, it's more than enough (defining an enumeration for this would look to me 'overkill').

[Mario Sanchez Prada]

(In reply to comment #2)
> I don't think the bug 57829 can influence this issue, as the former is related purely to the Web Inspector internals (unless you need to debug live textual node changes in the Elements panel).

Sorry for the noise, Alexander. I meant bug 57826 not 57829.

My mistake. Fixing it now

[Xan Lopez]

Comment on attachment 88259 [details]
Patch proposal + unskipped layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=88259&action=review

Looks good minus one comment.

> Tools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:507
> +        return false;

I suppose you want to return a string here?

[Mario Sanchez Prada]

(In reply to comment #6)
> (From update of attachment 88259 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88259&action=review
> 
> Looks good minus one comment.
> 
> > Tools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:507
> > +        return false;
> 
> I suppose you want to return a string here?

?? Looks like I uploaded a previous version of the patch, before changing that line (I have the right thing locally, I swear!)

Anyway, you're right, that should be an string.

Thanks for the review!

[Mario Sanchez Prada]

Committed r83466: <http://trac.webkit.org/changeset/83466>