Closed
Bug 585511
Opened 14 years ago
Closed 14 years ago
Icons sometimes set on wrong tab
Categories
(SeaMonkey :: Tabbed Browser, defect)
SeaMonkey
Tabbed Browser
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.1a3
People
(Reporter: kairo, Assigned: kairo)
References
Details
Attachments
(2 files)
5.80 KB,
patch
|
Callek
:
review+
|
Details | Diff | Splinter Review |
2.16 KB,
patch
|
Callek
:
review+
|
Details | Diff | Splinter Review |
Ever since my work on bug 554908 I watch tab icons more closely, and every now and then saw icons be magically shifted one tab to the right. Today, finally, I could reproduce it as much as to find out that when this happens, the indices of .tabs and .browsers in tabbrowser are out of sync, and so http://mxr.mozilla.org/comm-central/source/suite/browser/tabbrowser.xml#1283 sets its icon on the wrong tab. I also found out that it always happens when a tab left to the one being reloaded had been closed before and none added. This lead me to realize that the problem must be with _browsers invalidation in the removeTab method, and adding a Tab, which also invalidates that cache, works correctly. I had done some small-ish not-completely-related cleanup in tabbrowser until I reached this point. My final conclusion was/is that between the current invalidation in http://mxr.mozilla.org/comm-central/source/suite/browser/tabbrowser.xml#1698 and the actual removal of the tab from the tabContainer, we call something (when dispatching the TabClose event by chance?) that reads .browsers and therefore reconstructs the cache in the old to-be-obsoleted state. Firefox does the invalidation right before it removes the tab from the container, and when I moved it there, I couldn't reproduce the failure any more. Now that I know how to reproduce, I always can, by the way (in a build before my change): 1) Open a few tabs with different icons. 2) Close one in the middle of the tab bar. 3) Go to the one just right of where the closed one was and hit reload. 4) See the icon this tab should have appear to the one right of it.
Assignee | ||
Updated•14 years ago
|
blocking-seamonkey2.1: --- → ?
Assignee | ||
Comment 1•14 years ago
|
||
Here's a patch that fixes this and does some additional cleanup I came across while investigating this. The real fix is the move of |this._browsers = null| to the later place, I also adjusted the comment to the same that's used by Firefox, as it explains much better why we are doing that right there. The first three hunks clean up the onLinkAdded stuff a bit wrt to creating URI objects, making use of Services and removing that dependency on makeURI I introduced, as it's really unneeded. The last hunk makes the actual retrieval of .browsers and buildup of that _browsers cache much nicer, this is taken 1:1 from Firefox and much simpler than what we had before with the use of .map()!
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #463982 -
Flags: review?(neil)
Attachment #463982 -
Flags: review?(bugspam.Callek)
Assignee | ||
Comment 2•14 years ago
|
||
Oh, forgot to say, I'll take the review from whoever of you two is getting to it first, please cancel the other one when you comment on the patch!
Comment 3•14 years ago
|
||
Comment on attachment 463982 [details] [diff] [review] v1: move invalidation, do some investigation-related cleanup Nit: if its not too hard to test, I'd love a browser-chrome test for this bug, so we can be sure not to regress.
Attachment #463982 -
Flags: review?(neil)
Attachment #463982 -
Flags: review?(bugspam.Callek)
Attachment #463982 -
Flags: review+
Comment 4•14 years ago
|
||
Comment on attachment 463982 [details] [diff] [review] v1: move invalidation, do some investigation-related cleanup >- browsers.item = function(i) {return this[i];} This line should not have been deleted...
Attachment #463982 -
Flags: review-
Assignee | ||
Comment 5•14 years ago
|
||
(In reply to comment #4) > >- browsers.item = function(i) {return this[i];} > This line should not have been deleted... And why, please? Firefox doesn't need it, and I saw no errors in my testing without it, i.e. with this patch as it is.
Assignee | ||
Comment 6•14 years ago
|
||
Thanks, Callek, for the reminder that such things should have a test. And here is one that fails without the patch and succeeds with it. And it's soooo easy! ;-)
Attachment #464070 -
Flags: review?(bugspam.Callek)
Comment 7•14 years ago
|
||
Comment on attachment 463982 [details] [diff] [review] v1: move invalidation, do some investigation-related cleanup Withdrawing my r- since Ratty suggests that that line isn't necessary any more. I don't really want to use Services.io but it's better than using makeURI. [XBL fields are best because they are already lazy.]
Attachment #463982 -
Flags: review-
Comment 8•14 years ago
|
||
Comment on attachment 464070 [details] [diff] [review] test for this bug This is good at testing for underlying bug, I was also hoping something could test the user-facing issue we experienced. (The site-icon on wrong tab), but if that's not easy, I'm not expecting it.
Attachment #464070 -
Flags: review?(bugspam.Callek) → review+
Assignee | ||
Comment 9•14 years ago
|
||
After some talk on IRC, Callek approved landing on the tree, given the fact that the bustage is probably just shared builds and static ones might succeed, i.e. we'll have regular nightlies, and we want this to be fixed there. Pushed main patch as http://hg.mozilla.org/comm-central/rev/65c8c3ebfb11 and test as http://hg.mozilla.org/comm-central/rev/6d7d75ff7a84
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a3
Updated•14 years ago
|
Version: unspecified → Trunk
You need to log in
before you can comment on or make changes to this bug.
Description
•