583317 - Update status bar correctly after tab switching and Add calls to listeners for onUpdateCurrentBrowser. Port of bug 327604 and bug 331938

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Robert Kaiser]

As Firefox implements onUpdateCurrentBrowser, it's very much possible that add-ons are out there that use it. Also, having a notification fire when tabs change can be helpful in multiple ways, among other for those where Firefox uses it: http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js#4374

I'm trying to work on bug 386363 but have no idea where to put the call that is in there right now, I guess we should just have that notification as well.

See http://mxr.mozilla.org/comm-central/search?string=onUpdateCurrentBrowser to also see where it's called.

Originally, bug 327604 and bug 331938 added the current callers, the former of those bugs also implemented the status handler side. Are we still using the old hack there?

It probably makes sense to base our implementation on the new syntax after bug 577939.

Comment 1

[Robert Kaiser]

(In reply to comment #0)
> Are we still using the old hack there?

We do :(
http://mxr.mozilla.org/comm-central/source/suite/browser/nsBrowserStatusHandler.js#326

Comment 2

[[e-mail]]

(In reply to comment #0)
> I'm trying to work on bug 386363 but have no idea where to put the call that is
> in there right now, I guess we should just have that notification as well.
What's zoom got to do with tab switching?

Comment 3

[Robert Kaiser]

(In reply to comment #2)
> (In reply to comment #0)
> > I'm trying to work on bug 386363 but have no idea where to put the call that is
> > in there right now, I guess we should just have that notification as well.
> What's zoom got to do with tab switching?

If we remember zoom levels on a site-specific basis, we need to react to changed site zoom settings when switching to a tab with a page of that site open.

Comment 4

[Misak Khachatryan]

Attachment: patch v1

Patch for review.

Comment 5

[[e-mail]]

Why is using a location change notification an old hack?

Comment 6

[Robert Kaiser]

(In reply to comment #5)
> Why is using a location change notification an old hack?

I was referring to the "// XXX temporary hack for bug 104532." from 2003. A 7-years-old "temporary hack" is an "old hack" IMHO. ;-)

Comment 7

[Robert Kaiser]

Comment on attachment 466291 [details] [diff] [review]
patch v1

>+  onUpdateCurrentBrowser: function (aStateFlags, aStatus, aMessage, aTotalProgress) {
>+    if (FullZoom.updateBackgroundTabs)
>+      FullZoom.onLocationChange(gBrowser.currentURI, true);

As said over there, leave the latter twop lines of that snippet to bug 386363.

Comment 8

[Misak Khachatryan]

Also i just noted that one line from patch is wrong

if (content.document && this.mimeTypeIsTextBased(content.document.contentType))

This line shouldn't be changed.

Comment 9

[[e-mail]]

Comment on attachment 466291 [details] [diff] [review]
patch v1

Parts of this looks like part of another patch "Update status bar correctly after tab switching", rather than "Add onUpdateCurrentBrowser notification". But I've commented on that part of the code anyway, since it's here.

>+          // For keyword URIs clear the user typed value since they will be changed into real URIs
>+          if (location.scheme == "keyword" && aWebProgress.DOMWindow == content)
>+            gBrowser.userTypedValue = null;
What does this achieve?

>-        if (content.document && this.mimeTypeIsTextBased(content.document.contentType))
>+        if (content.document && mimeTypeIsTextBased(content.document.contentType))
This change looks wrong.

>-    // XXX temporary hack for bug 104532.
>-    // Depends heavily on setOverLink implementation
>-    if (!aRequest)
>-      this.status = this.jsStatus = this.jsDefaultStatus = "";
Note that jsStatus and jsDefaultStatus still belong to the previous tab, so you can't not clear them.

>+  // simulate all change notifications after switching tabs
onLocationChange, onSecurityChange and onStateChange are simulated in tabbrowser. So why are onStatusChange and onProgressChange simulated here?

>+    if (FullZoom.updateBackgroundTabs)
>+      FullZoom.onLocationChange(gBrowser.currentURI, true);
This looks like part of another patch.

>+              if (oldBlank) {
>+                this.mTabBrowser._callProgressListeners(this.mBrowser, "onUpdateCurrentBrowser",
>+                                                        [aStateFlags, aStatus, "", 0],
>+                                                        true, false);
And I still don't know what onUpdateCurrentBrowser is for and why we only call it in some very specific circumstances.

Comment 10

[Misak Khachatryan]

Attachment: v2

(In reply to comment #9)
> Comment on attachment 466291 [details] [diff] [review]
> patch v1
> 
> Parts of this looks like part of another patch "Update status bar correctly
> after tab switching", rather than "Add onUpdateCurrentBrowser notification".
> But I've commented on that part of the code anyway, since it's here.
> 

I can't properly separate them, so i ported everything i thought related to my current work.

> >+          // For keyword URIs clear the user typed value since they will be changed into real URIs
> >+          if (location.scheme == "keyword" && aWebProgress.DOMWindow == content)
> >+            gBrowser.userTypedValue = null;
> What does this achieve?
> 

Don't know exactly, but from comment i can say that if user typed keyword to open it clears it so they can be replaced by real url.

> >-        if (content.document && this.mimeTypeIsTextBased(content.document.contentType))
> >+        if (content.document && mimeTypeIsTextBased(content.document.contentType))
> This change looks wrong.

Fixed, comment #8
> 
> >-    // XXX temporary hack for bug 104532.
> >-    // Depends heavily on setOverLink implementation
> >-    if (!aRequest)
> >-      this.status = this.jsStatus = this.jsDefaultStatus = "";
> Note that jsStatus and jsDefaultStatus still belong to the previous tab, so you
> can't not clear them.
> 

Firefox didn't clear them, and this line above is the only difference between FF and SM. How i can check the effect of not clearing them ?

> >+  // simulate all change notifications after switching tabs
> onLocationChange, onSecurityChange and onStateChange are simulated in
> tabbrowser. So why are onStatusChange and onProgressChange simulated here?
> 
> >+    if (FullZoom.updateBackgroundTabs)
> >+      FullZoom.onLocationChange(gBrowser.currentURI, true);
> This looks like part of another patch.

This belongs to KaiRo, removed.

> 
> >+              if (oldBlank) {
> >+                this.mTabBrowser._callProgressListeners(this.mBrowser, "onUpdateCurrentBrowser",
> >+                                                        [aStateFlags, aStatus, "", 0],
> >+                                                        true, false);
> And I still don't know what onUpdateCurrentBrowser is for and why we only call
> it in some very specific circumstances.

I don't know exactly too, but for Wayne Woods's question and Simon Bünzli's answer about comparison of our old fix and this new one you can read from https://bugzilla.mozilla.org/show_bug.cgi?id=327604#c14

Comment 11

[Robert Kaiser]

Comment on attachment 466300 [details] [diff] [review]
v2


>             // Update the URL bar.
>             this.updateUrlBar(newBrowser.webProgress,
>                               null,
>                               newBrowser.currentURI,
>                               newBrowser.securityUI,
>                               newBrowser,
>                               this.mTabListeners[this.tabContainer.selectedIndex].mFeeds);
> 
>+            var listener = this.mTabListeners[this.tabContainer.selectedIndex] || null;
>+            if (listener && listener.mStateFlags) {
>+              this._callProgressListeners(null, "onUpdateCurrentBrowser",
>+                                          [listener.mStateFlags, listener.mStatus,
>+                                           listener.mMessage, listener.mTotalProgress],
>+                                          true, false);
>+            }
>+
>             // Update the window title.
>             this.updateTitlebar();
> 
>             // FAYT
>             this.fastFind.setDocShell(this.mCurrentBrowser.docShell);


Oops. You really should fire this right before the onStateChange and _after_ updating window title and FAYT. Between updating the URL bar and the window title is a very strange choice. ;-)

Comment 12

[Misak Khachatryan]

Attachment: v2a

Fixed issue pointed by KaiRo

Comment 13

[[e-mail]]

(In reply to comment #6)
> (In reply to comment #5)
> > Why is using a location change notification an old hack?
> I was referring to the "// XXX temporary hack for bug 104532." from 2003. A
> 7-years-old "temporary hack" is an "old hack" IMHO. ;-)
And one which Firefox copied in bug 327604! I don't know why it no longer exists on trunk though, since that would mean that the JS status string could persist between tab switches.

Comment 14

[Robert Kaiser]

(In reply to comment #13)
> And one which Firefox copied in bug 327604! 

Not that I find any checkin for it in http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/browser/base/content/browser.js -  the only checkin for that bug is http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/browser/base/content&command=DIFF_FRAMESET&file=browser.js&rev2=1.595&rev1=1.594

Comment 15

[Misak Khachatryan]

Review ping ...

Comment 16

[Misak Khachatryan]

Attachment: patch v2b

(In reply to comment #9)
> Comment on attachment 466291 [details] [diff] [review]
> >-    // XXX temporary hack for bug 104532.
> >-    // Depends heavily on setOverLink implementation
> >-    if (!aRequest)
> >-      this.status = this.jsStatus = this.jsDefaultStatus = "";
> Note that jsStatus and jsDefaultStatus still belong to the previous tab, so you
> can't not clear them.

Leaved this code for jsStatus and jsDefaultStatus.

> 
> >+  // simulate all change notifications after switching tabs
> onLocationChange, onSecurityChange and onStateChange are simulated in
> tabbrowser. So why are onStatusChange and onProgressChange simulated here?
> 

Here is also many other onSomethingChange - if you want all of them moved to tabbrowser, I can do it as followup.

Comment 17

[[e-mail]]

Attachment: WIP

This seems to restore:
1. The progress bar
2. The message (Connecting/Waiting/Transferring etc.)
But it occurs to me that maybe I shouldn't have changed the onStateChange code, I should have only added the progress/status changes.

Comment 18

[[e-mail]]

Attachment: Proposed patch

OK, so the differences between this patch and attachment 479952 [details] [diff] [review] are:
* Added the onUpdateCurrentBrowser notification for extension compatibility
* Only pass the STATE_START/STATE_STOP flags to onStateChange.
  nsBrowserStatusHandler.js gets confused if it sees STATE_IS_NETWORK.
  This worked before because we used to send a null request.
  (Should we remove the null-checks from nsBrowserStatusHandler.js?)

Comment 19

[Misak Khachatryan]

Comment on attachment 497273 [details] [diff] [review]
Proposed patch

I ran all test with this patch applied, no failures noticed.

Comment 20

[[e-mail]]

Pushed changeset afdde738a668 to comm-central.