779520 - Merge Accessible::Init into constructor

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Bug 779091 made Init() infallible, so there's no sense in having it be a separate function anymore.

Comment 1

[Michał Frontczak [:fxa90id] ♥]

hi, I would like to pick this bug :) or you're already working on it ?

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

It's assigned to nobody, so feel free to assign it to yourself.

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Instructions: in the first comment in the bug, I linked to bug 779091.  My patch there changes Accessible::Init, so you can look at all the files I changed:

https://bug779091.bugzilla.mozilla.org/attachment.cgi?id=647498

The Accessible class itself is declared in accessible/src/generic/Accessible.h, and implemented in accessible/src/generic/Accessible.cpp.  There are also a bunch of subclasses, which you'll find in the other files I modified in that patch: ApplicationAccessibleWrap, ApplicationAccessible, DocAccessible, XULTreeAccessible, and XULTreeGridCellAccessible.  For Accessible and every subclass it has, you'd want to

1) Remove the declaration of Init() from the .h file.

2) Go to the .cpp file where Init() is defined, and move all the code to the end of the constructor for that class (which should be in the same file).

Then try compiling, and the compiler will give an error for every Init(), since it's no longer defined.  Remove all the calls it gives an error for, then try compiling again, and repeat until you get no errors.

Thanks for contributing to Mozilla!  If you have any more questions, please feel free to ask.

Comment 4

[Michał Frontczak [:fxa90id] ♥]

Im bit confuse I cant find accessible/src/generic/Accessible.h everything else is there but no  accessible/src/generic/Accessible.h and .cpp :( I have default tip revision

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

You probably don't have a real checkout, then, or you're looking in the wrong directory.  Try searching for a file named "Accessible.h".  If you can't find it, I'd go on IRC for real-time help, so someone can walk you through your problem.  The easiest way is Mibbit:

http://client00.chat.mibbit.com/

In the drop-down where it says "Mibbit [webirc]", pick "Mozilla [webirc]" instead.  Then enter in a nick (e.g., "fxa90id", or whatever else you like) and enter "#developers" for the channel.  Then click Connect.  Say what your problem is and wait for someone to answer you.  Be patient; it might take a while, and you might have to try again later, depending on who's on.  I'm generally there under the name "AryehGregor", so you can say that to get my attention.  Don't log off if no one answers you, though -- someone might answer half an hour or an hour later, so leave the tab open and check back occasionally.

Comment 6

[Michał Frontczak [:fxa90id] ♥]

Attachment: Merge Accessible::Init into constructor

didnt find Init() in ApplicationAccessibleWrap

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 652798 [details] [diff] [review]
Merge Accessible::Init into constructor

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

Thanks a lot for the patch!  This is exactly the right way to do it -- there are just a couple of minor things to fix.  Once you fix these problems, feel free to ask me for feedback again.  Once the patch is good, you can ask for review by a module peer.  Then you can push your patch to try to make sure it doesn't cause any of our tests to fail.  And once you do that, finally, we can check it into the Mozilla codebase.

So there's one actual problem with your patch:

(In reply to Michał Frontczak :fxa90id from comment #6)
> didnt find Init() in ApplicationAccessibleWrap

It's in accessible/src/atk/ApplicationAccessibleWrap.h, and accessible/src/atk/ApplicationAccessibleWrap.cpp.  You will need to remove it; currently your patch doesn't compile for me.  These files are only used by Linux, which explains why it would have worked for you.

You will have to move the static function toplevel_event_watcher in ApplicationAccessibleWrap.cpp up to above the constructor, to avoid complaints by the compiler that it's not defined.  Currently it's defined after the constructor but before Init(), which is fine, but when you move the code from Init() up to the constructor it won't work anymore.

That's the only real issue I see with the patch.  But also, two stylistic nitpicks:

::: accessible/src/generic/ApplicationAccessible.cpp
@@ +26,5 @@
>  ApplicationAccessible::ApplicationAccessible() :
>    AccessibleWrap(nullptr, nullptr)
>  {
>    mFlags |= eApplicationAccessible;
> +  

You added two spaces here at the end of the line.  Could you remove them?  We prefer not to have whitespace at the end of lines.

::: accessible/src/xul/XULTreeAccessible.cpp
@@ +55,5 @@
>        mFlags |= eAutoCompletePopupAccessible;
>    }
>  
>    mAccessibleCache.Init(kDefaultTreeCacheSize);
> +  

Please remove the added blank line here.

Comment 8

[Michał Frontczak [:fxa90id] ♥]

True, I works on Windows 7 thats why it compiles for me successful :( I am going to fix it asap :)

Comment 9

[Michał Frontczak [:fxa90id] ♥]

Attachment: Merge Accessible::Init into constructor

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 653186 [details] [diff] [review]
Merge Accessible::Init into constructor

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

asking Trevor for feedback

::: accessible/src/atk/ApplicationAccessibleWrap.cpp
@@ +559,5 @@
>  }
>  
> +// ApplicationAccessibleWrap
> +
> +ApplicationAccessibleWrap::ApplicationAccessibleWrap():

nit: add a space before :

@@ +565,3 @@
>  {
> +  MAI_LOG_DEBUG(("======Create AppRootAcc=%p\n", (void*)this));
> +  

nit: space on empty line

Comment 11

[Trevor Saunders (:tbsaunde)]

Comment on attachment 653186 [details] [diff] [review]
Merge Accessible::Init into constructor

iff --git a/accessible/src/xul/XULTreeAccessible.cpp b/accessible/src/xul/XULTreeAccessible.cpp
>--- a/accessible/src/xul/XULTreeAccessible.cpp
>+++ b/accessible/src/xul/XULTreeAccessible.cpp
>@@ -1108,16 +1108,17 @@ XULTreeItemAccessibleBase::GetCellName(n
> 
> XULTreeItemAccessible::
>   XULTreeItemAccessible(nsIContent* aContent, DocAccessible* aDoc,
>                         Accessible* aParent, nsITreeBoxObject* aTree,
>                         nsITreeView* aTreeView, PRInt32 aRow) :
>   XULTreeItemAccessibleBase(aContent, aDoc, aParent, aTree, aTreeView, aRow)
> {
>   mColumn = nsCoreUtils::GetFirstSensibleColumn(mTree);
>+  Name(mCachedName);

I'm not particularly comfortable with calling the classes virtual methods in a constructor.  I believe in this case its "ok", but if anyone ever makes a class inherit from this one and over rides Name() expecting this constructor to call it it won't work.

Making this class and and its decl of Name() MOZ_FINAL would help, but I'm still not sure we wnat to do this.

Surkov thoughts?

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 653186 [details] [diff] [review]
Merge Accessible::Init into constructor

Looks good to me, other than Alexander and Trevor's points.

(In reply to alexander :surkov from comment #10)
> ::: accessible/src/atk/ApplicationAccessibleWrap.cpp
> @@ +559,5 @@
> >  }
> >  
> > +// ApplicationAccessibleWrap
> > +
> > +ApplicationAccessibleWrap::ApplicationAccessibleWrap():
> 
> nit: add a space before :

(He didn't actually touch this line, I don't think -- it just shows up that way in the diff because he moved a big function from below it to above it, so it shows it as moving the constructor/destructor down instead.)

(In reply to Trevor Saunders (:tbsaunde) from comment #11)
> I'm not particularly comfortable with calling the classes virtual methods in
> a constructor.  I believe in this case its "ok", but if anyone ever makes a
> class inherit from this one and over rides Name() expecting this constructor
> to call it it won't work.
> 
> Making this class and and its decl of Name() MOZ_FINAL would help, but I'm
> still not sure we wnat to do this.

Oh, good catch.  I didn't think of that.  It seems like a pity to keep Init() around just for the sake of that, though.  The worst that happens is mCachedName is set to XULTreeItemAccessible::Name() instead of the derived class' Name(), though, right?  Is that really going to be a problem, even assuming someone subclasses and overrides Name()?  I'd think MOZ_FINAL plus a comment should be fine here.

Comment 13

[Michał Frontczak [:fxa90id] ♥]

should I fix something more ?

Comment 14

[Trevor Saunders (:tbsaunde)]

(In reply to :Aryeh Gregor from comment #12)
> Comment on attachment 653186 [details] [diff] [review]
> Merge Accessible::Init into constructor
> 
> Looks good to me, other than Alexander and Trevor's points.
> 
> (In reply to alexander :surkov from comment #10)
> > ::: accessible/src/atk/ApplicationAccessibleWrap.cpp
> > @@ +559,5 @@
> > >  }
> > >  
> > > +// ApplicationAccessibleWrap
> > > +
> > > +ApplicationAccessibleWrap::ApplicationAccessibleWrap():
> > 
> > nit: add a space before :
> 
> (He didn't actually touch this line, I don't think -- it just shows up that
> way in the diff because he moved a big function from below it to above it,
> so it shows it as moving the constructor/destructor down instead.)
> 
> (In reply to Trevor Saunders (:tbsaunde) from comment #11)
> > I'm not particularly comfortable with calling the classes virtual methods in
> > a constructor.  I believe in this case its "ok", but if anyone ever makes a
> > class inherit from this one and over rides Name() expecting this constructor
> > to call it it won't work.
> > 
> > Making this class and and its decl of Name() MOZ_FINAL would help, but I'm
> > still not sure we wnat to do this.
> 
> Oh, good catch.  I didn't think of that.  It seems like a pity to keep
> Init() around just for the sake of that, though.  The worst that happens is
> mCachedName is set to XULTreeItemAccessible::Name() instead of the derived
> class' Name(), though, right?  Is that really going to be a problem, even

according to the c++ fqa 23.5 I believe so, but I haven't looked at the spec.

Other than how bad I feel for the person who breaks the rule and then gets to debug why things aren't working this seems sort of ok to me.

> assuming someone subclasses and overrides Name()?  I'd think MOZ_FINAL plus
> a comment should be fine here.

yeah, I think I'd agree with that although I'd probably mrk both the method and the class as final.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #11)

> I'm not particularly comfortable with calling the classes virtual methods in
> a constructor.  I believe in this case its "ok", but if anyone ever makes a
> class inherit from this one and over rides Name() expecting this constructor
> to call it it won't work.
> 
> Making this class and and its decl of Name() MOZ_FINAL would help, but I'm
> still not sure we wnat to do this.
> 
> Surkov thoughts?

Do GetCellName() instead

Comment 16

[alexander :surkov (:asurkov)]

(In reply to :Aryeh Gregor from comment #12)

> > nit: add a space before :
> 
> (He didn't actually touch this line, I don't think -- it just shows up that
> way in the diff because he moved a big function from below it to above it,
> so it shows it as moving the constructor/destructor down instead.)

if you move the code then you should make sure it's styled properly according to the rule "while you are here". in fact, you don't spend much time but it helps to keep the code consistent.

Comment 17

[Michał Frontczak [:fxa90id] ♥]

:-) Ok I'll fix this :)

Comment 18

[alexander :surkov (:asurkov)]

Trev, anything else than comment 15, comment 16?

Comment 19

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #18)
> Trev, anything else than comment 15, comment 16?

I'd love to see the atk ApplicationAccessibleWrap stuff get cleaned up some day, but seems fine with those two things from before fixed.

Comment 20

[alexander :surkov (:asurkov)]

Michał, it seems the patch is almost ready to go. It'd be nice if you find a minute to finish it.

Comment 21

[Michał Frontczak [:fxa90id] ♥]

Attachment: Merge Accessible::Init into constructor

Comment 22

[Trevor Saunders (:tbsaunde)]

Comment on attachment 671098 [details] [diff] [review]
Merge Accessible::Init into constructor

So, uhm, nobody ever checked that the browser even starts with a11y active with any of these patches, because it doesn't.

What happens is this we go to create a root accessible during the constructor we now call DocAccessible::AddListeners() which calls RootAccessible() which ends up trying to create the root accessible we're already creatting because we can't find it in the doc accessible cache.  So we end up recursing for ever.

I haven't though much about the right way to fix this yet.  One solution would be to move GetDocAccessible(nsIPresShell*) to use the DocAccessible we've cached on the pres shell now.  As it turns out we have set that pointer to point at the existing doc accessible so it would work, but feels a bit to sneaky / fragile.

Comment 23

[alexander :surkov (:asurkov)]

it seems the correct fix is bug 678477. We call RootAccessible() in DocAccessible::AddEventListeners() to add a caret listener.

Comment 24

[Michał Frontczak [:fxa90id] ♥]

Should I do something more here ? Or can I start 678477 ?

Comment 25

[alexander :surkov (:asurkov)]

(In reply to Michał Frontczak :fxa90id from comment #24)
> Should I do something more here ? Or can I start 678477 ?

if you can take bug 678477 then it's awesome. Let's wait Trev when he wakes up to get his feedback.

Comment 26

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #23)
> it seems the correct fix is bug 678477. We call RootAccessible() in
> DocAccessible::AddEventListeners() to add a caret listener.

I think that should fix it too, but it seems like significantly more work, and we should convert nsAcceDocManager to use the pres shell cahce anyway so I might just do that for now while we fix Caret Accessible

Comment 27

[alexander :surkov (:asurkov)]

I'm not really sure how doc accessible per pres shell can help since if understood right then we need to have root accessible when we create a root accessible.

Trev, please comment bug 678477.

Comment 28

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #27)
> I'm not really sure how doc accessible per pres shell can help since if
> understood right then we need to have root accessible when we create a root
> accessible.

so, the trick is that we have one, we just haven't ut it in the has table yet.

More specifically the order of events currently is this
DocAccessible constructor is called
doc acc constructor does aPresShell->SetDocumentAccessible(this);
then doc accessible constructor finishes
nsAccDocManager adds mapping for document to mDocAccessibleCache
nsAccDocManager calls DocAccessible::Init() which calls AddEventListeners() which ends up looking itself up in mDocAccessibleCache

This patch moves the call to AddEventListeners() to before the doc acc is added to mDocAccessibleCaceh however it puts it after aPresShell->SetDocumentAccessible()

so if we modify nsAccDocManager::GetDocAccessible(nsIPresShell*) which RootAccessible() calls to be
DocAccessible* doc = aPresShell->GetDOcumentAccessible();
if (doc)
  return doc;

return GetDocAccessible(aPresShell->GetDOcument());
then in this case aPresShell->GetDocumentAccessible() will succeed igving us a doc acc, and we won't try to create another root accessible.

> Trev, please comment bug 678477.

I'm not sure what you want me to say there.

Comment 29

[Trevor Saunders (:tbsaunde)]

(In reply to Trevor Saunders (:tbsaunde) from comment #28)
> (In reply to alexander :surkov from comment #27)
> > I'm not really sure how doc accessible per pres shell can help since if
> > understood right then we need to have root accessible when we create a root
> > accessible.
> 
> so, the trick is that we have one, we just haven't ut it in the has table
> yet.
> 
> More specifically the order of events currently is this
> DocAccessible constructor is called
> doc acc constructor does aPresShell->SetDocumentAccessible(this);
> then doc accessible constructor finishes
> nsAccDocManager adds mapping for document to mDocAccessibleCache
> nsAccDocManager calls DocAccessible::Init() which calls AddEventListeners()
> which ends up looking itself up in mDocAccessibleCache
> 
> This patch moves the call to AddEventListeners() to before the doc acc is
> added to mDocAccessibleCaceh however it puts it after
> aPresShell->SetDocumentAccessible()
> 
> so if we modify nsAccDocManager::GetDocAccessible(nsIPresShell*) which
> RootAccessible() calls to be
> DocAccessible* doc = aPresShell->GetDOcumentAccessible();
> if (doc)
>   return doc;
> 
> return GetDocAccessible(aPresShell->GetDOcument());
> then in this case aPresShell->GetDocumentAccessible() will succeed igving us
> a doc acc, and we won't try to create another root accessible.

of course this isn't as easy as I thought because GetDocumentAccessible() was part of bug 777603 which we could never agree on so you'd need to add that api.

Comment 30

[Michał Frontczak [:fxa90id] ♥]

now Im getting little cofuse, should I fix something or change ? whats about bug 678477 ?

Comment 31

[Trevor Saunders (:tbsaunde)]

(In reply to Michał Frontczak :fxa90id from comment #30)
> now Im getting little cofuse, should I fix something or change ? whats about
> bug 678477 ?

Well, I landed what I suggested in comment 26, so now the browser starts, but there's another bug in your patch, particularly DocAccessible::DocAccessible() now calls AddEventListeners() which is virtual, which as we discussed before in this bug doesn't really work from constructors.  In this case RootAccessible::AddEventListeners() never gets called, just the version on DocAccessible.  So you'll need to figure out a way to have the overload on RootAccessible called if the object being constructed is actually a root accessible, or something.

Comment 32

[Trevor Saunders (:tbsaunde)]

Comment on attachment 671098 [details] [diff] [review]
Merge Accessible::Init into constructor

> nsAccessibilityService::Init()
> {
>-  // Initialize accessible document manager.
>-  if (!nsAccDocManager::Init())
>-    return false;

and before you work on that, you should revert this changes because its wrong and means a11y just won't ever do anything.

Comment 33

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #31)

> Well, I landed what I suggested in comment 26, so now the browser starts,
> but there's another bug in your patch, particularly
> DocAccessible::DocAccessible() now calls AddEventListeners() which is
> virtual, which as we discussed before in this bug doesn't really work from
> constructors.  In this case RootAccessible::AddEventListeners() never gets
> called, just the version on DocAccessible.  So you'll need to figure out a
> way to have the overload on RootAccessible called if the object being
> constructed is actually a root accessible, or something.

What about to move AddEventListenrs code under constructor?

Comment 34

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #33)
> (In reply to Trevor Saunders (:tbsaunde) from comment #31)
> 
> > Well, I landed what I suggested in comment 26, so now the browser starts,
> > but there's another bug in your patch, particularly
> > DocAccessible::DocAccessible() now calls AddEventListeners() which is
> > virtual, which as we discussed before in this bug doesn't really work from
> > constructors.  In this case RootAccessible::AddEventListeners() never gets
> > called, just the version on DocAccessible.  So you'll need to figure out a
> > way to have the overload on RootAccessible called if the object being
> > constructed is actually a root accessible, or something.
> 
> What about to move AddEventListenrs code under constructor?

I think that would work.

Comment 35

[Michał Frontczak [:fxa90id] ♥]

great I'll try to make this asap :)

Comment 36

[Trevor Saunders (:tbsaunde)]

Attachment: fixed and rebased patch

Comment 37

[alexander :surkov (:asurkov)]

Comment on attachment 682873 [details] [diff] [review]
fixed and rebased patch

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

::: accessible/src/atk/ApplicationAccessibleWrap.cpp
@@ +519,5 @@
>  ApplicationAccessibleWrap::ApplicationAccessibleWrap():
>    ApplicationAccessible()
>  {
>    MAI_LOG_DEBUG(("======Create AppRootAcc=%p\n", (void*)this));
> +  

nit: whitespace

@@ +520,5 @@
>    ApplicationAccessible()
>  {
>    MAI_LOG_DEBUG(("======Create AppRootAcc=%p\n", (void*)this));
> +  
> +  if (ShouldA11yBeEnabled()) {

it's funny name since a11y is enabled already, perhaps ShouldPlatformA11yBeEnabled is nicer.

@@ +521,5 @@
>  {
>    MAI_LOG_DEBUG(("======Create AppRootAcc=%p\n", (void*)this));
> +  
> +  if (ShouldA11yBeEnabled()) {
> +      // load and initialize gail library

nit: upppercase it and dot in the end and for comments below please

@@ +528,5 @@
> +          (*sGail.init)();
> +      }
> +      else {
> +          MAI_LOG_DEBUG(("Fail to load lib: %s\n", sGail.libName));
> +      }

nit: no braces around if/else and below

@@ +530,5 @@
> +      else {
> +          MAI_LOG_DEBUG(("Fail to load lib: %s\n", sGail.libName));
> +      }
> +
> +      MAI_LOG_DEBUG(("Mozilla Atk Implementation initializing\n"));

nit: new line after this please

@@ +532,5 @@
> +      }
> +
> +      MAI_LOG_DEBUG(("Mozilla Atk Implementation initializing\n"));
> +      // Initialize the MAI Utility class
> +      // it will overwrite gail_util

nit: form it to 80 chars line and dot in the end and.

@@ +541,5 @@
> +
> +      // load and initialize atk-bridge library
> +      rv = LoadGtkModule(sAtkBridge);
> +      if (NS_SUCCEEDED(rv)) {
> +          // init atk-bridge

nit: remove this comment since it dupes the comment above

::: accessible/src/generic/DocAccessible.cpp
@@ +122,5 @@
> +  // having no container was loaded.
> +  if (mDocument->GetReadyStateEnum() == nsIDocument::READYSTATE_COMPLETE)
> +    mLoadState |= eDOMLoaded;
> +
> +  // 1) Set up scroll position listener

nit: it's obsolette comment, scroll listener is added at this point already

@@ +123,5 @@
> +  if (mDocument->GetReadyStateEnum() == nsIDocument::READYSTATE_COMPLETE)
> +    mLoadState |= eDOMLoaded;
> +
> +  // 1) Set up scroll position listener
> +  // 2) Check for editor and listen for changes to editor

nit: remove 2) and add do in the end, and move it where appropriate

@@ +126,5 @@
> +  // 1) Set up scroll position listener
> +  // 2) Check for editor and listen for changes to editor
> +  nsCOMPtr<nsISupports> container = mDocument->GetContainer();
> +  nsCOMPtr<nsIDocShellTreeItem> docShellTreeItem(do_QueryInterface(container));
> +  NS_ENSURE_TRUE_VOID(docShellTreeItem);

might be crash better? It should never happen.

@@ +129,5 @@
> +  nsCOMPtr<nsIDocShellTreeItem> docShellTreeItem(do_QueryInterface(container));
> +  NS_ENSURE_TRUE_VOID(docShellTreeItem);
> +
> +  // Make sure we're a content docshell
> +  // We don't want to listen to chrome progress

nit: change the comment to something more sensible, perhaps it makes sense to combine with a comment below ("We're not an editor yet").

@@ +135,5 @@
> +  docShellTreeItem->GetItemType(&itemType);
> +
> +  bool isContent = (itemType == nsIDocShellTreeItem::typeContent);
> +
> +  if (isContent) {

nit: no empty line between these lines

@@ +140,5 @@
> +    // We're not an editor yet, but we might become one
> +    nsCOMPtr<nsICommandManager> commandManager = do_GetInterface(docShellTreeItem);
> +    if (commandManager) {
> +      commandManager->AddCommandObserver(this, "obs_documentCreated");
> +    }

nit: no curves around it

@@ +145,5 @@
> +  }
> +
> +  nsCOMPtr<nsIDocShellTreeItem> rootTreeItem;
> +  docShellTreeItem->GetRootTreeItem(getter_AddRefs(rootTreeItem));
> +  if (rootTreeItem) {

we don't need this check

@@ +149,5 @@
> +  if (rootTreeItem) {
> +    a11y::RootAccessible* rootAccessible = RootAccessible();
> +
> +    // XXX we might be the root accessible in which case eRootAccessible hasn't
> +    // been added to mFlags yet, and so the downcast failed.

nit: "so the downcast fails and RootAccessible() returns null", otherwise it's not clear.

since this code is temporary then I'm fine with it. It makes sense to add a reference to bug 678477 which should fix it.

@@ +157,5 @@
> +        caretAccessible->AddDocSelectionListener(mPresShell);
> +    }
> +  }
> +
> +  // add document observer

nit: uppercase and dot in the end

@@ -764,1 @@
>  DocAccessible::RemoveEventListeners()

why wouldn't do the same for RemoveEventListenrs to have symmetrical code?

::: accessible/src/generic/RootAccessible.cpp
@@ +47,5 @@
>  
>  using namespace mozilla;
>  using namespace mozilla::a11y;
>  
> +const char* const docEvents[] = {

nit: kEventTypes?

@@ +50,5 @@
>  
> +const char* const docEvents[] = {
> +#ifdef DEBUG_DRAGDROPSTART
> +  // Capture mouse over events and fire fake DRAGDROPSTART event to simplify
> +  // debugging a11y objects with event viewers

nit: dot in the end

@@ +53,5 @@
> +  // Capture mouse over events and fire fake DRAGDROPSTART event to simplify
> +  // debugging a11y objects with event viewers
> +  "mouseover",
> +#endif
> +  // capture Form change events 

nit: whitespace in the end

perhaps: Fired whenever list or tree control selection is changed.

@@ +55,5 @@
> +  "mouseover",
> +#endif
> +  // capture Form change events 
> +  "select",
> +  // capture ValueChange events (fired whenever value changes, immediately after, whether focus moves or not)

perhaps: Fired whenever value changes, immediate ...

break if longer 80 chars

@@ +57,5 @@
> +  // capture Form change events 
> +  "select",
> +  // capture ValueChange events (fired whenever value changes, immediately after, whether focus moves or not)
> +  "ValueChange",
> +  // capture AlertActive events (fired whenever alert pops up)

remove that 'capture AlertActive events', it doesn't see uselfull and same below

@@ +102,5 @@
> +                   * const* e_end = ArrayEnd(docEvents);
> +         e < e_end; ++e) {
> +      nsresult rv = nstarget->AddEventListener(NS_ConvertASCIItoUTF16(*e),
> +                                               this, true, true, 2);
> +      NS_ENSURE_SUCCESS_VOID(rv);

ignore rv at all

@@ +107,5 @@
> +    }
> +  }
> +
> +  mCaretAccessible = new nsCaretAccessible(this);
> +  mCaretAccessible->AddDocSelectionListener(aPresShell);

might be worth to comment that DocAccessible fails to add selection listener in case of root accessible so we add it here

::: accessible/src/xul/XULTreeGridAccessible.cpp
@@ +471,5 @@
>    mFlags |= eSharedNode;
> +
> +  NS_ASSERTION(mTreeView, "mTreeView is null");
> +
> +  int16_t type;

pls initialize it with something

Comment 38

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39890b75361f

Comment 39

[Trevor Saunders (:tbsaunde)]

> @@ +520,5 @@
> >    ApplicationAccessible()
> >  {
> >    MAI_LOG_DEBUG(("======Create AppRootAcc=%p\n", (void*)this));
> > +  
> > +  if (ShouldA11yBeEnabled()) {
> 
> it's funny name since a11y is enabled already, perhaps
> ShouldPlatformA11yBeEnabled is nicer.

I'm fine however, file a bug if you like?  I feel like there was some reason for that name but don't remember.

> @@ +530,5 @@
> > +      else {
> > +          MAI_LOG_DEBUG(("Fail to load lib: %s\n", sGail.libName));
> > +      }
> > +
> > +      MAI_LOG_DEBUG(("Mozilla Atk Implementation initializing\n"));
> 
> nit: new line after this please

btw I plan to remove all this debug stuff since I'm pretty sure nobodies used in years.

> @@ -764,1 @@
> >  DocAccessible::RemoveEventListeners()
> 
> why wouldn't do the same for RemoveEventListenrs to have symmetrical code?

I'm not sure exactly where it should go off hand probably Shutdown().  In general it sounds like a good idea for a spin off since the AddEventListeners() change was just to make other stuff work.

> > +const char* const docEvents[] = {
> > +#ifdef DEBUG_DRAGDROPSTART
> > +  // Capture mouse over events and fire fake DRAGDROPSTART event to simplify
> > +  // debugging a11y objects with event viewers
> 
> nit: dot in the end


btw, do you use it for anything? if not any reason we shouldn't remove it?

Comment 40

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/fb6df9f4a6a2 - Windows was leaking a la https://tbpl.mozilla.org/php/getParsedLog.php?id=17190295&tree=Mozilla-Inbound ("leaked 9223372036854970900 bytes during test execution," pretty impressive!).

Comment 41

[Trevor Saunders (:tbsaunde)]

(In reply to Phil Ringnalda (:philor) from comment #40)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/fb6df9f4a6a2 -
> Windows was leaking a la
> https://tbpl.mozilla.org/php/getParsedLog.php?id=17190295&tree=Mozilla-
> Inbound ("leaked 9223372036854970900 bytes during test execution," pretty
> impressive!).

so I'm pretty sure what happens here is that the RootAccessible constructor calls AddEventListener() which wants an owning ref to the listener.  SO the AddRef() is logged for DocAccessible and the release for RootAccessible or maybe RootAccessible and RootAccesisbleWrap when that class exists.

I think the only real reasonable solution here is to restore Init() atleast for RootAccessibles, and probably  for DocAccessibles too.

Comment 42

[alexander :surkov (:asurkov)]

Ok, so not virtual calls in constructor. Agree, let's get back Init() for documents. They are special so that should be ok.

Comment 43

[alexander :surkov (:asurkov)]

btw, since you fixed those nits from Init() functions please keep them fixed :)

Comment 44

[Trevor Saunders (:tbsaunde)]

checked restoring doc accessible Init() fixed the issue locally and pushed again as https://hg.mozilla.org/integration/mozilla-inbound/rev/16777b16f524

Comment 45

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/16777b16f524