Closed Bug 499810 Opened 12 years ago Closed 12 years ago

Firefox missing app icon on Windows CE

Categories

(Firefox :: General, defect, P3)

All
Windows CE
defect

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: vlad, Assigned: Dolske)

References

Details

(Whiteboard: [nv])

Attachments

(1 file, 1 obsolete file)

Firefox doesn't have an application icon on Windows CE; the Fennec folks know how to do this, need to find out from mfinkle or dougt what things we need to add to our build.
The Fennec icon was handled in bug 474813, but I don't think that work applies here as it's xulrunner specific.

Looks like the problem here is that the icon bits don't get inserted into the exe. This is normally happens as a result of browser/app/Makefile.in setting RCINCLUDE = splash.rc, but that's wrapped in "ifeq ($(OS_ARCH),WINNT)" (we're WINCE), so it's not being run.

As a side effect, splash.rc is also responsible for bundling in some other cursors (zoom in/out, grab hand). These cursors don't show up in contexts where they should (viewing a large scaled image, customizing the toolbar), which is presumably a result of this too.

Sadly I think there are some other dependencies wrapped in other ifdefs, so I'm sorting that out now.
Attached patch Patch v.1Splinter Review
This changes the ifdef guards to let CE builds copy the icons and create the executable manifest with icons.

There is, however, a rather weird bug. CE shows the wrong icon!

We put two icons in firefox.exe -- the normal app icon, and a document icon (a sheet of paper with the logo on it)... config/version_win.pl takes browser/app/splash.rc, inserts it into the generated objdir/browser/app/module.rc, and at some point this gets compiled into module.res and inserted in the executable [I'm not sure what does this last step, I guess VC++ does it automagically?]

CE uses the document icon in the explorer, whereas XP shows the app icon. I think this is a CE bug -- if you copy firefox.exe from an official Firefox release build onto the device, you'll see the same problem. The actual icon data should be fine -- if I swap the contents of firefox.ico and document.ico, the resulting build shows looks "wrong" on XP and "right" on CE.

But wait, there's more!

If you open up the .exe in VisualStudio and examine its resources, the app icon has an ID of "0" (a string) and the document icon has an ID of 1 (an integer). If I manually change zero-the-string to zero-the-integer, explorer will show the correct icon on both XP and CE. This seems like the root cause, apparently explorer uses the icon with the lowest ID, and so I bet CE is missing whatever hack allows using a string here.

Another fun detail: the app icon is actually listed in the module.rc twice. Once with an ID of IDI_APPICON and once with IDI_APPLICATION. Something then converts IDI_APPICON to "0" (string). On an release build IDI_APPLICATION becomes 32512 (integer), but on my CE build it becomes "IDI_APPLICATION" (string). Weird.
Fennec uses the ID-as-integer when jamming the icon into the xulrunner stub
So most of the IDI_* defines are all controlled by us; IDI_APPLICATION isn't one of them, but it's also one that's not present on WINCE.  So maybe in nsNativeAppSupportWin.h, we should add something like:

#ifndef IDI_APPLICATION
#define IDI_APPLICATION 32512
#endif

this is what we seem to do in nsWindow.cpp, where we use IDI_APPLICATION in code (and where it would cause a compilation error; the resource compiler just converts it to a string, I guess).

However, IDI_APPICON is 0 in that .h, not sure why it's ending up as a string -- maybe see if something is overriding the #define?
So, I believe that the valid range for resource IDs is 1 to 0x6FFF though it doesn't appear to be strictly enforced on Windows likely due to trying to just work by looking for an icon resource.
Also, going with IDI_APPICON 1 and IDI_DOCUMENT 2 won't screw up the icons used by the OS since the number to specify the icon is for the order and not the ID.
Yeah, 32512 (0x7f00) is a system-provided resource that exists on win32, but not on WINCE.  We could #define it to something else on WinCE; if we do, we should remove the #define in nsWindow.cpp (this is probably a better thing to do, and just put it all in nsNativeAppSupportWin.h).

If we do this and then bump up IDI_APPICON etc. up by 1, we should be ok.. we could even keep the original 0 around if needed for whatever reason...
I believe keeping the original "0" around would end up requiring a change to the registry entry that points to the document icon which is currently the 2nd icon.
Also, I would think we could just use ID 1 in nsWindow.cpp instead of having the same resource added twice.
Looks like bug 464091 added
#define IDI_APPLICATION MAKEINTRESOURCE(32512)

cc'ing dougt for insight
Attached patch patch rev1 (obsolete) — Splinter Review
dolske, you want me to take this?
btw: for this to work on WinCE both patches are necessary
you probably can #ifdef IDI_APPLICATION for windows mobile.  iirc, it is used the LoadIcon call when creating windows.  Windows Mobile doesn't have IDI_APPLICATION defined, so we defined it in this file.

fwiw, the htc task manager seem to just find the first ICO file embedded in the exe -- and ignore the icon set per window.
(In reply to comment #11)
> Created an attachment (id=385017) [details]
> patch rev1
> 
> dolske, you want me to take this?

Sure, let's spin that off to a separate bug though.

My updated CE build just finished, with your patch I'm now getting the correct icon. (I also turned on --enable-official-branding to make sure that part of my patch is working too.)
Attachment #384921 - Flags: review?(vladimir)
Comment on attachment 384921 [details] [diff] [review]
Patch v.1

This looks fine, though do we still need it?
Attachment #384921 - Flags: review?(vladimir) → review+
Yeah, otherwise the .exe doesn't have any icons in it.
Cloned this bug to bug 500533 to fix the icon resource id's
Do we still need to check this in?
It should probably be checked in at the same time as bug 500533
Pushed http://hg.mozilla.org/mozilla-central/rev/2ea1240f2371
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
Attachment #385017 - Attachment is obsolete: true
Verified fix on branded build: Mozilla/5.0 (Windows; U; WindowsCE 6.0; en-US; rv:1.9.2a2pre) Gecko/20090811 Firefox/3.6a2pre.  New FF icon there, in both the .exe and the WinCE toolbar (or whatever thats called).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.