577939 - Port Bug 519216 [Removing a progress listener while it's being called affects subsequent listeners] and followup bug 577320 to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Misak Khachatryan]

Attachment: patch

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 1

[[e-mail]]

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.

Comment 2

[Misak Khachatryan]

(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.

Comment 3

[Misak Khachatryan]

Attachment: v2

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

Comment 4

[[e-mail]]

(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 5

[[e-mail]]

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 6

[[e-mail]]

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.

Comment 7

[Robert Kaiser]

(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...

Comment 8

[[e-mail]]

(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?

Comment 9

[Robert Kaiser]

(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.

Comment 10

[Philip Chee]

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.

Comment 11

[Robert Kaiser]

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.

Comment 12

[Misak Khachatryan]

Attachment: v3

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.

Comment 13

[Misak Khachatryan]

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

Comment 14

[[e-mail]]

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 15

[[e-mail]]

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.

Comment 16

[Misak Khachatryan]

Attachment: v3a

Fixed all nits.

Comment 17

[Misak Khachatryan]

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

Comment 18

[Misak Khachatryan]

Comment on attachment 466155 [details] [diff] [review]
v3a

I pushed without looking at the tree, it needs approval :(

Comment 19

[Misak Khachatryan]

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 :(

Comment 20

[Misak Khachatryan]

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