499810 - Firefox missing app icon on Windows CE

Status: VERIFIED FIXED

(Firefox :: General, defect, P3)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 1

[Justin Dolske [:Dolske]]

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.

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

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.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Fennec uses the ID-as-integer when jamming the icon into the xulrunner stub

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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?

Comment 5

[Robert Strong (they/them - no direct email)]

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.

Comment 6

[Robert Strong (they/them - no direct email)]

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.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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...

Comment 8

[Robert Strong (they/them - no direct email)]

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.

Comment 9

[Robert Strong (they/them - no direct email)]

Also, I would think we could just use ID 1 in nsWindow.cpp instead of having the same resource added twice.

Comment 10

[Robert Strong (they/them - no direct email)]

Looks like bug 464091 added
#define IDI_APPLICATION MAKEINTRESOURCE(32512)

cc'ing dougt for insight

Comment 11

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

dolske, you want me to take this?

Comment 12

[Robert Strong (they/them - no direct email)]

btw: for this to work on WinCE both patches are necessary

Comment 13

[Doug Turner (:dougt)]

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.

Comment 14

[Justin Dolske [:Dolske]]

(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.)

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 384921 [details] [diff] [review]
Patch v.1

This looks fine, though do we still need it?

Comment 16

[Justin Dolske [:Dolske]]

Yeah, otherwise the .exe doesn't have any icons in it.

Comment 17

[Robert Strong (they/them - no direct email)]

Cloned this bug to bug 500533 to fix the icon resource id's

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Do we still need to check this in?

Comment 19

[Robert Strong (they/them - no direct email)]

It should probably be checked in at the same time as bug 500533

Comment 20

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/2ea1240f2371

Comment 21

[Tony Chung [:tchung]]

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).