Bug 112015

Created attachment 215646 [details] Proposed patch

Created attachment 215648 [details] Rebased patch for a change

Comment on attachment 215648 [details] Rebased patch for a change View in context: https://bugs.webkit.org/attachment.cgi?id=215648&action=review > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:995 > + if (!ATK_IS_TEXT(m_element)) > + return -1; We normally put this check at the beginning of the function. Could you please move it there? > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:998 > + if (index > atk_text_get_character_count(ATK_TEXT(m_element))) > + return -1; Maybe you could join this check with the index < 0 in a single if condition. Also, you probably want to check index >= atk_text_get_character_count(ATK_TEXT(m_element)) here (opposite to just >) > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:1003 > + GOwnPtr<gchar> text(atk_text_get_text(ATK_TEXT(m_element), 0, index)); > + > + int lineNo = 0; > + You don't need these two extra blank lines here > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:1006 > + for (gchar* offset = text.get(); *offset; ++offset) > + if (*offset == '\n') > + ++lineNo; Please add braces to this for loop. Even if it only has the if statement inside, it's multiline content and adding braces will make it clearer > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:1035 > int AccessibilityUIElement::lineForIndex(int index) Same considerations here.

(In reply to comment #3) > Also, you probably want to check index >= atk_text_get_character_count(ATK_TEXT(m_element)) here (opposite to just >) If I understand correctly, the index equal to the string's length corresponds to a situation where the caret is placed right after the last character. Wouldn't that cause additional failures in cases where the string in question ended with a newline?

(In reply to comment #4) > (In reply to comment #3) > > Also, you probably want to check index >= atk_text_get_character_count(ATK_TEXT(m_element)) here (opposite to just >) > > If I understand correctly, the index equal to the string's length corresponds > to a situation where the caret is placed right after the last character. > Wouldn't that cause additional failures in cases where the string in question > ended with a newline? I'm not sure I understand what you mean? What kind of failures are you considering? Do you have an specific example/failing test case that we can use to discuss about this?

(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > Also, you probably want to check index >= atk_text_get_character_count(ATK_TEXT(m_element)) here (opposite to just >) > > > > If I understand correctly, the index equal to the string's length corresponds > > to a situation where the caret is placed right after the last character. > > Wouldn't that cause additional failures in cases where the string in question > > ended with a newline? > > I'm not sure I understand what you mean? What kind of failures are you considering? Do you have an specific example/failing test case that we can use to discuss about this? Let's consider a test string of "\n". Wouldn't a valid index range in this case be [0,1], with 0 corresponding to a caret placed just before the newline and 1 right after it, resulting in returned line numbers of 0 and 1 respectively? If so, shouldn't the index validity check accept ranges up to and including the length of the input string?

(In reply to comment #6) > [...] > Let's consider a test string of "\n". Wouldn't a valid index range in this > case be [0,1], with 0 corresponding to a caret placed just before the newline > and 1 right after it, resulting in returned line numbers of 0 and 1 > respectively? If so, shouldn't the index validity check accept ranges up to > and including the length of the input string? I see. The thing that confuses me a bit is that in this case the 'index' parameter actually acts more like a 'count'/'len' than as a 'index'/'position' in the text. From that point of view, then obviously the text length is a valid value for that, and so my previous comment is non-sense. Sorry about that and thanks for the clarification.

Created attachment 215918 [details] Corrected patch

Comment on attachment 215918 [details] Corrected patch Thanks

Comment on attachment 215918 [details] Corrected patch Clearing flags on attachment: 215918 Committed r158576: <http://trac.webkit.org/changeset/158576>

All reviewed patches have been landed. Closing bug.