2014-12-05 19:52 GMT+01:00 Mark Thomas <ma...@apache.org>:
> On 05/12/2014 08:42, r...@apache.org wrote:
> > Author: remm
> > Date: Fri Dec 5 08:42:50 2014
> > New Revision: 1643194
>
> Short version:
> +1 on some.
> Against anything that is added with the sole purpose of passing a broken
> TCK test.
> A few questions with the general aim of reducing the number of system
> properties.
>
> >
> > URL: http://svn.apache.org/viewvc?rev=1643194&view=rev
> > Log:
> > Add configuration options for extensions, the origin header, periodic
> check and buffer size. This restores the previous behavior, but adds some
> flexibility.
> >
> > Modified:
> > tomcat/trunk/java/org/apache/tomcat/websocket/Constants.java
> > tomcat/trunk/java/org/apache/tomcat/websocket/LocalStrings.properties
> >
> tomcat/trunk/java/org/apache/tomcat/websocket/TransformationFactory.java
> > tomcat/trunk/java/org/apache/tomcat/websocket/Util.java
> >
> tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
> >
> > Modified: tomcat/trunk/java/org/apache/tomcat/websocket/Constants.java
> > URL:
> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/websocket/Constants.java?rev=1643194&r1=1643193&r2=1643194&view=diff
> >
> ==============================================================================
> > --- tomcat/trunk/java/org/apache/tomcat/websocket/Constants.java
> (original)
> > +++ tomcat/trunk/java/org/apache/tomcat/websocket/Constants.java Fri
> Dec 5 08:42:50 2014
> > @@ -43,7 +43,8 @@ public class Constants {
> > static final byte INTERNAL_OPCODE_FLUSH = 0x18;
> >
> > // Buffers
> > - static final int DEFAULT_BUFFER_SIZE = 8 * 1024;
> > + static final int DEFAULT_BUFFER_SIZE =
> > +
> Integer.getInteger("org.apache.tomcat.websocket.DEFAULT_BUFFER_SIZE", 8 *
> 1024);
>
> +1.
>
Note: that one didn't fix anything, but it looked more consistent.
>
> > // Client connection
> > public static final String HOST_HEADER_NAME = "Host";
> > @@ -60,6 +61,24 @@ public class Constants {
> > public static final String WS_EXTENSIONS_HEADER_NAME =
> > "Sec-WebSocket-Extensions";
> >
> > + // Configuration for Origin header in client
> > + static final boolean ALWAYS_ADD_ORIGIN_HEADER =
> > +
> Boolean.getBoolean("org.apache.tomcat.websocket.ALWAYS_ADD_ORIGIN_HEADER");
>
> Do we need this? I'd be happy with DEFAULT_ORIGIN_HEADER_VALUE == "" ->
> Don't add an origin by default.
>
> > + static final boolean SET_TARGET_AS_ORIGIN_HEADER =
> > +
> Boolean.getBoolean("org.apache.tomcat.websocket.SET_TARGET_AS_ORIGIN_HEADER");
>
> This concept I still think is fundamentally wrong. Target != origin. I'd
> really like to avoid having this in the code base if at all possible.
> Can you get the TCK to pass with just the following option?
>
I would have preferred not hardcoding a value. But it is good enough so I
guess I'll have to remove that extra property.
>
> > + static final String DEFAULT_ORIGIN_HEADER_VALUE =
> > +
> System.getProperty("org.apache.tomcat.websocket.DEFAULT_ORIGIN_HEADER_VALUE",
> "null");
> > +
> > + // Configuration for background processing checks intervals
> > + static final int DEFAULT_PROCESS_PERIOD =
> > +
> Integer.getInteger("org.apache.tomcat.websocket.DEFAULT_PROCESS_PERIOD",
> 10);
>
> +1.
>
Good. This is for a test which tries to expire a single session, but only
waits 5s (due to funny side effect in the algorithm, it worked exactly half
the time).
>
> > + // Configuration for extensions
> > + static final boolean DISABLE_BUILTIN_EXTENSIONS =
> > +
> Boolean.getBoolean("org.apache.tomcat.websocket.DISABLE_BUILTIN_EXTENSIONS");
> > + static final boolean ALLOW_UNSUPPORTED_EXTENSIONS =
> > +
> Boolean.getBoolean("org.apache.tomcat.websocket.ALLOW_UNSUPPORTED_EXTENSIONS");
> > +
>
> I'm against adding both of the above solely to work around broken TCK
> tests. Do they have any real world use case?
>
They're trying to test the existing APIs, the user indeed has a (rather
well hidden) way to set its own extensions. So this is rather legitimate.
But then you would like to throw an error. Fine with me, but things go both
ways: unless there's something in the spec which says the server should
throw an error if it doesn't know an extension, then it should be
acceptable to at least have an option for that behavior.
>
>
> > - } else if (value.length() == 1 &&
> > - (type.equals(char.class) ||
> type.equals(Character.class))) {
> > + } else if (type.equals(char.class) ||
> type.equals(Character.class)) {
> > return Character.valueOf(value.charAt(0));
>
> On first reading I didn't like this but it is consistent with coercion
> in the EL spec so there is precedence. I guess that makes me +0.
>
Well, this didn't look intuitive to me either. But there was no way around
it.
Rémy
