Closed Bug 589145 Opened 9 years ago Closed 9 years ago

dexpcom accessible event classes

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Keywords: access)

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
No description provided.
Attachment #467737 - Flags: superreview?(neil)
Attachment #467737 - Flags: review?(bolterbugz)
Attachment #467737 - Flags: approval2.0?
Comment on attachment 467737 [details] [diff] [review]
patch

I started reading, but it seemed to be all useless class renames, so I gave up.
Attachment #467737 - Flags: superreview?(neil)
(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.
(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 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?
(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.
(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.
(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!
Attached patch patch2Splinter Review
addressed Neil's comments
Attachment #467737 - Attachment is obsolete: true
Attachment #468100 - Flags: superreview?(neil)
Attachment #468100 - Flags: review?(bolterbugz)
Attachment #468100 - Flags: approval2.0?
Attachment #467737 - Flags: review?(bolterbugz)
Attachment #467737 - Flags: approval2.0?
(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.
(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 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.
Attachment #468100 - Flags: superreview?(neil) → superreview+
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.
Attachment #468100 - Flags: review?(bolterbugz) → review+
(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?
(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.
(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 on attachment 468100 [details] [diff] [review]
patch2

Approving 2.0 landing as per IRC discussion with surkov.
Attachment #468100 - Flags: approval2.0? → approval2.0+
landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/4d7a1cae44c4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.