[webkit-reviews] review requested: [Bug 47365] getTextAtOffset returns incorrect results if a link includes text and an image : [Attachment 71412] Patch proposal + unit test

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has asked  for review:
Bug 47365: getTextAtOffset returns incorrect results if a link includes text
and an image
https://bugs.webkit.org/show_bug.cgi?id=47365

Attachment 71412: Patch proposal + unit test
https://bugs.webkit.org/attachment.cgi?id=71412&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
(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!