decomtaminate GetNumActions()

RESOLVED FIXED in mozilla8

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

unspecified
mozilla8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

6 years ago
create virtual PRUint8 ActionCount(0 and add GetNumActions() xpcom wrapper
(Assignee)

Updated

6 years ago
Summary: DECOMTAMINATE gETnUMaCTIONS() → decomtaminate GetNumActions()
(Assignee)

Comment 1

6 years ago
Created attachment 546246 [details] [diff] [review]
patch
Attachment #546246 - Flags: review?(surkov.alexander)

Comment 2

6 years ago
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
Attachment #546246 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 3

6 years ago
(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

6 years ago
(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.
(Assignee)

Comment 5

6 years ago
Created attachment 546365 [details] [diff] [review]
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.
Attachment #546246 - Attachment is obsolete: true

Comment 6

6 years ago
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
(Assignee)

Comment 7

6 years ago
Created attachment 547882 [details] [diff] [review]
patch v3
Attachment #546365 - Attachment is obsolete: true

Comment 8

6 years ago
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
(Assignee)

Comment 9

6 years ago
Created attachment 547893 [details] [diff] [review]
patch

I think I moved all the ActionCount(0 declarations, but you can probably check a bit faster than me.
Attachment #547882 - Attachment is obsolete: true

Comment 10

6 years ago
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
(Assignee)

Comment 11

6 years ago
Created attachment 547895 [details] [diff] [review]
patch

thanks, should be fixed
Attachment #547893 - Attachment is obsolete: true

Comment 12

6 years ago
landed http://hg.mozilla.org/mozilla-central/rev/be4b064f1159
Status: NEW → RESOLVED
Last Resolved: 6 years ago
OS: Linux → All
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.