The platform allows us to do per tab network prioritization, so why not. It make sense. From what I understand, network events are in a prioritized queue and so by adjusting priority on a tab's loadgroup, we can force network events for that tab to the top of the queue. The idea is better outlined on the linked wiki page, but the jist is that focused tabs & windows would get higher priority. This will really only be noticeable during session restore & when opening a bookmark group. (Tabbed Browser is my best fit component. I've actually implemented this as another component for now)

Here's the patch in progress. It depends on the patch for bug 511503. I implemented it as a component (using session store as a model). This isn't necessarily the best way to do it. I can think of a few improvements off the top of my head. No tests, but it seems to be working. In other words, a work in progress :) Try server builds coming up at https://build.mozilla.org/tryserver-builds/poshannessy@mozilla.com-try-11a9e78a8cb5/

Paul, do you have any ETA when this enhancement will be ready? I suspect post beta1?

(In reply to comment #4) This has fallen behind due to some other things, so chances are after beta1.

Uploading my current WIP. It's been drastically reorganized. Changes: * Made a JS module after some feedback from Dietrich - Less XPCOM is a good thing - It's only being used from JS (specifically browser.js) * Added a pref for easy disabling, along with methods that can be called directly There's still a bunch of debug code in there and cleanup to do, but I want eyeballs on this sooner rather than later. I'm still figuring out who needs to be on for the actual review so until then feel free to do some fly-by reviews.

Changes based on some feedback from sdwilsh offline. Updated to take more out of the exported object & some additional cleanup.

Fixed a couple nits. Poking dolske for prereview since he's doing the same to me.

What's the point of watching domwindowopened if each window calls init?

Updated the observer/event stuff. Put all the topics/events in the same place to make it easier to keep track of, change. (In reply to comment #9) > What's the point of watching domwindowopened if each window calls init? Yea there isn't really. I had it like that because it was leftover from writing it as an extension. Dão - if you have time and want to do a more thorough review, please do.

Comment on attachment 403912 [details] [diff] [review] Patch v0.5 (WIP) >+// Turn the network prioritizer on >+pref("browser.networkPrioritizer.enabled", true); Do we really need this? There's a lot code in the module that could be removed depending on this. Seems like if we wanted to disable it because it caused problems, we could just uncomment one line in browser.js. >+ uninit: function() { >+ // Remove observers >+ // XXX what if removeObserver failed? >+ Array.forEach(OBSERVING, function(topic) { >+ _observerService.removeObserver(NPObserver, topic, true); >+ }); s/OBSERVING/OBSERVER_TOPICS/ >+var TabHelper = { >+ onOpen: function(aWindow, aBrowser) { >+ // Just set it's priority to that of the window. We'll get TabSelect >+ // after this if it's an open+select. >+ TabHelper._setPriority(aBrowser, aWindow.__NP.basePriority); s/_setPriority/setPriority/ This could use aBrowser.ownerDocument.defaultView and get rid of aWindow. >+ Array.forEach(aWindow.gBrowser.browsers, function(browser) { aWindow.gBrowser.browsers.forEach ... >+ return aBrowser.webNavigation.QueryInterface(Ci.nsIDocumentLoader). >+ loadGroup.QueryInterface(Ci.nsISupportsPriority).priority; the trailing dot belongs on the beginning of the second line >+var WindowHelper = { >+ onLoad: function(aWindow) { >+ // is it a browser window? are we already tracking it? >+ if (!WindowHelper.isNavigator(aWindow) || aWindow.__NP) >+ return; Not needed, the caller is responsible for this. >+ // add event listeners >+ Array.forEach(BROWSER_EVENTS, function(event) { >+ aWindow.gBrowser.addEventListener(event, NPObserver, true); >+ }); I'd call this TAB_EVENTS and add the event listener to gBrowser.tabContainer >+ Array.forEach(WINDOW_EVENTS, function(event) { >+ aWindow.addEventListener(event, NPObserver, true); >+ }); WINDOW_EVENTS.forEach... >+ onUnload: function(aWindow) { >+ // Don't try to unload tracked windows >+ if (!aWindow.__NP) >+ return; This shouldn't happen. >+ // Stop tracking >+ aWindow.__NP = null; >+ >+ // Remove the event listeners >+ aWindow.gBrowser.removeEventListener("TabOpen", NPObserver, true); >+ aWindow.gBrowser.removeEventListener("TabSelect", NPObserver, true); >+ aWindow.removeEventListener("activate", NPObserver, true); You should use the constants here. >+ onActivate: function(aWindow) { >+ // is it a browser window? >+ if (!WindowHelper.isNavigator(aWindow)) >+ return; This shouldn't happen either. >+ // This shouldn't happen. Fix & log it if it does. >+ if (!aWindow.__NP) { >+ log("FOCUS happened before LOAD"); >+ WindowHelper.onLoad(aWindow); >+ } And I wouldn't try to fix things that shouldn't happen. >+ // If we have a last focused window, then we should change it's priority. >+ // This doesn't quite belong here, but that's ok. >+ if (aFocus == "focused" && _lastFocusedWindowID && >+ _lastFocusedWindowID != aWindow.__NP.id) { >+ let lastFocusedWindow = null; >+ let windowsEnum = _windowMediator.getEnumerator("navigator:browser"); >+ while (windowsEnum.hasMoreElements()) { >+ let window = windowsEnum.getNext(); >+ if (window.__NP && window.__NP.id == _lastFocusedWindowID) >+ lastFocusedWindow = window; >+ } >+ _lastFocusedWindowID = null; Why don't you store _lastFocusedWindow instead of _lastFocusedWindowID? You should probably listen to unload events of tracked windows, remove all event listeners and set _lastFocusedWindow to null if it's that window. >+ WindowHelper.setPriority(lastFocusedWindow, "unfocused"); You should use constants instead of "unfocused", "active" etc. >--- a/browser/base/content/browser.js >+++ b/browser/base/content/browser.js >+__defineGetter__("NetworkPrioritizer", function() { >+ Cu.import("resource://gre/modules/NetworkPrioritizer.jsm"); >+ return this.NetworkPrioritizer; >+}); >+__defineSetter__("NetworkPrioritizer", function(val) { >+ delete this.NetworkPrioritizer; >+ return this.NetworkPrioritizer = val; >+}); This isn't needed. Just do this: let temp = {}; Cu.import("resource://gre/modules/NetworkPrioritizer.jsm", temp); temp.NetworkPrioritizer.init(window);

Everything from comment #11. I had actually already done a bunch of those things locally :) > >+pref("browser.networkPrioritizer.enabled", true); > > Do we really need this? There's a lot code in the module that could be removed > depending on this. Seems like if we wanted to disable it because it caused > problems, we could just uncomment one line in browser.js. True, so I got rid of all that. I also took out the enable/disable API since that's where most of the work was. > You should use constants instead of "unfocused", "active" etc. Done(ish). I ended up using the same constants for windows & tabs, got rid of minimized (since we're not detecting that). Some duplication happening because building objects with constant keys is not pretty. Still to do: * Tests * Boris suggested that I adjust priority instead of just setting it, so I'm going to try that.

Comment on attachment 404685 [details] [diff] [review] Patch v0.6 >+// Constants /////////////////////////////////////////////////////////////////// Please drop the crazy amount of slashes... I don't think that comment and similar ones are useful at all, but feel free to keep them if you think they help. >+const FOCUSED = "focused"; >+const UNFOCUSED = "unfocused"; >+ >+const PRIORITIES = { >+ focused: { >+ focused: Ci.nsISupportsPriority.PRIORITY_HIGHEST, >+ unfocused: Ci.nsISupportsPriority.PRIORITY_NORMAL }, >+ unfocused: { >+ focused: Ci.nsISupportsPriority.PRIORITY_NORMAL, >+ unfocused: Ci.nsISupportsPriority.PRIORITY_LOW } >+}; I'd suggest this: > const FOCUSED = 0; > const UNFOCUSED = 1; > const PRIORITIES = [ > [ > Ci.nsISupportsPriority.PRIORITY_HIGHEST, > Ci.nsISupportsPriority.PRIORITY_NORMAL > ], > [ > Ci.nsISupportsPriority.PRIORITY_NORMAL, > Ci.nsISupportsPriority.PRIORITY_LOW > ] > ]; Or more explicit: > const FOCUSED = 0; > const UNFOCUSED = 1; > const PRIORITIES = {}; > PRIORITIES[FOCUSED] = {}; > PRIORITIES[UNFOCUSED] = {}; > PRIORITIES[FOCUSED][FOCUSED] = Ci.nsISupportsPriority.PRIORITY_HIGHEST; > PRIORITIES[FOCUSED][UNFOCUSED] = Ci.nsISupportsPriority.PRIORITY_NORMAL; > PRIORITIES[UNFOCUSED][FOCUSED] = Ci.nsISupportsPriority.PRIORITY_NORMAL; > PRIORITIES[UNFOCUSED][UNFOCUSED] = Ci.nsISupportsPriority.PRIORITY_LOW; >+// Attributes ////////////////////////////////////////////////////////////////// >+var _lastFocusedWindow = null; >+var _running = false; I don't think these variables can be considered attributes. >+ init: function(aWindow) { >+ // Since all windows call init, make sure the window is "loaded" by us. >+ if (!aWindow.__NP) This check seems unneccessary. How about calling that method "addBrowserWindow"? >+ uninit: function() { >+ // Remove observers >+ // XXX what if removeObserver failed? Why would it? >+let NPObserver = { >+ QueryInterface: XPCOMUtils.generateQI([Ci.nsIDOMEventListener, >+ Ci.nsIObserver, >+ Ci.nsISupportsWeakReference]), >+ >+ observe: function(aSubject, aTopic, aData) { >+ log(aTopic); >+ switch (aTopic) { >+ case "quit-application-granted": >+ this.uninit(); uninit is defined on a different object. You could move it here, though... as it's currently on the exported object without being used externally. >+ case "domwindowclosed": >+ // This fires for dialogs & other windows, which we don't care about Use the unload event of the windows you care about instead of domwindowclosed. >+ handleEvent: function(aEvent) { >+ log(aEvent.type); >+ switch (aEvent.type) { Why is this part of NPObserver? It could just be a global function... function eventHandler(aEvent) { switch (aEvent.type) ... >+ onOpen: function(aBrowser) { >+ // Just set it's priority to that of the window. We'll get TabSelect >+ // after this if it's an open+select. >+ let window = aBrowser.ownerDocument.defaultView; >+ TabHelper.setPriority(aBrowser, window.__NP.basePriority); I don't think I see the point of window.__NP.basePriority. It's always the following, right? PRIORITIES[(window == _lastFocusedWindow) ? FOCUSED: UNFOCUSED][UNFOCUSED] >+ onSelect: function(aBrowser) { >+ // This is probably not the best way to do this. We should keep track of >+ // the last selected tab like we're doing with windows. Right now we're >+ // QI'ing multiple times for each tab. Yeah, this doesn't seem ideal. >+var WindowHelper = { >+ onLoad: function(aWindow) { >+ if (aWindow.gBrowser.selectedBrowser) This check doesn't seem useful. >+ // Iterate over tabs & set priority to normal >+ aWindow.gBrowser.browsers.forEach(function(browser) { >+ TabHelper.setPriority(browser, Ci.nsISupportsPriority.PRIORITY_NORMAL); >+ }); This doesn't seem useful for a window that's closing anyway. >--- a/browser/base/content/browser.js >+++ b/browser/base/content/browser.js >- // initialize the session-restore service (in case it's not already running) >+ // initialize network prioritizer and the session-restore service > if (document.documentElement.getAttribute("windowtype") == "navigator:browser") { This is in browser.js' delayedStartup function, called by BrowserStartup, called by browser.xul, so I don't see the point of this check.

(In reply to comment #13) > Please drop the crazy amount of slashes... I don't think that comment and > similar ones are useful at all, but feel free to keep them if you think they > help. Yea, it's obnoxious. It was mostly for my own purposes when I was reorganizing the file. > I'd suggest this: > ... > Or more explicit: > ... Take a look again when I finish reworking it so I'm adjusting priority. I'm sure you'll have some feedback about constants there, but I'm mostly just keeping track of a single delta & multipliers, so this becomes a moot point. I do agree though that both of your suggestions are better. > I don't think these variables can be considered attributes. Fair. Another result of shuffling things around. > >+ init: function(aWindow) { > >+ // Since all windows call init, make sure the window is "loaded" by us. > >+ if (!aWindow.__NP) > > This check seems unneccessary. > How about calling that method "addBrowserWindow"? I suppose (this is what I get for modeling after sessionstore). And ok. > >+ this.uninit(); > > uninit is defined on a different object. You could move it here, though... as > it's currently on the exported object without being used externally. More shuffling fallout. I'll probably move uninit down to this object & rename NPObserver to NPHelper. > > >+ case "domwindowclosed": > >+ // This fires for dialogs & other windows, which we don't care about > > Use the unload event of the windows you care about instead of domwindowclosed. I was but I get a whole bunch of unnecessary unload events bubbling up (usually from HTMLDocuments when opening a new tab). So instead of ignoring them all, I thought it would be more efficient to observe something that happens less. Unless I'm doing something wrong. > >+ handleEvent: function(aEvent) { > >+ log(aEvent.type); > >+ switch (aEvent.type) { > > Why is this part of NPObserver? > It could just be a global function... > > function eventHandler(aEvent) { > switch (aEvent.type) ... Sure, but I was trying to keep code a little more organized. I really don't like global functions. Renaming to NPHelper (or similar) and adding init/uninit there might make this object a bit more useful. > >+ onOpen: function(aBrowser) { > >+ // Just set it's priority to that of the window. We'll get TabSelect > >+ // after this if it's an open+select. > >+ let window = aBrowser.ownerDocument.defaultView; > >+ TabHelper.setPriority(aBrowser, window.__NP.basePriority); > > I don't think I see the point of window.__NP.basePriority. It's always the > following, right? > > PRIORITIES[(window == _lastFocusedWindow) ? FOCUSED: UNFOCUSED][UNFOCUSED] Yea. I had plans to use it, and I might use it (or something similar) with adjust. If not, it's gone. > > >+ onSelect: function(aBrowser) { > > >+ // This is probably not the best way to do this. We should keep track of > >+ // the last selected tab like we're doing with windows. Right now we're > >+ // QI'ing multiple times for each tab. > > Yeah, this doesn't seem ideal. Yea, I'm going to keep a hold of the last selected tab for each window. It becomes especially hard to do the adjust method without knowing only which one needs to be adjusted. > >+var WindowHelper = { > >+ onLoad: function(aWindow) { > > >+ if (aWindow.gBrowser.selectedBrowser) > > This check doesn't seem useful. Yea, it doesn't really seem useful. Maybe I had a case where I was loading before the selectedBrowser was set? I'll test & take it out if I can. > >+ // Iterate over tabs & set priority to normal > >+ aWindow.gBrowser.browsers.forEach(function(browser) { > >+ TabHelper.setPriority(browser, Ci.nsISupportsPriority.PRIORITY_NORMAL); > >+ }); > > This doesn't seem useful for a window that's closing anyway. This was from onUnload, which was supposed to only be run when disabling the module. onClose is the only thing running when a window is closed. onUnload should have been deleted. > >--- a/browser/base/content/browser.js > >+++ b/browser/base/content/browser.js > > >- // initialize the session-restore service (in case it's not already running) > >+ // initialize network prioritizer and the session-restore service > > if (document.documentElement.getAttribute("windowtype") == "navigator:browser") { > > This is in browser.js' delayedStartup function, called by BrowserStartup, > called by browser.xul, so I don't see the point of this check. Again, this is what I get for following sessionstore's lead. I'll move it outside the if, and delete the if in a followup bug. As an aside, it sounds like sessionstore can be simplified a bit for Firefox. It still seems to be written to handle the general case (Firefox & Seamonkey). (eg. sessionstore.init gets called from delayedStartup too, but it listens for domwindowopened and does a browser:navigator check).

(In reply to comment #14) > > >+ case "domwindowclosed": > > >+ // This fires for dialogs & other windows, which we don't care about > > > > Use the unload event of the windows you care about instead of domwindowclosed. > > I was but I get a whole bunch of unnecessary unload events bubbling up (usually > from HTMLDocuments when opening a new tab). So instead of ignoring them all, I > thought it would be more efficient to observe something that happens less. > Unless I'm doing something wrong. You shouldn't get unload events from content there. I just briefly tested window.addEventListener("unload", function(){alert(1)}, false); on a browser window and was only getting the alert when closing the whole window, but not for single tabs. > > Why is this part of NPObserver? > > It could just be a global function... > > > > function eventHandler(aEvent) { > > switch (aEvent.type) ... > > Sure, but I was trying to keep code a little more organized. I really don't > like global functions. Renaming to NPHelper (or similar) and adding init/uninit > there might make this object a bit more useful. I don't think there's a problem with global stuff here, since the module has its very own scope. "NPHelper" doesn't really mean anything and doesn't structure the code any better. > As an aside, it sounds like sessionstore can be simplified a bit for Firefox. > It still seems to be written to handle the general case (Firefox & Seamonkey). > (eg. sessionstore.init gets called from delayedStartup too, but it listens for > domwindowopened and does a browser:navigator check). Yeah, you should probably file bugs for that too.

Cleaned up a bunch. Now adjusts priority instead of just setting it. Has tests. I marked most of my thoughts in there. Some code is not actually used (eg SetPriority), so should probably come out. Comment on the test: it's written right now under the assumption that bug 521233 will get landed first. There's a bug chunk of code that's currently commented out that would replace other code if that bug doesn't land. The cruft would be gotten rid of before it actually got checked in (based on the other bug).

Comment on attachment 407151 [details] [diff] [review] Patch v0.7 >+//XXXzpao Cc is only used in log, Cu only to .import XPCOMUtils >+const Cc = Components.classes; >+const Ci = Components.interfaces; >+const Cu = Components.utils; >+ >+Cu.import("resource://gre/modules/XPCOMUtils.jsm"); So just use Components.utils.import and Components.classes in log and get rid of the consts as well as the XXX comment? >+function _init() { >+ // Attach observers >+ OBSERVER_TOPICS.forEach(function(aTopic) { >+ _observerService.addObserver(NPObserver, aTopic, true); >+ }); >+ _running = true; >+} >+ >+function _uninit() { >+ // Attach observers >+ OBSERVER_TOPICS.forEach(function(aTopic) { >+ _observerService.removeObserver(NPObserver, aTopic, true); >+ }); >+ _running = false; >+ // Clean these up so that we don't leak the world >+ _windows = []; >+ _lastFocusedWindow = null; >+} >+// Helpers >+let NPObserver = { >+ QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver, >+ Ci.nsISupportsWeakReference]), >+ >+ observe: function(aSubject, aTopic, aData) { >+ log(aTopic); >+ switch (aTopic) { >+ case "quit-application-granted": >+ _uninit(); >+ break; >+ } >+ >+ _prettyPrint(); >+ } >+}; _init and _uninit don't seem to actually do something useful. So I'd say remove them and just don't observe quit-application-granted. >+// Methods that impact a tab. Put into single object for organization. >+let TabHelper = { >+ onOpen: function(aBrowser) { >+ // If the tab is in the focused window, leave priority as it is >+ let deltaMultiplier = >+ (aBrowser.ownerDocument.defaultView == _lastFocusedWindow) ? 0 : -1; >+ TabHelper.adjustPriority(aBrowser, deltaMultiplier * PRIORITY_DELTA); s/TabHelper/this/ >+ }, >+ >+ onSelect: function(aBrowser) { >+ let window = aBrowser.ownerDocument.defaultView; >+ >+ let lastSelectedTab = _windows[window.__NPi].lastSelectedTab; Can you return if window != _lastSelectedWindow and store _lastSelectedTab globally rather than for each window? It doesn't seem to make sense for background windows. This would allow you to get rid of __NPi. Also note that "lastSelectedTab" is misleading, since it's a browser. >+ if (lastSelectedTab) >+ TabHelper.adjustPriority(lastSelectedTab, -1 * PRIORITY_DELTA); >+ TabHelper.adjustPriority(aBrowser, PRIORITY_DELTA); s/TabHelper/this/ >+ //XXXzpao getPriority is only used in _prettyPrint, setPriority isn't used >+ getPriority: function(aBrowser) { >+ return TabHelper.getLoadgroup(aBrowser).priority; >+ }, >+ setPriority: function(aBrowser, aPriority) { >+ TabHelper.getLoadgroup(aBrowser).priority = aPriority; >+ }, So just use TabHelper.getLoadgroup(aBrowser).priority in _prettyPrint and remove setPriority. >+ adjustPriority: function(aBrowser, aDelta) { >+ TabHelper.getLoadgroup(aBrowser).adjustPriority(aDelta); >+ } s/TabHelper/this/ Rather than having every caller pass PRIORITY_DELTA, it looks like they should pass the multiplier and PRIORITY_DELTA should be used only in this method. >+ // This is lazy, but assume it's not focused first. >+ aWindow.gBrowser.browsers.forEach(function(aBrowser) { >+ TabHelper.adjustPriority(aBrowser, -1 * PRIORITY_DELTA); >+ }); >+ >+ // This gets called AFTER activate event, so if this is the focused window >+ // we want to activate it. >+ if (aWindow == _windowMediator.getMostRecentWindow("navigator:browser")) >+ WindowHelper.onActivate(aWindow); >+ >+ // Select the selected tab >+ TabHelper.onSelect(aWindow.gBrowser.selectedBrowser); >+ >+ //XXXzpao by now we've set priority on all tabs 2 times, and 3 times for >+ // the focused tab. That's no good. Why do you assume it's not focused rather than determining it first? >+ // Auxiliary methods >+ adjustPriority: function(aWindow, aFocus) { >+ // Adjust priority for each tab in the window. This doesn't check if a >+ // window is already focused. We shouldn't have to though. >+ let deltaMultiplier = (aFocus == FOCUSED) ? 1 : -1; >+ aWindow.gBrowser.browsers.forEach(function(aBrowser) { >+ TabHelper.adjustPriority(aBrowser, deltaMultiplier * PRIORITY_DELTA); >+ }); You could define FOCUSED as 1 and UNFOCUSED as -1 and just do aMultiplyer * PRIORITY_DELTA. It might also make sense to use something like GOT_FOCUS and LOST_FOCUS instead of FOCUSED and UNFOCUSED in order to make it clear that you aren't adjusting priorities for states (which could be done repeatedly) but transitions between states (which should be done exactly once for each transition).

(In reply to comment #18) > _init and _uninit don't seem to actually do something useful. So I'd say remove > them and just don't observe quit-application-granted. _uninit makes sure this doesn't leak. I can probably mitigate that by deleting entries from _windows in onUnload and I guess I'm already setting _lastFocusedWindow to null on Unload. So that should work. > >+ onSelect: function(aBrowser) { > >+ let window = aBrowser.ownerDocument.defaultView; > >+ > >+ let lastSelectedTab = _windows[window.__NPi].lastSelectedTab; > > Can you return if window != _lastSelectedWindow and store _lastSelectedTab > globally rather than for each window? It doesn't seem to make sense for > background windows. This would allow you to get rid of __NPi. So the thinking here is that the selected tab in an unfocused window should still have higher priority than the other tabs in that window. That's why I need to track it per window. That was kinda central to this project, so I'm going to keep it. That said, I can see it being not as valuable as originally thought. I'll think on it. It would mean less overhead. > Also note that "lastSelectedTab" is misleading, since it's a browser. Yea, I realized this. I think I'll do a bit of renaming. > >+ adjustPriority: function(aBrowser, aDelta) { > >+ TabHelper.getLoadgroup(aBrowser).adjustPriority(aDelta); > >+ } > > Rather than having every caller pass PRIORITY_DELTA, it looks like they should > pass the multiplier and PRIORITY_DELTA should be used only in this method. > > >+ // This is lazy, but assume it's not focused first. > >+ aWindow.gBrowser.browsers.forEach(function(aBrowser) { > >+ TabHelper.adjustPriority(aBrowser, -1 * PRIORITY_DELTA); > >+ }); > >+ > >+ // This gets called AFTER activate event, so if this is the focused window > >+ // we want to activate it. > >+ if (aWindow == _windowMediator.getMostRecentWindow("navigator:browser")) > >+ WindowHelper.onActivate(aWindow); > >+ > >+ // Select the selected tab > >+ TabHelper.onSelect(aWindow.gBrowser.selectedBrowser); > >+ > >+ //XXXzpao by now we've set priority on all tabs 2 times, and 3 times for > >+ // the focused tab. That's no good. > > Why do you assume it's not focused rather than determining it first? The "problem" here is that onActivate deprioritizes the other last focused window, then prioritizes this one. The prioritization is based on it previously being deprioritized. I guess the simple solution is to add an additional parameter to onActivate indicating if the window should get adjusted. > > >+ // Auxiliary methods > >+ adjustPriority: function(aWindow, aFocus) { > >+ // Adjust priority for each tab in the window. This doesn't check if a > >+ // window is already focused. We shouldn't have to though. > >+ let deltaMultiplier = (aFocus == FOCUSED) ? 1 : -1; > >+ aWindow.gBrowser.browsers.forEach(function(aBrowser) { > >+ TabHelper.adjustPriority(aBrowser, deltaMultiplier * PRIORITY_DELTA); > >+ }); > > You could define FOCUSED as 1 and UNFOCUSED as -1 and just do aMultiplyer * > PRIORITY_DELTA. > It might also make sense to use something like GOT_FOCUS and LOST_FOCUS instead > of FOCUSED and UNFOCUSED in order to make it clear that you aren't adjusting > priorities for states (which could be done repeatedly) but transitions between > states (which should be done exactly once for each transition). Good idea.

(In reply to comment #19) > So the thinking here is that the selected tab in an unfocused window should > still have higher priority than the other tabs in that window. That's why I > need to track it per window. That was kinda central to this project, so I'm > going to keep it. If it's really needed, _windows should be an array of [win, tab] or {window: win, lastSelectedTab: tab} entities. Finding the entity for a given window will be fast and trivial (should be done in a helper function, though), no need for __NPi.

Changes per comment #18 and 20.

Comment on attachment 407403 [details] [diff] [review] Patch v0.8 qrefreshed into the wrong patch; will post a new one when I clean this mess up

For real this time. The one thing I have remaining is that I'm holding a reference to a window in _lastFocusedWindow and then also in _windows[]. Is that OK or should I make _lastFocusedWindow point to an entry in _windows (which are in the form { window: X, lastSelectedBrowser: Y })?

(In reply to comment #23) > The one thing I have remaining is that I'm holding a > reference to a window in _lastFocusedWindow and then also in _windows[]. Is > that OK or should I make _lastFocusedWindow point to an entry in _windows > (which are in the form { window: X, lastSelectedBrowser: Y })? Both ways are ok. You should choose what fits your use of _lastFocusedWindow better.

Comment on attachment 407410 [details] [diff] [review] Patch v0.8 (for real) This looks fine, now some more subtle things... >+// Exported Module >+let NetworkPrioritizer = { >+ >+ addBrowserWindow: function(aWindow) { >+ WindowHelper.onLoad(aWindow); >+ >+ _prettyPrint(); >+ } >+}; "Exported Module" isn't quite accurate... it's an exported symbol of the NetworkPrioritizer module. Therefore, I'm not sure exporting "NetworkPrioritizer" makes sense. How about: let EXPORTED_SYMBOLS = ["handleBrowserWindow"]; ... function handleBrowserWindow(aWindow) { WindowHelper.onLoad(aWindow); } ... let NP = {}; Cu.import("resource://gre/modules/NetworkPrioritizer.jsm", NP); NP.handleBrowserWindow(window); This would be consistent with getService, too. >+ switch (aEvent.type) { >+ case "TabOpen": >+ BrowserHelper.onOpen(aEvent.originalTarget.linkedBrowser); >+ break; >+ case "TabSelect": >+ BrowserHelper.onSelect(aEvent.originalTarget.linkedBrowser); >+ break; >+ case "activate": >+ WindowHelper.onActivate(aEvent.originalTarget); >+ break; >+ case "unload": >+ WindowHelper.onUnload(aEvent.currentTarget); These can all use aEvent.target, can't they? >+let BrowserHelper = { >+ onOpen: function(aBrowser) { >+ // If the tab is in the focused window, leave priority as it is >+ let deltaMultiplier = >+ (aBrowser.ownerDocument.defaultView == _lastFocusedWindow) ? 0 : -1; >+ this.adjustPriority(aBrowser, deltaMultiplier); Calling adjustPriority if you don't want to change the priority seems strange, so I'd suggest: if (aBrowser.ownerDocument.defaultView != _lastFocusedWindow) this.adjustPriority(aBrowser, -1); As elsewhere, it would be nice to use a constant for -1. But LOST_FOCUS doesn't seem quite right. How about adding two new methods, increasePriority and decreasePriority, and not using constants at all? >+ getEntry: function(aWindow) { >+ // Since every object should have a unique window, this should be fine >+ for (let i = 0; i < _windows.length; i++) >+ if (_windows[i].window == aWindow) >+ return _windows[i]; >+ }, >+ >+ deleteEntry: function(aWindow) { >+ for (let i = 0; i < _windows.length; i++) >+ if (_windows[i].window == aWindow) >+ _windows.splice(i, 1); >+ } Could use a getEntryIndex method to avoid the code duplication. >- // initialize the session-restore service (in case it's not already running) >+ // initialize network prioritizer and the session-restore service > if (document.documentElement.getAttribute("windowtype") == "navigator:browser") { >+ let temp = {}; >+ Cu.import("resource://gre/modules/NetworkPrioritizer.jsm", temp); >+ temp.NetworkPrioritizer.addBrowserWindow(window); As discussed earlier, move this outside of the if.

Comment on attachment 407410 [details] [diff] [review] Patch v0.8 (for real) >+ // This gets called AFTER activate event, so if this is the focused window >+ // we want to activate it. Otherwise, deprioritize it. >+ if (aWindow == _windowMediator.getMostRecentWindow("navigator:browser")) This is probably better: if (aWindow == Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager).activeWindow) >+ onActivate: function(aWindow, aHasFocus) { >+ log(aWindow.gBrowser.contentTitle); >+ >+ // If this window was the last focused window, we don't need to do anything >+ if (aWindow == _lastFocusedWindow) >+ return; When would this happen? >+ // If we have a last focused window, we need to deprioritize it first >+ if (_lastFocusedWindow) >+ this.adjustPriority(_lastFocusedWindow, LOST_FOCUS); >+ >+ // Only adjust priority for this window if it's not already focused >+ if (!aHasFocus) >+ this.adjustPriority(aWindow, GOT_FOCUS); >+ _lastFocusedWindow = aWindow; >+ }, Instead of aHasFocus, could you make this two methods, like handleFocusedWindow and onActivate? onActivate would call handleFocusedWindow.

(In reply to comment #25) > let NP = {}; > Cu.import("resource://gre/modules/NetworkPrioritizer.jsm", NP); > NP.handleBrowserWindow(window); That's now how we've done any other jsm file, so why would we do something different now? I don't see the win here.

That's not quite true. I don't know about every js module, but LightweightThemeConsumer.jsm for instance exports a constructor rather than a host object. If the host object has only one method, I don't see its point. The win is that it simplifies the code. handleBrowserWindow might be too ambiguous if the module is imported into a shared scope, but that could be addressed by using a better name... although it doesn't actually make sense to import this module into a shared or even global scope.

(In reply to comment #25) > >+ switch (aEvent.type) { > >+ case "TabOpen": > >+ BrowserHelper.onOpen(aEvent.originalTarget.linkedBrowser); > >+ break; > >+ case "TabSelect": > >+ BrowserHelper.onSelect(aEvent.originalTarget.linkedBrowser); > >+ break; > >+ case "activate": > >+ WindowHelper.onActivate(aEvent.originalTarget); > >+ break; > >+ case "unload": > >+ WindowHelper.onUnload(aEvent.currentTarget); > > These can all use aEvent.target, can't they? target == originalTarget (right?), so the first 3 can. The originalTarget in "unload" is a XULDocument instead of a ChromeWindow. (In reply to comment #26) > (From update of attachment 407410 [details] [diff] [review]) > >+ // This gets called AFTER activate event, so if this is the focused window > >+ // we want to activate it. Otherwise, deprioritize it. > >+ if (aWindow == _windowMediator.getMostRecentWindow("navigator:browser")) > > This is probably better: > if (aWindow == > > Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager).activeWindow) What's the difference? Will that always return navigator:browser windows? That's all I care about. > >+ onActivate: function(aWindow, aHasFocus) { > >+ log(aWindow.gBrowser.contentTitle); > >+ > >+ // If this window was the last focused window, we don't need to do anything > >+ if (aWindow == _lastFocusedWindow) > >+ return; > > When would this happen? When switching back to Firefox from a different app. The activate event is fired anytime the window is activated. > >+ // If we have a last focused window, we need to deprioritize it first > >+ if (_lastFocusedWindow) > >+ this.adjustPriority(_lastFocusedWindow, LOST_FOCUS); > >+ > >+ // Only adjust priority for this window if it's not already focused > >+ if (!aHasFocus) > >+ this.adjustPriority(aWindow, GOT_FOCUS); > >+ _lastFocusedWindow = aWindow; > >+ }, > > Instead of aHasFocus, could you make this two methods, like handleFocusedWindow > and onActivate? onActivate would call handleFocusedWindow. I guess. It doesn't seem to make the code much cleaner & would separate related code (adjusting priority for each window would be in separate methods instead of a couple lines apart).

> > This is probably better: > > if (aWindow == > > > > Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager).activeWindow) > > What's the difference? I'd trust the focus manager more than the window mediator. Looking at bug 511503, the activate event is also going to be implemented there, so these two things should go well together. > Will that always return navigator:browser windows? No, but I think the important part is that aWindow isn't active and should get the activate event at some point.

I don't necessarily agree with making the exported symbol just a method, but it does make the usage cleaner. For now we can assume we won't need to export other methods so it's OK. Did a little bit of other cleanup in addition to suggested changes.

I think it would be good to document up front what priorities we're trying to set to what (looks like unfocused windows +10 for all tabs, focused window 0 for unselected tabs and -10 for the selected tab?). Your removeEventListener calls for the tab events in onUnload should be passing |false| for the last arg, no? I assume you tested that at application quit you get unloads for all the windows? >+ // This test assumes that the no priorities have been adjusted and the s/the no/no/ Other than that looks ok.

(In reply to comment #32) > I assume you tested that at application quit you get unloads for all the > windows? I just tested this on Linux, got the event for all three open windows on quit.

Comment on attachment 407631 [details] [diff] [review] Patch v0.9 You're going to remove DEBUG, log and prettyPrint before landing this, aren't you? >+let WindowHelper = { >+ onLoad: function(aWindow) { This code doesn't really know about the load event, so I'd probably call this method addWindow, and onUnload maybe removeWindow for consistency. >+ aWindow.gBrowser.tabContainer.removeEventListener(event, _handleEvent, true); See bz's comment. >+ deleteEntry: function(aWindow) { >+ _windows.splice(this.getEntryIndex(aWindow), 1); >+ }, This is called only once and doesn't seem universally useful, so this line can be called directly without a dedicated method, just like _windows.push(...) elsewhere.

Addresses last couple comments. Took out debug logging. Still has the backup code in the test, but I'll take that out depending on bug 521233

Failed tests on Linux try server. Other tests failed first so it might have been a cascading effect. I put it up on try again hoping for an all green this time, otherwise I'm getting my Linux env set up here to debug locally. Was fine on OSX/Win.

What I checked in. Had to do a little extra waitForFocus to get the tests to run correctly on Linux, but the guts of the tests are unchanged. Pushed as http://hg.mozilla.org/mozilla-central/rev/bbeaa4e518ee

For 1.9.2. Only difference from attachment #409414 [details] [diff] [review] is that this has the patch for bug 525635 folded in.

Comment on attachment 410870 [details] [diff] [review] Patch v1.3 (for mozilla-1.9.2) a192=beltzner

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7d7baabfedf9

This caused some Ts/Ts Shutdown jiggery-pokery on OSX Tiger (Darwin 8.2.2) according to the regression analysis emailers: Ts Shutdown MIN Dirty improvement: http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/0f69dbaf16940297# Ts MED Dirty regression: http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/5635e7019e6e254b# Ts MIN Dirty regression: http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/d9db3978d13df2ce# Did we expect that / see that on trunk?

(In reply to comment #41) > Did we expect that / see that on trunk? No, I don't remember seeing those on trunk and looking back, I don't see any associated regressions on trunk.

These regressions seem real - they haven't gone away. David: does Ts DIRTY do session restore? Is there a chance that this has moved the goalposts between startup and shutdown due to how we're doing the network prioritization?

Beltzner: Dirty profiles (as far as I know) do not interact with sessionrestore at all. CCing alice.

It's odd that there's any regression, let alone only these. The only thing in the startup path is in delayedStartup. We import the module and change the prioritization on the single tab that's open. The dirtiness of the profile shouldn't have any effect. Just to be clear, this work doesn't affect just session restore. It's always on. But the only times we'll (likely) see any effects is during session restore and multiple bookmarks opening. So I guess I'll back it out and see if we get the reverse effects.

Backed out on 1.9.2 in http://hg.mozilla.org/releases/mozilla-1.9.2/rev/cc91fc05ee42

There was a 30% regression on Ts Shutdown after backing out. http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/e86ef6ca7333b22b which corresponds with the 27% increase the other day. It also _looks_ like the regressions in comment #41 look real(ish) according to graphs.m.o http://tinyurl.com/yfc3ev8 even though nothing's been reported to dev.tree-management yet. I can't explain it. It's barely in the startup path (it's in delayedStartup) and it barely does anything on window close (just removes a reference we were holding and removes some events), so it shouldn't be any noticeable changes, let alone 15-30% differences.

What's the absolute number on that change? Is it about 100ms?

Backed out so removing final-fixed.

(In reply to comment #48) > What's the absolute number on that change? Is it about 100ms? Yes. ~95s on original improvement, ~135s on original regressions. ~85s on backout regression and looking at some specific boxes on g.m.o (though those are all over the place), 100+s improvements after backing out.

Sounds like the 100ms painting lag moving about... See bug 519893

Relanded on branch http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ce2fc8035c06

Finally I was able to have a deeper look into the fix for this bug. I have tested the following scenarios: * 1 window with a dozen of tabs open * 2 windows with a dozen of tabs open * 3 windows with a dozen of tabs open (1 window minimized) I can verify that the current tab gets the highest priority in those cases. Clicking on different links is really responsive while the other tabs are loaded in the background. Switching to another loading tab during restore makes that one react faster. Tabs in a background window are loaded with a lower priority as the foreground window. Minimized windows have the lowest priority. Paul, as what I have seen it's kinda hard to find the right pages to test with. So how much do your tests cover of this new feature? Can we say that's enough and don't need Litmus tests? Verified fixed on trunk and 1.9.2 with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091221 Minefield/3.7a1pre ID:20091221034526 Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b6pre) Gecko/20091222 Namoroka/3.6b6pre ID:20091222033648

This all looks complicated (and hence bug-prone). As I mentioned elsewhere, the BarTab add-on suits my needs and reduces Firefox's memory footprint. Thanks for your efforts! -- Rick

(In reply to comment #55) > This all looks complicated (and hence bug-prone). As I mentioned elsewhere, the > BarTab add-on suits my needs and reduces Firefox's memory footprint. > Thanks for your efforts! -- Rick It's not terribly complicated and it has a full arsenal of tests that landed with it...

Verified fixed on Mozilla/5.0 (Windows NT 6.1; rv:2.2a1pre) Gecko/20110404 Firefox/4.2a1pre