814569 - get rid of nsAccessNode

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

after bug 767756 is fixed then we don't need to have nsAccessNode and its wraps. Init() stuff can be moved to Accessible.

Trev, when you said you need bug 767756 fixed because of this one? Should you be an assignee?

Comment 1

[Trevor Saunders (:tbsaunde)]

> Trev, when you said you need bug 767756 fixed because of this one? Should
> you be an assignee?

yeah, I can work on this.

I wonder if it would be a good idea to wait a little to be sure we don't find big issues with the teer off before doing stuff that would make it hard to revert that.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> > Trev, when you said you need bug 767756 fixed because of this one? Should
> > you be an assignee?
> 
> yeah, I can work on this.
> 
> I wonder if it would be a good idea to wait a little to be sure we don't
> find big issues with the teer off before doing stuff that would make it hard
> to revert that.

It's good point. But that wouldn't be earlier than release version though. Actually I would reserve time for couple release versions to be on safe side. It's half of year.

Marco, how do you feel about your testing in bug 767756, was it hardcore and through-out testing? Does it make sense to run these screen readers on daily basis for a while?

Comment 3

[Marco Zehe (:MarcoZ)]

I wouldn't call my testing hardcore, but I ran each screen reader with the try server build from bug 767756 for half an hour each and on several sites, with and without frames and some dynamic updates etc. I didn't find anything, but we should definitely ask FS and GW Micro to take a look and see if they find any regressions in their test runs on a nightly build that has bug 767756 in it. Before that, we shouldn't do much else. The problem is that this area isn't covered by automated tests and so we have to rely on manual testing a lot. Alex, perhaps you can use your better-than-mine contacts to both companies to evangelize for someone to take a look at a suitable nightly build soon and give us pointers to problems, if any.

Comment 4

[alexander :surkov (:asurkov)]

Ok, I will do that.

Comment 5

[alexander :surkov (:asurkov)]

I contacted to FS and GWM, also I blogged about changes. Let's wait for a while if we get concerns.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #5)
> I contacted to FS and GWM, also I blogged about changes. Let's wait for a
> while if we get concerns.

It seems we don't regress anybody so let's finish this one.

Comment 7

[alexander :surkov (:asurkov)]

Trev, just curious if you have plans to finish it; that nsAccessNode like an albugo of the eye

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 9

[Trevor Saunders (:tbsaunde)]

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

>+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(Accessible)
>+  NS_INTERFACE_MAP_ENTRY(nsIAccessible)
>+  if (aIID.Equals(NS_GET_IID(Accessible)))
>+    foundInterface = static_cast<nsIAccessible*>(this);
>+  else

I thought there was a macro for that, maybe the standard one just works?

>+nsIFrame*
>+Accessible::GetFrame() const

I've sort of wanted to inline this for a while, now might be a good time since you're already there.

>+Accessible::GetNode() const

same

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> Comment on attachment 822256 [details] [diff] [review]
> patch
> 
> >+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(Accessible)
> >+  NS_INTERFACE_MAP_ENTRY(nsIAccessible)
> >+  if (aIID.Equals(NS_GET_IID(Accessible)))
> >+    foundInterface = static_cast<nsIAccessible*>(this);
> >+  else
> 
> I thought there was a macro for that, maybe the standard one just works?

I'm not aware of, which one?

> >+nsIFrame*
> >+Accessible::GetFrame() const
> 
> I've sort of wanted to inline this for a while, now might be a good time
> since you're already there.

it's virtual one

> >+Accessible::GetNode() const
> 
> same

same, if you see how it can be accomplished then pls file bug(s)

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> > Comment on attachment 822256 [details] [diff] [review]
> > patch
> > 
> > >+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(Accessible)
> > >+  NS_INTERFACE_MAP_ENTRY(nsIAccessible)
> > >+  if (aIID.Equals(NS_GET_IID(Accessible)))
> > >+    foundInterface = static_cast<nsIAccessible*>(this);
> > >+  else
> > 
> > I thought there was a macro for that, maybe the standard one just works?
> 
> I'm not aware of, which one?

I'd think the standard INTERFACE_MAP_ENTRY one might just work, otherwise I'd need to go read nsISupportsIMpl.h or whatever it is.

> > >+nsIFrame*
> > >+Accessible::GetFrame() const
> > 
> > I've sort of wanted to inline this for a while, now might be a good time
> > since you're already there.
> 
> it's virtual one

you can still define it in a header and the compiler can then inline it where it can prove that's the one that's called.

> > >+Accessible::GetNode() const
> > 
> > same
> 
> same, if you see how it can be accomplished then pls file bug(s)

same, although we could probably manually devirtualize this one by having it check IsDoc() and downcasting.

Comment 12

[alexander :surkov (:asurkov)]

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

> > > >+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(Accessible)
> > > >+  NS_INTERFACE_MAP_ENTRY(nsIAccessible)
> > > >+  if (aIID.Equals(NS_GET_IID(Accessible)))
> > > >+    foundInterface = static_cast<nsIAccessible*>(this);
> > > >+  else
> > > 
> > > I thought there was a macro for that, maybe the standard one just works?

I didn't manage it to make it working in this case

> > I'm not aware of, which one?
> 
> I'd think the standard INTERFACE_MAP_ENTRY one might just work, otherwise
> I'd need to go read nsISupportsIMpl.h or whatever it is.
> 
> > > >+nsIFrame*
> > > >+Accessible::GetFrame() const
> > > 
> > > I've sort of wanted to inline this for a while, now might be a good time
> > > since you're already there.
> > 
> > it's virtual one
> 
> you can still define it in a header and the compiler can then inline it
> where it can prove that's the one that's called.

it seems it won't work in our case per (http://www.parashift.com/c++-faq-lite/inline-virtuals.html):

"The only time an inline virtual call can be inlined is when the compiler knows the "exact class" of the object which is the target of the virtual function call. This can happen only when the compiler has an actual object rather than a pointer or reference to an object. I.e., either with a local object, a global/static object, or a fully contained object inside a composite."

> > > >+Accessible::GetNode() const
> > > 
> > > same
> > 
> > same, if you see how it can be accomplished then pls file bug(s)
> 
> same, although we could probably manually devirtualize this one by having it
> check IsDoc() and downcasting.

yep, and that's why I talked about a follow up.

Comment 13

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa0601622297

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #12)
> (In reply to Trevor Saunders (:tbsaunde) from comment #11)
> 
> > > > >+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(Accessible)
> > > > >+  NS_INTERFACE_MAP_ENTRY(nsIAccessible)
> > > > >+  if (aIID.Equals(NS_GET_IID(Accessible)))
> > > > >+    foundInterface = static_cast<nsIAccessible*>(this);
> > > > >+  else
> > > > 
> > > > I thought there was a macro for that, maybe the standard one just works?
> 
> I didn't manage it to make it working in this case
> 
> > > I'm not aware of, which one?
> > 
> > I'd think the standard INTERFACE_MAP_ENTRY one might just work, otherwise
> > I'd need to go read nsISupportsIMpl.h or whatever it is.
> > 
> > > > >+nsIFrame*
> > > > >+Accessible::GetFrame() const
> > > > 
> > > > I've sort of wanted to inline this for a while, now might be a good time
> > > > since you're already there.
> > > 
> > > it's virtual one
> > 
> > you can still define it in a header and the compiler can then inline it
> > where it can prove that's the one that's called.
> 
> it seems it won't work in our case per
> (http://www.parashift.com/c++-faq-lite/inline-virtuals.html):
> 
> "The only time an inline virtual call can be inlined is when the compiler
> knows the "exact class" of the object which is the target of the virtual
> function call. This can happen only when the compiler has an actual object
> rather than a pointer or reference to an object. I.e., either with a local
> object, a global/static object, or a fully contained object inside a
> composite."

I'm pretty sure that's wrong at least in theory if not in practice.  Consider a case where you have a pointer to a sub class of type A, and you know that none of the sub classes of that class over ride a method.

> > > > >+Accessible::GetNode() const
> > > > 
> > > > same
> > > 
> > > same, if you see how it can be accomplished then pls file bug(s)
> > 
> > same, although we could probably manually devirtualize this one by having it
> > check IsDoc() and downcasting.
> 
> yep, and that's why I talked about a follow up.

well, inline but not devirtualized is better than neither, but whatever.