[webkit-reviews] review granted: [Bug 120820] [Windows] Implement text offset methods of IAccessibleText interface : [Attachment 210692] patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Sep 5 20:47:52 PDT 2013
Brent Fulgham <bfulgham at webkit.org> has granted Roger Fong
<roger_fong at apple.com>'s request for review:
Bug 120820: [Windows] Implement text offset methods of IAccessibleText
interface
https://bugs.webkit.org/show_bug.cgi?id=120820
Attachment 210692: patch
https://bugs.webkit.org/attachment.cgi?id=210692&action=review
------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=210692&action=review
A couple of minor typos. Please fix when landing.
> Source/WebKit/win/AccessibleTextImpl.cpp:-206
> - return E_NOTIMPL;
Don't we need to make sure startOffset and endOffset are non-null, and return
E_POINTER if they are?
> Source/WebKit/win/AccessibleTextImpl.cpp:209
> + if (offset < 0 && offset > m_object->text().length())
Shouldn't this be an ||? I don't know if too man things that are negative and
also larger than the text length! :)
> Source/WebKit/win/AccessibleTextImpl.cpp:215
> + int previousPos = std::max(0, (int)(offset-1));
Please use a C++ cast.
> Source/WebKit/win/AccessibleTextImpl.cpp:256
> + *text = SysAllocStringLen(substringText.characters(),
substringText.length());
On - we should check that text is also a valid pointer passed to the function?
> Source/WebKit/win/AccessibleTextImpl.cpp:271
> + if (offset < 0 && offset > textLength)
These two cases are exclusive; use ||
> Source/WebKit/win/AccessibleTextImpl.cpp:311
> + *endOffset = textRange.end.deepEquivalent().offsetInContainerNode();
Check locations and return E_POINTER if they are not valid.
> Source/WebKit/win/AccessibleTextImpl.cpp:317
> + *text = SysAllocStringLen(substringText.characters(),
substringText.length());
Ditto for text.
> Source/WebKit/win/AccessibleTextImpl.cpp:333
> + if (offset < 0 && offset > textLength)
|| please.
More information about the webkit-reviews
mailing list