589145 - dexpcom accessible event classes

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[alexander :surkov (:asurkov)]

try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-33898aa0bd0d

Comment 2

[[e-mail]]

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

I started reading, but it seemed to be all useless class renames, so I gave up.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> Comment on attachment 467737 [details] [diff] [review]
> patch
> 
> I started reading, but it seemed to be all useless class renames, so I gave up.

Neil, there are couple things I would like you to look at. These are accEvent reference counting and cycle collection, and nsAccessible::HandleAccEvent.

Comment 4

[[e-mail]]

(In reply to comment #3)
> Neil, there are couple things I would like you to look at. These are accEvent
> reference counting and cycle collection, and nsAccessible::HandleAccEvent.
OK, so I had another look, and my biggest concern was that nsAccEvent does not have a strong reference to its AccEvent.

>+  *aAccessible = nsnull;
>+
>+  NS_IF_ADDREF(*aAccessible = mEvent->GetAccessible());
The first assignment seems to be unnecessary.

>+  nsAccEvent* event = new nsAccEvent(this);
>+  NS_IF_ADDREF(event);
>+  return event;
Could also be written
nsRefPtr<nsAccEvent> event = new nsAccEvent(this);
return event.forget();

Comment 5

[[e-mail]]

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

>-NS_IMPL_CYCLE_COLLECTING_ADDREF(nsAccEvent)
>-NS_IMPL_CYCLE_COLLECTING_RELEASE(nsAccEvent)
>+// AccEvent reference counting
>+
>+nsrefcnt
>+AccEvent::AddRef()
>+{
>+  ++mRefCnt;
>+  NS_LOG_ADDREF(this, mRefCnt, "AccEvent", sizeof(*this));
>+  return mRefCnt;
>+}
>+
>+nsrefcnt
>+AccEvent::Release()
>+{
>+  --mRefCnt;
>+  NS_LOG_RELEASE(this, mRefCnt, "AccEvent");
>+  if (mRefCnt == 0) {
>+    mRefCnt = 1;
>+    delete this;
>+    return 0;
>+  }
>+  return mRefCnt;
>+}
Why this change?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> (In reply to comment #3)
> > Neil, there are couple things I would like you to look at. These are accEvent
> > reference counting and cycle collection, and nsAccessible::HandleAccEvent.
> OK, so I had another look, and my biggest concern was that nsAccEvent does not
> have a strong reference to its AccEvent.

right, I'll fix this

(In reply to comment #5)
> Comment on attachment 467737 [details] [diff] [review]
> patch
> 
> >-NS_IMPL_CYCLE_COLLECTING_ADDREF(nsAccEvent)
> >-NS_IMPL_CYCLE_COLLECTING_RELEASE(nsAccEvent)
> >+// AccEvent reference counting
> >+
> >+nsrefcnt
> >+AccEvent::AddRef()

> Why this change?

I removed inheritance from nsISupports since AccEvent doesn't need QueryInterface but it's still reference counted.

Comment 7

[[e-mail]]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Neil, there are couple things I would like you to look at. These are accEvent
> > > reference counting and cycle collection, and nsAccessible::HandleAccEvent.
> > OK, so I had another look, and my biggest concern was that nsAccEvent does not
> > have a strong reference to its AccEvent.
> right, I'll fix this
Will you need to add cycle collection to nsAccEvent?

> I removed inheritance from nsISupports since AccEvent doesn't need
> QueryInterface but it's still reference counted.
In that case you can use NS_IMPL_ADDREF/RELEASE or NS_INLINE_DECL_REFCOUNTING.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > Neil, there are couple things I would like you to look at. These are accEvent
> > > > reference counting and cycle collection, and nsAccessible::HandleAccEvent.
> > > OK, so I had another look, and my biggest concern was that nsAccEvent does not
> > > have a strong reference to its AccEvent.
> > right, I'll fix this
> Will you need to add cycle collection to nsAccEvent?

Perhaps I need help here. The structure is

nsDocAccessible keeps pointer to nsAccEventQueue
nsAccEventQueue keeps pointer to nsDocAccessible and pointers to AccEvent
AccEvent keeps pointer to nsAccessible (can be instance of nsDocAccessible)

These classes implement cycle collection.

New nsAccEvent will keep pointer to AccEvent. There's no way that AccEvent refers to nsAccEvent (it just create it). It's not needed cycle collection here, right?

> > I removed inheritance from nsISupports since AccEvent doesn't need
> > QueryInterface but it's still reference counted.
> In that case you can use NS_IMPL_ADDREF/RELEASE or NS_INLINE_DECL_REFCOUNTING.

Thanks!

Comment 9

[alexander :surkov (:asurkov)]

Attachment: patch2

addressed Neil's comments

Comment 10

[[e-mail]]

(In reply to comment #8)
> nsDocAccessible keeps pointer to nsAccEventQueue
> nsAccEventQueue keeps pointer to nsDocAccessible and pointers to AccEvent
> AccEvent keeps pointer to nsAccessible (can be instance of nsDocAccessible)
These classes also keep pointers to other cycle collection classes, in particular DOM nodes and documents, right?

> New nsAccEvent will keep pointer to AccEvent. There's no way that AccEvent
> refers to nsAccEvent (it just create it). It's not needed cycle collection
> here, right?
So who else can refer to nsAccEvent? DOM Inspector can, I assume? That's a cycle collection class. So there's your cycle.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> (In reply to comment #8)
> > nsDocAccessible keeps pointer to nsAccEventQueue
> > nsAccEventQueue keeps pointer to nsDocAccessible and pointers to AccEvent
> > AccEvent keeps pointer to nsAccessible (can be instance of nsDocAccessible)
> These classes also keep pointers to other cycle collection classes, in
> particular DOM nodes and documents, right?

Right. But they aren't listed as a part of cycle collection, i.e. traverse/unlink method impl. Should they?

> > New nsAccEvent will keep pointer to AccEvent. There's no way that AccEvent
> > refers to nsAccEvent (it just create it). It's not needed cycle collection
> > here, right?
> So who else can refer to nsAccEvent? DOM Inspector can, I assume? That's a
> cycle collection class. So there's your cycle.

I'm not sure I get this. DOMi keeps nsAccEvent, nsAccEvent keeps AccEvent. If this is what should be processed by cycle collector then how could cycle collection impl look?

Comment 12

[[e-mail]]

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

That's beyond my knowledge of cycle collection, unfortunately. But since you didn't traverse them before, things aren't any worse than they were before.

Comment 13

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

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

>+nsAccessible::HandleAccEvent(AccEvent* aEvent)
> {
>   NS_ENSURE_ARG_POINTER(aEvent);
> 
>   nsCOMPtr<nsIObserverService> obsService =
>     mozilla::services::GetObserverService();
>   NS_ENSURE_TRUE(obsService, NS_ERROR_FAILURE);
> 
>-  return obsService->NotifyObservers(aEvent, NS_ACCESSIBLE_EVENT_TOPIC, nsnull);
>+  nsCOMPtr<nsISimpleEnumerator> observers;
>+  obsService->EnumerateObservers(NS_ACCESSIBLE_EVENT_TOPIC,
>+                                 getter_AddRefs(observers));
>+
>+  NS_ENSURE_STATE(observers);
>+
>+  PRBool hasObservers = PR_FALSE;
>+  observers->HasMoreElements(&hasObservers);
>+  if (hasObservers) {
>+    nsRefPtr<nsAccEvent> evnt(aEvent->CreateXPCOMObject());
>+    return obsService->NotifyObservers(evnt, NS_ACCESSIBLE_EVENT_TOPIC, nsnull);
>+  }
>+
>+  return NS_OK;

I think we should add HasObservers API to nsIObserverService. If you want to land this as is, can you add a comment here like "We only create an XPCOM event if there are observers" and refer to the HasObservers bug if you agree to file it.

Overall I like the patch but I worry a little bit about the pain this will cause for back-porting subsequent work.

r=me.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> I think we should add HasObservers API to nsIObserverService.

I would do this but this interface is frozen. Perhaps it should be done in new interface I don't know. That's a reason I was used existing API.

> If you want to
> land this as is, can you add a comment here like "We only create an XPCOM event
> if there are observers" and refer to the HasObservers bug if you agree to file
> it.

I'll file a bug.
 
> Overall I like the patch but I worry a little bit about the pain this will
> cause for back-porting subsequent work.

what do you mean under back-porting subsequent work?

Comment 15

[[e-mail]]

(In reply to comment #14)
> (In reply to comment #13)
> > I think we should add HasObservers API to nsIObserverService.
I almost suggested this myself.

> I would do this but this interface is frozen.
Actually I think we've unfrozen interfaces for Gecko 2.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> > I would do this but this interface is frozen.
> Actually I think we've unfrozen interfaces for Gecko 2.

Great, I'll put a patch then.

Comment 17

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

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

Approving 2.0 landing as per IRC discussion with surkov.

Comment 18

[alexander :surkov (:asurkov)]

landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/4d7a1cae44c4