920497 - Update closeTab method in tabs.js to prevent wrong index values

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[mario garbi]

Attachment: minimized.js

While working on other issues I have noticed that closeTab() method doesn't work right when we set the aIndex parameter to 0. 
This happens because 0 can have boolean value when used with || and as such we get the default value as can be seen in the minimized testcase attached.

Dump log shows:
 aIndex parameter: 0
 Final value:  defaultValue

 aIndex parameter: 1
 Final value: 1

Comment 1

[mario garbi]

Attachment: indexToString.patch

We can handle this issue by casting the parameter as a string so we won't have any confusions when combined with ||.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 809832 [details] [diff] [review]
indexToString.patch

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

Sorry for highjacking...

::: lib/tabs.js
@@ +189,5 @@
>     *   Index of the tab to close (only used for middleClick)
>     */
>    closeTab : function tabBrowser_closeTab(aEventType, aIndex) {
>      var type = aEventType || "menu";
> +    var index = aIndex.toString() || this.selectedIndex;

No, don't change the datatype of the parameter. Use a check for undefined instead.

Comment 3

[mario garbi]

Attachment: indexToString_v2.patch

Updated the patch to check for 0 value. If the parameter is undefined it will be assigned a default value correctly.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 809861 [details] [diff] [review]
indexToString_v2.patch

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

::: lib/tabs.js
@@ +189,5 @@
>     *   Index of the tab to close (only used for middleClick)
>     */
>    closeTab : function tabBrowser_closeTab(aEventType, aIndex) {
>      var type = aEventType || "menu";
> +    var index = (aIndex === 0) ? aIndex :

In my last review I said that you should check for undefined and not 0. So please do so.

Comment 5

[mario garbi]

Attachment: indexToString_v3.patch

Updated the patch to check if the parameter is undefined and only then set a default value for it. Seems to work fine now with aIndex = 0 and undefined values.

Comment 6

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 810564 [details] [diff] [review]
indexToString_v3.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/a8bb09dbda2a (default)

This needs backporting, please check that.

Comment 7

[mario garbi]

It applies cleanly on all branches as the part of the code that this changes is the same on all branches.

Comment 8

[mario garbi]

I have tested this again on a fresh repository and it applies cleanly up to esr17 with the exception of Aurora branch where it got merged already.

Comment 9

[mario garbi]

Beta:
http://mozmill-crowd.blargon7.com/#/functional/report/ad464590da4913b3dafe381be148419f
http://mozmill-crowd.blargon7.com/#/functional/report/ad464590da4913b3dafe381be1484c88

Release:
http://mozmill-crowd.blargon7.com/#/functional/report/ad464590da4913b3dafe381be14850a6
http://mozmill-crowd.blargon7.com/#/functional/report/ad464590da4913b3dafe381be14852ac

ESR24:
http://mozmill-crowd.blargon7.com/#/functional/report/ad464590da4913b3dafe381be1485b7d
http://mozmill-crowd.blargon7.com/#/functional/report/ad464590da4913b3dafe381be1486f78

Comment 10

[Andrei Eftimie]

Backported:
http://hg.mozilla.org/qa/mozmill-tests/rev/c09701875ffd (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/6b490a93fbca (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/51353c26ed05 (mozilla-esr24)

Nothing critical here, we can safely skip it for ESR17.