Closed Bug 577939 Opened 10 years ago Closed 10 years ago

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

Categories

(SeaMonkey :: Tabbed Browser, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: misak.bugzilla, Assigned: misak.bugzilla)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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.
Status: NEW → ASSIGNED
Depends on: 519216, 577320
Attachment #456757 - Flags: review?(neil)
Attachment #456757 - Attachment is patch: true
Attachment #456757 - Attachment mime type: application/octet-stream → text/plain
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.
Attachment #456757 - Flags: review?(neil) → review-
(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.
Attached patch v2 (obsolete) — Splinter Review
this fixes two Neil's comments, tests passing. Waiting for feedback for two other comments
Assignee: nobody → misak.bugzilla
Attachment #456757 - Attachment is obsolete: true
Attachment #460030 - Flags: review?(neil)
(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...
Blocks: 583317
(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.
Blocks: 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.
Attached patch v3 (obsolete) — Splinter Review
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.
Attachment #460030 - Attachment is obsolete: true
Attachment #466029 - Flags: review?(neil)
Attachment #460030 - Flags: review?(neil)
No longer blocks: 586340
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.
Attachment #466029 - Flags: review?(neil) → review+
Attached patch v3aSplinter Review
Fixed all nits.
Attachment #466029 - Attachment is obsolete: true
Attachment #466155 - Flags: superreview?(neil)
Attachment #466155 - Flags: review+
Attachment #466155 - Flags: superreview?(neil) → superreview+
Pushed: http://hg.mozilla.org/comm-central/rev/7d97c516fbe2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment on attachment 466155 [details] [diff] [review]
v3a

I pushed without looking at the tree, it needs approval :(
Attachment #466155 - Flags: approval-seamonkey2.1a3?
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 :(
Attachment #466155 - Flags: approval-seamonkey2.1a3?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed again: http://hg.mozilla.org/comm-central/rev/aa72d902b7e1
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.