The nsIWindowsHooksSettings object is generating a lot of code just to set/reset/check a number of Get/SetIsHandlingHTTP/FTP/GIF/etc... registry settings in Windows. This is only used in xpfe/bootstrap/nsNativeAppSupportWin.cpp. nsIWindowsHooks uses internally to check the 'isHandling' registry settings, and show a dialog (if needed) to reset these settings. This can easily be made all part of nsIWindowsHooks, preventing the need of a separate XPCOM object, furthermore, not all every association needs its own Get/Set function. Even more these booleans need not to be exposed as XPCOM values, and can therefor just be internal PRPackedBool's, possibly exposed with one Get/SetSettings with an long int with bits...

First cut at cutting nsIWindowsHooksSettings results in 27 Kilobytes codesize saving! OLD: 83.384 nsWindowsHooks.obj 106.226 winhooks_s.lib NEW: 62.942 nsWindowsHooks.obj 79.076 winhooks_s.lib Need to complete the patch (nsAppRunner.cpp is the most heavy user, but there are some other places to be checked as well), but that will result in more codesize savings...

With some more optimisation: 61.479 nsWindowsHooks.obj 77.614 winhooks_s.lib So, about 28.6K codesize reduction sofar!

Note, also four 4K char arrays are 'deadcode' and can be removed as well, saving 16K of unneeded memory usage, besides the savings of the removal of nsIWindowsHooksSettings itself.

Complete patch. Saving is about 30K from the .lib file

Comment on attachment 260715 [details] [diff] [review] V2: updated to current trunk At first glance it's looking good, just a few minor nits. Please add spaces around your = and ==, e.g.: + int i=0; + entry[i++] = &http; ... + NS_ASSERTION(i==MAX_ENTRIES, "nsWindowsHooks: too many preference entries"); and + for (int i=0; i < NS_ARRAY_LENGTH(kRegistrySettings); i++) { Also, you're using PRUint32 in a few places where you could/should use nsWindowsHookSetting, e.g. |nsWindowsHooks::RegistryMatches(nsWindowsHookSetting settings)| and inside |nsWindowsHooks::CheckSettings(...)| you can use |nsWindowsHookSetting settings;| There's some other nits I'll get to in a bit after I've gone through the rest of this patch. Is part of the patch missing? I don't see changes to make e.g. http://lxr.mozilla.org/seamonkey/source/suite/common/pref/win/platformPrefOverlay.xul#128 and onward go through nsIWindowsHooks. Same for http://lxr.mozilla.org/seamonkey/source/xpfe/components/winhooks/nsSetDefaultBrowser.js, and same for http://lxr.mozilla.org/seamonkey/source/suite/common/pref/pref-winhooks.js (where they use e.g. |prefs[ setting ]| as an alternate way of saying |prefs.isHandlingHTTP| (for setting == "isHandlingHTTP").) Index: nsWindowsHooks.h =================================================================== ... class nsWindowsHooks : public nsIWindowsHooks { ... + ProtocolRegistryEntry *entry[MAX_ENTRIES]; combined with: Index: nsWindowsHooks.cpp =================================================================== @@ -175,80 +135,53 @@ nsWindowsHooks::nsWindowsHooks() + entry[i++] = &xul; won't do the right thing at e.g. +nsWindowsHooks::SetSettings( nsWindowsHookSetting settings ) { ... + if (kRegistrySettings[i].bit & settings) { + (void) entry[i]->set(); + } else { + (void) entry[i]->reset(); + } Since set and reset aren't virtual you'll end up calling ProtocolRegistryEntry's versions instead of FileTypeRegistryEntry's. Not your doing, but while I'm looking at this, I'm not sure I like FileTypeRegistryEntry extending ProtocolRegistryEntry. It's not really an "is-a" relationship. Anyway, I guess make those structs become classes, add virtual where needed, and clean up the inheritance tree (please?).

Ah, one more nit: + if ((kRegistrySettings[i].bit & settings) + && (entry[i]) && (!entry[i]->isAlreadySet())) { + return PR_FALSE; + } Move that leading && to the end of the previous line in keeping with over-all Mozilla style. When you attach your new patch, see if you can find someone who can test this on Windows (unfortunately I only have access to Mac and Linux right now) and give you an r+. I could be the sr.

alfred, did you forget about this? Also this is Seamonkey-only, right?

1. No, I didn't forget this one. It is just low on my list of prio's. 2. It is not just seamonkey. This is XPCOM stuff used in all applications (but generally only to check for the default hooks).

winhooks is built ifdef MOZ_SUITE only: http://mxr.mozilla.org/seamonkey/source/xpfe/components/Makefile.in#106

As this is Seamonkey only, and Seamonkey is slowly moving to toolkit, I am removing myself from this bug...

MXR gives me the impression that there are no users left in the seamonkey tree anymore either. KaiRo, can you confirm?

Winhook was removed by bug 364168

So the fact that the files still exist in seamonkey is irrelevant?

(In reply to comment #13) > So the fact that the files still exist in seamonkey is irrelevant? It is replaced with nsIShellService by bug 441050 and bug 364167. So winhooks just removed.

oops s/bug 364167/bug 364168/

Maybe I'm not being clear here. I understand you to be saying that yes, all *uses* have been removed, but at http://mxr.mozilla.org/seamonkey/source/xpfe/components/winhooks/ you can see that the *implementation* still exists in that Hg tree. So, if anyone is still using that tree, this bug should be reopened to delete that code. Otherwise, no worries.

Zack, you seems to reference CVS tree. If you want to check mercurial repository of comm-entral, you should use http://mxr.mozilla.org/comm-1.9.1/ or http://mxr.mozilla.org/comm-central/

(In reply to comment #17) > Zack, you seems to reference CVS tree. So you confirm that nobody is using the "seamonkey" tree anymore?

(In reply to comment #18) > (In reply to comment #17) > > Zack, you seems to reference CVS tree. > > So you confirm that nobody is using the "seamonkey" tree anymore? Although Camino still use CVS, it isn't windows version. Others such as Thunderbird, SeaMonkey and Sunbird moved to comm-central.

What Makoto is saying is that the "seamonkey" tree in mxr is pointing at the CVS repository, and modern SeaMonkey/Thunderbird development has moved to the comm-central hg repository.

Yes, that the name "seamonkey" is being used for the "Mozilla applications" CVS repository index has only hysteric reasons and no direct connection to the product bearing the brand "SeaMonkey".