Decomify or get rid of nsIWindowsHooksSettings

RESOLVED DUPLICATE of bug 364168

Status

defect
RESOLVED DUPLICATE of bug 364168
13 years ago
9 years ago

People

(Reporter: alfredkayser, Unassigned)

Tracking

(Blocks 1 bug, {memory-footprint})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

13 years ago
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...
(Reporter)

Updated

13 years ago
Summary: Decomifty or get rid of nsIWindowsHooksSettings → Decomify or get rid of nsIWindowsHooksSettings
(Reporter)

Comment 1

13 years ago
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...
(Reporter)

Comment 2

13 years ago
With some more optimisation:
         61.479 nsWindowsHooks.obj
         77.614 winhooks_s.lib

So, about 28.6K codesize reduction sofar!
(Reporter)

Comment 3

13 years ago
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.
(Reporter)

Comment 4

12 years ago
Complete patch. Saving is about 30K from the .lib file
Attachment #213233 - Attachment is obsolete: true
Attachment #260715 - Flags: review?(jag)

Comment 5

12 years ago
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?).
Attachment #260715 - Flags: review?(jag) → review-

Comment 6

12 years ago
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.

Comment 7

12 years ago
alfred, did you forget about this?

Also this is Seamonkey-only, right?
Assignee: nobody → alfredkayser
(Reporter)

Comment 8

12 years ago
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).

Comment 9

12 years ago
winhooks is built ifdef MOZ_SUITE only:
http://mxr.mozilla.org/seamonkey/source/xpfe/components/Makefile.in#106
Component: XP Miscellany → General
Keywords: footprint
Product: Core → Mozilla Application Suite
(Reporter)

Comment 10

10 years ago
As this is Seamonkey only, and Seamonkey is slowly moving to toolkit, I am removing myself from this bug...
Assignee: alfredkayser → nobody
Blocks: deCOM
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
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of 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.
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.

Comment 21

9 years ago
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".
You need to log in before you can comment on or make changes to this bug.