[webkit-reviews] review granted: [Bug 25415] [GTK][ATK] Please implement support for get_text_at_offset : [Attachment 30614] gettextv4.patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has granted 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 30614: gettextv4.patch
https://bugs.webkit.org/attachment.cgi?id=30614&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
> +typedef int (*advanceFunc) (int);
> +
> +static int increaseInt(int i) {
> +    return i + 1;
> +}
> +
> +static int decreaseInt(int i) {
> +    return i - 1;
> +}
[...]
> +static bool findCharacterAttribute(isCharacterAttribute predicateFunction,
PangoLogAttr* attributes, Direction direction, int startOffset, int
attrsLength, int* resultOffset)
> +{
> +    advanceFunc advanceFunc = direction == DirectionForward ? increaseInt :
decreaseInt;
> +
> +    *resultOffset = -1;
> +
> +    for (int i = startOffset; i >= 0 && i < attrsLength; i = advanceFunc(i))
{

The patch looks good to me, except for this nitpick. I think using the
advanceFunc strategy here is a bit overblown. You should be able to use a
simple int variable here.

int advanceBy = direction == DirectionForward ? 1 : -1;

for (int i = startOffset; i >= 0 && i < attrsLength; i = i + advanceBy) {

If there's a reason that escaped my eye, please not in a comment =).