Implement ISimpleDOM COM interface in BrowserAccessibility. Also update...

Implement ISimpleDOM COM interface in BrowserAccessibility. Also update
a few IAccessible2 functions with some of the new attributes that are
available now.

BUG=48185
TEST=None


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

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

[Chris Guillory, 2010-08-31 19:10:01]

http://codereview.chromium.org/3219009/diff/1/2
File chrome/browser/browser_accessibility_win.cc (left):

http://codereview.chromium.org/3219009/diff/1/2#oldcode827
chrome/browser/browser_accessibility_win.cc:827: role_name_ = L"h1";
Nice.

http://codereview.chromium.org/3219009/diff/1/2
File chrome/browser/browser_accessibility_win.cc (right):

http://codereview.chromium.org/3219009/diff/1/2#newcode680
chrome/browser/browser_accessibility_win.cc:680: if (!instance_active_)
We should check input pointers for NULL.

http://codereview.chromium.org/3219009/diff/1/2#newcode683
chrome/browser/browser_accessibility_win.cc:683: string16 attr;
Maybe create a function like HRESULT
GetAttributeAsBstr(WebAccessibility::Attribute, BSTR*) and call it here and in
the other similar places.

http://codereview.chromium.org/3219009/diff/1/2#newcode797
chrome/browser/browser_accessibility_win.cc:797: for (unsigned int j = 0;  j <
html_attributes_.size(); ++j) {
Can we possibly use a map structure for html_attributes?

http://codereview.chromium.org/3219009/diff/1/2#newcode867
chrome/browser/browser_accessibility_win.cc:867: if (!instance_active_)
Should we be calling AddRef in these functions that are returning references?

http://codereview.chromium.org/3219009/diff/1/2#newcode919
chrome/browser/browser_accessibility_win.cc:919: index_in_parent_ <
static_cast<int>(parent_->children_.size()) - 1) {
Can index_in_parent_ be unsigned.

[dmazzoni, 2010-09-02 14:45:07]

On Tue, Aug 31, 2010 at 12:10 PM, <ctguil@chromium.org> wrote:

> http://codereview.chromium.org/3219009/diff/1/2#newcode680
> chrome/browser/browser_accessibility_win.cc:680: if (!instance_active_)
> We should check input pointers for NULL.
>

Done.


> http://codereview.chromium.org/3219009/diff/1/2#newcode683
> chrome/browser/browser_accessibility_win.cc:683: string16 attr;
> Maybe create a function like HRESULT
> GetAttributeAsBstr(WebAccessibility::Attribute, BSTR*) and call it here
> and in the other similar places.
>

Good idea, it saved several lines of code above, too.


> http://codereview.chromium.org/3219009/diff/1/2#newcode797
> chrome/browser/browser_accessibility_win.cc:797: for (unsigned int j =
> 0;  j < html_attributes_.size(); ++j) {
> Can we possibly use a map structure for html_attributes?
>

I don't think it would be worth the memory overhead. I strongly suspect that
if this method is used it only asks for a small number of attributes. We
should profile it at some point...but I don't want to prematurely optimize
for speed when we know it will increase memory usage.

http://codereview.chromium.org/3219009/diff/1/2#newcode867
> chrome/browser/browser_accessibility_win.cc:867: if (!instance_active_)
> Should we be calling AddRef in these functions that are returning
> references?
>

Good catch, done.


> http://codereview.chromium.org/3219009/diff/1/2#newcode919
> chrome/browser/browser_accessibility_win.cc:919: index_in_parent_ <
> static_cast<int>(parent_->children_.size()) - 1) {
> Can index_in_parent_ be unsigned.


Not easily, it's returned in other methods that expect a signed value, so
we'd have to cast it half the time either way.

- Dominic

[Chris Guillory, 2010-09-02 18:24:28]

LGTM.