[webkit-reviews] review denied: [Bug 25528] [Gtk] Text attributes not exposed : [Attachment 46683] Patch that implements webkit_accessible_text_get_default_attributes and webkit_accessible_text_get_run_attributes
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jan 29 04:05:56 PST 2010
Xan Lopez <xan.lopez at gmail.com> has denied José Millán Soto
<jmillan at igalia.com>'s request for review:
Bug 25528: [Gtk] Text attributes not exposed
https://bugs.webkit.org/show_bug.cgi?id=25528
Attachment 46683: Patch that implements
webkit_accessible_text_get_default_attributes and
webkit_accessible_text_get_run_attributes
https://bugs.webkit.org/attachment.cgi?id=46683&action=review
------- Additional Comments from Xan Lopez <xan.lopez at gmail.com>
>-static AtkAttributeSet* webkit_accessible_text_get_run_attributes(AtkText*
text, gint offset, gint* start_offset, gint* end_offset)
>+static void addAttribute(AtkAttributeSet* &attributeSet, AtkTextAttribute
textAttribute, gchar* value, bool copyValue = true)
So are pointers to references more idiomatic than pointers to pointers? I don't
think I've seen them used a lot (well, at all) in WebKit.
> {
>- notImplemented();
>- return 0;
>+ AtkAttribute* newAttribute = (AtkAttribute
*)g_malloc(sizeof(AtkAttribute));
In theory the preferred method to allocate structures in glib is with
g_slice_new. Also, C-style casts are against the style guidelines.
>+ newAttribute->name =
g_strdup(atk_text_attribute_get_name(textAttribute));
>+ if (copyValue) value = g_strdup(value);
The assignment has to go in its own line.
>+ newAttribute->value = value;
Or you could just do... = copyValue? g_strdup(value) : value;
>+ attributeSet = g_slist_prepend(attributeSet, newAttribute);
>+}
>+
>+/*
>+ Returns an AtkAttributeSet with the AtkAttributes which define style.
>+ If parentObject is given, only the ones which are different from style
and
>+ parentStyle would be in the result.
>+*/
I think C++-style comments are preferred. Also, the comment seems to refer to a
parameter that no longer exists?
>+static AtkAttributeSet* getAttributeSetForAccessibilityObject(const
AccessibilityObject* object, bool onlyDifferentFromParent = false)
>+{
>+ RenderObject* renderer;
>+ RenderObject* parentRenderer = 0;
>+ AtkAttributeSet* result = 0;
>+ RenderStyle* style;
>+ RenderStyle* parentStyle = 0;
>+ int intProp;
>+ bool styleProp;
>+
>+ if (object->isAccessibilityRenderObject()) {
>+ AccessibilityObject* parent;
>+ renderer = ((AccessibilityRenderObject*)object)->renderer();
Use C++ casts.
>+ style = renderer->style();
>+ if (onlyDifferentFromParent) {
>+ parent = object->parentObject();
>+ while (parent && !parent->isAccessibilityRenderObject())
>+ parent = parent->parentObject();
I believe you should check here if the parent objects you iterate through are
not ignored (see parentObjectUnignored).
>+ if (parent && parent->isAccessibilityRenderObject())
>+ parentRenderer =
((AccessibilityRenderObject*)parent)->renderer();
Casting again.
>+ }
>+ } else
>+ return 0;
Just return early at the beginning of the function.
>+
>+ if (parentRenderer)
>+ parentStyle = parentRenderer->style();
>+
>+ intProp = style->fontSize();
>+ if (!parentStyle || (intProp != parentStyle->fontSize()))
>+ addAttribute(result, ATK_TEXT_ATTR_SIZE, g_strdup_printf("%i",
intProp), false);
>+
>+ if (style->hasBackground()
>+ && !(parentStyle
>+ && parentStyle->hasBackground()
>+ && (style->backgroundColor() == parentStyle->backgroundColor()))
>+ && style->color().isValid())
>+ addAttribute(result, ATK_TEXT_ATTR_BG_COLOR,
g_strdup_printf("%i,%i,%i", style->backgroundColor().red(),
style->backgroundColor().green(), style->backgroundColor().blue()), false);
>+ if (style->color().isValid()
>+ && !(parentStyle && (style->color() == parentStyle->color())))
>+ addAttribute(result, ATK_TEXT_ATTR_FG_COLOR,
g_strdup_printf("%i,%i,%i", style->color().red(), style->color().green(),
style->color().blue()), false);
>+
>+ intProp = renderer->baselinePosition(true);
>+ if (!parentStyle
>+ || (parentStyle->verticalAlign() != style->verticalAlign())
>+ || (style->verticalAlign() == BASELINE ? false : (intProp !=
parentRenderer->baselinePosition(true)))) {
>+ bool includeRise;
>+ switch (style->verticalAlign()) {
>+ case SUB:
>+ intProp = -intProp;
>+ case SUPER:
>+ includeRise = true;
>+ break;
>+ case BASELINE:
>+ includeRise = true;
>+ intProp = 0;
>+ break;
>+ default:
>+ includeRise = false;
>+ }
>+ if (includeRise)
>+ addAttribute(result, ATK_TEXT_ATTR_RISE, g_strdup_printf("%i",
intProp), false);
>+ }
>+
>+ if (!parentStyle
>+ || (parentStyle->textIndent() != style->textIndent())) {
>+ gint value = style->textIndent().calcValue(object->size().width());
>+ if (value != undefinedLength)
>+ addAttribute(result, ATK_TEXT_ATTR_INDENT, g_strdup_printf("%i",
value), false);
>+ }
>+
>+ if (!parentStyle || (parentStyle->font().family().family() !=
style->font().family().family()))
>+ addAttribute(result, ATK_TEXT_ATTR_FAMILY_NAME,
g_strdup(style->font().family().family().string().utf8().data()), false);
>+
>+ intProp = style->font().weight();
>+ if (!parentStyle || (intProp != parentStyle->font().weight())) {
>+ int fontWeight = -1;
>+ switch (intProp) {
>+ case FontWeight100:
>+ fontWeight = 100;
>+ break;
>+ case FontWeight200:
>+ fontWeight = 200;
>+ break;
>+ case FontWeight300:
>+ fontWeight = 300;
>+ break;
>+ case FontWeight400:
>+ fontWeight = 400;
>+ break;
>+ case FontWeight500:
>+ fontWeight = 500;
>+ break;
>+ case FontWeight600:
>+ fontWeight = 600;
>+ break;
>+ case FontWeight700:
>+ fontWeight = 700;
>+ break;
>+ case FontWeight800:
>+ fontWeight = 800;
>+ break;
>+ case FontWeight900:
>+ fontWeight = 900;
>+ }
>+ if (fontWeight > 0)
>+ addAttribute(result, ATK_TEXT_ATTR_WEIGHT, g_strdup_printf("%i",
fontWeight), false);
>+ }
>+
>+ intProp = style->textAlign();
>+ if (!parentStyle || (intProp != parentStyle->textAlign())) {
>+ switch (intProp) {
>+ case TAAUTO:
>+ break;
>+ case LEFT:
>+ case WEBKIT_LEFT:
>+ addAttribute(result, ATK_TEXT_ATTR_JUSTIFICATION, (gchar
*)"left");
>+ break;
>+ case RIGHT:
>+ case WEBKIT_RIGHT:
>+ addAttribute(result, ATK_TEXT_ATTR_JUSTIFICATION, (gchar
*)"right");
>+ break;
>+ case CENTER:
>+ case WEBKIT_CENTER:
>+ addAttribute(result, ATK_TEXT_ATTR_JUSTIFICATION, (gchar
*)"center");
>+ break;
>+ case JUSTIFY:
>+ addAttribute(result, ATK_TEXT_ATTR_JUSTIFICATION, (gchar
*)"fill");
>+ }
>+ }
>+
>+ styleProp = style->textDecoration() & UNDERLINE;
>+ if (!parentStyle || (styleProp != (parentStyle->textDecoration() &
UNDERLINE)))
>+ addAttribute(result, ATK_TEXT_ATTR_UNDERLINE, (styleProp ?
(gchar*)"single" : (gchar*)"none"));
>+ styleProp = style->font().italic();
>+ if (!parentStyle || (styleProp != (parentStyle->font().italic())))
>+ addAttribute(result, ATK_TEXT_ATTR_STYLE, (styleProp ?
(gchar*)"italic" : (gchar*)"normal"));
>+ styleProp = style->textDecoration() & LINE_THROUGH;
>+ if (!parentStyle || (styleProp != (parentStyle->textDecoration() &
LINE_THROUGH)))
>+ addAttribute(result, ATK_TEXT_ATTR_STRIKETHROUGH, (styleProp ?
(gchar*)"true" : (gchar*)"false"));
>+ styleProp = style->visibility() == HIDDEN;
>+ if (!parentStyle || (styleProp != (parentStyle->visibility() == HIDDEN)))
>+ addAttribute(result, ATK_TEXT_ATTR_INVISIBLE, (styleProp ?
(gchar*)"true" : (gchar*)"false"));
>+
>+ return result;
>+}
I honestly feel that having the logic of getting the difference from the parent
object in the same function makes the code less clear than it could be. I think
it would work better if you had a (simple) function to get the attributes of
one object, another to get the difference between two attribute sets, and
possibly a third helper function to get the difference between one object's and
its parent set. It will probably be more code, but I think in the end things
will be more understandable.
>+
>+static gint compareAttributeName(const AtkAttribute* a, const AtkAttribute*
b)
>+{
>+ return g_strcmp0(a->name, b->name);
>+}
>+
>+/*
>+ Returns the union of two AtkAttributeSet, having the second one priority.
>+ Releases a1, and the AtkAttributes no longer used
The memory management of the function seems a bit weird and tailored for you
one use case. Making a new set with the union seems more reasonable if you
think this could be used elsewhere in the future.
Another thing, possibly it would be more efficient to sort the attributes by
name and then just iterate both lists at the same time.
>+*/
>+static AtkAttributeSet* attributeSetUnion(AtkAttributeSet* a1,
AtkAttributeSet* a2)
>+{
>+ if (!a1)
>+ return a2;
>+ if (!a2)
>+ return a1;
>+
>+ AtkAttributeSet* i = a1;
>+ AtkAttributeSet* found;
>+
>+ do {
>+ found = g_slist_find_custom(a2, i->data,
(GCompareFunc)compareAttributeName);
>+ if (!found) {
>+ AtkAttributeSet* t = i->next;
>+ a2 = g_slist_prepend(a2, i->data);
>+ a1 = g_slist_delete_link(a1, i);
>+ i = t;
>+ } else
>+ i = i->next;
>+ } while (i);
>+
>+ atk_attribute_set_free(a1);
>+ return a2;
>+}
>+
>+static guint accessibilityObjectLength(const AccessibilityObject* object)
>+{
>+ int value = object->stringValue().length();
>+ if (value)
>+ return value;
>+ if (object->isAccessibilityRenderObject()) {
>+ const RenderObject* renderer =
((AccessibilityRenderObject*)object)->renderer();
>+ if (renderer->isBR())
>+ return 1;
>+ }
>+ return object->textUnderElement().length();
>+}
use webkit_accessible_get_text and then calculate the length?
>+
>+/*
>+ Returns a list of AccessibilityObjects, so that each one is the
>+ child of the preceding one which is located in the desired offset.
>+ *startOffset and *endOffset would be the beggining and next offset of the
>+ most lower AccessibilityObject; if offset exceeds the length of the
object
>+ *startOffset and *endOffset will be set to -1.
>+*/
>+static GSList *getAccessibilityObjectsForOffset(const AccessibilityObject
*object, guint offset, gint *start_offset, gint *end_offset)
>+{
>+ GSList* result = 0;
>+ guint length = accessibilityObjectLength(object);
>+ if (length > offset) {
>+ *start_offset = 0;
>+ *end_offset = length;
>+ result = g_slist_append(result, (gpointer)object);
>+ } else {
>+ *start_offset = -1;
>+ *end_offset = -1;
>+ }
>+
>+ if (object->hasChildren()) {
>+ AccessibilityObject* child = object->firstChild();
>+ guint childPosition = 0;
>+ guint childLength;
>+ while (child) {
>+ childLength = accessibilityObjectLength(child);
>+ if ((childLength + childPosition) > offset) {
>+ gint childStartOffset;
>+ gint childEndOffset;
>+ result =
g_slist_concat(getAccessibilityObjectsForOffset(child, offset-childPosition,
&childStartOffset, &childEndOffset), result);
>+ if (childStartOffset >= 0) {
>+ *start_offset = childStartOffset + childPosition;
>+ *end_offset = childEndOffset + childPosition;
>+ }
>+ break;
>+ }
>+ childPosition += childLength;
>+ child = child->nextSibling();
>+ }
>+ }
>+ return result;
>+}
>+
>+static AtkAttributeSet* getRunAttributesFromAccesibilityObject(const
AccessibilityObject* element, gint offset, gint* startOffset, gint* endOffset,
RenderObject* defaultRenderer)
>+{
>+ AtkAttributeSet* result = 0;
>+
>+ GSList* childs = getAccessibilityObjectsForOffset(element, offset,
startOffset, endOffset);
children.
>+ GSList* child = childs;
>+ bool previousReadOnly = element->isReadOnly();
>+
>+ while (child) {
>+ const AccessibilityObject* childObject = (const AccessibilityObject
*)child->data;
>+ bool readOnly = childObject->isReadOnly();
>+ AtkAttributeSet* differentAttributes =
getAttributeSetForAccessibilityObject(childObject, true);
>+ if (readOnly != previousReadOnly) {
>+ previousReadOnly = readOnly;
>+ addAttribute(differentAttributes, ATK_TEXT_ATTR_EDITABLE,
readOnly ? (gchar *)"false" : (gchar *)"true");
>+ }
>+ result = attributeSetUnion(result, differentAttributes);
>+ child = child->next;
>+ }
>+ g_slist_free(childs);
>+
>+ return result;
>+}
>+
>+static AtkAttributeSet* webkit_accessible_text_get_run_attributes(AtkText*
text, gint offset, gint* startOffset, gint* endOffset)
>+{
>+ AccessibilityObject* coreObject = core(text);
>+ AtkAttributeSet* result;
>+ int rStartOffset, rEndOffset;
>+
>+ if ((!coreObject) || (!coreObject->hasChildren()))
>+ return 0;
>+
>+ RenderObject* baseRenderer = 0;
>+ if (coreObject->isAccessibilityRenderObject())
>+ baseRenderer = ((AccessibilityRenderObject*)coreObject)->renderer();
This seems to be completely unused in getRunAttributesFromAccessibilityObject.
>+
>+ if (offset == -1)
>+ offset = atk_text_get_caret_offset(text);
>+
>+ result = getRunAttributesFromAccesibilityObject(coreObject, offset,
&rStartOffset, &rEndOffset, baseRenderer);
>+
>+ if (rStartOffset >= 0) {
>+ *startOffset = rStartOffset;
>+ *endOffset = rEndOffset;
>+ } else {
>+ *startOffset = offset;
>+ *endOffset = offset;
>+ }
>+
>+ return result;
> }
>
> static AtkAttributeSet*
webkit_accessible_text_get_default_attributes(AtkText* text)
> {
>- notImplemented();
>- return 0;
>+ AccessibilityObject* coreObject = core(text);
>+ if ((!coreObject) || (!coreObject->isAccessibilityRenderObject()))
>+ return 0;
Extra parens not needed.
>+
>+ AtkAttributeSet* result = 0;
>+
>+ result = getAttributeSetForAccessibilityObject(coreObject);
>+ addAttribute(result, ATK_TEXT_ATTR_EDITABLE, coreObject->isReadOnly() ?
(gchar*)"false" : (gchar*)"true");
Why is that not handled by the function? Are the casts to (gchar*) really
needed? Couldn't the function take 'const char*'?
>+ return result;
> }
>
> static void webkit_accessible_text_get_character_extents(AtkText* text, gint
offset, gint* x, gint* y, gint* width, gint* height, AtkCoordType coords)
As a last comment, can we do first a patch just implementing
webkit_accessible_text_get_default_attributes, seems it would get rid of the
most complex functions needed for get_run_attributes, which we can add in a
later patch.
Also, as always, it would be good to have some test for this.
More information about the webkit-reviews
mailing list