Images from theme persist after switching, last through session

VERIFIED FIXED in M18

Status

P1
normal
VERIFIED FIXED
19 years ago
11 years ago

People

(Reporter: jmd, Assigned: pnunn)

Tracking

({regression})

Trunk
regression

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta3+])

Attachments

(1 attachment)

(Reporter)

Description

19 years ago
blue has classic pixmaps in it, classic has modern pixmaps in it... etc

regression.

examples:

go offline in classic.. the icon is from modern

the back (when its active) and reload buttons in blue are from classic
(Reporter)

Updated

19 years ago
Keywords: regression
(Reporter)

Comment 1

19 years ago
ok, this seems to only occur after switching themes once or twice, you cant just
start up in a theme and reproduce...

Comment 2

19 years ago
I'm having trouble reproducing this with a linux cvs build 2000090418.  Will
keep trying.

Comment 3

19 years ago
Setting OS to All: I just saw this on Windows NT4 SP6 with build 2000090504.
OS: Linux → All
Hardware: PC → All

Comment 4

19 years ago
*** Bug 51370 has been marked as a duplicate of this bug. ***

Comment 5

19 years ago
bringing over keywords from 51451 (nominated nsbeta3 by mscott@netscape.com)
51451 was marked dup of 51370 but is really dup of this.
Keywords: correctness, nsbeta3

Comment 6

19 years ago
*** Bug 51561 has been marked as a duplicate of this bug. ***

Comment 7

19 years ago
Sending to Hyatt.  I also vote for this one to be nsbeta3, it is a must fix if we 
want to have skin switching in this product.
Assignee: hangas → hyatt
Component: Themes → Skinability

Comment 8

19 years ago
*** Bug 51617 has been marked as a duplicate of this bug. ***

Comment 9

19 years ago
Need info: Please clarify exact steps to reproduce (including specific themes
and platforms). Also, is there any workaround?
Whiteboard: [need info]

Comment 10

19 years ago
This is a regression.  Someone must have changed the way chrome images are 
cached.  It lasts the rest of the session, so this is terrible.

Comment 11

19 years ago
Workaround would probably to restart moz after a theme switch.

For me just switching themes messes everything up. -- Well actually, I thought 
it was an improvement but ...

Comment 12

19 years ago
Pretty sure NS marketing wouldn't like that, since they added menu items to make
switching even easier.  

Updated

19 years ago
QA Contact: pmac → blakeross
Summary: someone's playing mix-and-match themes → Images from theme persist after switching, last through session

Comment 13

19 years ago
*** Bug 51646 has been marked as a duplicate of this bug. ***

Comment 14

19 years ago
*** Bug 51593 has been marked as a duplicate of this bug. ***
adding self to cc list. i see this regularly with both mac and win32 builds from
9/6/00.
Here's an image so we can truly grasp the ugliness...
Steps to repro:

1) Start up browser in modern.
2) Switch to classic through the themes prefs panel.
3) Voila!  A hybrid skin!

Comment 19

19 years ago
I liked that one. -- It's cute.

Actually I liked the other one two ;-)
For dup-counting:

*** Bug 51451 was marked as a duplicate of bug 51370, which was marked as a
   duplicate of this bug.

*** Bug 51479 was marked as a duplicate of bug 51370, which was marked as a
   duplicate of this bug.

Clearing [need info], since we now have platforms (All), steps to reproduce 
(switch themes), and workaround (restart Mozilla) as requested. :-)
Whiteboard: [need info]

Comment 21

19 years ago
Thanks, we get the picture, and have more than enough info for triage this aft.

Comment 22

19 years ago
->pnunn, per hyatt, who says you know what to do. cc hyatt
Assignee: hyatt → pnunn

Comment 23

19 years ago
*** Bug 51823 has been marked as a duplicate of this bug. ***

Comment 24

19 years ago
*** Bug 51823 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 25

19 years ago
9 dups -> mostfreq
Keywords: mostfreq
(Assignee)

Comment 26

19 years ago
I'm on it.
Status: NEW → ASSIGNED

Comment 27

19 years ago
*** Bug 51987 has been marked as a duplicate of this bug. ***

Comment 28

19 years ago
Adding CC

Comment 29

19 years ago
This is happening because IL_FlushCache refuses to remove chrome images from the 
image cache. I think that it should flush chrome images.

Comment 30

19 years ago
Here's a patch:

Index: mozilla/modules/libimg/src/ilclient.cpp
===================================================================
RCS file: /cvsroot/mozilla/modules/libimg/src/ilclient.cpp,v
retrieving revision 3.30
diff -u -2 -r3.30 ilclient.cpp
--- ilclient.cpp	2000/09/06 00:23:46	3.30
+++ ilclient.cpp	2000/09/10 21:09:05
@@ -715,10 +715,12 @@
     if (ic)
     {
+/*
 #ifdef PIN_CHROME
-    	NS_ASSERTION((ic->moz_type != TYPE_CHROME), "removing a chrome image from 
the image cache"); 
+    	NS_ASSERTION((ic->moz_type != TYPE_CHROME), "removing a chrome image from 
the image cache");
 #endif
 
 	    if ( ic->moz_type == TYPE_CHROME )
 		    printf( "Image type is chrome\n" );	
+*/
         ILTRACE(2,("il: remove ic=0x%08x from cache\n", ic));
         PR_ASSERT(ic->next || ic->prev || (il_cache.head == il_cache.tail));
@@ -871,5 +873,5 @@
     while (ic)
     {
-        if (ic->is_in_use || ic->moz_type == TYPE_CHROME) {
+        if (ic->is_in_use /* || ic->moz_type == TYPE_CHROME */) {
 #ifdef DEBUG_kipp
             printf("IL_FlushCache: il_container %p in use '%s'\n",

Comment 31

19 years ago
Hmmm... This completely breaks the idea of keeping chrome images longer. I have 
2 possible alternate solutions--

1. Somehow pass a flag in IL_FlushCache that identifies the skin-switch and 
hence flushes the chrome. 

2. Add a forceFlush call to Imagelib that gets called when the skin-switching 
occurs. This would just completely blow away any exising chrome in the imglib 
cache.
(Assignee)

Comment 32

19 years ago
I've got an idea that I haven't yet had time to test out.
(A new call that will reset all the chrome_types to std types
and then unload the cache. accomplishing Gagan's 2nd option.)
-p

Comment 33

19 years ago
So, now the algorithm is 2 pass, one to find all chrome images, mark them, then
another to delete them? Why not just a single entry point, FlushForceAll() ??

This is prolly what we need:

FlushAllForced(): ignores the type, flushes all, called when memory is low.
FlushAllForcedChrome(): flushed just the chrome, called when we switch skins.

Or, combined: FlushAllForced(type), for now, IMAGE_CHROME, IMAGE_NORMAL, or
IMAGE_ALL (IMAGE_CHOME | IMAGE_NORMAL). We can add types later as we need to.

Comment 34

19 years ago
The ability to switch skins is a major feature of the browser, and from a
marketing perspective, I think that this bug should be a P1.  Thoughts?
I agree.  Without this bug being fixed, skin-switching is DOA feature.  It 
should definitely be P1.

Comment 36

19 years ago
agreed.
Whiteboard: [nsbeta3+]

Updated

19 years ago
Priority: P3 → P1
(Assignee)

Comment 37

19 years ago
don't get your shorts in a bunch.
I'm gonna fix it.
-p

Comment 38

19 years ago
adding cc.
(Assignee)

Updated

19 years ago
Target Milestone: --- → M18
(Assignee)

Comment 39

19 years ago
I've got a patch ready. Am waiting for tree to open.
-p

Comment 40

19 years ago
*** Bug 52508 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 41

19 years ago
checked in fix.
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago
Resolution: --- → FIXED

Comment 42

19 years ago
verified fixed after switching themes a couple of times and 
mousingover/mousingdown on various toolbar buttons:

- win98 just pulled
- mac 2000092208
- linux 2000092208

Asa said there were problems in mac: "going from classic to modern and back to 
classic totally screws the UI (it's fine going from classic to modern but 
coming back to classic gives double buttons)".  But that sounds like bugs 53714 
and 53715 reported today, and the original issue here seems fixed (there are no 
longer remnants of images persisting after the theme switching).
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.