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.
Created attachment 88250 [details] Patch proposal + unskipped layout test Attaching patch proposal
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).
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?
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').
(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
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?
(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!
Committed r83466: <http://trac.webkit.org/changeset/83466>