[webkit-reviews] review denied: [Bug 112015] [EFL] accessibility/textarea-line-for-index.html is failing : [Attachment 215648] Rebased patch for a change
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Nov 1 03:30:34 PDT 2013
Mario Sanchez Prada <mario at webkit.org> has denied Rob Płóciennik
<r.plociennik at samsung.com>'s request for review:
Bug 112015: [EFL] accessibility/textarea-line-for-index.html is failing
https://bugs.webkit.org/show_bug.cgi?id=112015
Attachment 215648: Rebased patch for a change
https://bugs.webkit.org/attachment.cgi?id=215648&action=review
------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
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.
More information about the webkit-reviews
mailing list