449730 - favicon doesn't show up when dragging a tab between browser windows

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P2)

Description

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

Load a page with favicon (for example www.mozilla.org) and drag the tab to
some other window. The tab and location bar have just the default favicon, not
the one which was there in the first window.
Does tabbrowser rely on DOMLinkAdded event to update the favicon?

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Yeah, it does. Need to transfer mIconURL in

http://hg.mozilla.org/mozilla-central/index.cgi/annotate/cb95cbab62627c2a42e1adabedfc769a2472489b/toolkit/content/widgets/browser.xml#l961

and maybe call updateIcon after

http://hg.mozilla.org/mozilla-central/index.cgi/annotate/cb95cbab62627c2a42e1adabedfc769a2472489b/browser/base/content/tabbrowser.xml#l1965 I think.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Works fine in Firefox 3.0.x but fails in Firefox 3.1 nightly builds. Adding missing regression flag.

Comment 10

[tabmix.onemen]

also when we drag or detached tab while its loading the progress bar isnt updated and the tba not look busy

Comment 12

[Dietrich Ayala (:dietrich)]

prioritizing at P2 given the high volume of dupes being filed about this.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Mano: please reassign back to nobody@moz if this isn't going to be part of the fix you're working up already, and if you won't have time to get to it.

Comment 14

[John P Baker]

Attachment: patch v1

This seems to be on the right track.

A couple of points:
1) I am not sure where the various tabs and browsers are valid so there may be a better location for the code.
2) There may be other attributes that need setting.

Comment 15

[John P Baker]

(In reply to comment #14)
> 1) I am not sure where the various tabs and browsers are valid so there may be
> a better location for the code.

In particular - see comment #2 which I overlooked

> ourBrowser.mIconURL = remoteBrowser.getBrowserForTab(aOtherTab).mIconURL;

should probably be in fieldsToSwap in swapDocShells

Comment 16

[John P Baker]

Attachment: patch v2

Address my own point (1) apropos code location.

I don't now believe that mIconURL setting should be done in swapDocShells.

Comment 17

[John P Baker]

Attachment: patch for 1.9.1 branch

Although I think I am going in the right direction, the time between b3 and RC1 is not the time to be too adventurous [1]. Just implement comment #1 for the 1.9.1 branch.

I have left the check for busy in even though, as the progress listeners have just been set up, I doubt that it will ever be set; But I don't even want the possibility of a loading throbber with a page title.

[1] As well as the correct busy state, there are things like the feed icon and search button glow - but the correct status could easily get lost while dragging a loading tab.

Comment 18

[John P Baker]

Attachment: patch for 1.9.1 branch

Sorry!

Comment 19

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

Is the patch ready? You need to ask review, perhaps from mano.
attachment -> details -> review? <bugzilla-id-of-the-requestee>

Comment 20

[John P Baker]

Comment on attachment 360510 [details] [diff] [review]
patch for 1.9.1 branch

(In reply to comment #17)
> [1] As well as the correct busy state, there are things like the feed icon and
> search button glow - but the correct status could easily get lost while
> dragging a loading tab.

These have been spun off as bug 477014

(In reply to comment #19)
> Is the patch ready? You need to ask review, perhaps from mano.
> attachment -> details -> review? <bugzilla-id-of-the-requestee>

I wasn't sure what would be a polite delay for a response to comment #13

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Putting up a patch is always polite. :)

Mano: ETA for review?

Comment 22

[Boris Zbarsky [:bzbarsky]]

That patch doesn't look right to me.  It should update both mIconURLs, no?

Comment 23

[Asaf Romano (gone)]

Comment on attachment 360510 [details] [diff] [review]
patch for 1.9.1 branch

What Boris said.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

What other mIconURL is there to update?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Oh, I guess this always closes the other browser.  For some reason I thought that tabbrowser supported swapping two <browser>s in general....

In that case, the patch looks OK for the mIconURL part; no idea about the busy handling.

Comment 26

[Asaf Romano (gone)]

Comment on attachment 360510 [details] [diff] [review]
patch for 1.9.1 branch

Yes, I realized the context only after marking.

For the "busy" part, where is the "busy" attribute copied (and why isn't mIsBusy set there too).

Comment 27

[:Gavin Sharp [email: [e-mail]]]

AFAICT we don't copy the "busy" attribute - has that part been tested? If we did that, I think this patch would be good to go.

John, why don't you think mIconURL belongs in fieldsToSwap? It's not directly tied to the docShell, but it needs to be copied as well, so I think it makes sense to be there (though you might want to adjust the comment).

Comment 28

[John P Baker]

In reply to comment #27)
> AFAICT we don't copy the "busy" attribute - has that part been tested? If we
> did that, I think this patch would be good to go.

The problem is that there is other state held in tabbrowser (eg mIsBusy, mRequestCount) that might need to be kept in step. Which is why I filed bug 477104

> John, why don't you think mIconURL belongs in fieldsToSwap? It's not directly
> tied to the docShell, but it needs to be copied as well, so I think it makes
> sense to be there (though you might want to adjust the comment).

Mainly as it wasn't tied to the docShell, but I am happy either way.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

Ah, right, I forgot about bug 477014. In that case, this bug can just cover adding mIconURL to fieldsToSwap, right? Do you want to write that patch?

Comment 30

[John P Baker]

Attachment: patch v3

Moves copy (now swap) of mIconURL into swapDocShells. This patch is now as described in comment #27 [I'll look at simplified patch, without setting busy attribute, later today]

Comment 31

[John P Baker]

Attachment: patch for 1.9.1 branch

I still feel that checking the busy state for setTabTitle/setTabTitleLoading is technically correct - updateIcon will use it to decide between throbber and favicon and I want the title to match correctly - even though we are pretty sure _at the moment_ that it is never set.

Comment 32

[Asaf Romano (gone)]

Why isn't mIsBusy also swapped?

Comment 33

[:Gavin Sharp [email: [e-mail]]]

I thought we'd agreed to deal with load-related state in bug 477014, rather than in this bug. The fix the case of dragging an already-loaded tab to a new window, the mIconURL change and the addition of the call to updateIcon() is all that's needed, right? I'm trying to simplify this fix so that we can get it landed as soon as possible and fix the common case without having to sort out the more complicated issues related to loading state in bug 477014.

I'll r+ a patch that just includes the mIconURL and updateIcon() changes (as well as the comment change above fieldsToSwap, perhaps just adding "or related to the loaded page").

Comment 34

[John P Baker]

Attachment: trunk patch

(In reply to comment #33)
>
> I'll r+ a patch that just includes the mIconURL and updateIcon() changes (as
> well as the comment change above fieldsToSwap, perhaps just adding "or related
> to the loaded page").

I'm all for getting this off the radar. Patch as described in comment #1 (trunk).

Comment 35

[John P Baker]

Attachment: branch patch

Patch as described in comment #1 (1.9.1 branch).

Comment 36

[:Gavin Sharp [email: [e-mail]]]

Thanks, John.

Comment 37

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/04dfbaa7ed66
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1af81db46cc5

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on trunk and 1.9.1 branch with builds on OS X and Windows:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090213 Shiretoko/3.1b3pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090213 Minefield/3.2a1pre