Bug 1245916: XBL bindings should support global declarations in comments. r=miker draft
authorDave Townsend <dtownsend@oxymoronical.com>
Fri, 05 Feb 2016 12:13:34 -0800
changeset 331373 91a60ef0359ef53093fe197ed63dbc4e1a9f10a5
parent 331372 0f1cceca29ac398be97a55bbcd09fe58a8be1435
child 331374 94daff602b51d7ad57a24872d9eba9b304cf2da9
push id10969
push userdtownsend@mozilla.com
push dateTue, 16 Feb 2016 22:40:19 +0000
reviewersmiker
bugs1245916
milestone47.0a1
Bug 1245916: XBL bindings should support global declarations in comments. r=miker To properly lint XBL files we need to support things like import-globals-from and other ESlint comment directives so we have to pass comments through to the code blocks that ESlint parses. Unfortunately the way the XBL processor works now is by passing a separate code block for every method/property/etc. in the XBL and ESlint doesn't retain state across the blocks so we would have to prefix every block with every comment. Instead this change makes us output just a single block that roughly looks like this: <comments> var bindings = { "<binding-id>": { <binding-part-name>: function() { ... } } } This has some interesting bonuses. Defining the same ID twice will cause a lint failure. Same for the same field in a binding. The line mapping is a little harder and there are still a few lines that won't map directly back to the original file but they should be rare cases. The only downside is that since some bindings have the same binding declared differently for different platforms we have to exclude those from linting for now. MozReview-Commit-ID: CAsPt5dtf6T
.eslintignore
testing/eslint-plugin-mozilla/lib/processors/xbl-bindings.js
--- a/.eslintignore
+++ b/.eslintignore
@@ -184,16 +184,17 @@ toolkit/modules/tests/xpcshell/test_task
 
 # Not yet updated
 toolkit/components/osfile/**
 toolkit/components/passwordmgr/**
 
 # Uses preprocessing
 toolkit/content/contentAreaUtils.js
 toolkit/content/widgets/videocontrols.xml
+toolkit/content/widgets/wizard.xml
 toolkit/components/jsdownloads/src/DownloadIntegration.jsm
 toolkit/components/search/nsSearchService.js
 toolkit/components/url-classifier/**
 toolkit/components/urlformatter/nsURLFormatter.js
 toolkit/identity/FirefoxAccounts.jsm
 toolkit/modules/AppConstants.jsm
 toolkit/mozapps/downloads/nsHelperAppDlg.js
 toolkit/mozapps/extensions/internal/AddonConstants.jsm
--- a/testing/eslint-plugin-mozilla/lib/processors/xbl-bindings.js
+++ b/testing/eslint-plugin-mozilla/lib/processors/xbl-bindings.js
@@ -31,134 +31,169 @@ function parseError(err) {
 let entityRegex = /&[\w][\w-\.]*;/g;
 
 // A simple sax listener that generates a tree of element information
 function XMLParser(parser) {
   this.parser = parser;
   parser.onopentag = this.onOpenTag.bind(this);
   parser.onclosetag = this.onCloseTag.bind(this);
   parser.ontext = this.onText.bind(this);
+  parser.onopencdata = this.onOpenCDATA.bind(this);
   parser.oncdata = this.onCDATA.bind(this);
+  parser.oncomment = this.onComment.bind(this);
 
   this.document = {
     local: "#document",
     uri: null,
     children: [],
+    comments: [],
   }
   this._currentNode = this.document;
 }
 
 XMLParser.prototype = {
   parser: null,
 
   onOpenTag: function(tag) {
     let node = {
       parentNode: this._currentNode,
       local: tag.local,
       namespace: tag.uri,
       attributes: {},
       children: [],
+      comments: [],
       textContent: "",
-      textStart: { line: this.parser.line, column: this.parser.column },
+      textLine: this.parser.line,
+      textColumn: this.parser.column,
+      textEndLine: this.parser.line
     }
 
     for (let attr of Object.keys(tag.attributes)) {
       if (tag.attributes[attr].uri == "") {
         node.attributes[attr] = tag.attributes[attr].value;
       }
     }
 
     this._currentNode.children.push(node);
     this._currentNode = node;
   },
 
   onCloseTag: function(tagname) {
+    this._currentNode.textEndLine = this.parser.line;
     this._currentNode = this._currentNode.parentNode;
   },
 
   addText: function(text) {
     this._currentNode.textContent += text;
   },
 
   onText: function(text) {
     // Replace entities with some valid JS token.
     this.addText(text.replace(entityRegex, "null"));
   },
 
-  onCDATA: function(text) {
+  onOpenCDATA: function() {
     // Turn the CDATA opening tag into whitespace for indent alignment
     this.addText(" ".repeat("<![CDATA[".length));
+  },
+
+  onCDATA: function(text) {
     this.addText(text);
+  },
+
+  onComment: function(text) {
+    this._currentNode.comments.push(text);
   }
 }
 
-// Strips the indentation from lines of text and adds a fixed two spaces indent
-function buildCodeBlock(tag, prefix, suffix, indent) {
-  prefix = prefix === undefined ? "" : prefix;
-  suffix = suffix === undefined ? "\n}" : suffix;
-  indent = indent === undefined ? 2 : indent;
+// -----------------------------------------------------------------------------
+// Processor Definition
+// -----------------------------------------------------------------------------
+
+const INDENT_LEVEL = 2;
+
+function indent(count) {
+  return " ".repeat(count * INDENT_LEVEL);
+}
+
+// Stores any XML parse error
+let xmlParseError = null;
 
-  let text = tag.textContent;
-  let line = tag.textStart.line;
-  let column = tag.textStart.column;
+// Stores the lines of JS code generated from the XBL
+let scriptLines = [];
+// Stores a map from the synthetic line number to the real line number
+// and column offset.
+let lineMap = [];
+
+function addSyntheticLine(line, linePos) {
+  lineMap[scriptLines.length] = { line: linePos, offset: null };
+  scriptLines.push(line);
+}
 
-  let lines = text.split("\n");
+/**
+ * Adds generated lines from an XBL node to the script to be passed back to eslint.
+ */
+function addNodeLines(node, reindent) {
+  let lines = node.textContent.split("\n");
+  let startLine = node.textLine;
+  let startColumn = node.textColumn;
+
+  // The case where there are no whitespace lines before the first text is
+  // treated differently for indentation
+  let indentFirst = false;
 
   // Strip off any preceeding whitespace only lines. These are often used to
   // format the XML and CDATA blocks.
   while (lines.length && lines[0].trim() == "") {
-    column = 0;
-    line++;
+    indentFirst = true;
+    startLine++;
     lines.shift();
   }
 
   // Strip off any whitespace lines at the end. These are often used to line
   // up the closing tags
   while (lines.length && lines[lines.length - 1].trim() == "") {
     lines.pop();
   }
 
-  // Indent the first line with the starting position of the text block
-  if (lines.length && column) {
-    lines[0] = " ".repeat(column) + lines[0];
+  if (!indentFirst) {
+    let firstLine = lines.shift();
+    firstLine = " ".repeat(reindent * INDENT_LEVEL) + firstLine;
+    // ESLint counts columns starting at 1 rather than 0
+    lineMap[scriptLines.length] = { line: startLine, offset: reindent * INDENT_LEVEL - (startColumn - 1) };
+    scriptLines.push(firstLine);
+    startLine++;
   }
 
   // Find the preceeding whitespace for all lines that aren't entirely whitespace
   let indents = lines.filter(s => s.trim().length > 0)
                      .map(s => s.length - s.trimLeft().length);
   // Find the smallest indent level in use
   let minIndent = Math.min.apply(null, indents);
 
-  // Strip off the found indent level and prepend the new indent level, but only
-  // if the string isn't already empty.
-  let indentstr = " ".repeat(indent);
-  lines = lines.map(s => s.length ? indentstr + s.substring(minIndent) : s);
+  for (let line of lines) {
+    if (line.trim().length == 0) {
+      // Don't offset lines that are only whitespace, the only possible JS error
+      // is trailing whitespace and we want it to point at the right place
+      lineMap[scriptLines.length] = { line: startLine, offset: 0 };
+    } else {
+      line = " ".repeat(reindent * INDENT_LEVEL) + line.substring(minIndent);
+      lineMap[scriptLines.length] = { line: startLine, offset: reindent * INDENT_LEVEL - (minIndent - 1) };
+    }
 
-  let block = {
-    script: prefix + lines.join("\n") + suffix + "\n",
-    line: line - prefix.split("\n").length,
-    indent: minIndent - indent
-  };
-  return block;
+    scriptLines.push(line);
+    startLine++;
+  }
 }
 
-// -----------------------------------------------------------------------------
-// Processor Definition
-// -----------------------------------------------------------------------------
-
-// Stores any XML parse error
-let xmlParseError = null;
-
-// Stores the code blocks
-let blocks = [];
-
 module.exports = {
   preprocess: function(text, filename) {
     xmlParseError = null;
-    blocks = [];
+    scriptLines = [];
+    lineMap = [];
 
     // Non-strict allows us to ignore many errors from entities and
     // preprocessing at the expense of failing to report some XML errors.
     // Unfortunately it also throws away the case of tagnames and attributes
     let parser = sax.parser(false, {
       lowercase: true,
       xmlns: true,
     });
@@ -176,104 +211,150 @@ module.exports = {
       return [];
     }
 
     let bindings = document.children[0];
     if (bindings.local != "bindings" || bindings.namespace != NS_XBL) {
       return [];
     }
 
-    let scripts = [];
+    for (let comment of document.comments) {
+      addSyntheticLine(`/*`, 0);
+      for (let line of comment.split("\n")) {
+        addSyntheticLine(`${line.trim()}`, 0);
+      }
+      addSyntheticLine(`*/`, 0);
+    }
+
+    addSyntheticLine(`var bindings = {`, bindings.textLine);
 
     for (let binding of bindings.children) {
       if (binding.local != "binding" || binding.namespace != NS_XBL) {
         continue;
       }
 
+      addSyntheticLine(indent(1) + `"${binding.attributes.id}": {`, binding.textLine);
+
       for (let part of binding.children) {
         if (part.namespace != NS_XBL) {
           continue;
         }
 
-        if (part.local != "implementation" && part.local != "handlers") {
+        if (part.local == "implementation") {
+          addSyntheticLine(indent(2) + `implementation: {`, part.textLine);
+        } else if (part.local == "handlers") {
+          addSyntheticLine(indent(2) + `handlers: [`, part.textLine);
+        } else {
           continue;
         }
 
         for (let item of part.children) {
           if (item.namespace != NS_XBL) {
             continue;
           }
 
           switch (item.local) {
             case "field": {
-              // Fields get converted into variable declarations
+              // Fields are something like lazy getter functions
 
               // Ignore empty fields
               if (item.textContent.trim().length == 0) {
                 continue;
               }
-              blocks.push(buildCodeBlock(item, "", "", 0));
+
+              addSyntheticLine(indent(3) + `get ${item.attributes.name}() {`, item.textLine);
+              // TODO This will probably break some style rules when. We need
+              // to inject this next to the first non-whitespace character
+              addSyntheticLine(indent(4) + `return`, item.textLine);
+              addNodeLines(item, 4);
+              addSyntheticLine(indent(3) + `},`, item.textEndLine);
               break;
             }
             case "constructor":
             case "destructor": {
               // Constructors and destructors become function declarations
-              blocks.push(buildCodeBlock(item, `function ${item.local}() {\n`));
+              addSyntheticLine(indent(3) + `${item.local}() {`, item.textLine);
+              addNodeLines(item, 4);
+              addSyntheticLine(indent(3) + `},`, item.textEndLine);
               break;
             }
             case "method": {
               // Methods become function declarations with the appropriate params
+
               let params = item.children.filter(n => n.local == "parameter" && n.namespace == NS_XBL)
                                         .map(n => n.attributes.name)
                                         .join(", ");
               let body = item.children.filter(n => n.local == "body" && n.namespace == NS_XBL)[0];
-              blocks.push(buildCodeBlock(body, `function ${item.attributes.name}(${params}) {\n`));
+
+              addSyntheticLine(indent(3) + `${item.attributes.name}(${params}) {`, item.textLine);
+              addNodeLines(body, 4);
+              addSyntheticLine(indent(3) + `},`, item.textEndLine);
               break;
             }
             case "property": {
               // Properties become one or two function declarations
               for (let propdef of item.children) {
                 if (propdef.namespace != NS_XBL) {
                   continue;
                 }
 
-                let params = propdef.local == "setter" ? "val" : "";
-                blocks.push(buildCodeBlock(propdef, `function ${item.attributes.name}_${propdef.local}(${params}) {\n`));
+                if (propdef.local == "setter") {
+                  addSyntheticLine(indent(3) + `set ${item.attributes.name}(val) {`, propdef.textLine);
+                } else if (propdef.local == "getter") {
+                  addSyntheticLine(indent(3) + `get ${item.attributes.name}() {`, propdef.textLine);
+                } else {
+                  continue;
+                }
+                addNodeLines(propdef, 4);
+                addSyntheticLine(indent(3) + `},`, propdef.textEndLine);
               }
               break;
             }
             case "handler": {
               // Handlers become a function declaration with an `event` parameter
-              blocks.push(buildCodeBlock(item, `function onevent(event) {\n`));
+              addSyntheticLine(indent(3) + `function(event) {`, item.textLine);
+              addNodeLines(item, 4);
+              addSyntheticLine(indent(3) + `},`, item.textEndLine);
               break;
             }
             default:
               continue;
           }
         }
+
+        addSyntheticLine(indent(2) + (part.local == "implementation" ? `},` : `],`), part.textEndLine);
       }
+      addSyntheticLine(indent(1) + `},`, binding.textEndLine);
     }
+    addSyntheticLine(`};`, bindings.textEndLine);
 
-    return blocks.map(b => b.script);
+    let script = scriptLines.join("\n") + "\n";
+    return [script];
   },
 
   postprocess: function(messages, filename) {
     // If there was an XML parse error then just return that
     if (xmlParseError) {
       return [xmlParseError];
     }
 
     // For every message from every script block update the line to point to the
     // correct place.
     let errors = [];
     for (let i = 0; i < messages.length; i++) {
-      let block = blocks[i];
+      for (let message of messages[i]) {
+        // ESLint indexes lines starting at 1 but our arrays start at 0
+        let mapped = lineMap[message.line - 1];
 
-      for (let message of messages[i]) {
-        message.line += block.line + 1;
-        message.column += block.indent;
+        message.line = mapped.line + 1;
+        if (mapped.offset) {
+          message.column -= mapped.offset;
+        } else {
+          message.column = NaN;
+        }
+
         errors.push(message);
       }
     }
 
     return errors;
   }
 };