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

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Mario Sanchez Prada
<msanchez at igalia.com>'s request 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 71330: Patch proposal + unit test
https://bugs.webkit.org/attachment.cgi?id=71330&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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?