Closed Bug 1133322 Opened 5 years ago Closed 5 years ago

tweak shutdown procedure for accessibles maintaining own trees

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: surkov, Assigned: surkov)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
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
Attachment #8564722 - Flags: review?(yzenevich)
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?
Attachment #8564722 - Flags: review?(yzenevich) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/a58a44e992d2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1368269
You need to log in before you can comment on or make changes to this bug.