767754 - Move ApplicationAccessible logic from nsAccessNode to nsAccessibilityService

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Mark Capella [:capella]]

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()

Comment 1

[Mark Capella [:capella]]

nsAccessNode::Init() should just move to Accessible::Init()

Comment 2

[Trevor Saunders (:tbsaunde)]

(In reply to Mark Capella [:capella] from comment #1)
> nsAccessNode::Init() should just move to Accessible::Init()

that should be its own bug.

Comment 3

[Mark Capella [:capella]]

Disregard my comment 1 ... thats now Bug 767757 - Move nsAccessNode::Init() to Accessible::Init()

Comment 4

[Xi Yang]

Attachment: Patch_v1

the patch does not compile... just want someone to help

Comment 5

[Mark Capella [:capella]]

Comment on attachment 643869 [details] [diff] [review]
Patch_v1

More appropriate to ask your mentor - over to Trev...

Comment 6

[Trevor Saunders (:tbsaunde)]

(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 7

[Trevor Saunders (:tbsaunde)]

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.

Comment 8

[alexander :surkov (:asurkov)]

Please give overview of the idea. Turning the accessibility service into application accessible looks new.

Comment 9

[Trevor Saunders (:tbsaunde)]

(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.

Comment 10

[alexander :surkov (:asurkov)]

ok (the patch confused me)

Comment 11

[Xi Yang]

Attachment: Patch_v2

Comment 12

[Trevor Saunders (:tbsaunde)]

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.

Comment 13

[Xi Yang]

Attachment: Patch(v3)

Comment 14

[Trevor Saunders (:tbsaunde)]

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.

Comment 15

[Xi Yang]

Attachment: Patch(v4)

Comment 16

[Trevor Saunders (:tbsaunde)]

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.

Comment 17

[Xi Yang]

can anyone help me to change the keyword?

Comment 18

[Mark Capella [:capella]]

Xi - you just want it "checkin-needed" ?

Comment 19

[Xi Yang]

(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"

Comment 20

[Mark Capella [:capella]]

Attachment: Patch - Rebased

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

Comment 21

[Mark Capella [:capella]]

btw- use this version of the patch to start ... I tweaked yours due to .rej file applying to current code ...

Comment 22

[Xi Yang]

Attachment: Patch(v5)

Comment 23

[Xi Yang]

Attachment: Patch(v5)

Comment 24

[Xi Yang]

(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!

Comment 25

[Mark Capella [:capella]]

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.

Comment 26

[Trevor Saunders (:tbsaunde)]

(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.

Comment 27

[Xi Yang]

Attachment: Patch(v6)

Comment 28

[Trevor Saunders (:tbsaunde)]

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.

Comment 29

[Xi Yang]

Attachment: Patch(v7)

Comment 30

[Trevor Saunders (:tbsaunde)]

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.

Comment 31

[Xi Yang]

(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?

Comment 32

[Trevor Saunders (:tbsaunde)]

I just landed it for you
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d4d0dc404b8

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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)

Comment 34

[alexander :surkov (:asurkov)]

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

Comment 35

[Trevor Saunders (:tbsaunde)]

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()

Comment 36

[Xi Yang]

Attachment: Patch(v8)

Comment 37

[Trevor Saunders (:tbsaunde)]

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.

Comment 38

[Xi Yang]

Attachment: Patch(v9)

Comment 39

[Xi Yang]

(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?

Comment 40

[alexander :surkov (:asurkov)]

(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.

Comment 41

[Xi Yang]

(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 42

[alexander :surkov (:asurkov)]

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

Comment 43

[Mark Capella [:capella]]

Congrats!

Comment 44

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b9a3e26763a

Comment 45

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/1b9a3e26763a