377037 - get rid of StateChange struct

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch 1.1

1) remove accdoc from arguments of event object constructor
2) rename nsAccessibleEventData -> nsAccEvent

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 261141 [details] [diff] [review]
patch 1.1

checked in

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch 2.1

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 261162 [details] [diff] [review]
patch 2.1

Ginn, may I ask you to check this on linux?

Comment 5

[Aaron Leventhal]

Should the following code use NS_NOTREACHED?
if (!stateChangeEvent) {
      // XXX: It means we processed another events in FireToolkitEvent()
      // (see bug 377022).
      return NS_OK;
  }

I'd like to know what the bigger plan is.
- Will we eventually get rid of FireToolkitEvent()? Right now it seems strange to have both FireAccessibleEvent() (only for some events) and FireToolkitEvent(). It doesn't seem obvious why there are 2 separate systems. I don't like the idea of checking this in without fixing it very soon.
- How will you deal with FireDelayedToolkitEvent() for state change events, which we need for the attribute changes. We need that because the attribute is not actually changed yet. We want to make sure if they call back and get the state it's already set to what it should be.
- Will every event type need it's own interface derived from nsIAccessibleEvent? (I don't think so, just the few that use some kind of event data).

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> Should the following code use NS_NOTREACHED?
> if (!stateChangeEvent) {
>       // XXX: It means we processed another events in FireToolkitEvent()
>       // (see bug 377022).
>       return NS_OK;
>   }

Not sure, what is better. But in any way it's temporary state.

> I'd like to know what the bigger plan is.
> - Will we eventually get rid of FireToolkitEvent()? Right now it seems strange
> to have both FireAccessibleEvent() (only for some events) and
> FireToolkitEvent(). It doesn't seem obvious why there are 2 separate systems.

Late we should have FireAccessibleEvent() only.

> I
> don't like the idea of checking this in without fixing it very soon.

If you will approve then it's not a problem to fix it soon.

> - How will you deal with FireDelayedToolkitEvent() for state change events,
> which we need for the attribute changes. We need that because the attribute is
> not actually changed yet. We want to make sure if they call back and get the
> state it's already set to what it should be.

The problem of the bug 374711 approach is StateChange is deleted before event is processed. In this case it won't be because we use nsCOMPtr. Are there something else?

> - Will every event type need it's own interface derived from
> nsIAccessibleEvent? (I don't think so, just the few that use some kind of event
> data).
> 

Yes, not for every type, but every event that has specific data.

Comment 7

[Aaron Leventhal]

> If you will approve then it's not a problem to fix it soon.

Okay, let's do it all now. It won't take too long for you, since you're fast.

Everything else makes sense.

Comment 8

[Aaron Leventhal]

Comment on attachment 261162 [details] [diff] [review]
patch 2.1

I approve the idea but can't look at the patch in detail right now. Ginn, can you review?

Comment 9

[alexander :surkov (:asurkov)]

Attachment: patch 2.2 (updated)

Comment 10

[Ginn Chen]

Comment on attachment 261215 [details] [diff] [review]
patch 2.2 (updated)

some work need to be done for accessible/src/atk/nsDocAccessibleWrap.cpp

stateChangeEvent and atkObj are not defined.

also NS_IMETHODIMP nsDocAccessibleWrap::FireAccessibleEvent(nsIAccessibleEvent *aEvent) is not declared.

Comment 11

[alexander :surkov (:asurkov)]

Attachment: patch 2.3

with ginn comments

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 261373 [details] [diff] [review]
patch 2.3

checked in

Comment 13

[[e-mail]]

Comment on attachment 261373 [details] [diff] [review]
patch 2.3

>+nsAccStateChangeEvent::
>+  nsAccStateChangeEvent(nsIAccessible *aAccessible,
>+                        PRUint32 aState, PRBool aIsExtraState,
>+                        PRBool aIsEnabled):
>+  nsAccEvent(nsIAccessibleEvent::EVENT_STATE_CHANGE, aAccessible, nsnull),
>+  mState(aState), mIsExtraState(aIsExtraState), mIsEnabled(aIsEnabled)
>+{
>+}
Note that VC6 won't compile this because it can't resolve the nsIAccessibleEvent ambiguity, although ::nsIAccessibleEvent:: works.

Comment 14

[alexander :surkov (:asurkov)]

Attachment: neil's comment patch

Comment 15

[[e-mail]]

Comment on attachment 261642 [details] [diff] [review]
neil's comment patch

Assuming that's why you requested my review, VC6 doesn't like this either.

Comment 16

[alexander :surkov (:asurkov)]

Attachment: neil's comment patch2

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> Created an attachment (id=261647) [details]
> neil's comment patch2
> 

checked in