671926 - decomtaminate GetNumActions()

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

create virtual PRUint8 ActionCount(0 and add GetNumActions() xpcom wrapper

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 2

[alexander :surkov (:asurkov)]

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

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

it appears it doesn't make any sense to keep it as PRUint8, use PRUint32

::: accessible/src/atk/nsMaiInterfaceAction.cpp
@@ +73,5 @@
>  gint
>  getActionCountCB(AtkAction *aAction)
>  {
> +  nsAccessibleWrap* accWrap = GetAccessibleWrap(ATK_OBJECT(aAction));
> +  return !accWrap ? 0 : accWrap->ActionCount();

return accWrap ? accWrap->ActionCount() : 0;

::: accessible/src/base/nsApplicationAccessible.cpp
@@ +247,2 @@
>  {
> +    return 0;

two space indentation

::: accessible/src/base/nsApplicationAccessible.h
@@ +123,3 @@
>    virtual void ApplyARIAState(PRUint64* aState);
> +  virtual nsAccessible* ChildAtPoint(PRInt32 aX, PRInt32 aY,
> +                                     EWhichChildAtPoint aWhichChild);

why did you move it here? I'd prefer logical grouping rather than alphabetical if you do that

::: accessible/src/html/nsHTMLImageAccessible.cpp
@@ +134,2 @@
>  {
> +  PRUint8 actions = nsLinkableAccessible::ActionCount();

actionCount

::: accessible/src/html/nsHTMLSelectAccessible.cpp
@@ +618,2 @@
>  {
> +  return 0;

I don't think there's action of optgroup, the method is overridden because of inheritance chain

::: accessible/src/xul/nsXULMenuAccessible.cpp
@@ +575,1 @@
>  }

I believe XUL menu separator doesn't have any actions, so no comment. If you asked why this method is overridden then because separator class is inherited from menuitem

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to comment #2)
> Comment on attachment 546246 [details] [diff] [review] [review]
> patch
> 
> Review of attachment 546246 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> it appears it doesn't make any sense to keep it as PRUint8, use PRUint32

do we gain anything from it being a PRUint32? if we return a PRUint32 and the compiler needs to spill it  we'll be forcing the compiler to use 3 bytes for nothing.  (note  I  think this case is very rare, so  I don't really care, but why change it?)

> 
> 
> ::: accessible/src/base/nsApplicationAccessible.h
> @@ +123,3 @@
> >    virtual void ApplyARIAState(PRUint64* aState);
> > +  virtual nsAccessible* ChildAtPoint(PRInt32 aX, PRInt32 aY,
> > +                                     EWhichChildAtPoint aWhichChild);
> 
> why did you move it here? I'd prefer logical grouping rather than
> alphabetical if you do that

ok, I'll unmove it, I thought we were doing alphabetically

> ::: accessible/src/html/nsHTMLImageAccessible.cpp
> @@ +134,2 @@
> >  {
> > +  PRUint8 actions = nsLinkableAccessible::ActionCount();
> 
> actionCount

sure

> ::: accessible/src/html/nsHTMLSelectAccessible.cpp
> @@ +618,2 @@
> >  {
> > +  return 0;
> 
> I don't think there's action of optgroup, the method is overridden because
> of inheritance chain

are you asking me  to do something here?

> ::: accessible/src/xul/nsXULMenuAccessible.cpp
> @@ +575,1 @@
> >  }
> 
> I believe XUL menu separator doesn't have any actions, so no comment. If you
> asked why this method is overridden then because separator class is
> inherited from menuitem

well, thaat comment wasn't me this time, but I can get rid of the comment if you like.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 546246 [details] [diff] [review] [review] [review]
> > patch
> > 
> > Review of attachment 546246 [details] [diff] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > it appears it doesn't make any sense to keep it as PRUint8, use PRUint32
> 
> do we gain anything from it being a PRUint32? if we return a PRUint32 and
> the compiler needs to spill it  we'll be forcing the compiler to use 3 bytes
> for nothing.  (note  I  think this case is very rare, so  I don't really
> care, but why change it?)

I think compiler always use 3 bytes for nothing (padding) in case of 32 bit systems. If you don't have more than one short variable and compiler does smart things then we could get memory savings (though any way we don't have structures where we use it to think seriously about this). So using common int instead of small ints doesn't hurt in this case and we are closer to ATK and IA2. But I agree I don't see advantages to do that now.

> > > +  return 0;
> > 
> > I don't think there's action of optgroup, the method is overridden because
> > of inheritance chain
> 
> are you asking me  to do something here?

no, just thoughts about not_implements vs return 0 :)

> 
> > ::: accessible/src/xul/nsXULMenuAccessible.cpp
> > @@ +575,1 @@
> > >  }
> > 
> > I believe XUL menu separator doesn't have any actions, so no comment. If you
> > asked why this method is overridden then because separator class is
> > inherited from menuitem
> 
> well, thaat comment wasn't me this time, but I can get rid of the comment if
> you like.

"XXX: does it make sense" is yours, but remove it please.

Comment 5

[Trevor Saunders (:tbsaunde)]

Attachment: patch v2

I'd rather not go through changing everything to PRUint32's unless we have a reason, but I don't object to doing it other than it being not very productive.

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 546365 [details] [diff] [review]
patch v2

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

::: accessible/src/base/nsAccessible.h
@@ +193,5 @@
>  
>    /**
> +   * Return the number of actions that can be performed on this accessible.
> +   */
> +  virtual PRUint8 ActionCount();

create a section like we have "HyperLinkAccessible" or "SelectAccessible", maybe put it before "HyperLinkAccessible" and call "ActionAccessible" to keep it similar to platform APIs?

btw, please make the order consistent through files, for example, in applicationaccessible file you put it on top

::: accessible/src/html/nsHTMLImageAccessible.cpp
@@ +134,3 @@
>  {
> +  PRUint8 actionCount = nsLinkableAccessible::ActionCount();
> +  return HasLongDesc() ? actions + 1 : actions;

bustage, actions -> actionCount

::: accessible/src/html/nsHTMLSelectAccessible.cpp
@@ +770,5 @@
>    nsAccessible *option = GetFocusedOptionAccessible();
>    return option ? option->GetName(aValue) : NS_OK;
>  }
>  
>  /** Just one action ( click ). */

please remove this comment we shouldn't comment evident things

Comment 7

[Trevor Saunders (:tbsaunde)]

Attachment: patch v3

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 547882 [details] [diff] [review]
patch v3

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

::: accessible/src/base/nsApplicationAccessible.h
@@ +124,2 @@
>    virtual PRUint64 NativeState();
> +  virtual PRUint64 State();

nit: this change isn't related with this bug

::: accessible/src/html/nsHTMLFormControlAccessible.h
@@ +62,5 @@
>    NS_IMETHOD GetActionName(PRUint8 aIndex, nsAString& aName);
>    NS_IMETHOD DoAction(PRUint8 index);
>  
>    // nsAccessible
> +  virtual PRUint8 ActionCount();

please make ActionAccessible section and everywhere below

::: accessible/src/xforms/nsXFormsAccessible.h
@@ +193,4 @@
>    NS_IMETHOD DoAction(PRUint8 aIndex);
>  
> +  // nsAccessible
> +  virtual PRUint8 ActionCount();

ActionAccessible

Comment 9

[Trevor Saunders (:tbsaunde)]

Attachment: patch

I think I moved all the ActionCount(0 declarations, but you can probably check a bit faster than me.

Comment 10

[alexander :surkov (:asurkov)]

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

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

::: accessible/src/xul/nsXULFormControlAccessible.h
@@ +100,5 @@
>    NS_IMETHOD GetActionName(PRUint8 aIndex, nsAString& aName);
>    NS_IMETHOD DoAction(PRUint8 index);
>  
>    // nsAccessible
> +  virtual PRUint8 ActionCount();

this one is missed

@@ +119,5 @@
>    NS_IMETHOD GetActionName(PRUint8 aIndex, nsAString& aName);
>    NS_IMETHOD DoAction(PRUint8 index);
>  
>    // nsAccessible
> +  virtual PRUint8 ActionCount();

too

@@ +248,5 @@
>    // nsIAccessibleEditableText
>    NS_IMETHOD GetAssociatedEditor(nsIEditor **aEditor);
>  
>    // nsAccessible
> +  virtual PRUint8 ActionCount();

too

Comment 11

[Trevor Saunders (:tbsaunde)]

Attachment: patch

thanks, should be fixed

Comment 12

[alexander :surkov (:asurkov)]

landed http://hg.mozilla.org/mozilla-central/rev/be4b064f1159