From parent bug: I happened to notice last night that the 'list grows forever' part was fixed in bug 303075. This either means (a) That removing a listener inside a listener is broken and should be fixed [eg removing itself will skip the next listener] or (b) I can remove all those |if (p)| fragments now as they are not doing anything useful. The same also applies to mTabsProgressListeners of course. Some of changes in FF (like in bug 303075) is actually what we already have, but some of them worth to pick. Attaching patch suitable for review and comment, but it not thoroughly tested due our trunk is broken now.

Comment on attachment 456757 [details] [diff] [review] patch >+ _callProgressListeners: function () { >+ Array.unshift(arguments, this.mBrowser); >+ return this.mTabBrowser._callProgressListeners.apply(this.mTabBrowser, arguments); IMHO this should have been written out instead of using unshift and apply. >+ this._callProgressListeners("onUpdateCurrentBrowser", We don't have this notification. >+ this._callProgressListeners("onStateChange", >+ [aWebProgress, aRequest, aStateFlags, aStatus], >+ false); This is the only one with a parameter of "false", right? The others are all omitted or "true, false". But with the onUpdateCurrentBrowser removal we can now always notify global listeners. And aren't the "true, false" ones always passed a null (use current) browser? >+ return this._callProgressListeners("onRefreshAttempted", >+ [aWebProgress, aURI, aDelay, aSameURI]); I don't like the reuse in this case. It's subtly different to the other cases.

(In reply to comment #1) > Comment on attachment 456757 [details] [diff] [review] > patch > > >+ _callProgressListeners: function () { > >+ Array.unshift(arguments, this.mBrowser); > >+ return this.mTabBrowser._callProgressListeners.apply(this.mTabBrowser, arguments); > IMHO this should have been written out instead of using unshift and apply. > I even don't understand what this doing. Is called somewhere ? I tend to delete it. > >+ this._callProgressListeners("onStateChange", > >+ [aWebProgress, aRequest, aStateFlags, aStatus], > >+ false); > This is the only one with a parameter of "false", right? The others are all > omitted or "true, false". But with the onUpdateCurrentBrowser removal we can > now always notify global listeners. And aren't the "true, false" ones always > passed a null (use current) browser? > Actually as fixed in new patch we shouldn't notify global listeners on onLocationChange.

this fixes two Neil's comments, tests passing. Waiting for feedback for two other comments

(In reply to comment #2) > (In reply to comment #1) > > >+ _callProgressListeners: function () { > > >+ Array.unshift(arguments, this.mBrowser); > > >+ return this.mTabBrowser._callProgressListeners.apply(this.mTabBrowser, arguments); > > IMHO this should have been written out instead of using unshift and apply. > I even don't understand what this doing. Is called somewhere ? The progress listener (mTabProgressListener) calls it. This is to avoid writing out this.mTabBrowser._callProgressListeners(this.mBrowser, ...) each time. Instead it uses Array.unshift to insert this.mBrowser in at the beginning of arguments and applies arguments directly to this.mTabBrowser's call. I'm not surprised you don't understand it; maybe you would understand this version: _callProgressListeners: function(aMethod, aArguments, aCallGlobalListeners) { this.mTabBrowser._callProgressListeners(this.mBrowser, aMethod, aArguments, aCallGlobalListeners); }

Comment on attachment 460030 [details] [diff] [review] v2 >+ var rv = true; Now that we're not changing onRefreshAttempted we don't need rv at all. > if (this.mTabBrowser.mCurrentTab == this.mTab) > this.mTabBrowser.updateUrlBar(aWebProgress, aRequest, aLocation, > null, this.mBrowser, this.mFeeds); > >- this.mTabBrowser.mTabsProgressListeners.forEach( >- function notifyLocationChange(element) { >- try { >- element.onLocationChange(this.mBrowser, aWebProgress, aRequest, aLocation); >- } catch (e) { >- Components.utils.reportError(e); >- } >- } >- , this); >+ this._callProgressListeners("onLocationChange", >+ [aWebProgress, aRequest, aLocation], >+ false,true); Nit: don't need the ,true because that's the default. I don't suppose you know whether tabs progress listeners expect to see favicon and feed notifications after a location change? (Global listeners do.)

Comment on attachment 460030 [details] [diff] [review] v2 >+ aArguments.unshift(aBrowser); Of course if you like the sneaky use of Array.unshift(arguments) then you can also use it here: Array.unshift(aArguments, aBrowser); What difference does this make, you might ask? Well, it simplifies the calls in the progress listener. You just write: this._callProgressListeners("onProgressChange", arguments); since the arguments you want to forward are your own arguments.

(In reply to comment #1) > >+ this._callProgressListeners("onUpdateCurrentBrowser", > We don't have this notification. D'oh, and I hoped we might add calling it for compat reasons so implementing it might make sense. I just ran across something FF is calling there and don't know where to port it to...

(In reply to comment #7) > (In reply to comment #1) > > >+ this._callProgressListeners("onUpdateCurrentBrowser", > > We don't have this notification. > D'oh, and I hoped we might add calling it for compat reasons so implementing it > might make sense. I just ran across something FF is calling there and don't > know where to port it to... It's a bit of a weird notification... is there any documentation for it?

(In reply to comment #8) > > > >+ this._callProgressListeners("onUpdateCurrentBrowser", > [...] > It's a bit of a weird notification... is there any documentation for it? The comment in http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js#4399 is all I could find, it says: // simulate all change notifications after switching tabs So it looks like the important thing there is that what is the "current browser" changed and FF does everything there that needs to be updated due to that, like, say, page zoom when it's saved per-site.

Misak, are you porting this bit in updateCurrentBrowser as well? http://hg.mozilla.org/mozilla-central/rev/1e70fba7e663#l2.383 Because I would need that in Bug 586340.

Actually, I'd be happy if what is here could be updated to Neil's comments, get reviews, and get in, as it helps other work and removes pointless error console spew. If Philip's and my additional wishes are not in that right now, we can surely put them into our patches in either followups or the bugs we're working on, but this will set up the infrastructure to go with there.

I removed _callProgressListeners and rewrote all calls to it as Neil suggested on comment #4. (In reply to comment #10) I'll put all onUpdateCurrentBrowser stuff in the patch for bug 583317, which i'm going to take after this bug.

Actually I forgot to change one call to _callprogresslisteners. Line 692 in SetIcon should be: this._callProgressListeners(browser, "onLinkIconAvailable", [browser.mIconURL]);

Comment on attachment 466029 [details] [diff] [review] v3 I am unfortunately suffering build issues, otherwise I would test this. >+ this.mTabBrowser._callProgressListeners(this.mBrowser,"onProgressChange", Nit: space after comma >+ onStatusChange : function(aWebProgress, aRequest, aStatus, aMessage) { Nit: as you're changing this, delete the space between onStatusChange and : >+ this.mTabBrowser._callProgressListeners(browser, "onLinkIconAvailable", [browser.mIconURL]); Nit: might want to wrap this > var webProgress = this.mCurrentBrowser.webProgress; >+ this._callProgressListeners(null, "onStateChange", >+ [webProgress, null, >+ nsIWebProgressListener.STATE_START | >+ nsIWebProgressListener.STATE_IS_NETWORK, 0], >+ true, false); Nit: indentation is wrong; can also eliminate webProgress variable i.e. this._callProgressListeners(null, "onStateChange", [this.mCurrentBrowser.webProgress, null, nsIWebProgressListener.STATE_START | nsIWebProgressListener.STATE_IS_NETWORK, 0], true, false); ^ Note extra space indentation for array members > webProgress = this.mCurrentBrowser.webProgress; >+ this._callProgressListeners(null, "onStateChange", >+ [webProgress, null, >+ nsIWebProgressListener.STATE_STOP | >+ nsIWebProgressListener.STATE_IS_NETWORK, 0], >+ true, false); [Same principle here.]

Comment on attachment 466029 [details] [diff] [review] v3 >+ this.mTabBrowser._callProgressListeners(browser, "onLinkIconAvailable", [browser.mIconURL]); This should just be this._callProgressListeners. r=me with that and the nits fixed.

Fixed all nits.

Pushed: http://hg.mozilla.org/comm-central/rev/7d97c516fbe2

Comment on attachment 466155 [details] [diff] [review] v3a I pushed without looking at the tree, it needs approval :(

Comment on attachment 466155 [details] [diff] [review] v3a Backed out, will land after a3. http://hg.mozilla.org/comm-central/rev/b168750e523a Sorry for bugspam :(

Pushed again: http://hg.mozilla.org/comm-central/rev/aa72d902b7e1