Closed
Bug 1022354
Opened 10 years ago
Closed 8 years ago
SeaMonkey forces 3rd-party themes to not use defaultFavicon.png but hardcode a bookmarks-item.png
Categories
(SeaMonkey :: Themes, defect)
SeaMonkey
Themes
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)
5.24 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
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.
![]() |
Assignee | |
Comment 1•10 years ago
|
||
See Bug 648738 comment 0 And this: http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/places/nsIFaviconService.idl?rev=c9c29cc97af0#136 And this: http://mxr.mozilla.org/comm-central/source/mozilla/addon-sdk/source/lib/sdk/io/data.js?rev=9823cbcda8d8#22 http://mxr.mozilla.org/comm-central/ident?i=FAVICON_DEFAULT_URL http://mxr.mozilla.org/comm-central/search?string=DEF_FAVICON_URI
![]() |
Assignee | |
Comment 2•9 years ago
|
||
(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)
Comment 3•9 years ago
|
||
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)
![]() |
Reporter | |
Comment 4•9 years ago
|
||
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.
![]() |
Assignee | |
Comment 5•9 years ago
|
||
(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
![]() |
Assignee | |
Comment 6•8 years ago
|
||
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 | |
Comment 7•8 years ago
|
||
Comment 8•8 years ago
|
||
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-
![]() |
Assignee | |
Comment 9•8 years ago
|
||
> +++ 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?
![]() |
Assignee | |
Comment 10•8 years ago
|
||
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 11•8 years ago
|
||
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.
![]() |
Assignee | |
Comment 12•8 years ago
|
||
>>+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)
Comment 13•8 years ago
|
||
(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?
![]() |
Assignee | |
Comment 14•8 years ago
|
||
(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)
Comment 15•8 years ago
|
||
(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.
![]() |
Assignee | |
Comment 16•8 years ago
|
||
(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)
Updated•8 years ago
|
Attachment #8678477 -
Flags: review?(neil) → review+
![]() |
Assignee | |
Comment 17•8 years ago
|
||
http://hg.mozilla.org/comm-central/rev/2692e295dae7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-seamonkey2.42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.42
You need to log in
before you can comment on or make changes to this bug.
Description
•