add isValidXULTab() method to ease tab checking

RESOLVED FIXED in Firefox 14

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: ttaubert, Assigned: bellindira)

Tracking

Trunk
Firefox 14

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
Assignee: nobody → bellindira
(Assignee)

Comment 1

5 years ago
Created attachment 613426 [details] [diff] [review]
Added isValidXULTab() method
Attachment #613426 - Flags: review?(ttaubert)
(Reporter)

Comment 2

5 years ago
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;
Attachment #613426 - Flags: review?(ttaubert) → feedback+
(Assignee)

Comment 3

5 years ago
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!
Attachment #613691 - Flags: review?(ttaubert)
(Reporter)

Updated

5 years ago
Attachment #613426 - Attachment is obsolete: true
(Reporter)

Comment 4

5 years ago
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.
Attachment #613691 - Flags: review?(ttaubert) → review+
(Reporter)

Updated

5 years ago
Status: NEW → ASSIGNED
Whiteboard: [autoland-try:-b do -p linux,linux64,macosx64,win32,macosx -u mochitest-o -t none]
(Reporter)

Comment 5

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

https://tbpl.mozilla.org/?tree=Try&rev=8a526b77d8e5
Whiteboard: [autoland-try:-b do -p linux,linux64,macosx64,win32,macosx -u mochitest-o -t none]
(Reporter)

Comment 6

5 years ago
Try run was green.
(Reporter)

Comment 7

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/6edf254d7e5b
Whiteboard: [fixed-in-fx-team]
(Reporter)

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/6edf254d7e5b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 14
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.