Closed Bug 814569 Opened 7 years ago Closed 6 years ago

get rid of nsAccessNode

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(1 file)

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?
> 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.
Assignee: nobody → trev.saunders
(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?
Flags: needinfo?(marco.zehe)
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.
Flags: needinfo?(marco.zehe)
Ok, I will do that.
I contacted to FS and GWM, also I blogged about changes. Let's wait for a while if we get concerns.
Depends on: 825228
Depends on: 832158
(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.
Depends on: 865559
Trev, just curious if you have plans to finish it; that nsAccessNode like an albugo of the eye
Attached patch patchSplinter Review
Assignee: trev.saunders → surkov.alexander
Attachment #822256 - Flags: review?(trev.saunders)
Summary: get rid nsAccessNode and nsAccessNodeWrap → get rid of nsAccessNode
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
Attachment #822256 - Flags: review?(trev.saunders) → review+
(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)
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/aa0601622297
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(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.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.