1133322 - tweak shutdown procedure for accessibles maintaining own trees

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

1) we shouldn't attempt to shutdown the accessible tree twice during document shutdown
2) XUL trees have to unbind their children from document for shutdown

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8564722 [details] [diff] [review]
patch

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

Thanks, just some nits

::: accessible/tests/mochitest/common.js
@@ +212,5 @@
>  
>    var elm = null;
>  
>    if (aAccOrElmOrID instanceof nsIAccessible) {
> +    try { elm = aAccOrElmOrID.DOMNode; } catch(e) { }

maybe in catch clause: info("defunct accessible..");

::: accessible/tests/mochitest/states.js
@@ +202,5 @@
> + * Fails if no defunct state on the accessible.
> + */
> +function testIsDefunct(aAccessible, aTestName)
> +{
> +  var id = prettyName(aAccessible) + (aTestName ? " [" + aTestName + "]": "");

Nit: space before :

::: accessible/tests/mochitest/treeupdate/test_shutdown.xul
@@ +33,5 @@
> +
> +      this.invoke = function loadXULTree_invoke()
> +      {
> +        this.treeNode.view = aTreeView;
> +      }

Nit: ;

@@ +38,5 @@
> +
> +      this.getID = function loadXULTree_getID()
> +      {
> +        return "Load XUL tree " + prettyName(aTreeID);
> +      }

Nit: ;

@@ +48,5 @@
> +      this.lastItem = null;
> +
> +      this.eventSeq = [
> +        new invokerChecker(EVENT_REORDER, document)
> +      ]

Nit: ;

@@ +55,5 @@
> +      {
> +        this.lastItem = getAccessible(aID).lastChild;
> +        this.lastCell = this.lastItem.lastChild;
> +        getNode(aID).parentNode.removeChild(getNode(aID));
> +      }

Nit: ;

@@ +61,5 @@
> +      this.check = function check(aEvent)
> +      {
> +        testIsDefunct(this.tree, aID);
> +        testIsDefunct(this.lastItem, "last item of " + aID);
> +        if (this.lastCell)

Nit: if (...) {...}

@@ +63,5 @@
> +        testIsDefunct(this.tree, aID);
> +        testIsDefunct(this.lastItem, "last item of " + aID);
> +        if (this.lastCell)
> +          testIsDefunct(this.lastCell, "last item cell of " + aID);
> +      }

Nit: ;

@@ +67,5 @@
> +      }
> +      this.getID = function getID()
> +      {
> +        return "Remove tree from DOM";
> +      }

Nit: ;

::: accessible/tests/mochitest/treeview.js
@@ +59,5 @@
>  
>  function nsTreeTreeView()
>  {
>    this.__proto__ = new nsTreeView();
> +

Nit: this file wasn't really touch. Maybe we should leave it out?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Yura Zenevich [:yzen] from comment #1)
> >    if (aAccOrElmOrID instanceof nsIAccessible) {
> > +    try { elm = aAccOrElmOrID.DOMNode; } catch(e) { }
> 
> maybe in catch clause: info("defunct accessible..");

as we chatted in persons: this is a generic code that can be called for defunct accessible testing, no reports are needed

> >  {
> >    this.__proto__ = new nsTreeView();
> > +
> 
> Nit: this file wasn't really touch. Maybe we should leave it out?

if you don't mind I'll leave it out, I guess I touched it when played with the patch

Comment 3

[Ryan VanderMeulen [:RyanVM]]

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