276506 - Extra blank tabs not removed when opening bookmark folder in background

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Charles Melhorn]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

When opening a bookmark folder in tabs, unused blank tabs are not removed as
intended when tabs are loaded in the background
(browser.tabs.loadBookmarksInBackground = true). For example, if three blank
tabs are opened by selecting "New Tab" from the File menu three times and a
folder of two bookmarks is opened, the third unused tab should be removed,
regardless of the value of the loadBookmarksInBackground.

I believe the problem is due to a misplaced line in
bookmarks.js::openGroupBookmark():

653       // focus the first tab if prefs say to
654       if (!loadInBackground || doReplace) {
655         // Select the first tab in the group.
656         var tabs = browser.mTabContainer.childNodes;
657         browser.selectedTab = tabs[index0];
658       }
659 
660       // Close any remaining open tabs that are left over.
661       // (Always skipped when we append tabs)
662       for (var i = tabCount-1; i >= index; --i)
663         browser.removeTab(tabs[i]);
664 

The variable "tabs" should be declared outside the if (!loadInBackground ||
doReplace) statement, so that it can still be used in the subsequent call to
browser.removeTab() when loading tabs in background and doReplace is false.



Reproducible: Always

Steps to Reproduce:
See above.

Comment 1

[Phil Ringnalda (:philor)]

Attachment: A little shuffling

Seems like there should maybe be a little more done, since this is one of only
two places in the whole tree where code outside tabbrowser reaches directly in
to mTabContainer.childNodes, but I don't see any reasonable way around it.

Comment 2

[Phil Ringnalda (:philor)]

Attachment: assign outside if

Unrotted, un-lame-indexed, touches nothing but not assigning to a variable
inside an if when we then use it outside the if.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 194593 [details] [diff] [review]
assign outside if

r=vladimir

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Fixed on the trunk.

mozilla/browser/components/bookmarks/content/bookmarks.js; new revision: 1.106;

Comment 5

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change