kdillane commented on a change in pull request #351:
URL: https://github.com/apache/tomcat/pull/351#discussion_r482606855
##########
File path: java/org/apache/jasper/EmbeddedServletOptions.java
##########
@@ -635,6 +648,19 @@ public EmbeddedServletOptions(ServletConfig config,
ServletContext context) {
}
}
+ String jspWhiteSpaceTrim =
config.getInitParameter("JSPWhiteSpaceTrimming");
+ if (jspWhiteSpaceTrim != null) {
+ if (jspWhiteSpaceTrim.equalsIgnoreCase("true")) {
+ this.JSPWhiteSpaceTrimming = true;
+ } else if (jspWhiteSpaceTrim.equalsIgnoreCase("false")) {
+ this.JSPWhiteSpaceTrimming = false;
+ } else {
+ if (log.isWarnEnabled()) {
+ log.warn(Localizer.getMessage("Invalid Value for the
flag"));
Review comment:
Should you include which flag has an invalid value?
##########
File path: java/org/apache/jasper/compiler/Generator.java
##########
@@ -81,6 +83,13 @@
private static final Class<?>[] OBJECT_CLASS = { Object.class };
+ //context param to enable or disable the excess white space trimming.
+ private static final String JSP_WHITE_SPACE_TRIMMING =
"JSPWhiteSpaceTrimming";
Review comment:
Where is this used?
##########
File path: java/org/apache/jasper/JspC.java
##########
@@ -195,6 +196,7 @@
protected boolean compile = false;
protected boolean failFast = false;
protected boolean smapSuppressed = true;
+ protected boolean JSPWhiteSpaceTrimming = false;
Review comment:
Similar camelCase comment.
##########
File path: java/org/apache/jasper/JspC.java
##########
@@ -137,6 +137,7 @@
protected static final String SWITCH_POOLING = "-poolingEnabled";
protected static final String SWITCH_ENCODING = "-javaEncoding";
protected static final String SWITCH_SMAP = "-smap";
+ protected static final String JSP_WHITE_SPACE_TRIM =
"-JSPWhiteSpaceTrimming";
Review comment:
The flags surrounding this one all follow camelCase. Should we update
the flag name here to follow?
##########
File path: java/org/apache/jasper/Options.java
##########
@@ -47,6 +47,13 @@
*/
public boolean getKeepGenerated();
+ /**
+ * Returns the Value of JSPWhiteSpaceTrimming Flag
Review comment:
Provide a description that explains the impact of enabling this flag.
It's somewhat clear from the name, but being explicit helps.
##########
File path: java/org/apache/jasper/compiler/Generator.java
##########
@@ -81,6 +83,13 @@
private static final Class<?>[] OBJECT_CLASS = { Object.class };
+ //context param to enable or disable the excess white space trimming.
+ private static final String JSP_WHITE_SPACE_TRIMMING =
"JSPWhiteSpaceTrimming";
+
+ private static final Pattern PRE_TAG_PATTERN =
Pattern.compile("(?s).*(<pre>|</pre>).*");
+
+ private static final Pattern BLANK_LINE_PATTERN =
Pattern.compile("(\\s*(\\n|\\r)+\\s*)");
Review comment:
We should provide a set of test cases that exercises these matchers.
##########
File path: java/org/apache/jasper/compiler/Generator.java
##########
@@ -2095,6 +2104,20 @@ public void visit(Node.JspElement n) throws
JasperException {
public void visit(Node.TemplateText n) throws JasperException {
String text = n.getText();
+ // If the flag is active, attempt to minimize the frequency of
+ // regex operations.
+ if ((ctxt!=null) &&
+ ctxt.getOptions().getJSPWhiteSpaceTrimFlag() &&
+ text.contains("\n")) {
+ // Ensure there are no <pre> or </pre> tags embedded in this
+ // text - if there are, we want to NOT modify the whitespace.
+ Matcher preMatcher = PRE_TAG_PATTERN.matcher(text);
Review comment:
Are we guaranteed to have well-formed tags here (i.e. open/close) or
could this split across tags? What are examples of text we might expect here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
