I would like to make this Fast-Track Review ...

Target milestone: 4.2. It would be good to know the deadlines though ...

tcr: I found no tests. Especially for functionality that will not be used in 
NetBeans IDE you need tests that we can executed with every commit and that 
will verify that your changes are still working. 
 
tcr: Wait for separation of module system to be integrated. Where the 
FixedModule, StandardModule will go then? Into org.netbeans or 
org.netbeans.core.startup? Each one somehwhere else? 
 
tca: ModuleFactory should be found in lookup. I do not know if that is 
possible in 4.1, but it will be in 4.2. 
 
tca: Following line should be replaced by 
ModuleFactory.getParentClassLoaders() or something like that: 
+ if (System.getProperty("org.netbeans.modules.moduleFactory") != null)  
 
tcr: This is not nice either: 
+ if (m instanceof StandardModule) 
imho, this signals that some important method is missing in Module class. 
 

Thanks for the first review. I will wait for the separation of the module system
and then provide a more up to date diff which will include also the tests. I
will try to address also the API aspects you pointed out (possibility to add
suitable parent classloader + to deal with getJarFile on StandardModule called
from some places).

BTW the usage of the proposed API can be seen under installer/jnlp/modules
(currently on branch platform_32247-41).

Created attachment 23261 [details]
Proposed patch against trunk as of 20050725

I hope that I have paid attention to all comments - please review the new diff.
Thanks a lot for your help.

Since Yarda is currently working on a rewrite of the core class loaders, I guess
he would be best qualified to review this in that context.

Are there any objections against my latest patch? Is there anyone willing to
review the changes before commit?

I really overlaps with Yarda's attempt to improve our classloaders tracked in
#59596 and takes more time than we estimated. I hope we will be able to merge both.

Few TCRs imho (Y07, Y13, Y14), a lot of not that important comments. See 
bellow: 
 
 
Y01: IllegalStateException shall at least be documented. 
 
+    public void setReloadable(boolean r) { 
+        throw new IllegalStateException(); 
+    } 
+     
 
Y02 Here you could rather fire some IOException, I guess: 
 
+    public void reload() throws IOException { 
+        throw new IllegalStateException(); 
+    } 
 
Y03 It would be better to use accessor methods then expose fields like this. 
This comment applies also to other places in the diff: 
 
+    /** module manifest */ 
+    protected Manifest manifest; 
 
Y04 protected final taking the manifest to use? Which would then be assigned 
to the manifest variable? 
-    private void parseManifest() throws InvalidException { 
+    public void parseManifest() throws InvalidException { 
 
 
Y05 should not this be protected? This and other methods is only supposed to 
be called by ModuleManager, right? 
+    public abstract void cleanup(); 
 
Y06 Why we need two methods getClasspathDelegateClassLoader and 
removeBaseClassLoader? Cannot we have just the first one which will either 
return del or del.getParent()? I guess the effect would be the same... 
+    /** 
+     * Allows specifying different parent classloader of all modules 
classloaders. 
+     */ 
+    public abstract ClassLoader getClasspathDelegateClassLoader(ModuleManager 
mgr, ClassLoader del); 
+     
+    /** 
+     * If this method returns true (the default) the parent the original 
classpath 
+     * classloader will be removed from the parent classloaders of a module 
classloader. 
+     */ 
+    public boolean removeBaseClassLoader() { 
+        return true; 
+    } 
 
Y07 write default impl of ModuleFactory instead of this. If lookup returns 
null, assign DefaultModuleFactory to the ModuleManager.moduleFactory field. 
The code shall be more readable then and all the "extra" cases handled in one 
place. Or do pu. st. ModuleFactory.getDefault(); 
+        Module m = null; 
+        if (moduleFactory == null) { 
+            m = new StandardModule(this, ev, jar.getAbsoluteFile(), history, 
+                    reloadable, autoload, eager); 
+        } else { 
+            m = moduleFactory.create(jar, history, reloadable, autoload,  
+                    eager, this, ev); 
+        } 
 
Y08 Why methods of ModuleManager need to be public like following one? 
-    void refineClassLoader(Module m, List parents) { 
+    public void refineClassLoader(Module m, List parents) { 
 
Y09 Opps, another clash with my work - the loaders are likely to be rewritten 
a bit, once again. So loadInOrder may not be there anymore... 
+     
+    /** 
+     * See loadInOrder(...). Can be overriden by special classloaders 
+     * (see project installer/jnlp/modules). 
+     */ 
+    protected boolean shouldBeCheckedAsParentProxyClassLoader() { 
+        return true; 
+    } 
 
Y10 Opps, this is horrible method name, I am affraid. Do you really need to 
disable it? 
+    /** 
+     * Allows turning off the optimilazation in loadInOrder(...). 
+     */ 
+    protected boolean optimizeNBLoading() { 
 
Y11 For all newly visible methods and classes please add @since tag and 
increment core/bootstrap and core/startup spec. versions. 
-    interface PackageAccessibleClassLoader { 
+    public interface PackageAccessibleClassLoader { 
 
Y12 Why checks like this one? 
+            if (m.getJarFile() == null) continue; 
 
Y13 This is unrelated, undocumented and untested. Imho you should write test 
to make this work forever and document it in arch*xml. 
+          
+        boolean defaultLocaleOnly = 
Boolean.getBoolean("netbeans.locale.default_only"); 
+        if (defaultLocaleOnly) { 
 
Y14 The test coverage is pretty minimal with respect to all the changes. It 
tests just returning del in the getClasspathDelegateClassLoader does not check 
 removeBaseClassLoader and I do not understand why we are making so many 
methods public if they are not used in the test scenarios. If this is the only  
test we have, then let's make FixedModule and StandardModule package private 
and return them from default impl. of ModuleFactory.create. And be more 
restrictive with respect to other changes as well.  

Created attachment 24133 [details]
New patch.

Hi, I have attached a new patch file. Should address some of your concerns.
Deatils follow:

Y1: Documented.
Y2: Changed.
Y3: Does not really matter as we don't design a public API that someone besides
really friend modules will use.
Y4: Same as Y3.
Y5: Same as Y3.
Y6: I have changed the default value of removeBaseClassLoader. It is IMHO good
to have 2 methods, one returning boolean the other one the classloader.
Y7: Done.
Y8: Because I need to call them from the Module subclass.
Y9, Y10: I need the methods otherwise my module (classloader) implementations fail.
Y11: Is this really needed (Y3)?
Y12: The code bellow the statement did not work for modules without a jar file.
Y13: Removed from the diff.
Y14: Yes, I thought I would provide more tests with the installer/jnlp/modules
module (but did not yet migrate the installer part to the trunk). Otherwise I
could have pasted all the code from there to core/bootstrap/test somewhere. I
did not think this being appropriate for the tests.

Can you please enumerate which issues I have to fix during this week since if I
guess correctly I have to finish this issue during this week. BTW did someone
announce the feature freeze publicly? Will I be able to work on this during
September still?

Created attachment 24135 [details]
Some cosmetic changes.

Last thing first: I've told you that you have to integrate this week. You will 
be allowed and forced to do bugfixes in September. 
 
You are right this is just a friend API, but still it is one of the most 
important implementation in NetBeans. We need to make it work. I agree with 
your explanation of (Y01, Y02, Y03, Y04). 
 
Y05 if anything it can be protected, make it protected. Regardless of the 
status of the API 
Y08 dtto. 
 
Y06 I do not understand the need for two methods. Maybe you think it is good, 
but I do not know why. There has to be a test to show why needed(TCR). 
 
Y14 Well, people working on core/bootstrap, core/startup, including me are not 
likely to run tests in some other module. We need some tests in the module we 
develop. If you copy some or not, is up to you. Just make sure most important 
aspects of classloading and reuse of the methods made public/protected. 
Otherwise we are going to break your code with first refactoring. 
 
Please make sure that ant -f nbbuild/build.xml commit-validation and ant -f 
nbbuild/build.xml unit-validation do not regress. In the second case it means 
that the amount of failed test is not increased after your change. If this 
applies and you improve the tests, and you are willing to bugfix your changes, 
I guess I do not have anything against the integration. 
 

Y5, Y8: will do asap.
Y6, Y14: I will add the test during tomorrow.

AFAIK I did not introduce any regression but will rerun the tests before
integration. I am ready to do any possible bugfixing in September - I am out of
office in the week 4-10 Sept but I beleive that there will be no substantial
problems during integration and I can fix the minor things after I am back after
10th Sept (and there is still the next week if anything would be broken).

I plan to integrate tomorrow.

Created attachment 24211 [details]
Before integration patch

I have made the new classes StandardModule and FixedModule package private.
Added more tests to check the supplying of alternate classloaders.

Checking in arch/arch-core-launcher.xml;
/cvs/core/arch/arch-core-launcher.xml,v  <--  arch-core-launcher.xml
new revision: 1.34; previous revision: 1.33
done
Checking in bootstrap/manifest.mf;
/cvs/core/bootstrap/manifest.mf,v  <--  manifest.mf
new revision: 1.4; previous revision: 1.3
done
RCS file: /cvs/core/bootstrap/src/org/netbeans/FixedModule.java,v
done
Checking in bootstrap/src/org/netbeans/FixedModule.java;
/cvs/core/bootstrap/src/org/netbeans/FixedModule.java,v  <--  FixedModule.java
initial revision: 1.1
done
Checking in bootstrap/src/org/netbeans/Module.java;
/cvs/core/bootstrap/src/org/netbeans/Module.java,v  <--  Module.java
new revision: 1.7; previous revision: 1.6
done
RCS file: /cvs/core/bootstrap/src/org/netbeans/ModuleFactory.java,v
done
Checking in bootstrap/src/org/netbeans/ModuleFactory.java;
/cvs/core/bootstrap/src/org/netbeans/ModuleFactory.java,v  <--  ModuleFactory.java
initial revision: 1.1
done
Checking in bootstrap/src/org/netbeans/ModuleManager.java;
/cvs/core/bootstrap/src/org/netbeans/ModuleManager.java,v  <--  ModuleManager.java
new revision: 1.4; previous revision: 1.3
done
Checking in bootstrap/src/org/netbeans/ProxyClassLoader.java;
/cvs/core/bootstrap/src/org/netbeans/ProxyClassLoader.java,v  <-- 
ProxyClassLoader.java
new revision: 1.20; previous revision: 1.19
done
RCS file: /cvs/core/bootstrap/src/org/netbeans/StandardModule.java,v
done
Checking in bootstrap/src/org/netbeans/StandardModule.java;
/cvs/core/bootstrap/src/org/netbeans/StandardModule.java,v  <--  StandardModule.java
initial revision: 1.1
done
Checking in bootstrap/src/org/netbeans/Util.java;
/cvs/core/bootstrap/src/org/netbeans/Util.java,v  <--  Util.java
new revision: 1.3; previous revision: 1.2
done
Checking in src/org/netbeans/core/LookupCache.java;
/cvs/core/src/org/netbeans/core/LookupCache.java,v  <--  LookupCache.java
new revision: 1.7; previous revision: 1.6
done
Checking in startup/manifest.mf;
/cvs/core/startup/manifest.mf,v  <--  manifest.mf
new revision: 1.4; previous revision: 1.3
done
Checking in startup/src/org/netbeans/core/startup/ModuleList.java;
/cvs/core/startup/src/org/netbeans/core/startup/ModuleList.java,v  <-- 
ModuleList.java
new revision: 1.4; previous revision: 1.3
done
Checking in startup/src/org/netbeans/core/startup/ModuleSystem.java;
/cvs/core/startup/src/org/netbeans/core/startup/ModuleSystem.java,v  <-- 
ModuleSystem.java
new revision: 1.2; previous revision: 1.1
done
Checking in startup/src/org/netbeans/core/startup/NbInstaller.java;
/cvs/core/startup/src/org/netbeans/core/startup/NbInstaller.java,v  <-- 
NbInstaller.java
new revision: 1.14; previous revision: 1.13
done
Checking in startup/src/org/netbeans/core/startup/TopLogging.java;
/cvs/core/startup/src/org/netbeans/core/startup/TopLogging.java,v  <-- 
TopLogging.java
new revision: 1.2; previous revision: 1.1
done
RCS file:
/cvs/core/startup/test/unit/src/org/netbeans/core/startup/ModuleFactoryTest.java,v
done
Checking in startup/test/unit/src/org/netbeans/core/startup/ModuleFactoryTest.java;
/cvs/core/startup/test/unit/src/org/netbeans/core/startup/ModuleFactoryTest.java,v
 <--  ModuleFactoryTest.java
initial revision: 1.1
done