Clean up bookmark-item graphics logo usage in SeaMonkey themes

RESOLVED FIXED in seamonkey2.53

Status

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: frg, Assigned: frg)

Tracking

Trunk
seamonkey2.53
All
Unspecified

SeaMonkey Tracking Flags

(seamonkey2.53 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

+++ This bug was initially created as a clone of Bug #1367542 +++

We now use bookmark-item.png for defaultFavicon.png and defaultFavicon.svg. This is more of a hack. We need a proper svg. There is also a bookmark-item.gif still used in some places which needs to be removed.
Posted patch 1367864-bookmarkItem.patch (obsolete) — Splinter Review
Richard provided some converted svgs. They work fine. I am not setting review yet. I might get real vector ones from another Richard soon. 

Anyone more familiar with svgs. Can we just replace the pngs generally with them?
(In reply to Frank-Rainer Grahl from comment #1)
> Anyone more familiar with svgs. Can we just replace the pngs generally with
> them?

Now yes. Complex icon could use more memory. FX with it's simple monochrome icons improved the startup time against the bitmap icons.
Comment on attachment 8871400 [details] [diff] [review]
1367864-bookmarkItem.patch

Gettting a vector based icon takes more time so use the current patch to remove the hack.
Attachment #8871400 - Flags: review?(rsx11m.pub)
Comment on attachment 8871400 [details] [diff] [review]
1367864-bookmarkItem.patch

> .treecell-panel
> {
>- list-style-image: url("chrome://communicator/skin/bookmarks/bookmark-item.gif");
>+ list-style-image: url("chrome://communicator/skin/bookmarks/bookmark-item.png");
> }

Did you test this /before/ or /after/ changing the manifest from PNG to SVG?

>-% override chrome://mozapps/skin/places/defaultFavicon.svg        chrome://communicator/skin/bookmarks/bookmark-item.png
>+% override chrome://mozapps/skin/places/defaultFavicon.svg        chrome://communicator/skin/bookmarks/bookmark-item.svg

Looks like it should be
>+ list-style-image: url("chrome://communicator/skin/bookmarks/bookmark-item.svg");
or what am I missing?
Comment on attachment 8871400 [details] [diff] [review]
1367864-bookmarkItem.patch

Drat. sorry older version of the patch. Need to extract a new one from my tree.
Attachment #8871400 - Flags: review?(rsx11m.pub)
We have a bookmark-item.gif and a bookmark-item.png. First patch was an older version and only took care of the gif. This one also replaces the png. Tested and, as Richard mentioned, svg now works fine everywhere.
Attachment #8871400 - Attachment is obsolete: true
Attachment #8878773 - Flags: review?(iann_bugzilla)
Comment on attachment 8878773 [details] [diff] [review]
1367864-bookmarkItem-V2.patch

Switching reviewers as just discussed.
Attachment #8878773 - Flags: review?(iann_bugzilla) → review?(rsx11m.pub)
Comment on attachment 8878773 [details] [diff] [review]
1367864-bookmarkItem-V2.patch

LGTM, I didn't find any other occurrences of bookmark-item.png or bookmark-item.gif.

>+++ b/suite/themes/classic/communicator/bookmarks/bookmark-item.svg
>+<svg height="16" width="16" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
>+<image xlink:href="data:image/png;base64, ...
>+OvTPwhz+5giWNtcPheMCpetMnNIAAAAASUVORK5CYII=" width="16" height="16"/></svg>

I was wondering if height="16" and width="16" have to be listed twice here or if either of them may be redundant, but the inner specifications are for the <image> bitmap as such and the outer for the <svg> as a whole, thus probably makes it more efficient to have them explicitly stated rather than implied (= ok as is).
Attachment #8878773 - Flags: review?(rsx11m.pub) → review+
Target SeaMonkey 2.53

https://hg.mozilla.org/comm-central/rev/09b9510570becf3dbd0549d1114dc011e96714e9
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.53
Just came across this landing by accident, any idea what to do about https://dxr.mozilla.org/l10n-central/search?q=bookmark-item.png&redirect=false? AKA, all the copies of this across the localizations.
Hmm good question. compare-locales will likely not look in the help files. 

Its non fatal and means just the icon will not be displayed.

IanN is there a standard procedure for this? Should we just make sure all images in the help files are located in suite\locales\en-US\chrome\common\help\images? There sure will be some further png to svg conversions required.

FRG
Flags: needinfo?(iann_bugzilla)
This has always been a problem with the Help content, not much we can do about it.
Also, not all locales actually translate Help, so it's up to them to watch it or not.
Usually we would check the locales impacted, that are still shipped for SM, and flag it up to them. This could be done by posting in l10n groups and logging bugs.
Flags: needinfo?(iann_bugzilla)
You need to log in before you can comment on or make changes to this bug.