I just accidently middle-clicked the "Latest headlines" bookmark. All I can say is ouch! 39 tabs were simultaneously loading, and my whole computer hung for a good while. bsmedberg had a good suggestion: we should protect against (accidently) opening too many tabs at once. This would be similar to how we warn before opening a window with existing tabs. So if, for example, the user tries to open 5 or more tabs at once, we would put up a dialog, asking if this is really appropriate. Comments, ideas?

*** Bug 285543 has been marked as a duplicate of this bug. ***

The other option would be to make opening a gajillion tabs at the same time work well (probably by loading them in batches to avoid hammering the system). I heard that someone had taken a look at this in the past, anyone have a bug # handy?

(In reply to comment #2) > The other option would be to make opening a gajillion tabs at the same time > work well (probably by loading them in batches to avoid hammering the system). > I heard that someone had taken a look at this in the past, anyone have a bug # > handy? That doesn't really help if the action was a mistake from the beginning...

Duplicate of bug 253806? (In reply to comment #2) > I heard that someone had taken a look at this in the past, anyone have a bug # > handy? Maybe bug 326735?

*** Bug 253806 has been marked as a duplicate of this bug. ***

Hmm, so without some sort of dialog, I doubt that we could properly interrupt the addition of multiple tabs in an obvious way. Five is way too few though, I'd put the number at more than 15, so its really going to hammer the system real good. Its trivial to implement, really, but not especially a feature, so blocking for radar, b1 for review.

load-balancing tabbrowser stuff to sspitzer

gah. s/.org/.com/!

I remember that gemal's linky would prompt you before opening "too many" tabs, and that tbird would prompt you before opening "too many" messages, with "Opening <x> messages, may be slow. Continue? [ok] [cancel]". so if we do a prompt, it could be like the "warn against closing multiple tabs" prompt, with a check box that matches a pref with UI. we have: "You are about to close <x> open tabs. Are you sure you want to continue? [x] warn me when I attempt to close multiple tabs [close tabs] [cancel]" we could add the other half: "You are about to open <x> tabs. Are you sure you want to continue? [x] warn me when I attempt to open multiple tabs [open tabs] [cancel]" and then add pref UI to the tabs pane.

> I'd put the number at more than 15 I chose 15, following mconnor's comment, but made it a (hidden) pref in all.js. it's hidden as there is no UI to change that value. Since there is a UI change, I'll request a review from beltzner.

Comment on attachment 224393 [details] [diff] [review] a patch, based on existing code (and UI) to warn on closing tabs seeking sr= from ben, too.

Comment on attachment 224393 [details] [diff] [review] a patch, based on existing code (and UI) to warn on closing tabs You get to enjoy my nit-machine. >Index: browser/components/bookmarks/content/bookmarks.js >+ var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService(Components.interfaces.nsIPromptService); This line is really long. Are other lines in this file this long? An alternative is: var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]. getService(Components.interfaces.nsIPromptService); >+ //default to true: if it were false, we wouldn't get this far >+ var warnOnOpen = { value:true }; nit: { value: true }; (space between : and t) >+ var buttonPressed = promptService.confirmEx(window, >+ BookmarksUtils.getLocaleString('tabs.openWarningTitle'), nit: single quotes here, but double quotes elsewhere in file. >+ BookmarksUtils.getLocaleString('tabs.openWarningPromptMe'), ditto >Index: browser/components/preferences/tabs.xul > <preference id="browser.tabs.warnOnClose" name="browser.tabs.warnOnClose" type="bool"/> >+ <preference id="browser.tabs.warnOnOpen" name="browser.tabs.warnOnOpen" type="bool"/> nit: note the space alignment issue. I sorta wish I'd never lined these up this way. >Index: browser/locales/en-US/chrome/browser/preferences/tabs.dtd >+<!ENTITY warnOnOpen.label "Warn when opening multiple tabs"> >+<!ENTITY warnOnOpen.accesskey "o"> For the preferences UI, this is sort of vague. Multiple = 2, but you don't warn (by default) when only 2 tabs are to be opened. Being more specific is probably a good idea here, especially since the sense is slightly different from "warn on close multiple tabs" which will always prompt, regardless of number of tabs. Now, providing the exact number could be considered too much detail (second opinion?) so if that's the case a compromise might be "Warn when opening very many tabs" or "a lot of tabs" (flip side: too vague, but not sure people will care about the detail) I'd like to see this one particular thing resolved in some way (I'll kick the first person who suggests including a text box in prefs to control the .warnOnOpen pref ;-) before marking sr=. The rest looks OK to me nits accounted for.

in addition to addressing the nits in my patch, I can fix those same nits in the existing "warn on close code" code in http://lxr.mozilla.org/mozilla1.8/source/toolkit/content/widgets/tabbrowser.xml#1187 I see your point about the warning. Thinking about it, the wording should imply that this warning has nothing to do with having lots of tab open, it has to do with opening up many tabs at the same time from bookmarks UI. Does that impact how we want to word this? additionally, if I choose a bookmarks folder as my homepage, and the folder has > 15 bookmarks, it will not warn me. This seems good to me, but it is something I hadn't considered.

Maybe [x] Warn me when I open more than [ 5] tabs ?

(In reply to comment #17) > Maybe > > [x] Warn me when I open more than [ 5] tabs I don't see a point in exposing a textfield to set this pref, since the value is arbitrary anyway... I think something deliberately more fuzzy like "Warn when opening many tabs at once" is okay. We're supposed to figure out what "many tabs" is - not push that decision to the user. :-)

Yeah, I suppose that's a good point (and that I should stop commenting on bugs at 3 in the morning). The tradeoffs here are: - too vague can lead to user expectations not being met - too specific can lead to users wanting to control the exact number Ben's idea in comment 15 seems the most sensible, some wording options: [ ] Warn me before opening a lot of tabs at once [ ] Ask me before opening many tabs at once [ ] Warn me before opening enough tabs that I'd regret it

for comparison, here are the strings I'm using in my newest patch (I'll attach it). It might help to see the "open" ones alongside the "close" ones. <!ENTITY warnOnClose.label "Warn when closing multiple tabs"> tabs.closeWarningTitle=Confirm close tabs.closeWarning=This Browser window has %S tabs open. Do you want to close it and all its tabs? tabs.closeButton=Close all tabs tabs.closeWarningPromptMe=Warn me when closing multiple tabs <!ENTITY warnOnOpen.label "Warn when opening a lot of tabs at once"> tabs.openWarningTitle=Confirm open tabs.openWarningMultiple=You are about to open %S tabs. Are you sure you want to open these tabs? tabs.openButtonMultiple=Open tabs tabs.openWarningPromptMe=Warn me when I attempt to open a lot of tabs at once

Comment on attachment 224787 [details] [diff] [review] new patch, addressing ben's comments and the new wording note to ben, I also made some similar nit fixes to tabbrowser.xml (that you asked me to make to bookmarks.js)

Comment on attachment 224787 [details] [diff] [review] new patch, addressing ben's comments and the new wording r=ben@mozilla.org for this.

note, I need to port this functionality back to the trunk to work with places and the other bookmarks changes on the trunk.

(In reply to comment #23) > Created an attachment (id=224788) [edit] > prompt UI (new wording) I should have caught this before. Either make the warning a warning (not a question) or chance the "OK"/"Cancel" to a "Yes"/"No". Personally, I prefer making the warning a statement that explains why the action is deserving of a warning like: "You are about to open %s tabs at once. This might slow your system down as the pages are loading."

(In reply to comment #27) > I should have caught this before. Either make the warning a warning (not a > question) or chance the "OK"/"Cancel" to a "Yes"/"No". [...] Please do not change button titles from the very descriptive "Open tabs"/"Cancel" to a "Yes"/"No" variant though, that'd be one step backwards.

Gah. Yes. Of course. I meant: .-------------------------------------------------------------------. | You have asked to open %s tabs at once. This might slow down your | | system while the pages are loading. | | | | [x] Warn me when I attempt to open lots of tabs at once | | | | ( Open Tabs ) ( Cancel ) | '-------------------------------------------------------------------' or, explicitly: +tabs.openWarningMultiple=You have asked to open %S tabs at once. This might slow down your system while the pages are loading.

beltzner, good point about adding an explanation as to why we are prompting. I'll switch to your version of the warning and attach a final patch (and some new screen shots.) I'm not a wordsmith, so if you know of any (lurking in bugzilla), can you cc them on this bug and let them take a crack at re-wording the warning before the l10n freeze?

I'm usually better than I have been in this particular bug, but I'll get deb to take a peek at it. Although string changes are easy to do after the fact. cc'ing Axel since this'll need some l10n support.

Comment on attachment 224966 [details] [diff] [review] the patch, ported to places (for the trunk) As usual, lots of nits: >Index: browser/components/places/content/controller.js > openLinksInTabs: function PC_openLinksInTabs() { >+ var PREF = >+ Components.classes["@mozilla.org/preferences-service;1"]. >+ getService(Components.interfaces.nsIPrefBranch); lower case "pref" >+ const kWarnOnOpenPref = "browser.tabs.warnOnOpen"; >+ if (PREF.getBoolPref(kWarnOnOpenPref)) >+ { Style in this file is: if (pref.getBoolPref(kWarnOnOpenPref)) { r=ben@mozilla.org with nits fixed.

I've fixed the coding style changes that ben pointed and checked in on the trunk. I also fixed this problem with my patch that I failed to catch before on the trunk: < pref.setBoolPref(kWarnOnOpen, false); --- > pref.setBoolPref(kWarnOnOpenPref, false); and the branch: < PREF.setBoolPref(kWarnOnOpen, false); --- > PREF.setBoolPref(kWarnOnOpenPref, false); yikes! I've fixed that mistake on both branch and trunk.

(In reply to comment #33) > Created an attachment (id=224966) [edit] > the patch, ported to places (for the trunk) > "You have asked to open %S tabs at once. This might slow down your system while the pages are loading." should we really mix "tabs" and "pages"?

this caused Bug 340966

(In reply to comment #36) > should we really mix "tabs" and "pages"? Yes. Pages load in tabs. Opening the tabs themselves isn't slow, it's the action of loading pages within those tabs.

> this caused Bug 340966 I'll look into that QI failure on middle click (on the trunk)

I have a fix in hand for the QI alert, but it pointed out a scenario I have not yet protected against (at least on trunk, I need to look into the branch.) if you multiple select many individual bookmarks and then attempt to open them in tabs, we should warn you (heeding the pref values, of course).

Just FYI, I regularly open folders of bookmarks into tabs with 25 or more inside. The cutoff value IS configurable, right? Is this option on by default?

> The cutoff value IS configurable, right? yes. for details on the hidden pref that control the cutoff, see #340937 > Is this option on by default? yes.

People here may be interested in bug 341046, which I just filed. FWIW, this would be nothing but an annoyance to me, and I immediately turned it off.

"You have asked to open" seems a bit too passive on the part of the user - they *told/ordered/demanded* that Firefox open the tabs. I think "You are about to open" is a better choice of words; for a start it matches "You are about to close".

(In reply to comment #44) > "You have asked to open" seems a bit too passive on the part of the user - they > *told/ordered/demanded* that Firefox open the tabs. I think "You are about to > open" is a better choice of words; for a start it matches "You are about to > close". > good point. perhaps you should open a new bug since this is fixed

(In reply to comment #45) > (In reply to comment #44) > good point. perhaps you should open a new bug since this is fixed Filed bug 342930.

The shown number of tabs that are about to be opened seems to be wrong. When I middle-click click a folder that has subfolders, these subfolders are counted as pages. So it happens that I am warned that 16 tabs will be opened although there are only 10 or so pages.

c.ascheberg@gmx.de, feel free to file a follow-up bug on that issue and make it depend on this one, but commenting in this bug will get you nowhere.

> The shown number of tabs that are about to be opened seems to be wrong. > When I middle-click click a folder that has subfolders, these subfolders are > counted as pages. So it happens that I am warned that 16 tabs will be opened > although there are only 10 or so pages. good catch, thank you. I've logged bug #344040 on this.