Closed Bug 473337 Opened 15 years ago Closed 15 years ago

Refresh the updater application icon

Categories

(Toolkit :: Application Update, defect, P1)

x86
All
defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: faaborg, Assigned: ddahl)

References

()

Details

(Keywords: polish, verified1.9.1, Whiteboard: [polish-easy] [polish-visual] [icon-shiretoko][icon-refresh][polish-p2])

Attachments

(7 files, 13 obsolete files)

39.08 KB, application/x-zip-compressed
Details
7.23 KB, image/png
faaborg
: ui-review+
Details
11.21 KB, patch
ted
: review+
Details | Diff | Splinter Review
90.68 KB, application/octet-stream
Details
54.66 KB, application/octet-stream
Details
39.21 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
39.96 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
This bug is for landing new icons for software update on all platforms.  I'll attach the new files.  See http://blog.mozilla.com/faaborg/2008/12/16/new-os-x-icons-made-by-sofa/ for discussion on the design work.
Whiteboard: [polish-easy] [polish-visual] [icon-3.1]
Attached file New Updater icon for OS X (obsolete) —
This file should replace the file at /toolkit/mozapps/update/src/updater/macbuild/Contents/Resources/updater.icns

I'm likely going to post a new file that only goes up to 128x128 in resolution since this icon only appears in the dock (and that is the maximum size for dock icons).  However, we should go ahead and land this icon for beta 3 and we can reduce the file size in a follow up bug.
Attached file New Updater icon for Windows (obsolete) —
This file should replace the file at /toolkit/mozapps/update/src/updater/updater.ico
Attached file Updater icon for Linux
Similar to crash reporter, I don't think linux currently has an updater icon.  Here is the source artwork as png files.  We can either land the new linux icon as part of this bug, or as a follow up bug.
Attachment #356672 - Flags: ui-review+
Attachment #356673 - Flags: ui-review+
I'll take care of the windows and mac icons
Assignee: nobody → robert.bugzilla
I will take care of the Linux icons
Attachment #356673 - Flags: ui-review+ → ui-review-
Comment on attachment 356673 [details]
New Updater icon for Windows

Talked with Alex about this icon and it is missing several resolutions so r-'ing so it doesn't accidentally get checked in.
Keywords: checkin-needed
Assignee: robert.bugzilla → nobody
Let's try to check in the OS X icon in for beta 3 if possible.
Flags: blocking1.9.1?
Alex - there are ten images in the current image file checked in for mac, and only five in the new one.  Is this a problem?
Alex, I noticed that the new Mac image is significantly smaller than the existing one... the existing image has fallback to lower resolutions the same as the Windows one needs to have. Are you sure you want to go with just the high resolution images on Mac?
Comment on attachment 356674 [details]
Updater icon for Linux

inside of the updater code is this comment:

  // GTK 2.2 seems unable to prevent our dialog from being closed when
  // the user hits the close button on the dialog.  This problem only
  // occurs when either one of the following methods are called:
  //   gtk_window_set_position
  //   gtk_window_set_type_hint
  // For this reason, we disable window decorations.

So no window frame means no icon. Any pointers?
Attachment #356674 - Flags: review?(gavin.sharp)
>Alex - there are ten images in the current image file checked in for mac, and
>only five in the new one.  Is this a problem?

>Alex, I noticed that the new Mac image is significantly smaller than the
>existing one... the existing image has fallback to lower resolutions the same
>as the Windows one needs to have. Are you sure you want to go with just the
>high resolution images on Mac?

I just passed the file off assuming our design firm was making the correct decisions.  I'll need to look into this more carefully to figure out exactly why there was a change (possibly related to which OS versions we are intending to support on OS X).
(In reply to comment #10)

Looks like GTK 2.2 is beneath the system requirements for Fx3.0, 
  http://www.mozilla.com/en-US/firefox/system-requirements.html
so that comment probably doesn't apply anymore - assuming the whole GTK 2.2 vs 2.20 doesn't apply.
Attachment #356674 - Flags: review?(gavin.sharp)
Comment on attachment 356674 [details]
Updater icon for Linux

I'm not really sure what I'm supposed to be reviewing here - icons look fine to me, but I don't know enough about the updater or GTK to be able to offer advice about how they should be added.
Thanks Nick. I'll do some more research on this GTK stuff.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Who owns this?  David, are you actively working on this?  This blocker needs an owner.
Right now the owner is alex to address 6 and comment 11.
Assignee: nobody → faaborg
I'll own it after alex gets the icons for mac and win.
Assignee: faaborg → ddahl
Just wanted to get some review mid-way. The updater UI now has the new icon, an actual frame and titlebar, etc... This is not done yet, RS and I will work on the rest of it today.
David, this is the file that will need to copy the image used by the updater binary.
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsUpdateDriver.cpp
Attached image Linux updater window w/ Icon and Frame (obsolete) —
I am using the 48x48 png for this. The icon shows up in the window frame and alt-tab switcher. I will post another diff of code once I clean it up.
Attachment #359838 - Flags: ui-review?(faaborg)
Whoops - window was not focused. Now it is.
Attachment #359838 - Attachment is obsolete: true
Attachment #359841 - Flags: ui-review?(faaborg)
Attachment #359838 - Flags: ui-review?(faaborg)
Attached patch Testing Patch only! (for linux) (obsolete) — Splinter Review
I am adding this patch to show how I tested everything. Rob Strong helped me figure out how to shirt circuit the updater just to see the UI. I will remove the short circuiting comments before posting a patch for review.
Attached patch possible patch for Linux (obsolete) — Splinter Review
Will confer with RS on monday about this patch. I think there are some issues wiht the Makefile, also need additional linux and other platforms to test. Not asking for review yet.
Attachment #359533 - Attachment is obsolete: true
Attached file Test patch helper script (obsolete) —
This is a helper script to speed up testing withe the above "test patch"
Comment on attachment 359841 [details]
Linux updater window w/ Icon and Frame

What do you mean when you say you are using the 48x48 icon for this?  Is that to deal with cases where the title bar is taller and an icon larger than 16x16 is required?  I think the smaller sized icons are pixel polished, but this doesn't seem to look particularly bad.
I picked the largest size because the alt-tab application icon is bigger, but now that I think about it, it is probably only about 24 x 24.
Attached patch working, but has build issues (obsolete) — Splinter Review
I have gotten the icon appear in a framed gtk window. 

When I build a package and test by gaming the app.update.url.override with a recent build update url, the updater does not display the icon. When the progressgui_gtk ShowProgressUI() tries to free the pixbuf, I get this error: (updater:22452): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed

So the icon is never copied into the $DIST/updates/0/ directory.

I do not understand the build system, so thought I would ping Ted on this. (sdwilsh told me to:))
Attachment #359845 - Attachment is obsolete: true
Attachment #359851 - Attachment is obsolete: true
Attachment #359854 - Attachment is obsolete: true
Attachment #360409 - Flags: review?(ted.mielczarek)
(In reply to comment #27)
> Created an attachment (id=360409) [details]
> working, but has build issues
> 
> I have gotten the icon appear in a framed gtk window. 
> 
> When I build a package and test by gaming the app.update.url.override with a
> recent build update url, the updater does not display the icon. When the
> progressgui_gtk ShowProgressUI() tries to free the pixbuf, I get this error:
> (updater:22452): GLib-GObject-CRITICAL **: g_object_unref: assertion
> `G_IS_OBJECT (object)' failed
> 
> So the icon is never copied into the $DIST/updates/0/ directory.
> 
> I do not understand the build system, so thought I would ping Ted on this.
> (sdwilsh told me to:))
David, the png is copied into updates/0/ in nsUpdateDriver.cpp during runtime so this isn't a build problem
I don't see the png in my updates/0/ dir. is it removed right away?
(In reply to comment #29)
> I don't see the png in my updates/0/ dir. is it removed right away?
It is removed immediately after the update is applied during the restart just as the updater binary, etc. are removed. Also, I suspect the png isn't being copied successfully to updates/0/ by nsUpdateDriver.cpp
Looks like it works now. (I rebuilt the entire tree). Sorry for the noise, I guess I need to figure out everything that needs to be built before reporting a problem.  Attaching a new patch asap.
Attached patch Will review with rob strong (obsolete) — Splinter Review
Attachment #360409 - Attachment is obsolete: true
Attachment #360409 - Flags: review?(ted.mielczarek)
According to this document, 48x48 is the minimal default icon to include with an application. Of course this is more for dealing with display of documents via mime-types in Nautilus or Dolphin. Also, KDE4 tends to have much larger icons when you hit alt-tab. The default size on Gnome when hitting alt-tab is 32x32 px.

I think we should leave it at 48x48 as it scales down well, and will scale up better than smaller icons.
Attachment #360543 - Flags: review?(robert.bugzilla)
Forgot to post the Gnome icon spec: http://library.gnome.org/devel/icon-theme-spec/
Attachment #360543 - Flags: review?(robert.bugzilla) → review-
Comment on attachment 360543 [details] [diff] [review]
Will review with rob strong

>diff --git a/browser/installer/unix/packages-static b/browser/installer/unix/packages-static
>--- a/browser/installer/unix/packages-static
>+++ b/browser/installer/unix/packages-static
>@@ -276,16 +276,17 @@ bin/chrome/icons/default/default32.png
> bin/chrome/icons/default/default48.png
> bin/chrome/reporter.manifest
> bin/chrome/reporter.jar
> bin/@PREF_DIR@/reporter.js
> 
> ; shell icons
> bin/icons/*.xpm
> bin/icons/*.png
>+bin/icons/updater.png
Not necessary since the line above the added line includes all png's in the icons directory.

>diff --git a/toolkit/mozapps/update/src/updater/progressui_gtk.cpp b/toolkit/mozapps/update/src/updater/progressui_gtk.cpp
>--- a/toolkit/mozapps/update/src/updater/progressui_gtk.cpp
>+++ b/toolkit/mozapps/update/src/updater/progressui_gtk.cpp
>@@ -16,16 +16,17 @@
>  * The Original Code is mozilla.org code.
>  *
>  * The Initial Developer of the Original Code is Google Inc.
>  * Portions created by the Initial Developer are Copyright (C) 2005
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>  *  Darin Fisher <darin@meer.net>
>+ *  David Dahl <ddahl@mozilla.com>
>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either the GNU General Public License Version 2 or later (the "GPL"), or
>  * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>  * in which case the provisions of the GPL or the LGPL are applicable instead
>  * of those above. If you wish to allow use of your version of this file only
>  * under the terms of either the GPL or the LGPL, and not to allow others to
>  * use your version of this file under the terms of the MPL, indicate your
>@@ -48,22 +49,23 @@
> static float    sProgressVal;  // between 0 and 100
> static gboolean sQuit = FALSE;
> static gboolean sEnableUI;
> static guint    sTimerID;
> 
> static GtkWidget *sWin;
> static GtkWidget *sLabel;
> static GtkWidget *sProgressBar;
>-
Leave this newline you removed

>+static GdkPixbuf *pixbuf;
Make this local to the function

> static const char *sProgramPath;
> 
> static gboolean
> UpdateDialog(gpointer data)
> {
>+
Remove the newline you added

>   if (sQuit)
>   {
>     gtk_widget_hide(sWin);
>     gtk_main_quit();
>   }
> 
>   float progress = sProgressVal;
> 
>@@ -78,17 +80,16 @@ OnDeleteEvent(GtkWidget *widget, GdkEven
> {
>   return TRUE;
> }
> 
> int
> InitProgressUI(int *pargc, char ***pargv)
> {
>   sProgramPath = (*pargv)[0];
>-
Leave this newline you removed

>   sEnableUI = gtk_init_check(pargc, pargv);
>   return 0;
> }
> 
> int
> ShowProgressUI()
> {
>   if (!sEnableUI)
>@@ -97,42 +98,43 @@ ShowProgressUI()
>   // Only show the Progress UI if the process is taking significant time.
>   // Here we measure significant time as taking more than one second.
> 
>   usleep(500000);
> 
>   if (sQuit || sProgressVal > 50.0f)
>     return 0;
> 
>-  char path[PATH_MAX];
>-  snprintf(path, sizeof(path), "%s.ini", sProgramPath);
>-
>+  char ini_path[PATH_MAX];
>+  snprintf(ini_path, sizeof(ini_path), "%s.ini", sProgramPath);
>+  
Remove the spaces from the empty line

>   StringTable strings;
>-  if (ReadStrings(path, &strings) != OK)
>+  if (ReadStrings(ini_path, &strings) != OK)
>     return -1;
>   
>   sWin = gtk_window_new(GTK_WINDOW_TOPLEVEL);
>   if (!sWin)
>     return -1;
> 
>-  // GTK 2.2 seems unable to prevent our dialog from being closed when
>-  // the user hits the close button on the dialog.  This problem only
>-  // occurs when either one of the following methods are called:
>-  //   gtk_window_set_position
>-  //   gtk_window_set_type_hint
>-  // For this reason, we disable window decorations.
>- 
>+  char icon_path[PATH_MAX];
>+  snprintf(icon_path, sizeof(icon_path), "%s.png", sProgramPath);
>+
>   g_signal_connect(G_OBJECT(sWin), "delete_event",
>                    G_CALLBACK(OnDeleteEvent), NULL);
>-
Leave this newline you removed

>   gtk_window_set_title(GTK_WINDOW(sWin), strings.title);
>   gtk_window_set_type_hint(GTK_WINDOW(sWin), GDK_WINDOW_TYPE_HINT_DIALOG);
>   gtk_window_set_position(GTK_WINDOW(sWin), GTK_WIN_POS_CENTER_ALWAYS);
>   gtk_window_set_resizable(GTK_WINDOW(sWin), FALSE);
>-  gtk_window_set_decorated(GTK_WINDOW(sWin), FALSE);
>+  gtk_window_set_decorated(GTK_WINDOW(sWin), TRUE);
>+
Remove this empty line
cleaned it up
Attachment #360543 - Attachment is obsolete: true
Attachment #360612 - Flags: review?(robert.bugzilla)
Attachment #360612 - Attachment is obsolete: true
Attachment #360613 - Flags: review?(robert.bugzilla)
Attachment #360612 - Flags: review?(robert.bugzilla)
Attachment #360613 - Attachment is obsolete: true
Attachment #360618 - Flags: review?(robert.bugzilla)
Attachment #360613 - Flags: review?(robert.bugzilla)
Comment on attachment 360618 [details] [diff] [review]
formatting clean up, fixed Makefile with SYSINSTALL

Ted:

Just want to make sure the Makefile is ok here - we need i tot copy the png into dist/bin/icons

thanks!
ddahl
Attachment #360618 - Flags: review?(ted.mielczarek)
Attachment #359841 - Flags: ui-review?(faaborg) → ui-review+
Attachment #360618 - Flags: review?(robert.bugzilla) → review+
Comment on attachment 360618 [details] [diff] [review]
formatting clean up, fixed Makefile with SYSINSTALL

Looks fine to me as long as Ted is cool with the Makefile changes
I believe these changes for Linux will also fix bug 302949... adding dependency
Blocks: 302949
Comment on attachment 360618 [details] [diff] [review]
formatting clean up, fixed Makefile with SYSINSTALL

+ifeq ($(MOZ_WIDGET_TOOLKIT),gtk2)
+libs:: updater.png
+	$(SYSINSTALL) $(IFLAGS1) $^ $(DIST)/bin/icons
+endif

I don't think you really need to list updater.png as a dependency here, it's not a generated file. Just hardcode updater.png in the INSTALL line. Also, you should use $(INSTALL), not $(SYSINSTALL). (SYSINSTALL is for installing outside of the objdir, here you want to use a symlink if possible.)
Comment on attachment 360618 [details] [diff] [review]
formatting clean up, fixed Makefile with SYSINSTALL

r+ with those changes.
Attachment #360618 - Flags: review?(ted.mielczarek) → review+
cleaned up Makefile
Attachment #360618 - Attachment is obsolete: true
Attachment #360967 - Flags: review?(ted.mielczarek)
Comment on attachment 360967 [details] [diff] [review]
changed sysinstall to install as per ted's advice

Generally, if someone says "r+, with comments addressed", you don't need to re-request review. Just FYI.
Attachment #360967 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 360967 [details] [diff] [review]
changed sysinstall to install as per ted's advice

This is the patch to check in
Keywords: checkin-needed
before check in - rs recommends that we check Thunderbird, Sunbird and Seamonkey to make sure that bin/icons/*.png is in unix/packages-static

Do all of the other platform packages source this information from here?

http://mxr.mozilla.org/comm-central/source/mozilla/browser/installer/unix/packages-static#282
Thunderbird and Sunbird don't use a package manifest on Linux, only Windows; SeaMonkey's Linux manifest is http://mxr.mozilla.org/comm-central/source/suite/installer/unix/packages
How can Thunderbird and Sunbird take advantage of this patch? Do I need to add anything else to the Makefile, etc? (speaking out of complete ignorance here)
For a product/platform combination that defines a MOZ_PKG_MANIFEST (package-static or packages), the things listed in it are copied to the staging directory; for a product/platform that doesn't, dist/bin/* is tarred up and untarred in the staging directory. Then things listed in NO_PKG_FILES are removed, and what's left is what ships, so all you have to worry about is

1. Get into dist/bin
2. Be listed in any MOZ_PKG_MANIFEST_P (the before-preprocessing define for MOZ_PKG_MANIFEST, where you'll see separate platforms) that's defined for the platform you're interested in.
3. Don't match anything in NO_PKG_FILES (unlikely that you would, but just for completeness: don't ever name anything manglefoo and expect to be able to ship it out of dist/bin/manglefoo)
I had to rebuild my entire tree because of some unrelated tests, and got this error during Makefile creation:

creating toolkit/mozapps/update/src/updater/Makefile
make[8]: Entering directory `/home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-optimize/toolkit/mozapps/update/src/updater'
Makefile:93: *** commands commence before first target.  Stop.

here is the offending bit:

ifeq ($(MOZ_WIDGET_TOOLKIT),gtk2)
	$(INSTALL) $(IFLAGS1) updater.png $(DIST)/bin/icons
endif
Sorry for the trouble ted, I am a make newbie. This patch appears to work.
Attachment #360967 - Attachment is obsolete: true
Attachment #361880 - Flags: review?(ted.mielczarek)
Keywords: checkin-needed
Attached file New windows icon
Here is the windows icon with 8 bit and 4 bit versions
Attachment #356673 - Attachment is obsolete: true
Attached file New OS X icon
New icon for OS X (only goes up to 128x128)
Attachment #356672 - Attachment is obsolete: true
New icons are ready to go (previous two comments), please let me know if anyone notices anything wrong with the new files.
Would it be possible to get a 256x256 one for Windows? Look under the Look and feel section on the following page.
http://msdn.microsoft.com/en-us/library/aa511296.aspx
Whiteboard: [polish-easy] [polish-visual] [icon-3.1] → [polish-easy] [polish-visual] [icon-3.1][waiting ddahl]
Needs testing on a mac. Build the updater, make the package, check for updates and run the updater.
Is it possible to view the icon at 256x256? We have the artwork but I was just trying to keep the file size down.
Per the article...
"Windows Vista-style icons scale smoothly between 256x256 and 32x32 pixels. The 256x256 pixel icon size is required to support high-resolution monitors."

so it would appear it is used for scaling.

Also, other-licenses/branding/firefox/firefox.ico has a 256x256 icon.

Since the original didn't have a 256x256 we can just go with this one if you would prefer.
Comment on attachment 361880 [details] [diff] [review]
I misunderstood ted's directions, build was failing. see comment 51
[Checkin: Comment 66]

Any reason you reverted to listing updater.png as a dependency of the rule, which I asked you to change before?
Attachment #361880 - Flags: review?(ted.mielczarek) → review+
> Any reason you reverted to listing updater.png as a dependency of the rule,
> which I asked you to change before?

make was dying with an error, see comment 51. I am sure Rob Strong knows why, I am somewhat of a make noob. Sorry.
Uh, you left out the target completely in that version, you want:
+ifeq ($(MOZ_WIDGET_TOOLKIT),gtk2)
+libs::
+	$(INSTALL) $(IFLAGS1) updater.png $(DIST)/bin/icons
+endif
When I use your snippet in comment 62, this happens:

c++ -o updater  -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-long-long -pedantic -fno-strict-aliasing -fshort-wchar -pthread -pipe -I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/gtk-unix-print-2.0    -DNDEBUG -DTRIMMED -Os -freorder-blocks -fno-reorder-functions    updater.o bspatch.o archivereader.o progressui_gtk.o readstrings.o    -lpthread   -Wl,-rpath-link,/home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-optimize/dist/bin -Wl,-rpath-link,/usr/local/lib  -L../../../../../dist/bin -L../../../../../dist/lib ../../../../../modules/libmar/src/libmar.a -L../../../../../modules/libbz2/src -lbz2  -lasound -ldl -lm  -lgtk-x11-2.0 -latk-1.0 -lpangoft2-1.0 -lfreetype -lz -lfontconfig -lgdk-x11-2.0 -lgdk_pixbuf-2.0 -lm -lpangocairo-1.0 -lgio-2.0 -lpango-1.0 -lcairo -lgobject-2.0 -lgmodule-2.0 -lglib-2.0        
/home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-optimize/config/nsinstall -R -m 755 updater ../../../../../dist/bin
/home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-optimize/config/nsinstall -R -m 644 updater.png ../../../../../dist/bin/icons
/home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-optimize/config/nsinstall: cannot access updater.png: No such file or directory
make[3]: *** [libs] Error 1
make[3]: Leaving directory `/home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-optimize/toolkit/mozapps/update/src/updater'
make[2]: *** [libs] Error 2
make[2]: Leaving directory `/home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-optimize/toolkit/mozapps/update/src'
make[1]: *** [libs] Error 2
make[1]: Leaving directory `/home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-optimize/toolkit/mozapps/update'
make: *** [default] Error 2


When I revert to:

ifeq ($(MOZ_WIDGET_TOOLKIT),gtk2)
libs:: updater.png
	$(INSTALL) $(IFLAGS1) $^ $(DIST)/bin/icons
endif


it builds fine
Ah right, it's VPATH confusion at that point. Nevermind, not worth the hassle.
It would be nice to get the linux patch attachment 361880 [details] [diff] [review] checked in. Mac and Windows patches coming soon too. (Mac is attached, but not reviewed yet)
Keywords: checkin-needed
Comment on attachment 361880 [details] [diff] [review]
I misunderstood ted's directions, build was failing. see comment 51
[Checkin: Comment 66]


http://hg.mozilla.org/mozilla-central/rev/9911d8835e1b
Attachment #361880 - Attachment description: I misunderstood ted's directions, build was failing. see comment 51 → I misunderstood ted's directions, build was failing. see comment 51 [Checkin: Comment 66]
Whiteboard: [polish-easy] [polish-visual] [icon-3.1][waiting ddahl] → [needs 1.9.1 landing] [polish-easy] [polish-visual] [icon-3.1][waiting ddahl]
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
Needs building/test on windows
Comment on attachment 363925 [details] [diff] [review]
windows ico file (checked in)

Built/tested on Windows
Attachment #363925 - Flags: review+
Comment on attachment 363925 [details] [diff] [review]
windows ico file (checked in)

this patch needs checkin
Comment on attachment 362751 [details] [diff] [review]
Mac Icons Patch (checked in)

needs checkin - tested in finder, but not with a mock update.mar
Whiteboard: [needs 1.9.1 landing] [polish-easy] [polish-visual] [icon-3.1][waiting ddahl] → [needs 1.9.1 landing] [polish-easy] [polish-visual] [icon-3.1]
Attachment #362751 - Attachment description: Mac Icons Patch → Mac Icons Patch (checked in)
Comment on attachment 363925 [details] [diff] [review]
windows ico file (checked in)

Pushed the Windows and Mac icons to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/d9edd8cb95cf
Attachment #363925 - Attachment description: windows ico file → windows ico file (checked in)
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
I can verify that the Mac icon shows in the Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2a1pre) Gecko/20090304 Minefield/3.2a1pre build.
I can verify that the Windows icon shows using the latest nightly running on XP. When I updated the trunk on Windows 7, the icon that showed in the taskbar was the old updater icon, not the new one.
Marcia, can you try again with a trunk build and Windows 7... I highly suspect it may have been running with a Minefield version from prior to this landing.
rstrong: This machine did not have a trunk build on it, so I downloaded yesterday's trunk and updated to today's. I can certainly try those STR again but I did not see the updated icon.

(In reply to comment #74)
> Marcia, can you try again with a trunk build and Windows 7... I highly suspect
> it may have been running with a Minefield version from prior to this landing.
I replayed my STR from Comment 75 on Windows 7 - going from the 20090301 nightly to today's nightly showed the updated icon ( Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090304 Minefield/3.2a1pre). Will check Linux next.
--> P1, as this bug will require the wider feedback of a beta release or is of sufficient complexity that we should be looking at it sooner, not later.
Priority: P2 → P1
So, did this ever get checked in on branch, or did we completely miss it (and then miss comment 77)?

If it hasn't been checked in, someone should state exactly what needs to be checked into branch, because it's not clear.
This still needs to land on 1.9.1
checkin-needed 1.9.1 - applied to trunk already
Keywords: checkin-needed
Attachment #361880 - Flags: approval1.9.1?
Attachment #362751 - Flags: approval1.9.1?
Attachment #363925 - Flags: approval1.9.1?
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/120b0a019d09
Whiteboard: [needs 1.9.1 landing] [polish-easy] [polish-visual] [icon-3.1] → [polish-easy] [polish-visual] [icon-3.1]
Verified fix on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090408 Shiretoko/3.5b4pre
and Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b4pre) Gecko/20090408 Shiretoko/3.5b4pre
and trunk too.
Status: RESOLVED → VERIFIED
Blocks: 487012
Whiteboard: [polish-easy] [polish-visual] [icon-3.1] → [polish-easy] [polish-visual] [icon-3.1][icon-refresh]
This bug's priority relative to the set of other polish bugs is:
P2 - Polish issue that is in a secondary interface, occasionally encountered, and is easily identifiable.
Whiteboard: [polish-easy] [polish-visual] [icon-3.1][icon-refresh] → [polish-easy] [polish-visual] [icon-shiretoko][icon-refresh][polish-p2]
You need to log in before you can comment on or make changes to this bug.