238696 - URL bar does not update if 2nd ProgressListener added to <browser>

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Mark Smith [:mcs]]

I have a sidebar that adds a listener to the tabbed browser, like so:

  top.getBrowser().addProgressListener(mySidebarProgressListener,
                       Components.interfaces.nsIWebProgress.NOTIFY_LOCATION);

But there is a bad side effect: the URL bar text never updates once my sidebar
is loaded.  For example, if I click on a link in the browser I expect the URL
bar to show the URL of the new page once it loads -- but it does not change at all.

This bug only occurs when there are no tabs; if there is more than one tab open,
the problem goes away.

Comment 1

[Mark Smith [:mcs]]

This may be related to the changes made for bug 144533.

Comment 2

[Kathleen :Brade]

Attachment: proposed patch v1

Comment 3

[Kathleen :Brade]

Attachment: patch v1 with -w

Comment 4

[Mark Smith [:mcs]]

I did not do a very good of searching the open bugs.  This looks like a
duplicated of bug 216900.

Comment 5

[[e-mail]]

Comment on attachment 144776 [details] [diff] [review]
patch v1 with -w

>+                  // Transition from one listener to mTabListeners.
>+                  // Remove the progress listeners from the active browser's filter.
>+                  var p = this.mProgressListeners[0];
>+                  if (p)
>+                    this.mTabFilters[0].removeProgressListener(p);
>+
>+                  // Wire up a progress listener to our filter.
>+                  const listener = this.mTabProgressListener(this.mCurrentTab,
>+                                                             this.mCurrentBrowser,
>+                                                             false);
>+                  this.mTabFilters[0].addProgressListener(listener,
>+                                                          Components.interfaces.nsIWebProgress.NOTIFY_ALL);
>+                  this.mTabListeners[0] = listener;
When you do later enter tabbed mode, this will stomp over the work you've done
here - it tries to remove the progressListner[0] again and adds a new tab
listener; that happens to work because the tab filter does no error checking.
(Of course, if the filter did some basic error checking then this bug would
never have arisen in the first place...)

Comment 6

[Kathleen :Brade]

Attachment: patch v2 (simpler)

Comment 7

[Kathleen :Brade]

Attachment: patch v2 with -w

Comment 8

[Kathleen :Brade]

*** Bug 216900 has been marked as a duplicate of this bug. ***

Comment 9

[Adam Hauner]

Brade: Should this bug affect also site icon in URL Bar (bug 231427, testcase B
in comment #4)?

Comment 10

[[e-mail]]

Comment on attachment 144847 [details] [diff] [review]
patch v2 (simpler)

>+              // If we are adding a 2nd progress listener, we need to go to mTabbedMode
Looks ungrammatical to me; "we need to enter tabbed mode" perhaps?

>+              if (this.mTabFilters.length == 0) {
As discussed on IRC, I thought of a possible reason why the progress listeners
aren't spliced in which case you wouldn't need this change.

Comment 11

[jag (Peter Annema)]

I like the second patch much better.

Do we still need this?

  if (this.mTabFilters.length == 0) {

It looks like you need that guard if you have multiple progress listeners on the
tab browser while not in tabbed mode. Actually, you'd need something better,
namely the ability to register the 2nd etc. progress listeners with the filter,
but the filter only supports one listener currently. However, because of the
first part of your patch, we won't go into this block of code anymore for the
second progress listener.

Comment 12

[Kathleen :Brade]

jag--my thinking was that the check would still be needed if someone did this:
  addProgressListener, removeProgressListener, addProgressListener
I didn't see the point in creating a new filter and clobbering the first filter.

Strictly speaking, that piece is unrelated to this bug.  I'm happy to remove it
since I don't think it will impact the current browser (which gets first stab at
adding the listener).  It just seemed like a correctness thing (since I was in
the vicinity).  :-)

jag--let me know if you 
 (1) want a new patch without that last section,
 (2) if you prefer a conditional review on removing it or
 (3) you are ok with leaving it in

Comment 13

[Kathleen :Brade]

Adam Hauner (Comment 9)--I don't think bug 231427 really applies in this case. 
This code would only be triggered early if you are in a single browser window
and have a sidebar or other extension that adds a 2nd progress listener.  I
*think* that the problem in that bug is that the favicon wasn't loaded initially
or something but I am far from an expert on that.  I recommend we keep that
issue out of this bug.  :-)

Comment 14

[jag (Peter Annema)]

Comment on attachment 144847 [details] [diff] [review]
patch v2 (simpler)

Hmmm... The 0 length check seems right then. It just looks a little clunky.
Maybe we should really just remove the filter in the no tabs mode when we
remove the progress listener.

I'd say check it in as is, maybe add your comment to the 0 check? sr=jag.

Comment 15

[chris hofmann]

Comment on attachment 144847 [details] [diff] [review]
patch v2 (simpler)

a=chofmann for 1.7

Comment 16

[Kathleen :Brade]

fix checked in

Comment 17

[Daniel Veditz [:dveditz]]

Shouldn't this fix be put into the mozilla/toolkit version of tabbrowser.xml?
Who knows what future extension might try to add another listener.

Comment 18

[Kathleen :Brade]

Yes, this bug should be fixed in mozilla/toolkit.  I don't have a tree to test
that though.  Should I reopen this bug and reassign to someone else or file a
new bug or ?

Comment 19

[Kathleen :Brade]

Attachment: patch for firefox

Comment 20

[Kathleen :Brade]

Comment on attachment 146516 [details] [diff] [review]
patch for firefox

This is the patch ported over; would appreciate it if someone could test it. 
Reviews please! :-)

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 146516 [details] [diff] [review]
patch for firefox

looks fine to me but I'm not a peer in toolkit. You only need one review in
toolkit so I'll add bryner and you can take whichever one you get.

Comment 22

[Asa Dotzler [:asa]]

I don't see this in the list of 1.7 branch checkins. Can someone please land
this on the 1.7 branch or if it's already there, add the fixed1.7 keyword? Thanks.

Comment 23

[Kathleen :Brade]

This patch (for mozilla) was landed before the 1.7 branch was created.  The
patch for firefox landed May 6, 2004 on the trunk (rev 1.38).