719423 - Firefox EXE contains two of the same icon

Status: RESOLVED WONTFIX

(Firefox :: General, defect)

Description

[Mike Kaply [:mkaply]]

I could be totally wrong here, and if I am, please just close this.

I was looking at the Firefox EXE and noticed that it has two of the same Firefox icon, at position 1 and at position 32512.

These icons are approximately 300K in size.

Looking at the code in nsWindow, the second icon is used here:

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#787

static LPWSTR const gStockApplicationIcon = MAKEINTRESOURCEW(32512);

Is there any reason this line can't be changed to:

static LPWSTR const gStockApplicationIcon = MAKEINTRESOURCEW(1);

and have the second icon removed completely? This would cut the size of the EXE by 1/3.

Comment 1

[Mike Hommey [:glandium]]

The second icon was added in bug 236977

Comment 2

[Mike Kaply [:mkaply]]

Just gotta love a bug with no information on what was actually changed :)

So looking in the file, there are three icon references:

IDI_APPICON ICON FIREFOX_ICO
IDI_DOCUMENT ICON DOCUMENT_ICO
IDI_APPLICATION ICON FIREFOX_ICO

Looking at the docs, IDI_APPLICATION is documented and is important:

http://msdn.microsoft.com/en-us/library/windows/desktop/ms648072(v=vs.85).aspx

But IDI_APPICON does not appear to be.

I wonder what would happen if the order was changed to this?

IDI_APPLICATION ICON FIREFOX_ICO
IDI_DOCUMENT ICON DOCUMENT_ICO

Comment 3

[Mike Hommey [:glandium]]

It's not the order that counts, it's the value.

Comment 4

[Mike Kaply [:mkaply]]

I thought the first icon in the executable was used as the default icon (it didn't depend on the value).

Comment 5

[Mike Kaply [:mkaply]]

I find it odd that if IDI_APPICON is so important, yet there is not a single bit of documentation that mentions it on any Microsoft website. Maybe it is obsolete? Even bing corrects it from IDI_APPICON to IDI_APPLICATION

Comment 6

[Mike Hommey [:glandium]]

(In reply to Michael Kaply (mkaply) from comment #4)
> I thought the first icon in the executable was used as the default icon (it
> didn't depend on the value).

AFAIK, it uses the icon with the lowest id, which the IDI_* values are.

(In reply to Michael Kaply (mkaply) from comment #5)
> I find it odd that if IDI_APPICON is so important, yet there is not a single
> bit of documentation that mentions it on any Microsoft website. Maybe it is
> obsolete? Even bing corrects it from IDI_APPICON to IDI_APPLICATION

That's because it's defined in our headers: splash.rc includes nsNativeAppSupportWin.h and nsNativeAppSupportWin.h says:
#define IDI_APPICON 1

Comment 7

[Mike Hommey [:glandium]]

On the other hand, except for OS2, we don't even have a document.ico anymore.

Comment 8

[Mike Hommey [:glandium]]

oh we do, i wasn't looking at the right place.

Comment 9

[Mike Kaply [:mkaply]]

> oh we do, i wasn't looking at the right place

Where is that? I couldn't find it.


Couldn't we still fix this by making our defines for Document icon greater than 32512?

Or is there some requirement that the document icon be icon number 2?

Comment 10

[Mike Kaply [:mkaply]]

> Where is that? I couldn't find it.

What I meant was, I see the icon in the tree, I just don't see it referenced anywhere.

Comment 11

[Mike Hommey [:glandium]]

At the very least here:
http://mxr.mozilla.org/mozilla-central/source/browser/components/shell/src/nsWindowsShellService.cpp#194

Not easy to find other references :(

Changing the id for IDI_DOCUMENT would require proper migration.

Comment 12

[Mike Hommey [:glandium]]

Actually, this code suggests we're back to having bug 372236

Comment 13

[Mike Hommey [:glandium]]

And that would be a regression from bug 500533.

Comment 14

[Mike Kaply [:mkaply]]

I just checked on Windows 7 and we are still using the document icon.

Interesting side note - chrome doesn't use the document icon, it uses just the app icon (for associated HTML files).

So all this being said, it appears we are stuck where we are short of writing code that messes with the registry to change the values.

Although I do wonder what happens if you remove IDI_APPLICATION completely.

Comment 15

[Mike Kaply [:mkaply]]

I think we are where we are and there's not much that's going to change that.

Comment 16

[Masatoshi Kimura [:emk]]

I think we can just remove the IDI_APPLICATION icon from our resource (and change gStockApplicationIcon). IDI_APPLICATION is not what applications define. It is used only when applications do not want to define their own icon.