Closed Bug 572043 Opened 14 years ago Closed 14 years ago

Add-on Manager Icons for Beta 1 Release

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla2.0b1
Tracking Status
blocking2.0 --- beta1+

People

(Reporter: jboriss, Assigned: mossop)

References

Details

(Whiteboard: [AddonsRewrite])

Attachments

(9 files, 2 obsolete files)

The attached file shows each of the icons that should be used for the add-on manager categories in the upcoming beta 1 release.  Most of the icons come from what's already in toolkit.  

For Vista:

http://mxr.mozilla.org/mozilla1.9.2/source/toolkit/themes/winstripe/mozapps/extensions/viewButtons-aero.png

For OSX and Windows XP:

http://mxr.mozilla.org/mozilla1.9.2/source/toolkit/themes/winstripe/mozapps/extensions/viewButtons.png

The remaining icons have been attached
Attached image OSX: Recent Updates (obsolete) —
Attached image OSX: Available Updates
Attached image Vista: Recent Updates
Attached image OSX: Recent Updates
Attachment #451192 - Attachment is obsolete: true
blocking2.0: --- → beta1+
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [AddonsRewrite]
Version: unspecified → Trunk
(In reply to comment #5)
> Created an attachment (id=451195) [details]
> Vista: Recent Updates

Jennifer, what is the reason to show the arrows for recent updates in the opposite direction?
(In reply to comment #8)
> (In reply to comment #5)
> > Created an attachment (id=451195) [details] [details]
> > Vista: Recent Updates
> 
> Jennifer, what is the reason to show the arrows for recent updates in the
> opposite direction?

The idea is that the down arrow implies a download (see downloads manager, get add-ons, etc).  eg, here's an item you already have.  It's also to associate recent updates with available updates visually.  But overall, it's the quickest way to get an icon out based on the ones we have
Attached patch patch rev 1 (obsolete) — Splinter Review
This imports the new icons into the tree and does a small amount of moving around. Let me know if you want the binary bits in a different form or anything.
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Attachment #452302 - Flags: review?(robert.bugzilla)
Comment on attachment 452302 [details] [diff] [review]
patch rev 1

Doesn't look like the right patch to me.

btw: is there any reason not to add test pilot under chrome (components and modules if necessary) instead? It is a hell of a lot easier to clean up with app update that way.
Attachment #452302 - Flags: review?(robert.bugzilla) → review-
Also, can this review wait until Tuesday?
Attached patch patch rev 1Splinter Review
This should be the right patch. We really want this for beta 1 I think, but I don't know when the freeze is planned for right now so I don't know.
Attachment #452302 - Attachment is obsolete: true
Attachment #452307 - Flags: review?(robert.bugzilla)
No problem, I'll get this reviewed today
(In reply to comment #12)
> (From update of attachment 452302 [details] [diff] [review])
> Doesn't look like the right patch to me.
> 
> btw: is there any reason not to add test pilot under chrome (components and
> modules if necessary) instead? It is a hell of a lot easier to clean up with
> app update that way.

Tracking this in bug 573079, but generally we want users to be able to enable/disable this as an extension. If there are reasons that is going to be difficult to clean up after then let us know!
Comment on attachment 452307 [details] [diff] [review]
patch rev 1

>diff --git a/toolkit/themes/gnomestripe/mozapps/extensions/extensions.css b/toolkit/themes/gnomestripe/mozapps/extensions/extensions.css
>--- a/toolkit/themes/gnomestripe/mozapps/extensions/extensions.css
>+++ b/toolkit/themes/gnomestripe/mozapps/extensions/extensions.css
>@@ -313,13 +313,17 @@
>   margin: 7px 5px;
>   width: 32px;
>   height: 32px;
>-  list-style-image: url("chrome://mozapps/skin/xpinstall/xpinstallItemGeneric.png");
>+  list-style-image: url("chrome://mozapps/skin/extensions/extensionGeneric.png");
> }
> 
> .addon[type="theme"] .icon {
>   list-style-image: url("chrome://mozapps/skin/extensions/themeGeneric.png");
> }
> 
>+.addon[type="locale"] .icon {
>+  list-style-image: url("chrome://mozapps/skin/extensions/localeGeneric.png");
>+}
>+
> .addon[type="plugin"] .icon {
>   list-style-image: url("chrome://mozapps/skin/plugins/pluginGeneric.png");
> }
>@@ -451,13 +455,17 @@
>   width: 32px;
>   height: 32px;
>   -moz-margin-end: 10px;
>-  list-style-image: url("chrome://mozapps/skin/xpinstall/xpinstallItemGeneric.png");
>+  list-style-image: url("chrome://mozapps/skin/extensions/extensionGeneric.png");
> }
> 
> #detail-view[type="theme"] #detail-icon {
>   list-style-image: url("chrome://mozapps/skin/extensions/themeGeneric.png");
> }
> 
>+#detail-view[type="locale"] #detail-icon {
>+  list-style-image: url("chrome://mozapps/skin/extensions/localeGeneric.png");
>+}
>+
> #detail-view[type="plugin"] #detail-icon {
>   list-style-image: url("chrome://mozapps/skin/plugins/pluginGeneric.png");
> }
Curious why you don't combine the rules?

>diff --git a/toolkit/themes/gnomestripe/mozapps/jar.mn b/toolkit/themes/gnomestripe/mozapps/jar.mn
>--- a/toolkit/themes/gnomestripe/mozapps/jar.mn
>+++ b/toolkit/themes/gnomestripe/mozapps/jar.mn
>@@ -3,7 +3,6 @@ toolkit.jar:
> + skin/classic/mozapps/downloads/downloadIcon.png          (downloads/downloadIcon.png)
> + skin/classic/mozapps/downloads/downloads.css             (downloads/downloads.css)
> + skin/classic/mozapps/extensions/extensions.css           (extensions/extensions.css)
>-+ skin/classic/mozapps/extensions/themeGeneric.png         (extensions/themeGeneric.png)
> + skin/classic/mozapps/extensions/category-search.png      (extensions/category-search.png)
> + skin/classic/mozapps/extensions/category-discover.png    (extensions/category-discover.png)
> + skin/classic/mozapps/extensions/category-languages.png   (extensions/category-languages.png)
>@@ -11,6 +10,13 @@ toolkit.jar:
> + skin/classic/mozapps/extensions/category-extensions.png  (extensions/category-extensions.png)
> + skin/classic/mozapps/extensions/category-themes.png      (extensions/category-themes.png)
> + skin/classic/mozapps/extensions/category-plugins.png     (extensions/category-plugins.png)
>++ skin/classic/mozapps/extensions/category-recent.png      (extensions/category-recent.png)
>++ skin/classic/mozapps/extensions/category-available.png   (extensions/category-available.png)
>++ skin/classic/mozapps/extensions/extensionGeneric.png     (extensions/extensionGeneric.png)
>++ skin/classic/mozapps/extensions/extensionGeneric-16.png  (extensions/extensionGeneric-16.png)
>++ skin/classic/mozapps/extensions/themeGeneric.png         (extensions/themeGeneric.png)
>++ skin/classic/mozapps/extensions/themeGeneric-16.png      (extensions/themeGeneric-16.png)
>++ skin/classic/mozapps/extensions/localeGeneric.png        (extensions/localeGeneric.png)
nit: would be nice to sort these so they are easier to scan / find

SeaMonkey also uses xpinstallItemGeneric.png
http://mxr.mozilla.org/comm-central/source/suite/themes/modern/mozapps/xpinstall/xpinstallConfirm.css#110
http://mxr.mozilla.org/comm-central/source/suite/themes/modern/jar.mn#518
http://mxr.mozilla.org/comm-central/source/suite/common/bindings/notification.xml#210
(In reply to comment #17)
> (From update of attachment 452307 [details] [diff] [review])
> >diff --git a/toolkit/themes/gnomestripe/mozapps/extensions/extensions.css b/toolkit/themes/gnomestripe/mozapps/extensions/extensions.css
> >--- a/toolkit/themes/gnomestripe/mozapps/extensions/extensions.css
> >+++ b/toolkit/themes/gnomestripe/mozapps/extensions/extensions.css
> >@@ -313,13 +313,17 @@
> >   margin: 7px 5px;
> >   width: 32px;
> >   height: 32px;
> >-  list-style-image: url("chrome://mozapps/skin/xpinstall/xpinstallItemGeneric.png");
> >+  list-style-image: url("chrome://mozapps/skin/extensions/extensionGeneric.png");
> > }
> > 
> > .addon[type="theme"] .icon {
> >   list-style-image: url("chrome://mozapps/skin/extensions/themeGeneric.png");
> > }
> > 
> >+.addon[type="locale"] .icon {
> >+  list-style-image: url("chrome://mozapps/skin/extensions/localeGeneric.png");
> >+}
> >+
> > .addon[type="plugin"] .icon {
> >   list-style-image: url("chrome://mozapps/skin/plugins/pluginGeneric.png");
> > }
> >@@ -451,13 +455,17 @@
> >   width: 32px;
> >   height: 32px;
> >   -moz-margin-end: 10px;
> >-  list-style-image: url("chrome://mozapps/skin/xpinstall/xpinstallItemGeneric.png");
> >+  list-style-image: url("chrome://mozapps/skin/extensions/extensionGeneric.png");
> > }
> > 
> > #detail-view[type="theme"] #detail-icon {
> >   list-style-image: url("chrome://mozapps/skin/extensions/themeGeneric.png");
> > }
> > 
> >+#detail-view[type="locale"] #detail-icon {
> >+  list-style-image: url("chrome://mozapps/skin/extensions/localeGeneric.png");
> >+}
> >+
> > #detail-view[type="plugin"] #detail-icon {
> >   list-style-image: url("chrome://mozapps/skin/plugins/pluginGeneric.png");
> > }
> Curious why you don't combine the rules?

Mostly to keep them separated so all the rules for the list were in one place and for the detail in another. I could combine them if you think it is worth breaking that though.

> >diff --git a/toolkit/themes/gnomestripe/mozapps/jar.mn b/toolkit/themes/gnomestripe/mozapps/jar.mn
> >--- a/toolkit/themes/gnomestripe/mozapps/jar.mn
> >+++ b/toolkit/themes/gnomestripe/mozapps/jar.mn
> >@@ -3,7 +3,6 @@ toolkit.jar:
> > + skin/classic/mozapps/downloads/downloadIcon.png          (downloads/downloadIcon.png)
> > + skin/classic/mozapps/downloads/downloads.css             (downloads/downloads.css)
> > + skin/classic/mozapps/extensions/extensions.css           (extensions/extensions.css)
> >-+ skin/classic/mozapps/extensions/themeGeneric.png         (extensions/themeGeneric.png)
> > + skin/classic/mozapps/extensions/category-search.png      (extensions/category-search.png)
> > + skin/classic/mozapps/extensions/category-discover.png    (extensions/category-discover.png)
> > + skin/classic/mozapps/extensions/category-languages.png   (extensions/category-languages.png)
> >@@ -11,6 +10,13 @@ toolkit.jar:
> > + skin/classic/mozapps/extensions/category-extensions.png  (extensions/category-extensions.png)
> > + skin/classic/mozapps/extensions/category-themes.png      (extensions/category-themes.png)
> > + skin/classic/mozapps/extensions/category-plugins.png     (extensions/category-plugins.png)
> >++ skin/classic/mozapps/extensions/category-recent.png      (extensions/category-recent.png)
> >++ skin/classic/mozapps/extensions/category-available.png   (extensions/category-available.png)
> >++ skin/classic/mozapps/extensions/extensionGeneric.png     (extensions/extensionGeneric.png)
> >++ skin/classic/mozapps/extensions/extensionGeneric-16.png  (extensions/extensionGeneric-16.png)
> >++ skin/classic/mozapps/extensions/themeGeneric.png         (extensions/themeGeneric.png)
> >++ skin/classic/mozapps/extensions/themeGeneric-16.png      (extensions/themeGeneric-16.png)
> >++ skin/classic/mozapps/extensions/localeGeneric.png        (extensions/localeGeneric.png)
> nit: would be nice to sort these so they are easier to scan / find
> 
> SeaMonkey also uses xpinstallItemGeneric.png
> http://mxr.mozilla.org/comm-central/source/suite/themes/modern/mozapps/xpinstall/xpinstallConfirm.css#110
> http://mxr.mozilla.org/comm-central/source/suite/themes/modern/jar.mn#518
> http://mxr.mozilla.org/comm-central/source/suite/common/bindings/notification.xml#210

Maybe it's worth double-packaging that icon for now so we don't break other app.
(In reply to comment #18)
>...
> Mostly to keep them separated so all the rules for the list were in one place
> and for the detail in another. I could combine them if you think it is worth
> breaking that though.
No worries and definitely not in this bug. I tend to go with a common section for things like this and have sections that are specific after.

>...
> Maybe it's worth double-packaging that icon for now so we don't break other
> app.
For now I think it would be a good thing since it doesn't hurt anything and prevents breaking SeaMonkey... just remove it in a followup on getting this sorted for SeaMonkey.
Comment on attachment 452307 [details] [diff] [review]
patch rev 1

Looks good. I didn't verify the stripping of the png's. r=me
Attachment #452307 - Flags: review?(robert.bugzilla) → review+
cc'ing callek and kairo. See comment #19
(In reply to comment #18)
> > SeaMonkey also uses xpinstallItemGeneric.png
> > http://mxr.mozilla.org/comm-central/source/suite/themes/modern/mozapps/xpinstall/xpinstallConfirm.css#110
> > http://mxr.mozilla.org/comm-central/source/suite/themes/modern/jar.mn#518
> > http://mxr.mozilla.org/comm-central/source/suite/common/bindings/notification.xml#210
> 
> Maybe it's worth double-packaging that icon for now so we don't break other
> app.

Don't care about modern, and I actually hate how notification.xml directly points to an URL without giving themes to point to a possibly differently-named file. But enough ranting about that - what icon does Firefox use for install notifications in the new code?
SeaMonkey should probably just switch over to the same one.
(In reply to comment #22)
> (In reply to comment #18)
> > > SeaMonkey also uses xpinstallItemGeneric.png
> > > http://mxr.mozilla.org/comm-central/source/suite/themes/modern/mozapps/xpinstall/xpinstallConfirm.css#110
> > > http://mxr.mozilla.org/comm-central/source/suite/themes/modern/jar.mn#518
> > > http://mxr.mozilla.org/comm-central/source/suite/common/bindings/notification.xml#210
> > 
> > Maybe it's worth double-packaging that icon for now so we don't break other
> > app.
> 
> Don't care about modern, and I actually hate how notification.xml directly
> points to an URL without giving themes to point to a possibly differently-named
> file. But enough ranting about that - what icon does Firefox use for install
> notifications in the new code?
> SeaMonkey should probably just switch over to the same one.

We will be using chrome://mozapps/skin/extensions/extensionGeneric.png and chrome://mozapps/skin/extensions/extensionGeneric-16.png for default extension icons. We are probably also switching to the new doorhanger notifications if they ever get landed which I think allow you to use css for icons.
I landed this as http://hg.mozilla.org/mozilla-central/rev/e3ccdd93ba10 including double-packaging the xpinstallItemGeneric.png to avoid breaking anyone using that. We can just remove that from the package manifest at a later time if we feel confident.

Don't know if there is any point in having a litmus test for new icons Henrik?
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite-
Flags: in-litmus?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a6
Spotted that I accidentally packaged the aero icon for the non-aero xpinstallItemGeneric.png. I'll correct it tomorrow.
(In reply to comment #23)
> We will be using chrome://mozapps/skin/extensions/extensionGeneric.png and
> chrome://mozapps/skin/extensions/extensionGeneric-16.png for default extension
> icons. We are probably also switching to the new doorhanger notifications if
> they ever get landed which I think allow you to use css for icons.

Sounds good. So we should just go and use extensionGeneric*.png for SeaMonkey as well, and move this to the doorhanger when switching over to it (which we're also planning for 2.1). Callek?

Mossop, thanks for the info.
Corrects the typo I made when fixing the comments from the previous patch.
Comment on attachment 453407 [details] [diff] [review]
Typo correction (checked in)

http://hg.mozilla.org/mozilla-central/rev/83489c0c3e00
Attachment #453407 - Attachment description: Typo correction → Typo correction (checked in)
(In reply to comment #24)
> Don't know if there is any point in having a litmus test for new icons Henrik?

We don't have tests for icons. We will simply verify the landing of all icons. Regarding Linux I have a question... We have different icons for Themes and Plugins. Those are not listed in any of the attached mock-ups. Because they looks brilliant and shiny I would say they have also landed as expected.
Flags: in-litmus? → in-litmus-
As talked with Dave on IRC the icons which have been checked-in are the ones he got from Boriss. For Linux those are mostly the ones we already use for Firefox 3.6. Marking as verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.