Closed Bug 940625 Opened 6 years ago Closed 6 years ago

SVG image cache should invalidate when the SVG depends on the system theme and the theme changes

Categories

(Core :: ImageLib, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: alex_mayorga, Assigned: jwatt)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, Whiteboard: [Australis:P4-])

Attachments

(3 files, 1 obsolete file)

Steps:
Switch theme to "Windows Classic"

Result:
Tabs and app tabs show a shade of blue on the left and right borders.
Ah - I think this is the result of us caching the tab shape for performance reasons...

Note that this requires one to switch themes while Firefox is open - that often results in weird artifacts and things, so I don't think this is 100% out of character. Restarting Firefox should resolve the issue.

I don't think it's really worth trying to flush the cache or anything when the system changes themes. I'm going to say this is a WONTFIX unless there's any serious objection.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
IE11 didn't glitch, so I'd say this should be a valid bug if only for parity sake.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
The issue also exist when turning Aero back on, but the blueish hue is inverted.
Summary: Blueish artifacts on tabs and app tabs appear when using "Windows Classic" theme → Blueish artifacts on tabs and app tabs appear when switching Windows themes
(In reply to alex_mayorga from comment #2)
> IE11 didn't glitch, so I'd say this should be a valid bug if only for parity
> sake.

Even for parity bugs, we can still decide to WONTFIX, and I think Mike's decision here is correct. I don't think we should forego the caching here just to get rid of a bug that pretty much nobody will ever see.

There are far worse bugs with switching OS themes while Firefox is running. It's just not something we've really ever supported very well, and I don't think this bug is worth fixing.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → WONTFIX
Is there no event/message you get from the operating system that allows you to clear the cache?
(In reply to Mike Kaply (:mkaply) from comment #5)
> Is there no event/message you get from the operating system that allows you
> to clear the cache?

There's WM_THEMECHANGED message[1] it seems but seems this will be WONTFIX anyway =(

1 http://msdn.microsoft.com/en-us/library/windows/desktop/ms632650%28v=vs.85%29.aspx
Quoting Elbart from http://forums.mozillazine.org/viewtopic.php?p=13335635#p13335635 as it seems to contain valuable info:

"I'd run into this on a daily basis, as I'm using a tool (Aerofoil) which switches to the Basic-theme when the laptop is running in battery-mode. i.e. http://superuser.com/questions/63574/windows-7-aero-theme-when-on-battery

HPs, Dells and maybe other have similar features.

And yes, seems to be the same bug. Last good 2013-11-18, first bad 2013-11-19.
And they're already using WM_THEMECHANGED: http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp?rev=89f9304ff4ba#4604"
(In reply to alex_mayorga from comment #6)
> (In reply to Mike Kaply (:mkaply) from comment #5)
> > Is there no event/message you get from the operating system that allows you
> > to clear the cache?
> 
> There's WM_THEMECHANGED message[1] it seems but seems this will be WONTFIX
> anyway =(
> 
> 1
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms632650%28v=vs.
> 85%29.aspx

The problem is that that message isn't propagated to frontend/JS. A patch would need to deal with that issue. :-(
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Whiteboard: [Australis:P?]
Duplicate of this bug: 949790
(In reply to :Gijs Kruitbosch from comment #8)
> The problem is that that message isn't propagated to frontend/JS. A patch
> would need to deal with that issue. :-(

Layout code can invalidate the SVG image cache when that message occurs. The front-end shouldn't need to know about it. I thought I filed a bug on this when I fixed bug 921038 as I remember making a testcase. I can provide one but don't have time at the moment. An example is at https://hg.mozilla.org/mozilla-central/diff/e771168640a7/browser/themes/shared/tab-selected.svg#l1.42
Blocks: 921038, 764299
No longer blocks: australis-tabs-win
Status: REOPENED → NEW
Component: Theme → ImageLib
Keywords: testcase-wanted
Product: Firefox → Core
Hardware: x86_64 → All
Summary: Blueish artifacts on tabs and app tabs appear when switching Windows themes → SVG image cache should invalidate when the SVG depends on the system theme and the theme changes
Whiteboard: [Australis:P?] → [Australis:P4]
Whiteboard: [Australis:P4] → [Australis:P4-]
Blocks: 730723
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → jwatt
Attachment #8442930 - Flags: review?(seth)
Attached patch patchSplinter Review
According to Try, on OS X 10.8, nsPresContext::ThemeChangedInternal calls DiscardAllSurfacesForVectorImages before the surface cache has been initialized, so here's a patch tweaked to check for that.
Attachment #8442930 - Attachment is obsolete: true
Attachment #8442930 - Flags: review?(seth)
Attachment #8443084 - Flags: review?(seth)
Comment on attachment 8443084 [details] [diff] [review]
patch

Review of attachment 8443084 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this, Jonathan!

::: image/src/SurfaceCache.h
@@ +174,5 @@
> +   * Evicts all caches surfaces that are for vector images from ths cache.
> +   * This is useful when the app theme changes, since vector images may be
> +   * dependent on the theme (on system colors).
> +   */
> +  static void DiscardAllSurfacesForVectorImages();

I'm not sure we gain much over just calling DiscardAll. Obviously they do exactly the same thing right now, but even in the future when they won't, this is such a rare event that I think it's OK to flush the whole cache. I'd prefer to avoid adding extra machinery for this case until we have evidence that it matters. (That may well happen, but I think there's a decent possibility it won't.)

r+ with DiscardAllSurfacesForVectorImages removed. (If you feel strongly that I'm wrong, though, leave it in. I don't feel so strongly that I'd r- the patch over it.)
Attachment #8443084 - Flags: review?(seth) → review+
Meh, and a follow-up for build bustage due to DiscardAll not being callable before sInstance is set like DiscardAllSurfacesForVectorImages was:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e069ea10ef1
https://hg.mozilla.org/mozilla-central/rev/723f393bbf44
https://hg.mozilla.org/mozilla-central/rev/8e069ea10ef1
Status: NEW → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Duplicate of this bug: 1058488
QA Whiteboard: [good first verify]
Blocks: 1296344
Duplicate of this bug: 1156048
You need to log in before you can comment on or make changes to this bug.