Implement additional IAccessibleText methods to return the nearest...

Implement additional IAccessibleText methods to return the nearest
character, word, line, sentence, or paragraph within a text field.
These methods are needed by SAToGo.

BUG=36217
TEST=Added new unit test.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=61979

[Paweł Hajdan Jr., 2010-10-07 16:51:23]

Drive-by with a test comment. No need to wait for another review by me.

http://codereview.chromium.org/3550017/diff/6001/7003
File chrome/browser/accessibility/browser_accessibility_win_unittest.cc (right):

http://codereview.chromium.org/3550017/diff/6001/7003#newcode299
chrome/browser/accessibility/browser_accessibility_win_unittest.cc:299:
BrowserAccessibilityManagerWin* manager =
Should this be a scoped_ptr?

[Chris Guillory, 2010-10-07 17:02:54]

Looks great in general. Some comments.

http://codereview.chromium.org/3550017/diff/6001/7001
File chrome/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/3550017/diff/6001/7001#newcode1209
chrome/browser/accessibility/browser_accessibility_win.cc:1209: if (result ==
text_size)
Should we change this to >= since we only DCHECK start_offset <= text_size
above? Or maybe explicitly set start_offset to be in the expected range here or
in the callers.

http://codereview.chromium.org/3550017/diff/6001/7001#newcode1213
chrome/browser/accessibility/browser_accessibility_win.cc:1213: if (result == 0)
Should we change this <= since we only DCHECK start_offset >= 0 above?

http://codereview.chromium.org/3550017/diff/6001/7001#newcode1228
chrome/browser/accessibility/browser_accessibility_win.cc:1228: if ((text[pos]
== '.' || text[pos] == '!') &&
Should be add '?' to this list. I vaguely remember glancing over something in
the codebase that defined punctuational boundaries if that's what we're
interested in.

http://codereview.chromium.org/3550017/diff/6001/7001#newcode1237
chrome/browser/accessibility/browser_accessibility_win.cc:1237: result +=
direction;
Should be explicitly set direction to -1 or 1 since we only DCHECK above.

http://codereview.chromium.org/3550017/diff/6001/7002
File chrome/browser/accessibility/browser_accessibility_win.h (right):

http://codereview.chromium.org/3550017/diff/6001/7002#newcode492
chrome/browser/accessibility/browser_accessibility_win.h:492: long start_offset,
Nit: LONG instead of long.

[Chris Guillory, 2010-10-07 18:34:32]

More comments.

http://codereview.chromium.org/3550017/diff/6001/7003
File chrome/browser/accessibility/browser_accessibility_win_unittest.cc (right):

http://codereview.chromium.org/3550017/diff/6001/7003#newcode300
chrome/browser/accessibility/browser_accessibility_win_unittest.cc:300: new
BrowserAccessibilityManagerWin(
Interesting, I thought this constructor was private. Should be
BrowserAccessibilityManager::Create.

[dmazzoni, 2010-10-08 16:01:32]

On Thu, Oct 7, 2010 at 10:02 AM, <ctguil@chromium.org> wrote:

> http://codereview.chromium.org/3550017/diff/6001/7001#newcode1209
> chrome/browser/accessibility/browser_accessibility_win.cc:1209: if
> (result == text_size)
> Should we change this to >= since we only DCHECK start_offset <=
> text_size above? Or maybe explicitly set start_offset to be in the
> expected range here or in the callers.
>

Done. It couldn't hurt to be safe.


> http://codereview.chromium.org/3550017/diff/6001/7001#newcode1213
> chrome/browser/accessibility/browser_accessibility_win.cc:1213: if
> (result == 0)
> Should we change this <= since we only DCHECK start_offset >= 0 above?
>

Done.


> http://codereview.chromium.org/3550017/diff/6001/7001#newcode1228
> chrome/browser/accessibility/browser_accessibility_win.cc:1228: if
> ((text[pos] == '.' || text[pos] == '!') &&
> Should be add '?' to this list. I vaguely remember glancing over
> something in the codebase that defined punctuational boundaries if
> that's what we're interested in.
>

Done, but I decided to actually return S_FALSE for sentence boundary
support, since the IAccessible2 spec says that we can skip that one and
allow the screenreader to separate into sentences instead.


> http://codereview.chromium.org/3550017/diff/6001/7001#newcode1237
> chrome/browser/accessibility/browser_accessibility_win.cc:1237: result
> += direction;
> Should be explicitly set direction to -1 or 1 since we only DCHECK
> above.
>

Done. I added a NOTREACHED() and a return if direction is zero, so at least
it won't be an infinite loop if the logic is bad.


> http://codereview.chromium.org/3550017/diff/6001/7002
> File chrome/browser/accessibility/browser_accessibility_win.h (right):
>
> http://codereview.chromium.org/3550017/diff/6001/7002#newcode492
> chrome/browser/accessibility/browser_accessibility_win.h:492: long
> start_offset,
> Nit: LONG instead of long.


Changed throughout.

[dmazzoni, 2010-10-08 16:02:43]

On Thu, Oct 7, 2010 at 9:51 AM, <phajdan.jr@chromium.org> wrote:

> http://codereview.chromium.org/3550017/diff/6001/7003#newcode299
> chrome/browser/accessibility/browser_accessibility_win_unittest.cc:299:
> BrowserAccessibilityManagerWin* manager =
> Should this be a scoped_ptr?


Thanks for checking, but in this case I want to delete it explicitly - since
this code uses reference counting, there's an explicit delete and a  check
that all instances have been deleted at the bottom of the function.

[dmazzoni, 2010-10-08 16:03:17]

On Thu, Oct 7, 2010 at 11:34 AM, <ctguil@chromium.org> wrote:

> More comments.
>
> http://codereview.chromium.org/3550017/diff/6001/7003
> File chrome/browser/accessibility/browser_accessibility_win_unittest.cc
> (right):
>
> http://codereview.chromium.org/3550017/diff/6001/7003#newcode300
> chrome/browser/accessibility/browser_accessibility_win_unittest.cc:300:
> new BrowserAccessibilityManagerWin(
> Interesting, I thought this constructor was private. Should be
> BrowserAccessibilityManager::Create.


Fixed.

[Chris Guillory, 2010-10-08 17:15:06]

LGTM.

http://codereview.chromium.org/3550017/diff/15001/16003
File chrome/browser/accessibility/browser_accessibility_win_unittest.cc (right):

http://codereview.chromium.org/3550017/diff/15001/16003#newcode298
chrome/browser/accessibility/browser_accessibility_win_unittest.cc:298:
static_cast<BrowserAccessibilityWin*>(manager->GetRoot());
You can avoid a cast here by using toBrowserAccessibilityWin().

http://codereview.chromium.org/3550017/diff/15001/16003#newcode299
chrome/browser/accessibility/browser_accessibility_win_unittest.cc:299:
BrowserAccessibilityWin* text1_obj =
Optional: It might be interesting to see if you could use a subset of the
interface since text1_obj looks like it could be an IAccessibleText*.