Provisional API review. Added API: tasks. No code change.

31 files changed, 89 insertions, 19 deletions

diff --git a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/internal/provisional/util/JDTBeanIntrospector.java b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/internal/provisional/util/JDTBeanIntrospector.java
index 6d8ab0228..07d34fb46 100644
--- a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/internal/provisional/util/JDTBeanIntrospector.java
+++ b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/internal/provisional/util/JDTBeanIntrospector.java
@@ -18,16 +18,22 @@ import org.eclipse.jdt.core.Signature;
 import org.eclipse.jst.jsf.common.JSFCommonPlugin;
 
 /**
- * A class that does bean introspector on a JDT IType
+ * A class that does bean introspection on a JDT IType
+ * 
+ * This functionality is not meant to replace runtime bean 
+ * introspection.  Rather, it is meant to provide a 
+ * more "lightweight" (in terms of class loading as well as
+ * error handling of bean instantiation out of context) way
+ * to determine a bean's properties at design time
  * 
  * @author cbateman
  *
  */
 public class JDTBeanIntrospector 
 {
-	private final static String GET_PREFIX = "get";
-	private final static String SET_PREFIX = "set";
-	private final static String IS_PREFIX = "is";
+	private final static String GET_PREFIX = "get"; //$NON-NLS-1$
+	private final static String SET_PREFIX = "set"; //$NON-NLS-1$
+	private final static String IS_PREFIX = "is"; //$NON-NLS-1$
 	
 	private final IType 	_type;
 
@@ -60,7 +66,7 @@ public class JDTBeanIntrospector
 			catch (JavaModelException jme)
 			{
 				// log and then proceed to next method
-				JSFCommonPlugin.log(jme, "Error processing IMethod for bean property info");
+				JSFCommonPlugin.log(jme, "Error processing IMethod for bean property info"); //$NON-NLS-1$
 			}
 		}
 		
@@ -160,7 +166,7 @@ public class JDTBeanIntrospector
 		}
 		catch(JavaModelException jme)
 		{
-            JSFCommonPlugin.log(jme, "Error getting type information for bean");
+            JSFCommonPlugin.log(jme, "Error getting type information for bean"); //$NON-NLS-1$
 		}
 
 		return methods;
@@ -185,7 +191,7 @@ public class JDTBeanIntrospector
                 final IType superType = closure[i];
                 methods.addAll(Arrays.asList(superType.getMethods()));
             } catch (JavaModelException e) {
-                JSFCommonPlugin.log(e, "Error getting super type information for bean");
+                JSFCommonPlugin.log(e, "Error getting super type information for bean"); //$NON-NLS-1$
             }
         }
             
diff --git a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/internal/provisional/util/TypeUtil.java b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/internal/provisional/util/TypeUtil.java
index a1c3ddbcf..a56d647ec 100644
--- a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/internal/provisional/util/TypeUtil.java
+++ b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/internal/provisional/util/TypeUtil.java
@@ -197,8 +197,7 @@ public final class TypeUtil
     }
     
     /**
-     * @param typeSignature
-     * @return a fully qualified Java class name from a type signature
+     * @param typeSignature     * @return a fully qualified Java class name from a type signature
      * i.e. Ljava.lang.String; -> java.lang.String
      */
     public static String getFullyQualifiedName(final String typeSignature)
diff --git a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/AbstractResultSet.java b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/AbstractResultSet.java
index e8f0ab124..8356579ca 100644
--- a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/AbstractResultSet.java
+++ b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/AbstractResultSet.java
@@ -44,6 +44,9 @@ public abstract class AbstractResultSet/*<T>*/ implements IResultSet/*<T>*/ {
 		getInternalResults().add(item);
 	}
 
+    // API: should return Collection so that subs can use
+    // co-variance to return other sorts of collections
+    // API: consider making final and make getInternalResults protected abstract
 	public List/*<T>*/ getResults() {
 		if (results == null)
 			return Collections.EMPTY_LIST;
@@ -54,6 +57,8 @@ public abstract class AbstractResultSet/*<T>*/ implements IResultSet/*<T>*/ {
 	/**
 	 * @return resultset size
 	 */
+    // API: should make this final and force
+    // implementers to handle getInternalResults
 	public int size(){
 		if (results == null)
 			return 0;
diff --git a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/IEntityVisitor.java b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/IEntityVisitor.java
index 3f001ec56..235c40722 100644
--- a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/IEntityVisitor.java
+++ b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/IEntityVisitor.java
@@ -15,7 +15,7 @@ import org.eclipse.jst.jsf.common.metadata.internal.provisional.Entity;
 
 /**
  * Visitor interface for Entities
- *
+ * API: should we force extension through an abstract class?
  */
 public abstract interface IEntityVisitor extends IMetaDataVisitor {
 	/**
@@ -26,6 +26,7 @@ public abstract interface IEntityVisitor extends IMetaDataVisitor {
 	/**
 	 * Signal that the last visited entity is now completely 'visited'.  
 	 * Needed to signal that all children have also been visited.
+     * API: what is the contract here for implementers of this interface?
 	 */
 	public void visitCompleted();
 }
diff --git a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/IMetaDataModelContext.java b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/IMetaDataModelContext.java
index d7df17782..7c1d51ca3 100644
--- a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/IMetaDataModelContext.java
+++ b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/IMetaDataModelContext.java
@@ -16,18 +16,25 @@ import org.eclipse.core.resources.IProject;
 /**
  * Context for determining the Model to locate in the MetaDataManager
  * Should not be implemented by clients
+ * API: should it extend IModelContext?
  */
 public interface IMetaDataModelContext{
 	/**
-	 * @return project 
+	 * @return project
+     * API: if this becomes an IModelContext, may be able take
+     * IProject off the interface and push down into an workspace resolver 
 	 */
 	public IProject getProject();
 	/**
 	 * @return domain id as defined by the DomainLoading
+     * API: what's DomainLoading?  I would prefer to see a 
+     * type instead of an open-ended string
 	 */
 	public String getDomain();
 	/**
 	 * @return the namespace URI as String
+     * API: isn't URI part of the domain?  For example, package not URI
+     * would be the hiearchy id in a Java class domain.
 	 */
 	public String getURI();
 }
diff --git a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/IMetaDataVisitor.java b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/IMetaDataVisitor.java
index cacbfa7a7..80bafe39d 100644
--- a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/IMetaDataVisitor.java
+++ b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/IMetaDataVisitor.java
@@ -15,5 +15,6 @@ package org.eclipse.jst.jsf.common.metadata.internal.provisional.query;
  * Common interface for Entity and Trait visitors
  */
 public abstract interface IMetaDataVisitor {
+    // API: contract?
 	public boolean stopVisiting();
 }
diff --git a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/IResultSet.java b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/IResultSet.java
index ef24b82c0..4acc2d6df 100644
--- a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/IResultSet.java
+++ b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/IResultSet.java
@@ -28,6 +28,7 @@ public interface IResultSet/*<T>*/{
 	
 	/**
 	 * Signal that the query results are no longer required allowing for any cleanup that may be required
+     * API: event to signal that result set is no longer valid?
 	 */
 	public void close();
 }
diff --git a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/ITraitVisitor.java b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/ITraitVisitor.java
index ffade6956..b12d30140 100644
--- a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/ITraitVisitor.java
+++ b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/ITraitVisitor.java
@@ -21,6 +21,8 @@ public abstract interface ITraitVisitor extends IMetaDataVisitor {
 	/**
 	 * Visit the Trait
 	 * @param trait
+     * API: what is the responsibilit of the implementor? pre/post-conditions?
+     * API: what is the order of the visitation (or should it not be assumed)
 	 */
 	abstract void visit(final Trait trait);
 }
diff --git a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/MetaDataQueryHelper.java b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/MetaDataQueryHelper.java
index f0e19489b..df69774aa 100644
--- a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/MetaDataQueryHelper.java
+++ b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/MetaDataQueryHelper.java
@@ -31,6 +31,8 @@ import org.eclipse.jst.jsf.common.metadata.internal.provisional.Trait;
  * @see IEntityQueryVisitor
  * @see ITraitQueryVisitor
  * @see IMetaDataModelContext
+ * 
+ * API: class should be final, constructor made private.
  */
 public class MetaDataQueryHelper{
 	public static final String TAGLIB_DOMAIN = "TagLibraryDomain"; //need better place for this
diff --git a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/SearchControl.java b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/SearchControl.java
index 1cf562b28..e73fdc7b8 100644
--- a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/SearchControl.java
+++ b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/SearchControl.java
@@ -14,6 +14,7 @@ package org.eclipse.jst.jsf.common.metadata.internal.provisional.query;
 /**
  * Simple class used by {@link SimpleMetaDataQueryVisitorImpl} allowing some control of a metadata query.  
  * A query visitor can use this to optimize the query results and signal completion.
+ * API: extensibility?  should this be package private? 
  */
 public class SearchControl {
 	
diff --git a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/SimpleMetaDataQueryVisitorImpl.java b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/SimpleMetaDataQueryVisitorImpl.java
index 02da00d1b..9763dfd7e 100644
--- a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/SimpleMetaDataQueryVisitorImpl.java
+++ b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/SimpleMetaDataQueryVisitorImpl.java
@@ -23,6 +23,7 @@ import org.eclipse.jst.jsf.common.metadata.internal.provisional.Trait;
  * A simple metadata query visitor implementing {@link IEntityQueryVisitor} and {@link ITraitQueryVisitor}.
  * - simple find entity and traits by id only 	
  * - Does not allow for wild card searchs
+ * API: extensibility?
  */
 public class SimpleMetaDataQueryVisitorImpl implements IEntityQueryVisitor, ITraitQueryVisitor  {
 
diff --git a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/SimpleResultSet.java b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/SimpleResultSet.java
index cd97419fc..4eb2916d6 100644
--- a/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/SimpleResultSet.java
+++ b/jsf/plugins/org.eclipse.jst.jsf.common/src/org/eclipse/jst/jsf/common/metadata/internal/provisional/query/SimpleResultSet.java
@@ -18,4 +18,6 @@ package org.eclipse.jst.jsf.common.metadata.internal.provisional.query;
  *
  */
 public class SimpleResultSet/*<T>*/ extends AbstractResultSet/*<T>*/ {
+    // API: Should push down overridable functionality from AbstractResultSet
+    // and make final all functionality left in AbstractResultSet
 }
diff --git a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/DTHTMLOutputRenderer.java b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/DTHTMLOutputRenderer.java
index 0ee7a266d..d3c3a4d31 100644
--- a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/DTHTMLOutputRenderer.java
+++ b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/DTHTMLOutputRenderer.java
@@ -44,6 +44,9 @@ import org.w3c.dom.Element;
  * Design-time metadata-driven HTML IOutputRenderer implementation.
  * 
  * @author Ian Trimble - Oracle
+ * API: as we have discussed I really think this should be done through a factory.
+ * Even if we don't go full blown extension point right away, we should split out
+ * the transform construction responsibility to a separate factory.
  */
 public class DTHTMLOutputRenderer implements IOutputRenderer {
 
diff --git a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/DTTagConverter.java b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/DTTagConverter.java
index 45083b396..bcb6c23dc 100644
--- a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/DTTagConverter.java
+++ b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/DTTagConverter.java
@@ -34,6 +34,8 @@ import org.w3c.dom.Text;
  * Design-Time metadata-driven tag converter.
  * 
  * @author Ian Trimble - Oracle
+ * API: should consider splitting out an AbstractDTTagConverter public and making
+ * this package private (or simply non-API)
  */
 public class DTTagConverter implements
 	ITagConverter, ITagEditInfo, INodeAdapter, IDOMFactory {
diff --git a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/DTTagConverterContext.java b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/DTTagConverterContext.java
index 18c427994..456820476 100644
--- a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/DTTagConverterContext.java
+++ b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/DTTagConverterContext.java
@@ -19,6 +19,10 @@ import org.w3c.dom.Text;
  * Design-time tag converter implementation of ITagConverterContext.
  * 
  * @author Ian Trimble - Oracle
+ * API: should we make some these public methods?  Is it reasonable
+ * to allow people to reuse/override these methods?  Perhaps it
+ * makes sense to split this to an AbstractTagConverterContext usable
+ * by all?
  */
 public class DTTagConverterContext implements ITagConverterContext {
 
diff --git a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/DTTagConverterDecorator.java b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/DTTagConverterDecorator.java
index 045e6f147..7c9a032d3 100644
--- a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/DTTagConverterDecorator.java
+++ b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/DTTagConverterDecorator.java
@@ -29,6 +29,7 @@ import org.w3c.dom.Text;
  * ITagConverterDecorator implementation for DTTagConverter.
  * 
  * @author Ian Trimble - Oracle
+ * API: should this really be API?
  */
 public class DTTagConverterDecorator implements ITagConverterDecorator {
 
diff --git a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/DefaultTransformer.java b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/DefaultTransformer.java
index 6de2a9cfc..3bfca7db2 100644
--- a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/DefaultTransformer.java
+++ b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/DefaultTransformer.java
@@ -24,14 +24,9 @@ import org.w3c.dom.Element;
 public class DefaultTransformer implements ITransformer {
 
 	private ITagConverterContext tagConverterContext;
-	private Collection transformOperations;
-
-	/**
-	 * Constructs an instance.
-	 */
-	public DefaultTransformer() {
-		transformOperations = new ArrayList();
-	}
+    // synchronization occurs on this object, so it must never
+    // be allowed to be null
+	private final Collection transformOperations = new ArrayList();
 
 	/*
 	 * (non-Javadoc)
diff --git a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/IOutputRenderer.java b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/IOutputRenderer.java
index 5f6a8b9b8..f203e6ad7 100644
--- a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/IOutputRenderer.java
+++ b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/IOutputRenderer.java
@@ -26,6 +26,8 @@ public interface IOutputRenderer {
 	 * ITagConverter-specific context and functionality.
 	 * @return Element instance representing top Element in resulting output
 	 * tree.
+     * API: let's parameterize the return type either with a generic type
+     * or with Object so that sub-classes can co-vary
 	 */
 	public Element render(ITagConverterContext tagConverterContext);
 
diff --git a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/ITransformOperation.java b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/ITransformOperation.java
index 057309b70..fb7fcc7a6 100644
--- a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/ITransformOperation.java
+++ b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/ITransformOperation.java
@@ -19,6 +19,9 @@ import org.w3c.dom.Element;
  * to output Element instances.
  * 
  * @author Ian Trimble - Oracle
+ * API: as with ITransformer, I'd like to consider 
+ * ITransfomOperation<SRC, RESULT>
+ * API: extensibilty? Should clients be allowed to implement/sub-class?
  */
 public interface ITransformOperation {
 
@@ -37,6 +40,7 @@ public interface ITransformOperation {
 	 * 
 	 * @param tagConverterContext ITagConverterContext instance allowing access
 	 * to context and functionality of the current ITagConverter instance.
+     * 
 	 */
 	public void setTagConverterContext(ITagConverterContext tagConverterContext);
 
diff --git a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/ITransformer.java b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/ITransformer.java
index e98107d02..653cf8219 100644
--- a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/ITransformer.java
+++ b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/internal/provisional/ITransformer.java
@@ -24,6 +24,8 @@ public interface ITransformer {
 	 * Appends an ITransformOperation instance to the collection.
 	 * 
 	 * @param operation ITransformOperation instance to be appended.
+     * API: does this need to be on the interface or could it be pushed
+     * into the instance constructor (i.e. factory).
 	 */
 	public void appendTransformOperation(ITransformOperation operation);
 
@@ -34,6 +36,9 @@ public interface ITransformer {
 	 * 
 	 * @param srcElement Original input Element instance.
 	 * @return Transformed output Element instance.
+     * API: I'd like to consider templating the generic ITransformer
+     * interface to ITransformer<I,O> and making this a normative instance
+     * case of ITransformer<Element, Element>
 	 */
 	public Element transform(Element srcElement);
 
@@ -43,6 +48,9 @@ public interface ITransformer {
 	 * 
 	 * @param tagConverterContext ITagConverterContext instance allowing access
 	 * to context and functionality of the current ITagConverter instance.
+     * API: does this need to be on the interface?  Why couldn't this be set
+     * at construction time from a factory?  Alternatively, should it be passed
+     * as an argument to tranform()?
 	 */
 	public void setTagConverterContext(ITagConverterContext tagConverterContext);
 
diff --git a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/AbstractTransformOperation.java b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/AbstractTransformOperation.java
index 4b3d8fb83..774c1cba9 100644
--- a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/AbstractTransformOperation.java
+++ b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/AbstractTransformOperation.java
@@ -31,6 +31,7 @@ public abstract class AbstractTransformOperation implements ITransformOperation
 	/**
 	 * ITagConverterContext instance.
 	 */
+    // API: should make private and provide a getter.
 	protected ITagConverterContext tagConverterContext;
 
 	/**
@@ -49,6 +50,8 @@ public abstract class AbstractTransformOperation implements ITransformOperation
 	 * @see org.eclipse.jst.pagedesigner.dtmanager.converter.internal.provisional.ITransformOperation#setTagConverterContext(org.eclipse.jst.pagedesigner.dtmanager.converter.internal.provisional.ITagConverterContext)
 	 */
 	public void setTagConverterContext(ITagConverterContext tagConverterContext) {
+        // API: this should really be set on construction since other methods
+        // cannot be called until it is set.
 		this.tagConverterContext = tagConverterContext;
 	}
 
diff --git a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/AppendChildTextOperation.java b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/AppendChildTextOperation.java
index 36be3262d..10ee5497d 100644
--- a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/AppendChildTextOperation.java
+++ b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/AppendChildTextOperation.java
@@ -21,6 +21,8 @@ import org.w3c.dom.Text;
  * a call to the transform(...) method.
  * 
  * @author Ian Trimble - Oracle
+ * API: should this be public or should we restrict so can only be constructed
+ * through a factory?
  */
 public class AppendChildTextOperation extends AbstractTransformOperation {
 
diff --git a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/ConvertAttributeToTextOperation.java b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/ConvertAttributeToTextOperation.java
index c51a0d437..c44e18b4a 100644
--- a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/ConvertAttributeToTextOperation.java
+++ b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/ConvertAttributeToTextOperation.java
@@ -22,6 +22,8 @@ import org.w3c.dom.Text;
  * a call to the transform(...) method.
  * 
  * @author Ian Trimble - Oracle
+ * API: should this be public or should we restrict so can only be constructed
+ * through a factory?
  */
 public class ConvertAttributeToTextOperation extends AbstractTransformOperation {
 
diff --git a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/CopyAllAttributesOperation.java b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/CopyAllAttributesOperation.java
index 63427a54b..4d97a68ba 100644
--- a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/CopyAllAttributesOperation.java
+++ b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/CopyAllAttributesOperation.java
@@ -19,6 +19,8 @@ import org.w3c.dom.NamedNodeMap;
  * source Element instance to the current Element instance.
  * 
  * @author Ian Trimble - Oracle
+ * API: should this be public or should we restrict so can only be constructed
+ * through a factory?
  */
 public class CopyAllAttributesOperation extends AbstractTransformOperation {
 
diff --git a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/CopyAttributeOperation.java b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/CopyAttributeOperation.java
index 370c37ff8..b4138ee92 100644
--- a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/CopyAttributeOperation.java
+++ b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/CopyAttributeOperation.java
@@ -19,6 +19,8 @@ import org.w3c.dom.Element;
  * Element instance if no such attribute exists on the source Element instance.
  * 
  * @author Ian Trimble - Oracle
+ * API: should this be public or should we restrict so can only be constructed
+ * through a factory?
  */
 public class CopyAttributeOperation extends AbstractTransformOperation {
 
diff --git a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/CopyChildrenOperation.java b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/CopyChildrenOperation.java
index 45d4c7df0..ff8e4a24f 100644
--- a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/CopyChildrenOperation.java
+++ b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/CopyChildrenOperation.java
@@ -21,6 +21,8 @@ import org.w3c.dom.Element;
  * a call to the transform(...) method.
  * 
  * @author Ian Trimble - Oracle
+ * API: should this be public or should we restrict so can only be constructed
+ * through a factory?
  */
 public class CopyChildrenOperation extends AbstractTransformOperation {
 
diff --git a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/CreateAttributeOperation.java b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/CreateAttributeOperation.java
index 8f4baee10..fe2241e8e 100644
--- a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/CreateAttributeOperation.java
+++ b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/CreateAttributeOperation.java
@@ -17,6 +17,8 @@ import org.w3c.dom.Element;
  * current Element.
  * 
  * @author Ian Trimble - Oracle
+ * API: should this be public or should we restrict so can only be constructed
+ * through a factory?
  */
 public class CreateAttributeOperation extends AbstractTransformOperation {
 
diff --git a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/CreateElementOperation.java b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/CreateElementOperation.java
index aed53f9e9..6d3aa37b9 100644
--- a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/CreateElementOperation.java
+++ b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/CreateElementOperation.java
@@ -20,6 +20,8 @@ import org.w3c.dom.Element;
  * a call to the transform(...) method.
  * 
  * @author Ian Trimble - Oracle
+ * API: should this be public or should we restrict so can only be constructed
+ * through a factory?
  */
 public class CreateElementOperation extends AbstractTransformOperation {
 
diff --git a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/MakeParentElementCurrentOperation.java b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/MakeParentElementCurrentOperation.java
index 9e38f6b35..c5939ff9d 100644
--- a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/MakeParentElementCurrentOperation.java
+++ b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/MakeParentElementCurrentOperation.java
@@ -18,6 +18,8 @@ import org.w3c.dom.Node;
  * Element the new current Element.
  * 
  * @author Ian Trimble - Oracle
+ * API: should this be public or should we restrict so can only be constructed
+ * through a factory?
  */
 public class MakeParentElementCurrentOperation extends AbstractTransformOperation {
 
diff --git a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/RemoveAttributeOperation.java b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/RemoveAttributeOperation.java
index 9ccde2206..8543d1c19 100644
--- a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/RemoveAttributeOperation.java
+++ b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/RemoveAttributeOperation.java
@@ -17,6 +17,8 @@ import org.w3c.dom.Element;
  * current Element.
  * 
  * @author Ian Trimble - Oracle
+ * API: should this be public or should we restrict so can only be constructed
+ * through a factory?
  */
 public class RemoveAttributeOperation extends AbstractTransformOperation {
 
diff --git a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/RenameAttributeOperation.java b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/RenameAttributeOperation.java
index 0c14bb3f3..60042411b 100644
--- a/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/RenameAttributeOperation.java
+++ b/jsf/plugins/org.eclipse.jst.pagedesigner/src/org/eclipse/jst/pagedesigner/dtmanager/converter/operations/internal/provisional/RenameAttributeOperation.java
@@ -18,6 +18,8 @@ import org.w3c.dom.Element;
  * Element.
  * 
  * @author Ian Trimble - Oracle
+ * API: should this be public or should we restrict so can only be constructed
+ * through a factory?
  */
 public class RenameAttributeOperation extends AbstractTransformOperation {