Last Comment Bug 671926 - decomtaminate GetNumActions()
: decomtaminate GetNumActions()
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Trevor Saunders (:tbsaunde)
:
Mentors:
Depends on:
Blocks: dexpcoma11y
  Show dependency treegraph
 
Reported: 2011-07-15 12:04 PDT by Trevor Saunders (:tbsaunde)
Modified: 2011-08-03 21:00 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (59.57 KB, patch)
2011-07-15 15:28 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review
patch v2 (59.32 KB, patch)
2011-07-16 17:20 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
patch v3 (60.71 KB, patch)
2011-07-22 19:36 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
patch (61.70 KB, patch)
2011-07-22 23:09 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
patch (61.96 KB, patch)
2011-07-22 23:23 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2011-07-15 12:04:00 PDT
create virtual PRUint8 ActionCount(0 and add GetNumActions() xpcom wrapper
Comment 1 Trevor Saunders (:tbsaunde) 2011-07-15 15:28:15 PDT
Created attachment 546246 [details] [diff] [review]
patch
Comment 2 alexander :surkov 2011-07-15 23:21:02 PDT
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) 2011-07-15 23:50:05 PDT
(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 2011-07-16 08:41:28 PDT
(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) 2011-07-16 17:20:48 PDT
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.
Comment 6 alexander :surkov 2011-07-16 23:23:59 PDT
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) 2011-07-22 19:36:46 PDT
Created attachment 547882 [details] [diff] [review]
patch v3
Comment 8 alexander :surkov 2011-07-22 22:00:39 PDT
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) 2011-07-22 23:09:34 PDT
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.
Comment 10 alexander :surkov 2011-07-22 23:12:13 PDT
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) 2011-07-22 23:23:51 PDT
Created attachment 547895 [details] [diff] [review]
patch

thanks, should be fixed
Comment 12 alexander :surkov 2011-08-03 21:00:26 PDT
landed http://hg.mozilla.org/mozilla-central/rev/be4b064f1159

Note You need to log in before you can comment on or make changes to this bug.