RESOLVED FIXED 57854
[GTK] Implement AccessibilityUIElement::{row|column}IndexRange in DRT
https://bugs.webkit.org/show_bug.cgi?id=57854
Summary [GTK] Implement AccessibilityUIElement::{row|column}IndexRange in DRT
Mario Sanchez Prada
Reported 2011-04-05 09:17:17 PDT
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.
Attachments
Patch proposal + unskipped layout test (7.35 KB, patch)
2011-04-05 09:24 PDT, Mario Sanchez Prada
no flags
Patch proposal + unskipped layout test (7.03 KB, patch)
2011-04-05 09:51 PDT, Mario Sanchez Prada
xan.lopez: review+
Mario Sanchez Prada
Comment 1 2011-04-05 09:24:08 PDT
Created attachment 88250 [details] Patch proposal + unskipped layout test Attaching patch proposal
Alexander Pavlov (apavlov)
Comment 2 2011-04-05 09:25:55 PDT
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 3 2011-04-05 09:32:32 PDT
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
Comment 4 2011-04-05 09:51:07 PDT
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
Comment 5 2011-04-05 09:54:01 PDT
(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 6 2011-04-05 09:54:35 PDT
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
Comment 7 2011-04-05 10:01:23 PDT
(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
Comment 8 2011-04-11 11:57:51 PDT
Note You need to log in before you can comment on or make changes to this bug.