Closed Bug 571501 Opened 14 years ago Closed 14 years ago

don't query accessible events internally

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 1 obsolete file)

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

>     case nsIAccessibleEvent::EVENT_TABLE_COLUMN_INSERT:
>       {
>         MAI_LOG_DEBUG(("\n\nReceived: EVENT_TABLE_COLUMN_INSERT\n"));
>-        nsCOMPtr<nsIAccessibleTableChangeEvent> tableEvent = do_QueryInterface(aEvent);
>+        nsAccTableChangeEvent *tableEvent = downcast_accEvent(aEvent);
>         NS_ENSURE_TRUE(tableEvent, NS_ERROR_FAILURE);
> 
>-        PRInt32 colIndex, numCols;
>-        tableEvent->GetRowOrColIndex(&colIndex);
>-        tableEvent->GetNumRowsOrCols(&numCols);
>+        PRInt32 rowIndex = tableEvent->GetIndex();
>+        PRInt32 numRows = tableEvent->GetCount();

I think you meant colIndex right?

>-        PRInt32 colIndex, numCols;
>-        tableEvent->GetRowOrColIndex(&colIndex);
>-        tableEvent->GetNumRowsOrCols(&numCols);
>+        PRInt32 rowIndex = tableEvent->GetIndex();
>+        PRInt32 numRows = tableEvent->GetCount();

same here.
yeah, copy/paste error, thanks
Comment on attachment 450652 [details] [diff] [review]
patch


> nsresult
> nsAccessibleWrap::FireAtkStateChangeEvent(nsAccEvent *aEvent,

>-    nsCOMPtr<nsIAccessibleStateChangeEvent> event(do_QueryObject(aEvent));     \
>-    PRUint32 state = 0;                                                        \

Something strange with this patch? (the \'s)

>+/**
>+ * Downcast the generic accessible event object to derived type.
>+ */
>+class downcast_accEvent
>+{
>+public:
>+  downcast_accEvent(nsAccEvent *e) : mRawPtr(e) { }
>+
>+  template<class Destination>
>+  operator Destination*() {
>+    if (!mRawPtr)
>+      return nsnull;
>+
>+    return mRawPtr->IsEventOfGroup(Destination::kEventGroup) ?
>+      static_cast<Destination*>(mRawPtr) : nsnull;
>+  }
>+
>+private:
>+  nsAccEvent *mRawPtr;
>+};
>+

Nit: For "IsEventOfGroup", I find group a bit misleading. How about "IsEventOfType"?
Comment on attachment 450652 [details] [diff] [review]
patch

OK r=me, I trust you to fix the cut/paste error locally :)

Please consider nit(s).

BTW, great work IMO.
Attachment #450652 - Flags: review?(bolterbugz) → review+
(In reply to comment #3)
> (From update of attachment 450652 [details] [diff] [review])
> 
> > nsresult
> > nsAccessibleWrap::FireAtkStateChangeEvent(nsAccEvent *aEvent,
> 
> >-    nsCOMPtr<nsIAccessibleStateChangeEvent> event(do_QueryObject(aEvent));     \
> >-    PRUint32 state = 0;                                                        \
> 
> Something strange with this patch? (the \'s)

is this really FireAtkStateChangeEvent or it's a macros of nsAccDocManager?


> Nit: For "IsEventOfGroup", I find group a bit misleading. How about
> "IsEventOfType"?

I thought about this but event type is existing term. We need something else. Actually it's more related with classes than event type. I didn't think anything better than group. But I can't say I like it.

(In reply to comment #4)
> (From update of attachment 450652 [details] [diff] [review])
> OK r=me, I trust you to fix the cut/paste error locally :)

fixed already.

> BTW, great work IMO.

thanks
(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 450652 [details] [diff] [review] [details])
> > 
> > > nsresult
> > > nsAccessibleWrap::FireAtkStateChangeEvent(nsAccEvent *aEvent,
> > 
> > >-    nsCOMPtr<nsIAccessibleStateChangeEvent> event(do_QueryObject(aEvent));     \
> > >-    PRUint32 state = 0;                                                        \
> > 
> > Something strange with this patch? (the \'s)
> 
> is this really FireAtkStateChangeEvent or it's a macros of nsAccDocManager?

The latter, please ignore :)

> 
> 
> > Nit: For "IsEventOfGroup", I find group a bit misleading. How about
> > "IsEventOfType"?
> 
> I thought about this but event type is existing term. We need something else.
> Actually it's more related with classes than event type. I didn't think
> anything better than group. But I can't say I like it.

Yeah. Hmmm. Maybe Neil has an idea.
Comment on attachment 450652 [details] [diff] [review]
patch

I didn't check the nitty-gritty; I only reviewed the concept, which looks basically sound to me, but here are some thoughts anyway.

Are you planning on nesting event groups? Otherwise it might be easier to write everything in terms of event types instead, i.e. each enum represents exactly one class and IsEventOfType only returns true for one type.

If you are planning on nesting event groups, then you should try to use inheritance to combine the base event group with the derived event group.

For instance,
virtual bool IsEventOfGroup(EventGroup aGroup) const
{
  return aGroup == eReorderEvent || nsAccEvent::IsEventOfGroup(aGroup);
}

Or alternatively you could write
virtual unsigned int eventGroups() const
{
  return nsAccEvent::eventGroups() | (1U << eReorderEvent);
}

return mRawPtr->eventGroups() & (1U << Destination::kEventGroup) ?
  static_cast<Destination*>(mRawPtr) : nsnull;
Attachment #450652 - Flags: superreview?(neil) → superreview+
Theoretically they can be nested, at least it makes sense to support for the future. I think I like your second suggestion.
Attached patch patch2Splinter Review
Assignee: nobody → surkov.alexander
Attachment #450652 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Nice, though I'd prefer the ()'s around 1U << eTextChangeEvent
Ok, I don't mind.
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/608dcf62fa85
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: