Closed Bug 767754 Opened 8 years ago Closed 7 years ago

Move ApplicationAccessible logic from nsAccessNode to nsAccessibilityService

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: capella, Assigned: vinceyang15)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 8 obsolete files)

Streamline nsAccessNode, move ApplicationAccessible logic out...

Consider creating a function a11y::ApplicationAccessible() that's a friend of nsAccessibilityService, the same thing we do with FocusMgr()
nsAccessNode::Init() should just move to Accessible::Init()
(In reply to Mark Capella [:capella] from comment #1)
> nsAccessNode::Init() should just move to Accessible::Init()

that should be its own bug.
Disregard my comment 1 ... thats now Bug 767757 - Move nsAccessNode::Init() to Accessible::Init()
Attached patch Patch_v1 (obsolete) — Splinter Review
the patch does not compile... just want someone to help
Attachment #643869 - Flags: review?(markcapella)
Comment on attachment 643869 [details] [diff] [review]
Patch_v1

More appropriate to ask your mentor - over to Trev...
Attachment #643869 - Flags: review?(markcapella) → feedback?(trev.saunders)
(In reply to Mark Capella [:capella] from comment #5)
> Comment on attachment 643869 [details] [diff] [review]
> Patch_v1
> 
> More appropriate to ask your mentor - over to Trev...

anyone can is free to give help :)
Comment on attachment 643869 [details] [diff] [review]
Patch_v1

it seems like this needs a bit of work but is going in the right direction.

The big things I see you have left to do are reintroduce logic to create ApplicationAccessible in nsAccessibilityService::Init(), you also need to remove the Release() of the application accessible from nsAccessNode::ShutdoownxpAccessibility() and probably some other cleanup.

if you want help with specific issues you'll need to provide compiler errors or more specific questions.
Please give overview of the idea. Turning the accessibility service into application accessible looks new.
(In reply to alexander :surkov from comment #8)
> Please give overview of the idea. Turning the accessibility service into
> application accessible looks new.

nah, just move GetApplicationAccessible() from nsAccessNode to nsAccessibilityService, and make nsAccessibilityService create it directly at start up.  It should make getting the application accessible slightly faster since we don't need to test for the case  that we need to create it.  It also seems to make more sense that you get the pplication accessible from the service not nsAccessNode.
ok (the patch confused me)
Attached file Patch_v2 (obsolete) —
Attachment #643869 - Attachment is obsolete: true
Attachment #643869 - Flags: feedback?(trev.saunders)
Attachment #644579 - Flags: review?(trev.saunders)
Comment on attachment 644579 [details]
Patch_v2

>diff --git a/accessible/src/base/nsAccDocManager.cpp b/accessible/src/base/nsAccDocManager.cpp
>--- a/accessible/src/base/nsAccDocManager.cpp
>+++ b/accessible/src/base/nsAccDocManager.cpp
>@@ -40,17 +40,17 @@ using namespace mozilla::a11y;
> 
> DocAccessible*
> nsAccDocManager::GetDocAccessible(nsIDocument *aDocument)
> {
>   if (!aDocument)
>     return nsnull;
> 
>   // Ensure CacheChildren is called before we query cache.
>-  nsAccessNode::GetApplicationAccessible()->EnsureChildren();
>+  mozilla::a11y::GetApplicationAccessible()->EnsureChildren();

nit, you are in using namespace mozilla::a11y, su you don't need it here.

> 
>   // Bind the document to the tree.
>   if (isRootDoc) {
>-    Accessible* appAcc = nsAccessNode::GetApplicationAccessible();
>+    Accessible* appAcc = mozilla::a11y::GetApplicationAccessible();

same

>diff --git a/accessible/src/base/nsAccessNode.cpp b/accessible/src/base/nsAccessNode.cpp
>--- a/accessible/src/base/nsAccessNode.cpp
>+++ b/accessible/src/base/nsAccessNode.cpp
>@@ -23,17 +23,17 @@
> #include "mozilla/Services.h"

you don't need to include ApplicationAccessible.h any more do you?

>-ApplicationAccessible* nsAccessNode::gApplicationAccessible = nsnull;
>+//ApplicationAccessible* nsAccessNode::gApplicationAccessible = nsnull;

just remove it

>-
>-void nsAccessNode::ShutdownXPAccessibility()
>+/*void nsAccessNode::ShutdownXPAccessibility()
> {
>   // Called by nsAccessibilityService::Shutdown()
>   // which happens when xpcom is shutting down
>   // at exit of program
> 
>   // Release gApplicationAccessible after everything else is shutdown
>   // so we don't accidently create it again while tearing down root accessibles
>   ApplicationAccessibleWrap::Unload();
>   if (gApplicationAccessible) {
>     gApplicationAccessible->Shutdown();
>     NS_RELEASE(gApplicationAccessible);
>   }
>-}
>+  }*/

just remove it

> nsAccessibilityService::GetApplicationAccessible(nsIAccessible** aAccessibleApplication)
> {
>   NS_ENSURE_ARG_POINTER(aAccessibleApplication);
> 
>-  NS_IF_ADDREF(*aAccessibleApplication = nsAccessNode::GetApplicationAccessible());
>+  NS_IF_ADDREF(*aAccessibleApplication = mozilla::a11y::GetApplicationAccessible());

nit, no mozilla::a11y::

>+  // Initialize application accessible
>+  if (!gApplicationAccessible) {

this should only ever be called at start up, so you can remove the check

>+    ApplicationAccessibleWrap::PreCreate();
>+	
>+    gApplicationAccessible = new ApplicationAccessibleWrap();
>+	
>+    // Addref on create. Will Release in ShutdownXPAccessibility()

update comment

>+    NS_ADDREF(gApplicationAccessible);
>+    
>+    nsresult rv = gApplicationAccessible->Init();

it returns bool not nsresult

>+    if (NS_FAILED(rv)) {
>+      gApplicationAccessible->Shutdown();
>+      NS_RELEASE(gApplicationAccessible);
>+      return false;
>+    }

set it to null too

> 
>-  nsAccessNodeWrap::ShutdownAccessibility();
>+  //nsAccessNodeWrap::ShutdownAccessibility();

nit, just remove it

>+/**
>+ * Return an application accessible.

s/an/the/

>   friend nsAccessibilityService* GetAccService();
>   friend mozilla::a11y::FocusManager* mozilla::a11y::FocusMgr();
>+  friend mozilla::a11y::ApplicationAccessible* mozilla::a11y::GetApplicationAccessible();

nit, just name it ApplicationAccessible()

looks good, but I'd like to see another version.
Attachment #644579 - Flags: review?(trev.saunders)
Attached file Patch(v3) (obsolete) —
Attachment #644753 - Flags: review?(trev.saunders)
Attachment #644579 - Attachment is obsolete: true
Comment on attachment 644753 [details]
Patch(v3)

> 
>   gIsShutdown = true;
> 
>-  nsAccessNodeWrap::ShutdownAccessibility();
>+  ApplicationAccessibleWrap::Unload();
>+  if (gApplicationAccessible) {
>+    gApplicationAccessible->Shutdown();
>+  }

nit, it can't be null, and you could consider setting null here instead of the constructor if you like.
Attachment #644753 - Flags: review?(trev.saunders) → review+
Attached patch Patch(v4) (obsolete) — Splinter Review
Attachment #644753 - Attachment is obsolete: true
Attachment #644945 - Flags: review?(trev.saunders)
Comment on attachment 644945 [details] [diff] [review]
Patch(v4)

you don't need to ask for review again since I r+'d the last patch.
Attachment #644945 - Flags: review?(trev.saunders)
can anyone help me to change the keyword?
Xi - you just want it "checkin-needed" ?
(In reply to Mark Capella [:capella] from comment #18)
> Xi - you just want it "checkin-needed" ?
You can push it to TRY first, then set it "checkin-needed"
Attached patch Patch - RebasedSplinter Review
Won't build locally ... you better review it again .. Quick reading the (above) ... if you renamed something you may need to scan for this and all other references ...

c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(132) : error C3861: 'GetApplicationAccessible': identifier not found
btw- use this version of the patch to start ... I tweaked yours due to .rej file applying to current code ...
Attached patch Patch(v5) (obsolete) — Splinter Review
Attachment #644945 - Attachment is obsolete: true
Attachment #645766 - Attachment is obsolete: true
Attached patch Patch(v5) (obsolete) — Splinter Review
(In reply to Mark Capella [:capella] from comment #21)
> btw- use this version of the patch to start ... I tweaked yours due to .rej
> file applying to current code ...

Hi Mark, would you please check my newly updated patch? Thanks!
Meh, I do what I can :)  I filed the original bug at the request of tbsaunde, he's your mentor and module-peer and I don't want to overstep my involvement, but for now:

I tried to build it for you on my local WIN machine and you failed again in the final link step with:

===> nsAccessNodeWrap.obj : error LNK2019: unresolved external symbol "public: static void __cdecl nsAccessNode::ShutdownXPAccessibility(void)" (?ShutdownXPAccessibility@nsAccessNode@@SAXXZ) referenced in function "public: static void __cdecl nsAccessNodeWrap::ShutdownAccessibility(void)" (?ShutdownAccessibility@nsAccessNodeWrap@@SAXXZ)

It looks like you removed that function in your patch in nsaccessnode.cpp, but left code references in place still in nsaccessnode.h, and the four nsaccessnodewrap.cpp/.mm.

See: http://mxr.mozilla.org/mozilla-central/search?string=ShutdownXPAccessibility&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

S/b an easy fix... but I have to wonder, are you building / testing your patches locally? I think this failure should have shown up on any development platform. 

Ok. Sometimes things pass review but fail on push to TRY. With patch approval, you can now (and in future for this patch) attempt an AUTOLAND TRY server push, see: https://wiki.mozilla.org/Build:Autoland for setting up keywords and such and: http://trychooser.pub.build.mozilla.org/ for determining build / test options.
(In reply to Xi Yang from comment #24)
> (In reply to Mark Capella [:capella] from comment #21)
> > btw- use this version of the patch to start ... I tweaked yours due to .rej
> > file applying to current code ...
> 
> Hi Mark, would you please check my newly updated patch? Thanks!

you should get try server access http://www.mozilla.org/hacking/committer/

(In reply to Mark Capella [:capella] from comment #25)
> Meh, I do what I can :)  I filed the original bug at the request of
> tbsaunde, he's your mentor and module-peer and I don't want to overstep my
> involvement, but for now:

everyone is free to comment, and try to help however :)

> I tried to build it for you on my local WIN machine and you failed again in
> the final link step with:
> 
> ===> nsAccessNodeWrap.obj : error LNK2019: unresolved external symbol
> "public: static void __cdecl nsAccessNode::ShutdownXPAccessibility(void)"
> (?ShutdownXPAccessibility@nsAccessNode@@SAXXZ) referenced in function
> "public: static void __cdecl nsAccessNodeWrap::ShutdownAccessibility(void)"
> (?ShutdownAccessibility@nsAccessNodeWrap@@SAXXZ)
> 
> It looks like you removed that function in your patch in nsaccessnode.cpp,
> but left code references in place still in nsaccessnode.h, and the four
> nsaccessnodewrap.cpp/.mm.
> 
> See:
> http://mxr.mozilla.org/mozilla-central/
> search?string=ShutdownXPAccessibility&find=&findi=&filter=^[^\0]*%24&hitlimit
> =&tree=mozilla-central
> 
> S/b an easy fix... but I have to wonder, are you building / testing your
> patches locally? I think this failure should have shown up on any
> development platform. 

I thought I remember seeing some of them get removed, but I'm not sure off hand.
Assignee: nobody → vinceyang15
Status: NEW → ASSIGNED
Attached patch Patch(v6) (obsolete) — Splinter Review
Attachment #645769 - Attachment is obsolete: true
Whiteboard: [mentor=trev.saunders@gmail.com][lang=c++] → [mentor=trev.saunders@gmail.com][lang=c++][autoland-$branch:648365:-b do -p all -u all -t none]
Attachment #648365 - Attachment is patch: true
Whiteboard: [mentor=trev.saunders@gmail.com][lang=c++][autoland-$branch:648365:-b do -p all -u all -t none] → [autoland-$branch:648365]
Attachment #648365 - Flags: review?(trev.saunders)
Comment on attachment 648365 [details] [diff] [review]
Patch(v6)

It looks like you lost the chunk that creates the application accessible (bad merge?)  I'll be suprised if that doesn't make all of the a11y tests fail.
Attachment #648365 - Flags: review?(trev.saunders) → review-
Attached patch Patch(v7)Splinter Review
Attachment #648365 - Attachment is obsolete: true
Whiteboard: [autoland-$branch:648365] → [autoland-$branch:648742]
Attachment #648742 - Flags: review?(trev.saunders)
Comment on attachment 648742 [details] [diff] [review]
Patch(v7)

> 
>-  AccessibleWrap* appAccWrap = nsAccessNode::GetApplicationAccessible();
>+  AccessibleWrap* appAccWrap = ApplicationAcc();
>   if (appAccWrap != accWrap && !accWrap->IsValidObject())
>     return nullptr;

nit, no need for the local variable.

>     // don't have DOM node (see bug 506206).
>     ApplicationAccessible* applicationAcc =
>-      nsAccessNode::GetApplicationAccessible();
>+      ApplicationAcc();
> 
>     if (mAccessible != static_cast<nsIAccessible*>(applicationAcc))

same

>   // Bind the document to the tree.
>   if (isRootDoc) {
>-    Accessible* appAcc = nsAccessNode::GetApplicationAccessible();
>+    Accessible* appAcc = ApplicationAcc();
>     if (!appAcc->AppendChild(docAcc)) {

same

> nsAccessibilityService::GetApplicationAccessible(nsIAccessible** aAccessibleApplication)
> {
>   NS_ENSURE_ARG_POINTER(aAccessibleApplication);
> 
>-  NS_IF_ADDREF(*aAccessibleApplication = nsAccessNode::GetApplicationAccessible());
>+  NS_IF_ADDREF(*aAccessibleApplication = ApplicationAcc());

nit, you could use the non-nullchecking version.

>-  nsAccessNodeWrap::ShutdownAccessibility();
>+  ApplicationAccessibleWrap::Unload();
>+  gApplicationAccessible->Shutdown();
>+  gApplicationAccessible = nullptr;

you leak here since you don't release gApplicationAccessible, but it doesn't end up mattering because we don't shutdown the accessibility service until shutdown.

I'll fix nits before landing.
Attachment #648742 - Flags: review?(trev.saunders) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #30)
> Comment on attachment 648742 [details] [diff] [review]
> Patch(v7)
> 
> > 
> >-  AccessibleWrap* appAccWrap = nsAccessNode::GetApplicationAccessible();
> >+  AccessibleWrap* appAccWrap = ApplicationAcc();
> >   if (appAccWrap != accWrap && !accWrap->IsValidObject())
> >     return nullptr;
> 
> nit, no need for the local variable.
> 
> >     // don't have DOM node (see bug 506206).
> >     ApplicationAccessible* applicationAcc =
> >-      nsAccessNode::GetApplicationAccessible();
> >+      ApplicationAcc();
> > 
> >     if (mAccessible != static_cast<nsIAccessible*>(applicationAcc))
> 
> same
> 
> >   // Bind the document to the tree.
> >   if (isRootDoc) {
> >-    Accessible* appAcc = nsAccessNode::GetApplicationAccessible();
> >+    Accessible* appAcc = ApplicationAcc();
> >     if (!appAcc->AppendChild(docAcc)) {
> 
> same
> 
> > nsAccessibilityService::GetApplicationAccessible(nsIAccessible** aAccessibleApplication)
> > {
> >   NS_ENSURE_ARG_POINTER(aAccessibleApplication);
> > 
> >-  NS_IF_ADDREF(*aAccessibleApplication = nsAccessNode::GetApplicationAccessible());
> >+  NS_IF_ADDREF(*aAccessibleApplication = ApplicationAcc());
> 
> nit, you could use the non-nullchecking version.
> 
> >-  nsAccessNodeWrap::ShutdownAccessibility();
> >+  ApplicationAccessibleWrap::Unload();
> >+  gApplicationAccessible->Shutdown();
> >+  gApplicationAccessible = nullptr;
> 
> you leak here since you don't release gApplicationAccessible, but it doesn't
> end up mattering because we don't shutdown the accessibility service until
> shutdown.
> 
> I'll fix nits before landing.

Hi Trevor, so can I set the keyword to checkin-needed now?
Unfortunately, this had to be backed out due to Windows mochitest-a11y leaks.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4250f3bbb10c

https://tbpl.mozilla.org/php/getParsedLog.php?id=14258816&tree=Mozilla-Inbound

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1402487 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of AccEvent with size 32 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of Accessible with size 80 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of AccessibleWrap with size 100 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of AsyncStatement with size 48 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 497 instances of AtomImpl with size 24 bytes each (11928 bytes total)
Whiteboard: [autoland-$branch:648742]
Comment on attachment 648742 [details] [diff] [review]
Patch(v7)

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

since the patch was backed out then few nits

::: accessible/src/base/AccEvent.cpp
@@ +127,5 @@
>      // XXX: remove this hack during reorganization of 506907. Meanwhile we
>      // want to get rid an assertion for application accessible events which
>      // don't have DOM node (see bug 506206).
>      ApplicationAccessible* applicationAcc =
> +      ApplicationAcc();

keep on the same line

@@ +132,3 @@
>  
>      if (mAccessible != static_cast<nsIAccessible*>(applicationAcc))
>        NS_ASSERTION(targetNode, "There should always be a DOM node for an event");

but it doesn't seem you need a variable for it since this is a unique use case. Btw, it sounds like we don't need that static_cast.

::: accessible/src/base/nsAccessibilityService.cpp
@@ +94,5 @@
>  nsAccessibilityService::~nsAccessibilityService()
>  {
>    NS_ASSERTION(gIsShutdown, "Accessibility wasn't shutdown!");
>    gAccessibilityService = nullptr;
> +  gApplicationAccessible = nullptr;

it must be a null at this point. You meant to add an assertion instead?

@@ +1209,5 @@
> +  gApplicationAccessible = new ApplicationAccessibleWrap();
> +  
> +  // Addref on create. Will Release in Shutdown()
> +  NS_ADDREF(gApplicationAccessible);
> +  

fix style please, no spaces nor tabs on empty lines

@@ +1256,5 @@
>    gIsShutdown = true;
>  
> +  ApplicationAccessibleWrap::Unload();
> +  gApplicationAccessible->Shutdown();
> +  gApplicationAccessible = nullptr;

release is missed but it was fixed on landing

@@ +1685,5 @@
>  nsAccessibilityService::AddNativeRootAccessible(void* aAtkAccessible)
>  {
>  #ifdef MOZ_ACCESSIBILITY_ATK
>    ApplicationAccessible* applicationAcc =
> +    ApplicationAcc();

on the same line pls

@@ +1706,5 @@
>  nsAccessibilityService::RemoveNativeRootAccessible(Accessible* aAccessible)
>  {
>  #ifdef MOZ_ACCESSIBILITY_ATK
>    ApplicationAccessible* applicationAcc =
> +    ApplicationAcc();

same

::: accessible/src/base/nsAccessibilityService.h
@@ +19,5 @@
>  class nsITreeView;
>  
>  namespace mozilla {
>  namespace a11y {
> +  class ApplicationAccessible;

incorrect indent

@@ +259,5 @@
>  
>    /**
> +	 * Reference for application accessible instance.
> +   */
> +	static mozilla::a11y::ApplicationAccessible* gApplicationAccessible;

wrong styling
So, the way this leaks is that you removed the call to nsAccessNodeWrap::ShutdownAccessibility() which on windows only drops a ref to an event which in turn holds a ref to an accessible.  Really I'd say we shouldn't hold the event alive, but a good fix her is just to re add the call to nsAccessNodeWrap::ShutdownAccessibility()
Attached patch Patch(v8) (obsolete) — Splinter Review
Attachment #659567 - Flags: review?(trev.saunders)
Comment on attachment 659567 [details] [diff] [review]
Patch(v8)

> nsAccessibilityService::GetApplicationAccessible(nsIAccessible** aAccessibleApplication)
> {
>-  NS_ENSURE_ARG_POINTER(aAccessibleApplication);
>+  if (!aAccessibleApplication)
>+    *aAccessibleApplication = ApplicationAcc();
> 
>-  NS_IF_ADDREF(*aAccessibleApplication = nsAccessNode::GetApplicationAccessible());

that's completely broken.  First it'll only try and do anything if its given a null pointer in which case it will crash, and if it somehow doesn't crash then you'll have reutnred a pointer to the caller who believes you gave them a reference when you didn't.

the way this was in the previous patch was fine, please just revert this to that.
Attachment #659567 - Flags: review?(trev.saunders) → review-
Attached patch Patch(v9)Splinter Review
Attachment #659686 - Flags: review?
Attachment #659686 - Flags: review? → review?(trev.saunders)
Attachment #659567 - Attachment is obsolete: true
(In reply to Trevor Saunders (:tbsaunde) from comment #37)
> Comment on attachment 659567 [details] [diff] [review]
> Patch(v8)
> 
> > nsAccessibilityService::GetApplicationAccessible(nsIAccessible** aAccessibleApplication)
> > {
> >-  NS_ENSURE_ARG_POINTER(aAccessibleApplication);
> >+  if (!aAccessibleApplication)
> >+    *aAccessibleApplication = ApplicationAcc();
> > 
> >-  NS_IF_ADDREF(*aAccessibleApplication = nsAccessNode::GetApplicationAccessible());
> 
> that's completely broken.  First it'll only try and do anything if its given
> a null pointer in which case it will crash, and if it somehow doesn't crash
> then you'll have reutnred a pointer to the caller who believes you gave them
> a reference when you didn't.
> 
> the way this was in the previous patch was fine, please just revert this to
> that.

Hi Trevor,

would you please review my patch and land it?
(In reply to Xi Yang from comment #39)

> > >-  NS_IF_ADDREF(*aAccessibleApplication = nsAccessNode::GetApplicationAccessible());
> > 
> > that's completely broken.  First it'll only try and do anything if its given
> > a null pointer in which case it will crash, and if it somehow doesn't crash
> > then you'll have reutnred a pointer to the caller who believes you gave them
> > a reference when you didn't.
> > 
> > the way this was in the previous patch was fine, please just revert this to
> > that.
> 
> Hi Trevor,
> 
> would you please review my patch and land it?

Xi, you should address Trevor's comment and file a new patch for review I think. I think Trevor wants you to not remove NS_IF_ADDREF line.
(In reply to alexander :surkov from comment #40)
> (In reply to Xi Yang from comment #39)
> 
> > > >-  NS_IF_ADDREF(*aAccessibleApplication = nsAccessNode::GetApplicationAccessible());
> > > 
> > > that's completely broken.  First it'll only try and do anything if its given
> > > a null pointer in which case it will crash, and if it somehow doesn't crash
> > > then you'll have reutnred a pointer to the caller who believes you gave them
> > > a reference when you didn't.
> > > 
> > > the way this was in the previous patch was fine, please just revert this to
> > > that.
> > 
> > Hi Trevor,
> > 
> > would you please review my patch and land it?
> 
> Xi, you should address Trevor's comment and file a new patch for review I
> think. I think Trevor wants you to not remove NS_IF_ADDREF line.

I think I have fixed it in the patch(v9)
Comment on attachment 659686 [details] [diff] [review]
Patch(v9)

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

stealing review from Trevor, I'll fix nits and land the patch. Thank you for the work! Let me know if you need one more bug.

::: accessible/src/base/nsAccessibilityService.cpp
@@ +1209,5 @@
> +  gApplicationAccessible = new ApplicationAccessibleWrap();
> +  
> +  // Addref on create. Will Release in Shutdown()
> +  NS_ADDREF(gApplicationAccessible);
> +  

nit: spaces on empty lines

::: accessible/src/base/nsAccessibilityService.h
@@ +19,5 @@
>  class nsITreeView;
>  
>  namespace mozilla {
>  namespace a11y {
> +class ApplicationAccessible;

nit: new line before
Attachment #659686 - Flags: review?(trev.saunders) → review+
Congrats!
https://hg.mozilla.org/mozilla-central/rev/1b9a3e26763a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 793316
Depends on: 794243
You need to log in before you can comment on or make changes to this bug.