Index: war-util/src/main/resources/com/sun/logging/enterprise/system/container/web/LogStrings.properties
===================================================================
--- war-util/src/main/resources/com/sun/logging/enterprise/system/container/web/LogStrings.properties	(revision 24578)
+++ war-util/src/main/resources/com/sun/logging/enterprise/system/container/web/LogStrings.properties	(working copy)
@@ -203,6 +203,11 @@


-> WOLFRAM: General comments:

1. I would include the name of the web module in any of the log messages,
   as shown below. I know the same should also be done for most of the
   existing messages. ;-)

2. Since the new messages are emitted from WebModule.java, I would change
   the "pewebcontainer" prefix in the message keys to "webmodule", and add
   "valve" to them, so they are more specific, i.e., change
   pewebcontainer.xxx to webmodule.valve.xxx

So in summary, I would make these changes:

OLD: 
pewebcontainer.specifiedMethodMissing=There is no method [{0}(java.lang.String)] in the valve named [{1}]

NEW:
webmodule.valve.specifiedMethodMissing=There is no method [{0}(java.lang.String)] defined on valve [{1}] of web module [{2}]


OLD:
pewebcontainer.valveSetterCausedException=The method [{0}] in the valve [{1}] caused an exception

NEW:
webmodule.valve.setterCausedException=Exception during execution of method [{0}] on valve [{1}] of web module [{2}]


OLD:
pewebcontainer.noNameForValve=No name for one of the valves given

NEW: 
webmodule.valve.missingName=Web module [{0}] has a valve without any name


OLD:
pewebcontainer.noClassnameForValve=No classname for the valve [{0}] given

NEW:
webmodule.valve.missingClassname=Valve [{0}] of web module [{1}] has no class name


OLD:
pewebcontainer.noNameForProperty=No name for a property of the valve [{0}] given
 
NEW:
webmodule.valve.missingPropertyName=Valve [{0}] of web module [{1}] has a property without any name
 

Index: web-glue/src/main/java/com/sun/enterprise/web/WebModule.java
===================================================================
--- web-glue/src/main/java/com/sun/enterprise/web/WebModule.java	(revision 24578)
+++ web-glue/src/main/java/com/sun/enterprise/web/WebModule.java	(working copy)
@@ -935,7 +954,71 @@
         }     
     }    
     
+    /**
+     * Add a <code>Valve</code> to a <code>VirtualServer</code> pipeline.


-> WOLFRAM: I would change the comment to read:

  Constructs a <tt>Valve</tt> from the given <tt>valveDescriptor</tt>  
  and adds it to the <tt>Pipeline</tt> of this WebModule

(I know the javadoc comment of the existing addValve(String valveName)
needs to be fixed as well, and I'll take care of that. ;-)


+     * @param valveDescriptor the object containing the information to create the valve.
+     */
+    protected void addValve(com.sun.enterprise.deployment.runtime.web.Valve valveDescriptor) {
+        String valveName = valveDescriptor.getAttributeValue(
+                com.sun.enterprise.deployment.runtime.web.Valve.NAME);
+        String className = valveDescriptor.getAttributeValue(
+                com.sun.enterprise.deployment.runtime.web.Valve.CLASSNAME);
+        if (valveName == null) {
+            logger.log(Level.WARNING, rb.getString("pewebcontainer.noNameForValve"));


-> WOLFRAM: Here you could just pass the message key to the logger, and
   have it map it to the corresponding log message. With my
   recommended change for message keys from above, this line would read
   as follows:

    logger.log(Level.WARNING, "webmodule.valve.missingName", getName());


+            return;
+        }
+        if (className == null) {
+            String msg = rb.getString("pewebcontainer.noClassnameForValve");
+            msg = MessageFormat.format(msg, new Object[] {valveName});
+            logger.log(Level.WARNING, msg);


-> WOLFRAM: Here, we would also need to pass in the web module name
   for parametric replacement, if we were to adopt my recommendation
   from above (regarding inclusion of web module name in log messages):

       new Object[] {valveName, getName()}



+            return;
+        }
+        Object valve = loadInstance(className);
+        if (valve == null) {
+            return;
+        }


-> WOLFRAM: I would move one of the instanceof checks from below to here,
   to make sure we're actually dealing with a valve before invoking
   any of its methods:

     if ((!valve instanceof Valve) &&
             !(valve instanceof GlassFishValve)) {
         logger.log(Level.WARNING,
                    "Object of type classname " + className +
                    " not an instance of Valve or GlassFishValve");
    }


   The 2nd else case could then be removed from here:

+        if (valve instanceof Valve) {
+            super.addValve((Valve) valve);
+        } else if (valve instanceof GlassFishValve) {
+            super.addValve((GlassFishValve) valve);
+        } else {
+            logger.log(Level.WARNING,
+                        "Object of type classname " + className +
+                        " not an instance of Valve or GlassFishValve");
+        }
+    }