merge jsloader module into xpconnect

VERIFIED FIXED in mozilla1.2beta

Status

()

Core
XPConnect
P2
normal
VERIFIED FIXED
17 years ago
15 years ago

People

(Reporter: John Bandhauer, Assigned: Alec Flett)

Tracking

Trunk
mozilla1.2beta
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fix in hand)

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

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

Updated

17 years ago
Status: NEW → ASSIGNED
Priority: -- → P2

Comment 1

17 years ago
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

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

Comment 4

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
Created attachment 34869 [details] [diff] [review]
Patch for the make files

Comment 9

17 years ago
Created attachment 34870 [details] [diff] [review]
Patch for the source files

Comment 10

17 years ago
Thanks for fixing the comment in the subscriptloader, I had forgotten all about
it.  I'll test on linux.
(Reporter)

Comment 11

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
Created attachment 35041 [details] [diff] [review]
Patches files in the js/src/xpconnect/loader directory

Comment 15

17 years ago
Created attachment 35042 [details] [diff] [review]
Patches makefiles and sources for xpconnect
(Reporter)

Comment 16

17 years ago
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

17 years ago
Created attachment 35128 [details] [diff] [review]
Added extern keyword, replaces the previous srcdiff.txt

Comment 18

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

Comment 19

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

Updated

17 years ago
Target Milestone: --- → mozilla1.0

Comment 20

16 years ago
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.
Target Milestone: mozilla1.0 → mozilla1.1

Comment 21

16 years ago
After further discussion it was decided 1.0.1 makes more sense as a post 1.0
milestone.
Target Milestone: mozilla1.1 → mozilla1.0.1

Comment 22

16 years ago
Retargetting to the proper post 1.0 milestone
Target Milestone: mozilla1.0.1 → mozilla1.2
(Assignee)

Updated

15 years ago
Blocks: 163737
(Assignee)

Comment 23

15 years ago
Created attachment 99886 [details] [diff] [review]
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.
Attachment #34869 - Attachment is obsolete: true
Attachment #34870 - Attachment is obsolete: true
Attachment #35041 - Attachment is obsolete: true
Attachment #35042 - Attachment is obsolete: true
Attachment #35128 - Attachment is obsolete: true
(Assignee)

Comment 24

15 years ago
Created attachment 99887 [details] [diff] [review]
mac project changes

and here are the mac project changes
(Assignee)

Updated

15 years ago
Whiteboard: fix in hand
Target Milestone: mozilla1.2alpha → mozilla1.2beta
(Assignee)

Comment 25

15 years ago
oops, over to me.
Assignee: dbradley → alecf
Status: ASSIGNED → NEW
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
Attachment #99886 - Flags: superreview+

Comment 27

15 years ago
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.
(Assignee)

Comment 28

15 years ago
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

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

r=dbradley

Ok, thanks for getting this done.
Attachment #99886 - Flags: review+
(Assignee)

Comment 30

15 years ago
cool, fixed.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 31

15 years ago
Marking Verified -
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.