Last Comment Bug 742695 - densify nsARIAGridAccessible
: densify nsARIAGridAccessible
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Mark Capella [:capella]
:
:
Mentors:
Depends on:
Blocks: densifya11y
  Show dependency treegraph
 
Reported: 2012-04-05 05:48 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-04-10 08:27 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (38.91 KB, patch)
2012-04-07 18:34 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v2) (40.77 KB, patch)
2012-04-08 02:08 PDT, Mark Capella [:capella]
tbsaunde+mozbugs: review+
surkov.alexander: feedback+
Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2012-04-05 05:48:31 PDT
similar to bug 739889
first rename nsARIAGridWrap to mozilla::a11y::ARIAGridWrap and nsARIAGridCellAccessible -> mozilla::a11y::ARIAGridCellAccessibleWrap
then rename nsARIAGridAccessibl to ARIAGridAccessible and same for nsARIGridCellAccessible, and move them to generi folder (accessible/src/generic/ from accessible/src/base/)
Comment 1 alexander :surkov 2012-04-05 05:52:45 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #0)
> first rename nsARIAGridWrap to mozilla::a11y::ARIAGridWrap and

nsARIAGridAccessibleWrap -> mozilla::a11y::ARIAGridAccessibleWrap
Comment 2 Mark Capella [:capella] 2012-04-06 13:25:43 PDT
Please verify that this patch request involves these three files:

   accessible\src\atk\nsARIAGridAccessibleWrap.h

   accessible\src\base\nsARIAGridAccessible.cpp
   accessible\src\base\nsARIAGridAccessible.h

And that these are not affected by this change at this time:

   accessible\src\base\nsARIAMap.cpp
   accessible\src\base\nsARIAMap.h

   accessible\src\mac\nsARIAGridAccessibleWrap.h

   accessible\src\msaa\nsARIAGridAccessibleWrap.cpp
   accessible\src\msaa\nsARIAGridAccessibleWrap.h

   accessible\src\other\nsARIAGridAccessibleWrap.h
Comment 3 alexander :surkov 2012-04-06 20:47:43 PDT
(In reply to Mark Capella [:capella] from comment #2)
> Please verify that this patch request involves these three files:
> 
>    accessible\src\atk\nsARIAGridAccessibleWrap.h
> 
>    accessible\src\base\nsARIAGridAccessible.cpp
>    accessible\src\base\nsARIAGridAccessible.h

there's nsARIAGridAccessibleWrap.h file in each platform folder: atk/msaa/mac/other

> And that these are not affected by this change at this time:
> 
>    accessible\src\base\nsARIAMap.cpp
>    accessible\src\base\nsARIAMap.h

yes

>    accessible\src\mac\nsARIAGridAccessibleWrap.h
> 
>    accessible\src\msaa\nsARIAGridAccessibleWrap.cpp
>    accessible\src\msaa\nsARIAGridAccessibleWrap.h
> 
>    accessible\src\other\nsARIAGridAccessibleWrap.h

affected
Comment 4 Mark Capella [:capella] 2012-04-07 18:34:06 PDT
Created attachment 613151 [details] [diff] [review]
Patch (v1)

Ok Trev ... First pass for this bug, feedback / please tear it up for me  :)
Comment 5 Mark Capella [:capella] 2012-04-08 02:08:38 PDT
Created attachment 613168 [details] [diff] [review]
Patch (v2)

Ok Trevor, this patch is updated with the comments you made in IRC, re: paragraph alignment and header wraps ...
Comment 6 alexander :surkov 2012-04-08 21:25:31 PDT
Comment on attachment 613168 [details] [diff] [review]
Patch (v2)

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

I'll fix those before landing to avoid merging problems (I have couple big Mark's patches on landing queue).

::: accessible/src/atk/Makefile.in
@@ +70,5 @@
>  
>  EXPORTS = \
>    AtkSocketAccessible.h \
>    nsAccessNodeWrap.h \
> +  ARIAGridAccessibleWrap.h \

no need to export it

::: accessible/src/base/nsAccessibilityService.cpp
@@ +43,5 @@
>  #include "nsAccessiblePivot.h"
>  #include "nsCoreUtils.h"
>  #include "nsAccUtils.h"
>  #include "nsApplicationAccessibleWrap.h"
> +#include "ARIAGridAccessibleWrap.h"

it's not in alphabetical order

::: accessible/src/base/nsARIAGridAccessible.cpp
@@ +83,5 @@
>  ////////////////////////////////////////////////////////////////////////////////
>  // nsIAccessibleTable
>  
>  NS_IMETHODIMP
> +ARIAGridAccessible::GetSummary(nsAString &aSummary)

type& aName

@@ +95,5 @@
>    return NS_ERROR_NOT_IMPLEMENTED;
>  }
>  
>  NS_IMETHODIMP
> +ARIAGridAccessible::GetColumnCount(PRInt32 *acolumnCount)

type* aColumnCount while you are here pls

@@ +118,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +ARIAGridAccessible::GetRowCount(PRInt32 *arowCount)

type* aRowCount

@@ +135,5 @@
>  }
>  
>  NS_IMETHODIMP
> +ARIAGridAccessible::GetCellAt(PRInt32 aRowIndex, PRInt32 aColumnIndex,
> +                              nsIAccessible **aAccessible)

type** aCell and everywhere below

@@ +270,5 @@
>  }
>  
>  NS_IMETHODIMP
> +ARIAGridAccessible::GetRowExtentAt(PRInt32 aRow, PRInt32 aColumn,
> +                                   PRInt32 *aExtentCount)

any particular reason to move the method implementation?

::: accessible/src/base/nsARIAGridAccessible.h
@@ +53,5 @@
>   */
> +class ARIAGridAccessible : public nsAccessibleWrap,
> +                           public xpcAccessibleTable,
> +                           public nsIAccessibleTable,
> +                           public mozilla::a11y::TableAccessible

public TableAccessible

::: accessible/src/mac/Makefile.in
@@ +64,5 @@
>  EXPORTS = \
>    nsAccessNodeWrap.h \
>    nsTextAccessibleWrap.h \
>    nsAccessibleWrap.h \
> +  ARIAGridAccessibleWrap.h \

I think we don't need to export it

::: accessible/src/msaa/Makefile.in
@@ +83,5 @@
>    nsTextAccessibleWrap.h \
>    nsDocAccessibleWrap.h \
>    nsRootAccessibleWrap.h \
>    nsHTMLWin32ObjectAccessible.h \
> +  ARIAGridAccessibleWrap.h \

no need to export

::: accessible/src/other/Makefile.in
@@ +54,5 @@
>    $(NULL)
>  
>  EXPORTS = \
>    nsAccessNodeWrap.h \
> +  ARIAGridAccessibleWrap.h \

same
Comment 7 Trevor Saunders (:tbsaunde) 2012-04-08 23:58:19 PDT
> ::: accessible/src/atk/Makefile.in
> @@ +70,5 @@
> >  
> >  EXPORTS = \
> >    AtkSocketAccessible.h \
> >    nsAccessNodeWrap.h \
> > +  ARIAGridAccessibleWrap.h \
> 
> no need to export it

I think if you stop exporting all of these at once it'll work, but since we have a bunch of other stuff we should stop exporting and a bug for it, I didn't see much reason to make Mark deal with it now.

> ::: accessible/src/base/nsAccessibilityService.cpp
> @@ +43,5 @@
> >  #include "nsAccessiblePivot.h"
> >  #include "nsCoreUtils.h"
> >  #include "nsAccUtils.h"
> >  #include "nsApplicationAccessibleWrap.h"
> > +#include "ARIAGridAccessibleWrap.h"
> 
> it's not in alphabetical order

true, but if you ignore all the ns's which will go away soon they are modulo nsCoreUtils.h so if you like we can move things up as we rename them, but that seems kind of silly.
Comment 8 alexander :surkov 2012-04-08 23:59:49 PDT
(In reply to alexander :surkov from comment #6)
> ::: accessible/src/atk/Makefile.in
> @@ +70,5 @@
> >  
> >  EXPORTS = \
> >    AtkSocketAccessible.h \
> >    nsAccessNodeWrap.h \
> > +  ARIAGridAccessibleWrap.h \
> 
> no need to export it

it produces linker errors (event if I add dependency on these folders into base Makefile.in). So the best way to address that as separate bug.
Comment 9 alexander :surkov 2012-04-09 00:02:17 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > ::: accessible/src/base/nsAccessibilityService.cpp
> > @@ +43,5 @@
> > >  #include "nsAccessiblePivot.h"
> > >  #include "nsCoreUtils.h"
> > >  #include "nsAccUtils.h"
> > >  #include "nsApplicationAccessibleWrap.h"
> > > +#include "ARIAGridAccessibleWrap.h"
> > 
> > it's not in alphabetical order
> 
> true, but if you ignore all the ns's which will go away soon they are modulo
> nsCoreUtils.h so if you like we can move things up as we rename them, but
> that seems kind of silly.

perhaps not all of them are just pending for ns removal, nsCoreUtils/nsAccUtils might be a part of utils namespace, nsAccessiblePivot could be xpcAccessiblePivot or something else. It's cleaner to move them as you go.
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2012-04-09 00:36:49 PDT
Comment on attachment 613168 [details] [diff] [review]
Patch (v2)

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

::: accessible/src/base/nsAccessibilityService.cpp
@@ +39,4 @@
>  #include "mozilla/Util.h"
>  
>  // NOTE: alphabetically ordered
>  #include "nsAccessibilityService.h"

It's better to keep X.h included first in X.cpp, to ensure X.h doesn't have hidden dependencies on headers it doesn't include itself.
Comment 11 Trevor Saunders (:tbsaunde) 2012-04-09 03:10:05 PDT
(In reply to alexander :surkov from comment #8)
> (In reply to alexander :surkov from comment #6)
> > ::: accessible/src/atk/Makefile.in
> > @@ +70,5 @@
> > >  
> > >  EXPORTS = \
> > >    AtkSocketAccessible.h \
> > >    nsAccessNodeWrap.h \
> > > +  ARIAGridAccessibleWrap.h \
> > 
> > no need to export it
> 
> it produces linker errors (event if I add dependency on these folders into
> base Makefile.in). So the best way to address that as separate bug.

i'D THINK YOU COULD HAVEISSUES WITH INCLUDE PATHS, BUT LINKER ISSUES SEEMS FUNNY.

(In reply to Ms2ger from comment #10)
> Comment on attachment 613168 [details] [diff] [review]
> Patch (v2)
> 
> Review of attachment 613168 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/nsAccessibilityService.cpp
> @@ +39,4 @@
> >  #include "mozilla/Util.h"
> >  
> >  // NOTE: alphabetically ordered
> >  #include "nsAccessibilityService.h"
> 
> It's better to keep X.h included first in X.cpp, to ensure X.h doesn't have
> hidden dependencies on headers it doesn't include itself.

I'm not clear on your concern here, its first though should be seperated from other headers modulo the mozilla/Util.h that seems to get included first all over the place.
Comment 12 alexander :surkov 2012-04-09 03:13:35 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #11)

> I'm not clear on your concern here, its first though should be seperated
> from other headers modulo the mozilla/Util.h that seems to get included
> first all over the place.

I think the point is "nsAccessibilityService.h" is not the first include in "nsAccessibilityService.cpp" file. Usually we follow this rule. I changed locally to address this.

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