Closed Bug 1022354 Opened 11 years ago Closed 10 years ago

SeaMonkey forces 3rd-party themes to not use defaultFavicon.png but hardcode a bookmarks-item.png

Categories

(SeaMonkey :: Themes, defect)

defect
Not set
normal

Tracking

(seamonkey2.42 fixed)

RESOLVED FIXED
seamonkey2.42
Tracking Status
seamonkey2.42 --- fixed

People

(Reporter: kairo, Assigned: philip.chee)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Due to the patch in bug 648738, SeaMonkey has 3rd-party themes required to duplicate their default favicon into a bookmarks-item.png - if that doesn't exist (like in my EarlyBlue and LCARStrek themes where there's only a GIF by that name but no PNG), the following error message is spit out: Error: NS_ERROR_FILE_NOT_FOUND: Failed to open input source 'chrome://mozapps/skin/places/defaultFavicon.png' Source File: resource://gre/modules/WindowsPreviewPerTab.jsm Line: 75 The faulty line is http://mxr.mozilla.org/comm-central/source/suite/common/jar.mn#5 of course. We should not do a general redirect across all themes just because Modern doesn't have a file that is required somewhere.
Blocks: 648738
(In reply to neil@parkwaycc.co.uk from Bug 648738 comment #19) >>+toolkit.jar: >>+## overwrite mozapps/places/defaultFavicon.png with /communicator/bookmarks/bookmark-item.png >>++ skin/classic/mozapps/places/defaultFavicon.png (communicator/bookmarks/bookmark-item.png) >>++ skin/classic/aero/mozapps/places/defaultFavicon.png (communicator/bookmarks/bookmark-item.png) > Sorry, this is a no-go. But what you could do is override (not overwrite) > defaultFavicon.png in suite/common/jar.mn and add bookmark-item.png to > Modern (using the file that you had attached as defaultFavicon.png). I could move the override from common/jar.mn to classic/jar.mn but will Neil agree? I can't remember why he wanted it in suite/common/jar.mn
Flags: needinfo?(neil)
The reason the override is there is because there's no other way for SeaMonkey to replace defaultFavicon.png in Classic. (In reply to Philip Chee from comment #2) > I can't remember why he wanted it in suite/common/jar.mn You can't put overrides in themes, it doesn't work. (In reply to Robert Kaiser) > We should not do a general redirect across all themes just because Modern > doesn't have a file that is required somewhere. Modern actually suffers from the same problem as other third-party themes, it's forced to duplicate its default favicon bookmarks-item.gif into bookmarks-item.png so that it can be found consistently.
Flags: needinfo?(neil)
Grr, so the only fix is to duplicate an icon that I need for the theme's Firefox support again just so I can support SeaMonkey as well? That sucks.
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #4) > Grr, so the only fix is to duplicate an icon that I need for the theme's > Firefox support again just so I can support SeaMonkey as well? That sucks. Or you could fix the underlying problem: > http://mxr.mozilla.org/comm-central/ident?i=FAVICON_DEFAULT_URL
Now that Bug 1170207 is fixed, themes can override things in chrome skin packages with other things in chrome skin packages.
Depends on: 1170207
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attachment #8641830 - Flags: review?(neil)
Comment on attachment 8641830 [details] [diff] [review] Patch v1.0 Move override for defaultFavicon from content to classic skin This breaks Modern.
Attachment #8641830 - Flags: review?(neil) → review-
Depends on: 1190465
> +++ b/suite/configure.in > +AC_DEFINE(MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES) > +AC_SUBST(MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES) > +++ b/suite/confvars.sh > +MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES=1 These bits are for Bug 1190465 https://bugzilla.mozilla.org/show_bug.cgi?id=1190465 Bug 1190465 - Move default theme overrides into separate chrome.manifest for other non-firefox toolkit consumers too
Attachment #8641830 - Attachment is obsolete: true
Attachment #8643539 - Flags: review?
Comment on attachment 8643539 [details] [diff] [review] Patch v1.0 Fix Modern + support for toolkit theme overrides > +++ b/suite/configure.in > +AC_DEFINE(MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES) > +AC_SUBST(MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES) > +++ b/suite/confvars.sh > +MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES=1 These bits are for Bug 1190465 https://bugzilla.mozilla.org/show_bug.cgi?id=1190465 Bug 1190465 - Move default theme overrides into separate chrome.manifest for other non-firefox toolkit consumers too
Attachment #8643539 - Flags: review? → review?(neil)
Comment on attachment 8643539 [details] [diff] [review] Patch v1.0 Fix Modern + support for toolkit theme overrides >+AC_DEFINE(MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES) Don't need this; you're already defining it in the appropriate moz.build files when you subst it in the config. >- 'chrome.manifest', You didn't remove the file from the tree.
>>+AC_DEFINE(MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES) > Don't need this; you're already defining it in the appropriate moz.build files when you subst it in the config. I spent several fustrating hours searching developer.mozilla.org but I couldn't find any proper documentation on how these work. In the end I found that I needed AC_DEFINE but not AC_SUBST for Bug 1190465. On the other hand I needed AC_SUBST(MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES) for Bug 1189918 > +AC_DEFINE(MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES) > +AC_DEFINE(MOZ_DEFAULT_THEME_IS_JAR) > .... > +MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES=1 > +MOZ_DEFAULT_THEME_IS_JAR=1 I added MOZ_DEFAULT_THEME_IS_JAR because Thunderbird doesn't jar their default theme. Needed for Bug 1189918 >>- 'chrome.manifest', > You didn't remove the file from the tree. Removed.
Attachment #8643539 - Attachment is obsolete: true
Attachment #8643539 - Flags: review?(neil)
Attachment #8655459 - Flags: review?(neil)
(In reply to Philip Chee from comment #12) > I added MOZ_DEFAULT_THEME_IS_JAR because Thunderbird doesn't jar their > default theme. Needed for Bug 1189918 What are we doing differently?
(In reply to neil@parkwaycc.co.uk from comment #13) > (In reply to Philip Chee from comment #12) >> I added MOZ_DEFAULT_THEME_IS_JAR because Thunderbird doesn't jar their >> default theme. Needed for Bug 1189918 > What are we doing differently? I think it was IanN who decided to keep the extensions in the application/extensions/ packed. To IanN. Can we revert the default theme to unpacked. This will help https://bugzilla.mozilla.org/show_bug.cgi?id=1189918#c10
Flags: needinfo?(iann_bugzilla)
(In reply to Philip Chee from comment #12) >>>+AC_DEFINE(MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES) >> Don't need this; you're already defining it in the appropriate moz.build files when you subst it in the config. >I spent several fustrating hours searching developer.mozilla.org but I >couldn't find any proper documentation on how these work. AC_SUBST is used in moz.build and Makefile. AC_DEFINE is used in jar.mn and C++. In this case, I thought bug 1190465 was using AC_SUBST but I just looked again and it seems unnecessary, so in fact it is AC_DEFINE that you neeed there. > On the other hand I needed > AC_SUBST(MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES) for Bug 1189918 Only because you use its value in a moz.build to set a DEFINE. If instead you perform the calculation in the C++ preprocessor [#if defined(MOZ_BUILD_APP_IS_BROWSER) || defined(MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES)] then you wouldn't need the AC_SUBST.
Blocks: 1133743
(In reply to neil@parkwaycc.co.uk from comment #15) > AC_SUBST is used in moz.build and Makefile. AC_DEFINE is used in jar.mn and > C++. In this case, I thought bug 1190465 was using AC_SUBST but I just > looked again and it seems unnecessary, so in fact it is AC_DEFINE that you > neeed there. >> On the other hand I needed >> AC_SUBST(MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES) for Bug 1189918 > Only because you use its value in a moz.build to set a DEFINE. If instead > you perform the calculation in the C++ preprocessor [#if > defined(MOZ_BUILD_APP_IS_BROWSER) || > defined(MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES)] then you wouldn't need > the AC_SUBST. OK I'm down to only one AC_DEFINE()
Attachment #8655459 - Attachment is obsolete: true
Attachment #8655459 - Flags: review?(neil)
Flags: needinfo?(iann_bugzilla)
Attachment #8678477 - Flags: review?(neil)
Blocks: 1223341
Blocks: 1222818
Attachment #8678477 - Flags: review?(neil) → review+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.42
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: