[webkit-reviews] review denied: [Bug 25528] [Gtk] Text attributes not exposed : [Attachment 58253] webkit_accessible_text_get_default_attributes and webkit_accessible_text_get_run_attributes implementation

[bugzilla-daemon at webkit.org]

Xan Lopez <xan.lopez at gmail.com> has denied Mario Sanchez Prada
<msanchez at igalia.com>'s request for review:
Bug 25528: [Gtk] Text attributes not exposed
https://bugs.webkit.org/show_bug.cgi?id=25528

Attachment 58253: webkit_accessible_text_get_default_attributes and
webkit_accessible_text_get_run_attributes implementation
https://bugs.webkit.org/attachment.cgi?id=58253&action=review

------- Additional Comments from Xan Lopez <xan.lopez at gmail.com>
>+    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.