Repository: GoogleChrome/lighthouse (js)

Issue:
Minification estimation erroneously reports very large savings
While `minification-estimator.js` currently correctly [ignores license comments](https://github.com/GoogleChrome/lighthouse/blob/54c2b5df7fdcbc289c2ac5942e2d17efa5a8194c/lighthouse-core/lib/minification-estimator.js#L108), it seems like JavaScript and Cascading Style Sheets source mapping comments like `//# sourceMappingURL=foo.min.js.map` or `/*# sourceMappingURL=foo.css.map */` are still counted, which seems incorrect. 



Pull request:
core(minification): properly handle regex character classes
**Summary**
The minification estimator heuristics weren't accounting for regex character classes properly which meant that it could leave a regex prematurely.

**Related Issues/PRs**
fixes #6744


Patch:
diff --git a/lighthouse-core/lib/minification-estimator.js b/lighthouse-core/lib/minification-estimator.js
index 88686e1ca..256ed1565 100644
--- a/lighthouse-core/lib/minification-estimator.js
+++ b/lighthouse-core/lib/minification-estimator.js
@@ -47,6 +47,7 @@ function computeTokenLength(content, features) {
   let isInLicenseComment = false;
   let isInString = false;
   let isInRegex = false;
+  let isInRegexCharacterClass = false;
   let stringOpenChar = null;
 
   for (let i = 0; i < content.length; i++) {
@@ -94,7 +95,13 @@ function computeTokenLength(content, features) {
         // Skip over any escaped characters
         totalTokenLength++;
         i++;
-      } else if (char === '/') {
+      } else if (char === '[') {
+        // Register that we're entering a character class so we don't leave the regex prematurely
+        isInRegexCharacterClass = true;
+      } else if (char === ']' && isInRegexCharacterClass) {
+        // Register that we're exiting the character class
+        isInRegexCharacterClass = false;
+      } else if (char === '/' && !isInRegexCharacterClass) {
         // End the string when we hit the regex close character
         isInRegex = false;
         // console.log(i, 'leaving regex', char)


Test patch:
diff --git a/lighthouse-core/test/lib/minification-estimator-test.js b/lighthouse-core/test/lib/minification-estimator-test.js
index e04771723..f82c13b41 100644
--- a/lighthouse-core/test/lib/minification-estimator-test.js
+++ b/lighthouse-core/test/lib/minification-estimator-test.js
@@ -180,6 +180,31 @@ describe('minification estimator', () => {
       assert.equal(computeJSTokenLength(js), 9);
     });
 
+    it('should handle regular expression character classes', () => {
+      // test a slash inside of a character class to make sure it doesn't end the regex
+      // The below is the string-equivalent of
+      const _ = /regex [^/]\//.test('this should be in string not comment 123456789');
+
+      const js = `
+        /regex [^/]\\//.test('this should be in string not comment 123456789')
+      `;
+
+      assert.equal(computeJSTokenLength(js), 69);
+      assert.equal(computeJSTokenLength(js), js.trim().length);
+    });
+
+    it('should handle escaped regular expression characters', () => {
+      // test an escaped [ to make sure we can still close regexes
+      // This is the string-equivalent of
+      const _ = /regex \[/; // this should be in comment not string 123456789
+
+      const js = `
+        /regex \\[/ // this should be in comment not string 123456789
+      `;
+
+      assert.equal(computeJSTokenLength(js), 10);
+    });
+
     it('should distinguish regex from divide', () => {
       const js = `
         return 1 / 2 // hello
@@ -190,8 +215,8 @@ describe('minification estimator', () => {
 
     it('should handle large, real javscript files', () => {
       assert.equal(angularFullScript.length, 1364217);
-      // 1 - 405199 / 1364217 = estimated 70% smaller minified
-      assert.equal(computeJSTokenLength(angularFullScript), 405199);
+      // 1 - 334968 / 1364217 = estimated 75% smaller minified
+      assert.equal(computeJSTokenLength(angularFullScript), 334968);
     });
   });
 });