737032 - add isValidXULTab() method to ease tab checking

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

We should have some kind of isValidXULTab() method that eases checking if a given XUL tab is still valid. The first place where this can be used is:

https://hg.mozilla.org/integration/fx-team/rev/90d3b1fb284d

That's also how the check should be implemented (without the .pinned condition, of course). When dealing with asynchronous code wrt to tabs we should always check that the tab is still valid. There should already be other places where this check needs to be applied as well.

Comment 1

[Bellindira Castillo [:bellindira]]

Attachment: Added isValidXULTab() method

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 613426 [details] [diff] [review]
Added isValidXULTab() method

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

This looks good but there a couple of places where we should use isValidXULTab() as well:

http://mxr.mozilla.org/mozilla-central/source/browser/components/tabview/tabview.js#63
http://mxr.mozilla.org/mozilla-central/source/browser/components/tabview/ui.js#794
http://mxr.mozilla.org/mozilla-central/source/browser/components/tabview/tabitems.js#242

Please address the comments below, use isValidXULTab() in those other three places and ask me for review again. Thanks!

::: browser/components/tabview/modules/utils.jsm
@@ +637,5 @@
>    },
>  
>    // ----------
> +  // Function: isValidXULTab
> +  // Returns true if the tab is valid.

Please say what a "valid tab" exactly is (i.e. has not been closed, and not been removed from the DOM).

@@ +639,5 @@
>    // ----------
> +  // Function: isValidXULTab
> +  // Returns true if the tab is valid.
> +  isValidXULTab: function Utils_isValidXULTab(xulTab) {
> +    return (!xulTab.closing && xulTab.parentNode) ? true : false;

We can make this even simpler: 

return !xulTab.closing && xulTab.parentNode;

Comment 3

[Bellindira Castillo [:bellindira]]

Attachment: Added isValidXULTab() method to ease tab checking and used it on places where it is useful.

Addressed the latest comments. Thanks!

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 613691 [details] [diff] [review]
Added isValidXULTab() method to ease tab checking and used it on places where it is useful.

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

Thank you! Looks good to me, will push it to try.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Forgot, that my bugzilla and LDAP ID are different :(

https://tbpl.mozilla.org/?tree=Try&rev=8a526b77d8e5

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Try run was green.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/6edf254d7e5b

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/6edf254d7e5b