[Webkit-unassigned] [Bug 25528] [Gtk] Text attributes not exposed

[bugzilla-daemon at webkit.org]

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


Xan Lopez <xan.lopez at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #58253|review?                     |review-
               Flag|                            |




--- Comment #14 from Xan Lopez <xan.lopez at gmail.com>  2010-06-14 01:06:24 PST ---
(From update of attachment 58253)
>+    RenderObject* renderer = (static_cast<const AccessibilityRenderObject*>(object))->renderer();

You don't need those parenthesis.

>+    RenderStyle* style = renderer->style();
>+
>+    const int bufferSize = 16;
>+    gchar buffer[bufferSize];
>+    g_snprintf(buffer, bufferSize, "%i", style->fontSize());

Can we just use g_strdup_printf with GOwnPtr here and in the other places instead of buffers with hardcoded lenght?

>+    result = addAttributeToSet(result, atk_text_attribute_get_name(ATK_TEXT_ATTR_UNDERLINE), ((style->textDecoration() & UNDERLINE) ? "single" : "none"));

Don't really need that parenthesis either AFAIK.

>+static gint compareAttribute(const AtkAttribute* a, const AtkAttribute* b)
>+{
>+    return (g_strcmp0(a->name, b->name) || g_strcmp0(a->value, b->value));

Neither this.

>+}

>+static guint accessibilityObjectLength(const AccessibilityObject* object)
>+{
>+    int value = object->stringValue().length();
>+    if (value)
>+        return value;
>+    if (object->isAccessibilityRenderObject()) {
>+        const RenderObject* renderer = (static_cast<const AccessibilityRenderObject*>(object))->renderer();
>+        if (renderer->isBR())
>+            return 1;
>+    }
>+    // return object->textUnderElement().length();
>+    GOwnPtr<gchar> text(webkit_accessible_text_get_text(ATK_TEXT(object->wrapper()), 0, -1));
>+    return g_utf8_strlen (text.get(), -1);

You are kind of duplicating, I think, everything webkit_accessible_get_text does (at least it has code for stringValue().lenght and some special casing for BR); do you positively know you can't just return the lenght of what is returned by that method? If not, why?. Also, there's an extra space in the strlen call.

> 
>+static gint compAtkAttribute(AtkAttribute *a1, AtkAttribute *a2)
>+{
>+    gint strcmpVal;
>+    strcmpVal = g_strcmp0(a1->name, a2->name);
>+    if (strcmpVal)
>+        return strcmpVal;
>+    return g_strcmp0(a1->value, a2->value);
>+}
>+
>+static gint compAtkAttributeName(AtkAttribute *a1, AtkAttribute *a2)
>+{
>+    return g_strcmp0(a1->name, a2->name);
>+}
>+

The '*' are all wrong; I think the style script should just ignore this by now, but if it still gives false warnings just ignore it.

>+static gboolean atkAttributeSetAttributeHasValue(AtkAttributeSet *set, AtkTextAttribute attribute, const gchar *value)
>+{
>+    AtkAttribute at;
>+    at.name = g_strdup(atk_text_attribute_get_name(attribute));
>+
>+    GSList *element = g_slist_find_custom(set, &at, (GCompareFunc)compAtkAttributeName);
>+    g_free(at.name);

Do you really need to make that copy?

>+
>+    gboolean result = (element && !g_strcmp0(((AtkAttribute*)(element->data))->value, value));

No parenthesis.

>+    if (!result && element)
>+        printf("Expected %s got %s\n", value, ((AtkAttribute*)(element->data))->value);

What is this printf for? Shouldn't we assert or something, being this a test...

I think the patch is almost there, let's just fix those things.

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