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)

Product:
SeaMonkey
Component:
Themes
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(seamonkey2.42 fixed)

Status:
RESOLVED FIXED
Milestone:
seamonkey2.42
Tracking Flags:
Tracking Status
seamonkey2.42 --- fixed

People

(Reporter: kairo, Assigned: philip.chee)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Patch v4.0 only use AC_DEFINE()
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.
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+

    
    

    
  
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.