745740 - De-ns-ify nsApplicationAccessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Mark Capella [:capella]]

Similar to bug 742695 and bug 745428 with base/nsApplicationAccessible.cpp/h this time.  Make sure to update the nsApplicationAccessibleWraps.cpp/.h in msaa/ atk/ mac/ and other/

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v1)

Coded, built, and unit tested locally against the mochitest-a11y suite ... asking for initial feedback from the mentor ...

Comment 2

[Eitan Isaacson [:eeejay]]

Comment on attachment 616094 [details] [diff] [review]
Patch (v1)

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

Looks good.

::: accessible/src/atk/nsApplicationAccessibleWrap.cpp
@@ -57,5 @@
>  #endif
>  
> -using namespace mozilla;
> -using namespace mozilla::a11y;
> -

Are these namespaces not necessary because of this patch? If this is just a generic cleanup, which is great, just make sure to add that in the second line of the commit message.

::: accessible/src/base/nsAccessNode.cpp
@@ +71,1 @@
>  

Thanks for putting the asterisk in the right place in all changes..

::: accessible/src/base/nsApplicationAccessible.h
@@ +45,1 @@
>  

Is MOZILLA_A11Y_* the new prefix we are going with for everything? I like keeping it a de-camel-cased version of the filename, like __APPLICATION_ACCESSIBLE_H__. Maybe we need a prefix because of identically named files in the project? In that case A11Y would work: A11Y__APPLICATION_ACCESSIBLE_H__.

Of course this is bike shedding, and not worth blocking anything.

Comment 3

[Mark Capella [:capella]]

Attachment: Patch (v2)

Thanks for your feedback+ !

Re: /atk/nsApplicationAccessibleWrap.cpp, I had pulled those lines while researching a build error. It turns out they weren't related. I forgot to replace them, and have done so now. (Nice catch)

Re: putting the asterisk in the right place in all changes, I'm learning how to avoid nit changes sooner rather than later ... helps all involved!

Re: MOZILLA_A11Y_* the new prefix, this was started somewhere along de-ns-ify bug 739889 comment#15, carried into de-ns-ify bug 742695, and I'm still following the convention.

I'm going to now ask Alex for the "review?" .... I've been keeping him busy lately :)

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 616682 [details] [diff] [review]
Patch (v2)

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

redirecting review to Trevor per irc

::: accessible/src/atk/nsApplicationAccessibleWrap.cpp
@@ +482,1 @@
>          nsAccessNode::GetApplicationAccessible();

type* name
please use two spaces indentation while you are here

@@ +554,3 @@
>  
> +ApplicationAccessibleWrap::ApplicationAccessibleWrap():
> +    ApplicationAccessible()

same here

Comment 5

[Mark Capella [:capella]]

Attachment: Patch (v3)

Re-based the patch, re-aligned the two routines, re-built and re-tested.

Comment 6

[Trevor Saunders (:tbsaunde)]

Comment on attachment 617449 [details] [diff] [review]
Patch (v3)

> mai_util_get_root(void)
> {
>-    if (nsAccessibilityService::IsShutdown()) {
>-        // We've shutdown, try to use gail instead
>-        // (to avoid assert in spi_atk_tidy_windows())
>-        if (gail_get_root)
>-            return gail_get_root();
>-
>-        return nsnull;
>-    }
>-
>-    nsApplicationAccessible *applicationAcc =
>-        nsAccessNode::GetApplicationAccessible();
>-
>-    if (applicationAcc)
>-        return applicationAcc->GetAtkObject();
>+  if (nsAccessibilityService::IsShutdown()) {
>+    // We've shutdown, try to use gail instead
>+    // (to avoid assert in spi_atk_tidy_windows())
>+    if (gail_get_root)
>+      return gail_get_root();
> 
>     return nsnull;
>+  }
>+
>+  ApplicationAccessible* applicationAcc =
>+    nsAccessNode::GetApplicationAccessible();
>+
>+  if (applicationAcc)

can't be false, so you should be able to do GetApplicationAcc()->GetAtkObject();

>rename to accessible/src/atk/ApplicationAccessibleWrap.h
>--- a/accessible/src/atk/nsApplicationAccessibleWrap.h
>+++ b/accessible/src/atk/ApplicationAccessibleWrap.h
>@@ -36,27 +36,27 @@
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #ifndef __NS_APP_ROOT_ACCESSIBLE_H__
> #define __NS_APP_ROOT_ACCESSIBLE_H__

nit, rename it

>--- a/accessible/src/atk/nsAccessNodeWrap.cpp
>+++ b/accessible/src/atk/nsAccessNodeWrap.cpp
>@@ -33,17 +33,17 @@
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsAccessNodeWrap.h"
>-#include "nsApplicationAccessibleWrap.h"
>+#include "ApplicationAccessibleWrap.h"

I'm pretty sure that isn't needed, so please just stop including it if you can

/nsAccessibleWrap.cpp
>--- a/accessible/src/atk/nsAccessibleWrap.cpp
>+++ b/accessible/src/atk/nsAccessibleWrap.cpp
>@@ -38,17 +38,17 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsAccessibleWrap.h"
> 
> #include "Accessible-inl.h"
> #include "InterfaceInitFuncs.h"
> #include "nsAccUtils.h"
>-#include "nsApplicationAccessibleWrap.h"
>+#include "ApplicationAccessibleWrap.h"

nit, please keep them in order here and below

>+    ApplicationAccessible* applicationAcc =
>         nsAccessNode::GetApplicationAccessible();
>     nsAccessibleWrap* tmpAppAccWrap =
>         static_cast<nsAccessibleWrap*>(applicationAcc);

nit, while your here you should be able to get rid of nsAccessNode:: and the cast

>+    gApplicationAccessible = new ApplicationAccessibleWrap();
>     if (!gApplicationAccessible)
>       return nsnull;

nit, new is infalable now, so please remove the if while your here

>+++ b/accessible/src/base/nsAccessNode.h
>@@ -49,17 +49,17 @@
> 
> #include "nsIContent.h"
> #include "nsIDOMNode.h"
> #include "nsINameSpaceManager.h"
> #include "nsIStringBundle.h"
> #include "nsWeakReference.h"
> 
> class nsAccessNode;
>-class nsApplicationAccessible;
>+class ApplicationAccessible;

nit, keep these in order too.

>+++ b/accessible/src/base/nsRootAccessible.cpp
>@@ -37,17 +37,17 @@
> 
> #include "mozilla/Util.h"
> 
> #define CreateEvent CreateEventA
> #include "nsIDOMDocument.h"
> 
> #include "Accessible-inl.h"
> #include "nsAccessibilityService.h"
>-#include "nsApplicationAccessibleWrap.h"
>+#include "ApplicationAccessibleWrap.h"

I don't think its needed

>--- a/accessible/src/mac/Makefile.in
>+++ b/accessible/src/mac/Makefile.in
>@@ -69,17 +69,17 @@ EXPORTS = \
>   nsDocAccessibleWrap.h \
>   nsRootAccessibleWrap.h \
>   nsXULMenuAccessibleWrap.h \
>   nsXULListboxAccessibleWrap.h \
>   nsXULTreeGridAccessibleWrap.h \
>   nsHyperTextAccessibleWrap.h \
>   nsHTMLImageAccessibleWrap.h \
>   nsHTMLTableAccessibleWrap.h \
>-  nsApplicationAccessibleWrap.h \
>+  ApplicationAccessibleWrap.h \

I think you should be able to just stop exporting it all together

> public:
>   static void PreCreate();
>   static void Unload();
> };
>-
>+ 

nit, you add whitespace at the end of a line

>@@ -86,17 +86,17 @@ EXPORTS = \
>   nsRootAccessibleWrap.h \
>   nsHTMLWin32ObjectAccessible.h \
>   nsXULMenuAccessibleWrap.h \
>   nsXULListboxAccessibleWrap.h \
>   nsXULTreeGridAccessibleWrap.h \
>   nsHyperTextAccessibleWrap.h \
>   nsHTMLImageAccessibleWrap.h \
>   nsHTMLTableAccessibleWrap.h \
>-  nsApplicationAccessibleWrap.h \
>+  ApplicationAccessibleWrap.h \

nit, please stop exporting it

>+++ b/accessible/src/other/Makefile.in
>@@ -61,17 +61,17 @@ EXPORTS = \
>   nsDocAccessibleWrap.h \
>   nsRootAccessibleWrap.h \
>   nsXULMenuAccessibleWrap.h \
>   nsXULListboxAccessibleWrap.h \
>   nsXULTreeGridAccessibleWrap.h \
>   nsHyperTextAccessibleWrap.h \
>   nsHTMLImageAccessibleWrap.h \
>   nsHTMLTableAccessibleWrap.h \
>-  nsApplicationAccessibleWrap.h \
>+  ApplicationAccessibleWrap.h \

same

Comment 7

[Mark Capella [:capella]]

The exports in the makefiles were needed to build, so I added them back in.

Comment 8

[Mark Capella [:capella]]

Also, re: I'm pretty sure that isn't needed ... "ApplicationAccessibleWrap.h" was actually required in nsaccessnodewrap.cpp so I had to add it back again.

Comment 9

[Mark Capella [:capella]]

Attachment: Patch (v4)

Ok. I normally don't like making unrelated changes to code while working on a patch just because we're in the neighborhood ... but the rest of the nits you pointed out were small and built and tested out successfully.

See the attached.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #9)
> Created attachment 617811 [details] [diff] [review]
> Patch (v4)
> 
> Ok. I normally don't like making unrelated changes to code while working on
> a patch just because we're in the neighborhood

in general that's a right way to go but it makes sense to do for nits because that allows us to make the code consistent

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to Mark Capella [:capella] from comment #7)
> The exports in the makefiles were needed to build, so I added them back in.

as far as I can see you now only export ApplicationAccessibleWrap.h on other mac and windows but not ApplicationAccessible.h so I don't see how it can be included somewhere this matters.

what were the errors?

Comment 12

[Trevor Saunders (:tbsaunde)]

Comment on attachment 617811 [details] [diff] [review]
Patch (v4)

>+++ b/accessible/src/msaa/Makefile.in
>@@ -56,47 +56,47 @@ CPPSRCS = \
>   ARIAGridAccessibleWrap.cpp \
>   nsRootAccessibleWrap.cpp \
>   nsXULMenuAccessibleWrap.cpp \
>   nsXULListboxAccessibleWrap.cpp \
>   nsXULTreeGridAccessibleWrap.cpp \
>   nsHyperTextAccessibleWrap.cpp \
>   nsHTMLImageAccessibleWrap.cpp \
>   nsHTMLTableAccessibleWrap.cpp \
>-  nsApplicationAccessibleWrap.cpp \
>+  ApplicationAccessibleWrap.cpp \

not put in order

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #11)
> (In reply to Mark Capella [:capella] from comment #7)
> > The exports in the makefiles were needed to build, so I added them back in.
> 
> as far as I can see you now only export ApplicationAccessibleWrap.h on other
> mac and windows but not ApplicationAccessible.h so I don't see how it can be
> included somewhere this matters.
> 
> what were the errors?

I think base directory doesn't have dependency on platform folders (except atk), so there's no ApplicationAccessibleWrap.h for nsAccessNode.cpp.

I'd suggest to file a good first bug for that.

Comment 14

[alexander :surkov (:asurkov)]

btw, I pushed try server build with comment #12 addressed

Comment 15

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba1c9beaf75f

Comment 16

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #13)
> I'd suggest to file a good first bug for that.

I filed bug 748716.

Comment 17

[alexander :surkov (:asurkov)]

btw, mozilla::a11y namespace was missed around ApplicationAccessible class (I filed bug 748719)

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/ba1c9beaf75f