[webkit-reviews] review requested: [Bug 122644] [ATK] Simplify implementation of atk_text_get_text : [Attachment 213996] New patch proposal
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Oct 11 10:13:10 PDT 2013
Mario Sanchez Prada <mario at webkit.org> has asked for review:
Bug 122644: [ATK] Simplify implementation of atk_text_get_text
https://bugs.webkit.org/show_bug.cgi?id=122644
Attachment 213996: New patch proposal
https://bugs.webkit.org/attachment.cgi?id=213996&action=review
------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
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
More information about the webkit-reviews
mailing list