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)
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•11 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•11 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•11 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•11 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•11 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•10 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•10 years ago
|
||
Comment 8•10 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•10 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•10 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•10 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•10 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•10 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•10 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•10 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•10 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•10 years ago
|
Attachment #8678477 -
Flags: review?(neil) → review+
![]() |
Assignee | |
Comment 17•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 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
•