Last Comment Bug 737032 - add isValidXULTab() method to ease tab checking
: add isValidXULTab() method to ease tab checking
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 14
Assigned To: Bellindira Castillo [:bellindira]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-19 08:00 PDT by Tim Taubert [:ttaubert]
Modified: 2016-04-12 14:00 PDT (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Added isValidXULTab() method (2.18 KB, patch)
2012-04-09 16:12 PDT, Bellindira Castillo [:bellindira]
ttaubert: feedback+
Details | Diff | Review
Added isValidXULTab() method to ease tab checking and used it on places where it is useful. (5.29 KB, patch)
2012-04-10 11:25 PDT, Bellindira Castillo [:bellindira]
ttaubert: review+
Details | Diff | Review

Description Tim Taubert [:ttaubert] 2012-03-19 08:00:43 PDT
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] 2012-04-09 16:12:09 PDT
Created attachment 613426 [details] [diff] [review]
Added isValidXULTab() method
Comment 2 Tim Taubert [:ttaubert] 2012-04-10 05:18:54 PDT
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] 2012-04-10 11:25:21 PDT
Created attachment 613691 [details] [diff] [review]
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] 2012-04-11 02:36:49 PDT
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] 2012-04-11 02:40:16 PDT
Forgot, that my bugzilla and LDAP ID are different :(

https://tbpl.mozilla.org/?tree=Try&rev=8a526b77d8e5
Comment 6 Tim Taubert [:ttaubert] 2012-04-11 07:21:19 PDT
Try run was green.
Comment 7 Tim Taubert [:ttaubert] 2012-04-11 07:28:30 PDT
https://hg.mozilla.org/integration/fx-team/rev/6edf254d7e5b
Comment 8 Tim Taubert [:ttaubert] 2012-04-13 03:24:12 PDT
https://hg.mozilla.org/mozilla-central/rev/6edf254d7e5b

Note You need to log in before you can comment on or make changes to this bug.