Closed Bug 671926 Opened 13 years ago Closed 13 years ago

decomtaminate GetNumActions()

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

Details

Attachments

(1 file, 4 obsolete files)

create virtual PRUint8 ActionCount(0 and add GetNumActions() xpcom wrapper
Summary: DECOMTAMINATE gETnUMaCTIONS() → decomtaminate GetNumActions()
Attached patch patch (obsolete) — Splinter Review
Attachment #546246 - Flags: review?(surkov.alexander)
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+
(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.
(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.
Attached patch patch v2 (obsolete) — Splinter Review
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 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
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #546365 - Attachment is obsolete: true
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
Attached patch patch (obsolete) — Splinter Review
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 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
Attached patch patchSplinter Review
thanks, should be fixed
Attachment #547893 - Attachment is obsolete: true
landed http://hg.mozilla.org/mozilla-central/rev/be4b064f1159
Status: NEW → RESOLVED
Closed: 13 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.