[webkit-reviews] review granted: [Bug 97246] AX: Chromium doesn't pass accessibility text range and line number tests : [Attachment 165146] Rebase

[bugzilla-daemon at webkit.org]

chris fleizach <cfleizach at apple.com> has granted Dominic Mazzoni
<dmazzoni at google.com>'s request for review:
Bug 97246: AX: Chromium doesn't pass accessibility text range and line number
tests
https://bugs.webkit.org/show_bug.cgi?id=97246

Attachment 165146: Rebase
https://bugs.webkit.org/attachment.cgi?id=165146&action=review

------- Additional Comments from chris fleizach <cfleizach at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=165146&action=review


mostly minor nits. i think this looks ok once those are addressed

> Source/WebCore/accessibility/AccessibilityObject.h:637
> +    virtual void allLineBreaks(Vector<int>&) const { }

i think "all" is implicit in the name. Just naming it "lineBreaks" feels like
it's enough

> Source/WebKit/chromium/src/WebAccessibilityObject.cpp:770
> +    for (size_t i = 0; i< lineBreaksVector.size(); i++)

i would move .size() call out of for loop so it doesn't have to be called each
iteration

>
Tools/DumpRenderTree/chromium/TestRunner/AccessibilityUIElementChromium.cpp:647

> +    while (line < static_cast<int>(lineBreaks.size()) && lineBreaks[line] <=
index)

move .size() call out of loop