ace.js: fix the logic to inline CSS

src/node/utils/Minify.js

@@ -1,6 +1,7 @@
 /**
- * This Module manages all /minified/* requests. It controls the
+ * @file
- * minification && compression of Javascript and CSS.
+ * This Module manages all /static/* requests. It controls the
+ * minification and compression of Javascript and CSS.
  */
 
 /*
@@ -23,8 +24,6 @@ var ERR = require("async-stacktrace");
 var settings = require('./Settings');
 var async = require('async');
 var fs = require('fs');
-var StringDecoder = require('string_decoder').StringDecoder;
-var CleanCSS = require('clean-css');
 var path = require('path');
 var plugins = require("ep_etherpad-lite/static/js/pluginfw/plugin_defs");
 var RequireKernel = require('etherpad-require-kernel');
@@ -32,6 +31,7 @@ var urlutil = require('url');
 var mime = require('mime-types')
 var Threads = require('threads')
 var log4js = require('log4js');
+let _ = require('underscore')
 
 var logger = log4js.getLogger("Minify");
 
@@ -55,6 +55,14 @@ var LIBRARY_WHITELIST = [
 // Rewrite tar to include modules with no extensions and proper rooted paths.
 var LIBRARY_PREFIX = 'ep_etherpad-lite/static/js';
 exports.tar = {};
+
+/**
+ * Prefix a path with `LIBRARY_PREFIX`
+ * If path starts with '$' it is not prefixed
+ *
+ * @param {string} path
+ * @returns {string} 
+ */
 function prefixLocalLibraryPath(path) {
   if (path.charAt(0) == '$') {
     return path.slice(1);
@@ -76,8 +84,13 @@ for (var key in tar) {
     );
 }
 
-// What follows is a terrible hack to avoid loop-back within the server.
+/**
-// TODO: Serve files from another service, or directly from the file system.
+ * @param {string} url The file to be retrieved
+ * @param {'GET'|'HEAD'} method The request method
+ *
+ * What follows is a terrible hack to avoid loop-back within the server.
+ * @todo Serve files from another service, or directly from the file system.
+ */
 function requestURI(url, method, headers, callback) {
   var parsedURL = urlutil.parse(url);
 
@@ -142,8 +155,7 @@ function requestURIs(locations, method, headers, callback) {
 
 /**
  * creates the minifed javascript for the given minified name
- * @param req the Express request
+ *
- * @param res the Express response
  */
 function minify(req, res)
 {
@@ -233,16 +245,53 @@ function minify(req, res)
   }, 3);
 }
 
-// find all includes in ace.js and embed them.
+/**
+ * Find all includes in ace.js and if `settings.minify`
+ * is enabled it compresses and embeds them.
+ * In case `settings.minify` is false, it only embeds the
+ * `require-kernel`.
+ * The format of the INCLUDE_-lines is explained in `ace.js`
+ *
+ * @param {Function} callback 
+ *
+ */
 function getAceFile(callback) {
   fs.readFile(ROOT_DIR + 'js/ace.js', "utf8", function(err, data) {
     if(ERR(err, callback)) return;
 
-    // Find all includes in ace.js and embed them
+    let parts = [];
-    var founds = data.match(/\$\$INCLUDE_[a-zA-Z_]+\("[^"]*"\)/gi);
+    let founds = [];
-    if (!settings.minify) {
+    let result;
-      founds = [];
+    // files are inlined only when minify is true
+    if(settings.minify){
+      /**
+       * @example
+       * $$INCLUDE_CSS("../static/css/iframe_editor.css")
+       * $$INCLUDE_CSS("../static/css/pad.css?v=" + clientVars.randomVersionString);
+       * $$INCLUDE_CSS("../static/css/pad.css?v=" + clientVars.randomVersionString);
+       * $$INCLUDE_CSS("../static/skins/" + clientVars.skinName + "/pad.css?v=" + clientVars.randomVersionString);
+       */
+      // matches a INCLUDE_CSS line and parses it's parts
+      let includesLine = data.match(/^\s+?\$\$INCLUDE_CSS\(.*$/mg);
+      includesLine.map(function(line){
+        parts = line.split(/ |\+|"|;|\)/);
+        parts.push('")');
+        result = parts.map(function(part){
+          if(~part.indexOf("clientVars.skinName")){
+            return settings.skinName;
+          } else if(~part.indexOf("clientVars.randomVersionString")){
+            return settings.randomVersionString;
+          } else if(part === "$$INCLUDE_CSS("){
+            return '$$INCLUDE_CSS("';
+          } else {
+            return part;
+          }
+        }).filter(Boolean).join("");
+        founds.push(result);
+      })
+      founds = _.uniq(founds);
     }
+
     // Always include the require kernel.
     founds.push('$$INCLUDE_JS("../static/js/require-kernel.js")');
 
@@ -253,10 +302,14 @@ function getAceFile(callback) {
     // them into the file.
     async.forEach(founds, function (item, callback) {
       var filename = item.match(/"([^"]*)"/)[1];
+      // the file that is included from client side contains the version string,
+      // so we need to keep it as key for Ace2Editor.EMBEDED. However, the file
+      // is located at a path that does not contain the version string.
+      var resource = filename.split("?v=")[0];
 
       // Hostname "invalid.invalid" is a dummy value to allow parsing as a URI.
       var baseURI = 'http://invalid.invalid';
-      var resourceURI = baseURI + path.normalize(path.join('/static/', filename));
+      var resourceURI = baseURI + path.normalize(path.join('/static/', resource));
       resourceURI = resourceURI.replace(/\\/g, '/'); // Windows (safe generally?)
 
       requestURI(resourceURI, 'GET', {}, function (status, headers, body) {
@@ -275,7 +328,14 @@ function getAceFile(callback) {
   });
 }
 
-// Check for the existance of the file and get the last modification date.
+/**
+ *
+ * Check for the existance of the file and get the last modification date.
+ *
+ * @param {string} filename The name of the file
+ * @param {Function} callback 
+ * @param {number} dirStatLimit
+ */
 function statFile(filename, callback, dirStatLimit) {
   /*
    * The only external call to this function provides an explicit value for
@@ -314,6 +374,16 @@ function statFile(filename, callback, dirStatLimit) {
     });
   }
 }
+
+/**
+ * Iterates over `static/js` and `static/css` to find the modifiedtime of the most recent modified
+ * file. Used for `ace.js` because it has inlined files
+ *
+ * @todo Iterating over `static/js` is not necessary anymore, because inlining JS functionality has been removed.
+ * only require-kernel needs to be checked.
+ *
+ * @param {Function} callback 
+ */
 function lastModifiedDateOfEverything(callback) {
   var folders2check = [ROOT_DIR + 'js/', ROOT_DIR + 'css/'];
   var latestModification = 0;
@@ -354,16 +424,40 @@ function lastModifiedDateOfEverything(callback) {
   });
 }
 
-// This should be provided by the module, but until then, just use startup
+/**
-// time.
+ * @todo This should be provided by the module, but until then, just use startup
+ time.
+ */
 var _requireLastModified = new Date();
+
+/**
+ * Returns the startup time as UTCString
+ *
+ * @returns {string} startup time as UTCString
+ */
 function requireLastModified() {
   return _requireLastModified.toUTCString();
 }
+
+/**
+ * Returns the source code of `etherpad-require-kernel`s kernel definition
+ * @todo compress if minify is enabled
+ *
+ * @returns {string} the `etherpad-require-kernel` definition
+ */
 function requireDefinition() {
   return 'var require = ' + RequireKernel.kernelSource + ';\n';
 }
 
+/**
+ * Calls `getFile` to retrieve file content and if it's javascript or css it compresses
+ * the file via `threadPool` and calls `callback` with the compressed content.
+ * In case `settings.minify` is false it just returns the uncompressed content
+ *
+ * @param {string} filename file to be handled
+ * @param {string} contentType content type of file
+ * @param {Function} callback Function to be called with the compressed content 
+ */
 function getFileCompressed(filename, contentType, callback) {
   getFile(filename, function (error, content) {
     if (error || !content || !settings.minify) {
@@ -405,6 +499,13 @@ function getFileCompressed(filename, contentType, callback) {
   });
 }
 
+/**
+ * Gets the file `filename` and calls callback with the file's content
+ * Special handling of `ace.js` and `require-kernel.js` included
+ *
+ * @param {string} filename The file to be processed
+ * @param {Function} callback function to be called with the file's content
+ */
 function getFile(filename, callback) {
   if (filename == 'js/ace.js') {
     getAceFile(callback);

src/static/js/ace.js

@@ -1,4 +1,13 @@
 /**
+ * @file
+ * This file loads the iframes of the Editor.
+ * It constructs the HTML for the inner iframe, that contains all the pad lines, and
+ * for the outer iframe, that contains the sidediv with line numbers. The outer iframe
+ * dynamically inserts the inner iframe in it's onload event handler.
+ * The file is dynamically modified before delivered to clients in `Minify.js`. If `settings.minify`
+ * is enabled, then CSS files included here with `INCLUDE_CSS` are inlined. If `settings.minify`
+ * is false, then only the require-kernel is included.
+ *
  * This code is mostly from the old Etherpad. Please help us to comment this code.
  * This helps other people to understand this code better and helps them to improve it.
  * TL;DR COMMENTS ON THIS FILE ARE HIGHLY APPRECIATED
@@ -33,6 +42,12 @@ var hooks = require('./pluginfw/hooks');
 var pluginUtils = require('./pluginfw/shared');
 var _ = require('./underscore');
 
+/**
+ * Puts javascript code given in parameter `source` inside script tags
+ *
+ * @param {string} source javascript code
+ * @returns {string} A script element containing `source`
+ */
 function scriptTag(source) {
   return (
     '<script type="text/javascript">\n'
@@ -160,6 +175,13 @@ function Ace2Editor()
 
 
 
+  /**
+   * Takes an array of filenames and if they are keys in `Ace2Editor.EMBEDED` they
+   * are embeded and if not they should be included as remote links
+   *
+   * @param {string[]} files array of filenames
+   * @returns {{embeded: string[], remote: string[]}} An object containing filenames to embed and those to be included as remote files
+   */
   function sortFilesByEmbeded(files) {
     var embededFiles = [];
     var remoteFiles = [];
@@ -179,6 +201,14 @@ function Ace2Editor()
 
     return {embeded: embededFiles, remote: remoteFiles};
   }
+
+  /**
+   * Inserts style tags with css content from the files in `files`
+   * and link tags that reference remote stylesheets
+   *
+   * @param {string[]} buffer An array of html tags as strings
+   * @param {string[]} files An array of filenames
+   */
   function pushStyleTagsFor(buffer, files) {
     var sorted = sortFilesByEmbeded(files);
     var embededFiles = sorted.embeded;
@@ -192,8 +222,8 @@ function Ace2Editor()
       }
       buffer.push('<\/style>');
     }
-    for (var i = 0, ii = remoteFiles.length; i < ii; i++) {
+    for (i = 0, ii = remoteFiles.length; i < ii; i++) {
-      var file = remoteFiles[i];
+      file = remoteFiles[i];
       buffer.push('<link rel="stylesheet" type="text/css" href="' + encodeURI(file) + '"\/>');
     }
   }
@@ -227,10 +257,31 @@ function Ace2Editor()
       iframeHTML.push(doctype);
       iframeHTML.push("<html class='inner-editor " + clientVars.skinVariants + "'><head>");
 
-      // calls to these functions ($$INCLUDE_...)  are replaced when this file is processed
+      /**
-      // and compressed, putting the compressed code from the named file directly into the
+       * calls to these functions ($$INCLUDE_...) are replaced when this file is processed
-      // source here.
+       * and compressed, putting the compressed code from the named file directly into the
-      // these lines must conform to a specific format because they are passed by the build script:
+       * source here.
+       *
+       * When changing this lines ensure that the logic in `Minify.js` that inlines the files,
+       * knows which files to include. Files that should be inlined must appear on a newline with
+       * only whitespaces before them and can include pure filenames
+       *
+       * $$INCLUDE_CSS("../static/css/iframe_editor.css")
+       *
+       * filenames with an version string attached
+       * $$INCLUDE_CSS("../static/css/pad.css?v=" + clientVars.randomVersionString);
+       * in which case it assumes the string after `?v=` is based on `settings.randomVersionString`
+       *
+       * and filenames matching /skins/, either
+       * $$INCLUDE_CSS("../static/css/pad.css?v=" + clientVars.randomVersionString);
+       * or
+       * $$INCLUDE_CSS("../static/skins/" + clientVars.skinName + "/pad.css?v=" + clientVars.randomVersionString);
+       * in which case it assumes the first string to be the path, followed after
+       * `settings.skinName`, followed by the filename and `settings.randomVersionString` in case 
+       * the filename contains `?v=`
+       *
+       * Double quotes *must* be used
+       */
       var includedCSS = [];
       var $$INCLUDE_CSS = function(filename) {includedCSS.push(filename)};
       $$INCLUDE_CSS("../static/css/iframe_editor.css");
@@ -240,6 +291,10 @@ function Ace2Editor()
         $$INCLUDE_CSS("../static/css/pad.css?v=" + clientVars.randomVersionString);
       }
 
+      /**
+       * @todo
+       * css from plugins is not inlined
+       */
       var additionalCSS = _(hooks.callAll("aceEditorCSS")).map(function(path){
         if (path.match(/\/\//)) { // Allow urls to external CSS - http(s):// and //some/path.css
           return path;
@@ -316,12 +371,19 @@ window.onload = function () {\n\
 
       var outerHTML = [doctype, '<html class="inner-editor outerdoc ' + clientVars.skinVariants + '"><head>']
 
-      var includedCSS = [];
+      includedCSS = [];
-      var $$INCLUDE_CSS = function(filename) {includedCSS.push(filename)};
+      /*
+       * When using $$INCLUDE_CSS read the comment above first
+       */
+      $$INCLUDE_CSS = function(filename) {includedCSS.push(filename)};
       $$INCLUDE_CSS("../static/css/iframe_editor.css");
       $$INCLUDE_CSS("../static/css/pad.css?v=" + clientVars.randomVersionString);
 
 
+      /**
+       * @todo
+       * css from plugins is not inlined
+       */
       var additionalCSS = _(hooks.callAll("aceEditorCSS")).map(function(path){
         if (path.match(/\/\//)) { // Allow urls to external CSS - http(s):// and //some/path.css
           return path;
@@ -346,6 +408,9 @@ window.onload = function () {\n\
           '<div id="linemetricsdiv">x</div>',
           '</body></html>');
 
+      /**
+       * @type {HTMLIFrameElement}
+       */
       var outerFrame = document.createElement("IFRAME");
       outerFrame.name = "ace_outer";
       outerFrame.frameBorder = 0; // for IE