113798 - Tabbrowser icon(s) do not work properly

Status: RESOLVED EXPIRED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

I think the favicon should always be what the main
frame describes in LINK-element, but in the given
URL when user changes the content of the IFRAME,
a new icon is loaded from server's root. 
And this happens even if all the pages 
includes the same LINK-declaration 
(which does not point to the server's root).

This occurs only when using Tabs, not in the 'one-page-one-window'-mode.

BTW. The Tab loses also the title of the page.

Comment 1

[eberry]

Have you verified by looking at the server logs that the link icon is loaded
each time?  It might just be that the spinner replaces the icon while the
network library loads the file ad the link icon is cached.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: This is a patch v1

This patch makes the icons and titles of the tabs 
to keep right values when browsing 
pages that contains IFRAMES or FRAMES.

This works fine for me.

(BTW. I don't have a CVS access)

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Changing bug-summary. 
was: Favicon and title change wierdly when using IFRAMES and Tabs
now: Favicon changes wierdly when using IFRAMES and Tabs

The 'title'-part of this bug is a dublicate of 
http://bugzilla.mozilla.org/show_bug.cgi?id=101831.

I'll try to make a patch which corrects only the 'icon'-bug.
(My patch for titles has some problems.)

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Patch v.2 to correct some favicon/tabs bugs

So this patch is trying to make icons to show up
more correctly. 
It changes tabbrowser so, that when browsing 
frames, the mainframe's icon will be used (if an icon is defined).

Also:
Formenly when using a page with an icon in "one-page-per-window"-mode and then
starting to work with tabs, the icon was lost. This patch
removes that problem too. 

Probably there is still something that could be made better (or a bit cleaner),

but for now this is enough for me.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Patch v.3

This is a more polished patch.
Could someone try it - I hope it works well enough.

Why is this bug UNCONFIRMED? I see icons to disapearing
in Windows & Linux when using pages with frames.
And also icons get lost sometimes when changing from
'one-tab-mode' to 'multi-tab-mode' (= adding another tab).
This patch is trying to make those things to works properly.

Comment 6

[Asa Dotzler [:asa]]

->hyatt (patch awaiting review)

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

I'll send soon a new patch. It does not use the stupid 
temporary attribute like the patch v. 3.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Patch v. 4

So this is even more clean patch.
Hope it works ok.
It is made with Patch Maker using Linux (Moz 2001121008).
(There is something wierd when trying to
use PM's patches made in Windows in Linux.)

I've been using this patch for a while now 
and it seems to work the way I like.

Comment 9

[timeless]

Comment on attachment 61250 [details] [diff] [review]
Patch v. 4

>--- xpfe/global/./resources/content/bindings/tabbrowser.xml.bak	Tue Dec 11 14:57:58 2001
>+++ xpfe/global/./resources/content/bindings/tabbrowser.xml	Tue Dec 11 16:37:23 2001
>@@ -126,7 +126,10 @@
>+	    _mOldURI: "", //This is a hack for keeping the icon of the mainframe
>+            _mOldIcon: "", //This is a hack for keeping icon of the mainframe
>+            _mTabPanel: null,
your indentations should line up.

>@@ -156,9 +159,19 @@
>               const nsIChannel = Components.interfaces.nsIChannel;
>               if (!this.mBlank && aStateFlags & nsIWebProgressListener.STATE_START && 
>                   aStateFlags & nsIWebProgressListener.STATE_IS_NETWORK) {
>+                var i = 0;
this thing here spells like a for loop, you're interating over a list:
<<<<<<
>+                var tab = this.mTabBrowser.mTabContainer.firstChild;
>+                do  {
>+                  if (tab == this.mTab) break;
>+                  ++i;
>+                  tab = tab.nextSibling
>+                }while(tab);
======
for (var tab = this.mTabBrowser.mTabContainer.firstChild;
     tab && tab != this.mTab;
     ++i,
     tab = tab.nextSibling);
>>>>>>
is it possible for you to run out of tabs?
if it is, then this next statement will cause some sort of error.
if not, then please remove the tab condition from the loop.
>+                this._mTabPanel = this.mTabBrowser.mPanelContainer.childNodes[i];
>+                this._mOldURI = this._mTabPanel.currentURI;
>                 this.mTab.setAttribute("busy", "true"); 
>                 this.mTab.label = this.mTabBrowser.mStringBundle.getString("tabs.loading");
>                 this.mTab.removeAttribute("image");
>+                this._mOldIcon = this.mIcon;
>                 this.mIcon = "";
> 
>                 if (this.mTabBrowser.mCurrentTab == this.mTab)
>@@ -166,12 +179,22 @@
>               }
>               else if (aStateFlags & nsIWebProgressListener.STATE_STOP &&
>                        aStateFlags & nsIWebProgressListener.STATE_IS_NETWORK) {
>+                var newuri = "";
>+                if(this._mTabPanel)
>+                  newuri = this._mTabPanel.currentURI;
>+                this._mTabPanel = null;
>+                var wasURIChanged = !(newuri == this._mOldURI);
>+                this._mOldURI = "";
>                 if (this.mBlank)
>                   this.mBlank = false;
>                 
>                 this.mTab.removeAttribute("busy");
>                   
>                 var location = aRequest.QueryInterface(nsIChannel).URI;
>+                if(!wasURIChanged) {
>+                  this.mIcon = this._mOldIcon;
>+                }
hrm. local style would appear to be
if<space>(cond)\n<whitespace-indent>statement\n [no braces]
>+                this._mOldIcon = "";
>                 if (this.mIcon) {
>                   this.mTab.setAttribute("image", this.mIcon);
>                   mIcon = "";
>@@ -526,11 +549,15 @@
>               // Create our close box.
>               this.createCloseBox();
> 
>-              // Hook up our favicon.
>-              var uri = this.mCurrentBrowser.currentURI;
>-              if (this.shouldLoadFavIcon(uri))
>-                this.loadFavIcon(uri, "image", this.mCurrentTab);
>+              // Hook up our favicon. We'll get the same icon what was used already.
>+              var iconhref = null;
>+              if(gProxyFavIcon){
again, if<space>...
>+                iconhref = gProxyFavIcon.getAttribute('src');
>+                if(iconhref)
>+                  this.mCurrentTab.setAttribute('image', iconhref);
>+              }
>                 
>+
addition of a random newline ^^
>               // Remove all our progress listeners from the active browser.
>               if (this.mProgressListeners) {
>                 for (var i = 0; i < this.mProgressListeners.length; i++) {

clearly a final patch would use mFoo for member variables instead of _mFoo.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Thanks for comments.
I'm just starting to make patches to Mozilla, 
so there are many things to learn about 
Mozilla's coding style, etc.

Those "your indentations should line up." are
made by Patch Maker, in my code indentations are ok.

"if<space>(cond)\n<whitespace-indent>statement\n [no braces]"
I forgot to use this, instead I used what I'm used to do.

"clearly a final patch would use mFoo for member variables instead of _mFoo."
I saw this _mFoo elsewhere too, so thought it could be
allowed to use it. And I think it is even pretty clear this way
that those variables are kind of 'private'-variables.  

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Patch v. 5 !

I really hope that this patch might be good enough.
(I should think more and write less :) )

This patch is correcting also bugs:
http://bugzilla.mozilla.org/show_bug.cgi?id=101831
http://bugzilla.mozilla.org/show_bug.cgi?id=101827
or at least the test cases started to work as expected.

(This patch is made in Windows with Patch Maker using Moz 2001121003)

Comment 12

[David Hyatt]

There's a much easier way to fix this that just involves testing window parentage.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Hyatt, I don't understand what you mean, but probably 
its just so simple that I cannot see it.

But which part of the patch v5 did you mean anyway.
There are actually fixes for at least these bugs (I suppose):
http://bugzilla.mozilla.org/show_bug.cgi?id=113798
http://bugzilla.mozilla.org/show_bug.cgi?id=101831
http://bugzilla.mozilla.org/show_bug.cgi?id=101827
and a bug when icon get lost when changing from
'one-page-one-window'-mode to tabbed mode.
(is it http://bugzilla.mozilla.org/show_bug.cgi?id=109942 ?)

and maybe:
http://bugzilla.mozilla.org/show_bug.cgi?id=109959
(I can't test this currently, because I'm using an old Moz.)

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Patch v.6 (wondering, how many there will be ;)

Because I didn't understand what Hyatt was saying I made 
still a new patch, which actually corrects bugs (I hope):
http://bugzilla.mozilla.org/show_bug.cgi?id=113798
http://bugzilla.mozilla.org/show_bug.cgi?id=101831
http://bugzilla.mozilla.org/show_bug.cgi?id=101827
http://bugzilla.mozilla.org/show_bug.cgi?id=109942
http://bugzilla.mozilla.org/show_bug.cgi?id=109959
http://bugzilla.mozilla.org/show_bug.cgi?id=105842
http://bugzilla.mozilla.org/show_bug.cgi?id=108189
http://bugzilla.mozilla.org/show_bug.cgi?id=108350
(Some of these are probably dublicates.)

The whole patch is becoming too large, but 
unfortunately there is pretty many bugs 
in the handling of titles and icons.
Maybe the icon&title -stuff should be somehow
rearranged?

(This patch was made in Linux with Patch Maker using Moz 2001121108)

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Patch 2, v.1

This totally rewritten patch is for current tabbrowser.
It includes some parts of [e-mail]'s patch
in http://bugzilla.mozilla.org/show_bug.cgi?id=101827.

There was not enough parentNodes in <xul:tab
onerror="this.parentNode.parentNode.., 
this patch fixes that too.

(PatchMaker / Win98 / Moz 2002010203)

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Has anyone tried my patch.
For me it has been working well in windows
and linux.

Unfortunately there are still some 
bugs in tabs (or maybe those are not tab-bugs 
but something else). Anyway I think 
the patch is fixing the most common bugs conserning
titles and icons.

reviews?

Comment 17

[David Hyatt]

I'm not sure we should be setting an icon property on the document.  That seems
kind of hacky to me.

Comment 18

[David Hyatt]

What was wrong with the mIcon property being on the listener?  Why did it have
to be removed?

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

I think the icon should be in the document-object.
It is easier to use, and it is really a property
of a document - document might or might not have an icon.
So somehow an icon is a 'member' of a document.

And I can't understand why an icon should be a property of
a TabProgressListener-object. Those are pretty unrelated 
things.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Patch 2, v.1.1

Still one patch.
Polishing syntax and adding 2 hasAttribute -checks 
(and fixing one bug more, I don't know the bug-id :) .

Using mIcon-property would make
the code a bit more unclean, especially in the addTab and onLinkAdded -methods.

So I do prefer the icon-property in document.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Patch 2, v.1.2

Well, this is the patch which uses mIcon-property in
TabProgressListener-objects. (The icon-property in
document-objects is removed.)

Reviews?

(If this is finally OK, I hope someone could
check this in.)

Comment 22

[David Hyatt]

I think we should go further and make tabs completely ignore both starts and
stops that are initiated because of iframe transitions.  It doesn't make sense
to put in the animating icon and the "Loading..." message when only an iframe is
changing.

Thoughts?

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

I think there should be some thing to
indicate that a frame is loading.

It should be possible to set a tab to load some
big frame and meanwhile browse another tab.
And the indicator (=icon) tells to the user
when the big-frame-tab is ready to use (=loaded).
This is important especially with slow internet connections.
A good example is the http://java.sun.com/j2se/1.4/docs/api/index.html.

So that's why we need the start-stop -state handling
in tabbrowser too, I think.

But maybe the animated icon is enough with frames
and Loading... -text should be seen only when
a totally new page is loading?

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Patch 2, v.1.3

This is the diff for current tabbrowser.xml

Added some minor things, so that the patch is
working even better.

This patch is fixing also bug
http://bugzilla.mozilla.org/show_bug.cgi?id=107669
(at least for me).

(PatchMaker / Linux / Moz 2002012106)

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Patch 3, v1

This patch fixes even more bugs.
It also includes a hack for http://bugzilla.mozilla.org/show_bug.cgi?id=103452.

(That might be just a temporary fix, but it is still better than nothing.)

I had to change tabbox.xml#tab a bit too, so that an icon of a tab is always
painted (the default icon or a site/favicon).

(PatchMaker/Linux/Moz2002012921)

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Patch 3, v1.01

Oops, one 'removeAttribute' was missing.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Patch 3, v1.1

Well, this patch could be ready for reviews.
It fixes still more bugs:
1.when using href="#xyz" -style links the icon
  was lost (some cases).
2.when switching tabs the urlbars icon disapeared (in one case).

(Linux / Moz 2002020222)

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Patch 3, v1.2

Sorry for all these obsolete patches...
But I still found one case where the icon disapeared.
(Image-element's onload is sometimes called even if there is no image
to paint, so there must be a hasAttribute('src')-check.)

Reviews?

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

The patch for current tree is in 
http://www.cs.helsinki.fi/u/pettay/bugzilla/

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

I have used the patch for 9 days and it is working very well, I think.
Maybe someone doesn't like the window.close() -hack, but otherwise
the code should be pretty clean.
So reviews or at least some comments?

Comment 31

[jag (Peter Annema)]

Would you mind splitting this patch into patches for each of the issues it tries
to address? You can lump things together if the issues are related or interact.
Attach these patches to whichever bug report covers the issue (see your comment
#14 for a list).

The reason I ask this is because that way we can more easily accept or reject
parts of the current patch, instead of having to reject the whole patch because
of disagreement over a small part of it.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

I'm afraid, it is impossible to split the patch,
the changes to the code of the tabbrowser are to big.
(Only the window.close() -hack could be easily remowed.)
(And I do not have time to do the splitting.)

But I made a commented version of this patch.
http://www.cs.helsinki.fi/u/pettay/bugzilla/bug113798.20020228.diff_with_extra_comments
(All the // -starting lines are comments.)
I hope it makes things a bit more clear.

The newest real patch is still in http://www.cs.helsinki.fi/u/pettay/bugzilla/

Comment 33

[Michiel van Leeuwen (email: mvl+moz@)]

Smaug: I took the parts of the patch that apply to bug 101723, created a
seperate patch, and attached it to that bug. Hope it helps you if you need te
split the patch.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

The bug 101723 is now fixed (?), so this patch
is a bit smaller again.

Comment 35

[Ere Maijala (slow)]

I've extracted the part (hack) that fixes window.close(). Diff attached to bug
103452.

Comment 36

[Ere Maijala (slow)]

Oh, great :( I just didn't happen to notice the comment about the most recent
version until now... well, got to do it again then, I guess.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

The current version fixes http://bugzilla.mozilla.org/show_bug.cgi?id=110421 too.
(Actually there has been a test for image blocking for a long time but it was not
working properly.)
And also fixing some (old) mixed using of URI and URL.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Tabbrowser focus bugs, fix part 1:

The focus of the elements is now remembered in tabbrowser, 
when switching tabs with mouse.

Unfortunately I haven't got it to work with
ctrl-PgUp / ctrl-PgDn. Yet?

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Patch 20020404

I removed all the hacks so now the patch is fixing only
(many) icon and title related bugs in tabbrowser.

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Still a bit better patch, fixing more bugs.
(for example trying to load icon a bit fewer times in some cases.)
Now when <link>-icon cannot be loaded, trying to load favicon.

http://www.cs.helsinki.fi/u/pettay/bugzilla/

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

I'm still using the patch all the time.
It just fixes so many annoying but not so critical
bugs.

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

The newest patch works at least with Moz20020610/WinXP
http://www.cs.helsinki.fi/u/pettay/bugzilla/

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

The current patch fixes one title related bug more.
Tested with WinXP 20020701.
http://www.cs.helsinki.fi/u/pettay/bugzilla/

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

titles were fixed in bug 152127, so changing the title.
Current patch is in the old place.

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

-> me
I'll write a new (smaller?) fix soon.

Comment 46

[Brian 'netdragon' Bober]

Please re-summarize this bug with a more appropriate summary.

Comment 47

[Robert Kaiser]

MASS-CHANGE:
This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state.

If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way.
If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar).
If no action happens within the next few months, we move this bug report to an EXPIRED state.

Query tag for this change: mass-UNCONFIRM-20090614

Comment 48

[Robert Kaiser]

MASS-CHANGE:
This bug report is registered in the SeaMonkey product, but still has no comment since the inception of the SeaMonkey project 5 years ago.

Because of this, we're resolving the bug as EXPIRED.

If you still can reproduce the bug on SeaMonkey 2 or otherwise think it's still valid, please REOPEN it and if it is a platform or toolkit issue, move it to the according component.

Query tag for this change: EXPIRED-20100420