[Modern 2.1] mozapps/places/defaultFavicon.png

RESOLVED FIXED in seamonkey2.1final

Status

RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: philip.chee, Assigned: philip.chee)

Tracking

({modern})

Trunk
seamonkey2.1final
x86
Windows 7
modern

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

8 years ago
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.)
(Assignee)

Comment 1

8 years ago
Created attachment 524840 [details] [diff] [review]
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.
Attachment #524840 - Flags: review?(neil)

Comment 2

8 years ago
(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?
(Assignee)

Comment 3

8 years ago
Created attachment 524844 [details] [diff] [review]
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
Attachment #524840 - Attachment is obsolete: true
Attachment #524840 - Flags: review?(neil)
Attachment #524844 - Flags: ui-review?(stefanh)
Attachment #524844 - Flags: review?(neil)

Comment 4

8 years ago
(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

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

Comment 7

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

See comment #5. You might want to rename it.
Attachment #524844 - Flags: ui-review?(stefanh) → ui-review-
(Assignee)

Comment 8

8 years ago
> You might want to rename it.
Suggestions?

Comment 9

8 years ago
(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)
(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 ?
(Assignee)

Comment 11

8 years ago
> 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

8 years ago
(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

8 years ago
Hmm, now I start to wonder if we use communicator/directory.
(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

8 years ago
(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.
(Assignee)

Comment 16

8 years ago
Created attachment 525051 [details] [diff] [review]
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

> neil@parkwaycc.co.uk      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).
Attachment #524844 - Attachment is obsolete: true
Attachment #524844 - Flags: review?(neil)
Attachment #525051 - Flags: ui-review?(stefanh)
Attachment #525051 - Flags: review?(neil)

Comment 17

8 years ago
Created attachment 525066 [details]
Recompressed file.png

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

Comment 18

8 years ago
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).
Attachment #525051 - Flags: ui-review?(stefanh) → ui-review+
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?
Attachment #525051 - Flags: review?(neil) → review-

Comment 20

8 years ago
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

8 years ago
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

8 years ago
jftr, filed bug 649136 for the mac directory.css fixes - I will refer to pinstripe icons in the css file.
(Assignee)

Comment 23

8 years ago
Created attachment 525408 [details] [diff] [review]
Patch v1.3

> neil@parkwaycc.co.uk      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.
Attachment #525051 - Attachment is obsolete: true
Attachment #525408 - Flags: ui-review?(stefanh)
Attachment #525408 - Flags: review?(neil)

Updated

8 years ago
Attachment #525408 - Flags: ui-review?(stefanh) → ui-review+

Updated

8 years ago
Attachment #525408 - Flags: review?(neil) → review+
(Assignee)

Comment 24

8 years ago
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/a06e5d0d8e1e
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1final

Updated

4 years ago
Depends on: 1022354
You need to log in before you can comment on or make changes to this bug.