52725 - Prevent winMM.dll from statically linking to mozilla

Status: VERIFIED FIXED

(SeaMonkey :: General, defect, P2)

Description

[rickg]

The winMM.dll is unnecessarily linked to mozilla. There are only 2 uses: nspr 
and nsWindow. I have a patch to fix nsWindow to use WINMM.DLL dynamically, and 
the nspr group has agreed to eliminate their dependency. By fixing this (and a 
related bug involving IMM32) we can eliminate 700K from loading with mozilla. 
It's a start.

Comment 1

[rickg]

Requesting approval to land for beta3. This prevents a system DLL from loading 
until needed, and saves memory. 

Comment 2

[Chris Waterson]

what is the patch?

Comment 3

[rickg]

I shipped the main code in the patch to the porkjockey's newgroup. Here's the 
actual patch. Note that you also have to remove WINMM.DLL from the windows 
makefile.

> ////////////////////////////////////////////////////////////////////////
>
> class CWinMM {
>   typedef int (CALLBACK *PlayPtr)(const char*,HMODULE,DWORD);
>
>
> public:
>
>   CWinMM(const char* aModuleName="WINMM.DLL") {
>     mInstance=::LoadLibrary(aModuleName);
>     mPlay=(mInstance) ? (PlayPtr)GetProcAddress(mInstance,"PlaySound") : 0;
>   }
>
>   ~CWinMM() {
>     if(mInstance)
>       ::FreeLibrary(mInstance);
>     mInstance=0;
>     mPlay=0;
>   }
>
>   BOOL PlaySound(const char *aSoundFile,HMODULE aModule,DWORD aOptions) {
>     return (mPlay) ? mPlay(aSoundFile, aModule, aOptions) : FALSE;
>   }
>
> private:
>   HINSTANCE mInstance;
>   PlayPtr mPlay;
> };
>
> static CWinMM& GetModuleWinMM() {
>   static CWinMM gSharedWinMM;  //construct this only after you *really* need i
t.
>   return gSharedWinMM;
> }
>
> ////////////////////////////////////////////////////////////////////////
>
80a118,119
>
> #if 1
81a121,130
> #else
>       //Rather than link WINMM statically, let's grab it only when we need it.

>       //This will help startup time, and will only affect startup performance
>       //marginally, the first time the method is invoked.
>
>     CWinMM &theMM=GetModuleWinMM();
>     theMM.PlaySound(string, nsnull, (SND_MEMORY | SND_NODEFAULT | SND_SYNC));
>
> #endif

Comment 4

[rickg]

requesting beta3+ status.

Comment 5

[Phil Peterson]

PDT agrees P2

Comment 6

[Wan-Teh Chang]

The NSPR patch can be found in Bug #42714.  It is included
with NSPR 4.1 (released).

Comment 7

[Wan-Teh Chang]

Attachment: Patch for NSPR.

Comment 8

[Brendan Eich [:brendan]]

a=[e-mail] on that NSPR patch.

/be

Comment 9

[rickg]

partial fix landed. The nsSound file dependency has be patched, and we're
waiting the patch from nspr (see attached). Syd will confirm on AIM.

Comment 10

[Wan-Teh Chang]

The NSPR winmm.dll fix has been checked in on
NSPRPUB_CLIENT_BRANCH.  (See bug #42714.)

Comment 11

[Peter Trudelle]

PDT marking nsbeta3-, nominating for rtm

Comment 12

[Wan-Teh Chang]

I just wanted to note that the NSPR winmm.dll fix is
in the Netscape_20000922_BRANCH.

Comment 13

[rickg]

Hang on, PDT. The changes are in and being verified so that I can remove the 
library. Marking RTM.

Comment 14

[rickg]

Marking ++ for review!

Comment 15

[Michael La Guardia]

Rick, please let us know what has been landed where.  Also, you need to have
your patched reviewed and super reviewed.

Comment 16

[rickg]

This code was reviewed by several folks (attinasi, syd) and super-reviewed by 
buster. 

The win (as Michael suggests) is that we would defer the loading of this DLL 
until AIM actually tried to play a sound (I don't think any other code uses this 
library). It saves 500K on startup, best case.

The only remaining change is to strip the winmm.lib statement from the 
makefile.win:

! winmm.lib

Comment 17

[Phil Peterson]

PDT marking [rtm++]

Comment 18

[Jim Roskind]

What is the status of this bug?  It was approved for branch landing 8 days
ago... but there were no further comments?

Comment 19

[Jim Roskind]

Reading this, I can't figure out what was landed, and what was not landed.
Similarly, I can't figure out what needs to be landed.  Pushing back to rtm need
info until we have clarity.
Please re-nominate to rtm-plus when there is a reviewed plan for landing
something (additional?)

Comment 20

[rickg]

Perhaps I wasn't clear enough when I said that all we need to do now was remove
winmm.lib from the makefile. But there you have it.

I referred to this bug when I spoke to the PDT (Phil/Jar) last Thursday. They
were clear that bugs of this type would not be accepted. If they've changed
their mind, I'm happy to land the tiny change.

Comment 21

[Peter Trudelle]

PDT says rtm-, benefit/risk ratio not high enough for this late in the release
cycle.

Comment 22

[Jaime Rodriguez, Jr.]

Removing [PDT] grafitti.

Comment 23

[hacker formerly known as [e-mail]]

Rick's gone. Bouncing to cathleen.

Comment 24

[Vidar Haarr (not reading bugmail)]

What is the status here ?
see comment #19.

Comment 25

[Cathleen]

Sounds like we're trying to delay load winmm.dll till it's needed by aim, for
startup performance improvement.

Syd, do you know anything about this?
I'm confused regarding to what's been done, and what's left to do.

This is what's in the mozilla tree currently:
http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsSound.cpp#39

Comment 26

[timeless]

i think this is fixed the last outstanding reference listed was a makefile.win 
which doesn't appear in my lxr search.

what does appear is some cldap, nss and configure references but i think they 
could be handled in a new bug.

Comment 27

[Matthias Bockelkamp]

LXR shows winmm.dll only in /widget/src/windows/nsSound.cpp, not in any
makefiles. So I think this bug can be closed.

Comment 28

[Matthias Bockelkamp]

v