Bug 122644 – [ATK] Simplify implementation of atk_text_get_text

• Component: Accessibility
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

Currently, the implementation of atk_text_get_text() is definitely not the most efficient in the world (multiple times to textUnderElement, treating ColorWell elements at the end instead of at the beginning...) and that needs to change IMHO, so this bug is just for simplifying that function, while still keep passing the tests.

Actually more things need to change in this file and this bug is probably a first step in that direction, but let's go step by step.

[Radar WebKit Bug Importer]

<rdar://problem/15206444>

[Mario Sanchez Prada]

Created attachment 213978 [details]
Patch proposal

[Mario Sanchez Prada]

Created attachment 213996 [details]
New patch proposal

I just realized that the previous patch had an issue: when calling doAXStringForRange() from the AtkText implementation, it's very common that we pass (0, -1) as start/length offset, which is something doAXStringForRange() won't handle:

  String AccessibilityRenderObject::doAXStringForRange(const PlainTextRange& range) const
  {
     [...]
     String elementText = isPasswordField() ? passwordFieldValue() : text();
     if (range.start + range.length > elementText.length())
         return String();
    
     return elementText.substring(range.start, range.length);
  }

However, I don't see any reason why doAXStringForRange() could not handle a (start, -1) range, since String::substring() would gracefully handle that situation in case we passed a too big length as a second parameter, or just -1 (which would be translated to an unsigned). Besides, it seems to me that the check (range.start + range.length > elementText.length()) might be wrong anyway, or at least I don't understand very well what it's trying to check.

For all those reasons, the current patch proposes to remove that if and let String::substring() handle the out-of-bounds scenario when passing start/end

[chris fleizach]

Comment on attachment 213996 [details]
New patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=213996&action=review

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:-2113
> -    if (range.start + range.length > elementText.length())

why remove this check? it seems like a good safety check to have here

[Mario Sanchez Prada]

(In reply to comment #4)
> (From update of attachment 213996 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213996&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:-2113
> > -    if (range.start + range.length > elementText.length())
> 
> why remove this check? it seems like a good safety check to have here

I have three reasons mainly :):

1. Because I think it's checking the wrong thing, since a proper check to see if the range is out of bounds would be something like this, IMO:

   (range.length - range.start > elementText.length())

2. Because in any case the String::substring() method will check, and adjust if needed, any limit that falls out of bounds for that string.

3. Because that way ranges like (0, -1) will actually work as "request all the test", and that's convenient from the ATK perspective (yet I understand it might be not the best reason)

[chris fleizach]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 213996 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=213996&action=review
> > 
> > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:-2113
> > > -    if (range.start + range.length > elementText.length())
> > 
> > why remove this check? it seems like a good safety check to have here
> 
> I have three reasons mainly :):
> 
> 1. Because I think it's checking the wrong thing, since a proper check to see if the range is out of bounds would be something like this, IMO:
> 
>    (range.length - range.start > elementText.length())

I think that's wrong. I think we still want to check the MAXRange of that range, which would be +.

> 
> 2. Because in any case the String::substring() method will check, and adjust if needed, any limit that falls out of bounds for that string.

If it in fact does do this, then it's OK to remove. I was assuming that would throw an error

> 
> 3. Because that way ranges like (0, -1) will actually work as "request all the test", and that's convenient from the ATK perspective (yet I understand it might be not the best reason)

[Mario Sanchez Prada]

(In reply to comment #6)
> [...]
> > 1. Because I think it's checking the wrong thing, since a proper check to see if the range is out of bounds would be something like this, IMO:
> > 
> >    (range.length - range.start > elementText.length())
> 
> I think that's wrong. I think we still want to check the MAXRange of that 
> range, which would be +.

Stupid me. I wast reading that line as if was using the final position ("range.end", if it existed) instead of the length.

You are actually checking that the range of text does not fall out of the limits of the text control, hence the original check is semantically right, not mine.

> > 
> > 2. Because in any case the String::substring() method will check, and adjust if needed, any limit that falls out of bounds for that string.
> 
> If it in fact does do this, then it's OK to remove. I was assuming that
> would throw an error
> 

This is the implementation of substring():

  PassRefPtr<StringImpl> StringImpl::substring(unsigned start, unsigned length)
  {
    if (start >= m_length)
        return empty();
    unsigned maxLength = m_length - start;
    if (length >= maxLength) {
        if (!start)
            return this;
        length = maxLength;
    }
    if (is8Bit())
        return create(m_data8 + start, length);

    return create(m_data16 + start, length);
  }


AS far as I can see, if the length passed is bigger than the String's upper limit, this implementation will just adjust it to maxLength, so I think it should be fine to remove that check anyway.

What do you think?

[Mario Sanchez Prada]

Committed r157628: <http://trac.webkit.org/changeset/157628>