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

[bugzilla-daemon at webkit.org]

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


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

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #58253|0                           |1
        is obsolete|                            |
  Attachment #58670|                            |review?
               Flag|                            |




--- Comment #15 from Mario Sanchez Prada <msanchez at igalia.com>  2010-06-14 10:55:07 PST ---
Created an attachment (id=58670)
 --> (https://bugs.webkit.org/attachment.cgi?id=58670)
webkit_accessible_text_get_default_attributes and webkit_accessible_text_get_run_attributes implementation

(In reply to comment #14)
> (From update of attachment 58253 [details])
> >+    RenderObject* renderer = (static_cast<const AccessibilityRenderObject*>(object))->renderer();
> 
> You don't need those parenthesis.

Fixed

> >+    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?

Sure! :-)
Done.

> >+    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.

Fixed

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

Fixed

> >+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.

Fixed

> > 
> >+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.

Fixed

> >+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?

I guess not. Reverted to the original code from Millan, which didn't duplicate anything.

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

Fixed

> >+    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...

Reading the whole patch I think this is just some legacy code for debugging purposes, as the return value for this function is immediately used in an assertion so it wouldn't make sense either to assert the same thing here also. Hence, code removed.

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

Attached a new patch addressing all these issues, and tested it both in Accerciser and with the unittests/testatk binary.

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