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 | Splinter 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 | Splinter Review

Description User image 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 User image Bellindira Castillo [:bellindira] 2012-04-09 16:12:09 PDT
Created attachment 613426 [details] [diff] [review]
Added isValidXULTab() method
Comment 2 User image 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 User image 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 User image 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 User image 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 User image Tim Taubert [:ttaubert] 2012-04-11 07:21:19 PDT
Try run was green.
Comment 7 User image Tim Taubert [:ttaubert] 2012-04-11 07:28:30 PDT
https://hg.mozilla.org/integration/fx-team/rev/6edf254d7e5b
Comment 8 User image 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.