Merge pull request #532 from mrclay/urls_in_lines

Improve Minify_Lines algorithm and add tests

lib/Minify/Lines.php

@@ -41,11 +41,6 @@ class Minify_Lines {
             : '';
         $content = str_replace("\r\n", "\n", $content);
 
-        // Hackily rewrite strings with XPath expressions that are
-        // likely to throw off our dumb parser (for Prototype 1.6.1).
-        $content = str_replace('"/*"', '"/"+"*"', $content);
-        $content = preg_replace('@([\'"])(\\.?//?)\\*@', '$1$2$1+$1*', $content);
-
         $lines = explode("\n", $content);
         $numLines = count($lines);
         // determine left padding
@@ -89,33 +84,43 @@ class Minify_Lines {
      *
      * @param string $line current line of code
      *
-     * @param bool $inComment was the parser in a comment at the
-     * beginning of the line?
+     * @param bool $inComment was the parser in a C-style comment at the
+     * beginning of the previous line?
      *
      * @return bool
      */
     private static function _eolInComment($line, $inComment)
     {
-        // crude way to avoid things like // */
-        $line = preg_replace('~//.*?(\\*/|/\\*).*~', '', $line);
-
         while (strlen($line)) {
-            $search = $inComment
-                ? '*/'
-                : '/*';
-            $pos = strpos($line, $search);
-            if (false === $pos) {
-                return $inComment;
-            } else {
-                if ($pos == 0
-                    || ($inComment
-                        ? substr($line, $pos, 3)
-                        : substr($line, $pos-1, 3)) != '*/*')
-                {
-                        $inComment = ! $inComment;
+            if ($inComment) {
+                // only "*/" can end the comment
+                $index = self::_find($line, '*/');
+                if ($index === false) {
+                    return true;
                 }
-                $line = substr($line, $pos + 2);
+
+                // stop comment and keep walking line
+                $inComment = false;
+                @$line = (string)substr($line, $index + 2);
+                continue;
             }
+
+            // look for "//" and "/*"
+            $single = self::_find($line, '//');
+            $multi = self::_find($line, '/*');
+            if ($multi === false) {
+                return false;
+            }
+
+            if ($single === false || $multi < $single) {
+                // start comment and keep walking line
+                $inComment = true;
+                @$line = (string)substr($line, $multi + 2);
+                continue;
+            }
+
+            // a single-line comment preceeded it
+            return false;
         }
 
         return $inComment;
@@ -137,8 +142,63 @@ class Minify_Lines {
      */
     private static function _addNote($line, $note, $inComment, $padTo)
     {
-        return $inComment
+        $line = $inComment
             ? '/* ' . str_pad($note, $padTo, ' ', STR_PAD_RIGHT) . ' *| ' . $line
             : '/* ' . str_pad($note, $padTo, ' ', STR_PAD_RIGHT) . ' */ ' . $line;
+        return rtrim($line);
+    }
+
+    /**
+     * Find a token trying to avoid false positives
+     *
+     * @param string $str   String containing the token
+     * @param string $token Token being checked
+     * @return bool
+     */
+    private static function _find($str, $token) {
+        switch ($token) {
+            case '//':
+                $fakes = array(
+                    '://' => 1,
+                    '"//' => 1,
+                    '\'//' => 1,
+                    '".//' => 2,
+                    '\'.//' => 2,
+                );
+                break;
+            case '/*':
+                $fakes = array(
+                    '"/*' => 1,
+                    '\'/*' => 1,
+                    '"//*' => 2,
+                    '\'//*' => 2,
+                    '".//*' => 3,
+                    '\'.//*' => 3,
+                    '*/*' => 1,
+                    '\\/*' => 1,
+                );
+                break;
+            default:
+                $fakes = array();
+        }
+
+        $index = strpos($str, $token);
+        $offset = 0;
+
+        while ($index !== false) {
+            foreach ($fakes as $fake => $skip) {
+                $check = substr($str, $index - $skip, strlen($fake));
+                if ($check === $fake) {
+                    // move offset and scan again
+                    $offset += $index + strlen($token);
+                    $index = strpos($str, $token, $offset);
+                    break;
+                }
+            }
+            // legitimate find
+            return $index;
+        }
+
+        return $index;
     }
 }

min_extras/tools/minifyTextarea.php

@@ -55,7 +55,7 @@ if ($env->post('method') === 'Minify and serve') {
 }
 
 $tpl = array();
-$tpl['classes'] = array('Minify_HTML', 'JSMin\\JSMin', 'Minify_CSS');
+$tpl['classes'] = array('Minify_HTML', 'JSMin\\JSMin', 'Minify_CSS', 'Minify_Lines');
 
 if (in_array($env->post('method'), $tpl['classes'])) {
 

tests/MinifyLinesTest.php

@@ -4,8 +4,6 @@ class MinifyLinesTest extends TestCase
 {
     public function test_lines()
     {
-        $exp = file_get_contents(self::$test_files . "/minify/lines_output.js");
-
         $env = new Minify_Env(array(
             'server' => array(
                 'DOCUMENT_ROOT' => dirname(__DIR__),
@@ -15,15 +13,27 @@ class MinifyLinesTest extends TestCase
         $controller = new Minify_Controller_Files($env, $sourceFactory);
         $minify = new Minify(new Minify_Cache_Null());
 
+        $files = glob(self::$test_files . "/lines/*.in.js");
+
+        // uncomment to debug one
+        //$files = array(self::$test_files . "/lines/basic.in.js");
+
+        foreach ($files as $file) {
             $ret = $minify->serve($controller, array(
-            'debug' => true
-            ,'quiet' => true
-            ,'encodeOutput' => false
-            ,'files' => array(
-                self::$test_files . "/js/before.js"
-            )
+                'debug' => true,
+                'quiet' => true,
+                'encodeOutput' => false,
+                'files' => array($file),
             ));
 
-        $this->assertEquals($exp, $ret['content']);
+            $outFile = str_replace('.in.js', '.out.js', $file);
+
+            $exp = file_get_contents($outFile);
+
+            // uncomment to set up expected output
+            //file_put_contents($outFile, $ret['content']);
+
+            $this->assertEquals($exp, $ret['content'], "Did not match: " . basename($outFile));
+        }
     }
 }

tests/_test_files/lines/basic.in.js

@@ -0,0 +1,66 @@
+/*! is.js
+
+ (c) 2001 Douglas Crockford
+ 2001 June 3
+*/
+
+// is
+
+// The -is- object is used to identify the browser.  Every browser edition
+// identifies itself, but there is no standard way of doing it, and some of
+// the identification is deceptive. This is because the authors of web
+// browsers are liars. For example, Microsoft's IE browsers claim to be
+// Mozilla 4. Netscape 6 claims to be version 5.
+
+var is = {
+    ie:      navigator.appName == 'Microsoft Internet Explorer',
+    java:    navigator.javaEnabled(),
+    ns:      navigator.appName == 'Netscape',
+    ua:      navigator.userAgent.toLowerCase(),
+    version: parseFloat(navigator.appVersion.substr(21)) ||
+             parseFloat(navigator.appVersion),
+    win:     navigator.platform == 'Win32'
+}
+/*!*
+ * preserve this comment, too
+ */
+is.mac = is.ua.indexOf('mac') >= 0;
+if (is.ua.indexOf('opera') >= 0) {
+    is.ie = is.ns = false;
+    is.opera = true;
+}
+if (is.ua.indexOf('gecko') >= 0) {
+    is.ie = is.ns = false;
+    is.gecko = true;
+}
+
+/*@cc_on
+   /*@if (@_win32)
+    if (is.ie && is.win)
+        document.write("PASS: IE/win honored conditional comment.<br>");
+   @else @*/
+    if (is.ie && is.win)
+        document.write("FAIL: IE/win did not honor multi-line conditional comment.<br>");
+    else
+        document.write("PASS: Non-IE/win browser ignores multi-line conditional comment.<br>");
+   /*@end
+@*/
+
+var recognizesCondComm = true;
+//@cc_on/*
+recognizesCondComm = false;
+//@cc_on*/
+
+if ((is.ie && is.win) == recognizesCondComm)
+    document.write("PASS: IE/win honored single-line conditional comment.<br>");
+else
+    document.write("FAIL: Non-IE/win browser did not ignore single-line conditional comment.<br>");
+
+// hello
+//@cc_on/*
+// world
+//@cc_on*/
+//@cc_on/*
+'hello';
+/*!* preserved */
+/*!* preserved */

tests/_test_files/lines/basic.out.js

@@ -1,5 +1,5 @@
 
-/* before.js */
+/* basic.in.js */
 
 /* 1  */ /*! is.js
 /* 2  *|
@@ -52,7 +52,7 @@
 /* 49 */ var recognizesCondComm = true;
 /* 50 */ //@cc_on/*
 
-/* before.js */
+/* basic.in.js */
 
 /* 51 */ recognizesCondComm = false;
 /* 52 */ //@cc_on*/

tests/_test_files/lines/misc.in.js

@@ -8,3 +8,7 @@ xpath = {
     f: 0
 };
 document._getElementsByXPath('.//*' + cond, element);
+
+// from angular 1.4.8
+var URL_REGEXP = /^[a-z][a-z\d.+-]*:\/*(?:[^:@]+(?::[^@]+)?@)?(?:[^\s:/?#]+|\[[a-f\d:]+\])(?::\d+)?(?:\/[^?#]*)?(?:\?[^#]*)?(?:#.*)?$/i;
+

tests/_test_files/lines/misc.out.js

@@ -0,0 +1,18 @@
+
+/* misc.in.js */
+
+/* 1  */ // sections from Prototype 1.6.1
+/* 2  */ var xpath = ".//*[local-name()='ul' or local-name()='UL']" +
+/* 3  */           "//*[local-name()='li' or local-name()='LI']";
+/* 4  */ this.matcher = ['.//*'];
+/* 5  */ xpath = {
+/* 6  */     descendant:   "//*",
+/* 7  */     child:        "/*",
+/* 8  */     f: 0
+/* 9  */ };
+/* 10 */ document._getElementsByXPath('.//*' + cond, element);
+/* 11 */
+/* 12 */ // from angular 1.4.8
+/* 13 */ var URL_REGEXP = /^[a-z][a-z\d.+-]*:\/*(?:[^:@]+(?::[^@]+)?@)?(?:[^\s:/?#]+|\[[a-f\d:]+\])(?::\d+)?(?:\/[^?#]*)?(?:\?[^#]*)?(?:#.*)?$/i;
+/* 14 */
+/* 15 */

tests/_test_files/lines/url.in.js

@@ -0,0 +1,13 @@
+foo; /* http://example.com */
+bar;
+
+foo; /*
+    http://example.com */
+bar;
+
+foo = "http://example.com"; /* hello */
+bar;
+
+foo = "http://example.com"; /*
+hello */
+bar;

tests/_test_files/lines/url.out.js

@@ -0,0 +1,17 @@
+
+/* url.in.js */
+
+/* 1  */ foo; /* http://example.com */
+/* 2  */ bar;
+/* 3  */
+/* 4  */ foo; /*
+/* 5  *|     http://example.com */
+/* 6  */ bar;
+/* 7  */
+/* 8  */ foo = "http://example.com"; /* hello */
+/* 9  */ bar;
+/* 10 */
+/* 11 */ foo = "http://example.com"; /*
+/* 12 *| hello */
+/* 13 */ bar;
+/* 14 */