704852 - Calculate compatibility mode when accessibility starts

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

::GetModuleHandle is expensive call (see bug 703198) for details. I suggest to calculate compatibility mode at accessibility start up and don't check it during runtime. Anyway Firefox isn't switched between compatibility modes successfully when you change screen readers during Firefox run or run Firefox before screen reader.

Comment 1

[alexander :surkov (:asurkov)]

try server build will be available here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8cd24fdac85b

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 576519 [details] [diff] [review]
patch

David touched this code recently and it seems he's hot to review it ;) Changing review requests :)

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to alexander surkov from comment #0)
> Created attachment 576519 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> ::GetModuleHandle is expensive call (see bug 703198) for details. I suggest

It'd be nice to understand exactly how slow.

> to calculate compatibility mode at accessibility start up and don't check it
> during runtime. Anyway Firefox isn't switched between compatibility modes
> successfully when you change screen readers during Firefox run or run
> Firefox before screen reader.

I feel like that's only mostly true, and a direction it would be nice to move away from.
This is only most

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> (In reply to alexander surkov from comment #0)
> > Created attachment 576519 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > patch
> > 
> > ::GetModuleHandle is expensive call (see bug 703198) for details. I suggest
> 
> It'd be nice to understand exactly how slow.

it regressed perf test at 2% (bug 703198), that's not number in milliseconds but I think it gives us good reason to reduce amount of these calls.

> > to calculate compatibility mode at accessibility start up and don't check it
> > during runtime. Anyway Firefox isn't switched between compatibility modes
> > successfully when you change screen readers during Firefox run or run
> > Firefox before screen reader.
> 
> I feel like that's only mostly true, and a direction it would be nice to
> move away from.
> This is only most

we need to watch when module is loaded into our process and start invalidation of things we did compatible for specific AT. Anyway compatibility mode always will be based on some assumptions and it can't work correctly in some cases (for example, when two screen readers are running that use different compatibility modes). Sure we should try to keep the number of cases low as possible but it's not high priority because that makes rather for testing than for real users.

Comment 5

[Trevor Saunders (:tbsaunde)]

Comment on attachment 576519 [details] [diff] [review]
patch

>diff --git a/accessible/src/msaa/Compatibility.cpp b/accessible/src/msaa/Compatibility.cpp
>new file mode 100644
>--- /dev/null
>+++ b/accessible/src/msaa/Compatibility.cpp
>@@ -0,0 +1,121 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: set ts=2 et sw=2 tw=80: */

\O/

>+bool
>+compatibility::IsJAWS()

maybe IsJawsRunning()? and same for others

>+#include "nsAccessNodeWrap.h"

why?

>+/**
>+ * Return true if JAWS disabled.

shouldn't that be enabled?

>+/**
>+ * Return true if WE is running.

nit, different wording from jaws one.

>+/**
>+ * Used to manage compatibility settings across ATs. The instance of the class
>+ * is managed by nsAccessNodeWrap.
>+ */
>+class Manager

this looks like a static int private to the compatability code dressed up in a fancy suit.  the int would take negligibly less memory, be simpler and smaller, and ever so slightly faster since init / shutdown have one less malloc / free.

>+  bool IsModuleVersionLessThan(HMODULE aModuleHandle,
>+                               DWORD aMajor, DWORD aMinor);

why is this exposed out of compatability.cpp?

>+  delete gCompatibilityMgr;
>+  gCompatibilityMgr = nsnull;

if you really want to keep this object an nsAutoPtr might make this a little simpler

>+  if (compatibility::IsJAWS()) {
>+    if (eventType == nsIAccessibleEvent::EVENT_SELECTION &&
>+      accessible->Role() == nsIAccessibleRole::ROLE_COMBOBOX_OPTION) {

why 2 ifs not one?

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to alexander surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > (In reply to alexander surkov from comment #0)
> > > Created attachment 576519 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review] [diff] [details] [review]
> > > patch
> > > 
> > > ::GetModuleHandle is expensive call (see bug 703198) for details. I suggest
> > 
> > It'd be nice to understand exactly how slow.
> 
> it regressed perf test at 2% (bug 703198), that's not number in milliseconds
> but I think it gives us good reason to reduce amount of these calls.

I don't disagree with the conclusion, however I think it would be useful to understand what say an additional 1msec per eveent costs as a perf percentage, and what the overall effect is.>

> > > to calculate compatibility mode at accessibility start up and don't check it
> > > during runtime. Anyway Firefox isn't switched between compatibility modes
> > > successfully when you change screen readers during Firefox run or run
> > > Firefox before screen reader.
> > 
> > I feel like that's only mostly true, and a direction it would be nice to
> > move away from.
> > This is only most
> 
> we need to watch when module is loaded into our process and start
> invalidation of things we did compatible for specific AT. Anyway
> compatibility mode always will be based on some assumptions and it can't
> work correctly in some cases (for example, when two screen readers are
> running that use different compatibility modes). Sure we should try to keep
> the number of cases low as possible but it's not high priority because that
> makes rather for testing than for real users.


that's fair enough, the case of 2 screen readers sems unimportant, but I think it'd be nice for users to be able to change screen readers and not need to restart there browser.  I'll guess Marco has more recent experience with this than me though.

in any case I'd be happy to take something like this with comment 4  addressed.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> Comment on attachment 576519 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> >diff --git a/accessible/src/msaa/Compatibility.cpp b/accessible/src/msaa/Compatibility.cpp
> >new file mode 100644
> >--- /dev/null
> >+++ b/accessible/src/msaa/Compatibility.cpp
> >@@ -0,0 +1,121 @@
> >+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> >+/* vim: set ts=2 et sw=2 tw=80: */
> 
> \O/

sorry, what's wrong with that?

> >+bool
> >+compatibility::IsJAWS()
> 
> maybe IsJawsRunning()? and same for others

they are used to deal with compatibility modes. Technically JAWS might be not running but we can be in JAWS compatible mode. Maybe proper name would IsJAWSMode() but I liked IsJAWS() because it's short and compatibility::IsJAWS() sounds descriptive. So what's your preference?

> >+#include "nsAccessNodeWrap.h"
> 
> why?

artifact from previous patch versions

> 
> >+/**
> >+ * Return true if JAWS disabled.
> 
> shouldn't that be enabled?

yes, copy/paste error, thanks

> 
> >+/**
> >+ * Return true if WE is running.
> 
> nit, different wording from jaws one.

yep, will fix it

> >+/**
> >+ * Used to manage compatibility settings across ATs. The instance of the class
> >+ * is managed by nsAccessNodeWrap.
> >+ */
> >+class Manager
> 
> this looks like a static int private to the compatability code dressed up in
> a fancy suit.  the int would take negligibly less memory, be simpler and
> smaller, and ever so slightly faster since init / shutdown have one less
> malloc / free.

the class is supposed to control compatibility initialization/shutdown, if you put initialization/shutdown in namespace as functions then it's just functions and we can't control who and why calls them. It's just logic encapsulation. I don't think global object creation can hit us in performance terms.

> >+  bool IsModuleVersionLessThan(HMODULE aModuleHandle,
> >+                               DWORD aMajor, DWORD aMinor);
> 
> why is this exposed out of compatability.cpp?

it's a private method of the class that's used for methods implementation. Which way is better?

> >+  delete gCompatibilityMgr;
> >+  gCompatibilityMgr = nsnull;
> 
> if you really want to keep this object an nsAutoPtr might make this a little
> simpler

sometime ago I've heard that we shouldn't use smart pointers as globals. I don't recall a reason. But this a pattern we use in our code (refer to gAccessibilityService for example)

> >+  if (compatibility::IsJAWS()) {
> >+    if (eventType == nsIAccessibleEvent::EVENT_SELECTION &&
> >+      accessible->Role() == nsIAccessibleRole::ROLE_COMBOBOX_OPTION) {
> 
> why 2 ifs not one?

I thought it must be nicer :)

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #6)
> (In reply to alexander surkov from comment #4)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > > (In reply to alexander surkov from comment #0)
> > > > Created attachment 576519 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review] [diff] [details] [review] [diff] [details] [review]
> > > > patch
> > > > 
> > > > ::GetModuleHandle is expensive call (see bug 703198) for details. I suggest
> > > 
> > > It'd be nice to understand exactly how slow.
> > 
> > it regressed perf test at 2% (bug 703198), that's not number in milliseconds
> > but I think it gives us good reason to reduce amount of these calls.
> 
> I don't disagree with the conclusion, however I think it would be useful to
> understand what say an additional 1msec per eveent costs as a perf
> percentage, and what the overall effect is.>

I'm always up to know exact answer but I don't always want look it up myself :) especially when exact answer is sort of nice option.

> that's fair enough, the case of 2 screen readers sems unimportant, but I
> think it'd be nice for users to be able to change screen readers and not
> need to restart there browser.  I'll guess Marco has more recent experience
> with this than me though.

I agree. Nice to have. I'll file a bug for that.

> in any case I'd be happy to take something like this with comment 4 
> addressed.

comment #4 was mine :)

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to alexander surkov from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > Comment on attachment 576519 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > patch
> > 
> > >diff --git a/accessible/src/msaa/Compatibility.cpp b/accessible/src/msaa/Compatibility.cpp
> > >new file mode 100644
> > >--- /dev/null
> > >+++ b/accessible/src/msaa/Compatibility.cpp
> > >@@ -0,0 +1,121 @@
> > >+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> > >+/* vim: set ts=2 et sw=2 tw=80: */
> > 
> > \O/
> 
> sorry, what's wrong with that?

its a good thing.

> 
> > >+bool
> > >+compatibility::IsJAWS()
> > 
> > maybe IsJawsRunning()? and same for others
> 
> they are used to deal with compatibility modes. Technically JAWS might be
> not running but we can be in JAWS compatible mode. Maybe proper name would
> IsJAWSMode() but I liked IsJAWS() because it's short and
> compatibility::IsJAWS() sounds descriptive. So what's your preference?

I guess that's fair leaving it seems fine

> > >+/**
> > >+ * Used to manage compatibility settings across ATs. The instance of the class
> > >+ * is managed by nsAccessNodeWrap.
> > >+ */
> > >+class Manager
> > 
> > this looks like a static int private to the compatability code dressed up in
> > a fancy suit.  the int would take negligibly less memory, be simpler and
> > smaller, and ever so slightly faster since init / shutdown have one less
> > malloc / free.
> 
> the class is supposed to control compatibility initialization/shutdown, if
> you put initialization/shutdown in namespace as functions then it's just
> functions and we can't control who and why calls them. It's just logic
> encapsulation. I don't think global object creation can hit us in

I think trying to use technical means to control who calls what function is silly and won't work.  For example you  can prevent people from calling Manager() but you can't prevent people from calling nsAccessNodeWrap::Init() etc.  If you wanted to do this usefully I think you'd have to have something that looks a lot like acl's on each function, and then you'd still need to deal with function pointers and people who do other "fun" things.

Practically I think its better to just include headers as few places as you can so the  identifier doesn't exist in most places.  If people go to the lengths of defining a prototype themselves with extern just so they can call your functions I tend to think they get what they deserve.

> performance terms.

I tend to agree, to me its just an additional reason on top of the this is entirely more complex than it needs to be.

> > >+  bool IsModuleVersionLessThan(HMODULE aModuleHandle,
> > >+                               DWORD aMajor, DWORD aMinor);
> > 
> > why is this exposed out of compatability.cpp?
> 
> it's a private method of the class that's used for methods implementation.
> Which way is better?

I don't think it matters a lot, I didn't catch it was private.

> > >+  delete gCompatibilityMgr;
> > >+  gCompatibilityMgr = nsnull;
> > 
> > if you really want to keep this object an nsAutoPtr might make this a little
> > simpler
> 
> sometime ago I've heard that we shouldn't use smart pointers as globals. I
> don't recall a reason. But this a pattern we use in our code (refer to
> gAccessibilityService for example)

yeah, I've seen that, wasn't aware ofthe reason.

> > >+  if (compatibility::IsJAWS()) {
> > >+    if (eventType == nsIAccessibleEvent::EVENT_SELECTION &&
> > >+      accessible->Role() == nsIAccessibleRole::ROLE_COMBOBOX_OPTION) {
> > 
> > why 2 ifs not one?
> 
> I thought it must be nicer :)

I think I'd disagree with having 2  ifs for the same test.

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Heh. I actually like the patch.

I'm curious to hear what Marco thinks about the switching screen readers at runtime case.

Comment 11

[Marco Zehe (:MarcoZ)]

Comment on attachment 576519 [details] [diff] [review]
patch

f- as per IRC discussion: The try-server build immediately exits with an "Error on program execution" error message, regardless what screen reader is running.

Comment 12

[Trevor Saunders (:tbsaunde)]

(In reply to David Bolter [:davidb] from comment #10)
> Heh. I actually like the patch.

I agree other than the manager thing which imho has properties of the goblin

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Making this block my Telemetry patch since we're touching the same code. I'll wait.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #9)

> I think trying to use technical means to control who calls what function is
> silly and won't work.

Sometimes I have a feeling that we battle over object-oriented approach vs procedural approach. So we need a thing that is responsible for initialization. Having a singleton class for that sounds like reasonable approach because only this object is allowed to do all magic.

>  For example you  can prevent people from calling
> Manager() but you can't prevent people from calling nsAccessNodeWrap::Init()
> etc.

This is a code I don't like, that's why I suggested to have wrappers around accessible service class so it can control whole accessibility initialization.

>  If you wanted to do this usefully I think you'd have to have something
> that looks a lot like acl's on each function, and then you'd still need to
> deal with function pointers and people who do other "fun" things.

can you give me an example applicable to our case?

> Practically I think its better to just include headers as few places as you
> can so the  identifier doesn't exist in most places.  If people go to the
> lengths of defining a prototype themselves with extern just so they can call
> your functions I tend to think they get what they deserve.

If we have builtin mechanism to do alternative of this approach then I would use them

> > > >+  if (compatibility::IsJAWS()) {
> > > >+    if (eventType == nsIAccessibleEvent::EVENT_SELECTION &&
> > > >+      accessible->Role() == nsIAccessibleRole::ROLE_COMBOBOX_OPTION) {
> > > 
> > > why 2 ifs not one?
> > 
> > I thought it must be nicer :)
> 
> I think I'd disagree with having 2  ifs for the same test.

ok, some objections why I do this way. if (compatibility::IsJAWS()) says this block is for JAWS only and if I'm not interesting in JAWS I skip it. Otherwise I should look at whole if condition. It makes me think it should allow to skim the code faster.

Comment 15

[Trevor Saunders (:tbsaunde)]

> encapsulation. I don't think global object creation can hit us in
> performance terms.

It occurs to me using a global object means calling IsFoo() takes 2 memory accesses instead of  1,  since you have to retrieve the global pointer to the object then dereference the pointer to get the bitfield itself, instead of directly getting the bitfield.  I  can't think of a reason to expect either of these memory accesses will be cache hits, but we don't call those methods terribly often so I'm not sure this is worth that much concern.

(In reply to alexander surkov from comment #14)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> 
> > I think trying to use technical means to control who calls what function is
> > silly and won't work.
> 
> Sometimes I have a feeling that we battle over object-oriented approach vs
> procedural approach. So we need a thing that is responsible for

That's probably reasonably accurate, I'll certainly grant that people have drunk a lot of the OO coolaid would claim a singleton is the write answer, but personally I tend to like the simplest most direct solution.

> initialization. Having a singleton class for that sounds like reasonable
> approach because only this object is allowed to do all magic.

I'm not clear who your trying to prevent from doing what.  I don't think anybody working in say layout/ is going to decide its a good idea to call a11y::determinCompat() or something in the middle of something, and I'm pretty sure nobody is going to be allowed to do do that in accessible/.

> >  For example you  can prevent people from calling
> > Manager() but you can't prevent people from calling nsAccessNodeWrap::Init()
> > etc.
> 
> This is a code I don't like, that's why I suggested to have wrappers around
> accessible service class so it can control whole accessibility
> initialization.

I'd like to see that stuff cleaned up some too. that feels kind of heavy weight to me,  I think my current prefered solution is  a function a11y::init() defined by each platform.  But I think the point  would still stand if they can't call that they can call ns_getAccessibilityService() or they could just copy the logic.

> >  If you wanted to do this usefully I think you'd have to have something
> > that looks a lot like acl's on each function, and then you'd still need to
> > deal with function pointers and people who do other "fun" things.
> 
> can you give me an example applicable to our case?

Well, in this patch the manager has a method to return its state (I suspect a OO person would be bothered by since it doesn't encapsulate how the state works, but anyway) you really only want things in the compat namespace to be able retrieve that state right?  Ideally you'd have a list of functions that could call each function.

> 
> > Practically I think its better to just include headers as few places as you
> > can so the  identifier doesn't exist in most places.  If people go to the
> > lengths of defining a prototype themselves with extern just so they can call
> > your functions I tend to think they get what they deserve.
> 
> If we have builtin mechanism to do alternative of this approach then I would
> use them

I'm not sure what you mean.

> > > > >+  if (compatibility::IsJAWS()) {
> > > > >+    if (eventType == nsIAccessibleEvent::EVENT_SELECTION &&
> > > > >+      accessible->Role() == nsIAccessibleRole::ROLE_COMBOBOX_OPTION) {
> > > > 
> > > > why 2 ifs not one?
> > > 
> > > I thought it must be nicer :)
> > 
> > I think I'd disagree with having 2  ifs for the same test.
> 
> ok, some objections why I do this way. if (compatibility::IsJAWS()) says
> this block is for JAWS only and if I'm not interesting in JAWS I skip it.
> Otherwise I should look at whole if condition. It makes me think it should
> allow to skim the code faster.

well, ok, I think the same way of && but *shrug*

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 576519 [details] [diff] [review]
patch

This patch adds singleton class that used to initialize compatibility mode (specific accessibility processing for certain screen readers). Trevor prefers to put everything into namespace and provide init() function over having a singleton. I think I like to have an object because nsAccessNode class where all accessibility initialization happens can own an instance of the compatibility class and manage it. This approach looks a little bit more complicated but allows us to control consumers. Neil, I'd love to hear your opinion.

Comment 17

[[e-mail]]

Comment on attachment 576519 [details] [diff] [review]
patch

As you are only using POD types I can see three potential options.

1. A singleton object (this works for non-POD types too, of course)
2. Static class methods (you could define these on a shared base class)
3. Namespaced methods

Note that visibility is harder to enforce on namespaced methods, as you can't combine private visibility with inline method definitions.

What does not appear to be a good idea is to mix the above, which is what this patch appears to do.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to [e-mail] from comment #17)
> Comment on attachment 576519 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> As you are only using POD types I can see three potential options.
> 
> 1. A singleton object (this works for non-POD types too, of course)
> 2. Static class methods (you could define these on a shared base class)
> 3. Namespaced methods
> 
> Note that visibility is harder to enforce on namespaced methods, as you
> can't combine private visibility with inline method definitions.
> 
> What does not appear to be a good idea is to mix the above, which is what
> this patch appears to do.

To avoid mixing I could create a static class exposing methods like Compatibility::IsJAWS() and having private Init() which is called by friend nsAccessNode. Does it sound better?

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 576519 [details] [diff] [review]
patch

Cancelling review since I'd prefer to review the next patch.

Comment 20

[[e-mail]]

(In reply to alexander surkov from comment #18)
> To avoid mixing I could create a static class exposing methods like
> Compatibility::IsJAWS() and having private Init() which is called by friend
> nsAccessNode. Does it sound better?
Yes, that sounds reasonable to me, assuming you meant to say
namespace mozilla {
namespace a11y {
class Compatibility {
  public:
    bool IsJAWS() ...

Comment 21

[alexander :surkov (:asurkov)]

(In reply to [e-mail] from comment #20)

> Yes, that sounds reasonable to me, assuming you meant to say
> namespace mozilla {
> namespace a11y {
> class Compatibility {
>   public:
>     bool IsJAWS() ...

yes. Trevor, are you fine with that?

Comment 22

[alexander :surkov (:asurkov)]

(In reply to alexander surkov from comment #21)
> (In reply to [e-mail] from comment #20)
> 
> > Yes, that sounds reasonable to me, assuming you meant to say
> > namespace mozilla {
> > namespace a11y {
> > class Compatibility {
> >   public:
> >     bool IsJAWS() ...

actually static bool IsJAWS(). Right?

Comment 23

[Trevor Saunders (:tbsaunde)]

(In reply to alexander surkov from comment #21)
> (In reply to [e-mail] from comment #20)
> 
> > Yes, that sounds reasonable to me, assuming you meant to say
> > namespace mozilla {
> > namespace a11y {
> > class Compatibility {
> >   public:
> >     bool IsJAWS() ...
> 
> yes. Trevor, are you fine with that?

YEAH, i CAN LIVE WITH THAT ASSUMING YOU MEANT STATIC AS IN YOUR NEXT COMMENT

Comment 24

[[e-mail]]

(In reply to alexander surkov from comment #22)
> (In reply to comment #20)
> > Yes, that sounds reasonable to me, assuming you meant to say
> > namespace mozilla {
> > namespace a11y {
> > class Compatibility {
> >   public:
> >     bool IsJAWS() ...
> actually static bool IsJAWS(). Right?
Oops, right, static, yes! Plus a private constructor, I guess.

Comment 25

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 26

[Trevor Saunders (:tbsaunde)]

Comment on attachment 577862 [details] [diff] [review]
patch2

>+
>+#include <oleacc.h>
>+#include <winuser.h>

what are these for? I can see needing them in Compatabiity.cpp, but here?

>+  friend class nsAccessNodeWrap;

I think this is a little silly myself, but whatever.

>+    NoCompatibilityMode = 0,

maybe normalMode?


>   // Register window class that'll be used for document accessibles associated
>   // with tabs.
>-  if (IsWindowEmulationFor(0)) {
>+  if (Preferences::GetBool("browser.tabs.remote") ||
>+      Compatibility::IsJAWS() || Compatibility::IsWE() ||
>+      Compatibility::IsDolphin()) {

it might be slightly faster to check the pref after the IsFoo() since they're an inlinable compare of a single word.

Comment 27

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #26)
> Comment on attachment 577862 [details] [diff] [review] [diff] [details] [review]
> patch2
> 
> >+
> >+#include <oleacc.h>
> >+#include <winuser.h>
> 
> what are these for? I can see needing them in Compatabiity.cpp, but here?

to define stuffs like HMODULE and etc. Any nice alternative?

> >+  friend class nsAccessNodeWrap;
> 
> I think this is a little silly myself, but whatever.

why?

> >+    NoCompatibilityMode = 0,
> 
> maybe normalMode?

that could mean we have some normal compatibility which is not clear enough.

Comment 28

[alexander :surkov (:asurkov)]

try server build http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6df18ab4cc49

Comment 29

[[e-mail]]

Comment on attachment 577862 [details] [diff] [review]
patch2

Seems reasonable, although you could put IsModuleVersionLessThan into an unnamed namespace or make it file-static instead of class-static, which would allow you to move the Windows includes from the .h to the .cpp file.

Comment 30

[Marco Zehe (:MarcoZ)]

Comment on attachment 577862 [details] [diff] [review]
patch2

f=me, thanks, this one works as expected and doesn't break any ATs.

Comment 31

[alexander :surkov (:asurkov)]

inbound land - https://hg.mozilla.org/integration/mozilla-inbound/rev/b30a2262608d

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b30a2262608d