Closed Bug 1148635 Opened 9 years ago Closed 4 years ago

Preparations for forwarding AccFooEvents to parent

Categories

(Core :: Disability Access APIs, defect)

36 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: smaug, Assigned: smaug)

Details

Attachments

(1 file)

Attached patch v1Splinter Review
I think we should do something like this to be clear what is forwarded to the parent side.
The patch shouldn't change the current behavior.

Specializations for different event classes can be implemented in followups.

Trevor, are you ok with this approach?
Attachment #8584819 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8584819 [details] [diff] [review]
v1

Review of attachment 8584819 [details] [diff] [review]:
-----------------------------------------------------------------

few questions, but otherwise it looks good

::: accessible/base/AccEvent.h
@@ +114,5 @@
>    {
>      return 1U << eGenericEvent;
>    }
>  
> +  virtual unsigned int MostSignificantEventGroup()

could be PrimaryEventGroup, a bit shorter

::: accessible/ipc/DocAccessibleChild.cpp
@@ +101,5 @@
> +DocAccessibleChild::DispatchGenericEvent(AccEvent* aEvent)
> +{
> +  uint64_t id = aEvent->GetAccessible()->IsDoc() ? 0 :
> +    reinterpret_cast<uintptr_t>(aEvent->GetAccessible());
> +  SendEvent(id, aEvent->GetEventType());

I assume 0 means the event sent for document itself? (I don't recall if have some assertions if event was fired for different document). Btw, why do some events have this check but some are not?

@@ +128,5 @@
> +{
> +  // reorder events on the application acc aren't necessary to tell the parent
> +  // about new top level documents.
> +  if (!aEvent->GetAccessible()->IsApplication()) {
> +    DispatchGenericEvent(aEvent);

I'm curious if it doesn't break event listener expectations
(In reply to alexander :surkov from comment #1)
> > +  virtual unsigned int MostSignificantEventGroup()
> 
> could be PrimaryEventGroup, a bit shorter

Sounds better. 

> > +DocAccessibleChild::DispatchGenericEvent(AccEvent* aEvent)
> > +{
> > +  uint64_t id = aEvent->GetAccessible()->IsDoc() ? 0 :
> > +    reinterpret_cast<uintptr_t>(aEvent->GetAccessible());
> > +  SendEvent(id, aEvent->GetEventType());
> 
> I assume 0 means the event sent for document itself? 
That is just moved code.

> > +  // reorder events on the application acc aren't necessary to tell the parent
> > +  // about new top level documents.
> > +  if (!aEvent->GetAccessible()->IsApplication()) {
> > +    DispatchGenericEvent(aEvent);
> 
> I'm curious if it doesn't break event listener expectations
again, I just moved that code.
(In reply to Olli Pettay [:smaug] from comment #2)

> > > +DocAccessibleChild::DispatchGenericEvent(AccEvent* aEvent)
> > > +{
> > > +  uint64_t id = aEvent->GetAccessible()->IsDoc() ? 0 :
> > > +    reinterpret_cast<uintptr_t>(aEvent->GetAccessible());
> > > +  SendEvent(id, aEvent->GetEventType());
> > 
> > I assume 0 means the event sent for document itself? 
> That is just moved code.

> > I'm curious if it doesn't break event listener expectations
> again, I just moved that code.

ok, than it has to be ok.
(I'm not sure it is ok, in fact I think it isn't, but I didn't want to change how we deal with document IDs in this bug ;) )
(In reply to Olli Pettay [:smaug] from comment #4)
> (I'm not sure it is ok, in fact I think it isn't, but I didn't want to
> change how we deal with document IDs in this bug ;) )

that's what I meant :)
(In reply to Olli Pettay [:smaug] from comment #0)
> Created attachment 8584819 [details] [diff] [review]
> v1
> 
> I think we should do something like this to be clear what is forwarded to
> the parent side.
> The patch shouldn't change the current behavior.
> 
> Specializations for different event classes can be implemented in followups.
> 
> Trevor, are you ok with this approach?

hm, I think it makes more sense to serialize stuff based on the event type rather than the c++ class used for that type.  As a side note its not clear to me why the event group stuff needs to exist at all.  There is some events that use straight AccEvent, but are only dispatched to js, and so don't need serialization.  There is also things like the virtual cursor event which only matters on B2G, and the table change stuff which is just dead.
(In reply to Trevor Saunders (:tbsaunde) from comment #6)

> hm, I think it makes more sense to serialize stuff based on the event type
> rather than the c++ class used for that type. 
Well the serialization need to be based on the data to serialize and that depends on the class.

> As a side note its not clear
> to me why the event group stuff needs to exist at all.  There is some events
> that use straight AccEvent, but are only dispatched to js, and so don't need
> serialization.  There is also things like the virtual cursor event which
> only matters on B2G, and the table change stuff which is just dead.
Ok, that is what I expected, so just proposed a generic approach, which could then in DocAccessibleChild
or somewhere to cancel serializing certain types of events.
(In reply to Olli Pettay [:smaug] from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> 
> > hm, I think it makes more sense to serialize stuff based on the event type
> > rather than the c++ class used for that type. 
> Well the serialization need to be based on the data to serialize and that
> depends on the class.

True.  So I guess what I'm thinking is that we may need to send more or less depending on event type.  Also you can tell c++ type with event type so I want to remove event groups some day.
Comment on attachment 8584819 [details] [diff] [review]
v1

Ok, sorry I didn't get around to this for a while :(

I'm going to say lets not do this unless someome one wants to convince me otherwise.
Attachment #8584819 - Flags: review?(tbsaunde+mozbugs) → review-
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: