Jim's Javadoc review

27 files changed, 3972 insertions, 474 deletions

diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IElement.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IElement.java
index a66d6ddb7..e43316ed6 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IElement.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IElement.java
@@ -1,6 +1,6 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
- * All rights reserved.   This program and the accompanying materials
+ * Copyright (c) 2004 IBM Corporation and others.
+ * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
  * http://www.eclipse.org/legal/cpl-v10.html
@@ -16,31 +16,67 @@ import java.util.Map;
 import org.eclipse.core.runtime.CoreException;
 import org.eclipse.core.runtime.IStatus;
 /**
- * An element.
+ * Provides common methods for working with elements that are
+ * modified via working copies. This interface provides the
+ * getters; the setters are on {@link IElementWorkingCopy}.
+ * <p>
+ * [issue: Why are attributes exposed? The attribute ids and
+ * values are passed to property change listeners. However,
+ * they are not useful unless there is a spec'd correlation
+ * between methods like getName and an attribute "name". The
+ * constants are declared on Base, which is internal.]
+ * </p>
+ * <p>
+ * This interface is not intended to be implemented by clients.
+ * </p>
  * 
- * <p>This interface is not intended to be implemented by clients.</p>
+ * @since 1.0
  */
 public interface IElement {
 	/**
-	 * Return the label (name) of this element.
-	 * 
-	 * @return java.lang.String
+	 * Returns the displayable name for this element.
+	 * <p>
+	 * Note that this name is appropriate for the current locale.
+	 * </p>
+	 *
+	 * @return a displayable name
 	 */
 	public String getName();
 	
 	/**
-	 * Return the id of this element.
+	 * Returns the id of this element.
+	 * Each element (of a given type) has a distinct id, fixed for
+	 * its lifetime. Ids are intended to be used internally as keys;
+	 * they are not intended to be shown to end users.
 	 * 
-	 * @return java.lang.String
+	 * @return the element id
 	 */
 	public String getId();
 
+	/**
+	 * Deletes the persistent representation of this element.
+	 * <p>
+	 * [issue: This method is out of place. Elements do not
+	 * have a notion of being connected to an underlying file.]
+	 * </p>
+	 * 
+	 * @throws CoreException [missing]
+	 */
 	public void delete() throws CoreException;
 
 	/**
-	 * Returns true if this element is locked. (user cannot make changes).
+	 * Returns whether this element is locked.
+	 * When an element is locked, the user cannot make changes to it.
+	 * <p>
+	 * [issue: It's odd to have this at the API level because
+	 * it has no force. Once would modify a locked element as
+	 * readily as an unlocked one, via a working copy.
+	 * This facility, which is unused, needs to be motivated
+	 * (or removed).]
+	 * </p>
 	 *
-	 * @return boolean
+	 * @return <code>true</code> if this element is locked,
+	 * and <code>false</code> otherwise
 	 */
 	public boolean isLocked();
 
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IElementWorkingCopy.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IElementWorkingCopy.java
index 3131f10cf..a739f46b4 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IElementWorkingCopy.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IElementWorkingCopy.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved.   This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -14,47 +14,145 @@ import java.beans.PropertyChangeListener;
 import java.util.List;
 import java.util.Map;
 /**
- * A server element.
+ * Provides additional methods common to working copies of an element.
+ * <p>
+ * This interface is not intended to be implemented by clients.
+ * </p>
  * 
- * <p>This interface is not intended to be implemented by clients.</p>
+ * @since 1.0
  */
 public interface IElementWorkingCopy extends IElement {
-	public void setAttribute(String attributeName, int value);
+	
+	/**
+	 * Sets the value of the specified integer-valued attribute of this
+	 * element.
+	 * 
+	 * @param id the attribute id
+	 * @param value the value of the specified attribute
+	 * @see IElement#getAttribute(String, int)
+	 */
+	public void setAttribute(String id, int value);
 
-	public void setAttribute(String attributeName, boolean value);
+	/**
+	 * Sets the value of the specified boolean-valued attribute of this
+	 * element.
+	 * 
+	 * @param id the attribute id
+	 * @param value the value of the specified attribute
+	 * @see IElement#getAttribute(String, boolean)
+	 */
+	public void setAttribute(String id, boolean value);
 
-	public void setAttribute(String attributeName, String value);
+	/**
+	 * Sets the value of the specified string-valued attribute of this
+	 * element.
+	 * 
+	 * @param id the attribute id
+	 * @param value the value of the specified attribute
+	 * @see IElement#getAttribute(String, String)
+	 */
+	public void setAttribute(String id, String value);
 
-	public void setAttribute(String attributeName, List value);
+	/**
+	 * Sets the value of the specified list-valued attribute of this
+	 * element.
+	 * <p>
+	 * [issue: Serialization/deserialization]
+	 * </p>
+	 * 
+	 * @param id the attribute id
+	 * @param value the value of the specified attribute
+	 * @see IElement#getAttribute(String, List)
+	 */
+	public void setAttribute(String id, List value);
 
-	public void setAttribute(String attributeName, Map value);
+	/**
+	 * Sets the value of the specified map-valued attribute of this
+	 * element.
+	 * <p>
+	 * [issue: Serialization/deserialization]
+	 * </p>
+	 * 
+	 * @param id the attribute id
+	 * @param value the value of the specified attribute
+	 * @see IElement#getAttribute(String, Map)
+	 */
+	public void setAttribute(String id, Map value);
 
+	/**
+	 * Sets the displayable name for this element.
+	 * <p>
+	 * The name should be appropriate for the current locale.
+	 * </p>
+	 *
+	 * @param name a displayable name
+	 * @see IElement#getName()
+	 */
 	public void setName(String name);
 	
+	/**
+	 * Sets whether this element is locked.
+	 * When an element is locked, the user cannot make changes to it.
+	 * <p>
+	 * [issue: See issue on IElement.isLocked.]
+	 * </p>
+	 *
+	 * @param b <code>true</code> if this element is locked,
+	 * and <code>false</code> otherwise
+	 * @see IElement#isLocked()
+	 */
 	public void setLocked(boolean b);
 	
+	/**
+	 * Sets whether this element is private.
+	 * Generally speaking, elements marked private are internal ones
+	 * that should not be shown to users (because they won't know
+	 * anything about them).
+	 * 
+	 * @param b <code>true</code> if this element is private,
+	 * and <code>false</code> otherwise
+	 * @see IElement#isPrivate()
+	 */
 	public void setPrivate(boolean b);
 	
+	/**
+	 * Returns whether this working copy has unsaved changes.
+	 * <p>
+	 * [issue: It unfortunate that working copy creation and saving
+	 * are nowhere represented in the IElement and IElementWorkingCopy
+	 * (everything is on the particular subinterfaces).]
+	 * </p>
+	 * 
+	 * @return <code>true</code> if this working copy has unsaved
+	 * changes, and <code>false</code> otherwise
+	 */
 	public boolean isDirty();
 	
+	/**
+	 * Releases this working copy.
+	 * <p>
+	 * Element implementations retain internal references to
+	 * extant working copies. Calling this method drop that reference.
+	 * </p>
+	 */
 	public void release();
 
 	/**
-	 * Add a property change listener to this server.
+	 * Adds a property change listener to this server.
 	 *
 	 * @param listener java.beans.PropertyChangeListener
 	 */
 	public void addPropertyChangeListener(PropertyChangeListener listener);
 
 	/**
-	 * Remove a property change listener from this server.
+	 * Removes a property change listener from this server.
 	 *
 	 * @param listener java.beans.PropertyChangeListener
 	 */
 	public void removePropertyChangeListener(PropertyChangeListener listener);
 	
 	/**
-	 * Fire a property change event.
+	 * Fires a property change event.
 	 */
 	public void firePropertyChangeEvent(String propertyName, Object oldValue, Object newValue);
 }
\ No newline at end of file
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IModuleFactory.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IModuleFactory.java
index 97995d617..afc934b9f 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IModuleFactory.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IModuleFactory.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -16,53 +16,142 @@ import org.eclipse.wst.server.core.model.IModule;
 import org.eclipse.wst.server.core.model.IModuleFactoryDelegate;
 import org.eclipse.wst.server.core.model.IModuleFactoryListener;
 /**
- * A module factory.
+ * A factory for creating modules.
+ * <p>
+ * The server core framework supports
+ * an open-ended set of module factories, which are contributed via
+ * the <code>moduleFactories</code> extension point in the server core
+ * plug-in. The global list of module factories is available via
+ * {@link ServerCore#getModuleFactories()}. 
+ * </p>
+ * <p>
+ * [issue: Module factories have no display name, suggesting that
+ * they never need to be shown to users.]
+ * </p>
+ * <p>
+ * [issue: Are module factories SPI-side objects, or do
+ * normal clients need access to them?]
+ * </p>
+ * <p>
+ * This interface is not intended to be implemented by clients.
+ * </p>
+ * <p>
+ * [issue: It is notoriously difficult to place any kind of
+ * useful order on objects that are contributed independently by
+ * non-collaborating parties. The IOrdered mechanism is weak, and
+ * can't really solve the problem. Issues of presentation are usually
+ * best left to the UI, which can sort objects based on arbitrary
+ * properties.]
+ * </p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
  * 
- * <p>This interface is not intended to be implemented by clients.</p>
+ * @since 1.0
  */
 public interface IModuleFactory extends IOrdered {
+
 	/**
-	 * Returns the id of the factory.
-	 *
-	 * @return java.lang.String
+	 * Returns the id of this module factory.
+	 * Each known module factory has a distinct id. 
+	 * Ids are intended to be used internally as keys; they are not
+	 * intended to be shown to end users.
+	 * 
+	 * @return the module factory id
 	 */
 	public String getId();
 	
 	/**
-	 * Returns the type of module that the factory produces.
-	 * @return
+	 * Returns the types of modules that the factory is capable of
+	 * producing.
+	 * <p>
+	 * [issue: The list returned is precious. You would not want a client
+	 * to accidentally or malicously whack it. Normal practice is to
+	 * return an array instead of a List, and to return a new copy each call.
+	 * This allows the spec to say that the client can do what they want
+	 * with the result, and that it won't change under foot.
+	 * Another alternative is to return a UnmodifiableList implementation
+	 * so that clients cannot modify.]
+	 * </p>
+	 * 
+	 * @return a list of module types (element type: {@link IModuleType})
 	 */
 	public List getModuleTypes();
 	
 	/**
-	 * Returns true if this module factory produces project modules.
+	 * Returns whether this module factory produces project modules.
+	 * <p>
+	 * [issue: This surfaces the "projects" attribute of the
+	 * moduleFactory element. What is the significance of this?
+	 * (There are no senders of this method.)]
+	 * </p>
 	 *
-	 * @return boolean
+	 * @return <code>true</code> if it can produce project modules,
+	 * and <code>false</code> if it cannot
 	 */
 	public boolean isProjectModuleFactory();
 
 	/**
-	 * Returns the delegate for this factory.
+	 * Returns the delegate for this module factory.
+	 * The module factory delegate is a module-factory-specific object.
+	 * <p>
+	 * [issue: If module factories are SPI-side objects, then
+	 * exposing the delegate is probably fine. If module factories
+	 * are available to clients, you'll want to keep the delegate
+	 * out of the client's hands.]
+	 * </p>
 	 * 
-	 * @return org.eclipse.wst.server.core.model.IModuleFactoryDelegate
+	 * @return the module factory delegate
 	 */
 	public IModuleFactoryDelegate getDelegate();
 
 	/**
-	 * Gets a module from the factory with the given memento.
+	 * Finds a module create by this factory with the given id.
+	 * <p>
+	 * [issue: Does this "create" a module with the given id?
+	 * That's what you'd expect of a factory. But a module has
+	 * a module type and module resources, neither of which are
+	 * in evidence. And since a module factory can create
+	 * several types of modules, that seems to rule this out
+	 * as "creating" a module. Is "discovering" a module the
+	 * correct interpretation?
+	 * The (abstract) ProjectModuleFactoryDelegate class does indeed
+	 * rip through all the projects in the workspace and ask which
+	 * ones hold valid modules.] 
+	 * </p>
 	 * 
-	 * @param memento java.lang.String
-	 * @return org.eclipse.wst.server.core.model.IModule
+	 * @param id the module id
+	 * @return the module with the given id, or <code>null</code>
+	 * if none
 	 */
-	public IModule getModule(String memento);
+	public IModule getModule(String id);
 
 	/**
-	 * Return all modules that are available to be added
-	 * to servers. This method might look through projects
-	 * to find modules or may return modules from
-	 * other sources.
-	 *
-	 * @return java.util.List
+	 * Return all modules created by this factory.
+	 * <p>
+	 * Note: Implementations of this method might look through
+	 * projects in the workspace to find modules, or might return
+	 * modules from other sources.
+	 * </p>
+	 * <p>
+	 * [issue: What does "modules from other sources" mean?
+	 * All modules have to be in the workspace, right?]
+	 * </p>
+	 * <p>
+	 * [issue: Consistency: IServer.getModules returns IModule[] rather than List<IModule>.]
+	 * </p>
+	 * <p>
+	 * [issue: The list returned is precious. You would not want a client
+	 * to accidentally or malicously whack it. Normal practice is to
+	 * return an array instead of a List, and to return a new copy each call.
+	 * This allows the spec to say that the client can do what they want
+	 * with the result, and that it won't change under foot.
+	 * Another alternative is to return a UnmodifiableList implementation
+	 * so that clients cannot modify.]
+	 * </p>
+	 * 
+	 * @return a list of modules (element type: {@link IModule})
 	 */
 	public List getModules();
 
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IModuleKind.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IModuleKind.java
index a771da8f7..c15e6aa0f 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IModuleKind.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IModuleKind.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -10,21 +10,52 @@
  **********************************************************************/
 package org.eclipse.wst.server.core;
 /**
+ * Represents the kind of a module.
+ * <p>
+ * The server core framework supports an open-ended set of module kinds,
+ * which are contributed via the <code>moduleKinds</code> extension point
+ * in the server core plug-in. Module kind objects carry no state
+ * (all information is read-only and is supplied by the module kind
+ * declaration). The global list of known module kinds is available via
+ * {@link ServerCore#getModuleKinds()}. 
+ * </p>
+ * <p>
+ * [issue: It's confusing to have a type named IModuleType as well.
+ * The terminology should be "module types", to make it consistent with
+ * server types, etc. On that reading, this interface would be named
+ * IModuleType, and the existing one something else (if still needed).]
+ * </p>
+ * <p>
+ * This interface is not intended to be implemented by clients.
+ * </p>
+ * <p>
+ * [issue: Equality/identify for module kinds?]
+ * </p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
  * 
- * <p>This interface is not intended to be implemented by clients.</p>
+ * @since 1.0
  */
 public interface IModuleKind {
 	/**
-	 * Returns the id, e.g. "j2ee.web".
+	 * Returns the id of this module kind.
+	 * Each known module kind has a distinct id.
+	 * Ids are intended to be used internally as keys; they are not
+	 * intended to be shown to end users.
 	 * 
-	 * @return java.lang.String
+	 * @return the module kind id
 	 */
 	public String getId();
 
 	/**
-	 * Returns the name, e.g. "Web module".
-	 * 
-	 * @return java.lang.String
+	 * Returns the displayable name for this module kind.
+	 * <p>
+	 * Note that this name is appropriate for the current locale.
+	 * </p>
+	 *
+	 * @return a displayable name for this module kind
 	 */
 	public String getName();
 }
\ No newline at end of file
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IModuleType.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IModuleType.java
index 3d3cf27d2..8fdb73fcc 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IModuleType.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IModuleType.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -10,19 +10,67 @@
  **********************************************************************/
 package org.eclipse.wst.server.core;
 /**
+ * Represents the type of a module.
+ * <p>
+ * [issue: This interface plays two roles. First, it is used
+ * by the system to represent module type objects. Module
+ * type objects are instances of the internal ModuleType class.
+ * They get created for the "moduleType" elements of the
+ * runtimeTypes and moduleFactories extension points.
+ * The second role is that it is a superinterface of IModule,
+ * the interface that represents module instances.
+ * It would be clearer if it only played the former role.
+ * IModule should instance have a getModuleType() method that
+ * returns an IModuleType.]
+ * </p>
+ * <p>
+ * [issue: It's confusing to have a type named IModuleKind as well.
+ * The terminology should be "module types", to make it consistent with
+ * server types, etc. On that reading, this interface would be named
+ * IModuleType, and the existing one something else (if still needed).]
+ * </p>
+ * <p>
+ * This interface is not intended to be implemented by clients.
+ * </p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
  * 
- * <p>This interface is not intended to be implemented by clients.</p>
+ * @since 1.0
  */
 public interface IModuleType {
+	
 	/**
-	 * Returns the type, e.g. "j2ee.ejb".
-	 * @return
+	 * Returns the module type id.
+	 * <p>
+	 * [issue: Are these strings "module kind ids"? I.e., the same as those returned by 
+	 * IModuleKind.getId()?]
+	 * </p>
+	 * <p>
+	 * [issue: Should be renamed getId (except that would clash
+	 * in its current role as superfinterface of IModule).]
+	 * </p>
+	 * 
+	 * @return the module type id
 	 */
 	public String getType();
 
 	/**
-	 * Returns the version (spec level), e.g. "1.0", "1.3.2"
-	 * @return
+	 * Returns the version (spec level), e.g., "1.0", "1.3.2".
+	 * <p>
+	 * [issue: This notion of a module type "version" appears here.
+	 * There is no counterpart elsewhere (and certainly not in
+	 * IModuleKind). The phrase "spec level" suggests something
+	 * a little more J2EE-centric (what would be the spec level for
+	 * a static html web module?) It feels like this should be folded
+	 * in to the module type/kind id.]
+	 * </p>
+	 * <p>
+	 * [issue: Spec format of version string?]
+	 * </p>
+	 * 
+	 * @return the version
 	 */
 	public String getVersion();
 }
\ No newline at end of file
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IResourceManager.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IResourceManager.java
index 32c1a5ac6..afcc0b9bb 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IResourceManager.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IResourceManager.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -13,22 +13,34 @@ package org.eclipse.wst.server.core;
 import java.util.List;
 
 import org.eclipse.core.resources.IFile;
-import org.eclipse.wst.server.core.model.*;
+import org.eclipse.wst.server.core.model.IModuleEventsListener;
+import org.eclipse.wst.server.core.model.IServerLifecycleEventHandler;
+import org.eclipse.wst.server.core.model.IServerResourceListener;
 /**
- * IResourceManager handles the mappings between resources
- * and servers or configurations, and creates notification
- * of servers or configurations being added, removed, or modified.
- *
- * <p>Servers and configurations may be a single resource, or they may
- * be a folder that contains a group of files.Folder-resources may not
- * contain other servers or configurations. (i.e. they cannot be nested)</p>
- *
- * <p>Changes made to server element resources (e.g. an edit or deletion of a
+ * The resource manager handles the mappings between resources
+ * and servers or configurations, and notifies of servers or configurations
+ * being added, removed, or modified.
+ * <p>
+ * Servers and configurations may be a single resource, or they may
+ * be a folder that contains a group of files. Folder resources may not
+ * contain other servers or configurations (i.e., they cannot be nested).
+ * </p>
+ * <p>
+ * Changes made to server element resources (e.g., an edit or deletion of a
  * file) are processed as a reload or deletion of the element. Note that saving
  * a folder-based server or configuration may result in a series of reload
- * events.</p>
- *
- * <p>This interface is not intended to be implemented by clients.</p>
+ * events.
+ * </p>
+ * <p>
+ * [issue: Not sure why "resource" is in the title. Runtimes
+ * are not represented in the workspace. This grouping of things pertaining
+ * to workspace resources is not particularly motivating to clients.]
+ * </p>
+ * <p>
+ * This interface is not intended to be implemented by clients.
+ * </p>
+ * 
+ * @since 1.0
  */
 public interface IResourceManager {
 	/**
@@ -46,96 +58,323 @@ public interface IResourceManager {
 	public void removeResourceListener(IServerResourceListener listener);
 
 	/**
-	 * Returns a list of all runtimes.
-	 *
-	 * @return java.util.List
+	 * Returns the list of all known runtime instances.
+	 * <p>
+	 * Clients must not modify the list that is returned.
+	 * If the set of runtimes changes, the affect on
+	 * the returned list is unspecified.
+	 * </p>
+	 * <p>
+	 * [issue: The list returned is precious. You would not want a client
+	 * to accidentally or malicously whack it. Normal practice is to
+	 * return an array instead of a List, and to return a new copy each call.
+	 * This allows the spec to say that the client can do what they want
+	 * with the result, and that it won't change under foot.
+	 * Another alternative is to return a UnmodifiableList implementation
+	 * so that clients cannot modify. But if you don't copy, you still
+	 * have the problem of the list changing under foot as runtime instances
+	 * come and go.]
+	 * </p>
+	 * <p>
+	 * [issue: Clarify whether the list may include working copies.]
+	 * </p>
+	 * 
+	 * @return a possibly-empty list of runtime instances (element type: {@link IRuntime})
 	 */
 	public List getRuntimes();
 	
 	/**
 	 * Returns the default runtime. Test API - do not use.
+	 * <p>
+	 * [issue: This is marked "Test API - do not use."]
+	 * </p>
 	 *
-	 * @return java.util.List
+	 * @return a runtime instance, or <code>null</code> if none
+	 * @see #setDefaultRuntime(IRuntime)
 	 */
 	public IRuntime getDefaultRuntime();
 	
 	/**
-	 * Sets the default runtime. Test API - do not use.
+	 * Sets the default runtime.
+	 * <p>
+	 * [issue: This is marked "Test API - do not use."]
+	 * </p>
 	 *
-	 * @return java.util.List
+	 * @param runtime a runtime instance, or <code>null</code>
+	 * @see #getDefaultRuntime()
 	 */
 	public void setDefaultRuntime(IRuntime runtime);
 	
 	/**
-	 * Returns the runtimes with the given runtime type.
-	 *
-	 * @return java.util.List
+	 * Returns the list of all known runtime instances of
+	 * the given runtime type. This convenience method filters the list of known
+	 * runtime ({@link #getRuntimes()}) for ones with a matching
+	 * runtime type ({@link IRuntime#getRuntimeType()}).
+	 * <p>
+	 * Clients must not modify the list that is returned.
+	 * If the set of runtimes changes, the affect on
+	 * the returned list is unspecified.
+	 * </p>
+	 * <p>
+	 * [issue: The list returned is precious. You would not want a client
+	 * to accidentally or malicously whack it. Normal practice is to
+	 * return an array instead of a List, and to return a new copy each call.
+	 * This allows the spec to say that the client can do what they want
+	 * with the result, and that it won't change under foot.
+	 * Another alternative is to return a UnmodifiableList implementation
+	 * so that clients cannot modify. But if you don't copy, you still
+	 * have the problem of the list changing under foot as runtime instances
+	 * come and go.]
+	 * </p>
+	 * <p>
+	 * [issue: Clarify whether the list may include working copies.]
+	 * </p>
+	 * <p>
+	 * [issue: Is this convenience method really necessary?
+	 * It's straightforward enough for a client to do.]
+	 * </p>
+	 * 
+	 * @param runtimeType the runtime type
+	 * @return a possibly-empty list of runtime instances (element type: {@link IRuntime})
+	 * of the given runtime type
 	 */
 	public List getRuntimes(IRuntimeType runtimeType);
 	
 	/**
-	 * Returns the runtime with the given id.
+	 * Returns the runtime with the given id, or <code>null</code>
+	 * if none. This convenience method searches the list of known
+	 * runtimes ({@link #getRuntimes()}) for the one with a matching
+	 * runtime id ({@link IRuntime#getId()}).
+	 * <p>
+	 * [issue: It does not really make sense for a key parameter
+	 * like id to be null. 
+	 * Null id should be spec'd as illegal, 
+	 * and the implementation should immediately throw an unspecified 
+	 * RuntimeException if null is passed.]
+	 * </p>
+	 * <p>
+	 * [issue: Consider renaming this method findRuntime to make
+	 * it clear that it is searching.]
+	 * </p>
 	 *
-	 * @return IRuntime
+	 * @param the runtime id, or <code>null</code>
+	 * @return the runtime instance, or <code>null</code> if 
+	 * id is <code>null</code> or there is no runtime
+	 * with the given id
 	 */
 	public IRuntime getRuntime(String id);
 
 	/**
-	 * Returns a list of all servers.
-	 *
-	 * @return java.util.List
+	 * Returns the list of all known server instances.
+	 * <p>
+	 * Clients must not modify the list that is returned.
+	 * If the set of servers changes, the affect on
+	 * the returned list is unspecified.
+	 * </p>
+	 * <p>
+	 * [issue: The list returned is precious. You would not want a client
+	 * to accidentally or malicously whack it. Normal practice is to
+	 * return an array instead of a List, and to return a new copy each call.
+	 * This allows the spec to say that the client can do what they want
+	 * with the result, and that it won't change under foot.
+	 * Another alternative is to return a UnmodifiableList implementation
+	 * so that clients cannot modify. But if you don't copy, you still
+	 * have the problem of the list changing under foot as server instances
+	 * come and go.]
+	 * </p>
+	 * <p>
+	 * [issue: Clarify whether the list may include working copies.]
+	 * </p>
+	 * 
+	 * @return a possibly-empty list of server instances (element type: {@link IServer})
 	 */
 	public List getServers();
 
 	/**
-	 * Returns the server with the given id.
+	 * Returns the server with the given id, or <code>null</code>
+	 * if none. This convenience method searches the list of known
+	 * servers ({@link #getServers()}) for the one with a matching
+	 * server id ({@link IServer#getId()}).
+	 * <p>
+	 * [issue: It does not really make sense for a key parameter
+	 * like id to be null. 
+	 * Null id should be spec'd as illegal, 
+	 * and the implementation should immediately throw an unspecified 
+	 * RuntimeException if null is passed.]
+	 * </p>
+	 * <p>
+	 * [issue: Consider renaming this method findServer to make
+	 * it clear that it is searching.]
+	 * </p>
 	 *
-	 * @return java.util.List
+	 * @param the server id, or <code>null</code>
+	 * @return the server instance, or <code>null</code> if 
+	 * id is <code>null</code> or there is no server
+	 * with the given id
 	 */
 	public IServer getServer(String id);
 
 	/**
-	 * Returns the server that came from the given file.
+	 * Returns the server that came from the given file, or <code>null</code>
+	 * if none. This convenience method searches the list of known
+	 * servers ({@link #getServers()}) for the one with a matching
+	 * location ({@link IServer#getFile()}).
+	 * <p>
+	 * [issue: Is this convenience method really necessary?
+	 * It's straightforward enough for a client to do.]
+	 * </p>
+	 * <p>
+	 * [issue: Consider renaming this method findServer to make
+	 * it clear that it is searching.]
+	 * </p>
 	 *
-	 * @param file org.eclipse.core.resources.IFile
-	 * @return org.eclipse.wst.server.model.IServer
+	 * @param a server file
+	 * @return the server instance, or <code>null</code> if 
+	 * there is no server associated with the given file
 	 */
 	public IServer getServer(IFile file);
 	
 	/**
-	 * Returns a list of all servers.
-	 *
-	 * @return java.util.List
+	 * Returns the list of all known server instances of
+	 * the given server type. This convenience method filters the list of known
+	 * servers ({@link #getServers()}) for ones with a matching
+	 * server type ({@link IServer#getServerType()}).
+	 * <p>
+	 * Clients must not modify the list that is returned.
+	 * If the set of servers changes, the affect on
+	 * the returned list is unspecified.
+	 * </p>
+	 * <p>
+	 * [issue: The list returned is precious. You would not want a client
+	 * to accidentally or malicously whack it. Normal practice is to
+	 * return an array instead of a List, and to return a new copy each call.
+	 * This allows the spec to say that the client can do what they want
+	 * with the result, and that it won't change under foot.
+	 * Another alternative is to return a UnmodifiableList implementation
+	 * so that clients cannot modify. But if you don't copy, you still
+	 * have the problem of the list changing under foot as server instances
+	 * come and go.]
+	 * </p>
+	 * <p>
+	 * [issue: Clarify whether the list may include working copies.]
+	 * </p>
+	 * <p>
+	 * [issue: Is this convenience method really necessary?
+	 * It's straightforward enough for a client to do.]
+	 * </p>
+	 * 
+	 * @param serverType the server type
+	 * @return a possibly-empty list of server instances (element type: {@link IServer})
+	 * of the given server type
 	 */
 	public List getServers(IServerType serverType);
 
 	/**
-	 * Returns a list of all currently active server configurations.
-	 *
-	 * @return java.util.List
+	 * Returns the list of all known server configuration instances.
+	 * <p>
+	 * Clients must not modify the list that is returned.
+	 * If the set of server configurations changes, the affect on
+	 * the returned list is unspecified.
+	 * </p>
+	 * <p>
+	 * [issue: The list returned is precious. You would not want a client
+	 * to accidentally or malicously whack it. Normal practice is to
+	 * return an array instead of a List, and to return a new copy each call.
+	 * This allows the spec to say that the client can do what they want
+	 * with the result, and that it won't change under foot.
+	 * Another alternative is to return a UnmodifiableList implementation
+	 * so that clients cannot modify. But if you don't copy, you still
+	 * have the problem of the list changing under foot as server 
+	 * configuration instances come and go.]
+	 * </p>
+	 * <p>
+	 * [issue: Clarify whether the list may include working copies.]
+	 * </p>
+	 * 
+	 * @return a possibly-empty list of server configuration instances 
+	 * (element type: {@link IServerConfiguration})
 	 */
 	public List getServerConfigurations();
 	
 	/**
-	 * Returns a list of all currently active server configurations.
-	 *
-	 * @return java.util.List
+	 * Returns the list of all known server configuration instances of
+	 * the given server configuration type. This convenience method filters
+	 * the list of known server configurations
+	 * ({@link #getServerConfigurations()}) for ones with a matching
+	 * server configuration type
+	 * ({@link IServerConfiguration#getServerConfigurationType()}).
+	 * <p>
+	 * Clients must not modify the list that is returned.
+	 * If the set of server configurations changes, the affect on
+	 * the returned list is unspecified.
+	 * </p>
+	 * <p>
+	 * [issue: The list returned is precious. You would not want a client
+	 * to accidentally or malicously whack it. Normal practice is to
+	 * return an array instead of a List, and to return a new copy each call.
+	 * This allows the spec to say that the client can do what they want
+	 * with the result, and that it won't change under foot.
+	 * Another alternative is to return a UnmodifiableList implementation
+	 * so that clients cannot modify. But if you don't copy, you still
+	 * have the problem of the list changing under foot as server 
+	 * configuration instances come and go.]
+	 * </p>
+	 * <p>
+	 * [issue: Clarify whether the list may include working copies.]
+	 * </p>
+	 * <p>
+	 * [issue: Is this convenience method really necessary?
+	 * It's straightforward enough for a client to do.]
+	 * </p>
+	 * 
+	 * @param configType the server configuration type
+	 * @return a possibly-empty list of server configuration instances
+	 * (element type: {@link IServer}) of the given server configuration type
 	 */
 	public List getServerConfigurations(IServerConfigurationType configType);
 
 	/**
-	 * Returns the servers with the given id.
+	 * Returns the server configuration with the given id, or <code>null</code>
+	 * if none. This convenience method searches the list of known
+	 * server configurations ({@link #getServerConfigurations()}) for the one
+	 * with a matching server configuration id
+	 * ({@link IServerConfiguration#getId()}).
+	 * <p>
+	 * [issue: It does not really make sense for a key parameter
+	 * like id to be null. 
+	 * Null id should be spec'd as illegal, 
+	 * and the implementation should immediately throw an unspecified 
+	 * RuntimeException if null is passed.]
+	 * </p>
+	 * <p>
+	 * [issue: Consider renaming this method findServerConfiguration to make
+	 * it clear that it is searching.]
+	 * </p>
 	 *
-	 * @return java.util.List
+	 * @param the server configuration id, or <code>null</code>
+	 * @return the server configuration instance, or <code>null</code> if 
+	 * id is <code>null</code> or there is no server configuration
+	 * with the given id
 	 */
 	public IServerConfiguration getServerConfiguration(String id);
 
 	/**
-	 * Returns the server configuration that came from the
-	 * given resource.
+	 * Returns the server configuration that came from the given file, 
+	 * or <code>null</code> if none. This convenience method searches the list
+	 * of known server configurations ({@link #getServerConfigurations()}) for
+	 * the one with a matching location ({@link IServerConfiguration#getFile()}).
+	 * <p>
+	 * [issue: Is this convenience method really necessary?
+	 * It's straightforward enough for a client to do.]
+	 * </p>
+	 * <p>
+	 * [issue: Consider renaming this method findServerConfiguration to make
+	 * it clear that it is searching.]
+	 * </p>
 	 *
-	 * @param resource org.eclipse.core.resources.IResource
-	 * @return org.eclipse.wst.server.model.IServerConfiguration
+	 * @param a server configuration file
+	 * @return the server configuration instance, or <code>null</code> if 
+	 * there is no server configuration associated with the given file
 	 */
 	public IServerConfiguration getServerConfiguration(IFile file);
 
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IRuntime.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IRuntime.java
index 007cf8f68..6376cdad9 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IRuntime.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IRuntime.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -15,24 +15,145 @@ import org.eclipse.core.runtime.IStatus;
 
 import org.eclipse.wst.server.core.model.IRuntimeDelegate;
 /**
+ * Represents a runtime instance. Every runtime is an instance of a
+ * particular, fixed runtime type.
+ * <p>
+ * Servers have a runtime. The server runtime corresponds to the
+ * installed code base for the server. The main role played by the server
+ * runtime is in identifying code libraries to compile or build against.
+ * In the case of local servers, the server runtime may play a secondary role
+ * of being used to launch the server for testing. Having the server runtimes
+ * identified as an entity separate from the server itself facilitates sharing
+ * server runtimes between several servers.
+ * </p>
+ * <p>
+ * [issue: As mentioned in an issue on IRuntimeType, the term "runtime"
+ * is misleading, given that the main reason is for build time classpath
+ * contributions, not for actually running anything. "libraries" might be a
+ * better choice.]
+ * </p>
+ * <p>
+ * The resource manager maintains a global list of all known runtime instances
+ * ({@link IResourceManager#getRuntimes()}).
+ * </p>
+ * <p>
+ * [issue: Equality/identify for runtimes?]
+ * </p>
  * 
  * <p>This interface is not intended to be implemented by clients.</p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
+ * 
+ * @since 1.0
  */
 public interface IRuntime extends IElement {
+	
+	/**
+	 * Returns the type of this runtime instance.
+	 * 
+	 * @return the runtime type
+	 */
 	public IRuntimeType getRuntimeType();
 
+	/**
+	 * Returns the delegate for this runtime.
+	 * The runtime delegate is a runtime-type-specific object.
+	 * By casting the runtime delegate to the type prescribed in
+	 * the API documentation for that particular runtime type, 
+	 * the client can access runtime-type-specific properties and
+	 * methods.
+	 * <p>
+	 * [issue: Exposing IRuntimeDelegate to clients of IRuntime
+	 * is confusing and dangerous. Instead, replace this
+	 * method with something like getRuntimeExtension() which
+	 * returns an IRuntimeExtension. IRuntimeExtension is an
+	 * "marker" interface that runtime providers would 
+	 * implement or extend if they want to expose additional
+	 * API for their runtime type. That way IRuntimeDelegate
+	 * can be kept entirely on the SPI side, out of view from 
+	 * clients.]
+	 * </p>
+	 * <p>
+	 * [issue: runtimeTypes schema, class attribute is optional.
+	 * This suggests that a server need not provide a delegate class.
+	 * This seems implausible. I've spec'd this method as delegate optional.]
+	 * </p>
+	 * 
+	 * @return the runtime delegate, or <code>null</code> if none
+	 */
 	public IRuntimeDelegate getDelegate();
 
+	/**
+	 * Returns a runtime working copy for modifying this runtime instance.
+	 * If this instance is already a working copy, it is returned.
+	 * If this instance is not a working copy, a new runtime working copy
+	 * is created with the same id and attributes.
+	 * Clients are responsible for saving or releasing the working copy when
+	 * they are done with it.
+	 * <p>
+	 * The runtime working copy is related to this runtime instance
+	 * in the following ways:
+	 * <pre>
+	 * this.getWorkingCopy().getId() == this.getId()
+	 * this.getWorkingCopy().getOriginal() == this
+	 * </pre>
+	 * </p>
+	 * <p>
+	 * [issue: IRuntimeWorkingCopy extends IRuntime. 
+	 * Runtime.getWorkingCopy() create a new working copy;
+	 * RuntimeWorkingCopy.getWorkingCopy() returns this.
+	 * This may be convenient in code that is ignorant of
+	 * whether they are dealing with a working copy or not.
+	 * However, it is hard for clients to manage working copies
+	 * with this design.
+	 * This method should be renamed "createWorkingCopy"
+	 * or "newWorkingCopy" to make it clear to clients that it
+	 * creates a new object, even for working copies.]
+	 * </p>
+	 * 
+	 * @return a new working copy
+	 */
 	public IRuntimeWorkingCopy getWorkingCopy();
 	
+	/**
+	 * Returns the location of this runtime.
+	 * <p>
+	 * [issue: Explain what this "location" is.]
+	 * </p>
+	 * 
+	 * @return the location of this runtime, or <code>null</code> if none
+	 */
 	public IPath getLocation();
 	
+	/**
+	 * Returns whether this runtime can be used as a test environment.
+	 * <p>
+	 * [issue: How does one explain what a "test environment" is?
+	 * How does this property of runtime square with 
+	 * IServerType.isTestEnvironment(), a *type-generic*
+	 * property of a server type?]
+	 * </p>
+	 * 
+	 * @return <code>true</code> if this runtime can be use as a
+	 * test environment, and <code>false</code> if it cannot
+	 */
 	public boolean isTestEnvironment();
 	
 	/**
-	 * Return the validation status of the runtime.
-	 * 
-	 * @return
+	 * Validates this runtime instance.
+	 * <p>
+	 * [issue: Need to explain what could be wrong with a runtime.]
+	 * </p>
+	 * <p>
+	 * [issue: Would it make more sense to validate working copies
+	 * before prior to save?]
+	 * </p>
+	 *
+	 * @return a status object with code <code>IStatus.OK</code> if this
+	 * runtime is valid, otherwise a status object indicating what is
+	 * wrong with it
 	 */
 	public IStatus validate();
 }
\ No newline at end of file
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IRuntimeType.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IRuntimeType.java
index fefce7e8e..c281d4545 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IRuntimeType.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IRuntimeType.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -13,40 +13,170 @@ package org.eclipse.wst.server.core;
 import java.util.List;
 
 import org.eclipse.core.runtime.CoreException;
+import org.eclipse.core.runtime.IProgressMonitor;
 /**
+ * Represents a (server) runtime type from which runtime instances can be
+ * created.
+ * <p>
+ * The server core framework supports
+ * an open-ended set of runtime types, which are contributed via
+ * the <code>runtimeTypes</code> extension point in the server core
+ * plug-in. Runtime type objects carry no state (all information is
+ * read-only and is supplied by the server runtime type declaration).
+ * The global list of known runtime types is available via
+ * {@link ServerCore#getRuntimeTypes()}.
+ * </p>
+ * <p>
+ * This interface is not intended to be implemented by clients.
+ * </p>
+ * <p>
+ * [issue: The term "runtime" is misleading, given that the main
+ * reason is for build time classpath contributions, not for actually
+ * running anything. "libraries" might be a better choice.]
+ * </p>
+ * <p>
+ * [issue: What value do runtimes add?
+ * It's main role is for setting up the Java build classpath
+ * for projects holding modules that must be Java compiled.
+ * If the notion of module is to transcend the vagaries of particular
+ * types of server, and, indeed, be published to multiple servers
+ * simultaneously, then matters of build classpath had better not
+ * be tied to the particular servers involved.]
+ * </p>
+ * <p>
+ * [issue: It is notoriously difficult to place any kind of
+ * useful order on objects that are contributed independently by
+ * non-collaborating parties. The IOrdered mechanism is weak, and
+ * can't really solve the problem. Issues of presentation are usually
+ * best left to the UI, which can sort objects based on arbitrary
+ * properties.]
+ * </p>
+ * <p>
+ * [issue: Equality/identify for runtime types? Are IRuntimeType
+ * instances guaranteed to be canonical (client can use ==),
+ * or is it possible for there to be non-identical IRuntimeType
+ * objects in play that both represent the same runtime type?
+ * The latter is the more common; type should spec equals.]
+ * </p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
  * 
- * <p>This interface is not intended to be implemented by clients.</p>
+ * @since 1.0
  */
 public interface IRuntimeType extends IOrdered {
+
 	/**
+	 * Returns the id of this runtime type.
+	 * Each known server runtime type has a distinct id. 
+	 * Ids are intended to be used internally as keys; they are not
+	 * intended to be shown to end users.
 	 * 
-	 * @return
+	 * @return the runtime type id
 	 */
 	public String getId();
 
 	/**
-	 * 
-	 * @return
+	 * Returns the displayable name for this runtime type.
+	 * <p>
+	 * Note that this name is appropriate for the current locale.
+	 * </p>
+	 *
+	 * @return a displayable name for this runtime type
 	 */
 	public String getName();
 
 	/**
-	 * 
-	 * @return
+	 * Returns the displayable description for this runtime type.
+	 * <p>
+	 * Note that this description is appropriate for the current locale.
+	 * </p>
+	 *
+	 * @return a displayable description for this runtime type
 	 */
 	public String getDescription();
 	
+	/**
+	 * Returns the displayable vendor name for this runtime type.
+	 * <p>
+	 * Note that this description is appropriate for the current locale.
+	 * </p>
+	 * <p>
+	 * [issue: "vendor" attribute is optional. What is expected return
+	 * when omitted? Should be empty string.]
+	 * </p>
+	 *
+	 * @return a displayable vendor name for this runtime type
+	 */
 	public String getVendor();
 	
+	/**
+	 * Returns the displayable version name for this runtime type.
+	 * <p>
+	 * Note that this description is appropriate for the current locale.
+	 * </p>
+	 * <p>
+	 * [issue: "versionId" attribute is optional. What is expected return
+	 * when omitted? Should be empty string.]
+	 * </p>
+	 * </p>
+	 *
+	 * @return a displayable version name for this runtime type
+	 */
 	public String getVersion();
 
 	/**
+	 * Returns the list of module types that this runtime type 
+	 * can support.
+	 * <p>
+	 * [issue: The list returned is precious. You would not want a client
+	 * to accidentally or malicously whack it. Normal practice is to
+	 * return an array instead of a List, and to return a new copy each call.
+	 * This allows the spec to say that the client can do what they want
+	 * with the result, and that it won't change under foot.
+	 * Another alternative is to return a UnmodifiableList implementation
+	 * so that clients cannot modify. But if you don't copy, you still
+	 * have the problem of the list chaning under foot if a new plug-in
+	 * is installed that happens to define a module kind (a scenario that
+	 * Eclipse should support).]
+	 * </p>
 	 * 
-	 * @return
+	 * @return the list of module types (element type: {@link IModuleType})
 	 */
 	public List getModuleTypes();
 	
+	/**
+	 * Returns whether this runtime type can be instantiated.
+	 * <p>
+	 * [issue: It's unclear what this method is for.
+	 * The implementation checks whether the "class"
+	 * and "workingCopyClass" attributes (both optional) were specified.
+	 * What would be the point of a runtime type that didn't
+	 * have both of these attributes and could not be "created"?]
+	 * </p>
+	 * 
+	 * @return <code>true</code> if this type of runtime can be
+	 * instantiated, and <code>false</code> if it cannot
+	 * @see #createRuntime(String)
+	 */
 	public boolean canCreate();
 
+	/**
+	 * Creates a working copy instance of this runtime type.
+	 * After setting various properties of the working copy,
+	 * the client should call {@link IRuntimeWorkingCopy#save(IProgressMonitor)}
+	 * to bring the runtime instance into existence.
+	 * <p>
+	 * [issue: This method is declared as throwing CoreException.
+	 * From a clients's point of view, what are the circumstances that
+	 * cause this operation to fail?]
+	 * </p>
+	 * 
+	 * @param id the id to assign to the runtime instance; a generated
+	 * id is used if id is <code>null</code> or an empty string
+	 * @return a new runtime working copy with the given id
+	 * @throws CoreException [missing]
+	 */
 	public IRuntimeWorkingCopy createRuntime(String id) throws CoreException;
 }
\ No newline at end of file
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IRuntimeWorkingCopy.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IRuntimeWorkingCopy.java
index b3e299bdc..7a0f8db68 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IRuntimeWorkingCopy.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IRuntimeWorkingCopy.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -15,17 +15,137 @@ import org.eclipse.core.runtime.IPath;
 import org.eclipse.core.runtime.IProgressMonitor;
 import org.eclipse.wst.server.core.model.IRuntimeWorkingCopyDelegate;
 /**
+ * A working copy runtime object used for formulating changes
+ * to a runtime instance ({@link IRuntime}).
+ * <p>
+ * [issue: The default value of location and test environment
+ * should be specified here (or in IServerType.createRuntime).
+ * If the initial value is unsuitable for actual use, then
+ * save needs to deal with the case where the client forgets
+ * to initialize this property.]
+ * </p>
+ * <p>
+ * [issue: There can be other runtime-type-specific properties.
+ * The default values for these need to be specified somewhere
+ * too (probably in the API subclass of IRuntimeWorkingCopyDelegate).]
+ * </p>
+ * <p>
+ * [issue: IElementWorkingCopy and IElement support an open-ended set
+ * of attribute-value pairs. What is relationship between these
+ * attributes and (a) the get/setXXX methods found on this interface,
+ * and (b) get/setXXX methods provided by specific server types?
+ * Is it the case that these attribute-values pairs are the only
+ * information about a runtime instance that can be preserved
+ * between workbench sessions? That is, any information recorded
+ * just in instance fields of an IRuntimeDelegate implementation
+ * will be lost when the session ends.]
+ * </p>
+ * <p>
+ * This interface is not intended to be implemented by clients.
+ * </p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
  * 
- * <p>This interface is not intended to be implemented by clients.</p>
+ * @since 1.0
  */
 public interface IRuntimeWorkingCopy extends IRuntime, IElementWorkingCopy {	
+
+	/**
+	 * Returns the runtime instance that this working copy is
+	 * associated with.
+	 * <p>
+	 * For a runtime working copy created by a call to
+	 * {@link IRuntime#getWorkingCopy()},
+	 * <code>this.getOriginal()</code> returns the original
+	 * runtime object. For a runtime working copy just created by
+	 * a call to {@link IRuntimeType#createRuntime(String)},
+	 * <code>this.getOriginal()</code> returns <code>null</code>.
+	 * </p>
+	 * 
+	 * @return the associated runtime instance, or <code>null</code> if none
+	 */
 	public IRuntime getOriginal();
 	
+	/**
+	 * Returns the delegate for this runtime working copy.
+	 * The runtime working copy delegate is a
+	 * runtime-type-specific object. By casting the runtime working copy
+	 * delegate to the type prescribed in the API documentation for that
+	 * particular runtime working copy type, the client can access
+	 * runtime-type-specific properties and methods.
+	 * <p>
+	 * [issue: Exposing IRuntimeWorkingCopyDelegate to clients
+	 * of IRuntimeWorkingCopy is same problem as exposing
+	 * IRuntimeDelegate to clients of IRuntime. The suggested fix 
+	 * is to replace this method with something like
+	 * getRuntimeWorkingCopyExtension() which
+	 * returns an IRuntimeWorkingCopyExtension.]
+	 * </p>
+	 * <p>
+	 * [issue: runtimeTypes schema, workingCopyClass attribute is optional.
+	 * This suggests that a runtime need not provide a working copy
+	 * delegate class. Like the class attribute, this seems implausible.
+	 * I've spec'd this method as if working copy delegate is mandatory.]
+	 * </p>
+	 * 
+	 * @return the delegate for the runtime working copy
+	 */
 	public IRuntimeWorkingCopyDelegate getWorkingCopyDelegate();
 	
+	/**
+	 * Sets the location of this runtime.
+	 * <p>
+	 * [issue: Explain what this "location" is.]
+	 * </p>
+	 * 
+	 * @param path the location of this runtime, or <code>null</code> if none
+	 * @see IRuntime#getLocation()
+	 */
 	public void setLocation(IPath path);
 	
+	/**
+	 * Commits the changes made in this working copy. If there is
+	 * no extant runtime instance with a matching id and runtime
+	 * type, this will create a runtime instance with attributes
+	 * taken from this working copy. If there an existing runtime
+	 * instance with a matching id and runtime type, this will
+	 * change the runtime instance accordingly.
+	 * <p>
+	 * [issue: What is relationship to 
+	 * this.getOriginal() and the IRuntime returned by this.save()?
+	 * The answer should be: they're the same runtime, for an
+	 * appropriate notion of "same". As currently implemented, they
+	 * are different IRuntime instances but have the same runtime
+	 * id and same runtime types. Client that are hanging on to
+	 * the old runtime instance will not see the changes. 
+	 * If IRuntime were some kind of handle object as elsewhere in 
+	 * Eclipse Platform, this kind of change could be done much
+	 * more smoothly.]
+	 * </p>
+	 * <p>
+	 * [issue: What if this object has already been saved
+	 * or released?]
+	 * </p>
+	 * <p>
+	 * [issue: What is lifecycle for IRuntimeWorkingCopyDelegate
+	 * associated with this working copy?]
+	 * </p>
+	 * 
+	 * @param monitor a progress monitor, or <code>null</code> if progress
+	 *    reporting and cancellation are not desired
+	 * @return a new runtime instance
+	 * @throws CoreException [missing]
+	 */
 	public IRuntime save(IProgressMonitor monitor) throws CoreException;
 	
+	/**
+	 * Sets whether this runtime can be used as a test environment.
+	 * 
+	 * @param b <code>true</code> if this runtime can be use as a
+	 * test environment, and <code>false</code> if it cannot
+	 * @see IRuntime#isTestEnvironment()
+	 */
 	public void setTestEnvironment(boolean b);
 }
\ No newline at end of file
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServer.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServer.java
index 28240f98c..b7ab5e71e 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServer.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServer.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -20,63 +20,264 @@ import org.eclipse.core.runtime.IStatus;
 import org.eclipse.debug.core.ILaunch;
 import org.eclipse.debug.core.ILaunchConfiguration;
 import org.eclipse.debug.core.ILaunchConfigurationWorkingCopy;
-import org.eclipse.wst.server.core.model.*;
+import org.eclipse.wst.server.core.model.IModule;
+import org.eclipse.wst.server.core.model.IModuleEvent;
+import org.eclipse.wst.server.core.model.IModuleFactoryEvent;
+import org.eclipse.wst.server.core.model.IPublishListener;
+import org.eclipse.wst.server.core.model.IPublisher;
+import org.eclipse.wst.server.core.model.IServerDelegate;
+import org.eclipse.wst.server.core.model.IServerListener;
 /**
+ * Represents a server instance. Every server is an instance of a
+ * particular, fixed server type.
+ * <p>
+ * Not surprisingly, the notion of <b>server</b> is central in the web tools
+ * server infrastructure. In this context, understand that a server is
+ * a web server of some ilk. It could be a simple web server lacking Java
+ * support, or an J2EE based server, or perhaps even some kind of database
+ * server. A more exact definition is not required for the purposes of this API.
+ * From a tool-centric point of view, a server
+ * is something that the developer is writing "content" for.
+ * The unit of content is termed a module.
+ * In a sense, the server exists, but lacks useful content. The
+ * development task is to provide that content. The content can include
+ * anything from simple, static HTML web pages to complex, highly dynamic
+ * web applications.
+ * In the course of writing and debugging this content,
+ * the developer will want to test their content on a web server, to see how it
+ * gets served up. For this they will need to launch a server process running on
+ * some host machine (often the local host on which the IDE is running), or
+ * attach to a server that's already running on a remote (or local) host. 
+ * The newly developed content sitting in the developer's workspace needs to
+ * end up in a location and format that the running server can use for its
+ * serving purposes.
+ * </p>
+ * <p>
+ * In this picture, an <code>IServer</code> object is a proxy for the real web
+ * server. Through this proxy, a client can configure the server, and start,
+ * stop, and restart it.
+ * </p>
+ * <p>
+ * The resource manager maintains a global list of all known server instances
+ * ({@link IResourceManager#getServers()}).
+ * </p>
+ * <p>
+ * [rough notes:
+ * Server has a state.
+ * Server can be started, stopped, and restarted.
+ * To modify server attributes, get a working copy, modify it, and then save it
+ * to commit the changes.
+ * Server attributes. Serialization.
+ * Chained working copies for runtime, server configuration.
+ * Server has a set of root modules.
+ * Modules have state wrt a server.
+ * Restarting modules.
+ * ]
+ * </p>
+ * <p>
+ * [issue: The information actually stored in the (.server) file is:
+ * server id and name, server type id, runtime id, server configuration id,
+ * and test-environment. It's unclear what's gained by storing this
+ * information in a workspace file. Is it so that this information
+ * can be shared between users via a repository? Or is it just so that
+ * there would be something to open in the resource navigator view?]
+ * </p>
+ * <p>
+ * [issue: Equality/identify for servers?]
+ * </p>
  * 
  * <p>This interface is not intended to be implemented by clients.</p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
+ * 
+ * @since 1.0
  */
 public interface IServer extends IElement {
+	
+	/**
+	 * File extension (value "server") for serialized representation of
+	 * server instances.
+	 * <p>
+	 * [issue: What is relationship between this file extension and
+	 * the file passed to IServerType.create(...) or returned by
+	 * IServer.getFile()? That is, are server files expected to end
+	 * in ".server", or is this just a default? If the former
+	 * (as I suspect), then IServerType.create needs to say so,
+	 * and the implementation should enforce the restriction.]
+	 * </p>
+	 */
 	public static final String FILE_EXTENSION = "server";
 	
-	// attribute for launch configurations
+	/**
+	 * Server id attribute (value "server-id") of launch configurations.
+	 * This attribute is used to tag a launch configuration with the
+	 * id of the corresponding server.
+	 * <p>
+	 * [issue: This feels like an implementation detail. If it is to
+	 * remain API, need to explain how a client uses this attribute.]
+	 * </p>
+	 * @see ILaunchConfiguration
+	 */
 	public static final String ATTR_SERVER_ID = "server-id";
 
-	// --- Server State Constants ---
-	// (returned from getServerState() method)
-
-	// the server state is unknown
+	/**
+	 * Server state constant (value 0) indicating that the
+	 * server is in an unknown state.
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 * 
+	 * @see #getServerState()
+	 */
 	public static final byte SERVER_UNKNOWN = 0;
 
-	// the server is starting, but not yet ready to serve content
+	/**
+	 * Server state constant (value 1) indicating that the
+	 * server is starting, but not yet ready to serve content.
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 * 
+	 * @see #getServerState()
+	 */
 	public static final byte SERVER_STARTING = 1;
 
-	// the server is ready to serve content
+	/**
+	 * Server state constant (value 2) indicating that the
+	 * server is ready to serve content.
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 * 
+	 * @see #getServerState()
+	 */
 	public static final byte SERVER_STARTED = 2;
 
-	// the server is started in debug mode and ready
-	// to serve content
+	/**
+	 * Server state constant (value 3) indicating that the
+	 * server is started in debug mode and is ready to serve content.
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 * <p>
+	 * [issue: SERVER_STARTED_DEBUG and SERVER_STARTED_PROFILE
+	 * could be folded into SERVER_STARTED is there were 
+	 * IServer.getMode() for querying what mode the server is running in.]
+	 * </p>
+	 * 
+	 * @see #getServerState()
+	 */
 	public static final byte SERVER_STARTED_DEBUG = 3;
 	
-	// the server is started in profiling mode and ready
-	// to serve content
+	/**
+	 * Server state constant (value 4) indicating that the
+	 * server is started in profiling mode and is ready to serve content.
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 * <p>
+	 * [issue: SERVER_STARTED_DEBUG and SERVER_STARTED_PROFILE
+	 * could be folded into SERVER_STARTED is there were 
+	 * IServer.getMode() for querying what mode the server is running in.]
+	 * </p>
+	 * 
+	 * @see #getServerState()
+	 */
 	public static final byte SERVER_STARTED_PROFILE = 4;
 
-	// the server is shutting down
+	/**
+	 * Server state constant (value 5) indicating that the
+	 * server is shutting down.
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 * 
+	 * @see #getServerState()
+	 */
 	public static final byte SERVER_STOPPING = 5;
 
-	// the server is stopped
+	/**
+	 * Server state constant (value 6) indicating that the
+	 * server is stopped.
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 * 
+	 * @see #getServerState()
+	 */
 	public static final byte SERVER_STOPPED = 6;
 
-	// getting the server state is unsupported
+	/**
+	 * Server state constant (value 7) indicating that the
+	 * server does not support getting the server state.
+	 * <p>
+	 * [issue: Given SERVER_UNKNOWN, is this state really
+	 * necessary?]
+	 * </p>
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 * 
+	 * @see #getServerState()
+	 */
 	public static final byte SERVER_UNSUPPORTED = 7;
 
 
-	// --- Module State Constants ---
-	// (returned from getModuleState() method)
-
-	// the module state is unknown
+	/**
+	 * Module state constant (value 0) indicating that the
+	 * module is in an unknown state.
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 * 
+	 * @see #getModuleState(IModule)
+	 */
 	public static final byte MODULE_STATE_UNKNOWN = 0;
 
-	// the module is starting up
+	/**
+	 * Module state constant (value 1) indicating that the
+	 * module is starting up, but not yet ready to serve its content.
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 * 
+	 * @see #getModuleState(IModule)
+	 */
 	public static final byte MODULE_STATE_STARTING = 1;
 
-	// the module is ready to serve content
+	/**
+	 * Module state constant (value 2) indicating that the
+	 * module is ready to serve its content.
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 * 
+	 * @see #getModuleState(IModule)
+	 */
 	public static final byte MODULE_STATE_STARTED = 2;
 
-	// the module is shutting down
+	/**
+	 * Module state constant (value 3) indicating that the
+	 * module is shutting down.
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 * 
+	 * @see #getModuleState(IModule)
+	 */
 	public static final byte MODULE_STATE_STOPPING = 3;
 
-	// the module is stopped
+	/**
+	 * Module state constant (value 4) indicating that the
+	 * module is stopped.
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 * 
+	 * @see #getModuleState(IModule)
+	 */
 	public static final byte MODULE_STATE_STOPPED = 4;
 
 
@@ -93,35 +294,180 @@ public interface IServer extends IElement {
 	public static final byte SYNC_STATE_DIRTY = 2;
 	
 	/**
-	 * Returns the current state of the server. (see SERVER_XXX
-	 * constants)
+	 * Returns the current state of this server.
+	 * <p>
+	 * Note that this operation is guaranteed to be fast
+	 * (it does not actually communicate with any actual
+	 * server).
+	 * </p>
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
 	 *
-	 * @return byte
+	 * @return one of the server state (<code>SERVER_XXX</code>)
+	 * constants declared on {@link IServer}
 	 */
 	public byte getServerState();
 	
+	/**
+	 * Returns the host for the running server.
+	 * The format of the host conforms to RFC 2732.
+	 * <p>
+	 * [issue: Consider renaming to "getHost" to bring in line
+	 * with terminology used in java.net.URL. The host name can be
+	 * either a host name or octets.]
+	 * </p>
+	 * 
+	 * @return a host string conforming to RFC 2732
+	 * @see java.net.URL.getHost()
+	 */
 	public String getHostname();
 	
+	/**
+	 * Returns the file where this server instance is serialized.
+	 * 
+	 * @return the file in the workspace where the server instance
+	 * is serialized, or <code>null</code> if the information is
+	 * instead to be persisted with the workspace but not with any
+	 * particular workspace resource
+	 */
 	public IFile getFile();
 	
-	//public boolean hasRuntime();
-	
+	/**
+	 * Returns the runtime associated with this server.
+	 * <p>
+	 * Note: The runtime of a server working copy may or may not
+	 * be a working copy. For a server instance that is not a
+	 * working copy, the runtime instance is not a working copy
+	 * either.
+	 * </p>
+	 * <p>
+	 * [issue: According to serverType extension point, 
+	 * runtimeTypeId is a mandatory attribute. It seems odd
+	 * then to have server runtime instance being an
+	 * optional property of server instance. What does it mean
+	 * for a server to not have a runtime?]
+	 * </p>
+	 * 
+	 * @return the runtime, or <code>null</code> if none
+	 */
 	public IRuntime getRuntime();
 	
+	/**
+	 * Returns the type of this server.
+	 * 
+	 * @return the server type
+	 */
 	public IServerType getServerType();
 	
+	/**
+	 * Returns the server configuration associated with this server.
+	 * <p>
+	 * Note: The server configuration of a server working copy may
+	 * or may not be a working copy. For a server instance that is
+	 * not a working copy, the server configuration instance is not
+	 * a working copy either.
+	 * </p>
+	 * <p>
+	 * [issue: According to serverType extension point, 
+	 * configurationTypeId is an optional attribute. If a server type
+	 * has no server configuration type, then it seems reasonable to 
+	 * expect this method to return null for all instances of that server
+	 * type. But what about a server type that explicitly specifies
+	 * a server configuration type. Does that mean that all server
+	 * instances of that server type must have a server configuration
+	 * instance of that server configuration type, and that this method
+	 * never returns null in those cases?]
+	 * </p>
+	 * 
+	 * @return the server configuration, or <code>null</code> if none
+	 */
 	public IServerConfiguration getServerConfiguration();
 	
+	/**
+	 * Returns the server delegate for this server.
+	 * The server delegate is a server-type-specific object.
+	 * By casting the server delegate to the type prescribed in
+	 * the API documentation for that particular server type, 
+	 * the client can access server-type-specific properties and
+	 * methods.
+	 * <p>
+	 * [issue: Exposing IServerDelegate to clients of IServer
+	 * is confusing and dangerous. Instead, replace this
+	 * method with something like getServerExtension() which
+	 * returns an IServerExtension. IServerExtension is an
+	 * "marker" interface that server providers would 
+	 * implement or extend if they want to expose additional
+	 * API for their server type. That way IServerDelegate
+	 * can be kept entirely on the SPI side, out of view from 
+	 * clients.]
+	 * </p>
+	 * <p>
+	 * [issue: serverTypes schema, class attribute is optional.
+	 * This suggests that a server need not provide a delegate class.
+	 * This seems implausible. I've spec'd this method 
+	 * as if delegate is mandatory.]
+	 * </p>
+	 * 
+	 * @return the server delegate
+	 */
 	public IServerDelegate getDelegate();
 	
+	/**
+	 * Returns a server working copy for modifying this server instance.
+	 * If this instance is already a working copy, it is returned.
+	 * If this instance is not a working copy, a new server working copy
+	 * is created with the same id and attributes.
+	 * Clients are responsible for saving or releasing the working copy when
+	 * they are done with it.
+	 * <p>
+	 * The server working copy is related to this server instance
+	 * in the following ways:
+	 * <pre>
+	 * this.getWorkingCopy().getId() == this.getId()
+	 * this.getWorkingCopy().getFile() == this.getFile()
+	 * this.getWorkingCopy().getOriginal() == this
+	 * this.getWorkingCopy().getRuntime() == this.getRuntime()
+	 * this.getWorkingCopy().getServerConfiguration() == this.getServerConfiguration()
+	 * </pre>
+	 * </p>
+	 * <p>
+	 * [issue: IServerWorkingCopy extends IServer. 
+	 * Server.getWorkingCopy() create a new working copy;
+	 * ServerWorkingCopy.getWorkingCopy() returns this.
+	 * This may be convenient in code that is ignorant of
+	 * whether they are dealing with a working copy or not.
+	 * However, it is hard for clients to manage working copies
+	 * with this design.
+	 * This method should be renamed "createWorkingCopy"
+	 * or "newWorkingCopy" to make it clear to clients that it
+	 * creates a new object, even for working copies.]
+	 * </p>
+	 * 
+	 * @return a new working copy
+	 */
 	public IServerWorkingCopy getWorkingCopy();
 	
 	/**
+	 * Returns whether the given server configuration can be used with
+	 * this server.
+	 * <p>
+	 * [issue: This seems to be just a convenience method. Given that it's 
+	 * straightforward enought for a client to compare 
+	 * this.getServerType().getServerConfiguration()
+	 * to configuration.getServerConfigurationType(),
+	 * it's not clear that there is a great need for this method.]
+	 * </p>
+	 * <p>
+	 * [issue: It does not make sense to allow a null configuration.]
+	 * </p>
+	 * 
 	 * Returns true if this is a configuration that is
 	 * applicable to (can be used with) this server.
 	 *
-	 * @param configuration org.eclipse.wst.server.core.model.IServerConfiguration
-	 * @return boolean
+	 * @param configuration the server configuration
+	 * @return <code>true</code> if this server supports the given server
+	 * configuration, and <code>false/code> otherwise
 	 */
 	public boolean isSupportedConfiguration(IServerConfiguration configuration);
 
@@ -142,16 +488,25 @@ public interface IServer extends IElement {
 	public List getUnpublishedModules();
 
 	/**
-	 * Add a listener to this server.
+	 * Adds the given server state listener to this server.
+	 * Once registered, a listener starts receiving notification of 
+	 * state changes to this server. The listener continues to receive
+	 * notifications until it is removed.
+	 * <p>
+	 * [issue: Duplicate server listeners should be ignored.]
+	 * </p>
 	 *
-	 * @param listener org.eclipse.wst.server.model.IServerListener
+	 * @param listener the server listener
+	 * @see #removeServerListener(IServerListener)
 	 */
 	public void addServerListener(IServerListener listener);
 	
 	/**
-	 * Remove a listener from this server.
-	 *
-	 * @param listener org.eclipse.wst.server.model.IServerListener
+	 * Removes the given server state listener from this server. Has no
+	 * effect if the listener is not registered.
+	 * 
+	 * @param listener the listener
+	 * @see #addServerListener(IServerListener)
 	 */
 	public void removeServerListener(IServerListener listener);
 
@@ -170,10 +525,11 @@ public interface IServer extends IElement {
 	public void removePublishListener(IPublishListener listener);
 	
 	/**
-	 * Returns true if the server is in a state that it can
+	 * Returns whether this server is in a state that it can
 	 * be published to.
 	 *
-	 * @return boolean
+	 * @return <code>true</code> if this server can be published to,
+	 * and <code>false</code> otherwise
 	 */
 	public boolean canPublish();
 	
@@ -187,27 +543,71 @@ public interface IServer extends IElement {
 	
 	/**
 	 * Returns the publisher that can be used to publish the
-	 * given module. If the module should never
-	 * be published to the server, it may return null.
+	 * given module to this server.
+	 * <p>
+	 * [issue: It is unclear why the IPublisher is being exposed
+	 * here at all. A normal client would call
+	 * IServer.publish to instigate a publish operation; they would
+	 * not be involved with anything finer-grained. Even the package
+	 * suggests that IPublisher is something that should only be 
+	 * relevant on the SPI side.]
+	 * </p>
+	 * <p>
+	 * [issue: Explain the role of the parents parameter.]
+	 * </p>
 	 *
-	 * @param parents java.util.List
-	 * @param module org.eclipse.wst.server.core.model.IModule
-	 * @return org.eclipse.wst.server.core.model.IPublisher
+	 * @param parents the parent modules (element type: <code>IModule</code>)
+	 * @param module the module
+	 * @return the publisher that handles the given module, or
+	 * <code>null</code> if the module cannot be published to
+	 * this server
 	 */
 	public IPublisher getPublisher(List parents, IModule module);
 
+	/**
+	 * Publishes all associated modules to this server using
+	 * the default publish manager.
+	 * <p>
+	 * [issue: The Server implementation of this method currently reads
+	 * ServerCore.getPublishManager(ServerPreferences.DEFAULT_PUBLISH_MANAGER).
+	 * This means that there is a fixed default (the smart publish manager).
+	 * I think it should instead read
+	 * ServerCore.getPublishManager(ServerPreferences.getDefaultPublishManager())
+	 * which allows the default publish manager to be configured via 
+	 * a preference.]
+	 * </p>
+	 * 
+	 * @param monitor a progress monitor, or <code>null</code> if progress
+	 *    reporting and cancellation are not desired
+	 * @return status indicating what (if anything) went wrong
+	 */
 	public IStatus publish(IProgressMonitor monitor);
 	
+	/**
+	 * Publishes all associated modules to this server using
+	 * the given publish manager.
+	 * 
+	 * @param publishManager the publish manager that is to coordinate
+	 * this publishing operation
+	 * @param monitor a progress monitor, or <code>null</code> if progress
+	 *    reporting and cancellation are not desired
+	 * @return status indicating what (if anything) went wrong
+	 */
 	public IStatus publish(IPublishManager publishManager, IProgressMonitor monitor);
 	
 	/**
-	 * Returns true if the server is in a state that it can
-	 * be started, and supports the given mode.
+	 * Returns whether this server is in a state that it can
+	 * be started in the given mode.
 	 *
-	 * @param mode
-	 * @return boolean
+	 * @param launchMode a mode in which a server can be launched,
+	 * one of the mode constants defined by
+	 * {@link org.eclipse.debug.core.ILaunchManager}
+	 * @return <code>true</code> if this server can be started
+	 * in the given mode, and <code>false</code> if it is either
+	 * not ready to be started or if it does not support the given
+	 * mode
 	 */
-	public boolean canStart(String mode);
+	public boolean canStart(String launchMode);
 	
 	public ILaunch getExistingLaunch();
 	
@@ -223,81 +623,189 @@ public interface IServer extends IElement {
 
 	public void setLaunchDefaults(ILaunchConfigurationWorkingCopy workingCopy);
 
-	public ILaunch start(String mode, IProgressMonitor monitor) throws CoreException;
+	/**
+	 * Asynchronously starts this server in the given launch mode.
+	 * Returns the debug launch object that can be used in a debug
+	 * session.
+	 * <p>
+	 * [issue: This method should specify what it does if
+	 * canStart(launchMode) returns false.]
+	 * </p>
+	 * <p>
+	 * [issue: There is no way to communicate failure to the
+	 * client for the async portion of this operation. Given that
+	 * this operation can go awry, there probably should be a mechanism
+	 * that allows failing asynch operations to be diagnosed.]
+	 * </p>
+	 *
+	 * @param launchMode a mode in which a server can be launched,
+	 * one of the mode constants defined by
+	 * {@link org.eclipse.debug.core.ILaunchManager}
+	 * @param monitor a progress monitor, or <code>null</code> if progress
+	 *    reporting and cancellation are not desired
+	 * @return a debug launch object
+	 * @exception CoreException if an error occurs while trying to start the server
+	 */
+	public ILaunch start(String launchMode, IProgressMonitor monitor) throws CoreException;
 	
 	/**
-	 * Start the server in the given start mode and waits until the server
-	 * has finished started.
+	 * Starts this server in the given launch mode and waits until the server
+	 * has finished starting.
+	 * <p>
+	 * This convenience method uses {@link #start(String, IProgressMonitor)}
+	 * to start the server, and an internal thread and listener to detect
+	 * when the server has finished starting.
+	 * </p>
+	 * <p>
+	 * [issue: Is there are particular reason why this method
+	 * does not return the ILaunch that was used?]
+	 * </p>
 	 *
-	 * @param mode java.lang.String
-	 * @param monitor org.eclipse.core.runtime.IProgressMonitor
-	 * @exception CoreException - thrown if an error occurs while trying to start the server
+	 * @param launchMode a mode in which a server can be launched,
+	 * one of the mode constants defined by
+	 * {@link org.eclipse.debug.core.ILaunchManager}
+	 * @param monitor a progress monitor, or <code>null</code> if progress
+	 *    reporting and cancellation are not desired
+	 * @exception CoreException if an error occurs while trying to start the server
 	 */
-	public void synchronousStart(String mode, IProgressMonitor monitor) throws CoreException;
+	public void synchronousStart(String launchMode, IProgressMonitor monitor) throws CoreException;
 	
 	/**
-	 * Returns true if the server is in a state that it can
-	 * be restarted.
+	 * Returns whether this server is in a state that it can
+	 * be restarted in the given mode. Note that only servers
+	 * that are currently running can be restarted.
 	 *
-	 * @return boolean
+	 * @param launchMode a mode in which a server can be launched,
+	 * one of the mode constants defined by
+	 * {@link org.eclipse.debug.core.ILaunchManager}
+	 * @return <code>true</code> if this server can be restarted
+	 * in the given mode, and <code>false</code> if it is either
+	 * not ready to be restarted or if it does not support the given
+	 * mode
 	 */
 	public boolean canRestart(String mode);
 	
 	/**
-	 * Returns true if the server is not in sync and needs to be
-	 * restarted. Returns false if the server should not be restarted.
-	 * (e.g. if the contents have not been modified and the server
-	 * process is still in sync)
-	 * Result is undefined if the server is not running.
-	 *
-	 * @return boolean
+	 * Returns whether this server is out of sync and needs to be
+	 * restarted.
+	 * <p>
+	 * [issue: Need to explain what is it that can get out of
+	 * "out of sync" here, and how this can happen.]
+	 * </p>
+	 * <p>
+	 * [issue: Rather than have an unspecified result when the
+	 * server is not running, this method should be spec'd to
+	 * return false whenever canRestart() returns false.]
+	 * </p>
+	 * 
+	 * @return <code>true</code> if this server is out of sync and needs to be
+	 * restarted, and <code>false</code> otherwise (e.g., if the contents have
+	 * not been modified and the server process is still in sync); the
+	 * result is unspecified if the server is not currently running
 	 */
 	public boolean isRestartNeeded();
 
 	/**
-	 * Restart the server with the given debug mode.
-	 * A server may only be restarted when it is currently running.
-	 * This method is asynchronous.
+	 * Asynchronously restarts this server. This operation does
+	 * nothing if this server cannot be stopped ({@link #canRestart()}
+	 * returns <code>false</code>.
+	 * <p>
+	 * [issue: Lack of symmetry. Why is there no synchronousRestart? start and
+	 * stop both have synchronous equivalents.]
+	 * </p>
+	 * <p>
+	 * [issue: There is no way to communicate failure to the
+	 * client. Given that this operation can go awry, there probably
+	 * should be a mechanism that allows failing asynch operations
+	 * to be diagnosed.]
+	 * </p>
+	 *
+	 * @param launchMode a mode in which a server can be launched,
+	 * one of the mode constants defined by
+	 * {@link org.eclipse.debug.core.ILaunchManager}
 	 */
 	public void restart(String mode);
 
 	/**
-	 * Returns true if the server is in a state that it can
+	 * Returns whether this server is in a state that it can
 	 * be stopped.
+	 * <p>
+	 * [issue: Are there servers (or server types) that cannot be
+	 * stopped? For instance, a server running on a remote host that 
+	 * can be attached to, a published to, but neither started or
+	 * stopped via this API. Or are we only talking about whether
+	 * it is inconvenient to stop at this time?]
+	 * </p>
 	 *
-	 * @return boolean
+	 * @return <code>true</code> if this server can be stopped,
+	 * and <code>false</code> otherwise
 	 */
 	public boolean canStop();
 
 	/**
-	 * Stop the server if it is running.
+	 * Asynchronously stops this server. This operation does
+	 * nothing if this server cannot be stopped ({@link #canStop()}
+	 * returns <code>false</code>.
+	 * <p>
+	 * [issue: There is no way to communicate failure to the
+	 * client. Given that this operation can go awry, there probably
+	 * should be a mechanism that allows failing asynch operations
+	 * to be diagnosed.]
+	 * </p>
 	 */
 	public void stop();
 
 	/**
-	 * Stop the server and wait until the
-	 * server has completely stopped.
+	 * Stops this server and waits until the server has completely stopped.
+	 * <p>
+	 * This convenience method uses {@link #stop()}
+	 * to stop the server, and an internal thread and listener to detect
+	 * when the server has complied.
+	 * </p>
 	 */
 	public void synchronousStop();
 
 	/**
-	 * Terminate the server process(es). This method should only be
+	 * Terminates the server process(es). This method should only be
 	 * used as a last resort after the stop() method fails to work.
 	 * The server should return from this method quickly and
 	 * use the server listener to notify shutdown progress.
 	 * It MUST terminate the server completely and return it to
 	 * the stopped state.
+	 * <p>
+	 * [issue: Since IServer already has stop(), it's hard to explain
+	 * in what way this method is truely different. Given that stop()
+	 * did not do the trick, why would terminate() have better luck.]
+	 * </p>
 	 */
 	public void terminate();
 
 	/**
-	 * Trigger a restart of the given module and wait until it has finished restarting.
-	 *
-	 * @param module org.eclipse.wst.server.core.IModule
-	 * @param monitor org.eclipse.core.runtime.IProgressMonitor
-	 * @exception org.eclipse.core.runtime.CoreException - thrown if an error occurs while trying to restart the module
+	 * Restarts the given module and waits until it has finished restarting.
+	 * <p>
+	 * [issue: Lack of symmetry. Why is there no moduleRestart? start, restart,
+	 * and stop all have assynchronous equivalents.]
+	 * </p>
+	 * <p>
+	 * [issue: It should probably be spec'd to throw an exception error if the
+	 * given module is not associated with the server.]
+	 * </p>
+	 * <p>
+	 * [issue: This method should be renamed ""synchronousRestartModule".
+	 * This would bring it into line with IServerDelegate.restartModule.]
+	 * </p>
+	 * <p>
+	 * [issue: If the module was just published to the server
+	 * and had never been started, would is be ok to "start"
+	 * the module using this method?]
+	 * </p>
+	 * 
+	 * @param module the module to be restarted
+	 * @param monitor a progress monitor, or <code>null</code> if progress
+	 *    reporting and cancellation are not desired
+	 * @exception CoreException if an error occurs while trying to restart the module
 	 */
-	public void synchronousModuleRestart(final IModule module, IProgressMonitor monitor) throws CoreException;
+	public void synchronousModuleRestart(IModule module, IProgressMonitor monitor) throws CoreException;
 
 	/**
 	 * Returns a temporary directory that the requestor can use
@@ -323,31 +831,66 @@ public interface IServer extends IElement {
 	public void updateConfiguration();
 
 	/**
-	 * Returns true if this module can be added to this
-	 * configuration at the current time, and false otherwise.
-	 *
-	 * <p>This method may decide based on the type of module
+	 * Returns whether the specified module modifications could be made to this
+	 * server at this time.
+	 * <p>
+	 * This method may decide based on the type of module
 	 * or refuse simply due to reaching a maximum number of
-	 * modules or other criteria.</p>
+	 * modules or other criteria.
+	 * </p>
+	 * <p>
+	 * [issue: This seems odd to have a pre-flight method.
+	 * I should expect that the client can propose making
+	 * any set of module changes they desire (via a server
+	 * working copy). If the server doesn't like it, the operation
+	 * should fail.]
+	 * </p>
 	 *
-	 * @param add org.eclipse.wst.server.core.model.IModule
-	 * @return boolean
+	 * @param add a possibly-empty list of modules to add
+	 * @param remove a possibly-empty list of modules to remove
+	 * @param monitor a progress monitor, or <code>null</code> if progress
+	 *    reporting and cancellation are not desired
+	 * @return <code>true</code> if the proposed modifications
+	 * look feasible, and <code>false</code> otherwise
 	 */
 	public IStatus canModifyModules(IModule[] add, IModule[] remove);
 
 	/**
-	 * Returns the modules that are in this configuration.
+	 * Returns the list of modules that are associated with
+	 * this server.
+	 * <p>
+	 * [issue: Clarify that these are root modules, not ones parented
+	 * by some other module.]
+	 * </p>
 	 *
-	 * @return org.eclipse.wst.server.core.model.IModule[]
+	 * @return a possibly-empty list of modules
 	 */
 	public IModule[] getModules();
 	
 	/**
-	 * Returns the current state of the given module. See
-	 * class header for MODULE_XXX constants.
+	 * Returns the current state of the given module on this server.
+	 * Returns <code>MODULE_STATE_UNKNOWN</code> if the module
+	 * is not among the ones associated with this server.
+	 * </p>
+	 * <p>
+	 * [issue: This operation gets forwarded to the delegate.
+	 * It's unclear whether this operations is guaranteed to be fast
+	 * or whether it could involve communication with any actual
+	 * server. If it is not fast, the method should take a progress
+	 * monitor.]
+	 * </p>
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 * <p>
+	 * [issue: The SERVER_XXX and MODULE_STATE_XXX constants
+	 * should be combined into a single set: {unknown, starting, started,
+	 * stopping, stopped}.]
+	 * </p>
 	 *
-	 * @param module org.eclipse.wst.server.core.model.IModule
-	 * @return byte
+	 * @param module the module
+	 * @return one of the module state (<code>MODULE_STATE_XXX</code>)
+	 * constants declared on {@link IServer}
 	 */
 	public byte getModuleState(IModule module);
 	
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerConfiguration.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerConfiguration.java
index 2beaacff1..3aa585e4a 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerConfiguration.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerConfiguration.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -13,26 +13,184 @@ package org.eclipse.wst.server.core;
 import org.eclipse.core.resources.IFile;
 import org.eclipse.core.resources.IFolder;
 import org.eclipse.core.runtime.IPath;
-import org.eclipse.wst.server.core.model.*;
+import org.eclipse.wst.server.core.model.IServerConfigurationDelegate;
 /**
- * A server configuration. Server configurations usually contain
- * directories (the resources to be run on the server) and configuration
- * information. (i.e. mime types, data sources, etc.)
+ * Represents a server configuration instance. Every server configuration is an
+ * instance of a particular, fixed server configuration type.
+ * <p>
+ * Servers have an optional server configuration. The server configuration is
+ * information used to configure a running server. Simple types of servers
+ * might not require any configuration, whereas full-featured web server have
+ * an extensive set of parameters for adjusting the server's behavior. Even
+ * though server configuration information usually takes the form of one or
+ * more files, configuration information is treated separately from actual
+ * content. Actual web content can be deployed on different servers without
+ * change, whereas server configuration information is usually highly
+ * dependent on the particulars of the server. Having the server configuration
+ * identified as an entity separate from the server itself facilitates
+ * switching an existing server between configurations, and sharing server
+ * configurations between several servers of the same type (e.g., a local test
+ * server and a remote server running on another host). * </p>
+ * <p>
+ * The resource manager maintains a global list of all known server 
+ * configuration instances ({@link IResourceManager#getServerConfigurations()}).
+ * </p>
+ * <p>
+ * [issue: The information actually stored in the (.config) file is:
+ * server configuration id and name, and server configuration type id.
+ * It's unclear what's gained by storing this
+ * information in a workspace file. Is it so that this information
+ * can be shared between users via a repository? Or is it just so that
+ * there would be something to open in the resource navigator view?]
+ * </p>
+ * <p>
+ * [issue: What is the role of the data files in the configuration
+ * data folder? Where do they come from? Are they created by the
+ * client (UI), or do they get created automatically?]
+ * </p>
+ * <p>
+ * [issue: Are data files ever changed as a direct and immediate
+ * result of a change to a working copy? Changing any of the data
+ * files prior to the save() request would be problematic since
+ * these files are shared by the original. The only way it could
+ * work is to the data files to be updated only when a working copy
+ * is saved.]
+ * </p>
+ * <p>
+ * [issue: Equality/identify for server configurations?]
+ * </p>
  * 
  * <p>This interface is not intended to be implemented by clients.</p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
+ * 
+ * @since 1.0
  */
 public interface IServerConfiguration extends IElement {
+	
+	/**
+	 * File extension (value "config") for serialized representation of
+	 * server configuration instances.
+	 * <p>
+	 * [issue: What is relationship between this file extension and
+	 * the file passed to IServerConfigurationType.createServerConfiguration(...)
+	 * and importFromPath/Runtime or returned by IServerConfiguration.getFile()?
+	 * That is, are server files configuration expected to end
+	 * in ".config", or is this just a default? If the former
+	 * (as I suspect), then IServerConfigurationType operations needs to say so,
+	 * and the implementation should enforce the restriction.]
+	 * </p>
+	 */
 	public static final String FILE_EXTENSION = "config";
 
+	/**
+	 * Returns the type of this server configuration.
+	 * 
+	 * @return the server configuration type
+	 */
 	public IServerConfigurationType getServerConfigurationType();
 	
+	/**
+	 * Returns the file where this server configuration instance is serialized.
+	 * 
+	 * @return the file in the workspace where the server configuration instance
+	 * is serialized, or <code>null</code> if the information is
+	 * instead to be persisted with the workspace but not with any
+	 * particular workspace resource
+	 */
 	public IFile getFile();
 	
+	/**
+	 * Returns the delegate for this server configuration.
+	 * The delegate is a server-configuration-type-specific object.
+	 * By casting the server configuration delegate to the type prescribed in
+	 * the API documentation for that particular server configuration type, 
+	 * the client can access server-configuration-type-specific properties and
+	 * methods.
+	 * <p>
+	 * [issue: Exposing IServerConfigurationDelegate to clients of IServer
+	 * is confusing and dangerous. Instead, replace this
+	 * method with something like getServerConfigurationExtension() which
+	 * returns an IServerConfigurationExtension. IServerConfigurationExtension is an
+	 * "marker" interface that server configuration providers would 
+	 * implement or extend if they want to expose additional
+	 * API for their server configuration type. That way
+	 * IServerConfigurationDelegate can be kept entirely on the SPI side, out
+	 * of view from clients.]
+	 * </p>
+	 * <p>
+	 * [issue: serverConfigurationTypes schema, class attribute is optional.
+	 * This suggests that a server need not provide a delegate class.
+	 * This seems implausible. I've spec'd this method 
+	 * as if delegate is mandatory.]
+	 * </p>
+	 * 
+	 * @return the server configuration delegate
+	 */
 	public IServerConfigurationDelegate getDelegate();
 	
+	/**
+	 * Returns a working copy for modifying this server configuration instance.
+	 * If this instance is already a working copy, it is returned.
+	 * If this instance is not a working copy, a new server configuration
+	 * working copy is created with the same id and attributes.
+	 * Clients are responsible for saving or releasing the working copy when
+	 * they are done with it.
+	 * <p>
+	 * The server configuration working copy is related to this server 
+	 * configuration instance in the following ways:
+	 * <pre>
+	 * this.getWorkingCopy().getId() == this.getId()
+	 * this.getWorkingCopy().getFile() == this.getFile()
+	 * this.getWorkingCopy().getOriginal() == this
+	 * </pre>
+	 * </p>
+	 * <p>
+	 * [issue: IServerConfigurationWorkingCopy extends IServerConfiguration. 
+	 * ServerConfiguration.getWorkingCopy() create a new working copy;
+	 * ServerConfigurationWorkingCopy.getWorkingCopy() returns this.
+	 * This may be convenient in code that is ignorant of
+	 * whether they are dealing with a working copy or not.
+	 * However, it is hard for clients to manage working copies
+	 * with this design.
+	 * This method should be renamed "createWorkingCopy"
+	 * or "newWorkingCopy" to make it clear to clients that it
+	 * creates a new object, even for working copies.]
+	 * </p>
+	 * 
+	 * @return a new working copy
+	 */
 	public IServerConfigurationWorkingCopy getWorkingCopy();
 	
+	/**
+	 * Returns the handle of a workspace folder where this server
+	 * configuration's files are stored. Returns <code>null</code>
+	 * if the folder is outside the workspace (in which case use
+	 * {@link #getConfigurationDataPath()} to get the file system
+	 * path).
+	 * <p>
+	 * When a configuration instance is serialized in a workspace
+	 * file, the accompanying data files are stored in a
+	 * sibling folder with a derivative name. If the configuration file
+	 * is named "MyConfig.config", the sibling folder that holds the data
+	 * files would be named "MyConfig-data".
+	 * </p>
+	 * 
+	 * @return a workspace folder, or <code>null</code>
+	 * if the data folder is not in the workspace
+	 */
 	public IFolder getConfigurationDataFolder();
 	
+	/**
+	 * Returns the path to a non-workspace folder where this server
+	 * configuration's files are stored. Returns <code>null</code>
+	 * if the folder is inside the workspace (in which case use
+	 * {@link #getConfigurationDataFolder()} to get the folder's handle).
+	 * 
+	 * @return a folder not in the workspace, or <code>null</code>
+	 * if the data folder is in the workspace
+	 */
 	public IPath getConfigurationDataPath();
 }
\ No newline at end of file
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerConfigurationType.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerConfigurationType.java
index 08cee7d7f..af957e9fc 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerConfigurationType.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerConfigurationType.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -15,28 +15,71 @@ import org.eclipse.core.runtime.CoreException;
 import org.eclipse.core.runtime.IPath;
 import org.eclipse.core.runtime.IProgressMonitor;
 /**
- * A server configuration. Server configurations usually contain
- * directories (the resources to be run on the server) and configuration
- * information. (i.e. mime types, data sources, etc.)
+ * Represents a server configuration type from which server configuration 
+ * instances ({@link IServerConfiguration}) can be created.
+ * <p>
+ * The server core framework supports
+ * an open-ended set of server configuration types, which are contributed via
+ * the <code>serverConfigurationTypes</code> extension point in the server core
+ * plug-in. Server configuration type objects carry no state (all information is
+ * read-only and is supplied by the server configuration type declaration).
+ * The global list of known server configuration types is available via
+ * {@link ServerCore#getServerConfigurationTypes()}. 
+ * </p>
+ * <p>
+ * This interface is not intended to be implemented by clients.
+ * </p>
+ * <p>
+ * [issue: It is notoriously difficult to place any kind of
+ * useful order on objects that are contributed independently by
+ * non-collaborating parties. The IOrdered mechanism is weak, and
+ * can't really solve the problem. Issues of presentation are usually
+ * best left to the UI, which can sort objects based on arbitrary
+ * properties.]
+ * </p>
+ * <p>
+ * [issue: Equality/identify for server types? Are IServerConfigurationType
+ * instances guaranteed to be canonical (client can use ==),
+ * or is it possible for there to be non-identical IServerConfigurationType
+ * objects in play that both represent the same server configuration type?
+ * The latter is the more common; type should spec equals.]
+ * </p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
  * 
- * <p>This interface is not intended to be implemented by clients.</p>
+ * @since 1.0
  */
 public interface IServerConfigurationType extends IOrdered {
+
 	/**
+	 * Returns the id of this server configuration type.
+	 * Each known server configuration type has a distinct id. 
+	 * Ids are intended to be used internally as keys; they are not
+	 * intended to be shown to end users.
 	 * 
-	 * @return
+	 * @return the server configuration type id
 	 */
 	public String getId();
 
 	/**
-	 * 
-	 * @return
+	 * Returns the displayable name for this server configuration type.
+	 * <p>
+	 * Note that this name is appropriate for the current locale.
+	 * </p>
+	 *
+	 * @return a displayable name for this server configuration type
 	 */
 	public String getName();
 
 	/**
-	 * 
-	 * @return
+	 * Returns the displayable description for this server configuration type.
+	 * <p>
+	 * Note that this description is appropriate for the current locale.
+	 * </p>
+	 *
+	 * @return a displayable description for this server configuration type
 	 */
 	public String getDescription();
 
@@ -45,16 +88,125 @@ public interface IServerConfigurationType extends IOrdered {
 	 * resource. If these extensions are given, the resource is
 	 * assumed to be a file. If null is returned, the import will
 	 * look for folders instead.
+	 * <p>
+	 * [issue: Explain how these are used.]
+	 * </p>
+	 * <p>
+	 * [issue: Seems like a very UI-centric API, useful only
+	 * in conjuction with importFromPath(...).]
+	 * </p>
 	 *
 	 * @return java.lang.String[]
 	 */
 	public String[] getImportFilterExtensions();
 
+	/**
+	 * Returns whether this type of server configuration requires
+	 * requires it's own data in a folder in the workspace.
+	 * <p>
+	 * [issue: What does it mean when isFolder returns false?]
+	 * </p>
+	 * 
+	 * @return <code>true</code> if this type of server configuration
+	 * needs a folder, and <code>false</code> if it does not
+	 */
 	public boolean isFolder();
 
+	/**
+	 * Creates a working copy instance of this server configuration type.
+	 * After setting various properties of the working copy,
+	 * the client should call
+	 * {@link IServerConfigurationWorkingCopy#save(IProgressMonitor)}
+	 * to bring the server configuration instance into existence.
+	 * <p>
+	 * [issue: Since this method just creates a working copy,
+	 * it's not clear the operation is long-running and in need
+	 * of a progress monitor.]
+	 * </p>
+	 * <p>
+	 * [issue: This method is declared as throwing CoreException.
+	 * From a clients's point of view, what are the circumstances that
+	 * cause this operation to fail?]
+	 * </p>
+	 * 
+	 * @param id the id to assign to the server configuration instance;
+	 * a generated id is used if id is <code>null</code> or an empty string
+	 * @param file the file in the workspace where the server configuration
+	 * instance is to be serialized, or <code>null</code> if the information is
+	 * instead to be persisted with the workspace but not with any
+	 * particular workspace resource
+	 * @param monitor a progress monitor, or <code>null</code> if progress
+	 *    reporting and cancellation are not desired
+	 * @return a new server configuration working copy with the given id
+	 * @throws CoreException [missing]
+	 */
 	public IServerConfigurationWorkingCopy createServerConfiguration(String id, IFile file, IProgressMonitor monitor) throws CoreException;
 	
+	/**
+	 * Creates a working copy instance of this server configuration type,
+	 * by importing from the given local file system path (outside the workspace).
+	 * After setting additional various properties of the working copy,
+	 * the client should call
+	 * {@link IServerConfigurationWorkingCopy#save(IProgressMonitor)}
+	 * to bring the server configuration instance into existence.
+	 * <p>
+	 * [issue: What this does is type-dependent. Perhaps these
+	 * methods should instead be type-dependent API on 
+	 * IServerConfigurationWorkingCopy(Delegate).]
+	 * </p>
+	 * <p>
+	 * [issue: This method is declared as throwing CoreException.
+	 * From a clients's point of view, what are the circumstances that
+	 * cause this operation to fail?]
+	 * </p>
+	 * 
+	 * @param id the id to assign to the server configuration instance;
+	 * a generated id is used if id is <code>null</code> or an empty string
+	 * @param file the file in the workspace where the server configuration
+	 * instance is to be serialized, or <code>null</code> if the information is
+	 * instead to be persisted with the workspace but not with any
+	 * particular workspace resource
+	 * @param path a local file system path
+	 * @param monitor a progress monitor, or <code>null</code> if progress
+	 *    reporting and cancellation are not desired
+	 * @return a new server configuration working copy with the given id
+	 * @throws CoreException [missing]
+	 */
 	public IServerConfigurationWorkingCopy importFromPath(String id, IFile file, IPath path, IProgressMonitor monitor) throws CoreException;
 	
+	/**
+	 * Creates a working copy instance of this server configuration type,
+	 * by importing from the given runtime instance.
+	 * After setting additional various properties of the working copy,
+	 * the client should call
+	 * {@link IServerConfigurationWorkingCopy#save(IProgressMonitor)}
+	 * to bring the server configuration instance into existence.
+	 * <p>
+	 * [issue: A server runtime is primarily for building against.
+	 * How is it that it can cough up a server configuration?]
+	 * </p>
+	 * <p>
+	 * [issue: What this does is type-dependent. Perhaps these
+	 * methods should instead be type-dependent API on 
+	 * IServerConfigurationWorkingCopy(Delegate).]
+	 * </p>
+	 * <p>
+	 * [issue: This method is declared as throwing CoreException.
+	 * From a clients's point of view, what are the circumstances that
+	 * cause this operation to fail?]
+	 * </p>
+	 * 
+	 * @param id the id to assign to the server configuration instance;
+	 * a generated id is used if id is <code>null</code> or an empty string
+	 * @param file the file in the workspace where the server configuration
+	 * instance is to be serialized, or <code>null</code> if the information is
+	 * instead to be persisted with the workspace but not with any
+	 * particular workspace resource
+	 * @param runtime a runtime
+	 * @param monitor a progress monitor, or <code>null</code> if progress
+	 *    reporting and cancellation are not desired
+	 * @return a new server configuration working copy with the given id
+	 * @throws CoreException [missing]
+	 */
 	public IServerConfigurationWorkingCopy importFromRuntime(String id, IFile file, IRuntime runtime, IProgressMonitor monitor) throws CoreException;
 }
\ No newline at end of file
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerConfigurationWorkingCopy.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerConfigurationWorkingCopy.java
index 626942f2f..0036633aa 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerConfigurationWorkingCopy.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerConfigurationWorkingCopy.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -14,13 +14,113 @@ import org.eclipse.core.runtime.CoreException;
 import org.eclipse.core.runtime.IProgressMonitor;
 import org.eclipse.wst.server.core.model.IServerConfigurationWorkingCopyDelegate;
 /**
- * 
+ * A working copy server object used for formulating changes
+ * to a server configuration instance
+ * ({@link org.eclipse.wst.server.core.IServerConfiguration}).
+ * <p>
+ * [issue: There can be other server-configuration-type-specific properties.
+ * The default values for these need to be specified somewhere
+ * too (probably in the API subclass of IServerConfigurationWorkingCopyDelegate).]
+ * </p>
+ * <p>
+ * [issue: IElementWorkingCopy and IElement support an open-ended set
+ * of attribute-value pairs. What is relationship between these
+ * attributes and (a) the get/setXXX methods found on this interface,
+ * and (b) get/setXXX methods provided by specific server types?
+ * Is it the case that these attribute-values pairs are the only
+ * information about a server instance that can be preserved
+ * between workbench sessions? That is, any information recorded
+ * just in instance fields of an IServerConfigurationDelegate implementation
+ * will be lost when the session ends.]
+ * </p>
  * <p>This interface is not intended to be implemented by clients.</p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
+ * 
+ * @since 1.0
  */
 public interface IServerConfigurationWorkingCopy extends IServerConfiguration, IElementWorkingCopy {
+	
+	/**
+	 * Returns the server configuration instance that this working copy is
+	 * associated with.
+	 * <p>
+	 * For a server configuration working copy created by a call to
+	 * {@link IServerConfiguration#getWorkingCopy()},
+	 * <code>this.getOriginal()</code> returns the original
+	 * server configuration object. For a server configuration working copy
+	 * just created by a call to
+	 * {@link IServerConfigurationType#createServerConfiguration(String, IFile, IProgressMonitor)},
+	 * <code>this.getOriginal()</code> returns <code>null</code>.
+	 * </p>
+	 * 
+	 * @return the associated server configuration instance, or <code>null</code> if none
+	 */
 	public IServerConfiguration getOriginal();
 	
+	/**
+	 * Returns the delegate for this server configuration working copy.
+	 * The server configuration working copy delegate is a
+	 * server-configuration-type-specific object. By casting the server
+	 * configuration working copy delegate to the type prescribed in the API
+	 * documentation for that particular server configuration working copy type,
+	 * the client can access server-configuration-type-specific properties and
+	 * methods.
+	 * <p>
+	 * [issue: Exposing IServerConfigurationWorkingCopyDelegate to clients
+	 * of IServerConfigurationWorkingCopy is same problem as exposing
+	 * IServerConfigurationDelegate to clients of IServerConfiguration.
+	 * The suggested fix is to replace this method with something like
+	 * getServerConfigurationWorkingCopyExtension() which
+	 * returns an IServerConfigurationWorkingCopyExtension.]
+	 * </p>
+	 * <p>
+	 * [issue: serverConfigurationTypes schema, workingCopyClass attribute is
+	 * optional. This suggests that a server configuration need not provide a
+	 * working copy delegate class. Like the class attribute, this seems
+	 * implausible. I've spec'd this method as if working copy delegate is
+	 * mandatory.]
+	 * </p>
+	 * 
+	 * @return the delegate for the server configuration working copy
+	 */
 	public IServerConfigurationWorkingCopyDelegate getWorkingCopyDelegate();
 	
+	/**
+	 * Commits the changes made in this working copy. If there is
+	 * no extant server configuration instance with a matching id and
+	 * server configuration type, this will create a server configuration
+	 * instance with attributes taken from this working copy.
+	 * If there an existing server configuration instance with a matching id
+	 * and server configuration type, this will change the server configuration
+	 * instance accordingly.
+	 * <p>
+	 * [issue: What is relationship to 
+	 * this.getOriginal() and the IServerConfiguration returned by this.save()?
+	 * The answer should be: they're the same server configuration, for an
+	 * appropriate notion of "same". As currently implemented, they
+	 * are different IServerConfiguration instances but have the same
+	 * ids and types. Client that are hanging on to the old server configuration
+	 * instance will not see the changes. 
+	 * If IServerConfiguration were some kind of handle object as elsewhere in 
+	 * Eclipse Platform, this kind of change could be done much
+	 * more smoothly.]
+	 * </p>
+	 * <p>
+	 * [issue: What if this object has already been saved
+	 * or released?]
+	 * </p>
+	 * <p>
+	 * [issue: What is lifecycle for IServerConfigurationWorkingCopyDelegate
+	 * associated with this working copy?]
+	 * </p>
+	 * 
+	 * @param monitor a progress monitor, or <code>null</code> if progress
+	 *    reporting and cancellation are not desired
+	 * @return a new server configuration instance
+	 * @throws CoreException [missing]
+	 */
 	public IServerConfiguration save(IProgressMonitor monitor) throws CoreException;
 }
\ No newline at end of file
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerState.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerState.java
index 3d901f9b8..64cf94665 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerState.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerState.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved.   This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -12,14 +12,41 @@ package org.eclipse.wst.server.core;
 
 import org.eclipse.wst.server.core.model.IModule;
 /**
- * 
+ * Interface providing privileged access for setting the state of 
+ * a server instance.
+ * <p>
+ * Objects of this type are passed to IServerDelegate.initialize.
+ * </p>
+ * <p>
+ * [issue: This is pure SPI. Should be in org.eclipse.wst.server.core.model package.]
+ * </p>
+ * <p>
+ * [issue: This API is vulnerable to abuse. Any client with an IServer
+ * can cast it to an IServerState and call these methods. A more secure
+ * way to achieve the same thing is to have IServerState provide an extra
+ * getServer() method rather than extend IServer. That way, casting an
+ * IServer is not an option.]
+ * </p>
  * <p>This interface is not intended to be implemented by clients.</p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
+ * 
+ * @see org.eclipse.wst.server.core.model.IServerDelegate#initialize(IServerState)
+ * @since 1.0
  */
-public interface IServerState extends IServer {	
+public interface IServerState extends IServer {
+	
 	/**
-	 * Set the server state.
-	 * 
-	 * @param state
+	 * Sets the current state of this server.
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 *
+	 * @param state one of the server state (<code>SERVER_XXX</code>)
+	 * constants declared on {@link IServer}
+	 * @see IServer#getServerState()
 	 */
 	public void setServerState(byte state);
 
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerType.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerType.java
index c84a481fc..9c4d4b6c0 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerType.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerType.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -14,113 +14,430 @@ import org.eclipse.core.resources.IFile;
 import org.eclipse.core.runtime.CoreException;
 import org.eclipse.core.runtime.IProgressMonitor;
 /**
+ * Represents a server type from which server instances can be created.
+ * <p>
+ * The server core framework supports
+ * an open-ended set of server types, which are contributed via
+ * the <code>serverTypes</code> extension point in the server core
+ * plug-in. Server type objects carry no state (all information is
+ * read-only and is supplied by the server type declaration).
+ * The global list of known server types is available via
+ * {@link ServerCore#getServerTypes()}. 
+ * </p>
+ * <p>
+ * This interface is not intended to be implemented by clients.
+ * </p>
+ * <p>
+ * [issue: It is notoriously difficult to place any kind of
+ * useful order on objects that are contributed independently by
+ * non-collaborating parties. The IOrdered mechanism is weak, and
+ * can't really solve the problem. Issues of presentation are usually
+ * best left to the UI, which can sort objects based on arbitrary
+ * properties.]
+ * </p>
+ * <p>
+ * [issue: Equality/identify for server types? Are IServerType
+ * instances guaranteed to be canonical (client can use ==),
+ * or is it possible for there to be non-identical IServerType
+ * objects in play that both represent the same server type?
+ * The latter is the more common; type should spec equals.]
+ * </p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
  * 
- * <p>This interface is not intended to be implemented by clients.</p>
+ * @since 1.0
  */
 public interface IServerType extends IOrdered {
-	//	--- State Set Constants ---
-	// (returned from the getServerStateSet() method)
 	
-	// a server that can be directly started/stopped, etc.
+	/**
+	 * Constant (value 0) indicating that a type of server that can be
+	 * directly started and stopped.
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 * 
+	 * @see #getServerStateSet()
+	 */
 	public static final byte SERVER_STATE_SET_MANAGED = 0;
 	
-	// a server that is attached to, typically for debugging
+	/**
+	 * Constant (value 1) indicating that a type of server that can be
+	 * attached to, typically for debugging.
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 * 
+	 * @see #getServerStateSet()
+	 */
 	public static final byte SERVER_STATE_SET_ATTACHED = 1;
 	
-	// a server that is only used for publishing
+	/**
+	 * Constant (value 2) indicating that a type of server that
+	 * can only be used for publishing.
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 * 
+	 * @see #getServerStateSet()
+	 */
 	public static final byte SERVER_STATE_SET_PUBLISHED = 2;
 
 	/**
+	 * Returns the id of this server type.
+	 * Each known server type has a distinct id. 
+	 * Ids are intended to be used internally as keys; they are not
+	 * intended to be shown to end users.
 	 * 
-	 * @return
+	 * @return the server type id
 	 */
 	public String getId();
 
 	/**
-	 * 
-	 * @return
+	 * Returns the displayable name for this server type.
+	 * <p>
+	 * Note that this name is appropriate for the current locale.
+	 * </p>
+	 *
+	 * @return a displayable name for this server type
 	 */
 	public String getName();
 
 	/**
-	 * 
-	 * @return
+	 * Returns the displayable description for this server type.
+	 * <p>
+	 * Note that this description is appropriate for the current locale.
+	 * </p>
+	 *
+	 * @return a displayable description for this server type
 	 */
 	public String getDescription();
 
 	/**
+	 * Returns the type of server runtime that this type
+	 * of server requires.
+	 * <p>
+	 * [issue: "runtimeTypeId" is mandatory according the
+	 * serverTypes schema. This suggests that all types
+	 * of servers have a server runtime. But there is also
+	 * a boolean "runtime" attribute indicating whether the
+	 * server requires a runtime. I supect that server type
+	 * has an optional server runtime, in which case you
+	 * can make "runtimeTypeId" optional and dispense with
+	 * "runtime".]
+	 * </p>
+	 * <p>
+	 * [issue: Does it really make sense for
+	 * runtimeTypes and serverTypes be separate extension
+	 * points? Would it not be sufficient to have the party declaring
+	 * the server type also declare the server runtime type?
+	 * Having runtimeType as a separate extension point
+	 * only makes sense if it would be possible in principle to 
+	 * declare a server runtime type that could actually be
+	 * used on serveral server types. If server runtimes
+	 * always end up being server-type specific, it would be better
+	 * to combine them.]
+	 * </p>
+	 * <p>
+	 * [issue: What should happen when a server type mentions
+	 * the id of a server runtime type that is not known
+	 * to the system?]
+	 * </p>
 	 * 
-	 * @return
+	 * @return a server runtime type
 	 */
 	public IRuntimeType getRuntimeType();
 	
+	/**
+	 * Returns whether this type of server requires a server
+	 * runtime.
+	 * <p>
+	 * [issue: See issues on getRuntimeType(). I suspect this
+	 * method is unnecessary, and that 
+	 * this.getRuntimeType() != null will do.]
+	 * </p>
+	 * 
+	 * @return <code>true</code> if this type of server requires
+	 * a server runtime, and <code>false</code> if it does not
+	 * @see #getRuntimeType()
+	 */
 	public boolean hasRuntime();
 	
 	/**
-	 * Returns true if this server can start or may already be started
-	 * in the given mode, and false if not. Uses the launchConfigId attribute
-	 * to find the server's launch configuration.
+	 * Returns whether this type of server supports the given launch mode.
+	 * <p>
+	 * [issue: It also seems odd that this is part of the server type
+	 * declaration. This means that any server type has to commit
+	 * so early on which modes it supports.]
+	 * </p>
+	 * <p>
+	 * [issue: Because the spec for this method piggy-backs off the
+	 * debug API, this method is vulnerable to any expansion that may
+	 * happen there. For instance, a 3rd mode (PROFILE_MODE) was added in 3.0.
+	 * As spec'd, clients can reasonably expect to launch servers
+	 * in this mode as well. It may also make sense for the server
+	 * core to define its own notion of legal modes.]
+	 * </p>
 	 * 
-	 * @param launchMode String
-	 * @return boolean
+	 * @param launchMode a mode in which a server can be launched,
+	 * one of the mode constants defined by
+	 * {@link org.eclipse.debug.core.ILaunchManager}
+	 * @return whether this type of server supports the given mode
 	 */
 	public boolean supportsLaunchMode(String launchMode);
 
 	/**
-	 * Returns an IStatus message to verify if a server of this type will be able
-	 * to run the module immediately after being created, without any user
-	 * interaction. If OK, this server may be used as a default server. This
-	 * method should return ERROR if the user must supply any information to
-	 * configure the server correctly, or if the module is not supported.
+	 * Returns the server state set that should for instances of this server type.
+	 * If the state set is {@link #SERVER_STATE_SET_MANAGED}, this is
+	 * a runnable server that may be directly started and stopped 
+	 * (i.e., it should be represented as starting, started in debug mode,
+	 * etc.) If the state set is {@link #SERVER_STATE_SET_ATTACHED}, this is a
+	 * server that can be attached to, typically for debugging purposes
+	 * (i.e., it should be represented as attaching, attached for
+	 * debugging, etc.).
+	 * If the state set is {@link #SERVER_STATE_SET_PUBLISHED}, this is a
+	 * server that only be published to (i.e., it should be represented as
+	 * not having states).
+	 * <p>
+	 * [issue: The notion of a "server state set" is a little abstruce.
+	 * It feels like the main thing to capture in the server type
+	 * is that there are 3 different styles of servers, and this
+	 * has a bearing on how you control them. That would suggest
+	 * there will be different rules for operating on them, and
+	 * that they might cycle through different sets of states.
+	 *  ]
+	 * </p>
+	 * <p>
+	 * [issue: It also seems odd that this is part of the server type
+	 * declaration. This means that any server type has to commit
+	 * so early on which one it is.]
+	 * </p>
+	 * <p>
+	 * [issue: byte is rarely used in Java. Always use int instead.]
+	 * </p>
 	 *
-	 * @return org.eclipse.core.resources.IStatus
+	 * @return one of {@link #SERVER_STATE_SET_MANAGED},
+	 * {@link #SERVER_STATE_SET_ATTACHED}, or
+	 * {@link #SERVER_STATE_SET_PUBLISHED}
 	 */
-	//public IStatus isDefaultAvailable(IModule module);
+	public byte getServerStateSet();
 
 	/**
-	 * Returns the server state set that should be used to represent
-	 * this server. If the state set is SERVER_STATE_SET_MANAGED, this is
-	 * a runnable server that may be directly started and stopped.
-	 * (i.e. it should be represented as starting, started in debug mode,
-	 * etc.) If the state set is SERVER_STATE_SET_ATTACHED, this is a
-	 * server that can be attached to, typically for debugging purposes.
-	 * (i.e. it should be represented as attaching, attached for
-	 * debugging, etc.)
-	 *
-	 * @return byte
+	 * Returns the type of server configuration that this type
+	 * of server requires.
+	 * <p>
+	 * [issue: configurationTypeId is optional according the
+	 * serverTypes schema. This suggests that some types
+	 * of servers do not take a server configuration.
+	 * Is this correct?
+	 * ]
+	 * </p>
+	 * <p>
+	 * [issue: Does it really make sense for
+	 * serverConfigurationTypes and serverTypes be separate extension
+	 * points? Would it not be sufficient to have the party declaring
+	 * the server type also declare the server configuration type?
+	 * Having serverConfigurationType as a separate extension point
+	 * only makes sense if it would be possible in principle to 
+	 * declare a server configuration type that could actually be
+	 * used on serveral server types. If server configurations
+	 * always end up being server-type specific, it would be better
+	 * to combine them.]
+	 * </p>
+	 * <p>
+	 * [issue: What should happen when a server type mentions
+	 * the id of a server configuration type that is not known
+	 * to the system?]
+	 * </p>
+	 * 
+	 * @return a server configuration type, or <code>null</code>
+	 * if this server does not require any 
 	 */
-	public byte getServerStateSet();
-
 	public IServerConfigurationType getServerConfigurationType();
 
+	/**
+	 * Returns whether this type of server requires a server
+	 * configuration.
+	 * <p>
+	 * [issue: It's not clear how this method differs from 
+	 * this.getServerConfigurationType() != null]
+	 * </p>
+	 * 
+	 * @return <code>true</code> if this type of server requires
+	 * a server configuration, and <code>false</code> if it does not
+	 * @see #getServerConfigurationType()
+	 */
 	public boolean hasServerConfiguration();
 
+	/**
+	 * Returns whether this type of server can run on the local host.
+	 * <p>
+	 * [issue: What is the significance of a server type supporting
+	 * local or remote host? Can a server type support both local and
+	 * remote hosts? What about supporting neither?]
+	 * </p>
+	 * <p>
+	 * [issue: Should be renamed "supportsLocalHost" (capital "H").]
+	 * </p>
+	 * <p>
+	 * [issue: Again, it seems odd to me that this is something
+	 * hard-wired to a server type.]
+	 * </p>
+	 * <p>
+	 * [issue: hosts is optional according the serverTypes schema.
+	 * The schema suggests that "local" and "remote" are the legal values.
+	 * The implementation is quite different; it looks to see if
+	 * the value contains either "localhost" or "127.0.0.1". The schema spec
+	 * be tightened up and the implementation brought into line.]
+	 * </p>
+	 * 
+	 * @return <code>true</code> if this type of server can run on
+	 * the local host, and <code>false</code> if it cannot
+	 */
 	public boolean supportsLocalhost();
 
+	/**
+	 * Returns whether this type of server can run on a remote host.
+	 * <p>
+	 * [issue: What is the significance of a server type supporting
+	 * local or remote host? Can a server type support both local and
+	 * remote hosts? What about supporting neither?]
+	 * </p>
+	 * <p>
+	 * [issue: Should be renamed "supportsRemoteHost" (no "s").]
+	 * </p>
+	 * <p>
+	 * [issue: Again, it seems odd to me that this is something
+	 * hard-wired to a server type.]
+	 * </p>
+	 * <p>
+	 * [issue: hosts is optional according the serverTypes schema.
+	 * The schema suggests that "local" and "remote" are the legal values.
+	 * The implementation is quite different; it looks to see if
+	 * the value contains "remote" as a substring. The schema spec
+	 * be tightened up and the implementation brought into line.]
+	 * </p>
+	 * 
+	 * @return <code>true</code> if this type of server can run on
+	 * a remote host, and <code>false</code> if it cannot
+	 */
 	public boolean supportsRemoteHosts();
 
 	/**
-	 * Returns true if the "monitorable" attribute is set. If true, this
-	 * server's delegate should implement IMonitorableServer.
+	 * Returns whether this type of server can be monitored.
+	 * <p>
+	 * For instances of server types that can be monitored, 
+	 * the corresponding server delegate implements
+	 * {@link org.eclipse.wst.server.core.model.IMonitorableServer}.
+	 * </p>
+	 * <p>
+	 * [issue: Again, it seems odd to me that this is something
+	 * hard-wired to a server type. Without loss of generality,
+	 * the notion of "monitorable" need only show up at the
+	 * server instance level. All that would be required
+	 * would be to have the server delegate implement
+	 * IMonitorableServer.]
+	 * </p>
+	 * <p>
+	 * [issue: It's not clear how much of this is a client concern.
+	 * If it is a client concern, care should be taken to do
+	 * it in such a way that the server delegate does not need to be
+	 * exposed to ordinary clients.]
+	 * </p>
 	 * 
-	 * @return boolean
+	 * @return <code>true</code> if this type of server can be monitored,
+	 * and <code>false</code> if it cannot
 	 */
 	public boolean isMonitorable();
 
 	/**
-	 * Returns true if the "testEnvironment" attribute is set. If true, this
+	 * Returns whether this type of server can be used as a
+	 * test environment.
+	 * <p>
+	 * [issue: How does one explain what a "test environment" is?
+	 * How does this property of server types square with 
+	 * IRuntime.isTestEnvironment(), an *instance-specific*
+	 * property of a server runtime (it's not on IRuntimeType)?]
+	 * </p>
+	 * <p>
+	 * [issue: The old spec read:
+	 * "Returns true if the "testEnvironment" attribute is set. If true, this
 	 * server can only be created when there is an existing runtime that has
-	 * the property "testEnvironment" set to true.
+	 * the property "testEnvironment" set to true."
+	 * ]
+	 * </p>
 	 * 
-	 * @return boolean
+	 * @return <code>true</code> if this type of server can be use as a
+	 * test environment, and <code>false</code> if it cannot
 	 */
 	public boolean isTestEnvironment();
 
 	/**
-	 * Create a server. If file is null, it will be created in metadata. Otherwise,
-	 * it will be associated with the given file.
+	 * Creates an working copy instance of this server type.
+	 * After setting various properties of the working copy,
+	 * the client should call {@link IServerWorkingCopy#save(IProgressMonitor)}
+	 * to bring the server instance into existence.
+	 * <p>
+	 * [issue: Why is a runtime passed in? 
+	 * IServerWorkingCopy.setRuntime(runtime) could be called on
+	 * the result to accomplish the same thing.]
+	 * </p>
+	 * <p>
+	 * [issue: The implementation of this method never creates a server
+	 * config working copy, whereas the other one does!?]
+	 * Consider combining the method with the other.]
+	 * </p>
+	 * <p>
+	 * [issue: This method is declared as throwing CoreException.
+	 * From a clients's point of view, what are the circumstances that
+	 * cause this operation to fail?]
+	 * </p>
+	 * 
+	 * @param id the id to assign to the server instance; a generated
+	 * id is used if id is <code>null</code> or an empty string
+	 * @param file the file in the workspace where the server instance
+	 * is to be serialized, or <code>null</code> if the information is
+	 * instead to be persisted with the workspace but not with any
+	 * particular workspace resource
+	 * @param runtime the runtime to associate with the server instance,
+	 * or <code>null</code> if none
+	 * @return a new server working copy with the given id
+	 * @throws CoreException [missing]
 	 */
 	public IServerWorkingCopy createServer(String id, IFile file, IRuntime runtime) throws CoreException;
 	
+	/**
+	 * Creates a working copy instance of this server type.
+	 * After setting various properties of the working copy,
+	 * the client should call {@link IServerWorkingCopy#save(IProgressMonitor)}
+	 * to bring the server instance into existence.
+	 * <p>
+	 * [issue: Since this method just creates a working copy,
+	 * it's not clear the operation is long-running and in need
+	 * of a progress monitor.]
+	 * </p>
+	 * <p>
+	 * [issue: The implementation of this method creates a server
+	 * config working copy, whereas the other one does not!?
+	 * Consider combining the method with the other.]
+	 * </p>
+	 * <p>
+	 * [issue: This method is declared as throwing CoreException.
+	 * From a clients's point of view, what are the circumstances that
+	 * cause this operation to fail?]
+	 * </p>
+	 * 
+	 * @param id the id to assign to the server instance; a generated
+	 * id is used if id is <code>null</code> or an empty string
+	 * @param file the file in the workspace where the server instance
+	 * is to be serialized, or <code>null</code> if the information is
+	 * instead to be persisted with the workspace but not with any
+	 * particular workspace resource
+	 * @param monitor a progress monitor, or <code>null</code> if progress
+	 *    reporting and cancellation are not desired
+	 * @return a new server working copy with the given id
+	 * @throws CoreException [missing]
+	 */
 	public IServerWorkingCopy createServer(String id, IFile file, IProgressMonitor monitor) throws CoreException;
 }
\ No newline at end of file
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerWorkingCopy.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerWorkingCopy.java
index 86ec80cb3..c75fda537 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerWorkingCopy.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/IServerWorkingCopy.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -15,39 +15,277 @@ import org.eclipse.core.runtime.IProgressMonitor;
 import org.eclipse.wst.server.core.model.IModule;
 import org.eclipse.wst.server.core.model.IServerWorkingCopyDelegate;
 /**
- * 
+ * A working copy server object used for formulating changes
+ * to a server instance ({@link IServer}).
+ * <p>
+ * [issue: The default value of host name should be specified
+ * here (or in IServerType.createServer). If the initial value is null (or
+ * something simularly unsuitable for actual use), then IServer.getHostname
+ * needs to be spec'd to allow null return, and save needs to deal with the
+ * case where the client forgets to initialize this property.]
+ * </p>
+ * <p>
+ * [issue: The default value of runtime should be specified
+ * here (or in IServerType.createServer). If the initial value is null (or
+ * something simularly unsuitable for actual use), then IServer.getRuntime
+ * needs to be spec'd to allow null return (it does), and save needs to deal
+ * with the case where the client forgets to initialize this property.]
+ * </p>
+ * <p>
+ * [issue: The default value of server configuration should be specified
+ * here (or in IServerType.createServer). If the initial value is null (or
+ * something simularly unsuitable for actual use), then
+ * IServer.getServerConfiguration needs to be spec'd to allow null return
+ * (it does), and save needs to deal with the case where the client forgets
+ * to initialize this property.]
+ * </p>
+ * <p>
+ * [issue: There can be other server-type-specific properties.
+ * The default values for these need to be specified somewhere
+ * too (probably in the API subclass of IServerWorkingCopyDelegate).]
+ * </p>
+ * <p>
+ * [issue: IElementWorkingCopy and IElement support an open-ended set
+ * of attribute-value pairs. What is relationship between these
+ * attributes and (a) the get/setXXX methods found on this interface,
+ * and (b) get/setXXX methods provided by specific server types?
+ * Is it the case that these attribute-values pairs are the only
+ * information about a server instance that can be preserved
+ * between workbench sessions? That is, any information recorded
+ * just in instance fields of an IServerDelegate implementation
+ * will be lost when the session ends.]
+ * </p>
+ * <p>
+ * [issue: It seems strange that IServerWorkingCopy extends IServer, which
+ * has all sorts of method for starting and stopping the server, none of
+ * which are relevant to working copies. It should be changed so
+ * that IServerWorkingCopy does not extend IServer. This will requires
+ * copying getters from IServer to IServerWorkingCopy to make it
+ * self-sufficient: getHostname, getServerConfiguration, getRuntime, getId,
+ * getFile, getServerType, etc.]
+ * </p>
  * <p>This interface is not intended to be implemented by clients.</p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
+ * 
+ * @since 1.0
  */
-public interface IServerWorkingCopy extends IServer, IElementWorkingCopy {	
+public interface IServerWorkingCopy extends IServer, IElementWorkingCopy {
+	
+	/**
+	 * Sets the server configuration associated with this server working copy.
+	 * <p>
+	 * Note: The server configuration of a server working copy may
+	 * or may not be a working copy.
+	 * </p>
+	 * <p>
+	 * [issue: According to serverType extension point, 
+	 * configurationTypeId is an optional attribute. What happens if the
+	 * server configuration passed is null but the server must have a
+	 * server configuration? What happens of the server configuration
+	 * has the wrong type? Do the errors get detected and reported now, or
+	 * upon save()?]
+	 * </p>
+	 * 
+	 * @param configuration the server configuration, or <code>null</code> if none
+	 */
 	public void setServerConfiguration(IServerConfiguration configuration);
 	
+	/**
+	 * Returns the server instance that this working copy is
+	 * associated with.
+	 * <p>
+	 * For a server working copy created by a call to
+	 * {@link IServer#getWorkingCopy()},
+	 * <code>this.getOriginal()</code> returns the original
+	 * server object. For a server working copy just created by
+	 * a call to {@link IServerType#createServer(String, IFile, IProgressMonitor)},
+	 * <code>this.getOriginal()</code> returns <code>null</code>.
+	 * </p>
+	 * 
+	 * @return the associated server instance, or <code>null</code> if none
+	 */
 	public IServer getOriginal();
 	
+	/**
+	 * Returns the delegate for this server working copy.
+	 * The server working copy delegate is a
+	 * server-type-specific object. By casting the server working copy
+	 * delegate to the type prescribed in the API documentation for that
+	 * particular server working copy type, the client can access
+	 * server-type-specific properties and methods.
+	 * <p>
+	 * [issue: Exposing IServerWorkingCopyDelegate to clients
+	 * of IServerWorkingCopy is same problem as exposing
+	 * IServerDelegate to clients of IServer. The suggested fix 
+	 * is to replace this method with something like
+	 * getServerWorkingCopyExtension() which
+	 * returns an IServerWorkingCopyExtension.]
+	 * </p>
+	 * <p>
+	 * [issue: serverTypes schema, workingCopyClass attribute is optional.
+	 * This suggests that a server need not provide a working copy
+	 * delegate class. Like the class attribute, this seems implausible.
+	 * I've spec'd this method as if working copy delegate is mandatory.]
+	 * </p>
+	 * 
+	 * @return the delegate for the server working copy
+	 */
 	public IServerWorkingCopyDelegate getWorkingCopyDelegate();
 
+	/**
+	 * Commits the changes made in this working copy. If there is
+	 * no extant server instance with a matching id and server
+	 * type, this will create a server instance with attributes
+	 * taken from this working copy. If there an existing server
+	 * instance with a matching id and server type, this will
+	 * change the server instance accordingly.
+	 * <p>
+	 * [issue: What is relationship to 
+	 * this.getOriginal() and the IServer returned by this.save()?
+	 * The answer should be: they're the same server, for an
+	 * appropriate notion of "same". As currently implemented, they
+	 * are different IServer instances but have the same server
+	 * id and same server types. Clienst that are hanging on to
+	 * the old server instance will not see the changes. 
+	 * If IServer were some kind of handle object as elsewhere in 
+	 * Eclipse Platform, this kind of change could be done much
+	 * more smoothly.]
+	 * </p>
+	 * <p>
+	 * [issue: What if this object has already been saved
+	 * or released?]
+	 * </p>
+	 * <p>
+	 * [issue: What is lifecycle for IServerWorkingCopyDelegate
+	 * associated with this working copy?]
+	 * </p>
+	 * <p>
+	 * [issue: Since it does not make sense to commit a server
+	 * working copy without first committing any associated
+	 * runtime and server config working copies, the semantics
+	 * of saveAll should be part and parcel of the
+	 * normal save, and the saveAll method eliminated.]
+	 * </p>
+	 * 
+	 * @param monitor a progress monitor, or <code>null</code> if progress
+	 *    reporting and cancellation are not desired
+	 * @return a new server instance
+	 * @throws CoreException [missing]
+	 */
 	public IServer save(IProgressMonitor monitor) throws CoreException;
 
+	/**
+	 * Commits the changes made in this server working copy after
+	 * first committing any associated server configuration or
+	 * server runtime working copies.
+	 * <p>
+	 * This convenience method is equivalent to:
+	 * <pre>
+	 * IRuntime rt = this.getRuntime();
+	 * if (rt != null && rt.isWorkingCopy()) {
+	 *    ((IRuntimeWorkingCopy) rt).save(monitor);
+	 * }
+	 * IServerConfiguration cf = this.getServerConfiguration();
+	 * if (cf != null && cf.isWorkingCopy()) {
+	 *    ((IServerConfigurationWorkingCopy) cf).save(monitor);
+	 * }
+	 * return save(monitor);
+	 * </pre>
+	 * </p>
+	 * <p>
+	 * [issue: Since it does not make sense to commit a server
+	 * working copy without first committing any associated
+	 * runtime and server config working copies, the semantics
+	 * of this operation should be part and parcel of the
+	 * normal save, and the saveAll method eliminated.]
+	 * </p>
+	 * 
+	 * @param monitor a progress monitor, or <code>null</code> if progress
+	 *    reporting and cancellation are not desired
+	 * @return a new server instance
+	 * @throws CoreException [missing]
+	 */
 	public IServer saveAll(IProgressMonitor monitor) throws CoreException;
 
+	/**
+	 * Sets the runtime associated with this server working copy.
+	 * <p>
+	 * Note: The runtime of a server working copy may
+	 * or may not be a working copy.
+	 * </p>
+	 * <p>
+	 * [issue: According to serverType extension point, 
+	 * runtimeTypeId is a mandatory attribute. But IServer.getRuntime()
+	 * is allowed to return null, suggesting that it is optional for instances.
+	 * What happens if the runtime passed is null but the server must
+	 * have a runtime? What happens if the runtime has the wrong
+	 * type? Do the errors get detected and reported now, or upon save()?]
+	 * </p>
+	 * 
+	 * @param runtime the runtime, or <code>null</code> if none
+	 */
 	public void setRuntime(IRuntime runtime);
 
+	/**
+	 * Changes the host for the running server.
+	 * The format of the host must conform to RFC 2732.
+	 * <p>
+	 * [issue: Consider renaming to "setHost" to bring in line
+	 * with terminology used in java.net.URL. The host name can be
+	 * either a host name or octets.]
+	 * </p>
+	 * <p>
+	 * [issue: This is a questionable operation if there is a running
+	 * server associated with the original. When a host name
+	 * change is committed, the server instance loses contact with
+	 * the running server because of the host name change.]
+	 * </p>
+	 * 
+	 * @param host a host string conforming to RFC 2732
+	 * @see IServer#getHostname()
+	 * @see java.net.URL.getHost()
+	 */
 	public void setHostname(String host);
 
 	/**
-	 * Add the given module to this configuration. The
-	 * module must exist, should not already be deployed
-	 * within the configuration, and canModifyModules()
+	 * Modifies the list of modules associated with the server.
+	 * The modules included in the <code>add</code> list
+	 * must exist in the workspace and must not already be associated
+	 * with the server.
+	 * The modules included in the <code>remove</code> list
+	 * must be associated with the server, but may or may not exist
+	 * in the workspace.
+	 * <p>
+	 * [issue: How do formulate what it means
+	 * to say "the module must exist in the workspace"?]
+	 * </p>
+	 * <p>
+	 * [issue: The spec should be more lax. Attempting to add
+	 * a module that's already include should be quietly ignore;
+	 * ditto removing a module that's not on this list. This
+	 * simplifies the handling of various other wacko cases 
+	 * such as duplication within and between the add and remove
+	 * lists.]
+	 * </p>
+	 * <p>
+	 * [issue: The spec had also said: "...canModifyModules()
 	 * should have returned true. The configuration must assume
-	 * any default settings and add the module without any UI.
-	 * 
-	 * Removes the given module from this configuration.
-	 * The module must already exist in the configuration.
-	 * When this method is called, the module may no
-	 * longer exist in the workbench or filesystem.
+	 * any default settings and add the module without any UI."]
+	 * </p>
+	 * <p>
+	 * [issue: What error checking should be performed by this
+	 * operation, and what needs to be performed by save() if
+	 * the client tries to commit these hypothetisized changes?]
+	 * </p>
 	 *
-	 * @param add org.eclipse.wst.server.core.model.IModule[]
-	 * @param remove org.eclipse.wst.server.core.model.Module[]
-	 * @param monitor org.eclipse.core.runtime.IProgressMonitor
+	 * @param add a possibly-empty list of modules to add
+	 * @param remove a possibly-empty list of modules to remove
+	 * @param monitor a progress monitor, or <code>null</code> if progress
+	 *    reporting and cancellation are not desired
+	 * @throws CoreException [missing]
 	 */
 	public void modifyModules(IModule[] add, IModule[] remove, IProgressMonitor monitor) throws CoreException;
 }
\ No newline at end of file
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/ServerCore.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/ServerCore.java
index e11c78a0a..e5128a74c 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/ServerCore.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/ServerCore.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -14,19 +14,38 @@ import java.util.*;
 
 import org.eclipse.core.resources.*;
 import org.eclipse.core.runtime.*;
-import org.eclipse.wst.server.core.internal.*;
 import org.eclipse.wst.server.core.model.*;
 import org.eclipse.wst.server.core.util.ProgressUtil;
+import org.eclipse.wst.server.core.internal.*;
 /**
- * Base class for obtaining references to server models.
- * 
- * Also provides references for servers and server configurations.
+ * Main class for server core API.
+ * <p>
+ * This class provides references for servers and server configurations.
  * These references can be saved as tool or server-data and can be
  * used to return the original resource if it still exists. These
- * references are not OS specific and can be used in a team environment.
+ * references are not OS-specific and can be used in a team environment.
+ * </p>
+ * <p>
+ * This class provides all its functionality through static members.
+ * It is not intended to be subclassed or instantiated.
+ * </p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
+ * 
+ * @since 1.0
  */
 public class ServerCore {
-	// server core plugin id
+	/**
+	 // server core plugin id
+	 * <p>
+	 * [issue: Plug-in ids should not be exposed as client API
+	 * unless there is a good reason to do so. Client code generally
+	 * don't care about the plug-in structure. Same is usually true
+	 * for service providers.]
+	 * </p>
+	 */
 	public static final String PLUGIN_ID = "org.eclipse.wst.server.core";
 
 	// cached copy of all server startups
@@ -159,9 +178,30 @@ public class ServerCore {
 	}
 	
 	/**
-	 * Returns a List of all module kinds.
-	 *
-	 * @return java.util.List
+	 * Returns the list of all known module kinds.
+	 * <p>
+	 * Clients must not modify the list that is returned.
+	 * If the set of module kinds changes, the affect on
+	 * the returned list is unspecified.
+	 * </p>
+	 * <p>
+	 * [issue: The terminology should be "module types",
+	 * to make it consistent with server types, etc.]
+	 * </p>
+	 * <p>
+	 * [issue: The list returned is precious. You would not want a client
+	 * to accidentally or malicously whack it. Normal practice is to
+	 * return an array instead of a List, and to return a new copy each call.
+	 * This allows the spec to say that the client can do what they want
+	 * with the result, and that it won't change under foot.
+	 * Another alternative is to return a UnmodifiableList implementation
+	 * so that clients cannot modify. But if you don't copy, you still
+	 * have the problem of the list chaning under foot if a new plug-in
+	 * is installed that happens to define a module kind (a scenario that
+	 * Eclipse should support).]
+	 * </p>
+	 * 
+	 * @return the list of module kinds (element type: {@link IModuleKind})
 	 */
 	public static List getModuleKinds() {
 		if (moduleKinds == null)
@@ -170,9 +210,30 @@ public class ServerCore {
 	}
 	
 	/**
-	 * Returns the module kind with the given id.
+	 * Returns the module kind with the given id, or <code>null</code>
+	 * if none. This convenience method searches the list of known
+	 * module kinds ({@link #getModuleKinds()}) for the one a matching
+	 * module kind id ({@link IModuleKind#getId()}).
+	 * <p>
+	 * [issue: The terminology should be "module types",
+	 * to make it consistent with server types, etc.]
+	 * </p>
+	 * <p>
+	 * [issue: It does not really make sense for a key parameter
+	 * like id to be null. 
+	 * Null id should be spec'd as illegal, 
+	 * and the implementation should immediately throw an unspecified 
+	 * RuntimeException if null is passed.]
+	 * </p>
+	 * <p>
+	 * [issue: Consider renaming this method findModuleKind 
+	 * (findModuleType) to make it clear that it is searching.]
+	 * </p>
 	 *
-	 * @return org.eclipse.wst.server.core.IModuleKind
+	 * @param the module kind id, or <code>null</code>
+	 * @return the module kind, or <code>null</code> if 
+	 * id is <code>null</code> or there is no module kind
+	 * with the given id
 	 */
 	public static IModuleKind getModuleKind(String id) {
 		if (id == null)
@@ -191,9 +252,25 @@ public class ServerCore {
 	}
 	
 	/**
-	 * Returns a List of all runtime types.
-	 *
-	 * @return java.util.List
+	 * Returns the list of all known runtime types.
+	 * <p>
+	 * Clients must not modify the list that is returned.
+	 * If the set of runtime types changes, the affect on
+	 * the returned list is unspecified.
+	 * </p>
+	 * <p>
+	 * [issue: The list returned is precious. You would not want a client
+	 * to accidentally or malicously whack it. Normal practice is to
+	 * return an array instead of a List, and to return a new copy each call.
+	 * This allows the spec to say that the client can do what they want
+	 * with the result, and that it won't change under foot.
+	 * Another alternative is to return a UnmodifiableList implementation
+	 * so that clients cannot modify. But if you don't copy, you still
+	 * have the problem of the list chaning under foot if a new plug-in
+	 * is installed that happens to define a runtime type (a scenario that
+	 * Eclipse should support).]
+	 * </p>
+	 * @return the list of runtime types (element type: {@link IRuntimeType})
 	 */
 	public static List getRuntimeTypes() {
 		if (runtimeTypes == null)
@@ -202,9 +279,27 @@ public class ServerCore {
 	}
 	
 	/**
-	 * Returns the runtime type with the given id.
+	 * Returns the runtime type with the given id, or <code>null</code>
+	 * if none. This convenience method searches the list of known
+	 * runtime types ({@link #getRuntimeTypes()}) for the one with a matching
+	 * runtime type id ({@link IRuntimeType#getId()}).
+	 * <p>
+	 * [issue: Same issue as with IServerType.
+	 * It does not really make sense for a key parameter
+	 * like id to be null. 
+	 * Null id should be spec'd as illegal, 
+	 * and the implementation should immediately throw an unspecified 
+	 * RuntimeException if null is passed.]
+	 * </p>
+	 * <p>
+	 * [issue: Consider renaming this method findRuntimeType to make
+	 * it clear that it is searching.]
+	 * </p>
 	 *
-	 * @return org.eclipse.wst.server.core.IRuntimeType
+	 * @param the runtime type id, or <code>null</code>
+	 * @return the runtime type, or <code>null</code> if 
+	 * id is <code>null</code> or there is no runtime type
+	 * with the given id
 	 */
 	public static IRuntimeType getRuntimeType(String id) {
 		if (id == null)
@@ -266,9 +361,25 @@ public class ServerCore {
 	}
 
 	/**
-	 * Returns a List of all server types.
-	 *
-	 * @return java.util.List
+	 * Returns the list of all known server types.
+	 * <p>
+	 * Clients must not modify the list that is returned.
+	 * If the set of server types changes, the affect on
+	 * the returned list is unspecified.
+	 * </p>
+	 * <p>
+	 * [issue: The list returned is precious. You would not want a client
+	 * to accidentally or malicously whack it. Normal practice is to
+	 * return an array instead of a List, and to return a new copy each call.
+	 * This allows the spec to say that the client can do what they want
+	 * with the result, and that it won't change under foot.
+	 * Another alternative is to return a UnmodifiableList implementation
+	 * so that clients cannot modify. But if you don't copy, you still
+	 * have the problem of the list chaning under foot if a new plug-in
+	 * is installed that happens to define a server type (a scenario that
+	 * Eclipse should support).]
+	 * </p>
+	 * @return the list of server types (element type: {@link IServerType})
 	 */
 	public static List getServerTypes() {
 		if (serverTypes == null)
@@ -277,9 +388,26 @@ public class ServerCore {
 	}
 	
 	/**
-	 * Returns the server type with the given id.
+	 * Returns the server type with the given id, or <code>null</code>
+	 * if none. This convenience method searches the list of known
+	 * server types ({@link #getServerTypes()}) for the one with a matching
+	 * server type id ({@link IServerType#getId()}).
+	 * <p>
+	 * [issue: It does not really make sense for a key parameter
+	 * like id to be null. 
+	 * Null id should be spec'd as illegal, 
+	 * and the implementation should immediately throw an unspecified 
+	 * RuntimeException if null is passed.]
+	 * </p>
+	 * <p>
+	 * [issue: Consider renaming this method findServerType to make
+	 * it clear that it is searching.]
+	 * </p>
 	 *
-	 * @return org.eclipse.wst.server.core.IServerType
+	 * @param the server type id, or <code>null</code>
+	 * @return the server type, or <code>null</code> if 
+	 * id is <code>null</code> or there is no server type
+	 * with the given id
 	 */
 	public static IServerType getServerType(String id) {
 		if (id == null)
@@ -298,9 +426,27 @@ public class ServerCore {
 	}
 	
 	/**
-	 * Returns a List of all server configuration types.
-	 *
-	 * @return java.util.List
+	 * Returns the list of all known server configuration types.
+	 * <p>
+	 * Clients must not modify the list that is returned.
+	 * If the set of server configuration types changes, the affect on
+	 * the returned list is unspecified.
+	 * </p>
+	 * <p>
+	 * [issue: Same issue as with IServerType.
+	 * The list returned is precious. You would not want a client
+	 * to accidentally or malicously whack it. Normal practice is to
+	 * return an array instead of a List, and to return a new copy each call.
+	 * This allows the spec to say that the client can do what they want
+	 * with the result, and that it won't change under foot.
+	 * Another alternative is to return a UnmodifiableList implementation
+	 * so that clients cannot modify. But if you don't copy, you still
+	 * have the problem of the list chaning under foot if a new plug-in
+	 * is installed that happens to define a server configuration type (a scenario that
+	 * Eclipse should support).]
+	 * </p>
+	 * @return the list of server configuration types
+	 * (element type: {@link IServerConfigurationType})
 	 */
 	public static List getServerConfigurationTypes() {
 		if (serverConfigurationTypes == null)
@@ -309,9 +455,28 @@ public class ServerCore {
 	}
 	
 	/**
-	 * Returns the server configuration type with the given id.
+	 * Returns the server configuration type with the given id, 
+	 * or <code>null</code> if none. This convenience method searches
+	 * the list of known server configuration types
+	 * ({@link #getServerConfigurationTypes()}) for the one a matching
+	 * server id ({@link IServerConfigurationType#getId()}).
+	 * <p>
+	 * [issue: Same issue as with IServerType.
+	 * It does not really make sense for a key parameter
+	 * like id to be null. 
+	 * Null id should be spec'd as illegal, 
+	 * and the implementation should immediately throw an unspecified 
+	 * RuntimeException if null is passed.]
+	 * </p>
+	 * <p>
+	 * [issue: Consider renaming this method findServerConfigurationType
+	 * to make it clear that it is searching.]
+	 * </p>
 	 *
-	 * @return org.eclipse.wst.server.core.IServerConfigurationType
+	 * @param the server configuration type id, or <code>null</code>
+	 * @return the server configuration type, or <code>null</code> if 
+	 * id is <code>null</code> or there is no server configuration type
+	 * with the given id
 	 */
 	public static IServerConfigurationType getServerConfigurationType(String id) {
 		if (id == null)
@@ -330,9 +495,31 @@ public class ServerCore {
 	}
 
 	/**
-	 * Returns a List of all module factories.
-	 *
-	 * @return java.util.List
+	 * Returns the list of all known module module factories.
+	 * <p>
+	 * Clients must not modify the list that is returned.
+	 * If the set of module factories changes, the affect on
+	 * the returned list is unspecified.
+	 * </p>
+	 * <p>
+	 * [issue: The list returned is precious. You would not want a client
+	 * to accidentally or malicously whack it. Normal practice is to
+	 * return an array instead of a List, and to return a new copy each call.
+	 * This allows the spec to say that the client can do what they want
+	 * with the result, and that it won't change under foot.
+	 * Another alternative is to return a UnmodifiableList implementation
+	 * so that clients cannot modify. But if you don't copy, you still
+	 * have the problem of the list chaning under foot if a new plug-in
+	 * is installed that happens to define a module factory (a scenario that
+	 * Eclipse should support).]
+	 * </p>
+	 * <p>
+	 * [issue: Are module factories SPI-side objects or do
+	 * normal clients need access to them? If they are only SPI,
+	 * this method should be moved to the SPI package.]
+	 * </p>
+	 * 
+	 * @return the list of module factories (element type: {@link IModuleFactory})
 	 */
 	public static List getModuleFactories() {
 		if (moduleFactories == null)
@@ -341,9 +528,31 @@ public class ServerCore {
 	}
 	
 	/**
-	 * Returns the module factory.
+	 * Returns the module factory with the given id, or <code>null</code>
+	 * if none. This convenience method searches the list of known
+	 * module factories ({@link #getModuleFactories()}) for the one a matching
+	 * module factory id ({@link IModuleFactory#getId()}).
+	 * <p>
+	 * [issue: It does not really make sense for a key parameter
+	 * like id to be null. 
+	 * Null id should be spec'd as illegal, 
+	 * and the implementation should immediately throw an unspecified 
+	 * RuntimeException if null is passed.]
+	 * </p>
+	 * <p>
+	 * [issue: Consider renaming this method findModuleFactory
+	 * to make it clear that it is searching.]
+	 * </p>
+	 * <p>
+	 * [issue: Are module factories SPI-side objects or do
+	 * normal clients need access to them? If they are only SPI,
+	 * this method should be moved to the SPI package.]
+	 * </p>
 	 *
-	 * @return java.util.List
+	 * @param the module factory id, or <code>null</code>
+	 * @return the module factory, or <code>null</code> if 
+	 * id is <code>null</code> or there is no module factory
+	 * with the given id
 	 */
 	public static IModuleFactory getModuleFactory(String id) {
 		if (id == null)
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IModule.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IModule.java
index 66710ebbb..39824bb80 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IModule.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IModule.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -15,66 +15,181 @@ import org.eclipse.core.runtime.IStatus;
 import org.eclipse.wst.server.core.IModuleType;
 import org.eclipse.wst.server.core.resources.IModuleResource;
 /**
- * A resource (typically a project or folder) that can be deployed to a server.
- * This interface will be subclassed to provide further information about the
- * content.
+ * A module is a unit of "content" that can be published to a
+ * server.
+ * <p>
+ * All modules have a module type, which is fixed for the
+ * lifetime of the module. The set of module types (or
+ * "kinds") is open-ended.
+ * </p>
+ * <p>
+ * All modules are created by module factories
+ * ({@link org.eclipse.wst.server.core.IModuleFactory}).
+ * </p>
+ * <p>
+ * The content of a module is a collection of file and folder
+ * resources in the workspace.
+ * </p>
+ * <p>
+ * In principle, a module exists independent of any
+ * particular server. The same module instance can be associated
+ * with multiple server instances at the same time. That is
+ * why you cannot ask the module which server it's associated
+ * with.
+ * </p>
+ * <p>
+ * [issue: IModule is something that is exposed to normal 
+ * clients; it should move to org.eclipse.wst.server.core package.]
+ * </p>
+ * <p>
+ * [issue: See issues on IModuleType about why it should not
+ * be a superinterface of IModule.]
+ * </p>
+ * <p>
+ * [issue: Equality/identify for modules?]
+ * </p>
+ * <p>
+ * Concrete module types are represented by concrete classes
+ * implementing this interface. The only legitimate reason
+ * to declare a subclass is to implement a module factory.
+ * </p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
+ * 
+ * @since 1.0
  */
 public interface IModule extends IModuleType {
+	
 	/**
-	 * Returns a unique memento to distinguish this module
-	 * from all other module instances on a server.
-	 *
-	 * @return java.lang.String
+	 * Returns the id of this module.
+	 * Each module has a distinct id, used to distinguish this
+	 * module from all other modules in the workspace (and
+	 * within a server). Ids are intended to be used internally
+	 * as keys; they are not intended to be shown to end users.
+	 * 
+	 * @return the module id
 	 */
 	public String getId();
 
 	/**
-	 * Returns an IStatus that validates the contents of this module.
-	 * If there is an error that should block the server from starting,
-	 * (e.g. major errors) it should be returned from this method. This
-	 * method can also be used to return warning for such things as an
-	 * open (and dirty) editor.
+	 * Validates the contents of this module.
+	 * <p>
+	 * [issue: Need to define what the "contents" of a module means.]
+	 * </p>
+	 * <p>
+	 * [issue: Conjecture: Each different type of module prescribes
+	 * legal arrangements of, and the significance of, the files within
+	 * it. This would be spelled out in the spec for the particular
+	 * module types.
+	 * This validate operation is suppose to check the actual
+	 * arrangement of files in this module to see whether they
+	 * meet expectations.
+	 * It's an open question as to how "strenuous" a check this
+	 * is.]
+	 * </p>
+	 * <p>
+	 * [issue: Old comment said: "If there is an error
+	 * that should block the server from starting (e.g. major errors)
+	 * it should be returned from this method. This method can also be used to
+	 * return warning for such things as an open (and dirty) editor."]
+	 * </p>
+	 * <p>
+	 * [issue: All existing implementations of this return null,
+	 * which is illegal.]
+	 * </p>
 	 *
-	 * @return org.eclipse.core.runtime.IStatus
+	 * @return a status object with code <code>IStatus.OK</code> if the given
+	 * module is valid, otherwise a status object indicating what is
+	 * wrong with it
 	 */
 	public IStatus validate();
 
 	/**
-	 * Returns an IStatus that is used to determine if this object can
-	 * be published to the server. Should return an error if there is a
-	 * major problem with the resources, or can be used to return warnings
-	 * on unsaved files, etc.
+	 * Determines whether this module can be published.
+	 * <p>
+	 * [issue: Old comment said: "Returns an IStatus that is used to determine if this object can
+	 * be published to the server." Since the same module can
+	 * be associated with any number of servers, "the server" is
+	 * ill-defined.]
+	 * </p>
+	 * <p>
+	 * [issue: How is this predicate different from validate()?]
+	 * </p>
+	 * <p>
+	 * [issue: All existing implementations of this return null,
+	 * which is illegal.]
+	 * </p>
+	 * <p>
+	 * [issue: Old comment said: "Should return an error if there
+	 * is a major problem with the resources, or can be used to
+	 * return warnings on unsaved files, etc." It is usually
+	 * difficult in principle for core-level infrastructure to
+	 * detect whether there are open editors with unsaved changes.]
+	 * </p>
 	 *
-	 * @return org.eclipse.core.runtime.IStatus
+	 * @return a status object with code <code>IStatus.OK</code> if the given
+	 * module can be published, otherwise a status object indicating what is
+	 * wrong with it
 	 */
 	public IStatus canPublish();
 
 	/**
-	 * Returns the base level resources for this module.
-	 *
-	 * @return IModuleResource[]
+	 * Returns the root resources of this module. All members
+     * belong to this module (as do their members, and so on).
+	 * <p>
+	 * [issue: What are the exact constraints on where these
+	 * resources are located?
+	 * Do they all have to be inside the workspace?
+	 * Do they all have to be in the same project?
+	 * When a folder is included, does that mean the entire
+	 * subtree is published to server?]
+	 * </p>
+	 * 
+	 * @return the members of this module
+	 * @throws CoreException [missing]
 	 */
 	public IModuleResource[] members() throws CoreException;
 
 	/**
-	 * Returns the name of this module.
+	 * Returns the displayable name for this module.
+	 * <p>
+	 * Note that this name is appropriate for the current locale.
+	 * </p>
 	 *
-	 * @return java.lang.String
+	 * @return a displayable name for this module
 	 */
 	public String getName();
 
 	/**
-	 * Returns the id of the factory that created this module.
+	 * Returns the id of the module factory that created this module.
+	 * <p>
+	 * [issue: What is the rationale for exposing the module factory
+	 * at all. Is there a reason a client would want to know this?]
+	 * </p>
+	 * <p>
+	 * [issue: Consider returning the IModuleFactory instead.]
+	 * </p>
 	 *
-	 * @return java.lang.String
+	 * @return the module factory id
 	 */
 	public String getFactoryId();
 
 	/**
-	 * Returns true if this module currently exists, and false if it has
-	 * been deleted or moved and is no longer represented by this module.
+	 * Returns whether this module currently exists.
+	 * <p>
+	 * [issue: The method touches on the important issue
+	 * of when a module ceases to exist. Need to explain
+	 * the full lifecycle of a module.
+	 * Should it be synonymous with the module root 
+	 * IContainer.exists()? That is, the module exists
+	 * as long as the IContainer that holds all its module
+	 * resources exists()?]
+	 * </p>
 	 *
-	 * @return boolean
+	 * @return <code>true</code> this module currently exists, and
+	 * <code>false</code> if it has been deleted or moved
 	 */
 	public boolean exists();
 
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IModuleFactoryDelegate.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IModuleFactoryDelegate.java
index 887631745..29de29373 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IModuleFactoryDelegate.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IModuleFactoryDelegate.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -12,40 +12,129 @@ package org.eclipse.wst.server.core.model;
 
 import java.util.List;
 /**
+ * A module factory delegate provides a mechanism for discovering
+ * modules. A module factory delegate is specified by the
+ * <code>class</code> attribute of a <code>moduleFactories</code> extension.
+ * <p>
+ * When the module factory needs to be given a delegate, the delegate class
+ * specified for the module factory is instantiated with a 0-argument
+ * constructor.
+ * </p>
+ * <p>
+ * [issue: 2 differences from server delegate.
+ * (1) module factory delegate is associated with the module factory
+ * itself (server delegates are associated with each server instance
+ * (2) the module factory delegate has no backpoint to its IModuleFactory.
+ * The first is ok; the second is problematic because there is
+ * protocol on IModuleFactory that the delegate might need, such
+ * as the module factory id. Should add an initialize(IModuleFactory)
+ * method and spec that initialize is called at creation time.]
+ * </p>
+ * <p>
+ * Module factory delegates may keep state in instance fields, but that state is
+ * transient and will not be persisted across workbench sessions.
+ * </p>
+ * <p>
+ * [issue: Since service providers must implement this class, it is
+ * more flexible to provide an abstract class than an interface. It is
+ * not a breaking change to add non-abstract methods to an abstract class.]
+ * </p>
+ * <p>
+ * This interface is intended to be implemented by clients.
+ * </p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
  * 
+ * @see org.eclipse.wst.server.core.IModuleFactory#getDelegate()
+ * @since 1.0
  */
 public interface IModuleFactoryDelegate {
+
 	/**
-	 * Gets a module from a memento.
+	 * Finds a module create by this factory with the given id.
+	 * See the specification of
+	 * {@link org.eclipse.wst.server.core.IModuleFactory#getModule(String)}
+	 * for further details. 
+	 * <p>
+	 * This method is normally called by the web server core framework,
+	 * in response to a call to {@link IModuleFactory#getModule(String)}.
+	 * Clients (other than the delegate) should never call this method.
+	 * </p>
 	 * 
-	 * @param memento java.lang.String
-	 * @return org.eclipse.wst.server.core.model.IModule
+	 * @param id the module id
+	 * @return the module with the given id, or <code>null</code>
+	 * if none
 	 */
 	public IModule getModule(String memento);
 
 	/**
-	 * Return all modules that are available to be added
-	 * to servers. This method might look through projects
-	 * to find modules or may return modules from
-	 * other sources.
-	 *
-	 * @return java.util.List
+	 * Return all modules created by this factory.
+	 * See the specification of
+	 * {@link org.eclipse.wst.server.core.IModuleFactory#getModules()}
+	 * for further details. 
+	 * <p>
+	 * This method is normally called by the web server core framework,
+	 * in response to a call to {@link IModuleFactory#getModules()}.
+	 * Clients (other than the delegate) should never call this method.
+	 * </p>
+	 * <p>
+	 * [issue: Consistency: IServer.getModules returns IModule[] rather than List<IModule>.]
+	 * </p>
+	 * <p>
+	 * [issue: The list returned is precious. You would not want a client
+	 * to accidentally or malicously whack it. Normal practice is to
+	 * return an array instead of a List, and to return a new copy each call.
+	 * This allows the spec to say that the client can do what they want
+	 * with the result, and that it won't change under foot.
+	 * Another alternative is to return a UnmodifiableList implementation
+	 * so that clients cannot modify.
+	 * The trick here is that this is a method that a service
+	 * provider is supposed to implement. It is unwise to trust a service
+	 * provider to return an UnmodifiableList, it would be
+	 * wasteful to add an unmodifiable wrapper on every call and incorrect
+	 * to cache one. Returning an IModule[] seems the best
+	 * option.]
+	 * </p>
+	 * 
+	 * @return a possibly-empty list of modules (element type:
+	 * {@link IModule}
 	 */
 	public List getModules();
 	
 	/**
-	 * Add a listener for modules that are added/removed from this
-	 * factory.
-	 * 
-	 * @param listener org.eclipse.wst.server.core.model.IModuleFactoryListener
+	 * Adds the given listener to this module factory.
+	 * Once registered, a listener starts receiving notification of 
+	 * modules are added/removed. The listener continues to receive
+	 * notifications until it is removed.
+	 * <p>
+	 * This method is normally called by the web server core framework,
+	 * in response to a call to
+	 * {@link IModuleFactory#addModuleFactoryListener(IModuleFactoryListener)}.
+	 * Clients (other than the delegate) should never call this method.
+	 * </p>
+	 * <p>
+	 * [issue: Duplicate server listeners should be ignored.]
+	 * </p>
+	 *
+	 * @param listener the module factory listener to add
+	 * @see #removeModuleFactoryListener(IModuleFactoryListener)
 	 */
 	public void addModuleFactoryListener(IModuleFactoryListener listener);
 
 	/**
-	 * Add a listener for modules that are added/removed from this
-	 * factory.
-	 * 
-	 * @param listener org.eclipse.wst.server.core.model.IModuleFactoryListener
+	 * Removes the given listener from this module factory.
+	 * Has no effect if the listener is not registered.
+	 * <p>
+	 * This method is normally called by the web server core framework,
+	 * in response to a call to
+	 * {@link IModuleFactory#removeModuleFactoryListener(IModuleFactoryListener)}.
+	 * Clients (other than the delegate) should never call this method.
+	 * </p>
+	 *
+	 * @param listener the module factory listener to remove
+	 * @see #addModuleFactoryListener(IModuleFactoryListener)
 	 */
 	public void removeModuleFactoryListener(IModuleFactoryListener listener);
 }
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IRestartableModule.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IRestartableModule.java
index 16ae76bde..d10bb9499 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IRestartableModule.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IRestartableModule.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -13,18 +13,45 @@ package org.eclipse.wst.server.core.model;
 import org.eclipse.core.runtime.CoreException;
 import org.eclipse.core.runtime.IProgressMonitor;
 import org.eclipse.core.runtime.IStatus;
+import org.eclipse.wst.server.core.IServer;
 /**
- * An interface for a server that allows individual modules
- * to be restarted while the server is running. This interface
- * can be implemented by a server to allow it's
- * individual modules to be restarted one at a time.
+ * Represents a specialized server delegate for servers capable of
+ * restarting individual modules while the server is running.
+ * <p>
+ * [issue: This should be folded in to IServerDelegate. If
+ * the server delegate is represented as an abstract class as
+ * recommended elsewhere, all these methods could have default
+ * implementations appropriate for a server that does not support 
+ * restartable modules. IServerDelegate.supportsModuleRestart()
+ * could answer whether module restart was possible in pinciple
+ * (false by default).]
+ * </p>
+ * <p>
+ * Concrete module types are represented by concrete classes
+ * implementing this interface. The only legitimate reason
+ * to declare a subclass is to implement a module factory.
+ * </p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
+ * 
+ * @since 1.0
  */
 public interface IRestartableModule extends IServerDelegate {
+	
 	/**
-	 * Returns true if the given module can be restarted.
+	 * Returns whether the given module can be restarted.
+	 * <p>
+	 * [issue: It's unclear whether this operations is guaranteed to be fast
+	 * or whether it could involve communication with any actual
+	 * server. If it is not fast, the method should take a progress
+	 * monitor.]
+	 * </p>
 	 *
-	 * @param module org.eclipse.wst.server.core.model.IModule
-	 * @return boolean
+	 * @param module the module
+	 * @return <code>true</code> if the given module can be
+	 * restarted, and <code>false</code> otherwise 
 	 */
 	public boolean canRestartModule(IModule module);
 
@@ -39,12 +66,44 @@ public interface IRestartableModule extends IServerDelegate {
 	public boolean isModuleRestartNeeded(IModule module);
 
 	/**
-	 * Restart the user module on the server. This method should
-	 * update the module sync state and fire an event for the
-	 * module.
-	 *
-	 * @param module org.eclipse.wst.server.core.model.IModule
-	 * @param monitor org.eclipse.core.runtime.IProgressMonitor
+	 * Asynchronously restarts the given module on the server.
+	 * See the specification of 
+	 * {@link IServer#synchronousModuleRestart(IModule, IProgressMonitor)}
+	 * for further details. 
+	 * <p>
+	 * This method is called by the web server core framework,
+	 * in response to a call to <code>IServer.synchronousModuleRestart</code>.
+	 * Clients should never call this method.
+	 * </p>
+	 * <p>
+	 * The implementation should update the module sync state and fire
+	 * an event for the module.
+	 * </p>
+	 * <p>
+	 * [issue: It should probably be spec'd to throw an exception error if the
+	 * given module is not associated with the server.]
+	 * </p>
+	 * <p>
+	 * [issue: Since this method is ascynchronous, is there
+	 * any need for the progress monitor?]
+	 * </p>
+	 * <p>
+	 * [issue: Since this method is ascynchronous, how can
+	 * it return a meaningful IStatus? 
+	 * And IServer.synchronousModuleRestart throws CoreException
+	 * if anything goes wrong.]
+	 * </p>
+	 * <p>
+	 * [issue: If the module was just published to the server
+	 * and had never been started, would is be ok to "start"
+	 * the module using this method?]
+	 * </p>
+	 * 
+	 * @param module the module to be started
+	 * @param monitor a progress monitor, or <code>null</code> if progress
+	 *    reporting and cancellation are not desired
+	 * @return status object
+	 * @exception CoreException if an error occurs while trying to restart the module
 	 */
 	public IStatus restartModule(IModule module, IProgressMonitor monitor) throws CoreException;
 }
\ No newline at end of file
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IRuntimeDelegate.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IRuntimeDelegate.java
index 0e229c05f..a7315ae93 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IRuntimeDelegate.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IRuntimeDelegate.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -13,17 +13,109 @@ package org.eclipse.wst.server.core.model;
 import org.eclipse.core.runtime.IStatus;
 import org.eclipse.wst.server.core.IRuntime;
 /**
- * An interface for defining server runtimes.
+ * A runtime delegate provides the implementation for various 
+ * generic and server-type-specific operations for a specific type of runtime.
+ * A runtime delegate is specified by the
+ * <code>class</code> attribute of a <code>runtimeTypes</code> extension.
+ * <p>
+ * When the runtime instance needs to be given a delegate, the delegate class
+ * specified for the runtime type is instantiated with a 0-argument constructor
+ * and primed with <code>delegate.initialize(runtime)</code>, 
+ * which it is expected to hang on to. Later, when
+ * <code>delegate.dispose()</code> is called as the runtime instance is
+ * being discarded, the delegate is expected to let go of the runtime instance.
+ * </p>
+ * <p>
+ * Runtime delegates may keep state in instance fields, but that state is
+ * transient and will not be persisted across workbench sessions.
+ * </p>
+ * <p>
+ * [issue: Runtime delegates can read runtime attributes via
+ * IRuntime; runtime working copy delegates can also set them
+ * via IRuntimeWorkingCopy. However, current implementation does
+ * not serialize any attributes other than the ones server core
+ * knows about. So it's unclear whether there is any intent
+ * to support attributes that are runtime-type-specific.]
+ * </p>
+ * <p>
+ * [issue: Since service providers must implement this class, it is
+ * more flexible to provide an abstract class than an interface. It is
+ * not a breaking change to add non-abstract methods to an abstract class.]
+ * </p>
+ * <p>
+ * [issue: As mentioned on IRuntime.getDelegate(), 
+ * exposing IRuntimeDelegate to clients of IRuntime
+ * is confusing and dangerous. Instead, replace IRuntime.getDelegate()
+ * with something like IRuntime.getRuntimeExtension() which
+ * returns an IRuntimeExtension. The implementation of
+ * IRuntime.getRuntimeExtension() should forward to getRuntimeExtension()
+ * declared here. IRuntimeExtension is an * "marker" interface that
+ * runtime providers would implement or extend only if they want to expose
+ * additional API for their runtime type. That way IRuntimeDelegate
+ * can be kept entirely on the SPI side, out of view from 
+ * clients.]
+ * </p>
+ * <p>
+ * This interface is intended to be implemented only by clients
+ * to extend the <code>runtimeTypes</code> extension point.
+ * </p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
+ * 
+ * @see IRuntime#getDelegate()
+ * @since 1.0
  */
 public interface IRuntimeDelegate {
+	
+	/**
+	 * Initializes this runtime delegate with its life-long runtime instance.
+	 * <p>
+	 * This method is called by the web server core framework.
+	 * Clients should never call this method.
+	 * </p>
+	 * <p>
+	 * Implementations are expected to hang on to a reference to the runtime.
+	 * </p>
+	 * <p>
+	 * [issue: The class attribute of the runtimeTypes extension point
+	 * must stipulate that the class must have a public 0-arg constructor
+	 * in addition to implementing IRuntimeDelegate.]
+	 * </p>
+	 * 
+	 * @param runtime the runtime instance
+	 */
 	public void initialize(IRuntime runtime);
 
 	/**
-	 * Return the validation status of the runtime.
-	 * 
-	 * @return
+	 * Validates this runtime instance. See the specification of
+	 * {@link IRuntime#validate()} for further details. 
+	 * <p>
+	 * This method is called by the web server core framework,
+	 * in response to a call to <code>IRuntime.validate()</code>.
+	 * Clients should never call this method.
+	 * </p>
+	 * <p>
+	 * [issue: see issues flagged on IRuntime.validate().]
+	 * </p>
+	 *
+	 * @return a status object with code <code>IStatus.OK</code> if this
+	 * runtime is valid, otherwise a status object indicating what is
+	 * wrong with it
 	 */
 	public IStatus validate();
 
+	/**
+	 * Disposes of this runtime delegate.
+	 * <p>
+	 * This method is called by the web server core framework.
+	 * Clients should never call this method.
+	 * </p>
+	 * <p>
+	 * Implementations are expected to let go of the delegate's reference
+	 * to the runtime, deregister listeners, etc.
+	 * </p>
+	 */
 	public void dispose();
 }
\ No newline at end of file
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IRuntimeWorkingCopyDelegate.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IRuntimeWorkingCopyDelegate.java
index c10da3e1f..fc7bb8af1 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IRuntimeWorkingCopyDelegate.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IRuntimeWorkingCopyDelegate.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -14,24 +14,151 @@ import org.eclipse.core.runtime.IProgressMonitor;
 
 import org.eclipse.wst.server.core.IRuntimeWorkingCopy;
 /**
- * An interface for defining server runtimes.
+ * A runtime working copy delegate provides the implementation for various 
+ * generic and runtime-type-specific operations on runtime working copies
+ * for a specific type of runtime.
+ * A runtime working copy delegate is specified by the
+ * <code>workingCopyClass</code> attribute of a <code>runtimeTypes</code>
+ * extension.
+ * <p>
+ * When the runtime working copy instance needs to be given a delegate, the
+ * working copy delegate class specified for the runtime type is instantiated
+ * with a 0-argument constructor and primed with
+ * <code>delegate.initialize(runtime)</code>, 
+ * which it is expected to hang on to.
+ * Later, when <code>delegate.handleSave</code> is called,
+ * the working copy is expected to [TBD]. Finally, when
+ * <code>delegate.dispose()</code> is called as the runtime working copy is
+ * being discarded, the delegate is expected to let go of the runtime working
+ * copy.
+ * </p>
+ * <p>
+ * Runtime working copy delegates may keep state in instance fields, but that
+ * state is transient and will not be persisted across workbench sessions.
+ * </p>
+ * <p>
+ * [issue: Runtime delegates can read runtime attributes via
+ * IRuntime; runtime working copy delegates can also set them
+ * via IRuntimeWorkingCopy. However, current implementation does
+ * not serialize any attributes other than the ones server core
+ * knows about. So it's unclear whether there is any intent
+ * to support attributes that are runtime-type-specific.]
+ * </p>
+ * <p>
+ * [issue: IRuntimeWorkingCopyDelegate extending IRuntimeDelegate has
+ * some undesirable properties: the type has 2 initialize methods
+ * public void initialize(IRuntimeWorkingCopy workingCopy)
+ * public void initialize(IRuntime runtime).
+ * It would be simpler if these types were unrelated.
+ * ]
+ * </p>
+ * <p>
+ * [issue: Since service providers must implement this class, it is
+ * more flexible to provide an abstract class than an interface. It is
+ * not a breaking change to add non-abstract methods to an abstract class.]
+ * </p>
+ * <p>
+ * [issue: As mentioned on IRuntimeWorkingCopy.getWorkingCopyDelegate(), 
+ * exposing IRuntimeWorkingCopyDelegate to clients of IRuntimeWorkingCopy
+ * is confusing and dangerous. Instead, replace IRuntimeWorkingCopy.getWorkingCopyDelegate()
+ * with something like IRuntimeWorkingCopy.getRuntimeWorkingCopyExtension() which
+ * returns an IRuntimeWorkingCopyExtension. The implementation of
+ * IRuntimeWorkingCopy.getRuntimeWorkingCopyExtension() should forward to
+ * getRuntimeWorkingCopyExtension()
+ * declared here. IRuntimeWorkingCopyExtension is an * "marker" interface that
+ * runtime providers would implement or extend only if they want to expose
+ * additional API on working copies for their runtime type. That way 
+ * IRuntimeWorkingCopyDelegate can be kept entirely on the SPI side,
+ * out of view from clients.]
+ * </p>
+ * <p>
+ * This interface is intended to be implemented only by clients
+ * to extend the <code>runtimeTypes</code> extension point.
+ * </p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
+ * 
+ * @see org.eclipse.wst.server.core.IServerWorkingCopy#getWorkingCopyDelegate()
+ * @since 1.0
  */
 public interface IRuntimeWorkingCopyDelegate extends IRuntimeDelegate {
+	/**
+	 * Constant (value 0) indicating the pre-save call to 
+	 * {@link #handleSave(byte, IProgressMonitor)}.
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 * <p>
+	 * [issue: same constant is duplicated on IServerWorkingCopyDelegate.]
+	 * </p>
+	 */
 	public static final byte PRE_SAVE = 0;
+	
+	/**
+	 * Constant (value 1) indicating the post-save call to 
+	 * {@link #handleSave(byte, IProgressMonitor)}.
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 * <p>
+	 * [issue: same constant is duplicated on IServerWorkingCopyDelegate.]
+	 * </p>
+	 */
 	public static final byte POST_SAVE = 1;
 
-	public void initialize(IRuntimeWorkingCopy runtime);
+	/**
+	 * Initializes this runtime working copy delegate with its life-long server
+	 * working copy.
+	 * <p>
+	 * This method is called by the web server core framework.
+	 * Clients should never call this method.
+	 * </p>
+	 * <p>
+	 * Implementations are expected to hang on to a reference to the
+	 * runtime working copy.
+	 * </p>
+	 * <p>
+	 * [issue: The workingCopyClass attribute of the runtimeTypes extension point
+	 * must stipulate that the class must have a public 0-arg constructor
+	 * in addition to implementing IRuntimeWorkingCopyDelegate.]
+	 * </p>
+	 * 
+	 * @param workingCopy the runtime working copy
+	 */
+	public void initialize(IRuntimeWorkingCopy workingCopy);
 	
+	/**
+	 * <p>
+	 * [issue: Who would call this method, and what does it do?]
+	 * </p>
+	 */
 	public void setDefaults();
 	
 	/**
-	 * Handle a save of this runtime. This method is called when the runtime
-	 * working copy save() method is invoked and can be used to resolve
-	 * calculated fields or perform other operations related to the changes
-	 * that are being made.
+	 * Handles a save of this runtime working copy. This method is called
+	 * when the runtime working copy <code>save</code> method
+	 * is invoked and can be used to resolve calculated fields or perform
+	 * other operations related to the changes that are being made.
+	 * <p>
+	 * This method is called by the web server core framework.
+	 * Clients should never call this method.
+	 * </p>
+	 * <p>
+	 * [issue: It's unclear why this method is necessary.]
+	 * </p>
+	 * <p>
+	 * [issue: Explain how PRE_SAVE and POST_SAVE fit in.]
+	 * </p>
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.
+	 * Or, in this case, a boolean.]
+	 * </p>
 	 * 
-	 * @param id
-	 * @param monitor
+	 * @param id one of {@link #PRE_SAVE} or {@link #POST_SAVE}
+	 * @param monitor a progress monitor, or <code>null</code> if progress
+	 *    reporting and cancellation are not desired
 	 */
 	public void handleSave(byte id, IProgressMonitor monitor);
 }
\ No newline at end of file
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IServerDelegate.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IServerDelegate.java
index 1fb2ce68c..b45b08dc9 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IServerDelegate.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IServerDelegate.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -16,33 +16,118 @@ import org.eclipse.core.runtime.CoreException;
 import org.eclipse.core.runtime.IProgressMonitor;
 import org.eclipse.core.runtime.IStatus;
 import org.eclipse.debug.core.ILaunchConfigurationWorkingCopy;
+import org.eclipse.wst.server.core.IServer;
 import org.eclipse.wst.server.core.IServerState;
 import org.eclipse.wst.server.core.ITask;
 import org.eclipse.wst.server.core.resources.IModuleResourceDelta;
 /**
+ * A server delegate provides the implementation for various 
+ * generic and server-type-specific operations for a specific type of server.
+ * A server delegate is specified by the
+ * <code>class</code> attribute of a <code>serverTypes</code> extension.
+ * <p>
+ * When the server instance needs to be given a delegate, the delegate class
+ * specified for the server type is instantiated with a 0-argument constructor
+ * and primed with <code>delegate.initialize(((IServerState)server)</code>, 
+ * which it is expected to hang on to. Later, when
+ * <code>delegate.dispose()</code> is called as the server instance is
+ * being discarded, the delegate is expected to let go of the server instance.
+ * </p>
+ * <p>
+ * Server delegates may keep state in instance fields, but that state is
+ * transient and will not be persisted across workbench sessions.
+ * </p>
+ * <p>
+ * [issue: Server delegates can read server attributes via
+ * IServer; server working copy delegates can also set them
+ * via IServerWorkingCopy. However, current implementation does
+ * not serialize any attributes other than the ones server core
+ * knows about. So it's unclear whether there is any intent
+ * to support attributes that are server-type-specific.]
+ * </p>
+ * <p>
+ * [issue: Since service providers must implement this class, it is
+ * more flexible to provide an abstract class than an interface. It is
+ * not a breaking change to add non-abstract methods to an abstract class.]
+ * </p>
+ * <p>
+ * [issue: As mentioned on IServer.getDelegate(), 
+ * exposing IServerDelegate to clients of IServer
+ * is confusing and dangerous. Instead, replace IServer.getDelegate()
+ * with something like IServer.getServerExtension() which
+ * returns an IServerExtension. The implementation of
+ * IServer.getServerExtension() should forward to getServerExtension()
+ * declared here. IServerExtension is an * "marker" interface that
+ * server providers would implement or extend only if they want to expose
+ * additional API for their server type. That way IServerDelegate
+ * can be kept entirely on the SPI side, out of view from 
+ * clients.]
+ * </p>
+ * <p>
+ * This interface is intended to be implemented only by clients
+ * to extend the <code>serverTypes</code> extension point.
+ * </p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
  * 
+ * @see IServer#getDelegate()
+ * @since 1.0
  */
 public interface IServerDelegate {
+	
 	/**
-	 * Called when the server is loaded as a model object.
+	 * Initializes this server delegate with its life-long server instance.
+	 * <p>
+	 * This method is called by the web server core framework.
+	 * Clients should never call this method.
+	 * </p>
+	 * <p>
+	 * Implementations are expected to hang on to a reference to the server.
+	 * </p>
+	 * <p>
+	 * [issue: The class attribute of the serverTypes extension point
+	 * must stipulate that the class must have a public 0-arg constructor
+	 * in addition to implementing IServerDelegate.]
+	 * </p>
+	 * 
+	 * @param server the server instance
 	 */
-	public void initialize(IServerState liveServer);
+	public void initialize(IServerState server);
 
 	/**
-	 * Called when this server resource has become invalid or no longer
-	 * required and is being deregistered or dicarded. This method can
-	 * be used to remove listeners, etc.
+	 * Disposes of this server delegate.
+	 * <p>
+	 * This method is called by the web server core framework.
+	 * Clients should never call this method.
+	 * </p>
+	 * <p>
+	 * Implementations are expected to let go of the delegate's reference
+	 * to the server, deregister listeners, etc.
+	 * </p>
 	 */
 	public void dispose();
 	
 	/**
 	 * Returns the publisher that can be used to publish the
-	 * given module. If the module should never
-	 * be published to the server, it may return null.
+	 * given module to this server.
+	 * <p>
+	 * [issue: Although getPublisher is also found on
+	 * on IServer, it probably does not belong there. Here
+	 * on the SPI side is where it makes sense: it allows the
+	 * IPublisher implementation to be determined (and implemented)
+	 * in a server-type-specific manner.]
+	 * </p>
+	 * <p>
+	 * [issue: Explain the role of the parents parameter.]
+	 * </p>
 	 *
-	 * @param parents java.util.List
-	 * @param module org.eclipse.wst.server.core.model.IModule
-	 * @return org.eclipse.wst.server.core.model.IPublisher
+	 * @param parents the parent modules (element type: <code>IModule</code>)
+	 * @param module the module
+	 * @return the publisher that handles the given module, or
+	 * <code>null</code> if the module cannot be published to
+	 * this server
 	 */
 	public IPublisher getPublisher(List parents, IModule module);
 
@@ -51,6 +136,8 @@ public interface IServerDelegate {
 	 * quickly. If any republishing must occur, the relevant in-sync
 	 * methods should return a new value. If the server must be restarted,
 	 * the isRestartNeeded() method should return true.
+	 * 
+	 * @see IServer#updateConfiguration()
 	 */
 	public void updateConfiguration();
 
@@ -101,32 +188,72 @@ public interface IServerDelegate {
 	public IStatus publishStop(IProgressMonitor monitor);
 	
 	/**
+	 * Returns whether the specified module modifications could be made to this
+	 * server at this time. See the specification of
+	 * {@link IServer#canModifyModules(IModule[], IModule[])}
+	 * for further details. 
+	 * <p>
+	 * This method is called by the web server core framework,
+	 * in response to a call to <code>IServer.canModifyModules</code>.
+	 * Clients should never call this method.
+	 * </p>
+	 * <p>
+	 * [issue: See IServer.canModifyModules(IModule[], IModule[]).]
+	 * </p>
+	 *
+	 * @param add a possibly-empty list of modules to add
+	 * @param remove a possibly-empty list of modules to remove
+	 * @param monitor a progress monitor, or <code>null</code> if progress
+	 *    reporting and cancellation are not desired
+	 * @return <code>true</code> if the proposed modifications
+	 * look feasible, and <code>false</code> otherwise
 	 * Returns true if this module can be added to this
 	 * configuration at the current time, and false otherwise.
-	 *
-	 * <p>This method may decide based on the type of module
-	 * or refuse simply due to reaching a maximum number of
-	 * modules or other criteria.</p>
-	 *
-	 * @param add org.eclipse.wst.server.core.model.IModule[]
-	 * @param remove org.eclipse.wst.server.core.model.Module[]
-	 * @return boolean
 	 */
 	public IStatus canModifyModules(IModule[] add, IModule[] remove);
 
 	/**
-	 * Returns the modules that are in this configuration.
+	 * Returns the list of modules that are associated with
+	 * this server. See the specification of
+	 * {@link IServer#getModules()} for further details. 
+	 * <p>
+	 * This method is called by the web server core framework,
+	 * in response to a call to <code>IServer.getModules</code>.
+	 * Clients should never call this method.
+	 * </p>
+	 * <p>
+	 * [issue: Where does the delegate get these objects from,
+	 * especially at the start of a follow-on session?]
+	 * </p>
 	 *
-	 * @return org.eclipse.wst.server.core.model.IModule[]
+	 * @return a possibly-empty list of modules
 	 */
 	public IModule[] getModules();
 	
 	/**
-	 * Returns the current state of the given module. See
-	 * class header for MODULE_XXX constants.
+	 * Returns the current state of the given module on this server.
+	 * Returns <code>MODULE_STATE_UNKNOWN</code> if the module
+	 * is not among the ones associated with this server.
+	 * See the specification of {@link IServer#getModuleState(IModule)}
+	 * for further details. 
+	 * <p>
+	 * This method is called by the web server core framework,
+	 * in response to a call to <code>IServer.getModuleState</code>.
+	 * Clients should never call this method.
+	 * </p>
+	 * <p>
+	 * [issue: It's unclear whether this operations is guaranteed to be fast
+	 * or whether it could involve communication with any actual
+	 * server. If it is not fast, the method should take a progress
+	 * monitor.]
+	 * </p>
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
 	 *
-	 * @param module org.eclipse.wst.server.core.model.IModule
-	 * @return byte
+	 * @param module the module
+	 * @return one of the module state (<code>MODULE_STATE_XXX</code>)
+	 * constants declared on {@link IServer}
 	 */
 	public byte getModuleState(IModule module);
 
@@ -135,9 +262,7 @@ public interface IServerDelegate {
 	 * within this configuration occur. Return any necessary commands to repair
 	 * or modify the server configuration in response to these changes.
 	 * 
-	 * @param org.eclipse.wst.server.core.model.IModuleFactoryEvent[]
-	 * @param org.eclipse.wst.server.core.model.IModuleEvent[]
-	 * @return org.eclipse.wst.server.core.model.ITask[]
+	 * @see IServer#getRepairCommands(IModuleFactoryEvent[], IModuleEvent[])
 	 */
 	public ITask[] getRepairCommands(IModuleFactoryEvent[] factoryEvent, IModuleEvent[] moduleEvent);
 	
@@ -150,8 +275,7 @@ public interface IServerDelegate {
 	 * To obtain the full module tree, this method may be
 	 * recursively called on the children.</p>
 	 *
-	 * @param module org.eclipse.wst.server.core.model.IModule
-	 * @return java.util.List
+	 * @see IServer#getChildModules(IModule)
 	 */
 	public List getChildModules(IModule module);
 
@@ -174,11 +298,13 @@ public interface IServerDelegate {
 	 * always return the topmost parent module(s), even if
 	 * there are a few levels (a heirarchy) of modules.</p>
 	 *
-	 * @param module org.eclipse.wst.server.core.model.IModule
-	 * @return java.util.List
-	 * @throws org.eclipse.core.runtime.CoreException
+	 * @see IServer#getParentModules(IModule)
 	 */
 	public List getParentModules(IModule module) throws CoreException;
 	
+	/**
+	 * 
+	 * @see IServer#setLaunchDefaults(ILaunchConfigurationWorkingCopy)
+	 */
 	public void setLaunchDefaults(ILaunchConfigurationWorkingCopy workingCopy);
 }
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IServerWorkingCopyDelegate.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IServerWorkingCopyDelegate.java
index e4b5783a3..6013196b7 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IServerWorkingCopyDelegate.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/model/IServerWorkingCopyDelegate.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -15,40 +15,165 @@ import org.eclipse.core.runtime.IProgressMonitor;
 
 import org.eclipse.wst.server.core.IServerWorkingCopy;
 /**
- *
+ * A server working copy delegate provides the implementation for various 
+ * generic and server-type-specific operations on server working copies
+ * for a specific type of server.
+ * A server working copy delegate is specified by the
+ * <code>workingCopyClass</code> attribute of a <code>serverTypes</code>
+ * extension.
+ * <p>
+ * When the server working copy instance needs to be given a delegate, the
+ * working copy delegate class specified for the server type is instantiated
+ * with a 0-argument constructor and primed with
+ * <code>delegate.initialize(serverWorkingCopy)</code>, 
+ * which it is expected to hang on to.
+ * Later, when <code>delegate.handleSave</code> is called,
+ * the working copy is expected to [TBD]. Finally, when
+ * <code>delegate.dispose()</code> is called as the server working copy is
+ * being discarded, the delegate is expected to let go of the server working
+ * copy.
+ * </p>
+ * <p>
+ * Server working copy delegates may keep state in instance fields, but that
+ * state is transient and will not be persisted across workbench sessions.
+ * </p>
+ * <p>
+ * [issue: Server delegates can read server attributes via
+ * IServer; server working copy delegates can also set them
+ * via IServerWorkingCopy. However, current implementation does
+ * not serialize any attributes other than the ones server core
+ * knows about. So it's unclear whether there is any intent
+ * to support attributes that are server-type-specific.]
+ * </p>
+ * <p>
+ * [issue: IServerWorkingCopyDelegate extending IServerDelegate has
+ * some undesirable properties: the type has 2 initialize methods
+ * public void initialize(IServerWorkingCopy workingCopy)
+ * public void initialize(IServerState server).
+ * It would be simpler if these types were unrelated.
+ * ]
+ * </p>
+ * <p>
+ * [issue: Since service providers must implement this class, it is
+ * more flexible to provide an abstract class than an interface. It is
+ * not a breaking change to add non-abstract methods to an abstract class.]
+ * </p>
+ * <p>
+ * [issue: As mentioned on IServerWorkingCopy.getWorkingCopyDelegate(), 
+ * exposing IServerWorkingCopyDelegate to clients of IServerWorkingCopy
+ * is confusing and dangerous. Instead, replace IServerWorkingCopy.getWorkingCopyDelegate()
+ * with something like IServerWorkingCopy.getServerWorkingCopyExtension() which
+ * returns an IServerWorkingCopyExtension. The implementation of
+ * IServerWorkingCopy.getServerWorkingCopyExtension() should forward to
+ * getServerWorkingCopyExtension()
+ * declared here. IServerWorkingCopyExtension is an * "marker" interface that
+ * server providers would implement or extend only if they want to expose
+ * additional API on working copies for their server type. That way 
+ * IServerWorkingCopyDelegate can be kept entirely on the SPI side,
+ * out of view from clients.]
+ * </p>
+ * <p>
+ * This interface is intended to be implemented only by clients
+ * to extend the <code>serverTypes</code> extension point.
+ * </p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
+ * 
+ * @see org.eclipse.wst.server.core.IServerWorkingCopy#getWorkingCopyDelegate()
+ * @since 1.0
  */
 public interface IServerWorkingCopyDelegate extends IServerDelegate {
+	
+	/**
+	 * Constant (value 0) indicating the pre-save call to 
+	 * {@link #handleSave(byte, IProgressMonitor)}.
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 */
 	public static final byte PRE_SAVE = 0;
+	
+	/**
+	 * Constant (value 1) indicating the post-save call to 
+	 * {@link #handleSave(byte, IProgressMonitor)}.
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.]
+	 * </p>
+	 */
 	public static final byte POST_SAVE = 1;
 
 	/**
-	 * Called when the server is loaded as a model object.
+	 * Initializes this server working copy delegate with its life-long server
+	 * working copy.
+	 * <p>
+	 * This method is called by the web server core framework.
+	 * Clients should never call this method.
+	 * </p>
+	 * <p>
+	 * Implementations are expected to hang on to a reference to the
+	 * server working copy.
+	 * </p>
+	 * <p>
+	 * [issue: The workingCopyClass attribute of the serverTypes extension point
+	 * must stipulate that the class must have a public 0-arg constructor
+	 * in addition to implementing IServerWorkingCopyDelegate.]
+	 * </p>
+	 * 
+	 * @param workingCopy the server working copy
 	 */
 	public void initialize(IServerWorkingCopy workingCopy);
 
+	/**
+	 * <p>
+	 * [issue: Who would call this method, and what does it do?]
+	 * </p>
+	 */
 	public void setDefaults();
 
 	/**
-	 * Add the given module to this configuration. The
-	 * module must exist, should not already be deployed
-	 * within the configuration, and canAddProject()
-	 * should have returned true. The configuration must assume
-	 * any default settings and add the module without any UI.
+	 * Modifies the list of modules associated with the server.
+	 * See the specification of
+	 * {@link IServerWorkingCopy#modifyModules(IModule[], IModule[], IProgressMonitor)}
+	 * for further details.
+	 * <p>
+	 * This method is called by the web server core framework,
+	 * in response to a call to <code>IServerWorkingCopy.modifyModules</code>.
+	 * Clients should never call this method.
+	 * </p>
 	 *
-	 * @param add org.eclipse.wst.server.core.model.IModule[]
-	 * @param remove org.eclipse.wst.server.core.model.IModule[] 
-	 * @param monitor org.eclipse.core.runtime.IProgressMonitor
+	 * @param add a possibly-empty list of modules to add
+	 * @param remove a possibly-empty list of modules to remove
+	 * @param monitor a progress monitor, or <code>null</code> if progress
+	 *    reporting and cancellation are not desired
+	 * @throws CoreException [missing]
 	 */
 	public void modifyModules(IModule[] add, IModule[] remove, IProgressMonitor monitor) throws CoreException;
 
 	/**
-	 * Handle a save of this server. This method is called when the server
-	 * working copy save() method is invoked and can be used to resolve
-	 * calculated fields or perform other operations related to the changes
-	 * that are being made.
+	 * Handles a save of this server working copy. This method is called
+	 * when the server working copy <code>save</code> method
+	 * is invoked and can be used to resolve calculated fields or perform
+	 * other operations related to the changes that are being made.
+	 * <p>
+	 * This method is called by the web server core framework.
+	 * Clients should never call this method.
+	 * </p>
+	 * <p>
+	 * [issue: It's unclear why this method is necessary.]
+	 * </p>
+	 * <p>
+	 * [issue: Explain how PRE_SAVE and POST_SAVE fit in.]
+	 * </p>
+	 * <p>
+	 * [issue: byte is rarely used in Java. Use int instead.
+	 * Or, in this case, a boolean.]
+	 * </p>
 	 * 
-	 * @param id
-	 * @param monitor
+	 * @param id one of {@link #PRE_SAVE} or {@link #POST_SAVE}
+	 * @param monitor a progress monitor, or <code>null</code> if progress
+	 *    reporting and cancellation are not desired
 	 */
 	public void handleSave(byte id, IProgressMonitor monitor);
 }
\ No newline at end of file
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/resources/IModuleFile.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/resources/IModuleFile.java
index defba72a4..153615bbe 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/resources/IModuleFile.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/resources/IModuleFile.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -14,11 +14,31 @@ import java.io.InputStream;
 
 import org.eclipse.core.runtime.CoreException;
 /**
- *
+ * A module file is a leaf resource in a module. 
+ * Files contains data.
+ * <p>
+ * This interface is not intended to be implemented by clients
+ * other than module factories.
+ * </p>
+ * <p>
+ * [issue: See issues on IModuleResource about how to get rid
+ * of this interface.]
+ * </p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
+ * 
+ * @since 1.0
  */
 public interface IModuleFile extends IModuleResource {
+
 	/**
-	 * Gets the contents of the file as a stream.
+	 * Returns an open input stream on the contents of this file.
+	 * The client is responsible for closing the stream when finished.
+	 *
+	 * @return an input stream containing the contents of the file
+	 * @exception CoreException if this method fails
 	 */
 	public InputStream getContents() throws CoreException;
 }
\ No newline at end of file
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/resources/IModuleFolder.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/resources/IModuleFolder.java
index 82b26a805..9ef94f464 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/resources/IModuleFolder.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/resources/IModuleFolder.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -12,14 +12,37 @@ package org.eclipse.wst.server.core.resources;
 
 import org.eclipse.core.runtime.CoreException;
 /**
- * A remote folder is a remote resource that can have children.
+ * A module folder is leaf or non-leaf resource in a module.
+ * Folders may contain files and/or other folders. All members
+ * belong to the same module as their parent.
+ * <p>
+ * This interface is not intended to be implemented by clients
+ * other than module factories.
+ * </p>
+ * <p>
+ * [issue: See issues on IModuleResource about how to get rid
+ * of this interface.]
+ * </p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
+ * 
+ * @since 1.0
  */
 public interface IModuleFolder extends IModuleResource {
+
 	/**
-	 * Returns a list of IModuleResources directly contained
-	 * within this folder.
+	 * Returns a list of existing member resources (folders and files)
+	 * in this folder, in no particular order. 
+	 * <p>
+	 * Note that the members of a folder are the files and folders
+	 * immediately contained within it. All members belong to the
+	 * same module as this folder.
+	 * </p>
 	 *
-	 * @return java.util.List
+	 * @return a list of members of this folder
+	 * @exception CoreException if this method fails
 	 */
 	public IModuleResource[] members() throws CoreException;
 }
\ No newline at end of file
diff --git a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/resources/IModuleResource.java b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/resources/IModuleResource.java
index f09a9e129..189b7b1d7 100644
--- a/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/resources/IModuleResource.java
+++ b/plugins/org.eclipse.wst.server.core/servercore/org/eclipse/wst/server/core/resources/IModuleResource.java
@@ -1,5 +1,5 @@
 /**********************************************************************
- * Copyright (c) 2003 IBM Corporation and others.
+ * Copyright (c) 2004 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Common Public License v1.0
  * which accompanies this distribution, and is available at
@@ -13,44 +13,110 @@ package org.eclipse.wst.server.core.resources;
 import org.eclipse.core.runtime.IPath;
 import org.eclipse.wst.server.core.model.IModule;
 /**
- * A remote resource that has a name, parent, path, and timestamp.
+ * A module resource is a workspace file or folder contained
+ * in a module.
+ * <p>
+ * Every module resource is an instance of either
+ * {@link IModuleFile} or {@link IModuleFolder}.
+ * </p>
+ * <p>
+ * [issue: There is currently no way for a client to
+ * distinguish files from folders except by an instanceof
+ * check.]
+ * </p>
+ * <p>
+ * Each module resource belongs to exactly one module.
+ * </p>
+ * <p>
+ * This interface is not intended to be implemented directly.
+ * Module factories may implement one of the subinterfaces 
+ * <code>IModuleFile</code> and <code>IModuleFolder</code>.
+ * </p>
+ * <p>
+ * [issue: It is not clear that there is any need for module
+ * factories to implement IModuleFile and IModuleFolder.
+ * All that seems to be needed is an IResource paired with
+ * an IModule. The fact that all these interfaces are in
+ * org.eclipse.wst.server.core.resources suggests that this
+ * is the intent. If that's the case, one fixed implementation
+ * will do. The only API methods needed are:
+ *   public IResource getResource();
+ *   public IModule getModule();
+ *   public IContainer getModuleRoot();
+ * Names, paths, parents, and timestamps can all be handled
+ * by existing IResource API.]
+ * </p>
+ * <p>
+ * [issue: Equality/identify for module resources?]
+ * </p>
+ * <p>
+ * <it>Caveat: The server core API is still in an early form, and is
+ * likely to change significantly before the initial release.</it>
+ * </p>
+ * 
+ * @since 1.0
  */
 public interface IModuleResource {
+	
+	/** 
+	 * Time stamp constant (value -1) indicating the time stamp is
+	 * unknown.
+	 *
+	 * @see #getTimestamp()
+	 */
 	public static final long TIMESTAMP_UNKNOWN = -1;
 
 	/**
-	 * Returns the name of the remote resource.
+	 * Returns the name of this module resource.
 	 *
-	 * @return java.lang.String
+	 * @return the name
 	 */
 	public String getName();
 
 	/**
-	 * Returns the module that contains this resource.
+	 * Returns the module that contains this module resource.
+	 * 
+	 * @return the module 
 	 */
 	public IModule getModule();
 
 	/**
-	 * Returns the parent folder, or null if we're at the root.
+	 * Returns the parent folder in the same module, if any.
+	 * <p>
+	 * Note that a root resource is one that appears among
+	 * the list <code>getModule().members()</code>.
+	 * </p>
 	 *
-	 * @return org.eclipse.wst.server.model.IRemoteFolder
+	 * @returns the parent module folder, or <code>null</code>
+	 * this is a module root
 	 */
 	public IModuleFolder getParent();
 
 	/**
-	 * Return the path to this resource.
+	 * Return the path to this module resource.
+	 * <p>
+	 * Paths are relative to the module. That is, a root
+	 * resource named "foo" has path "foo"; a resource 
+	 * named "bar" insider it has path "foo/bar", and so on.
+	 * </p>
 	 *
-	 * @return org.eclipse.core.runtime.IPath
+	 * @return a module-relative resource path
 	 */
 	public IPath getPath();
 
 	/**
-	 * Returns the timestamp of the remote resource. This timestamp
-	 * does not need to match the local machine's time, but it
-	 * must be constantly increasing with time. (i.e. files with a
-	 * larger timestamp were created later)
+	 * Returns the time stamp of the module resource.
+	 * <p>
+	 * This time stamp need not match the local machine's time, but it
+	 * must be constantly increasing with time (i.e., files with a
+	 * larger time stamp must have been created later).
+	 * </p>
+	 * <p>
+	 * [issue: Rename to "getTimeStamp" for consistency with
+	 * IResource.getLocalTimeStamp().]
+	 * </p>
 	 *
-	 * @return long
+	 * @return a time stamp, or {@link #TIMESTAMP_UNKNOWN}
 	 */
 	public long getTimestamp();
 }