Closed Bug 583317 Opened 10 years ago Closed 9 years ago

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

Categories

(SeaMonkey :: Tabbed Browser, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kairo, Assigned: neil)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

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.
Blocks: 386363
(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
Blocks: 586340
(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?
(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.
Attached patch patch v1 (obsolete) — Splinter Review
Patch for review.
Assignee: nobody → misak.bugzilla
Status: NEW → ASSIGNED
Attachment #466291 - Flags: review?(neil)
Why is using a location change notification an old hack?
(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 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.
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 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.
Attachment #466291 - Flags: review?(neil) → review-
Attached patch v2 (obsolete) — Splinter Review
(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
Attachment #466291 - Attachment is obsolete: true
Attachment #466300 - Flags: review?(neil)
Summary: Add calls to listeners for onUpdateCurrentBrowser → Update status bar correctly after tab switching and Add calls to listeners for onUpdateCurrentBrowser. Port of bug 327604 and bug 331938
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. ;-)
Attached patch v2a (obsolete) — Splinter Review
Fixed issue pointed by KaiRo
Attachment #466300 - Attachment is obsolete: true
Attachment #467843 - Flags: review?(neil)
Attachment #466300 - Flags: review?(neil)
(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.
Review ping ...
Attached patch patch v2b (obsolete) — Splinter Review
(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.
Attachment #467843 - Attachment is obsolete: true
Attachment #478516 - Flags: review?(neil)
Attachment #467843 - Flags: review?(neil)
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #479952 - Flags: feedback?(misak.bugzilla)
Attachment #479952 - Flags: feedback?(misak.bugzilla) → feedback+
No longer blocks: 386363
Attached patch Proposed patchSplinter Review
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?)
Assignee: misak.bugzilla → neil
Attachment #478516 - Attachment is obsolete: true
Attachment #479952 - Attachment is obsolete: true
Attachment #497273 - Flags: review?(misak.bugzilla)
Attachment #478516 - Flags: review?(neil)
Comment on attachment 497273 [details] [diff] [review]
Proposed patch

I ran all test with this patch applied, no failures noticed.
Attachment #497273 - Flags: review?(misak.bugzilla) → review+
Pushed changeset afdde738a668 to comm-central.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.