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

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[Robert Kaiser]

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.

Comment 1

[Philip Chee]

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

Comment 2

[Philip Chee]

(In reply to [e-mail] 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

Comment 3

[[e-mail]]

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.

Comment 4

[Robert Kaiser]

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.

Comment 5

[Philip Chee]

(In reply to Robert Kaiser (:[e-mail]) 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

Comment 6

[Philip Chee]

Now that Bug 1170207 is fixed, themes can override things in chrome skin packages with other things in chrome skin packages.

Comment 7

[Philip Chee]

Attachment: Patch v1.0 Move override for defaultFavicon from content to classic skin

Comment 8

[[e-mail]]

Comment on attachment 8641830 [details] [diff] [review]
Patch v1.0 Move override for defaultFavicon from content to classic skin

This breaks Modern.

Comment 9

[Philip Chee]

Attachment: 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

Comment 10

[Philip Chee]

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

Comment 11

[[e-mail]]

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.

Comment 12

[Philip Chee]

Attachment: Patch v3.0 remove chrome.manifest from 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.

Comment 13

[[e-mail]]

(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?

Comment 14

[Philip Chee]

(In reply to [e-mail] 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

Comment 15

[[e-mail]]

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

Comment 16

[Philip Chee]

Attachment: Patch v4.0 only use AC_DEFINE()

(In reply to [e-mail] 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()

Comment 17

[Philip Chee]

http://hg.mozilla.org/comm-central/rev/2692e295dae7