Closed Bug 51249 Opened 24 years ago Closed 24 years ago

Images from theme persist after switching, last through session

Categories

(Core Graveyard :: Skinability, defect, P1)

Product:
Core Graveyard
Component:
Skinability
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: jmd, Assigned: pnunn)

References

Details

(Keywords: regression, Whiteboard: [nsbeta3+])

Attachments

(1 file)

Screenshot after switching from modern to classic
55.53 KB, image/jpeg
Details 
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
Keywords: regression
ok, this seems to only occur after switching themes once or twice, you cant just
start up in a theme and reproduce...
I'm having trouble reproducing this with a linux cvs build 2000090418.  Will
keep trying.
Setting OS to All: I just saw this on Windows NT4 SP6 with build 2000090504.
OS: Linux → All
Hardware: PC → All
*** Bug 51370 has been marked as a duplicate of this bug. ***
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
*** Bug 51561 has been marked as a duplicate of this bug. ***
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
*** Bug 51617 has been marked as a duplicate of this bug. ***
Need info: Please clarify exact steps to reproduce (including specific themes
and platforms). Also, is there any workaround?
Whiteboard: [need info]
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.

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 ...
Pretty sure NS marketing wouldn't like that, since they added menu items to make
switching even easier.
QA Contact: pmac → blakeross
Summary: someone's playing mix-and-match themes → Images from theme persist after switching, last through session
*** Bug 51646 has been marked as a duplicate of this bug. ***
*** 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!
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]
Thanks, we get the picture, and have more than enough info for triage this aft.
->pnunn, per hyatt, who says you know what to do. cc hyatt
Assignee: hyatt → pnunn
*** Bug 51823 has been marked as a duplicate of this bug. ***
*** Bug 51823 has been marked as a duplicate of this bug. ***
9 dups -> mostfreq
Keywords: mostfreq
I'm on it.
Status: NEW → ASSIGNED
*** Bug 51987 has been marked as a duplicate of this bug. ***
Adding CC
This is happening because IL_FlushCache refuses to remove chrome images from the 
image cache. I think that it should flush chrome images.

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",

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.
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

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.

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.
agreed.
Whiteboard: [nsbeta3+]
Priority: P3 → P1
don't get your shorts in a bunch.
I'm gonna fix it.
-p
adding cc.
Target Milestone: --- → M18
I've got a patch ready. Am waiting for tree to open.
-p
*** Bug 52508 has been marked as a duplicate of this bug. ***
checked in fix.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
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.

Attachment

General

Creator:
Created:
Updated:
Size: