Bug 47365

Created attachment 70231 [details] Patch proposal (WIP) Ataching a WorkInProgress patch that would fix this issue, just for sharing, as currently my devel environment is kind of broken so unit tests won't run... As soon as I get it everything properly working back again I'll write an unit test for this stuff and upload a final patch to be reviewed.

Created attachment 71330 [details] Patch proposal + unit test (In reply to comment #1) > [...] > As soon as I get it everything properly working back again I'll write an unit test for this stuff and upload a final patch to be reviewed. Back to normality, so attaching a patch proposal with the unit tests I couldn't implement before. Hance, asking for review.

Comment on attachment 71330 [details] Patch proposal + unit test View in context: https://bugs.webkit.org/attachment.cgi?id=71330&action=review Looks good, but I have a few concerns. > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:866 > + GString* str = g_string_new(0); Please don't use an abbreviation here. > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:873 > + for (RenderObject* obj = renderer->firstChild(); obj; obj = obj->nextSibling()) { > + if (obj->isBR()) { Again, we've been trying to avoid abbreviations in new code. > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:936 > + g_string_append(str, textForRenderer(accObject->renderer())); Isn't this a memory leak? textForRenderer returns a newly allocated string. > WebKit/gtk/tests/testatk.c:975 > + webkit_web_view_load_string(webView, linksWithInlineImages, NULL, NULL, NULL); We should follow WebKit style in the tests as much as possible, thus NULL => 0. > WebKit/gtk/tests/testatk.c:980 > + g_idle_add((GSourceFunc)bail_out, loop); > + g_main_loop_run(loop); > + Didn't we discover that this approach was unnecessary in a previous patch or does something about this require it?

Created attachment 71412 [details] Patch proposal + unit test (In reply to comment #3) > (From update of attachment 71330 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71330&action=review > > Looks good, but I have a few concerns. Thanks. Attaching a new patch trying to address all those issues. > > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:866 > > + GString* str = g_string_new(0); > > Please don't use an abbreviation here. Sorry. Fixed using 'resultText' > > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:873 > > + for (RenderObject* obj = renderer->firstChild(); obj; obj = obj->nextSibling()) { > > + if (obj->isBR()) { > > Again, we've been trying to avoid abbreviations in new code. Fixed. I used 'object' instead :P > > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:936 > > + g_string_append(str, textForRenderer(accObject->renderer())); > > Isn't this a memory leak? textForRenderer returns a newly allocated string. Good catch. It actually IS a memory leak. Fixed by using GOwnPtr in there. > > WebKit/gtk/tests/testatk.c:975 > > + webkit_web_view_load_string(webView, linksWithInlineImages, NULL, NULL, NULL); > > We should follow WebKit style in the tests as much as possible, thus NULL => 0. Fixed. > > WebKit/gtk/tests/testatk.c:980 > > + g_idle_add((GSourceFunc)bail_out, loop); > > + g_main_loop_run(loop); > > + > > Didn't we discover that this approach was unnecessary in a previous patch or does something about this require it? Actually, what happened back then (in testWebkitAtkTextChangedNotifications()) was that I needed to connect signals from accessible object before entering the main loop, but I couldn't do it that way since the accessible object wouldn't be created if the main loop was not started, so it was kind of an egg-chicken problem. After some digging and numerous experiments manually stopping and restarting the main loop, we (a lot of help from your side :-)) finally realized we could just remove this approach for that test and just manually spin the original main loop to make sure the accessible objects were created before connecting to their signals, with no need to bother about playing with stopping/restarting the main loop, resulting in the following code: [...] webkit_web_view_load_string(webView, formWithTextInputs, 0, 0, 0); // Manually spin the main context to get the accessible objects while (g_main_context_pending(0)) g_main_context_iteration(0, TRUE); AtkObject* obj = gtk_widget_get_accessible(GTK_WIDGET(webView)); g_assert(obj); AtkObject* form = atk_object_ref_accessible_child(obj, 0); g_assert(ATK_IS_OBJECT(form)); AtkObject* textEntry = atk_object_ref_accessible_child(form, 0); g_assert(ATK_IS_EDITABLE_TEXT(textEntry)); g_assert(atk_object_get_role(ATK_OBJECT(textEntry)) == ATK_ROLE_ENTRY); g_signal_connect(textEntry, "text-changed::insert", G_CALLBACK(textChangedCb), (gpointer)"insert"); g_signal_connect(textEntry, "text-changed::delete", G_CALLBACK(textChangedCb), (gpointer)"delete"); [...] However, this case is different as we don't know to connect to any signals, so the original chicken-egg problem (which led me to experiment with stopping-restarting the mainloop) is not present in this case. However, we'd still need to manually spin the original main loop to get valid accessible objects, in case we want to get rid of that g_main_loop_run() / bail_out() stuff, which would result in the following patch on top of the current proposal: --- a/WebKit/gtk/tests/testatk.c +++ b/WebKit/gtk/tests/testatk.c @@ -972,11 +972,11 @@ static void testWebkitAtkLinksWithInlineImages(void) g_object_ref_sink(webView); GtkAllocation alloc = { 0, 0, 800, 600 }; gtk_widget_size_allocate(GTK_WIDGET(webView), &alloc); webkit_web_view_load_string(webView, linksWithInlineImages, 0, 0, 0); - GMainLoop* loop = g_main_loop_new(NULL, TRUE); - g_idle_add((GSourceFunc)bail_out, loop); - g_main_loop_run(loop); + + // Manually spin the main context to get the accessible objects + while (g_main_context_pending(0)) + g_main_context_iteration(0, TRUE); AtkObject* obj = gtk_widget_get_accessible(GTK_WIDGET(webView)); g_assert(obj); ... and I'm not sure whether that's more clear than just using the other approach, which is used elsewhere in this file, instead of putting that weird while loop in this test as well (currently that other approach is only being used in testWebkitAtkTextChangedNotifications()). Hence, I'm not making that change yet in this new patch, just in case you agree with me it's ok to leave it as it is, because of the rationale above :-) Thanks!

Comment on attachment 71412 [details] Patch proposal + unit test View in context: https://bugs.webkit.org/attachment.cgi?id=71412&action=review Looks good, but I'm waiting for the reuslts of your experiments to give the final r+. We should switch to iterating the main main loop, if possible. > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:937 > + GOwnPtr<gchar> rendererText(textForRenderer(accObject->renderer())); > + g_string_append(str, rendererText.get()); Thanks for fixing this. In the future it might make sense for these methods to just return CStrings. Perhaps a cleanup in some other patch. > WebKit/gtk/tests/testatk.c:979 > + g_idle_add((GSourceFunc)bail_out, loop); > + g_main_loop_run(loop); I think we should also try to iterate the main main loop here as well. I think you're exploring that presently.

Created attachment 71852 [details] Patch proposal + unit test Attaching a new patch: (In reply to comment #5) > [...] > > WebKit/gtk/tests/testatk.c:979 > > + g_idle_add((GSourceFunc)bail_out, loop); > > + g_main_loop_run(loop); > > I think we should also try to iterate the main main loop here as well. I think you're exploring that presently. Fixed (by manually spinning the loop, which is still needed to get the accessible objects) Asking for review again

Comment on attachment 71852 [details] Patch proposal + unit test View in context: https://bugs.webkit.org/attachment.cgi?id=71852&action=review Great! Thanks. Perhaps make this small change before landing: > WebKit/gtk/tests/testatk.c:985 > + AtkObject* obj = gtk_widget_get_accessible(GTK_WIDGET(webView)); > + g_assert(obj); > + > + // First paragraph (link at the beginning) > + AtkObject* paragraph = atk_object_ref_accessible_child(obj, 0); Please rename obj to something like "object" to avoid the abbreviation.

Landed, http://trac.webkit.org/changeset/70634

http://trac.webkit.org/changeset/70634 might have broken GTK Linux 32-bit Release