[Webkit-unassigned] [Bug 47365] getTextAtOffset returns incorrect results if a link includes text and an image

Thu Oct 21 02:43:32 PDT 2010

https://bugs.webkit.org/show_bug.cgi?id=47365

Mario Sanchez Prada <msanchez at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #71330|0                           |1
        is obsolete|                            |
  Attachment #71412|                            |review?
               Flag|                            |

--- Comment #4 from Mario Sanchez Prada <msanchez at igalia.com>  2010-10-21 02:43:32 PST ---
Created an attachment (id=71412)
 --> (https://bugs.webkit.org/attachment.cgi?id=71412&action=review)
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!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.