[webkit-reviews] review denied: [Bug 25415] [GTK][ATK] Please implement support for get_text_at_offset : [Attachment 30465] gettextv3.patch

[bugzilla-daemon at webkit.org]

Jan Alonzo <jmalonzo at gmail.com> has denied Xan Lopez <xan.lopez at gmail.com>'s
request for review:
Bug 25415: [GTK][ATK] Please implement support for get_text_at_offset
https://bugs.webkit.org/show_bug.cgi?id=25415

Attachment 30465: gettextv3.patch
https://bugs.webkit.org/attachment.cgi?id=30465&action=review

------- Additional Comments from Jan Alonzo <jmalonzo at gmail.com>
> -static gchar* webkit_accessible_text_get_text_after_offset(AtkText* text,
gint offset, AtkTextBoundary boundary_type, gint* start_offset, gint*
end_offset)
> +typedef enum {
> +    AfterOffset,
> +    AtOffset,
> +    BeforeOffset
> +} GetTextFunctionType;

enum GetTextFunctionType would do.

> +typedef int (*advanceFunc) (int);
> +
> +static int increaseInt(int i) {
> +    return i + 1;
> +}
> +
> +static int decreaseInt(int i) {
> +    return i - 1;
> +}
> +
> +typedef enum {
> +    DirectionForward,
> +    DirectionBackwards
> +} Direction;

See enum comment above.

> +
> +static bool findCharacterAttribute(isCharacterAttribute predicateFunction,
PangoLogAttr* attributes, Direction direction, int startOffset, int
attrsLength, int* resultOffset)
> +{

attrsLength should be unsigned. can starOffset be negative here?

> +    int i;

You can move this in for loop below.

> +    advanceFunc advanceFunc = direction == DirectionForward ? increaseInt :
decreaseInt;
> +
> +    *resultOffset = -1;
> +
> +    for (i = startOffset; i >= 0 && i < attrsLength; i = advanceFunc(i)) {

int i = startOffset;

> +	   if (predicateFunction(attributes+i)) {

there should be spaces in attributes+1

> +static bool findCharacterAttributeSkip(isCharacterAttribute
predicateFunction, unsigned skip, PangoLogAttr* attributes, Direction
direction, int startOffset, int attrsLength, int* resultOffset)
> +{
> +    int tmpOffset;
> +    bool retValue;
> +
> +    retValue = findCharacterAttribute(predicateFunction, attributes,
direction, startOffset, attrsLength, &tmpOffset);

Better to assign value in the declaration.

> +static gchar* getTextHelper(GetTextFunctionType getTextFunctionType,
AtkText* textObject, gint offset, AtkTextBoundary boundaryType, gint*
startOffset, gint* endOffset)
> +{
> +    AccessibilityObject* coreObject = core(textObject);
> +    String text;
> +
> +    *startOffset = *endOffset = -1;
> +
> +    if (coreObject->isTextControl())
> +	   text = coreObject->text();
> +    else
> +	   text = coreObject->textUnderElement();
> +
> +    char* cText = g_strdup(text.utf8().data());
> +    glong textLength = g_utf8_strlen(cText, -1);

-1 means cText is nul-terminated. Is this going to be the case? Can't we just
use strlen here?

> +    if (boundaryType == ATK_TEXT_BOUNDARY_CHAR) {
> +	   int effectiveOffset;
> +
> +	   switch(getTextFunctionType) {

space between switch and (.

> +	   case AfterOffset:
> +	       effectiveOffset = offset + 1;
> +	       break;
> +	   case BeforeOffset:
> +	       effectiveOffset = offset - 1;
> +	       break;
> +	   case AtOffset:
> +	       effectiveOffset = offset;
> +	       break;
> +	   default:
> +	       g_assert_not_reached();
> +	   }

Is g_asset_not_reached the correct behaviour here? Can't we set effectiveOffset
to NULL in this case?

> +	   *startOffset = effectiveOffset;
> +	   *endOffset = effectiveOffset + 1;

what should be the behaviour if effectiveOffset is NULL? should we set
start/end offsets to 0?

> +    } else {
> +	   PangoLogAttr* attrs = g_new(PangoLogAttr, textLength+1);

spaces in textLength+1

> +	   PangoLanguage* language = pango_language_get_default();
> +	   pango_get_log_attrs(cText, -1, -1, language, attrs, textLength+1);

ditto.

> +	 
> +	   isCharacterAttribute predicate;
> +
> +	   if (boundaryType == ATK_TEXT_BOUNDARY_WORD_START)
> +	       predicate = isWordStart;
> +	   else if (boundaryType == ATK_TEXT_BOUNDARY_WORD_END)
> +	       predicate = isWordEnd;
> +	   else if (boundaryType == ATK_TEXT_BOUNDARY_SENTENCE_START)
> +	       predicate = isSentenceStart;
> +	   else if (boundaryType == ATK_TEXT_BOUNDARY_SENTENCE_END)
> +	       predicate = isSentenceEnd;
> +	   else
> +	       g_assert_not_reached();

what should be the default behaviour here?

> +
> +	   switch(boundaryType) {

space between switch and (.

> +	   case ATK_TEXT_BOUNDARY_WORD_START:
> +	   case ATK_TEXT_BOUNDARY_SENTENCE_START:
> +	       if (getTextFunctionType == AfterOffset) {
> +		   // Take the item after the current one in any case
> +		   findCharacterAttribute(predicate, attrs, DirectionForward,
offset+1, textLength + 1, startOffset);
> +		   findCharacterAttributeSkip(predicate, 1, attrs,
DirectionForward, offset+1, textLength + 1, endOffset);

Spaces in offset+1. Also, what's the difference between the two?

> +	       } else if (getTextFunctionType == AtOffset) {
> +		   // Take the item at point if the offset is in an item or
> +		   // the item before otherwise
> +		   findCharacterAttribute(predicate, attrs, DirectionBackwards,
offset, textLength + 1, startOffset);
> +		   if (!findCharacterAttribute(predicate, attrs,
DirectionForward, offset+1, textLength + 1, endOffset)) {
> +		       findCharacterAttribute(oppositePredicate(predicate),
attrs, DirectionForward, offset+1, textLength + 1, endOffset);

spaces in offset+1.

> +		   findCharacterAttribute(predicate, attrs, DirectionForward,
offset+1, textLength + 1, endOffset);

ditto.

> +	       } else {
> +		   // Take the item before the point in any case
> +		   if (!findCharacterAttributeSkip(predicate, 1, attrs,
DirectionBackwards, offset, textLength + 1, startOffset)) {
> +		       int tmpOffset;
> +		       // No match before offset, take the first opposite match
at or before the offset
> +		       findCharacterAttribute(predicate, attrs,
DirectionBackwards, offset, textLength + 1, &tmpOffset);
> +		       findCharacterAttribute(oppositePredicate(predicate),
attrs, DirectionBackwards, tmpOffset-1, textLength + 1, startOffset);

spaces in tmpOffset-1.

> +	   default:
> +	       g_assert_not_reached();

is asserting the right behaviour here? 

> +    char* start = g_utf8_offset_to_pointer(cText, (glong)*startOffset);
> +    char* end = g_utf8_offset_to_pointer(cText, (glong)*endOffset);
> +    char* resultText = g_strndup(start, end - start);
> +    g_free(cText);
> +    return resultText;

should we g_strdup resultText?

> +}
> +
> +static gchar* webkit_accessible_text_get_text_after_offset(AtkText* text,
gint offset, AtkTextBoundary boundaryType, gint* startOffset, gint* endOffset)
> +{
> +    return getTextHelper(AfterOffset, text, offset, boundaryType,
startOffset, endOffset);
> +}
> +
> +static gchar* webkit_accessible_text_get_text_at_offset(AtkText* text, gint
offset, AtkTextBoundary boundaryType, gint* startOffset, gint* endOffset)
> +{
> +    return getTextHelper(AtOffset, text, offset, boundaryType, startOffset,
endOffset);
>  }
>  
> -static gchar* webkit_accessible_text_get_text_before_offset(AtkText* text,
gint offset, AtkTextBoundary boundary_type, gint* start_offset, gint*
end_offset)
> +static gchar* webkit_accessible_text_get_text_before_offset(AtkText* text,
gint offset, AtkTextBoundary boundaryType, gint* startOffset, gint* endOffset)
> +{
> +    return getTextHelper(BeforeOffset, text, offset, boundaryType,
startOffset, endOffset);
> +}
> +
> +static gunichar webkit_accessible_text_get_character_at_offset(AtkText*
text, gint offset)

I think we should start using WebKit-style for the function names here.

r- for the style issues and the crash Joanmarie reported in comment #16. Would
also be nice to work out the default behaviour instead of just asserting.