648738 - [Modern 2.1] mozapps/places/defaultFavicon.png

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[Philip Chee]

Components.classes["@mozilla.org/browser/favicon-service;1"]
          .getService(Components.interfaces.nsIFaviconService)
          .defaultFavicon.spec;

=> chrome://mozapps/skin/places/defaultFavicon.png

Hardcoded in:
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/places/nsIFaviconService.idl#360
360 #define FAVICON_DEFAULT_URL "chrome://mozapps/skin/places/defaultFavicon.png"

In Modern this is M.I.A. (Discovered this while porting Favicon Picker2 to SeaMonkey 2.1.)

Comment 1

[Philip Chee]

Attachment: Patch v1.0 fix.

> diff --git a/suite/themes/modern/mozapps/places/defaultFavicon.png b/suite/themes/modern/mozapps/places/defaultFavicon.png
> new file mode 100644
This is from Past Modern by Kuden.

Comment 2

[Stefan [:stefanh]]

(In reply to comment #1)
> > diff --git a/suite/themes/modern/mozapps/places/defaultFavicon.png b/suite/themes/modern/mozapps/places/defaultFavicon.png
> > new file mode 100644
> This is from Past Modern by Kuden.

Hmm, should it be the disabled one?

Comment 3

[Philip Chee]

Attachment: Patch v1.0a Missed Aero.

I missed the fact that Winstripe does aero.

ui-review? Stefanh because of:
http://hg.mozilla.org/comm-central/annotate/3109ba7f3a36/suite/themes/classic/mac/communicator/search/searchbar.css#l34

Comment 4

[Stefan [:stefanh]]

(In reply to comment #3)
> ui-review? Stefanh because of:
> http://hg.mozilla.org/comm-central/annotate/3109ba7f3a36/suite/themes/classic/mac/communicator/search/searchbar.css#l34

Ah, yes. I don't want the bookmarks icon ;-). During what circumstances is the FAVICON_DEFAULT_URL used?

Comment 5

[Stefan [:stefanh]]

Perhaps the best thing to do is to put the pinstripe defaultFavicon.png in themes/classic/mac/communicator/search.

Comment 6

[Philip Chee]

Used in:
Sync:
http://mxr.mozilla.org/comm-central/source/mozilla/services/sync/modules/util.js#1254
Places autocomplete:
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/places/nsPlacesAutoComplete.js#1114

Comment 7

[Stefan [:stefanh]]

Comment on attachment 524844 [details] [diff] [review]
Patch v1.0a Missed Aero.

See comment #5. You might want to rename it.

Comment 8

[Philip Chee]

> You might want to rename it.
Suggestions?

Comment 9

[Stefan [:stefanh]]

(In reply to comment #8)
> > You might want to rename it.
> Suggestions?

Hmm, it's a default icon (it will appear if there's no icon for the engine), so perhaps 'default-engine-icon' or 'default-search-provider-icon' (hyphens match the other icons)

Comment 10

[[e-mail]]

(In reply to comment #1)
>(From update of attachment 524840 [details] [diff] [review])
>>diff --git a/suite/themes/modern/mozapps/places/defaultFavicon.png b/suite/themes/modern/mozapps/places/defaultFavicon.png
>>new file mode 100644
>This is from Past Modern by Kuden.
Shouldn't you convert chrome://communicator/skin/directory/file-icon.gif ?

Comment 11

[Philip Chee]

> Shouldn't you convert chrome://communicator/skin/directory/file-icon.gif ?
Hrm. I guess so.

I ported Favicon Picker 2 to SeaMonkey:
http://downloads.mozdev.org/xsidebar/mods/favicon_picker_2-0.6.1.14-mod.xpi

Trouble is if you choose the "Blank" (Erase) option. FIP2 uses the defaultFavicon.
e.g. this.faviconService.setAndLoadFaviconForPage(this.bookmark,this.faviconService.defaultFavicon,true);
It would be nice instead to remove favicon data for the bookmark so the default graphic appears.

Comment 12

[Stefan [:stefanh]]

(In reply to comment #9)
> Hmm, it's a default icon (it will appear if there's no icon for the engine), so
> perhaps 'default-engine-icon' or 'default-search-provider-icon' (hyphens match
> the other icons)

Actually, it's basically the mac version of file.gif in communicator/directory, so you might want to call it that. Hmm, I need a fork of directory.css... Optionally, the file could be put in mac/communicator/directory (the I'll do some forking in another bug).

Comment 13

[Stefan [:stefanh]]

Hmm, now I start to wonder if we use communicator/directory.

Comment 14

[[e-mail]]

(In reply to comment #11)
> Trouble is if you choose the "Blank" (Erase) option. FIP2 uses the
> defaultFavicon.
> e.g.
> this.faviconService.setAndLoadFaviconForPage(this.bookmark,this.faviconService.defaultFavicon,true);
> It would be nice instead to remove favicon data for the bookmark so the default
> graphic appears.
Well, the favicon service sucks, because it won't let you remove a favicon.

I take it Firefox's default favicon is also their default search engine icon?

Comment 15

[Stefan [:stefanh]]

(In reply to comment #14) 
> I take it Firefox's default favicon is also their default search engine icon?

AFAIK it's only pinstripe who uses the default favicon in mozapps. Pinstripe doesn't have anything usable in global/icons.

Comment 16

[Philip Chee]

Attachment: Patch v1.2

> Stefan (:stefanh)      2011-04-09 07:10:06 PDT
> 
> Perhaps the best thing to do is to put the pinstripe defaultFavicon.png in
> themes/classic/mac/communicator/search.
> 
> Stefan (:stefanh)      2011-04-09 09:21:30 PDT
> 
> (In reply to comment #8)
> > > You might want to rename it.
> > Suggestions?
> 
> Hmm, it's a default icon (it will appear if there's no icon for the engine), so
> perhaps 'default-engine-icon' or 'default-search-provider-icon' (hyphens match
> the other icons)
> 
> Stefan (:stefanh)      2011-04-09 12:56:18 PDT
> 
> (In reply to comment #9)
> > Hmm, it's a default icon (it will appear if there's no icon for the engine), so
> > perhaps 'default-engine-icon' or 'default-search-provider-icon' (hyphens match
> > the other icons)
> 
> Actually, it's basically the mac version of file.gif in communicator/directory,
> so you might want to call it that. Hmm, I need a fork of directory.css...
> Optionally, the file could be put in mac/communicator/directory (the I'll do
> some forking in another bug).

OK I'll put it in mac/communicator/directory/file.png

> [e-mail]      2011-04-09 10:49:10 PDT
> 
> (In reply to comment #1)
> >(From update of attachment 524840 [details] [diff] [review])
> >>diff --git a/suite/themes/modern/mozapps/places/defaultFavicon.png b/suite/themes/modern/mozapps/places/defaultFavicon.png
> >>new file mode 100644
> >This is from Past Modern by Kuden.
> Shouldn't you convert chrome://communicator/skin/directory/file-icon.gif ?

I've been thinking about this. It's a favicon so it represents a webpage and not a file in a directory listing. It's just that for bookmarks Firefox Winstripe uses a file icon for those bookmarks that don't have a favicon whereas Suite uses a specific bookmark-item icon. If I fix our classic/mac searchbar.css then the only other place where we might potentially use the defaultFavicon is:
http://mxr.mozilla.org/comm-central/source/suite/common/sync/aboutSyncTabs.js#186
Using our bookmark-item icon here might be more consistent with our UI.

Also for extensions like Favicon Picker trying to reset the favicon in a bookmark, the bookmark-item would make more sense than the modern file-icon.

Therefore I propose to use Kuden's PNG conversion of our modern bookmark-item (Which she uses as the Past Modern defaulFavicon).

Comment 17

[Stefan [:stefanh]]

Attachment: Recompressed file.png

I recompressed the file.png icon and managed to shrink it with 8.19%

Comment 18

[Stefan [:stefanh]]

Comment on attachment 525051 [details] [diff] [review]
Patch v1.2

This looks good mac ui-wise, thanks (please use the re-compressed icon). On second thought, I don't think mac/communicator/directory is the right place unless someone can prove to me that we use that directory. This leaves us with either mac/communicator/search or mac/communicator/icons. Since the icon is generic I'd say we could put it in mac/communicator/icons, but I don't object putting it in mac/communicator/search (I'll leave the decision to neil).

Comment 19

[[e-mail]]

Comment on attachment 525051 [details] [diff] [review]
Patch v1.2

>+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).

>-  list-style-image: url("chrome://mozapps/skin/places/defaultFavicon.png");
>+  list-style-image: url("chrome://communicator/skin/directory/file.png");
Should non-Mac use file.gif instead of defaultFavicon.png?

Comment 20

[Stefan [:stefanh]]

Re comment #18, seems that we use directory.css when network.dir.format is set to 3, so I think the layout in attachment #525051 [details] [diff] [review] is OK.

Comment 21

[Stefan [:stefanh]]

To add more confusion, you could use toolkit/themes/pinstripe/global/tree/item.png, since that's the same icon. Actually, that might be better since I can later refer to that in directory.css

Comment 22

[Stefan [:stefanh]]

jftr, filed bug 649136 for the mac directory.css fixes - I will refer to pinstripe icons in the css file.

Comment 23

[Philip Chee]

Attachment: Patch v1.3

> [e-mail]      2011-04-11 07:22:29 PDT
> 
> >+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).
Fixed.

> Stefan (:stefanh)      2011-04-11 13:09:18 PDT
> 
> you could use
> toolkit/themes/pinstripe/global/tree/item.png, since that's the same icon.
> Actually, that might be better since I can later refer to that in directory.css
Fixed.

Comment 24

[Philip Chee]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/a06e5d0d8e1e