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

RESOLVED FIXED

Status

SeaMonkey
Tabbed Browser
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Robert Kaiser, Assigned: neil@parkwaycc.co.uk)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Blocks: 386363
(Reporter)

Comment 1

7 years ago
(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

Updated

7 years ago
Blocks: 586340
(Assignee)

Comment 2

7 years ago
(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?
(Reporter)

Comment 3

7 years ago
(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

7 years ago
Created attachment 466291 [details] [diff] [review]
patch v1

Patch for review.
Assignee: nobody → misak.bugzilla
Status: NEW → ASSIGNED
Attachment #466291 - Flags: review?(neil)
(Assignee)

Comment 5

7 years ago
Why is using a location change notification an old hack?
(Reporter)

Comment 6

7 years ago
(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. ;-)
(Reporter)

Comment 7

7 years ago
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

7 years ago
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.
(Assignee)

Comment 9

7 years ago
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-

Comment 10

7 years ago
Created attachment 466300 [details] [diff] [review]
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
Attachment #466291 - Attachment is obsolete: true
Attachment #466300 - Flags: review?(neil)

Updated

7 years ago
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
(Reporter)

Comment 11

7 years ago
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

7 years ago
Created attachment 467843 [details] [diff] [review]
v2a

Fixed issue pointed by KaiRo
Attachment #466300 - Attachment is obsolete: true
Attachment #467843 - Flags: review?(neil)
Attachment #466300 - Flags: review?(neil)
(Assignee)

Comment 13

7 years ago
(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.
(Reporter)

Comment 14

7 years ago
(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

7 years ago
Review ping ...

Comment 16

7 years ago
Created attachment 478516 [details] [diff] [review]
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.
Attachment #467843 - Attachment is obsolete: true
Attachment #478516 - Flags: review?(neil)
Attachment #467843 - Flags: review?(neil)
(Assignee)

Comment 17

7 years ago
Created attachment 479952 [details] [diff] [review]
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.
Attachment #479952 - Flags: feedback?(misak.bugzilla)

Updated

7 years ago
Attachment #479952 - Flags: feedback?(misak.bugzilla) → feedback+
(Reporter)

Updated

7 years ago
No longer blocks: 386363
(Assignee)

Comment 18

7 years ago
Created attachment 497273 [details] [diff] [review]
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?)
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 19

7 years ago
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+
(Assignee)

Comment 20

7 years ago
Pushed changeset afdde738a668 to comm-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Blocks: 756572
You need to log in before you can comment on or make changes to this bug.