Removes all use of COM and dependencies on Windows-specific classes (includin...

Removes all use of COM and dependencies on Windows-specific classes (including the use of AccessibleBase and AccessibleDocument) in the glue accessibility implementation.Introduces the GlueAccessibilityObject, which serves as a platform-independent wrapper directly around WebKit's AccessibilityObject (also platoform-independent).Updates naming/comments to reflect the independence of IAccessible both in glue and (where appropriate) in the browser-side accessibility. 

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

[darin (slow to review), 2009-03-14 21:15:01]

Thanks for working on this!

http://codereview.chromium.org/46013/diff/1015/19
File chrome/browser/browser_accessibility.cc (right):

http://codereview.chromium.org/46013/diff/1015/19#newcode575
Line 575: if ((browser_accessibility_state >> STATE_LINKED) & 1)
this is totally optional, but for things like this an array + for loop may be
nicer (so you don't have to duplicate the bit munging).

const struct {
  long from;
  long to;
} kMap[] = {
  { STATE_LINKED, STATE_SYSTEM_LINKED },
  ...
};
for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kMap); ++i) {
  if ((browser_accessibility_state >> kMap[i].from) & 1)
    state |= kMap[i].to;
}

http://codereview.chromium.org/46013/diff/1015/22
File chrome/browser/browser_accessibility_manager.cc (right):

http://codereview.chromium.org/46013/diff/1015/22#newcode15
Line 15: static const int kAccessibilityMessageTimeOut = 10000;
will any chrome build respond to MSAA events automatically or does it have to be
enabled by a command line flag?  could this 10 seconds of blocking be
experienced by users who don't care about assistive technologies?

http://codereview.chromium.org/46013/diff/1015/18
File chrome/common/accessibility.h (right):

http://codereview.chromium.org/46013/diff/1015/18#newcode43
Line 43: DIR_NONE = 0,
it might be better to spell out DIRECTION to avoid conflicts with other uses of
DIR_.  maybe all of this should be namespaced.

http://codereview.chromium.org/46013/diff/1015/25
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/46013/diff/1015/25#newcode2660
Line 2660: glue_accessibility_manager_.reset(new GlueAccessibilityManager());
it would be better to create an abstract interface WebAccessibilityManager.  you
can then add a static method to that named "Create" and call that here (like we
do for WebView).  the implementation of that interface would be
WebAccessibilityManagerImpl.

http://codereview.chromium.org/46013/diff/1015/29
File webkit/glue/glue_accessibility_manager.cc (right):

http://codereview.chromium.org/46013/diff/1015/29#newcode189
Line 189: } else if (!root_->acc_obj_root_) {
nit: no need for else after return

http://codereview.chromium.org/46013/diff/1015/29#newcode218
Line 218: } else {
nit: you can delete this "else" and then un-indent the rest of the function :)

http://codereview.chromium.org/46013/diff/1015/28
File webkit/glue/glue_accessibility_manager.h (right):

http://codereview.chromium.org/46013/diff/1015/28#newcode9
Line 9: #include "chrome/common/accessibility.h"
webkit/glue cannot depend on anything under chrome/.  it is okay for chrome/ to
depend on webkit/, so perhaps you should move the shared types into
webkit/glue/.

http://codereview.chromium.org/46013/diff/1015/28#newcode14
Line 14: typedef base::hash_map<int, GlueAccessibilityObject* > IntToAccObjMap;
kind of sucks to have these at global scope.  how about putting everything here
into the webkit_glue namespace?

http://codereview.chromium.org/46013/diff/1015/28#newcode25
Line 25: class GlueAccessibilityManager {
please use Web as the prefix instead of Glue.

http://codereview.chromium.org/46013/diff/1015/26
File webkit/glue/glue_accessibility_object.cc (right):

http://codereview.chromium.org/46013/diff/1015/26#newcode426
Line 426: return false;
nit: indentation is off

http://codereview.chromium.org/46013/diff/1015/26#newcode439
Line 439: return false;
nit: indentation is off

http://codereview.chromium.org/46013/diff/1015/26#newcode452
Line 452: result = CreateInstance(obj);
nit: indentation is off

http://codereview.chromium.org/46013/diff/1015/27
File webkit/glue/glue_accessibility_object.h (right):

http://codereview.chromium.org/46013/diff/1015/27#newcode10
Line 10: MSVC_PUSH_WARNING_LEVEL(0);
can you try compiling without these warning suppressions?  webcore should
compile cleanly these days.

http://codereview.chromium.org/46013/diff/1015/27#newcode15
Line 15: #include "chrome/common/accessibility.h"
same issue with incorrectly including code from chrome/

[darin (slow to review), 2009-03-18 20:38:35]

http://codereview.chromium.org/46013/diff/1026/2003
File chrome/common/accessibility.h (right):

http://codereview.chromium.org/46013/diff/1026/2003#newcode6
Line 6: #define CHROME_COMMON_ACCESSIBILITY_H_
looks like this file should be deleted now?

http://codereview.chromium.org/46013/diff/1026/2016
File webkit/glue/accessibility.h (right):

http://codereview.chromium.org/46013/diff/1026/2016#newcode5
Line 5: #ifndef WEBKIT_GLUE_ACCESSIBILITY_H_
nit: webaccessibility.h to be consistent with the web* prefix for public
headers.

http://codereview.chromium.org/46013/diff/1026/2016#newcode12
Line 12: enum AccFuncId {
maybe all of these types can be in the webkit_glue namespace or perhaps the
individual type names can be prefixed with Web?

or perhaps it would be good to just put everything inside a class named
WebAccessibility:

  class WebAccessibility {
   public:
    enum Function {
      FUNCTION_NONE,
      ...
    };
    enum Direction {
      DIRECTION_NONE,
      ...
    };
    enum Role {
      ROLE_PUSHBUTTON,
      ...
    };
    enum State {
      STATE_LINKED,
      ...
    };
    struct InParams {
      ...
    };
    struct OutParams {
      ...
    };
  };

this way you can make the type names a bit more brief (leaving out ACC_ and
Browser, etc.).

http://codereview.chromium.org/46013/diff/1026/2016#newcode136
Line 136: std::wstring output_string;
nit: can this be string16 instead?  we are trying to replace our wstring usage
with string16 to help out mac & linux.

http://codereview.chromium.org/46013/diff/1026/2012
File webkit/glue/glue_accessibility_object.h (right):

http://codereview.chromium.org/46013/diff/1026/2012#newcode32
Line 32: virtual void AddRef(void);
why don't we just use WTF::RefPtr instead?  why do we need AddRef and Release
here?

http://codereview.chromium.org/46013/diff/1026/2012#newcode101
Line 101: virtual WebCore::String name() const;
hmm... do these need to be virtual?  i don't see them defined on
AccessibilityObjectWrapper, and i don't see any subclass of
GlueAccessibilityObject that implements them.  am i overlooking something?

http://codereview.chromium.org/46013/diff/1026/2012#newcode116
Line 116: static GlueAccessibilityObject*
wrapper(WebCore::AccessibilityObject*);
nit: wrapper -> ToWrapper ?

http://codereview.chromium.org/46013/diff/1026/2013
File webkit/glue/webaccessibility_manager.h (right):

http://codereview.chromium.org/46013/diff/1026/2013#newcode6
Line 6: #define WEBKIT_GLUE_WEBACCESSIBILITY_MANAGER_H_
nit: this file should be named webaccessibilitymanager.h to be consistent with
the way we have named other files in webkit/glue.

http://codereview.chromium.org/46013/diff/1026/2013#newcode8
Line 8: #include "base/hash_tables.h"
nit: looks like this header isn't needed

http://codereview.chromium.org/46013/diff/1026/2013#newcode9
Line 9: #include "webkit/glue/accessibility.h"
nit: looks like you can replace this header with some forward decls of
AccessibilityInParams and AccessibilityOutParams

http://codereview.chromium.org/46013/diff/1026/2013#newcode46
Line 46: virtual bool InitAccObjRoot(WebView* view) = 0;
it seems strange for an interface to have private pure virtual methods.

[darin (slow to review), 2009-03-19 19:31:13]

Looks good to me!  Just spotted a few more things:

http://codereview.chromium.org/46013/diff/5002/1071
File webkit/glue/glue_accessibility_object.cc (right):

http://codereview.chromium.org/46013/diff/5002/1071#newcode15
Line 15: using namespace WebCore;
nit: google style is to import each item from WebCore that you need instead of
importing the entire namespace.  (in some cases you still use WebCore:: in the
code.)

http://codereview.chromium.org/46013/diff/5002/1076
File webkit/glue/webaccessibility.h (right):

http://codereview.chromium.org/46013/diff/5002/1076#newcode111
Line 111: int acc_obj_id;
sorry... more nits:

can this just be object_id?  we don't need the acc_ prefix anymore right?  same
goes for acc_func_id.  how about function_id?

http://codereview.chromium.org/46013/diff/5002/1076#newcode131
Line 131: int acc_obj_id;
ditto

http://codereview.chromium.org/46013/diff/5002/1077
File webkit/glue/webaccessibilitymanager.h (right):

http://codereview.chromium.org/46013/diff/5002/1077#newcode20
Line 20: class WebAccessibilityManager {
if you wanted to put all of this in the webkit_glue namespace too that would be
ok.  in general, everything in webkit/glue should be in the webkit_glue
namespace.  we just didn't start out that way, so most of it is not :-(  this is
optional for this CL... but given all of the webkit_glue::, i thought i'd
mention it.

http://codereview.chromium.org/46013/diff/5002/1079
File webkit/glue/webaccessibilitymanager_impl.cc (right):

http://codereview.chromium.org/46013/diff/5002/1079#newcode7
Line 7: #pragma warning(push, 0)
are these pragmas needed?  if so, please use the MSVC_ macros from
base/compiler_specific.h instead.  but hopefully the pragmas are unnecessary :)

[darin (slow to review), 2009-03-20 17:23:15]

LGTM

http://codereview.chromium.org/46013/diff/5008/5025
File webkit/glue/webaccessibilitymanager.h (right):

http://codereview.chromium.org/46013/diff/5008/5025#newcode52
Line 52: };  // namespace webkit_glue
nit: no ';' when closing a namespace.