[GitHub] [tomcat] kamnani opened a new pull request #351: Remove White Spaces from the JSP files

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
70 messages Options
1234
Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani opened a new pull request #351: Remove White Spaces from the JSP files

GitBox

kamnani opened a new pull request #351:
URL: https://github.com/apache/tomcat/pull/351


   This is a redo of Previous CR : https://github.com/apache/tomcat/pull/331
   
   These changes enable the compiler to remove excess white space from the JSP files & thus reduce the JVM metadata
   _(Constant whitespace in a JSP is passed unchanged to the client browser (indentations, newlines, etc.).  This results in bloated constant strings, and general waste in I/O operations. Trimming this whitespace results in smaller constants)._
   
   This can be controlled by providing context init params inside web.xml file. Example attached.
   
   Based on your previous suggestions the following changes have been made:
   1) Pre tags will be left untouched - to protect the behavioral changes on that tag.
   2) By default this remains false, and thus will not affect any other supporting feature (SMAP for instance as suggested in previous CR) .
   3) The flag is now initialized as a context param as mentioned in the example below.
   
   Apologies in case something is missed out.
   
   If any official documentation is required, can you attach the links on the PR?
   ```
   <web-app>
   <context-param>
       <param-name>JSPWhiteSpaceTrimming</param-name>
       <param-value>true</param-value>
   </context-param>
   </web-app>
   


----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] rotty3000 commented on pull request #351: Remove White Spaces from the JSP files

GitBox

rotty3000 commented on pull request #351:
URL: https://github.com/apache/tomcat/pull/351#issuecomment-685946608


   I've been wondering why this flag is set by a context param instead of along with all other jsp compiler flags in the [jasper options](https://github.com/apache/tomcat/blob/master/java/org/apache/jasper/Options.java)?


----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on pull request #351: Remove White Spaces from the JSP files

GitBox
In reply to this post by GitBox

kamnani commented on pull request #351:
URL: https://github.com/apache/tomcat/pull/351#issuecomment-686061378


   @rotty3000 Thanks for the comment.
   I have added the flag inside the jasper options as mentioned by you. Do we need any other change to this PR?
   


----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kdillane commented on a change in pull request #351: Remove White Spaces from the JSP files

GitBox
In reply to this post by GitBox

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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on a change in pull request #351: Remove White Spaces from the JSP files

GitBox
In reply to this post by GitBox

kamnani commented on a change in pull request #351:
URL: https://github.com/apache/tomcat/pull/351#discussion_r482622880



##########
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:
       Yes, I think we can add the name of the flag. That can be done.




----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on a change in pull request #351: Remove White Spaces from the JSP files

GitBox
In reply to this post by GitBox

kamnani commented on a change in pull request #351:
URL: https://github.com/apache/tomcat/pull/351#discussion_r482623460



##########
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:
       I think we need input here, if we should add another webapp - for testing this change.




----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kdillane commented on a change in pull request #351: Remove White Spaces from the JSP files

GitBox
In reply to this post by GitBox

kdillane commented on a change in pull request #351:
URL: https://github.com/apache/tomcat/pull/351#discussion_r482627998



##########
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:
       Can we not add tests here: https://github.com/apache/tomcat/blob/master/test/org/apache/jasper/compiler/TestGenerator.java?




----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on a change in pull request #351: Remove White Spaces from the JSP files

GitBox
In reply to this post by GitBox

kamnani commented on a change in pull request #351:
URL: https://github.com/apache/tomcat/pull/351#discussion_r482622880



##########
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:
       Yes, I think we can add the name of the flag. That can be done.

##########
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:
       I think we need input here, if we should add another webapp - for testing this change.




----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on a change in pull request #351: Remove White Spaces from the JSP files

GitBox
In reply to this post by GitBox

kamnani commented on a change in pull request #351:
URL: https://github.com/apache/tomcat/pull/351#discussion_r482630168



##########
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:
       I think we might need to add another JSP file, with the patterns. I will look into it.Thanks




----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on a change in pull request #351: Remove White Spaces from the JSP files

GitBox
In reply to this post by GitBox

kamnani commented on a change in pull request #351:
URL: https://github.com/apache/tomcat/pull/351#discussion_r482630168



##########
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:
       I think we might need to add another JSP file, with the patterns. I will look into it 👍




----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on a change in pull request #351: Remove White Spaces from the JSP files

GitBox
In reply to this post by GitBox

kamnani commented on a change in pull request #351:
URL: https://github.com/apache/tomcat/pull/351#discussion_r482630308



##########
File path: java/org/apache/jasper/Options.java
##########
@@ -47,6 +47,13 @@
      */
     public boolean getKeepGenerated();
 
+    /**
+     * Returns the Value of JSPWhiteSpaceTrimming Flag

Review comment:
       Yes, I think I can add some description there




----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on a change in pull request #351: Remove White Spaces from the JSP files

GitBox
In reply to this post by GitBox

kamnani commented on a change in pull request #351:
URL: https://github.com/apache/tomcat/pull/351#discussion_r482630409



##########
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:
       Will change it to :  jspWhiteSpaceTrimming




----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] martin-g commented on a change in pull request #351: Remove White Spaces from the JSP files

GitBox
In reply to this post by GitBox

martin-g commented on a change in pull request #351:
URL: https://github.com/apache/tomcat/pull/351#discussion_r483001907



##########
File path: java/org/apache/jasper/EmbeddedServletOptions.java
##########
@@ -84,6 +84,11 @@
      */
     private boolean classDebugInfo = true;
 
+    /**
+     * Do we want to Trim White Space in the in JSP Files?
+     */
+    private boolean JSPWhiteSpaceTrimming = false;

Review comment:
       `jspWhiteSpaceTrimming` looks better to me. It follows Java coding conventions

##########
File path: java/org/apache/jasper/EmbeddedServletOptions.java
##########
@@ -84,6 +84,11 @@
      */
     private boolean classDebugInfo = true;
 
+    /**
+     * Do we want to Trim White Space in the in JSP Files?

Review comment:
       why `Trim White Space` is capitalized here ?

##########
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:
       The `key` for `getMessage()` looks weird here.
   See the other usages of Localizer.getMessage() with `jsp.warning.**` keys.

##########
File path: java/org/apache/jasper/EmbeddedServletOptions.java
##########
@@ -84,6 +84,11 @@
      */
     private boolean classDebugInfo = true;
 
+    /**
+     * Do we want to Trim White Space in the in JSP Files?
+     */
+    private boolean JSPWhiteSpaceTrimming = false;

Review comment:
       What is the difference with https://github.com/apache/tomcat/blob/6aef6f468c843f2bcdec4b57884d1e7e65c03365/java/org/apache/jasper/EmbeddedServletOptions.java#L601 ?




----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on a change in pull request #351: Remove White Spaces from the JSP files

GitBox
In reply to this post by GitBox

kamnani commented on a change in pull request #351:
URL: https://github.com/apache/tomcat/pull/351#discussion_r483087177



##########
File path: java/org/apache/jasper/EmbeddedServletOptions.java
##########
@@ -84,6 +84,11 @@
      */
     private boolean classDebugInfo = true;
 
+    /**
+     * Do we want to Trim White Space in the in JSP Files?
+     */
+    private boolean JSPWhiteSpaceTrimming = false;

Review comment:
       trimSpaces will only touch the whitespaces. Examples:
    Node text =
   1 )  "          " => ""
   2) \n\n\n => ""
   3) <title>\n\n\n\n => <title>\n\n\n\n (untouched)
   
   jspWhiteSpaceTrimming will touch all of the above and reduce all the excess whitespace the JSP page might have.
   
   
   




----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on a change in pull request #351: Remove White Spaces from the JSP files

GitBox
In reply to this post by GitBox

kamnani commented on a change in pull request #351:
URL: https://github.com/apache/tomcat/pull/351#discussion_r483088141



##########
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:
       So this just allows us to leave the pre tags untouched. It should catch any node that might have <pre> or </pre> or both as the comment explains.




----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on a change in pull request #351: Remove White Spaces from the JSP files

GitBox
In reply to this post by GitBox

kamnani commented on a change in pull request #351:
URL: https://github.com/apache/tomcat/pull/351#discussion_r483088141



##########
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:
       So this just allows us to leave the pre tags untouched. It should catch any node that might have <pre> |  </pre> |  both as the comment explains.




----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on a change in pull request #351: Remove White Spaces from the JSP files

GitBox
In reply to this post by GitBox

kamnani commented on a change in pull request #351:
URL: https://github.com/apache/tomcat/pull/351#discussion_r483088141



##########
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:
       So this just allows us to leave the pre tags untouched. It should catch any node that might have < pre > |  < /pre > |  both as the comment explains.




----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on a change in pull request #351: Remove White Spaces from the JSP files

GitBox
In reply to this post by GitBox

kamnani commented on a change in pull request #351:
URL: https://github.com/apache/tomcat/pull/351#discussion_r483119869



##########
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:
       Resolved with latest push




----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on a change in pull request #351: Remove White Spaces from the JSP files

GitBox
In reply to this post by GitBox

kamnani commented on a change in pull request #351:
URL: https://github.com/apache/tomcat/pull/351#discussion_r483121124



##########
File path: java/org/apache/jasper/EmbeddedServletOptions.java
##########
@@ -84,6 +84,11 @@
      */
     private boolean classDebugInfo = true;
 
+    /**
+     * Do we want to Trim White Space in the in JSP Files?
+     */
+    private boolean JSPWhiteSpaceTrimming = false;

Review comment:
       Resolved with latest push




----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on a change in pull request #351: Remove White Spaces from the JSP files

GitBox
In reply to this post by GitBox

kamnani commented on a change in pull request #351:
URL: https://github.com/apache/tomcat/pull/351#discussion_r483121476



##########
File path: java/org/apache/jasper/EmbeddedServletOptions.java
##########
@@ -84,6 +84,11 @@
      */
     private boolean classDebugInfo = true;
 
+    /**
+     * Do we want to Trim White Space in the in JSP Files?

Review comment:
       Resolved with latest push




----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

1234