79175 - merge jsloader module into xpconnect

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P2)

Description

[John Bandhauer]

DLL consolidation is in style these days.

The files in mozilla/js/src/xpconnect/loader could get built into the same DLL 
as the core xpconnect. This would need to happen on all platforms. I'd vote for 
just moving the source files into xpconnect/src and integrating there to 
simplify the build hassles.

Comment 1

[David Bradley]

Why not leave the files where they are and just have them build from the 
makefiles in xpconnect? This would minimize CVS impact, and it would be easier 
to back out later if needed.

I'm still learning the build system, so I suspect I'm missing something about a 
convention or something. Set me straight if that's the case.

Comment 2

[David Bradley]

I'm thinking this probably breaks convention and might cause confusion down the 
road. I don't have enough exposure to other areas to know if this is true or 
not.

Comment 3

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

That's certainly a viable option, and while it's a _little_ confusing, it's not
without precedent, and who but us three will be working in there? =)

But we could also just get leaf to copy the CVS files over to src/, and make it
all a little nicer.  Let's do that.

Leaf, can you help us out?

Comment 4

[John Bandhauer]

dbradley: There are 3 build systems involved. It may sound easy to do what you 
say. It is not. It also leaves some ugliness behind that is likely to be tripped 
over now and then. Moving the files in cvs, merging the module registration code 
(and perhaps the #includes), and updating the build system is the way to go. It 
is actually simpler to do now and much cleaner into the future.

Comment 5

[David Bradley]

Then it's a greed, the sources will move to the xpconnect/src and the necessary 
mods to the build will be made. Leaf, will I just coordinate with you after the 
changes have been reviewed and tested?

Comment 6

[David Bradley]

I'm going to attach two diffs. One srcdiff.txt will be for a minor change to 
remove the NS_IMPL_THREADSAFE_ISUPPORTS1 macro from mozJSSubScriptLoader.cpp 
file, as the existing XPConnect source has it defined. Unfortunately I created 
this patch as a diff from the mozilla/js/src/xpconnect/loader directory. I 
couldn't find a way to do this otherwise in CVS.

The second is makediff.txt which contains the changes to the makefile's and the 
win32.order file. (I had merged it already before jband sent the message and 
figured it didn't matter to leave the changes in).

Because the srcdiff.txt was created from the original directory it will probably 
be easiest to apply the diff, then move the files and remove the directory. Feel 
free to educate me in a better way to do this.

Patrick, if you handle the Mac project files, that would be great.

So as it stands these are the steps needed to consolidate the loader into 
XPConnect:
1. Retrieve both patches
2. Apply the makediff.txt patch
3. Apply the srcdiff.txt patch
4. move the mozJS*.h & mozJS*.cpp files from the mozilla/js/src/xpconnect/loader  
to the mozilla/js/src/xpconnect/src directory
5. remove the mozilla/js/src/xpconnect/loader directory
6. Build and run

Unfortunately I don't have access to a Linux system so I haven't been able to 
test the makefile.in changes. If someone could volunteer to verify those changes 
I'd be greatly appreciate your help.

Comment 7

[David Bradley]

One correction and one addition before the patches arrive. The macro was removed 
from mozJSComponentLoader.cpp. I fixed a warning in mozJSSubScriptLoader.cpp 
where a pointer parameter was commented and causing the compiler to think it was 
an end of comment sequence.

Comment 8

[David Bradley]

Attachment: Patch for the make files

Comment 9

[David Bradley]

Attachment: Patch for the source files

Comment 10

[Robert Ginda]

Thanks for fixing the comment in the subscriptloader, I had forgotten all about
it.  I'll test on linux.

Comment 11

[John Bandhauer]

dbradley: a few issues...

- The Makefile.in additions need to have leading tabs not spaces. This is 
critical as it will break the build.

- I don't see the lines that will cause the two components being moved to be 
registered. Removing one NS_IMPL_NSGETMODULE is not enough. You need move...

NS_GENERIC_FACTORY_CONSTRUCTOR(mozJSComponentLoader);

#ifndef NO_SUBSCRIPT_LOADER
NS_GENERIC_FACTORY_CONSTRUCTOR(mozJSSubScriptLoader);
#endif

... to xpcmodule.cpp. And move ...

    { "JS component loader", MOZJSCOMPONENTLOADER_CID,
      mozJSComponentLoaderContractID, mozJSComponentLoaderConstructor,
      RegisterJSLoader, UnregisterJSLoader },
#ifndef NO_SUBSCRIPT_LOADER
   { "JS subscript loader", MOZ_JSSUBSCRIPTLOADER_CID,
      mozJSSubScriptLoadContractID, mozJSSubScriptLoaderConstructor }
#endif

...into the components array in xpcmodule.cpp

This implies that xpcmodule.cpp will need to include (at least):

#include "mozJSComponentLoader.h"
#ifndef NO_SUBSCRIPT_LOADER
#include "mozJSSubScriptLoader.h"
#endif

- What is the talk of NS_IMPL_THREADSAFE_ISUPPORTS1 ?
- You can delete your local copy of win32.order and update and then it won't 
pollute the diffs.

Comment 12

[David Bradley]

Interesting, I'm surprised it ran. If the JSComponent loader isn't registered, 
what should break? Maybe I didn't excersize the code enough, I was having fun 
with getting past ChangeTable, but I did get mail up and a few other things. I 
expected things to go wrong fairly quickly if I had missed something.

NS_IMPL_THREADSAFE_ISUPPORTS1 was a typo, I meant NS_IMPL_NSGETMODULE. Trust the 
diffs, sorry.

I'll make the necessary changes, test, and put up another patch.

Comment 13

[David Bradley]

Ok, here's revised instructions for applying the new patches that contain jbands 
catches and a few other additional things I came across also. These two patches 
replace the previous ones, don't run both sets.

Something to comment on, is I removed "static" from the RegisterJSLoader and 
UnregisterJSLoader in the mozJSComponentLoader.cpp and put the prototypes in 

I ran Chatzilla, if there is a better test pass it on to me.

So as it stands these are the steps needed to consolidate the loader into 
XPConnect:
1. Retrieve both patches
2. Apply the srcdiff.txt patch
3. Apply the src2diff.txt patch
4. move the mozJS*.h & mozJS*.cpp files from the mozilla/js/src/xpconnect/loader  
to the mozilla/js/src/xpconnect/src directory
5. remove the mozilla/js/src/xpconnect/loader directory
6. Build and run

Comment 14

[David Bradley]

Attachment: Patches files in the js/src/xpconnect/loader directory

Comment 15

[David Bradley]

Attachment: Patches makefiles and sources for xpconnect

Comment 16

[John Bandhauer]

The declaraions you moved to mozJSComponentLoader.h should be declared 'extern'. 
no?

Do you have a plan for landing this stuff? You picked an odd first bug to try to 
land. Build changes and cvs file moves require a lot more help than simple code 
changes.

You'll need to get leaf to move the underlying files in cvs (I think the scheme 
is to copy the underlying files and then later cvs remove the old ones, leaving 
the attic copies of the old file in place. Right?).

You'll need a Mac buddy to make the Mac project files changes and test them. And 
a Unix buddy to test that you got the Unix part right.

Comment 17

[David Bradley]

Attachment: Added extern keyword, replaces the previous srcdiff.txt

Comment 18

[David Bradley]

Going to push this off for a week or so till beard returns and can address the 
Mac side, so Rob, no hurry to test.

Comment 19

[John Bandhauer]

I remembered one other issue you should look into when landing this...

I think you'll want to update the installer packaging. Adding and removing 
endproduct files has an impact on what files get packaged up by the installer. 
We used to have a *lot* of problems with modules or .xpt files being added to 
the build and not added to the packaging scripts. This would leave us with 
developer builds that worked fine, but mysterious problems in installed builds 
due to missing parts. Here you are, of course, removing an end product file. It 
may be that the packaging system just doesn't care about that. I'm pretty sure 
it just emits warnings and continues if expected files are not present. But it 
*could* cause a packaging-time build problem. Most developers don't run the 
install packager as part of their build process so you would no see that in 
your own normal builds.

see the XPInstall section at the end of 
http://mozilla.org/build/making-additions.html 

Also note that there are packaging manifests for 3 platforms multiplied by two 
for the mozilla system and commercial system (which has its own package files).

Comment 20

[David Bradley]

I'm not sure we want to mess with this at this point. If someone feels otherwise
let me know and I'll move it back up.

Comment 21

[David Bradley]

After further discussion it was decided 1.0.1 makes more sense as a post 1.0
milestone.

Comment 22

[David Bradley]

Retargetting to the proper post 1.0 milestone

Comment 23

[Alec Flett]

Attachment: a fresh patch

Here's a fresh, comprehensive patch that covers everything except the mac
project file change. basically I'm making the loader into a static library that
gets linked in against xpconnect. I'm declaring the constructors in a header
file that gets included by xpcmodule.cpp so that the jsloader is still pretty
self-contained.

can I get reviews? dbradley? this saves us about 12k on disk and about 30k of
runtime overhead because there is only 1 dll instead of 2.

Comment 24

[Alec Flett]

Attachment: mac project changes

and here are the mac project changes

Comment 25

[Alec Flett]

oops, over to me.

Comment 26

[Brendan Eich [:brendan]]

Comment on attachment 99886 [details] [diff] [review]
a fresh patch

Looks ok to me, I'll rs= optimistically -- anything dbradley says is binding,
of course.

/be

Comment 27

[David Bradley]

Is /js/macbuild/JSLoader.xml and issue. The Mac patch doesn't have any mods for
this file and I don't know the Mac build well enough to know if this needs
changing, removed, or what. I also see references in
/xpfe/bootstrap/macbuild/StaticMerge.xml to JSLoader.o and JSLoaderDebug.o. Not
sure what .o's are on a Mac.

Other than that this looks good. I compiled with it on my system both debug and
release and no problems. If someone can insure the Mac issues I mentioned aren't
a problem I'll put my r= in asap.

Comment 28

[Alec Flett]

dbradley: JSLoader.xml just needs to be removed, which I was going to do after
my checkin (but not clutter up the patch with a 500+ line file removal)

Comment 29

[David Bradley]

Comment on attachment 99886 [details] [diff] [review]
a fresh patch

r=dbradley

Ok, thanks for getting this done.

Comment 30

[Alec Flett]

cool, fixed.

Comment 31

[Phil Schwartau]

Marking Verified -