Last Comment Bug 745740 - De-ns-ify nsApplicationAccessible
: De-ns-ify nsApplicationAccessible
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)
: mozilla15
Assigned To: Mark Capella [:capella]
:
Mentors:
Depends on:
Blocks: densifya11y
  Show dependency treegraph
 
Reported: 2012-04-16 07:35 PDT by Mark Capella [:capella]
Modified: 2012-04-25 20:42 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (49.81 KB, patch)
2012-04-18 05:56 PDT, Mark Capella [:capella]
eitan: feedback+
Details | Diff | Splinter Review
Patch (v2) (49.30 KB, patch)
2012-04-19 11:52 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v3) (50.11 KB, patch)
2012-04-23 05:48 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v4) (51.96 KB, patch)
2012-04-24 00:37 PDT, Mark Capella [:capella]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description Mark Capella [:capella] 2012-04-16 07:35:18 PDT
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] 2012-04-18 05:56:02 PDT
Created attachment 616094 [details] [diff] [review]
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] 2012-04-19 10:53:39 PDT
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] 2012-04-19 11:52:41 PDT
Created attachment 616682 [details] [diff] [review]
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 2012-04-23 00:56:11 PDT
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] 2012-04-23 05:48:02 PDT
Created attachment 617449 [details] [diff] [review]
Patch (v3)

Re-based the patch, re-aligned the two routines, re-built and re-tested.
Comment 6 Trevor Saunders (:tbsaunde) 2012-04-23 20:10:21 PDT
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] 2012-04-23 22:02:10 PDT
The exports in the makefiles were needed to build, so I added them back in.
Comment 8 Mark Capella [:capella] 2012-04-23 22:07:57 PDT
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] 2012-04-24 00:37:07 PDT
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 ... but the rest of the nits you pointed out were small and built and tested out successfully.

See the attached.
Comment 10 alexander :surkov 2012-04-24 00:47:05 PDT
(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) 2012-04-24 23:08:41 PDT
(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) 2012-04-24 23:09:47 PDT
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 2012-04-24 23:39:30 PDT
(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 2012-04-25 00:13:47 PDT
btw, I pushed try server build with comment #12 addressed
Comment 16 alexander :surkov 2012-04-25 04:37:11 PDT
(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 2012-04-25 04:42:40 PDT
btw, mozilla::a11y namespace was missed around ApplicationAccessible class (I filed bug 748719)
Comment 18 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-04-25 20:42:05 PDT
https://hg.mozilla.org/mozilla-central/rev/ba1c9beaf75f

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