572038 - Create new Tree Panel for Inspector (milestone 0.5)

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Rob Campbell [:rc] (:robcee)]

The initial Tree View was a proof-of-concept widget. For release, we want something that is more code-like than the simple tree provided by the inIDOMView.

The panel should provide visual indicators representing expandable nodes. Tags, attributes and values should be syntax highlighted.

Comment 1

[Rob Campbell [:rc] (:robcee)]

Attachment: tree-panel-rollup-20100623

first pass rollup patch. I know I'm going to need to split out some chunks, but needed to get this in place first.

Comment 2

[Johnathan Nightingale [:johnath]]

This scares me, but we need something better than what's currently there. Can you do anything here to ease review load (co-review from another dev tools hacker, stacked, small patches, try server builds full of unit test goodness?)

Comment 3

[Rob Campbell [:rc] (:robcee)]

I'm working on splitting this into two separate patches with decent testing. Hope to have my split patches later in the day and some additional unittests shortly afterwards.

Comment 4

[Rob Campbell [:rc] (:robcee)]

Attachment: tree-panel-rollup-20100726

replacement patch for the tree panel incorporating better commenting and reorganization.

Had to disable the DOM panel because it's not checked in yet (requires bug 561782).

Seeing an issue with the style panel not updating properly immediately after inspection. Possibly due to the missing DOM panel piece but not sure.

Still todo: I want to break this up into separate domplate and tree panel patches for easier reviewing.

Looking for feedback on this.

Comment 5

[Rob Campbell [:rc] (:robcee)]

Attachment: domplate

this is a subset of tree-panel-rollup. Includes just the domplate portion of the patch and a minimal test. I plan on breaking up tree-panel-rollup to exclude this portion when the DOM panel pieces land from bug 561782 and dependents.

todo: add some extra domplate tests.

Comment 6

[Julian Viereck]

Comment on attachment 460913 [details] [diff] [review]
domplate

>diff --git a/browser/base/Makefile.in b/browser/base/Makefile.in
>--- a/browser/base/Makefile.in
>+++ b/browser/base/Makefile.in
>@@ -56,6 +56,7 @@
> EXTRA_JS_MODULES = \
> 	content/openLocationLastURL.jsm \
> 	content/NetworkPrioritizer.jsm \
>+	content/domplate.jsm \
> 	content/stylePanel.jsm \
> 	$(NULL)
> 
>diff --git a/browser/base/content/domplate.jsm b/browser/base/content/domplate.jsm
>new file mode 100644
>--- /dev/null
>+++ b/browser/base/content/domplate.jsm
>@@ -0,0 +1,1928 @@
>+/* See license.txt for terms of usage */
>+
>+var EXPORTED_SYMBOLS = ["domplate", "HTMLTemplates", "setDOM"];
>+
>+const Ci = Components.interfaces;
>+const Cu = Components.utils;
>+
>+const invisibleTags = {
>+  "head": true,
>+  "base": true,
>+  "basefont": true,
>+  "isindex": true,
>+  "link": true,
>+  "meta": true,
>+  "script": true,
>+  "style": true,
>+  "title": true,
>+};
>+
>+// End tags for void elements are forbidden
>+// http://wiki.whatwg.org/wiki/HTML_vs._XHTML
>+const selfClosingTags = {
>+  "meta": 1,
>+  "link": 1,
>+  "area": 1,
>+  "base": 1,
>+  "col": 1,
>+  "input": 1,
>+  "img": 1,
>+  "br": 1,
>+  "hr": 1,
>+  "param": 1,
>+  "embed": 1
>+};
>+
>+const reNotWhitespace = /[^\s]/;
>+const showTextNodesWithWhitespace = false;
>+
>+var DOM = {};
>+
>+let setDOM = function(glob)
>+{
>+  DOM = glob;
>+};

o Don't declare anonymous function. Should be: let setDOM = function domplate_setDom(glob)
o Missing JavaDoc
o parameter should be aGlob (prefix arguments with "a")
(points apply for the rest of the patch)

>+
>+let domplate = function()
>+{
>+  let lastSubject;
>+  for (let i = 0; i < arguments.length; ++i)
>+    lastSubject = lastSubject ? copyObject(lastSubject, arguments[i]) : arguments[i];

o ddahl told me not to use let within for loops as TM can't trace them. Use var i = 0 ... instead (apply for the rest of the patch).
o is there a rule to use i++ instead of ++i?

use for (...) {\n ... body ... \n}

>+
>+  for (let name in lastSubject) {
>+    let val = lastSubject[name];
>+    if (isTag(val))
>+      val.tag.subject = lastSubject;

use if (...) { \n ... body ... \n }

>+  }
>+
>+  return lastSubject;
>+};
>+
>+var womb = null;
>+
>+///////////////////////////////////////////////////////////////////////////
>+//// Base functions
>+
>+function DomplateTag(tagName)
>+{
>+    this.tagName = tagName;

Indention is wrong.

>+}
>+
>+function DomplateEmbed()
>+{
>+}
>+
>+function DomplateLoop()
>+{
>+}
>+
>+///////////////////////////////////////////////////////////////////////////
>+//// Definition
>+
>+domplate.context = function(context, fn)
>+{
>+  let lastContext = domplate.lastContext;
>+  domplate.topContext = context;
>+  fn.apply(context);
>+  domplate.topContext = lastContext;
>+};
>+
>+domplate.TAG = function()
>+{
>+  let embed = new DomplateEmbed();
>+  return embed.merge(arguments);
>+};
>+
>+domplate.FOR = function()
>+{
>+  let loop = new DomplateLoop();
>+  return loop.merge(arguments);
>+};
>+
>+DomplateTag.prototype =
>+{
>+  /**
>+   * Initializer for DOM templates. Called to create new Functions objects
>+   * like TR, TD, OBJLINK, etc. See defineTag
>+   * @param args keyword argments for the template, the {} brace stuff after
>+   *        the tag name, eg TR({...}, TD(...
>+   * @param oldTag a nested tag, eg the TD tag in TR({...}, TD(...
>+   */
>+  merge: function(args, oldTag)
>+  {
>+    if (oldTag)
>+      this.tagName = oldTag.tagName;

use if (...) { \n ... body ... \n }

>+
>+    this.context = oldTag ? oldTag.context : null;  // normally null on construction
>+    this.subject = oldTag ? oldTag.subject : null;
>+    this.attrs = oldTag ? copyObject(oldTag.attrs) : {};
>+    this.classes = oldTag ? copyObject(oldTag.classes) : {};
>+    this.props = oldTag ? copyObject(oldTag.props) : null;
>+    this.listeners = oldTag ? copyArray(oldTag.listeners) : null;
>+    this.children = oldTag ? copyArray(oldTag.children) : [];
>+    this.vars = oldTag ? copyArray(oldTag.vars) : [];
>+
>+    let attrs = args.length ? args[0] : null;
>+    let hasAttrs = typeof(attrs) == "object" && !isTag(attrs);
>+
>+    // Do not clear children, they can be copied from the oldTag.
>+    //this.children = [];
>+
>+    if (domplate.topContext)
>+      this.context = domplate.topContext;

use if (...) { \n ... body ... \n }

>+
>+    if (args.length)
>+      parseChildren(args, hasAttrs ? 1 : 0, this.vars, this.children);

use if (...) { \n ... body ... \n }

>+
>+    if (hasAttrs)
>+      this.parseAttrs(attrs);

use if (...) { \n ... body ... \n }

>+
>+    return creator(this, DomplateTag);
>+  },
>+
>+  parseAttrs: function(args)
>+  {
>+    for (let name in args) {
>+      let val = parseValue(args[name]);
>+      readPartNames(val, this.vars);
>+
>+      if (name.indexOf("on") == 0) {
>+        let eventName = name.substr(2);
>+        if (!this.listeners)
>+          this.listeners = [];
>+        this.listeners.push(eventName, val);
>+      } else if (name[0] == "_") {
>+        let propName = name.substr(1);
>+        if (!this.props)

use if (...) { \n ... body ... \n }

>+          this.props = {};
>+        this.props[propName] = val;
>+      } else if (name[0] == "$") {
>+        let className = name.substr(1);
>+        if (!this.classes)

use if (...) { \n ... body ... \n }

>+          this.classes = {};
>+        this.classes[className] = val;
>+      } else {
>+        if (name == "class" && this.attrs.hasOwnProperty(name))

use if (...) { \n ... body ... \n }

>+          this.attrs[name] += " " + val;
>+        else

use else { \n ... body ... \n }

>+          this.attrs[name] = val;
>+      }
>+    }
>+  },

I stop complaining about missing {} for if(...) for(...) etc. There are to many missing in this patch to mention them all the time.

>+
>+  compile: function()
>+  {
>+    if (this.renderMarkup)
>+      return;
>+
>+    this.compileMarkup();
>+    this.compileDOM();
>+  },
>+
>+  compileMarkup: function()
>+  {
>+    this.markupArgs = [];
>+    let topBlock = [], topOuts = [], blocks = [], info = {args: this.markupArgs,
>+      argIndex: 0};

    More readable:

    let topBlock = [], topOuts = [], blocks = [], 
      info = {args: this.markupArgs, argIndex: 0};

>+
>+    this.generateMarkup(topBlock, topOuts, blocks, info);
>+    this.addCode(topBlock, topOuts, blocks);
>+
>+    let fnBlock = ['(function (__code__, __context__, __in__, __out__'];
>+    for (let i = 0; i < info.argIndex; ++i)
>+      fnBlock.push(', s', i);
>+    fnBlock.push(') {\n');
>+
>+    if (this.subject)
>+      fnBlock.push('with (this) {\n');
>+    if (this.context)
>+      fnBlock.push('with (__context__) {\n');
>+    fnBlock.push('with (__in__) {\n');
>+
>+    fnBlock.push.apply(fnBlock, blocks);
>+
>+    if (this.subject)
>+      fnBlock.push('}\n');
>+    if (this.context)
>+      fnBlock.push('}\n');
>+
>+    fnBlock.push('}})\n');
>+
>+    function __link__(tag, code, outputs, args)
>+    {
>+      tag.tag.compile();
>+
>+      let tagOutputs = [];
>+      let markupArgs = [code, tag.tag.context, args, tagOutputs];
>+      markupArgs.push.apply(markupArgs, tag.tag.markupArgs);
>+      tag.tag.renderMarkup.apply(tag.tag.subject, markupArgs);
>+
>+      outputs.push(tag);
>+      outputs.push(tagOutputs);
>+    }
>+
>+    function __escape__(value)
>+    {
>+      function replaceChars(ch)
>+      {
>+        switch (ch) {
>+          case "<":
>+            return "&lt;";
>+          case ">":
>+            return "&gt;";
>+          case "&":
>+            return "&amp;";
>+          case "'":
>+            return "&#39;";
>+          case '"':
>+            return "&quot;";
>+        }
>+        return "?";
>+      };
>+      return String(value).replace(/[<>&"']/g, replaceChars);
>+    }
>+
>+    function __loop__(iter, outputs, fn)
>+    {
>+      let iterOuts = [];
>+      outputs.push(iterOuts);
>+
>+      if (iter instanceof Array)
>+        iter = new ArrayIterator(iter);
>+
>+      try {
>+        while (1) {
>+          let value = iter.next();
>+          let itemOuts = [0,0];

Space after ",": let itemOuts = [0, 0];

>+          iterOuts.push(itemOuts);
>+          fn.apply(this, [value, itemOuts]);
>+        }
>+      } catch (exc) {
>+        if (exc != StopIteration)
>+          throw exc;
>+      }
>+    }
>+
>+    let js = fnBlock.join("");
>+    this.renderMarkup = eval(js);
>+  },
>+
>+  getVarNames: function(args)
>+  {
>+    if (this.vars)
>+      args.push.apply(args, this.vars);
>+
>+    for (let i = 0; i < this.children.length; ++i) {
>+      let child = this.children[i];
>+      if (isTag(child))
>+        child.tag.getVarNames(args);
>+      else if (child instanceof Parts) {
>+        for (let i = 0; i < child.parts.length; ++i) {
>+          if (child.parts[i] instanceof Variable) {
>+            let name = child.parts[i].name;
>+            let names = name.split(".");
>+            args.push(names[0]);
>+          }
>+        }
>+      }
>+    }
>+  },
>+
>+  generateMarkup: function(topBlock, topOuts, blocks, info)
>+  {
>+    topBlock.push(',"<', this.tagName, '"');
>+
>+    for (let name in this.attrs) {
>+      if (name != "class") {
>+        let val = this.attrs[name];
>+        topBlock.push(', " ', name, '=\\""');
>+        addParts(val, ',', topBlock, info, true);
>+        topBlock.push(', "\\""');
>+      }
>+    }
>+
>+    if (this.listeners) {
>+      for (let i = 0; i < this.listeners.length; i += 2)
>+        readPartNames(this.listeners[i+1], topOuts);
>+    }
>+
>+    if (this.props) {
>+      for (let name in this.props)
>+        readPartNames(this.props[name], topOuts);
>+    }
>+
>+    if ( this.attrs.hasOwnProperty("class") || this.classes) {
>+      topBlock.push(', " class=\\""');
>+      if (this.attrs.hasOwnProperty("class"))
>+        addParts(this.attrs["class"], ',', topBlock, info, true);
>+      topBlock.push(', " "');
>+      for (let name in this.classes) {
>+        topBlock.push(', (');
>+        addParts(this.classes[name], '', topBlock, info);
>+        topBlock.push(' ? "', name, '" + " " : "")');
>+      }
>+      topBlock.push(', "\\""');
>+    }
>+    topBlock.push(',">"');
>+
>+    this.generateChildMarkup(topBlock, topOuts, blocks, info);
>+    topBlock.push(',"</', this.tagName, '>"');
>+  },
>+
>+  generateChildMarkup: function(topBlock, topOuts, blocks, info)
>+  {
>+    for (let i = 0; i < this.children.length; ++i) {
>+      let child = this.children[i];
>+      if (isTag(child))
>+        child.tag.generateMarkup(topBlock, topOuts, blocks, info);
>+      else
>+        addParts(child, ',', topBlock, info, true);
>+    }
>+  },
>+
>+  addCode: function(topBlock, topOuts, blocks)
>+  {
>+    if (topBlock.length)
>+      blocks.push('__code__.push(""', topBlock.join(""), ');\n');
>+    if (topOuts.length)
>+      blocks.push('__out__.push(', topOuts.join(","), ');\n');
>+    topBlock.splice(0, topBlock.length);
>+    topOuts.splice(0, topOuts.length);
>+  },
>+
>+  addLocals: function(blocks)
>+  {
>+    let varNames = [];
>+    this.getVarNames(varNames);
>+
>+    let map = {};
>+    for (let i = 0; i < varNames.length; ++i) {
>+      let name = varNames[i];
>+      if ( map.hasOwnProperty(name) )
>+        continue;
>+
>+      map[name] = 1;
>+      let names = name.split(".");
>+      blocks.push('var ', names[0] + ' = ' + '__in__.' + names[0] + ';\n');
>+    }
>+  },
>+
>+  compileDOM: function()
>+  {
>+    let path = [];
>+    let blocks = [];
>+    this.domArgs = [];
>+    path.embedIndex = 0;
>+    path.loopIndex = 0;
>+    path.staticIndex = 0;
>+    path.renderIndex = 0;
>+    let nodeCount = this.generateDOM(path, blocks, this.domArgs);
>+
>+    let fnBlock = ['(function (root, context, o'];
>+
>+    for (let i = 0; i < path.staticIndex; ++i)
>+      fnBlock.push(', ', 's'+i);
>+
>+    for (let i = 0; i < path.renderIndex; ++i)
>+      fnBlock.push(', ', 'd'+i);
>+
>+    fnBlock.push(') {\n');
>+    for (let i = 0; i < path.loopIndex; ++i)
>+      fnBlock.push('var l', i, ' = 0;\n');
>+    for (let i = 0; i < path.embedIndex; ++i)
>+      fnBlock.push('var e', i, ' = 0;\n');
>+
>+    if (this.subject)
>+      fnBlock.push('with (this) {\n');
>+    if (this.context)
>+      fnBlock.push('with (context) {\n');
>+
>+    fnBlock.push(blocks.join(""));
>+
>+    if (this.subject)
>+      fnBlock.push('}\n');
>+    if (this.context)
>+      fnBlock.push('}\n');
>+
>+    fnBlock.push('return ', nodeCount, ';\n');
>+    fnBlock.push('})\n');
>+
>+    function __bind__(object, fn)
>+    {
>+      return function(event) { return fn.apply(object, [event]); }
>+    }
>+
>+    function __link__(node, tag, args)
>+    {
>+      if (!tag || !tag.tag)
>+        return;
>+
>+      tag.tag.compile();
>+
>+      let domArgs = [node, tag.tag.context, 0];
>+      domArgs.push.apply(domArgs, tag.tag.domArgs);
>+      domArgs.push.apply(domArgs, args);
>+
>+      return tag.tag.renderDOM.apply(tag.tag.subject, domArgs);
>+    }
>+
>+    let self = this;
>+    function __loop__(iter, fn)
>+    {
>+      let nodeCount = 0;
>+      for (let i = 0; i < iter.length; ++i) {
>+        iter[i][0] = i;
>+        iter[i][1] = nodeCount;
>+        nodeCount += fn.apply(this, iter[i]);
>+      }
>+      return nodeCount;
>+    }
>+
>+    function __path__(parent, offset)
>+    {
>+      let root = parent;
>+
>+      for (let i = 2; i < arguments.length; ++i) {
>+        let index = arguments[i];
>+        if (i == 3)
>+          index += offset;
>+
>+        if (index == -1)
>+          parent = parent.parentNode;
>+        else
>+          parent = parent.childNodes[index];
>+      }
>+
>+      return parent;
>+    }
>+    let js = fnBlock.join("");
>+    // Exceptions on this line are often in the eval
>+    this.renderDOM = eval(js);
>+  },
>+
>+  generateDOM: function(path, blocks, args)
>+  {
>+    if (this.listeners || this.props)
>+      this.generateNodePath(path, blocks);
>+
>+    if (this.listeners) {
>+      for (let i = 0; i < this.listeners.length; i += 2) {
>+        let val = this.listeners[i+1];

  space around "+": this.listeners[i + 1];

>+        let arg = generateArg(val, path, args);
>+        blocks.push('node.addEventListener("', this.listeners[i], 
>+          '", __bind__(this, ', arg, '), false);\n');
>+      }
>+    }
>+
>+    if (this.props) {
>+      for (let name in this.props) {
>+        let val = this.props[name];
>+        let arg = generateArg(val, path, args);
>+        blocks.push('node.', name, ' = ', arg, ';\n');
>+      }
>+    }
>+
>+    this.generateChildDOM(path, blocks, args);
>+    return 1;
>+  },
>+
>+  generateNodePath: function(path, blocks)
>+  {
>+    blocks.push("var node = __path__(root, o");
>+    for (let i = 0; i < path.length; ++i)
>+      blocks.push(",", path[i]);
>+    blocks.push(");\n");
>+  },
>+
>+  generateChildDOM: function(path, blocks, args)
>+  {
>+    path.push(0);
>+    for (let i = 0; i < this.children.length; ++i) {
>+      let child = this.children[i];
>+      if (isTag(child))
>+        path[path.length-1] += '+' + child.tag.generateDOM(path, blocks, args);

  space around "-": path[path.length - 1] +=

>+      else
>+        path[path.length-1] += '+1';

  space around "-": path[path.length - 1] +=

>+    }
>+    path.pop();
>+  },
>+
>+  /*
>+   * We are just hiding from javascript.options.strict. For some reasons it's ok if we return undefined here.
>+   * @return null or undefined or possibly a context.
>+   */
>+  getContext: function()
>+  {
>+    return this.context;
>+  }
>+};
>+
>+// * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
>+
>+DomplateEmbed.prototype = copyObject(DomplateTag.prototype,
>+{
>+  merge: function(args, oldTag)
>+  {
>+    this.value = oldTag ? oldTag.value : parseValue(args[0]);
>+    this.attrs = oldTag ? oldTag.attrs : {};
>+    this.vars = oldTag ? copyArray(oldTag.vars) : [];
>+
>+    let attrs = args[1];
>+    for (let name in attrs) {
>+      let val = parseValue(attrs[name]);
>+      this.attrs[name] = val;
>+      readPartNames(val, this.vars);
>+    }
>+
>+    return creator(this, DomplateEmbed);
>+  },
>+
>+  getVarNames: function(names)
>+  {
>+    if (this.value instanceof Parts)
>+      names.push(this.value.parts[0].name);
>+
>+    if (this.vars)
>+      names.push.apply(names, this.vars);
>+  },
>+
>+  generateMarkup: function(topBlock, topOuts, blocks, info)
>+  {
>+    this.addCode(topBlock, topOuts, blocks);
>+
>+    blocks.push('__link__(');
>+    addParts(this.value, '', blocks, info);
>+    blocks.push(', __code__, __out__, {\n');
>+
>+    let lastName = null;
>+    for (let name in this.attrs) {
>+      if (lastName)
>+        blocks.push(',');
>+      lastName = name;
>+
>+      let val = this.attrs[name];
>+      blocks.push('"', name, '":');
>+      addParts(val, '', blocks, info);
>+    }
>+
>+    blocks.push('});\n');
>+  },
>+
>+  generateDOM: function(path, blocks, args)
>+  {
>+    let embedName = 'e'+path.embedIndex++;

  space around "+": 'e' + path.embedIndex++;

>+
>+    this.generateNodePath(path, blocks);
>+
>+    let valueName = 'd' + path.renderIndex++;
>+    let argsName = 'd' + path.renderIndex++;
>+    blocks.push(embedName + ' = __link__(node, ', valueName, ', ',
>+      argsName, ');\n');
>+
>+    return embedName;
>+  }
>+});
>+
>+// * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
>+
>+DomplateLoop.prototype = copyObject(DomplateTag.prototype,
>+{
>+  merge: function(args, oldTag)
>+  {
>+    this.isLoop = true;
>+    this.varName = oldTag ? oldTag.varName : args[0];
>+    this.iter = oldTag ? oldTag.iter : parseValue(args[1]);
>+    this.vars = [];
>+
>+    this.children = oldTag ? copyArray(oldTag.children) : [];
>+
>+    let offset = Math.min(args.length, 2);
>+    parseChildren(args, offset, this.vars, this.children);
>+
>+    return creator(this, DomplateLoop);
>+  },
>+
>+  getVarNames: function(names)
>+  {
>+    if (this.iter instanceof Parts)
>+      names.push(this.iter.parts[0].name);
>+
>+    DomplateTag.prototype.getVarNames.apply(this, [names]);
>+  },
>+
>+  generateMarkup: function(topBlock, topOuts, blocks, info)
>+  {
>+    this.addCode(topBlock, topOuts, blocks);
>+
>+    let iterName;
>+    if (this.iter instanceof Parts) {
>+      let part = this.iter.parts[0];
>+      iterName = part.name;
>+
>+      if (part.format) {
>+        for (let i = 0; i < part.format.length; ++i)
>+          iterName = part.format[i] + "(" + iterName + ")";
>+      }
>+    } else
>+      iterName = this.iter;
>+
>+    blocks.push('__loop__.apply(this, [', iterName, ', __out__, function(', this.varName, ', __out__) {\n');
>+    this.generateChildMarkup(topBlock, topOuts, blocks, info);
>+    this.addCode(topBlock, topOuts, blocks);
>+    blocks.push('}]);\n');
>+  },
>+
>+  generateDOM: function(path, blocks, args)
>+  {
>+    let iterName = 'd'+path.renderIndex++;
>+    let counterName = 'i'+path.loopIndex;
>+    let loopName = 'l'+path.loopIndex++;

  Add spaces around the "+"

  let iterName = 'd' + path.renderIndex++;
  let counterName = 'i '+ path.loopIndex;
  let loopName = 'l' + path.loopIndex++;


>+
>+    if (!path.length)
>+        path.push(-1, 0);

  Two spaces to much indented.

>+
>+    let preIndex = path.renderIndex;
>+    path.renderIndex = 0;
>+
>+    let nodeCount = 0;
>+
>+    let subBlocks = [];
>+    let basePath = path[path.length-1];

  Add white spaces around "-": path.length - 1

>+    for (let i = 0; i < this.children.length; ++i) {
>+      path[path.length-1] = basePath+'+'+loopName+'+'+nodeCount;

  Add white spaces around "-" and "+":
    path[path.length - 1] = basePath + '+' + loopName + '+' + nodeCount;

>+
>+      let child = this.children[i];
>+      if (isTag(child))
>+        nodeCount += '+' + child.tag.generateDOM(path, subBlocks, args);
>+      else
>+        nodeCount += '+1';
>+    }
>+
>+    path[path.length-1] = basePath+'+'+loopName;

  Add white spaces around "-" and "+".

>+
>+    blocks.push(loopName,' = __loop__.apply(this, [', iterName, ', function(', counterName,',',loopName);
>+    for (let i = 0; i < path.renderIndex; ++i)
>+      blocks.push(',d'+i);
>+    blocks.push(') {\n');
>+    blocks.push(subBlocks.join(""));
>+    blocks.push('return ', nodeCount, ';\n');
>+    blocks.push('}]);\n');
>+
>+    path.renderIndex = preIndex;
>+
>+    return loopName;
>+  }
>+});
>+
>+///////////////////////////////////////////////////////////////////////////
>+
>+function Variable(name, format)
>+{
>+  this.name = name;
>+  this.format = format;
>+}
>+
>+function Parts(parts)
>+{
>+  this.parts = parts;
>+}
>+
>+///////////////////////////////////////////////////////////////////////////
>+
>+function parseParts(str)
>+{
>+  let re = /\$([_A-Za-z][_A-Za-z0-9.|]*)/g;
>+  let index = 0;
>+  let parts = [];
>+
>+  let m;
>+  while (m = re.exec(str)) {
>+    let pre = str.substr(index, (re.lastIndex-m[0].length)-index);
>+    if (pre)
>+      parts.push(pre);
>+
>+    let expr = m[1].split("|");
>+    parts.push(new Variable(expr[0], expr.slice(1)));
>+    index = re.lastIndex;
>+  }
>+
>+  if (!index)
>+    return str;
>+
>+  let post = str.substr(index);
>+  if (post)
>+    parts.push(post);
>+
>+  return new Parts(parts);
>+}
>+
>+function parseValue(val)
>+{
>+  return typeof(val) == 'string' ? parseParts(val) : val;
>+}
>+
>+function parseChildren(args, offset, vars, children)
>+{
>+  for (let i = offset; i < args.length; ++i) {
>+    let val = parseValue(args[i]);
>+    children.push(val);
>+    readPartNames(val, vars);
>+  }
>+}
>+
>+function readPartNames(val, vars)
>+{
>+  if (val instanceof Parts) {
>+    for (let i = 0; i < val.parts.length; ++i) {
>+      let part = val.parts[i];
>+      if (part instanceof Variable)
>+        vars.push(part.name);
>+    }
>+  }
>+}
>+
>+function generateArg(val, path, args)
>+{
>+  if (val instanceof Parts) {
>+    let vals = [];
>+    for (let i = 0; i < val.parts.length; ++i) {
>+      let part = val.parts[i];
>+      if (part instanceof Variable) {
>+        let varName = 'd'+path.renderIndex++;

  Add spaces around "+":  let varName = 'd' + path.renderIndex++;

>+        if (part.format) {
>+          for (let j = 0; j < part.format.length; ++j)
>+            varName = part.format[j] + '(' + varName + ')';
>+        }
>+
>+        vals.push(varName);
>+      }
>+      else
>+        vals.push('"'+part.replace(/"/g, '\\"')+'"');

  Add spaces around "+":  vals.push('"' + part.replace(/"/g, '\\"') + '"');

>+    }
>+
>+    return vals.join('+');
>+  } else {
>+    args.push(val);
>+    return 's' + path.staticIndex++;
>+  }
>+}
>+
>+function addParts(val, delim, block, info, escapeIt)
>+{
>+  let vals = [];
>+  if (val instanceof Parts) {
>+    for (let i = 0; i < val.parts.length; ++i) {
>+      let part = val.parts[i];
>+      if (part instanceof Variable) {
>+        let partName = part.name;
>+        if (part.format) {
>+          for (let j = 0; j < part.format.length; ++j)
>+            partName = part.format[j] + "(" + partName + ")";
>+        }
>+
>+        if (escapeIt)
>+          vals.push("__escape__(" + partName + ")");
>+        else
>+          vals.push(partName);
>+      }
>+      else
>+        vals.push('"'+ part + '"');
>+    }
>+  } else if (isTag(val)) {
>+    info.args.push(val);
>+    vals.push('s'+info.argIndex++);

  Add spaces around "+":  vals.push('s' + info.argIndex++);

>+  } else
>+    vals.push('"'+ val + '"');

  Add spaces around "+": vals.push('"' + val + '"');

>+
>+  let parts = vals.join(delim);
>+  if (parts)
>+    block.push(delim, parts);
>+}
>+
>+function isTag(obj)
>+{
>+  return (typeof(obj) == "function" || obj instanceof Function) && !!obj.tag;
>+}
>+
>+///////////////////////////////////////////////////////////////////////////
>+//// creator
>+
>+function creator(tag, cons)
>+{
>+  let fn = new Function(
>+    "var tag = arguments.callee.tag;" +
>+    "var cons = arguments.callee.cons;" +
>+    "var newTag = new cons();" +
>+    "return newTag.merge(arguments, tag);");
>+
>+  fn.tag = tag;
>+  fn.cons = cons;
>+  extend(fn, Renderer);
>+
>+  return fn;
>+}
>+
>+///////////////////////////////////////////////////////////////////////////
>+//// Utility functions
>+
>+function arrayInsert(array, index, other)
>+{
>+  for (let i = 0; i < other.length; ++i)
>+    array.splice(i+index, 0, other[i]);
>+
>+  return array;
>+}
>+
>+function cloneArray(array, fn)
>+{
>+  let newArray = [];
>+
>+  if (fn)
>+    for (var i = 0; i < array.length; ++i)
>+      newArray.push(fn(array[i]));
>+  else
>+    for (var i = 0; i < array.length; ++i)
>+      newArray.push(array[i]);
>+
>+  return newArray;
>+}
>+
>+// fn, thisObject, args => thisObject.fn(args, arguments);
>+function bind()
>+{
>+  let args = cloneArray(arguments), fn = args.shift(), object = args.shift();
>+  return function bind()
>+  { 
>+    return fn.apply(object, arrayInsert(cloneArray(args), 0, arguments));
>+  }
>+}
>+
>+function copyArray(oldArray)
>+{
>+  let array = [];
>+  if (oldArray)
>+    for (let i = 0; i < oldArray.length; ++i)
>+      array.push(oldArray[i]);
>+  return array;
>+}
>+
>+function copyObject(l, r)
>+{
>+  let m = {};
>+  extend(m, l);
>+  extend(m, r);
>+  return m;
>+}
>+
>+function escapeNewLines(value)
>+{
>+  return value.replace(/\r/gm, "\\r").replace(/\n/gm, "\\n");
>+}
>+
>+function extend(l, r)
>+{
>+  for (let n in r)
>+    l[n] = r[n];
>+}
>+
>+function cropString(text, limit, alterText)
>+{
>+  if (!alterText)
>+    alterText = "..."; //…
>+
>+  text = text + "";
>+
>+  if (!limit)
>+    limit = 88; // todo

  "todo" <<< what is the todo?

>+  var halfLimit = (limit / 2);
>+  halfLimit -= 2; // adjustment for alterText's increase in size
>+
>+  if (text.length > limit)
>+    return text.substr(0, halfLimit) + alterText + text.substr(text.length-halfLimit);

  Add spaces around "-": ... + text.substr(text.length - halfLimit);

>+  else
>+    return text;
>+}
>+
>+function cropMultipleLines(text, limit)
>+{
>+  return escapeNewLines(this.cropString(text, limit));
>+}
>+
>+function isVisible(elt)
>+{
>+  if (elt.localName) {
>+    return elt.offsetWidth > 0 || elt.offsetHeight > 0 || 
>+      elt.localName.toLowerCase() in invisibleTags;
>+  } else {
>+    return elt.offsetWidth > 0 || elt.offsetHeight > 0;
>+  }
>+    // || isElementSVG(elt) || isElementMathML(elt);
>+}
>+
>+// Local Helpers
>+
>+function isElementXHTML(node)
>+{
>+  return node.nodeName == node.nodeName.toLowerCase();
>+}
>+
>+function isContainerElement(element)
>+{
>+  let tag = element.localName.toLowerCase();
>+  switch (tag) {
>+    case "script":
>+    case "style":
>+    case "iframe":
>+    case "frame":
>+    case "tabbrowser":
>+    case "browser":
>+      return true;
>+    case "link":
>+      return element.getAttribute("rel") == "stylesheet";
>+    case "embed":
>+      return element.getSVGDocument();
>+  }
>+  return false;
>+}
>+
>+function isWhitespace(text)
>+{
>+  return !reNotWhitespace.exec(text);
>+};
>+
>+function isWhitespaceText(node)
>+{
>+  if (node instanceof DOM.HTMLAppletElement)
>+    return false;
>+  return node.nodeType == DOM.Node.TEXT_NODE && isWhitespace(node.nodeValue);
>+}
>+
>+function isSelfClosing(element)
>+{
>+  //if (isElementSVG(element) || isElementMathML(element))
>+  //    return true;
>+  var tag = element.localName.toLowerCase();
>+  return (selfClosingTags.hasOwnProperty(tag));
>+};
>+
>+function isEmptyElement(element)
>+{
>+  if (showTextNodesWithWhitespace) {
>+    return !element.firstChild && isSelfClosing(element);
>+  } else {
>+    for (var child = element.firstChild; child; child = child.nextSibling) {
>+      if (!isWhitespaceText(child))
>+        return false;
>+    }
>+  }
>+  return isSelfClosing(element);
>+}
>+
>+function getEmptyElementTag(node)
>+{
>+  let isXhtml= isElementXHTML(node);
>+  if (isXhtml)
>+    return HTMLTemplates.XEmptyElement.tag;
>+  else
>+    return HTMLTemplates.EmptyElement.tag;
>+}
>+
>+/**
>+ * Determines if the given node has any children which are elements.
>+ *
>+ * @param {Element} element Element to test.
>+ * @return true if immediate children of type Element exist, false otherwise
>+ */
>+function hasNoElementChildren(element)
>+{
>+  if (element.childElementCount != 0)  // FF 3.5+
>+    return false;
>+
>+  // https://developer.mozilla.org/en/XBL/XBL_1.0_Reference/DOM_Interfaces
>+  if (element.ownerDocument instanceof Ci.nsIDOMDocumentXBL) {
>+    let anonChildren = element.ownerDocument.getAnonymousNodes(element);
>+    if (anonChildren) {
>+      for (var i = 0; i < anonChildren.length; i++) {
>+        if (anonChildren[i].nodeType == Node.ELEMENT_NODE)
>+          return false;
>+      }
>+    }
>+  }
>+  return true;
>+}
>+
>+function getNodeTag(node, expandAll)
>+{
>+  if (node instanceof DOM.Element) {
>+    if (node instanceof DOM.HTMLHtmlElement && node.ownerDocument 
>+        && node.ownerDocument.doctype)
>+      return HTMLTemplates.HTMLHtmlElement.tag;
>+    else if (node instanceof DOM.HTMLAppletElement)
>+      return getEmptyElementTag(node);
>+    else if (isContainerElement(node))
>+      return HTMLTemplates.Element.tag;
>+    else if (isEmptyElement(node))
>+      return getEmptyElementTag(node);
>+    else if (hasNoElementChildren(node))
>+      return HTMLTemplates.TextElement.tag;
>+    else
>+      return HTMLTemplates.Element.tag;
>+  }
>+  else if (node instanceof DOM.Text)
>+    return HTMLTemplates.TextNode.tag;
>+  else if (node instanceof DOM.CDATASection)
>+    return HTMLTemplates.CDATANode.tag;
>+  else if (node instanceof DOM.Comment)
>+    return HTMLTemplates.CommentNode.tag;
>+  else if (node instanceof DOM.SourceText)
>+    return HTMLTemplates.SourceText.tag;
>+  else
>+    return HTMLTemplates.Nada.tag;
>+}
>+
>+function getNodeBoxTag(nodeBox)
>+{
>+  let re = /([^\s]+)NodeBox/;
>+  let m = re.exec(nodeBox.className);
>+  if (!m)
>+    return null;
>+
>+  let nodeBoxType = m[1];
>+  if (nodeBoxType == "container")
>+    return HTMLTemplates.Element.tag;
>+  else if (nodeBoxType == "text")
>+    return HTMLTemplates.TextElement.tag;
>+  else if (nodeBoxType == "empty")
>+    return HTMLTemplates.EmptyElement.tag;
>+}
>+
>+///////////////////////////////////////////////////////////////////////////
>+//// ArrayIterator
>+
>+function ArrayIterator(array)
>+{
>+  let index = -1;
>+
>+  this.next = function()
>+  {
>+    if (++index >= array.length)
>+      throw StopIteration;
>+
>+    return array[index];
>+  };
>+}
>+
>+function StopIteration() {}
>+
>+domplate.$break = function()
>+{
>+  throw StopIteration;
>+};
>+
>+///////////////////////////////////////////////////////////////////////////
>+//// Renderer
>+
>+var Renderer =
>+{
>+  renderHTML: function(args, outputs, self)
>+  {
>+    let code = [];
>+    let markupArgs = [code, this.tag.getContext(), args, outputs];
>+    markupArgs.push.apply(markupArgs, this.tag.markupArgs);
>+    this.tag.renderMarkup.apply(self ? self : this.tag.subject, markupArgs);
>+    return code.join("");
>+  },
>+
>+  insertRows: function(args, before, self)
>+  {
>+    if (!args)
>+      args = {};
>+
>+    this.tag.compile();
>+
>+    let outputs = [];
>+    let html = this.renderHTML(args, outputs, self);
>+
>+    let doc = before.ownerDocument;
>+    let table = doc.createElement("table");
>+    table.innerHTML = html;
>+
>+    let tbody = table.firstChild;
>+    let parent = before.localName.toLowerCase() == "tr" ? before.parentNode : before;
>+    let after = before.localName.toLowerCase() == "tr" ? before.nextSibling : null;
>+
>+    let firstRow = tbody.firstChild, lastRow;
>+    while (tbody.firstChild) {
>+      lastRow = tbody.firstChild;
>+      if (after)
>+        parent.insertBefore(lastRow, after);
>+      else
>+        parent.appendChild(lastRow);
>+    }
>+
>+    // To save the next poor soul:
>+    // In order to properly apply properties and event handlers on elements
>+    // constructed by a FOR tag, the tag needs to be able to iterate up and
>+    // down the tree, meaning if FOR is the root element as is the case with
>+    // many insertRows calls, it will need to iterator over portions of the
>+    // new parent.
>+    //
>+    // To achieve this end, __path__ defines the -1 operator which allows
>+    // parent traversal. When combined with the offset that we calculate
>+    // below we are able to iterate over the elements.
>+    //
>+    // This fails when applied to a non-loop element as non-loop elements
>+    // Do not generate to proper path to bounce up and down the tree.
>+    let offset = 0;
>+    if (this.tag.isLoop) {
>+      let node = firstRow.parentNode.firstChild;
>+      for (; node && node != firstRow; node = node.nextSibling)
>+        ++offset;
>+    }
>+
>+    // strict warning: this.tag.context undefined
>+    let domArgs = [firstRow, this.tag.getContext(), offset];
>+    domArgs.push.apply(domArgs, this.tag.domArgs);
>+    domArgs.push.apply(domArgs, outputs);
>+
>+    this.tag.renderDOM.apply(self ? self : this.tag.subject, domArgs);
>+    return [firstRow, lastRow];
>+  },
>+
>+  insertBefore: function(args, before, self)
>+  {
>+    return this.insertNode(args, before.ownerDocument,
>+      function(frag) {
>+        before.parentNode.insertBefore(frag, before);
>+      }, self);
>+  },
>+
>+  insertAfter: function(args, after, self)
>+  {
>+    return this.insertNode(args, after.ownerDocument,
>+      function(frag) {
>+        after.parentNode.insertBefore(frag, after.nextSibling);
>+      }, self);
>+  },
>+
>+  insertNode: function(args, doc, inserter, self)
>+  {
>+    if (!args)
>+      args = {};
>+
>+    this.tag.compile();
>+
>+    let outputs = [];
>+    let html = this.renderHTML(args, outputs, self);
>+
>+    let range = doc.createRange();
>+    range.selectNode(doc.body);
>+    let frag = range.createContextualFragment(html);
>+
>+    let root = frag.firstChild;
>+    root = inserter(frag) || root;
>+
>+    let domArgs = [root, this.tag.context, 0];
>+    domArgs.push.apply(domArgs, this.tag.domArgs);
>+    domArgs.push.apply(domArgs, outputs);
>+
>+    this.tag.renderDOM.apply(self ? self : this.tag.subject, domArgs);
>+
>+    return root;
>+  },
>+
>+  replace: function(args, parent, self)
>+  {
>+    if (!args)
>+      args = {};
>+
>+    this.tag.compile();
>+
>+    let outputs = [];
>+    let html = this.renderHTML(args, outputs, self);
>+
>+    let root;
>+    if (parent.nodeType == 1) {
>+      parent.innerHTML = html;
>+      root = parent.firstChild;
>+    } else {
>+      if (!parent || parent.nodeType != 9)
>+        parent = document;
>+
>+      if (!womb || womb.ownerDocument != parent)
>+        womb = parent.createElement("div");
>+
>+      womb.innerHTML = html;
>+
>+      root = womb.firstChild;
>+    }
>+
>+    let domArgs = [root, this.tag.context, 0];
>+    domArgs.push.apply(domArgs, this.tag.domArgs);
>+    domArgs.push.apply(domArgs, outputs);
>+    this.tag.renderDOM.apply(self ? self : this.tag.subject, domArgs);
>+
>+    return root;
>+  },
>+
>+  append: function(args, parent, self)
>+  {
>+    if (!args)
>+      args = {};
>+
>+    this.tag.compile();
>+
>+    let outputs = [];
>+    let html = this.renderHTML(args, outputs, self);
>+
>+    if (!womb || womb.ownerDocument != parent.ownerDocument)
>+      womb = parent.ownerDocument.createElement("div");
>+    womb.innerHTML = html;
>+
>+    let root = womb.firstChild;
>+    while (womb.firstChild)
>+      parent.appendChild(womb.firstChild);
>+
>+    let domArgs = [root, this.tag.context, 0];
>+    domArgs.push.apply(domArgs, this.tag.domArgs);
>+    domArgs.push.apply(domArgs, outputs);
>+
>+    this.tag.renderDOM.apply(self ? self : this.tag.subject, domArgs);
>+
>+    return root;
>+  }
>+};
>+
>+///////////////////////////////////////////////////////////////////////////
>+//// defineTags macro
>+
>+/**
>+ * Create default tags for a list of tag names.
>+ * @param Arguments
>+ *        list of string arguments
>+ */
>+
>+function defineTags()
>+{
>+  for (let i = 0; i < arguments.length; ++i) {
>+    let tagName = arguments[i];
>+    let fn = new Function("var newTag = new DomplateTag('" + tagName + 
>+      "'); return newTag.merge(arguments);");
>+
>+    let fnName = tagName.toUpperCase();
>+    domplate[fnName] = fn;
>+  }
>+}
>+
>+defineTags(
>+  "a", "button", "br", "canvas", "col", "colgroup", "div", "fieldset", "form",
>+  "h1", "h2", "h3", "hr", "img", "input", "label", "legend", "li", "ol",
>+  "optgroup", "option", "p", "pre", "select", "b", "span", "strong", "table",
>+  "tbody", "td", "textarea", "tfoot", "th", "thead", "tr", "tt", "ul", "iframe",
>+  "code"
>+);
>+
>+///////////////////////////////////////////////////////////////////////////
>+//// HTMLTemplates
>+
>+let HTMLTemplates = {
>+  showTextNodesWithWhitespace: false
>+};
>+
>+let BaseTemplates = {
>+  showTextNodesWithWhitespace: false
>+};
>+
>+///////////////////////////////////////////////////////////////////////////
>+//// HTMLTemplates.Reps
>+
>+BaseTemplates.OBJECTLINK = domplate.A({
>+  "class": "objectLink objectLink-$className a11yFocus",
>+  _repObject: "$object"
>+});
>+
>+BaseTemplates.Rep = domplate(
>+{
>+  className: "",
>+  inspectable: true,
>+
>+  supportsObject: function(object, type)
>+  {
>+    return false;
>+  },
>+
>+  inspectObject: function(object, context)
>+  {
>+    // Firebug.chrome.select(object);  // todo
>+  },
>+
>+  browseObject: function(object, context)
>+  {
>+  },
>+
>+  persistObject: function(object, context)
>+  {
>+  },
>+
>+  getRealObject: function(object, context)
>+  {
>+    return object;
>+  },
>+
>+  getTitle: function(object)
>+  { // todo
>+    let label = safeToString(object); // eg [object XPCWrappedNative [object foo]]
>+
>+    const re =/\[object ([^\]]*)/;
>+    let m = re.exec(label);
>+    let n = null;
>+    if (m)
>+      n = re.exec(m[1]);  // eg XPCWrappedNative [object foo

  As there is more then just one match, can you give m and n a more verbose name?

>+
>+    if (n)
>+      return n[1];  // eg foo
>+    else
>+      return m ? m[1] : label;
>+  },
>+
>+  getTooltip: function(object)
>+  {
>+    return null;
>+  },
>+
>+  /*
>+  * Called by chrome.onContextMenu to build the context menu when the underlying object has this rep.

  Far more then 80 characters per line.

>+  * See also Panel for a similar function also called by onContextMenu
>+  * Extensions may monkey patch and chain off this call

  Missing point at the end of sentence.

>+  * @param object: the 'realObject', a model value, eg a DOM property
>+  * @param target: the HTML element clicked on.
>+  * @param context: the context, probably FirebugContext
>+  * @return an array of menu items.
>+  */
>+  getContextMenuItems: function(object, target, context)
>+  {
>+    return [];
>+  },
>+
>+  /////////////////////////////////////////////////////////////////////////
>+  // Convenience for domplates
>+
>+  STR: function(name)
>+  {
>+    return name; // todo getproperty?
>+  },
>+
>+  cropString: function(text)
>+  {
>+    return cropString(text);
>+  },
>+
>+  cropMultipleLines: function(text, limit)
>+  {
>+    return cropMultipleLines(text, limit);
>+  },
>+
>+  toLowerCase: function(text)
>+  {
>+    return text ? text.toLowerCase() : text;
>+  },
>+
>+  plural: function(n)
>+  {
>+    return n == 1 ? "" : "s";
>+  }
>+});
>+
>+BaseTemplates.Element = domplate(BaseTemplates.Rep,
>+{
>+  tag:
>+    BaseTemplates.OBJECTLINK(
>+      "&lt;",
>+      domplate.SPAN({"class": "nodeTag"},
>+        "$object.localName|toLowerCase"),
>+      domplate.FOR("attr", "$object|attrIterator",
>+        "&nbsp;$attr.localName=&quot;",
>+        domplate.SPAN({"class": "nodeValue"},
>+          "$attr.nodeValue"),
>+        "&quot;"
>+      ),
>+      "&gt;"
>+    ),
>+
>+  shortTag:
>+    BaseTemplates.OBJECTLINK(
>+      domplate.SPAN({"class": "$object|getVisible"},
>+        domplate.SPAN({"class": "selectorTag"},
>+          "$object|getSelectorTag"),
>+        domplate.SPAN({"class": "selectorId"},
>+          "$object|getSelectorId"),
>+        domplate.SPAN({"class": "selectorClass"},
>+          "$object|getSelectorClass"),
>+        domplate.SPAN({"class": "selectorValue"},
>+          "$object|getValue")
>+      )
>+    ),
>+
>+  getVisible: function(elt)
>+  {
>+    return isVisible(elt) ? "" : "selectorHidden";
>+  },
>+
>+  getSelectorTag: function(elt)
>+  {
>+    return elt.localName.toLowerCase();
>+  },
>+
>+  getSelectorId: function(elt)
>+  {
>+    return elt.id ? ("#" + elt.id) : "";
>+  },
>+
>+  getSelectorClass: function(elt)
>+  {
>+    return elt.getAttribute("class")
>+      ? ("." + elt.getAttribute("class").split(" ")[0])
>+      : "";
>+  },
>+
>+  getValue: function(elt)
>+  { // todo getFileName
>+    let value;
>+/*
>+    if (elt instanceof HTMLImageElement)
>+      value = getFileName(elt.getAttribute("src"));
>+    else if (elt instanceof HTMLAnchorElement)
>+      value = getFileName(elt.getAttribute("href"));
>+    else if (elt instanceof HTMLInputElement)
>+      value = elt.getAttribute("value");
>+    else if (elt instanceof HTMLFormElement)
>+      value = getFileName(elt.getAttribute("action"));
>+    else if (elt instanceof HTMLScriptElement)
>+      value = getFileName(elt.getAttribute("src"));
>+
>+    return value ? " " + cropMultipleLines(value, 20) : ""; */
>+    // trying a simplified version from above commented section
>+    // todo
>+    if (elt instanceof DOM.HTMLImageElement)
>+      value = elt.getAttribute("src");
>+    else if (elt instanceof DOM.HTMLAnchorElement)
>+      value = elt.getAttribute("href");
>+    else if (elt instanceof DOM.HTMLInputElement)
>+      value = elt.getAttribute("value");
>+    else if (elt instanceof DOM.HTMLFormElement)
>+      value = elt.getAttribute("action");
>+    else if (elt instanceof DOM.HTMLScriptElement)
>+      value = elt.getAttribute("src");
>+
>+    return value ? " " + cropMultipleLines(value, 20) : "";
>+  },
>+
>+  attrIterator: function(elt)
>+  {
>+    let attrs = [];
>+    let idAttr, classAttr;
>+    if (elt.attributes) {
>+      for (let i = 0; i < elt.attributes.length; ++i) {
>+        var attr = elt.attributes[i];
>+        if (attr.localName.indexOf("-moz-math") != -1)
>+          continue;
>+        else if (attr.localName == "id")
>+          idAttr = attr;
>+        else if (attr.localName == "class")
>+          classAttr = attr;
>+        else
>+          attrs.push(attr);
>+      }
>+    }
>+    if (classAttr)
>+      attrs.splice(0, 0, classAttr);
>+    if (idAttr)
>+      attrs.splice(0, 0, idAttr);
>+    return attrs;
>+  },
>+
>+  shortAttrIterator: function(elt)
>+  {
>+    let attrs = [];
>+    if (elt.attributes) {
>+      for (let i = 0; i < elt.attributes.length; ++i) {
>+        let attr = elt.attributes[i];
>+          if (attr.localName == "id" || attr.localName == "class")
>+            attrs.push(attr);
>+      }
>+    }
>+
>+    return attrs;
>+  },
>+
>+  getHidden: function(elt)
>+  {
>+    return isVisible(elt) ? "" : "nodeHidden";
>+  },
>+
>+/* getXPath: function(elt)
>+  {
>+    return getElementTreeXPath(elt); // todo
>+  }, */
>+
>+  getNodeTextGroups: function(element)
>+  { // todo

  todo what?

>+    let text =  element.textContent;
>+/*    if (!Firebug.showFullTextNodes) {
>+      text=cropString(text,50);
>+    } */
>+
>+    // let escapeGroups=[];
>+
>+/*    if (Firebug.showTextNodesWithWhitespace)
>+      escapeGroups.push({
>+        'group': 'whitespace',
>+        'class': 'nodeWhiteSpace',
>+        'extra': {
>+          '\t': '_Tab',
>+          '\n': '_Para',
>+          ' ' : '_Space'
>+        }
>+      });
>+
>+    if (Firebug.showTextNodesWithEntities)
>+      escapeGroups.push({
>+        'group':'text',
>+        'class':'nodeTextEntity',
>+        'extra':{}
>+      });
>+
>+    if (escapeGroups.length)
>+      return escapeGroupsForEntities(text, escapeGroups);
>+    else */
>+      return [{str: text, 'class': '', extra: ''}];
>+  },
>+
>+  /////////////////////////////////////////////////////////////////////////
>+/*
>+  copyHTML: function(elt)
>+  {
>+    let html = getElementHTML(elt); // todo
>+    copyToClipboard(html);          // todo
>+  },
>+
>+  copyInnerHTML: function(elt)
>+  {
>+    copyToClipboard(elt.innerHTML); // todo
>+  },
>+
>+  copyXPath: function(elt)
>+  {
>+    let xpath = getElementXPath(elt); // todo
>+    copyToClipboard(xpath);           // todo
>+  },
>+
>+  copyCSSPath: function(elt)
>+  {
>+    let csspath = getElementCSSPath(elt); // todo
>+    copyToClipboard(csspath);             // todo
>+  },
>+
>+  persistor: function(context, xpath)
>+  {
>+    let elts = xpath
>+      ? getElementsByXPath(context.window.document, xpath) // todo
>+      : null;
>+
>+    return elts && elts.length ? elts[0] : null;
>+  },
>+*/
>+  /////////////////////////////////////////////////////////////////////////
>+
>+  className: "element",
>+
>+  supportsObject: function(object, type)
>+  {
>+    return object instanceof DOM.Element;
>+  },
>+
>+  browseObject: function(elt, context)
>+  {
>+    let tag = elt.localName.toLowerCase();
>+/*    if (tag == "script")
>+      openNewTab(elt.src);
>+    else if (tag == "link")
>+      openNewTab(elt.href);
>+    else if (tag == "a")
>+      openNewTab(elt.href);
>+    else if (tag == "img")
>+      openNewTab(elt.src);
>+*/
>+    return true;
>+  },
>+/*
>+  persistObject: function(elt, context)
>+  {
>+    let xpath = getElementXPath(elt);
>+
>+    return bind(this.persistor, top, xpath); // todo
>+  },
>+
>+  getTitle: function(element, context)
>+  {
>+      return getElementCSSSelector(element);
>+  },
>+
>+  getTooltip: function(elt)
>+  {
>+      return this.getXPath(elt);
>+  },
>+
>+  getContextMenuItems: function(elt, target, context)
>+  {
>+      var monitored = areEventsMonitored(elt, null, context);
>+      var CopyElement = "CopyHTML";
>+      if (isElementSVG(elt))
>+          CopyElement = "CopySVG";
>+      if (isElementMathML(elt))
>+          CopyElement = "CopyMathML";
>+
>+      var items=[{label: CopyElement, command: bindFixed(this.copyHTML, this, elt)}];
>+      if (!isElementSVG(elt) && !isElementMathML(elt))
>+          items.push({label: "CopyInnerHTML", command: bindFixed(this.copyInnerHTML, this, elt) });
>+
>+      return items.concat([
>+          {label: "CopyXPath", id: "fbCopyXPath", command: bindFixed(this.copyXPath, this, elt) },
>+          {label: "Copy CSS Path", id: "fbCopyCSSPath", command: bindFixed(this.copyCSSPath, this, elt) },
>+          "-",
>+          {label: "ShowEventsInConsole", id: "fbShowEventsInConsole", type: "checkbox", checked: monitored,
>+           command: bindFixed(toggleMonitorEvents, FBL, elt, null, monitored, context) },
>+          "-",
>+          {label: "ScrollIntoView", id: "fbScrollIntoView", command: bindFixed(elt.scrollIntoView, elt) }
>+      ]);
>+  }
>+  */
>+});
>+
>+
>+///////////////////////////////////////////////////////////////////////////
>+//// HTMLTemplates.tags
>+
>+BaseTemplates.AttrTag =
>+  domplate.SPAN({"class": "nodeAttr editGroup"},
>+    "&nbsp;",
>+    domplate.SPAN({"class": "nodeName editable"}, "$attr.nodeName"),
>+    "=&quot;",
>+    domplate.SPAN({"class": "nodeValue editable"}, "$attr.nodeValue"),
>+    "&quot;");
>+
>+BaseTemplates.TextTag =
>+  domplate.SPAN({"class": "nodeText editable"},
>+    domplate.FOR("char", "$object|getNodeTextGroups",
>+      domplate.SPAN({"class": "$char.class $char.extra"},
>+        "$char.str")));
>+
>+///////////////////////////////////////////////////////////////////////////
>+//// HTMLTemplates
>+
>+
>+
>+HTMLTemplates.CompleteElement = domplate(BaseTemplates.Element,
>+{
>+  tag:
>+    domplate.DIV({"class":
>+        "nodeBox open $object|getHidden repIgnore",
>+        _repObject: "$object", role : 'presentation'},
>+      domplate.DIV({"class": "nodeLabel", role: "presentation"},
>+        domplate.SPAN({"class": "nodeLabelBox repTarget repTarget",
>+          role : 'treeitem', 'aria-expanded' : 'false'},
>+          "&lt;",
>+          domplate.SPAN({"class": "nodeTag"},
>+            "$object.nodeName|toLowerCase"),
>+          domplate.FOR("attr", "$object|attrIterator", BaseTemplates.AttrTag),
>+          domplate.SPAN({"class": "nodeBracket"}, "&gt;")
>+        )
>+      ),
>+      domplate.DIV({"class": "nodeChildBox", role :"group"},
>+        domplate.FOR("child", "$object|childIterator",
>+          domplate.TAG("$child|getNodeTag", {object: "$child"})
>+        )
>+      ),
>+      domplate.DIV({"class": "nodeCloseLabel", role:"presentation"},
>+        "&lt;/",
>+        domplate.SPAN({"class": "nodeTag"},
>+          "$object.nodeName|toLowerCase"),
>+        "&gt;"
>+      )
>+    ),
>+
>+  getNodeTag: function(node)
>+  {
>+    return getNodeTag(node, true);
>+  },
>+
>+  childIterator: function(node)
>+  {
>+    if (node.contentDocument)
>+      return [node.contentDocument.documentElement];
>+
>+    if (this.showTextNodesWithWhitespace)
>+      return cloneArray(node.childNodes);
>+    else {
>+      let nodes = [];
>+      for (var child = node.firstChild; child; child = child.nextSibling) {
>+        if (child.nodeType != DOM.Node.TEXT_NODE || !isWhitespaceText(child))
>+          nodes.push(child);
>+      }
>+      return nodes;
>+    }
>+  }
>+});
>+
>+HTMLTemplates.SoloElement = domplate(HTMLTemplates.CompleteElement,
>+{
>+  tag:
>+    domplate.DIV({"class": "soloElement",
>+      onmousedown: "$onMouseDown"},
>+      HTMLTemplates.CompleteElement.tag),
>+
>+  onMouseDown: function(event)
>+  {
>+    for (let child = event.target; child; child = child.parentNode) {
>+      if (child.repObject) { // todo
>+          // let panel = Firebug.getElementPanel(child);
>+          // Firebug.chrome.select(child.repObject);
>+          break;

  Fix indention.

>+      }
>+    }
>+  }
>+});
>+
>+HTMLTemplates.Element = domplate(BaseTemplates.Element,
>+{
>+  tag:
>+    domplate.DIV({"class": "nodeBox containerNodeBox $object|getHidden repIgnore",
>+      _repObject: "$object", role: "presentation"},
>+      domplate.DIV({"class": "nodeLabel", role: "presentation"},
>+        domplate.IMG({"class": "twisty", role: "presentation"}),
>+        domplate.SPAN({"class": "nodeLabelBox repTarget",
>+          role: 'treeitem', 'aria-expanded': 'false'},
>+          "&lt;",
>+          domplate.SPAN({"class": "nodeTag"},
>+            "$object.nodeName|toLowerCase"),
>+          domplate.FOR("attr", "$object|attrIterator", BaseTemplates.AttrTag),
>+          domplate.SPAN({"class": "nodeBracket editable insertBefore"},
>+            "&gt;")
>+        )
>+      ),
>+      domplate.DIV({"class": "nodeChildBox", role: "group"}), /* nodeChildBox is special signal in insideOutBox */

  Far more then 80 characters.

>+      domplate.DIV({"class": "nodeCloseLabel", role: "presentation"},
>+        domplate.SPAN({"class": "nodeCloseLabelBox repTarget"},
>+          "&lt;/",
>+          domplate.SPAN({"class": "nodeTag"}, "$object.nodeName|toLowerCase"),
>+          "&gt;"
>+        )
>+      )
>+    )
>+});
>+
>+HTMLTemplates.HTMLHtmlElement = domplate(BaseTemplates.Element,
>+{
>+  tag:
>+    domplate.DIV({"class":
>+        "nodeBox htmlNodeBox containerNodeBox $object|getHidden repIgnore",
>+        _repObject: "$object", role: "presentation"},
>+      domplate.DIV({"class": "docType $object"},
>+        "$object|getDocType"),
>+      domplate.DIV({"class": "nodeLabel", role: "presentation"},
>+        domplate.IMG({"class": "twisty", role: "presentation"}),
>+        domplate.SPAN({"class": "nodeLabelBox repTarget",
>+            role: 'treeitem', 'aria-expanded' : 'false'},
>+          "&lt;",
>+          domplate.SPAN({"class": "nodeTag"},
>+            "$object.nodeName|toLowerCase"),
>+          domplate.FOR("attr", "$object|attrIterator", BaseTemplates.AttrTag),
>+          domplate.SPAN({"class":
>+            "nodeBracket editable insertBefore"}, "&gt;")
>+        )
>+      ), /* nodeChildBox is special signal in insideOutBox */
>+      domplate.DIV({"class": "nodeChildBox", role: "group"}),
>+      domplate.DIV({"class": "nodeCloseLabel", role:  "presentation"},
>+        domplate.SPAN({"class": "nodeCloseLabelBox repTarget"},
>+          "&lt;/",
>+          domplate.SPAN({"class": "nodeTag"},
>+            "$object.nodeName|toLowerCase"),
>+          "&gt;"
>+        )
>+      )
>+    ),
>+
>+  getDocType: function(obj)
>+  {
>+    let doctype = obj.ownerDocument.doctype;
>+    return '<!DOCTYPE ' + doctype.name + (doctype.publicId ? ' PUBLIC "' +
>+      doctype.publicId + '"': '') + (doctype.systemId ? ' "' + 
>+      doctype.systemId + '"' : '') + '>';
>+  }
>+});
>+
>+HTMLTemplates.TextElement = domplate(BaseTemplates.Element,
>+{
>+  tag:
>+    domplate.DIV({"class":
>+        "nodeBox textNodeBox $object|getHidden repIgnore",
>+        _repObject: "$object", role: 'presentation'},
>+      domplate.DIV({"class": "nodeLabel", role: "presentation"},
>+        domplate.SPAN({"class": "nodeLabelBox repTarget", 
>+            role: 'treeitem'},
>+          "&lt;",
>+          domplate.SPAN({"class": "nodeTag"},
>+            "$object.nodeName|toLowerCase"),
>+          domplate.FOR("attr", "$object|attrIterator", BaseTemplates.AttrTag),
>+          domplate.SPAN({"class":
>+            "nodeBracket editable insertBefore"}, "&gt;"),
>+          BaseTemplates.TextTag,
>+          "&lt;/",
>+          domplate.SPAN({"class": "nodeTag"},
>+            "$object.nodeName|toLowerCase"),
>+          "&gt;"
>+        )
>+      )
>+    )
>+});
>+
>+HTMLTemplates.EmptyElement = domplate(BaseTemplates.Element,
>+{
>+  tag:
>+    domplate.DIV({"class":
>+        "nodeBox emptyNodeBox $object|getHidden repIgnore",
>+        _repObject: "$object", role: 'presentation'},
>+      domplate.DIV({"class": "nodeLabel", role: "presentation"},
>+        domplate.SPAN({"class": "nodeLabelBox repTarget",
>+            role: 'treeitem'},
>+          "&lt;",
>+          domplate.SPAN({"class": "nodeTag"},
>+            "$object.nodeName|toLowerCase"),
>+          domplate.FOR("attr", "$object|attrIterator", BaseTemplates.AttrTag),
>+          domplate.SPAN({"class":
>+            "nodeBracket editable insertBefore"}, "&gt;")
>+        )
>+      )
>+    )
>+});
>+
>+HTMLTemplates.XEmptyElement = domplate(BaseTemplates.Element,
>+{
>+  tag:
>+    domplate.DIV({"class":
>+        "nodeBox emptyNodeBox $object|getHidden repIgnore",
>+        _repObject: "$object", role: 'presentation'},
>+      domplate.DIV({"class": "nodeLabel", role: "presentation"},
>+        domplate.SPAN({"class": "nodeLabelBox repTarget",
>+            role : 'treeitem'},
>+          "&lt;",
>+          domplate.SPAN({"class": "nodeTag"},
>+            "$object.nodeName|toLowerCase"),
>+          domplate.FOR("attr", "$object|attrIterator", BaseTemplates.AttrTag),
>+          domplate.SPAN({"class":
>+            "nodeBracket editable insertBefore"}, "/&gt;")
>+        )
>+      )
>+    )
>+});
>+
>+HTMLTemplates.AttrNode = domplate(BaseTemplates.Element,
>+{
>+  tag: BaseTemplates.AttrTag
>+});
>+
>+HTMLTemplates.TextNode = domplate(BaseTemplates.Element,
>+{
>+  tag:
>+    domplate.DIV({"class": "nodeBox", _repObject: "$object",
>+      role: 'presentation'}, BaseTemplates.TextTag)
>+});
>+
>+HTMLTemplates.CDATANode = domplate(BaseTemplates.Element,
>+{
>+  tag:
>+    domplate.DIV({"class": "nodeBox", _repObject: "$object",
>+      role: 'presentation'},
>+        "&lt;![CDATA[",
>+        domplate.SPAN({"class": "nodeText nodeCDATA editable"},
>+          "$object.nodeValue"),
>+        "]]&gt;")
>+});
>+
>+HTMLTemplates.CommentNode = domplate(BaseTemplates.Element,
>+{
>+  tag:
>+    domplate.DIV({"class": "nodeBox nodeComment",
>+        _repObject: "$object", role : 'presentation'},
>+      "&lt;!--",
>+      domplate.SPAN({"class": "nodeComment editable"},
>+        "$object.nodeValue"),
>+      "--&gt;")
>+});
>+
>+HTMLTemplates.Nada = domplate(BaseTemplates.Rep,
>+{
>+  tag: domplate.SPAN(""),
>+  className: "nada"
>+});
>+
>diff --git a/browser/base/content/test/Makefile.in b/browser/base/content/test/Makefile.in
>--- a/browser/base/content/test/Makefile.in
>+++ b/browser/base/content/test/Makefile.in
>@@ -163,6 +163,7 @@
>                  browser_tabfocus.js \
>                  browser_tabs_owner.js \
>                  discovery.html \
>+                 domplate_test.js \
>                  moz.png \
>                  test_bug435035.html \
>                  test_bug462673.html \
>diff --git a/browser/base/content/test/domplate_test.js b/browser/base/content/test/domplate_test.js
>new file mode 100644
>--- /dev/null
>+++ b/browser/base/content/test/domplate_test.js
>@@ -0,0 +1,84 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: set ts=2 et sw=2 tw=80: */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Domplate Test.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * The Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Rob Campbell <[e-mail]>
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+let doc;
>+let div;
>+let plate;
>+
>+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>+Components.utils.import("resource:///modules/domplate.jsm");
>+
>+function createDocument()
>+{
>+  doc.body.innerHTML = '<div id="first">no</div>';
>+  doc.title = "Domplate Test";
>+  setupDomplateTests();
>+}
>+
>+function setupDomplateTests()
>+{ 
>+  ok(domplate, "domplate is defined");
>+  plate = domplate({tag: domplate.DIV("Hello!")});
>+  ok(plate, "template is defined");
>+  div = doc.getElementById("first");
>+  ok(div, "we have our div");
>+  plate.tag.replace({}, div, template);
>+  is(div.innerText, "Hello!", "Is the div's innerText replaced?");
>+  finishUp();
>+}
>+
>+function finishUp()
>+{
>+  gBrowser.removeCurrentTab();
>+  finish();
>+}
>+
>+function test()
>+{
>+  waitForExplicitFinish();
>+  gBrowser.selectedTab = gBrowser.addTab();
>+  gBrowser.selectedBrowser.addEventListener("load", function() {
>+    gBrowser.selectedBrowser.removeEventListener("load", arguments.callee, true);
>+    doc = content.document;
>+    waitForFocus(createDocument, content);
>+  }, true);
>+  
>+  content.location = "data:text/html,basic domplate tests";
>+}
>+

Comment 7

[Julian Viereck]

Comment on attachment 460378 [details] [diff] [review]
tree-panel-rollup-20100726

Note: I've removed the domplate parts of this patch as I reviewed them in the other patch already and they are 100% the same. As I'm running out of time, I couldn't make it to review all of this patch. I removed the part that I haven't reviewed yet.

>diff --git a/browser/base/Makefile.in b/browser/base/Makefile.in
>--- a/browser/base/Makefile.in
>+++ b/browser/base/Makefile.in
>@@ -56,6 +56,7 @@
> EXTRA_JS_MODULES = \
> 	content/openLocationLastURL.jsm \
> 	content/NetworkPrioritizer.jsm \
>+	content/domplate.jsm \
> 	content/stylePanel.jsm \
> 	$(NULL)
> 
>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul
>@@ -65,6 +65,7 @@
>         xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
>         xmlns:svg="http://www.w3.org/2000/svg"
>         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+        xmlns:html="http://www.w3.org/1999/xhtml"
>         onload="BrowserStartup()" onunload="BrowserShutdown()" onclose="return WindowIsClosing();"
>         title="&mainWindow.title;@PRE_RELEASE_SUFFIX@"
>         title_normal="&mainWindow.title;@PRE_RELEASE_SUFFIX@"
>@@ -223,9 +224,8 @@
>            onclick="InspectorUI.stopInspecting();"
>            onmousemove="InspectorUI.highlighter.handleMouseMove(event);"/>
> 
>-    <panel id="inspector-panel"
>+    <panel id="inspector-tree-panel"
>            orient="vertical"
>-           hidden="true"
>            ignorekeys="true"
>            noautofocus="true"
>            noautohide="true"
>@@ -258,20 +258,11 @@
>                        class="toolbarbutton-text"
>                        command="Inspector:Dom"/>
>       </toolbar>
>-      <tree id="inspector-tree" class="plain"
>-            seltype="single"
>-            treelines="true"
>-            onselect="InspectorUI.onTreeSelected()"
>-            flex="1">
>-        <treecols>
>-          <treecol id="colNodeName" label="nodeName" primary="true"
>-                   persist="width,hidden,ordinal" flex="1"/>
>-          <splitter class="tree-splitter"/>
>-          <treecol id="colNodeValue" label="nodeValue"
>-                   persist="width,hidden,ordinal" flex="1"/>
>-        </treecols>
>-        <treechildren id="inspector-tree-body"/>
>-      </tree>
>+      <browser id="inspector-tree-browser"
>+               flex="1"
>+               src="chrome://browser/content/inspector.html"
>+               onclick="InspectorUI.onTreeClick(event);"
>+               disablehistory="true" />
>       <hbox align="end">
>         <spacer flex="1" />
>         <resizer dir="bottomend" />
>diff --git a/browser/base/content/inspector.html b/browser/base/content/inspector.html
>new file mode 100644
>--- /dev/null
>+++ b/browser/base/content/inspector.html
>@@ -0,0 +1,13 @@
>+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
>+  "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
>+
>+<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en">
>+<head>
>+  <title>Inspector</title>
>+  <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"/>
>+  <link rel="stylesheet" href="chrome://browser/skin/inspector.css" type="text/css"/>
>+</head>
>+<body role="application">
>+<div class="panelNode" role="progressbar">  ....loading....</div>
>+</body>
>+</html>
>diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js
>--- a/browser/base/content/inspector.js
>+++ b/browser/base/content/inspector.js
>@@ -51,6 +51,22 @@
>   "title": true,
> };
> 
>+const SELF_CLOSING_TAGS = {
>+  "meta": 1,
>+  "link": 1,
>+  "area": 1,
>+  "base": 1,
>+  "col": 1,
>+  "input": 1,
>+  "img": 1,
>+  "br": 1,
>+  "hr": 1,
>+  "param": 1,
>+  "embed": 1
>+};
>+
>+const RE_NOT_WHITESPACE = /[^\s]/;
>+
> ///////////////////////////////////////////////////////////////////////////
> //// PanelHighlighter
> 
>@@ -321,169 +337,596 @@
> };
> 
> ///////////////////////////////////////////////////////////////////////////
>-//// InspectorTreeView
>+//// InsideOutBox
> 
> /**
>- * TreeView object to manage the view of the DOM tree. Wraps and provides an
>- * interface to an inIDOMView object
>+ * InsideOutBoxView is a simple interface definition for views implementing
>+ * InsideOutBox controls. All implementors must define these methods.
>+ */
>+

Remove newline here so that the comment is a JavaDoc for InsideOutBoxView.

>+InsideOutBoxView = {
>+    /**
>+     * Retrieves the parent object for a given child object.
>+     */
>+    getParentObject: function(child) {},

o function parameter should be prefixed with "a": aChild.
o not a complete JavaDoc (@param and @returns is missing).
o functions declare anonymous function, instead:
      getParentObject: function IOBoxView_getParentObject(child) {},

This applies for the whole patch file.

>+
>+    /**
>+     * Retrieves a given child node.
>+     *
>+     * If both index and previousSibling are passed, the implementation
>+     * may assume that previousSibling will be the return for getChildObject
>+     * with index-1.
>+     */
>+    getChildObject: function(parent, index, previousSibling) {},
>+
>+    /**
>+     * Renders the HTML representation of the object. Should return an HTML
>+     * object which will be displayed to the user.
>+     */
>+    createObjectBox: function(object, isRoot) {}

Fix indention.

>+};
>+
>+/**
>+ * Creates a tree based on objects provided by a separate "view" object.
>  *
>- * @param aWindow
>- *        a top-level window object
>+ * Construction uses an "inside-out" algorithm, meaning that the view's job is
>+ * first to tell us the ancestry of each object, and secondarily its
>+ * descendants.
>+ *
>+ * Constructor
>+ * @param view
>+ *        The view requiring the InsideOutBox.
>+ * @param box
>+ *        The box object containing the InsideOutBox. Required to add/remove
>+ *        children during box manipulation (toggling opened or closed).

Missing @returns void. I don't repeat this on every JavaDoc.

>  */
>-function InspectorTreeView(aWindow)
>+InsideOutBox = function(view, box)
> {
>-  this.tree = document.getElementById("inspector-tree");
>-  this.treeBody = document.getElementById("inspector-tree-body");
>-  this.view = Cc["@mozilla.org/inspector/dom-view;1"]
>-              .createInstance(Ci.inIDOMView);
>-  this.view.showSubDocuments = true;
>-  this.view.whatToShow = NodeFilter.SHOW_ALL;
>-  this.tree.view = this.view;
>-  this.contentWindow = aWindow;
>-  this.view.rootNode = aWindow.document;
>-  this.view.rebuild();
>-}
>+  this.view = view;
>+  this.box = box;
> 
>-InspectorTreeView.prototype = {
>-  get editable() { return false; },
>-  get selection() { return this.view.selection; },
>+  this.rootObject = null;
> 
>+  this.rootObjectBox = null;
>+  this.selectedObjectBox = null;
>+  this.highlightedObjectBox = null;
>+  this.scrollIntoView = false;
>+};
>+
>+InsideOutBox.prototype =
>+{
>   /**
>-   * Destroy the view.
>+   * Highlight the given object node in the tree.
>+   * @param aObject
>+   *        the object to highlight.
>+   * @returns objectBox
>    */
>-  destroy: function ITV_destroy()
>+  highlight: function IOBox_highlight(aObject)
>   {
>-    this.tree.view = null;
>-    this.view = null;
>-    this.tree = null;
>+    let objectBox = this.createObjectBox(aObject);
>+    this.highlightObjectBox(objectBox);
>+    return objectBox;
>   },
> 
>   /**
>-   * Get the cell text at a given row and column.
>-   *
>-   * @param aRow
>-   *        The row index of the desired cell.
>-   * @param aCol
>-   *        The column index of the desired cell.
>-   * @returns string
>+   * Open the given object node in the tree.
>+   * @param aObject
>+   *        The object node to open.
>+   * @returns objectBox
>    */
>-  getCellText: function ITV_getCellText(aRow, aCol)
>+  openObject: function IOBox_openObject(aObject)
>   {
>-    return this.view.getCellText(aRow, aCol);
>+    let object = aObject;
>+    let firstChild = this.view.getChildObject(object, 0);
>+    if (firstChild)
>+      object = firstChild;

      if (firstChild) {
        object = firstChild;
      }

>+
>+    let objectBox = this.createObjectBox(object);
>+    this.openObjectBox(objectBox);
>+    return objectBox;
>   },
> 
>   /**
>-   * Get the index of the selected row.
>-   *
>-   * @returns number
>+   * Open the tree up to the given object node.
>+   * @param aObject
>+   *        The object in the tree to open to.
>+   * @returns objectBox
>    */
>-  get selectionIndex()
>+  openToObject: function IOBox_openToObject(aObject)
>   {
>-    return this.selection.currentIndex;
>+    let objectBox = this.createObjectBox(aObject);
>+    this.openObjectBox(objectBox);
>+    return objectBox;
>   },
> 
>   /**
>-   * Get the corresponding node for the currently-selected row in the tree.
>-   *
>-   * @returns DOMNode
>+   * Select the given object node in the tree.
>+   * @param aObject
>+   *        The object node to select.
>+   * @param makeBoxVisible
>+   *        Do we open the objectBox?
>+   * @param forceOpen
>+   *        Force the object box open?
>+   * @param scrollIntoView
>+   *        Scroll the objectBox into view?
>+   * @returns objectBox
>    */
>-  get selectedNode()
>+  select:
>+  function IOBox_select(aObject, makeBoxVisible, forceOpen, scrollIntoView)
>   {
>-    let rowIndex = this.selectionIndex;
>-    return this.view.getNodeFromRowIndex(rowIndex);
>+    let objectBox = this.createObjectBox(aObject);
>+    this.selectObjectBox(objectBox, forceOpen);
>+    if (makeBoxVisible) {
>+      this.openObjectBox(objectBox);
>+      if (scrollIntoView) {
>+        // object.scrollIntoView(true);
>+        this.view.scrollIntoCenterView(objectBox, this.box.ownerDocument, true);
>+      }
>+    }
>+    return objectBox;
>   },
> 
>   /**
>-   * Set the selected row in the table to the specified index.
>-   *
>-   * @param anIndex
>-   *        The index to set the selection to.
>+   * Expand the given object in the tree.
>+   * @param aObject
>+   *        The tree node to expand.
>    */
>-  set selectedRow(anIndex)
>+  expandObject: function IOBox_expandObject(aObject)
>   {
>-    this.view.selection.select(anIndex);
>-    this.tree.treeBoxObject.ensureRowIsVisible(anIndex);
>+    let objectBox = this.createObjectBox(aObject);
>+    if (objectBox)
>+      this.expandObjectBox(objectBox);

      if (objectBox) {
        this.expandObjectBox(objectBox);
      }

>   },
> 
>   /**
>-   * Set the selected node to the specified document node.
>-   *
>-   * @param aNode
>-   *        The document node to select in the tree.
>+   * Contract the given object in the tree.
>+   * @param aObject
>+   *        The tree node to contract.
>    */
>-  set selectedNode(aNode)
>+  contractObject: function IOBox_contractObject(aObject)
>   {
>-    let rowIndex = this.view.getRowIndexFromNode(aNode);
>-    if (rowIndex > -1) {
>-      this.selectedRow = rowIndex;
>-    } else {
>-      this.selectElementInTree(aNode);
>+    let objectBox = this.createObjectBox(aObject);
>+    if (objectBox)
>+      this.contractObjectBox(objectBox);

      if (objectBox) {
        this.contractObjectBox(objectBox);
      }

>+  },
>+
>+  /**
>+   * Highlight the given objectBox in the tree.
>+   * @param aObjectBox
>+   *        The objectBox to highlight.
>+   */
>+  highlightObjectBox: function IOBox_highlightObjectBox(aObjectBox)
>+  {
>+    if (this.highlightedObjectBox) {
>+      this.view.style.removeClass(this.highlightedObjectBox, "highlighted");
>+
>+      let highlightedBox = this.getParentObjectBox(this.highlightedObjectBox);
>+      for (; highlightedBox; highlightedBox = this.getParentObjectBox(highlightedBox))

  Can you use a while loop here? 
        while (highlightedBox = this.getParentObjectBox(highlightedBox)) {

>+        this.view.style.removeClass(highlightedBox, "highlightOpen");
>+    }
>+
>+    this.highlightedObjectBox = aObjectBox;
>+
>+    if (aObjectBox) {
>+      this.view.style.setClass(aObjectBox, "highlighted");
>+
>+      let highlightedBox = this.getParentObjectBox(aObjectBox);
>+      for (; highlightedBox; highlightedBox = this.getParentObjectBox(highlightedBox))

  Can you use a while loop here? 
        while (highlightedBox = this.getParentObjectBox(highlightedBox)) {

>+        this.view.style.setClass(highlightedBox, "highlightOpen");
>+
>+      aObjectBox.scrollIntoView(true);
>     }
>   },
> 
>   /**
>-   * Select the given node in the tree, searching for and expanding rows
>-   * as-needed.
>-   *
>-   * @param aNode
>-   *        The document node to select in the three.
>-   * @returns boolean
>-   *          Whether a node was selected or not if not found.
>+   * Select the given objectBox in the tree, forcing it to be open if necessary.
>+   * @param aObjectBox
>+   *        The objectBox to select.
>+   * @param forceOpen
>+   *        Force the box (subtree) to be open?
>    */
>-  selectElementInTree: function ITV_selectElementInTree(aNode)
>+  selectObjectBox: function IOBox_selectObjectBox(aObjectBox, forceOpen)
>   {
>-    if (!aNode) {
>-      this.view.selection.select(null);
>-      return false;      
>+    let isSelected = this.selectedObjectBox &&
>+      aObjectBox == this.selectedObjectBox;
>+
>+    if (!isSelected) {
>+      this.view.style.removeClass(this.selectedObjectBox, "selected");
>+      this.selectedObjectBox = aObjectBox;
>+
>+      if (aObjectBox) {
>+        this.view.style.setClass(aObjectBox, "selected");
>+
>+        // Force it open the first time it is selected
>+        if (forceOpen)
>+          this.toggleObjectBox(aObjectBox, true);
>+      }
>+    }
>+  },
>+
>+  /**
>+   * Open the ancestors of the given object box.
>+   * @param aObjectBox
>+   *        The object box to open.
>+   */
>+  openObjectBox: function IOBox_openObjectBox(aObjectBox)
>+  {
>+    if (aObjectBox) {
>+      // Set all of the node's ancestors to be permanently open
>+      let parentBox = this.getParentObjectBox(aObjectBox);
>+      let labelBox;
>+      for (; parentBox; parentBox = this.getParentObjectBox(parentBox)) {

Can you use a while loop here?
        while (parentBox = this.getParentObjectBox(parentBox)) {

>+        this.view.style.setClass(parentBox, "open");
>+        labelBox = parentBox.getElementsByClassName('nodeLabelBox').item(0);
>+        if (labelBox)
>+          labelBox.setAttribute('aria-expanded', 'true')

          if (labelBox) {
            labelBox.setAttribute('aria-expanded', 'true')
          }

>+      }
>+    }
>+  },
>+
>+  /**
>+   * Expand the given object box.
>+   * @param aObjectBox
>+   *        The object box to expand.
>+   */
>+  expandObjectBox: function IOBox_expandObjectBox(aObjectBox)
>+  {
>+    let nodeChildBox = this.getChildObjectBox(aObjectBox);
>+
>+    // no children means nothing to expand, return
>+    if (!nodeChildBox)
>+      return;

      if (!nodeChildBox) {
        return;
      }

>+
>+    if (!aObjectBox.populated) {
>+      let firstChild = this.view.getChildObject(aObjectBox.repObject, 0);
>+      this.populateChildBox(firstChild, nodeChildBox);
>+    }
>+    let labelBox = aObjectBox.getElementsByClassName('nodeLabelBox').item(0);
>+    if (labelBox)
>+      labelBox.setAttribute('aria-expanded', 'true');

  if (...) { }

>+    this.view.style.setClass(aObjectBox, "open");
>+  },
>+
>+  /**
>+   * Contract the given object box.
>+   * @param aObjectBox
>+   *        The object box to contract.
>+   */
>+  contractObjectBox: function IOBox_contractObjectBox(aObjectBox)
>+  {
>+    this.view.style.removeClass(aObjectBox, "open");
>+    let nodeLabel = aObjectBox.getElementsByClassName("nodeLabel").item(0);
>+    let labelBox = nodeLabel.getElementsByClassName('nodeLabelBox').item(0);
>+    if (labelBox)
>+      labelBox.setAttribute('aria-expanded', 'false');

  if (...) { }

>+  },
>+
>+  /**
>+   * Toggle the given object box, forcing open if requested.
>+   * @param aObjectBox
>+   *        The object box to toggle.
>+   * @param forceOpen
>+   *        Force the objectbox open?
>+   */
>+  toggleObjectBox: function IOBox_toggleObjectBox(aObjectBox, forceOpen)
>+  {
>+    let isOpen = this.view.style.hasClass(aObjectBox, "open");
>+    let nodeLabel = aObjectBox.getElementsByClassName("nodeLabel").item(0);
>+    let labelBox = nodeLabel.getElementsByClassName('nodeLabelBox').item(0);
>+
>+    if (!forceOpen && isOpen)
>+      this.contractObjectBox(aObjectBox);
>+    else if (!isOpen)
>+      this.expandObjectBox(aObjectBox);

  if (...) { }

>+  },
>+
>+  /**
>+   * Creates all of the boxes for an object, its ancestors, and siblings.
>+   * @param aObject
>+   *        The tree node to create the object boxes for.
>+   * @returns anObjectBox or null
>+   */
>+  createObjectBox: function IOBox_createObjectBox(aObject)
>+  {
>+    if (!aObject)
>+      return null;

  if (...) { }

>+
>+    this.rootObject = this.getRootNode(aObject) || aObject;
>+
>+    // Get or create all of the boxes for the target and its ancestors
>+    let objectBox = this.createObjectBoxes(aObject, this.rootObject);
>+
>+    if (!objectBox)
>+      return null;
>+    else if (aObject == this.rootObject)
>+      return objectBox;
>+    else
>+      return this.populateChildBox(aObject, objectBox.parentNode);

  if (...) { }

>+  },
>+
>+  /**
>+   * Creates all of the boxes for an object, its ancestors, and siblings up to
>+   * a root.
>+   * @param aObject
>+   *        The tree's object node to create the object boxes for.
>+   * @param aRootObject
>+   *        The root object at which to stop building object boxes.
>+   * @returns an object box or null
>+   */
>+  createObjectBoxes: function IOBox_createObjectBoxes(aObject, aRootObject)
>+  {
>+    if (!aObject)
>+      return null;

  if (...) { }

>+
>+    if (aObject == aRootObject) {
>+      if (!this.rootObjectBox || this.rootObjectBox.repObject != aRootObject) {
>+        if (this.rootObjectBox) {
>+          try {
>+            this.box.removeChild(this.rootObjectBox);
>+          } catch (exc) {
>+            InspectorUI._log("this.box.removeChild(this.rootObjectBox) FAILS " +
>+              this.box + " must not contain " + this.rootObjectBox);
>+          }
>+        }
>+
>+        this.highlightedObjectBox = null;
>+        this.selectedObjectBox = null;
>+        this.rootObjectBox = this.view.createObjectBox(aObject, true);
>+        this.box.appendChild(this.rootObjectBox);
>+      }
>+      return this.rootObjectBox;
>+    } else {
>+      let parentNode = this.view.getParentObject(aObject);
>+      let parentObjectBox = this.createObjectBoxes(parentNode, aRootObject);
>+
>+      if (!parentObjectBox)
>+        return null;

  if (...) { }

>+
>+      let parentChildBox = this.getChildObjectBox(parentObjectBox);
>+
>+      if (!parentChildBox)
>+        return null;

  if (...) { }

>+
>+      let childObjectBox = this.findChildObjectBox(parentChildBox, aObject);
>+
>+      return childObjectBox ? childObjectBox
>+        : this.populateChildBox(aObject, parentChildBox);
>+    }
>+  },
>+
>+  /**
>+   * Locate the object box for a given object node.
>+   * @param aObject
>+   *        The given object node in the tree.
>+   * @returns an object box or null.
>+   */
>+  findObjectBox: function IOBox_findObjectBox(aObject)
>+  {
>+    if (!aObject)
>+      return null;

  if (...) { }

>+
>+    if (aObject == this.rootObject) {
>+      return this.rootObjectBox;
>+    } else {
>+      let parentNode = this.view.getParentObject(aObject);
>+      let parentObjectBox = this.findObjectBox(parentNode);
>+      if (!parentObjectBox)
>+        return null;

  if (...) { }

>+
>+      let parentChildBox = this.getChildObjectBox(parentObjectBox);
>+      if (!parentChildBox)
>+        return null;

  if (...) { }

>+
>+      return this.findChildObjectBox(parentChildBox, aObject);
>+    }
>+  },
>+
>+  /**
>+   * Append an object box to a parent.
>+   * @param aParentNodeBox
>+   *        The box to attach the object to.
>+   * @param aRepObject
>+   *        A object with an associated objectbox.
>+   * @returns an object box or null
>+   */
>+  appendChildBox: function IOBox_appendChildBox(aParentNodeBox, aRepObject)
>+  {
>+    let childBox = this.getChildObjectBox(aParentNodeBox);
>+    let objectBox = this.findChildObjectBox(childBox, aRepObject);
>+
>+    // aRepObject already a child of aParentNodeBox, return
>+    if (objectBox)
>+      return objectBox;

  if (...) { }

>+
>+    objectBox = this.view.createObjectBox(aRepObject);
>+    if (objectBox) {
>+      let childBox = this.getChildObjectBox(aParentNodeBox); // todo - redundant?
>+      childBox.appendChild(objectBox);
>     }
> 
>-    // Keep searching until a pre-created ancestor is found, then 
>-    // open each ancestor until the found element is created.
>-    let domUtils = Cc["@mozilla.org/inspector/dom-utils;1"].
>-                    getService(Ci.inIDOMUtils);
>-    let line = [];
>-    let parent = aNode;
>-    let index = null;
>+    return objectBox;
>+  },
> 
>-    while (parent) {
>-      index = this.view.getRowIndexFromNode(parent);
>-      line.push(parent);
>-      if (index < 0) {
>-        // Row for this node hasn't been created yet.
>-        parent = domUtils.getParentForNode(parent,
>-          this.view.showAnonymousContent);
>-      } else {
>+  /**
>+   * Insert a child before a given sibling in the tree.
>+   * @param aParentNodeBox
>+   *        The parent node to insert the object under.
>+   * @param aRepObject
>+   *        The object to create an object box for that will be inserted.
>+   * @param aNextSibling
>+   *        The sibling to insert the repObject before.
>+   * @returns objectbox or null
>+   */
>+  insertChildBoxBefore:
>+  function IOBox_insertChildBoxBefore(aParentNodeBox, aRepObject, aNextSibling)
>+  {
>+    let childBox = this.getChildObjectBox(aParentNodeBox);
>+    let objectBox = this.findChildObjectBox(childBox, aRepObject);
>+
>+    // aRepObject is already in the tree
>+    if (objectBox)
>+      return objectBox;

  if (...) { }

>+
>+    objectBox = this.view.createObjectBox(aRepObject);
>+    if (objectBox) {
>+      let siblingBox = this.findChildObjectBox(childBox, aNextSibling);
>+      childBox.insertBefore(objectBox, siblingBox);
>+    }
>+    return objectBox;
>+  },
>+
>+  /**
>+   * Remove the given object from the tree described by the parentNodeBox.
>+   * @param aParentNodeBox
>+   *        A subtree from which to remove aRepObject
>+   * @param aRepObject
>+   *        The object to remove from the tree
>+   */
>+  removeChildBox: function IOBox_removeChildBox(aParentNodeBox, aRepObject)
>+  {
>+    let childBox = this.getChildObjectBox(aParentNodeBox);
>+    let objectBox = this.findChildObjectBox(childBox, aRepObject);
>+    if (objectBox)
>+      childBox.removeChild(objectBox);

  if (...) { }

>+  },
>+
>+  /**
>+   * We want all children of the parent of repObject.
>+   */
>+  populateChildBox: function(repObject, nodeChildBox)
>+  {
>+    if (!repObject)
>+      return null;

  if (...) { }

>+
>+    let parentObjectBox = this.view.style.getAncestorByClass(nodeChildBox, "nodeBox");
>+
>+    if (parentObjectBox.populated)
>+      return this.findChildObjectBox(nodeChildBox, repObject);

  if (...) { }

>+
>+    let lastSiblingBox = this.getChildObjectBox(nodeChildBox);
>+    let siblingBox = nodeChildBox.firstChild;
>+    let targetBox = null;
>+    let view = this.view;
>+    let targetSibling = null;
>+    let parentNode = view.getParentObject(repObject); // todo
>+
>+    for (let i = 0; 1; ++i) {

ddahl told me that TM can't trace when using let in as a for-loop variable. Use var instead.

>+      targetSibling = view.getChildObject(parentNode, i, targetSibling); // todo
>+      if (!targetSibling)
>         break;

  if (...) { }

>+
>+      // Check if we need to start appending, or continue to insert before
>+      if (lastSiblingBox && lastSiblingBox.repObject == targetSibling)
>+        lastSiblingBox = null;

  if (...) { }

>+
>+      if (!siblingBox || siblingBox.repObject != targetSibling) {
>+        let newBox = view.createObjectBox(targetSibling);
>+        if (newBox) {
>+          if (lastSiblingBox)
>+            nodeChildBox.insertBefore(newBox, lastSiblingBox);
>+          else
>+            nodeChildBox.appendChild(newBox);

  if (...) { }

>+        }
>+
>+        siblingBox = newBox;
>       }
>+
>+      if (targetSibling == repObject)
>+        targetBox = siblingBox;

  if (...) { }

>+
>+      if (siblingBox && siblingBox.repObject == targetSibling)
>+        siblingBox = siblingBox.nextSibling;

  if (...) { }

>     }
> 
>-    // We have all the ancestors, now open them one-by-one from the top
>-    // to bottom.
>-    let lastIndex;
>-    let view = this.tree.treeBoxObject.view;
>+    if (targetBox)
>+      parentObjectBox.populated = true;

  if (...) { }

> 
>-    for (let i = line.length - 1; i >= 0; --i) {
>-      index = this.view.getRowIndexFromNode(line[i]);
>-      if (index < 0) {
>-        // Can't find the row, so stop trying to descend.
>-        break;
>-      }
>-      if (i > 0 && !view.isContainerOpen(index)) {
>-        view.toggleOpenState(index);
>-      }
>-      lastIndex = index;
>+    return targetBox;
>+  },
>+
>+  /**
>+   * Get the parent object box of a given object box.
>+   * @params aObjectBox
>+   *         The object box of the parent.
>+   * @returns an object box or null
>+   */
>+  getParentObjectBox: function IOBox_getParentObjectBox(aObjectBox)
>+  {
>+    let parent = aObjectBox.parentNode ? aObjectBox.parentNode.parentNode : null;
>+    return parent && parent.repObject ? parent : null;
>+  },
>+
>+  /**
>+   * Get the child object box of a given object box.
>+   * @param aObjectBox
>+   *        The object box whose child you want.
>+   * @returns an object box or null
>+   */
>+  getChildObjectBox: function IOBox_getChildObjectBox(aObjectBox)
>+  {
>+    return aObjectBox.getElementsByClassName("nodeChildBox").item(0);
>+  },
>+
>+  /**
>+   * Find the child object box for a given repObject within the subtree
>+   * rooted at aParentNodeBox

Missing point at the end of the sentence.

>+   * @param aParentNodeBox
>+   *        root of the subtree in which to search for repObject

Missing point at the end of the sentence.

>+   * @param aRepObject
>+   *        The object you wish to locate in the subtree.
>+   * @returns an object box or null
>+   */
>+  findChildObjectBox: function IOBox_findChildObjectBox(aParentNodeBox, aRepObject)
>+  {
>+    for (let childBox = aParentNodeBox.firstChild; childBox; childBox = childBox.nextSibling) {

I don't like the for loop. I would write:

        var childBox = aParentNodeBox.firstChild;
        while (childBox) {
          body;
          childBox = childBox.nextSibling
        }

but that might be question of taste.

>+      if (childBox.repObject == aRepObject)
>+        return childBox;

  if (...) { }

>     }
>+    return null; // not found
>+  },
> 
>-    if (lastIndex >= 0) {
>-      this.selectedRow = lastIndex;
>-      return true;
>+  /**
>+   * Determines if the given node is an ancestor of the current root.
>+   * @param aNode
>+   *        The node to look for within the tree.
>+   * @returns boolean
>+   */
>+  isInExistingRoot: function IOBox_isInExistingRoot(aNode)
>+  {
>+    let parentNode = aNode;
>+    while (parentNode && parentNode != this.rootObject) {
>+      parentNode = this.view.getParentObject(parentNode);
>     }
>-    
>-    return false;
>+    return parentNode == this.rootObject;
>+  },
>+
>+  /**
>+   * Get the root node of a given node.
>+   * @param aNode
>+   *        The node whose root you wish to retrieve.
>+   * @returns a root node or null
>+   */
>+  getRootNode: function IOBox_getRootNode(aNode)
>+  {
>+    let node = aNode;
>+    let parentNode = aNode;
>+    do {
>+      parentNode = this.view.getParentObject(node);
>+      if (!parentNode)
>+        return node;
>+      else
>+        node = parentNode;

  if (...) { }

>+    } while (parentNode);
>+    return null;
>   },
> };
> 
> ///////////////////////////////////////////////////////////////////////////
>+//// Local Helpers
>+
>+
>+

This section is empty? Can it be removed?

>+///////////////////////////////////////////////////////////////////////////
> //// InspectorUI
> 
> /**
>@@ -493,9 +936,11 @@
>   browser: null,
>   _showTreePanel: true,
>   _showStylePanel: true,
>-  _showDOMPanel: true,
>+  _showDOMPanel: false,
>   selectEventsSuppressed: false,
>+  showTextNodesWithWhitespace: false,
>   inspecting: false,
>+  noScrollIntoView: false,
> 
>   /**
>    * Toggle the inspector interface elements on or off.
>@@ -505,7 +950,7 @@
>    */
>   toggleInspectorUI: function IUI_toggleInspectorUI(aEvent)
>   {
>-    if (this.isPanelOpen) {
>+    if (this.isTreePanelOpen) {
>       this.closeInspectorUI();
>     } else {
>       this.openInspectorUI();
>@@ -534,8 +979,8 @@
>       this.stylePanel.hidePopup();
>     } else {
>       this.openStylePanel();
>-      if (this.treeView.selectedNode) {
>-        this.updateStylePanel(this.treeView.selectedNode);
>+      if (this.selection) {
>+        this.updateStylePanel(this.selection);
>       }
>     }
>     this._showStylePanel = !this._showStylePanel;
>@@ -550,8 +995,8 @@
>       this.domPanel.hidePopup();
>     } else {
>       this.openDOMPanel();
>-      if (this.treeView.selectedNode) {
>-        this.updateDOMPanel(this.treeView.selectedNode);
>+      if (this.selection) {
>+        this.updateDOMPanel(this.selection);
>       }
>     }
>     this._showDOMPanel = !this._showDOMPanel;
>@@ -562,7 +1007,8 @@
>    *
>    * @returns boolean
>    */
>-  get isPanelOpen()
>+
>+  get isTreePanelOpen()
>   {
>     return this.treePanel && this.treePanel.state == "open";
>   },
>@@ -588,25 +1034,207 @@
>   },
> 
>   /**
>+   * Return the default selection element for the inspected document.
>+   */
>+  get defaultSelection()
>+  {
>+    let doc = this.win.document;
>+    return doc.body ? doc.body :
>+      this.getPreviousElement(doc.documentElement.lastChild);
>+  },
>+
>+  getPreviousElement: function IUI_getPreviousElement(aNode)
>+  {
>+    while (aNode && aNode.nodeType != 1)
>+      aNode = aNode.previousSibling;
>+
>+    return aNode;
>+  },
>+
>+  /**
>+   * Return the owner panel of the node.

Missing @param + @returns

>+   */
>+  getOwnerPanel: function IUI_getOwnerPanel(node)
>+  {
>+    for (; node; node = node.parentNode) {
>+      if (node.ownerPanel)
>+        return node.ownerPanel;
>+    }
>+  },
>+
>+  /**
>    * Open the inspector's tree panel and initialize it.

Missing @param + @returns

>    */
>   openTreePanel: function IUI_openTreePanel()
>   {
>     if (!this.treePanel) {
>-      this.treePanel = document.getElementById("inspector-panel");
>+      this.treePanel = document.getElementById("inspector-tree-panel");
>       this.treePanel.hidden = false;
>     }
>-    if (!this.isPanelOpen) {
>-      const panelWidthRatio = 7 / 8;
>-      const panelHeightRatio = 1 / 5;
>-      let bar = document.getElementById("status-bar");
>-      this.treePanel.openPopupAtScreen(this.win.screenX + 80,
>-        this.win.outerHeight + this.win.screenY);
>-      this.treePanel.sizeTo(this.win.outerWidth * panelWidthRatio, 
>-        this.win.outerHeight * panelHeightRatio);
>-      this.tree = document.getElementById("inspector-tree");
>-      this.createDocumentModel();
>+
>+    const panelWidthRatio = 7 / 8;
>+    const panelHeightRatio = 1 / 5;
>+    this.treePanel.openPopup(this.browser, "overlap", 80, this.win.innerHeight,
>+      false, false);
>+    this.treePanel.sizeTo(this.win.outerWidth * panelWidthRatio,
>+      this.win.outerHeight * panelHeightRatio);
>+
>+    this.treeBrowser = document.getElementById("inspector-tree-browser");
>+    let self = this;
>+
>+    this.treeBrowser.addEventListener("load", function() {
>+      self.treeBrowser.removeEventListener("load", arguments.callee, true);
>+      self.treeBrowserDocument = self.treeBrowser.contentDocument;
>+      self.treePanelDiv = self.treeBrowserDocument.createElement("div");
>+      self.treeBrowserDocument.body.appendChild(self.treePanelDiv);
>+      self.treePanelDiv.ownerPanel = self;
>+      self.ioBox = new InsideOutBox(self, self.treePanelDiv);
>+      self.ioBox.createObjectBox(self.win.document.documentElement);
>+      Services.obs.notifyObservers(null, "inspector-opened", null);
>+    }, true);
>+
>+    this.treeBrowser.reload();
>+  },
>+
>+  getNodeTag: function IUI_getNodeTag(node, expandAll)
>+  {
>+    if (node instanceof Element) {
>+      if (node instanceof HTMLHtmlElement && node.ownerDocument
>+          && node.ownerDocument.doctype)
>+        return this.HTMLTemplates.HTMLHtmlElement.tag;
>+      if (node instanceof HTMLAppletElement)
>+        return this.HTMLTemplates.EmptyElement.tag;
>+      else if (this.isContainerElement(node))
>+        return this.HTMLTemplates.CompleteElement.tag;
>+      else if (this.isEmptyElement(node))
>+        return this.HTMLTemplates.EmptyElement.tag;
>+      else if (this.hasNoElementChildren(node))
>+        return this.HTMLTemplates.TextElement.tag;
>+      else
>+        return this.HTMLTemplates.Element.tag;
>     }
>+    else if (node instanceof Text)
>+      return this.HTMLTemplates.TextNode.tag;
>+    else if (node instanceof CDATASection)
>+      return this.HTMLTemplates.CDATANode.tag;
>+    else if (node instanceof Comment)
>+      return this.HTMLTemplates.CommentNode.tag;
>+    else if (node instanceof SourceText)
>+      return this.HTMLTemplates.SourceText.tag;
>+    else
>+      return this.HTMLTemplates.Nada.tag;

  if (...) { }

>+  },
>+
>+  createObjectBox: function IUI_createObjectBox(object, isRoot)
>+  {
>+    let tag = this.getNodeTag(object);
>+    if (tag)
>+      return tag.replace({object: object}, this.treeBrowserDocument);

  if (...) { }

>+  },
>+
>+  getParentObject: function IUI_getParentObject(node)
>+  {
>+    let parentNode = node ? node.parentNode : null;
>+
>+    if (parentNode) {
>+      if (parentNode.nodeType == 9) { // then parentNode is Document element
>+        if (parentNode.defaultView) {
>+          return parentNode.defaultView.frameElement;
>+        } else if (this.embeddedBrowserParents) {
>+          let skipParent = this.embeddedBrowserParents[node];
>+          // HTML element? could be iframe?
>+          if (skipParent)
>+            return skipParent;

  if (...) { }

>+        } else // parent is document element, but no window at defaultView.

  if (...) { }

>+          return null;
>+      } else if (!parentNode.localName) {
>+        return null;
>+      } else
>+        return parentNode;

  if (...) { }

>+    } else {
>+      // Documents have no parentNode; Attr, Document, DocumentFragment, Entity,
>+      // and Notation. top level windows have no parentNode
>+      if (node && node.nodeType == 9) {
>+        // document type
>+        if (node.defaultView) {
>+          // generally a reference to the window object for the document,
>+          // however that is not defined in the specification
>+          let embeddingFrame = node.defaultView.frameElement;
>+          if (embeddingFrame)
>+            return embeddingFrame.parentNode;

  if (...) { }

>+        } else // a Document object without a parentNode or window
>+          return null;  // top level has no parent

  if (...) { }

>+      }
>+    }
>+  },
>+
>+  getChildObject: function IUI_getChildObject(node, index, previousSibling)
>+  {
>+    if (!node)
>+      return;

  if (...) { }
  Lots of them in this function.

>+
>+    /* if (this.isSourceElement(node)) {
>+      if (index == 0)
>+        return this.getElementSourceText(node);
>+      else
>+        return null;  // no siblings of source elements
>+    } else */
>+    if (node.contentDocument) {
>+      // then the node is a frame
>+      if (index == 0) {
>+        if (!this.embeddedBrowserParents)
>+          this.embeddedBrowserParents = {};
>+        let skipChild = node.contentDocument.documentElement;
>+        this.embeddedBrowserParents[skipChild] = node;
>+
>+        return skipChild;  // the node's HTMLElement
>+      } else
>+        return null;
>+    } else if (node.getSVGDocument && node.getSVGDocument()) {
>+      // then the node is a frame
>+      if (index == 0) {
>+        if (!this.embeddedBrowserParents)
>+          this.embeddedBrowserParents = {};
>+        let skipChild = node.getSVGDocument().documentElement; // unwrap
>+        this.embeddedBrowserParents[skipChild] = node;
>+
>+        return skipChild;  // the node's SVGElement
>+      } else
>+        return null;
>+    }
>+
>+    let child = null;
>+    if (previousSibling)  // then we are walking
>+      child = this.getNextSibling(previousSibling);  // may return null, meaning done with iteration.
>+    else
>+      child = this.getFirstChild(node); // child is set at the beginning of an iteration.
>+
>+    if (this.showTextNodesWithWhitespace)  // then the index is true to the node list
>+      return child;
>+    else {
>+      for (; child; child = this.getNextSibling(child)) {
>+        if (!this.isWhitespaceText(child))
>+          return child;
>+      }
>+    }
>+    return null;  // we have no children worth showing.
>+  },
>+
>+  getFirstChild: function IUI_getFirstChild(node)
>+  {
>+    this.treeWalker = node.ownerDocument.createTreeWalker(node,
>+      NodeFilter.SHOW_ALL, null, false);
>+    return this.treeWalker.firstChild();
>+  },
>+
>+  getNextSibling: function IUI_getNextSibling(node)
>+  {
>+    let next = this.treeWalker.nextSibling();
>+
>+    if (!next)
>+      delete this.treeWalker;

  if (...) { }

>+
>+    return next;
>   },
> 
>   /**
>@@ -663,17 +1291,21 @@
>   openInspectorUI: function IUI_openInspectorUI()
>   {
>     // initialization
>+    if (!this.domplate) {
>+      Cu.import("resource:///modules/domplate.jsm", this);
>+      let dom = Cu.getGlobalForObject(Node);
>+      this.setDOM(dom);
>+    }
>     this.browser = gBrowser.selectedBrowser;
>     this.win = this.browser.contentWindow;
>     if (!this.style) {
>       Cu.import("resource:///modules/stylePanel.jsm", this);
>       this.style.initialize();
>     }
>+    this.toolsInspectCmd = document.getElementById("Tools:Inspect");
> 
>     // open inspector UI
>-    if (this._showTreePanel) {
>-      this.openTreePanel();
>-    }
>+    this.openTreePanel();
>     if (this._showStylePanel) {
>       this.styleBox = document.getElementById("inspector-style-listbox");
>       this.clearStylePanel();
>@@ -703,7 +1335,7 @@
>     this.win.document.addEventListener("scroll", this, false);
>     this.win.addEventListener("resize", this, false);
>     gBrowser.tabContainer.addEventListener("TabSelect", this, false);
>-    this.inspectCmd.setAttribute("checked", true);
>+    this.toolsInspectCmd.setAttribute("checked", true);
>   },
> 
>   /**
>@@ -722,16 +1354,25 @@
>    */
>   closeInspectorUI: function IUI_closeInspectorUI()
>   {
>+    let toolsInspectCmd = document.getElementById("Tools:Inspect");
>     this.win.document.removeEventListener("scroll", this, false);
>     this.win.removeEventListener("resize", this, false);
>     gBrowser.tabContainer.removeEventListener("TabSelect", this, false);
>     this.stopInspecting();
>+    toolsInspectCmd.setAttribute("checked", "false");
>     if (this.highlighter && this.highlighter.isHighlighting) {
>       this.highlighter.unhighlight();
>     }
>-    if (this.isPanelOpen) {
>+    if (this.isTreePanelOpen) {
>+      if (this.treePanelDiv) {
>+        this.treePanelDiv.ownerPanel = null;
>+        delete this.treePanelDiv;
>+        delete this.treeBrowserDocument;
>+      }
>+      if (this.treeBrowser)
>+        delete this.treeBrowser;

  if (...) { }

>       this.treePanel.hidePopup();
>-      this.treeView.destroy();
>+      delete this.ioBox;
>     }
>     if (this.isStylePanelOpen) {
>       this.stylePanel.hidePopup();
>@@ -744,6 +1385,7 @@
>     }
>     this.inspectCmd.setAttribute("checked", false);
>     this.browser = this.win = null; // null out references to browser and window
>+    Services.obs.notifyObservers(null, "inspector-closed", null);
>   },
> 
>   /**
>@@ -770,24 +1412,50 @@
>     this.inspecting = false;
>     this.toggleDimForPanel(this.stylePanel);
>     this.toggleDimForPanel(this.domPanel);
>-    if (this.treeView.selection) {
>-      this.updateStylePanel(this.treeView.selectedNode);
>-      this.updateDOMPanel(this.treeView.selectedNode);
>+    if (this.highlighter.node) {
>+      this.select(this.highlighter.node, true);
>     }
>   },
> 
>+  /**
>+   * Select an object in the tree view.
>+   * @param aNode
>+   *        node to inspect
>+   * @param forceUpdate
>+   *        force an update?
>+   */
>+  select: function IUI_select(aNode, forceUpdate)
>+  {
>+    if (!aNode)
>+      aNode = this.defaultSelection;

  if (...) { }

>+
>+    if (forceUpdate || aNode != this.selection) {
>+      this.selection = aNode;
>+      let box = this.ioBox.createObjectBox(this.selection);
>+      if (!this.inspecting) {
>+        this.highlighter.highlightNode(this.selection);
>+        this.updateStylePanel(this.selection);
>+        this.updateDOMPanel(this.selection);
>+      } else {
>+        box.scrollIntoView(true); // todo scrollIntoCenterView would be nicer
>+      }
>+      this.updateSelection(this.selection);
>+    }
>+  },
>+
>+  /**
>+   * Update the tree panel's IOBox' selection.
>+   * @param aNode
>+   */
>+  updateSelection: function IUI_updateSelection(aNode)
>+  {
>+    this.ioBox.select(aNode, true, true);
>+  },
>+
>   /////////////////////////////////////////////////////////////////////////
>   //// Model Creation Methods
> 
>   /**
>-   * Create treeView object from content window.
>-   */
>-  createDocumentModel: function IUI_createDocumentModel()
>-  {
>-    this.treeView = new InspectorTreeView(this.win);
>-  },
>-
>-  /**
>    * Add a new item to the style panel listbox.
>    *
>    * @param aLabel
>@@ -967,20 +1635,20 @@
>   },
> 
>   /**
>-   * Event fired when a tree row is selected in the tree panel.
>+   * Handle click events in the html tree panel.

Missing @param and @returns.

>    */
>-  onTreeSelected: function IUI_onTreeSelected()
>+  onTreeClick: function IUI_onTreeClick(event)
>   {
>-    if (this.selectEventsSuppressed) {
>-      return false;

  if (...) { }

>+    let node;
>+    if (this.style.hasClass(event.target, "twisty"))
>+      node = this.getRepObject(event.target.nextSibling);
>+    else
>+      node = this.getRepObject(event.target);

  if (...) { } else { }

>+
>+    if (node) {
>+      this.toggleNode(node);
>+      this.select(node);
>     }
>-
>-    let node = this.treeView.selectedNode;
>-    this.highlighter.highlightNode(node);
>-    this.stopInspecting();
>-    this.updateStylePanel(node);
>-    this.updateDOMPanel(node);
>-    return true;
>   },
> 
>   /**
>@@ -1019,16 +1687,205 @@
>   {
>     this.highlighter.highlightNode(aNode);
>     this.selectEventsSuppressed = true;
>-    this.treeView.selectedNode = aNode;
>+    this.select(aNode, true);
>     this.selectEventsSuppressed = false;
>-    this.updateStylePanel(aNode);
>   },
> 
>   ///////////////////////////////////////////////////////////////////////////
>   //// Utility functions
> 
>   /**
>-   * debug logging facility
>+   * Get the repObject from the HTML panel's domplate-constructed DOM node.
>+   *
>+   * @param element
>+   *        The element in the HTML panel the user clicked.
>+   * @returns either a real node or null
>+   */
>+  getRepObject: function IUI_getRepObject(element)
>+  {
>+    let target = null;
>+    for (let child = element; child; child = child.parentNode) {
>+      if (this.style.hasClass(child, "repTarget"))
>+        target = child;

  if (...) { }

>+
>+      if (child.repObject) {
>+        if (!target && this.style.hasClass(child, "repIgnore"))
>+          break;
>+        else
>+          return child.repObject;

  if (...) { } else { }

>+      }
>+    }
>+    return null;
>+  },
>+
>+  toggleNode: function IUI_toggleNode(element)
>+  {
>+    let box = this.ioBox.createObjectBox(element);
>+    if (!this.style.hasClass(box, "open"))
>+      this.ioBox.expandObject(element);
>+    else
>+      this.ioBox.contractObject(element);

  if (...) { } else { }

>+  },
>+
>+  isContainerElement: function IUI_isContainerElement(element)
>+  {
>+    let tag = element.localName.toLowerCase();
>+    switch (tag) {
>+      case "script":
>+      case "style":
>+      case "iframe":
>+      case "frame":
>+      case "tabbrowser":
>+      case "browser":
>+        return true;
>+      case "link":
>+        return element.getAttribute("rel") == "stylesheet";
>+      case "embed":
>+        return element.getSVGDocument();
>+    }
>+    return false;
>+  },
>+
>+  isWhitespace: function IUI_isWhitespace(text)
>+  {
>+    return !RE_NOT_WHITESPACE.exec(text);
>+  },
>+
>+  isWhitespaceText: function IUI_isWhitespaceText(node)
>+  {
>+    if (node instanceof HTMLAppletElement)
>+      return false;

  if (...) { }

>+    return node.nodeType == Node.TEXT_NODE && this.isWhitespace(node.nodeValue);
>+  },
>+
>+  isSelfClosing: function IUI_isSelfClosing(element)
>+  {
>+    //if (isElementSVG(element) || isElementMathML(element))
>+    //    return true;
>+    var tag = element.localName.toLowerCase();
>+    return (SELF_CLOSING_TAGS.hasOwnProperty(tag));
>+  },
>+
>+  isEmptyElement: function IUI_isEmptyElement(element)
>+  {
>+    if (this.showTextNodesWithWhitespace) {
>+      return !element.firstChild && this.isSelfClosing(element);
>+    } else {
>+      for (let child = element.firstChild; child; child = child.nextSibling) {
>+        if (!this.isWhitespaceText(child))
>+          return false;

  if (...) { }

>+      }
>+    }
>+    return this.isSelfClosing(element);
>+  },
>+
>+  /**
>+   * Determines if the given node has any children which are elements.
>+   *
>+   * @param {Element} element Element to test.
>+   * @return true if immediate children of type Element exist, false otherwise
>+   */
>+  hasNoElementChildren: function IUI_hasNoElementChildren(element)
>+  {
>+    if (element.childElementCount != 0)  // FF 3.5+
>+      return false;

  if (...) { }

>+
>+    // https://developer.mozilla.org/en/XBL/XBL_1.0_Reference/DOM_Interfaces
>+    if (element.ownerDocument instanceof Ci.nsIDOMDocumentXBL) {
>+      let anonChildren = element.ownerDocument.getAnonymousNodes(element);
>+      if (anonChildren) {
>+        for (var i = 0; i < anonChildren.length; i++) {
>+          if (anonChildren[i].nodeType == Node.ELEMENT_NODE)
>+            return false;

  if (...) { }

>+        }
>+      }
>+    }
>+    return true;
>+  },
>+
>+  /**
>+   * Get the offset of an element within its document.
>+   * @param aNode
>+   * @returns an offset object (point)
>+   */
>+  getClientOffset: function IUI_getClientOffset(aNode)
>+  {
>+    function addOffset(elt, coords, view)
>+    {
>+      let p = elt.offsetParent;
>+
>+      let style = view.getComputedStyle(elt, "");
>+
>+      if (elt.offsetLeft)
>+        coords.x += elt.offsetLeft + parseInt(style.borderLeftWidth);
>+      if (elt.offsetTop)
>+        coords.y += elt.offsetTop + parseInt(style.borderTopWidth);
>+
>+      if (p) {
>+        if (p.nodeType == 1)
>+          addOffset(p, coords, view);
>+      }
>+      else if (elt.ownerDocument.defaultView.frameElement)
>+        addOffset(elt.ownerDocument.defaultView.frameElement, coords,
>+          elt.ownerDocument.defaultView);

  Some missing   if (...) { } here

>+    }
>+
>+    let coords = {x: 0, y: 0};
>+    if (aNode) {
>+      let view = aNode.ownerDocument.defaultView;
>+      addOffset(aNode, coords, view);
>+    }
>+
>+    return coords;
>+  },
>+
>+  getOverflowParent: function IUI_getOverflowParent(element)
>+  {
>+    for (let scrollParent = element.parentNode; scrollParent; scrollParent = scrollParent.offsetParent) {

As above: Don't use let to initialize a for-loop variable. Use var instead.

>+      if (scrollParent.scrollHeight > scrollParent.offsetHeight)
>+        return scrollParent;

  if (...) { }

>+    }
>+  },
>+
>+  scrollIntoCenterView:
>+  function IUI_scrollIntoCenterView(element, scrollBox, notX, notY)
>+  { // TODO needs fixin'
>+    if (!element)
>+      return;
>+
>+    if (!scrollBox)
>+      scrollBox = this.getOverflowParent(element);
>+

2x   if (...) { }

>+    if (!scrollBox) {
>+      return;
>+    }
>+
>+    let offset = this.getClientOffset(element);
>+
>+    if (!notY) {
>+      let topSpace = offset.y - scrollBox.scrollTop;
>+      let bottomSpace = (scrollBox.scrollTop + scrollBox.clientHeight)
>+        - (offset.y + element.offsetHeight);
>+
>+      if (topSpace < 0 || bottomSpace < 0) {
>+        let centerY = offset.y - (scrollBox.clientHeight / 2);
>+        scrollBox.scrollTop = centerY;
>+      }
>+    }
>+
>+    if (!notX) {
>+      let leftSpace = offset.x - scrollBox.scrollLeft;
>+      let rightSpace = (scrollBox.scrollLeft + scrollBox.clientWidth)
>+        - (offset.x + element.clientWidth);
>+
>+      if (leftSpace < 0 || rightSpace < 0) {
>+        let centerX = offset.x - (scrollBox.clientWidth / 2);
>+        scrollBox.scrollLeft = centerX;
>+      }
>+    }
>+  },
>+
>+  /**
>    * @param msg
>    *        text message to send to the log
>    */
>@@ -1036,6 +1893,12 @@
>   {
>     Services.console.logStringMessage(msg);
>   },
>+
>+  _debugTree: function DEBUGTREE()
>+  {
>+    let docEl = this.treeBrowser.contentDocument.documentElement;
>+    this._log(docEl.innerHTML);
>+  },
> }
> 
> XPCOMUtils.defineLazyGetter(InspectorUI, "inspectCmd", function () {
>diff --git a/browser/base/content/stylePanel.jsm b/browser/base/content/stylePanel.jsm
>--- a/browser/base/content/stylePanel.jsm
>+++ b/browser/base/content/stylePanel.jsm
>@@ -311,4 +311,88 @@
>       this.getStyleProperties(aNode, aRules, aUsedProps, aInherit);
>     }
>   },
>+
>+  // classList helpers
>+  getAncestorByClass: function CSS_getAncestorByClass(node, className)
>+  {
>+    for (let parent = node; parent; parent = parent.parentNode) {
>+      if (this.hasClass(parent, className))
>+        return parent;

  if (...) { }

>+    }
>+
>+    return null;
>+  },
>+
>+  setClass: function CSS_setClass(node, name)
>+  {
>+    if (!node || node.nodeType != 1 || name == '')
>+      return;

  if (...) { }
  Got to speed up. Don't claim on missing {} for if/for anymore starting from here.

>+
>+    if (name.indexOf(" ") != -1) {
>+      let classes = name.split(" "), len = classes.length;
>+      for (var i = 0; i < len; i++) {
>+        let cls = classes[i].trim();
>+        if (cls != "") {
>+          this.setClass(node, cls);
>+        }
>+      }
>+      return;
>+    }
>+
>+    if (!this.hasClass(node, name))
>+      node.className = node.className.trim() + " " + name;
>+  },
>+
>+  hasClass: function CSS_hasClass(node, name)
>+  {
>+    if (!node || node.nodeType != 1 || !node.className || name == '')
>+      return false;
>+
>+  if (name.indexOf(" ") != -1) {

Fix indention.

>+      let classes = name.split(" "), len = classes.length, found = false;
>+      for (var i = 0; i < len; i++) {
>+        let cls = classes[i].trim();
>+        if (cls != "") {
>+          if (this.hasClass(node, cls) == false)
>+            return false;
>+          found = true;
>+        }
>+      }
>+      return found;
>+    }
>+
>+    let re;
>+    if (name.indexOf("-") == -1)
>+      re = new RegExp('(^|\\s)' + name + '(\\s|$)', "g");
>+    else
>+      re = new RegExp('(^|\\s)' + name + '(\\s|$)', "g")
>+    return node.className.search(re) != -1;

I don't really understand what you're doing with the RegExp here. Would it be hard
to do things without them?

>+  },
>+
>+  removeClass: function CSS_removeClass(node, name)
>+  {
>+    if (!node || node.nodeType != 1 || node.className == '' || name == '')
>+      return;
>+
>+    if (name.indexOf(" ") != -1) {
>+      let classes = name.split(" "), len = classes.length;
>+      for (var i = 0; i < len; i++) {
>+        let cls = classes[i].trim();
>+        if (cls != "") {
>+          if (this.hasClass(node, cls) == false)
>+            this.removeClass(node, cls);
>+        }
>+      }
>+      return;
>+    }
>+
>+    let re;
>+    if (name.indexOf("-") == -1)
>+      re = new RegExp('(^|\\s)' + name + '(\\s|$)', "g");
>+    else
>+      re = new RegExp('(^|\\s)' + name + '(\\s|$)', "g");
>+
>+    node.className = node.className.replace(re, " ");

I don't really understand what you're doing with the RegExp here. Would it be hard
to do things without them?

Haven't to stop here.
=====================

Comment 8

[Julian Viereck]

Notes to my reviews so far: I only looked at formatting issues and things that I could spot right away. I haven't "analysed" more of the code to check if it makes sense or not.

Comment 10

[Rob Campbell [:rc] (:robcee)]

Attachment: inspector tree panel

rebased tree-panel (still requires dom introspector patches from bug 573102 and bug 561782 to apply cleanly). No longer includes domplate.

Have not cleaned up these patches based on previous feedback comments yet. Will do soon.

Still need some tests for this. Volunteers welcome!

Comment 11

[Mihai Sucan [:msucan]]

Hello Robert!

Below is my review/feedback on the tree panel patches.

- In inspector.js, the InsideOutBoxView object is unused.

The InspectorUI.ioBox is setup by the InspectorUI.treeBrowser onload event 
handler found in the InspectorUI.openTreePanel() method. There the view of the 
ioBox is the InspectorUI itself.

- InspectorUI makes use of this.embeddedBrowserParents which is not explained 
and not defined in at the start of the object definition. I believe it should be 
defined and explain with a short comment.

- InspectorUI.getParentObject() uses nodeType == 9. Why not use the 
DOCUMENT_NODE constant?

- InspectorUI.createObjectBox(object, isRoot): the second argument is unused.

- InspectorUI.createObjectBox() uses InspectorUI.getNodeTag(). Why not use the 
  getNodeTag() method of the domplate js module? It seems the InspectorUI method 
  only duplicates the functionality.

Not only that, but having copied the getNodeTag() method into the InspectorUI, 
it also required the copying of several more functions that are now new methods 
in the InspectorUI: isContainerElement, isWhitespace, isWhitespaceText, 
isSelfClosing, isEmptyElement, hasNoElementChildren. I believe that if these are 
not duplicated, then the inspector.js diff would be quite smaller and it would 
really help reviewing.

- In the InspectorUI.hasNoElementChildren() method I believe the code should be 
  trimmed to only check using the element.childElementCount property, because 
  the Inspector tool will only run inside Firefox 4+ anyway.

- In the InspectorUI.defaultSelection getter, the getPreviousElement() method is 
  used, which both look like:

  get defaultSelection()
  {
    let doc = this.win.document;
    return doc.body ? doc.body :
      this.getPreviousElement(doc.documentElement.lastChild);
  },

  getPreviousElement: function IUI_getPreviousElement(aNode)
  {
    while (aNode && aNode.nodeType != 1)
      aNode = aNode.previousSibling;

    return aNode;
  }

Why not use doc.documentElement.lastElementChild in the defaultSelection getter?

Also, the getPreviousElement() method is legacy code which can be removed. The 
previousElementSibling property of the node in question can be used.

I would add that the getPreviousElement() method is used only by the 
defaultSelection getter, making it easy to remove now.

- Applying these patches on top of the patch from bug 561782 makes the DOM panel 
  to fail. Somehow, for some reason, the DOM panel never shows up.

- In the InsideOutBox.toggleObjectBox() method:

    let nodeLabel = aObjectBox.getElementsByClassName("nodeLabel").item(0);
    let labelBox = nodeLabel.getElementsByClassName('nodeLabelBox').item(0);

... then the labelBox variable is never used. I believe both lines can be 
removed.

- The InspectorUI.toggleNode() looks like it should belong in the InsideOutBox 
  object, renamed to toggleObject() - there's already toggleObjectBox() and 
  there are the expand/contractObject() methods.

- In the InsideOutBox object there are unused methods: appendChildBox, 
  insertChildBoxBefore, and removeChildBox.

- In domplate.jsm, in the Renderer object, the replace() method: it uses hard 
  coded nodeTypes instead of the constants.

- In domplate.jsm, in the Renderer object, the replace() method: it makes use of 
  the document global object, which I doubt is correct. The document global 
  should be the treeBrowserDocument. The document object used as-is in the code, 
  may refer to the main browser chrome document object, or it may even be 
  undefined (the script is a module, it doesn't directly run in the chrome - am 
  I correct?)

I believe that, like setDOM() is invoked from the InspectorUI, there's a need to 
tell the domplate script what is the global document object.

I don't know when this is going to cause a bug, but it might be hard to debug 
when it hits us. Better to be safe than sorry.

- In domplate.jsm, in the BaseTemplates.Element.attrIterator method:

    if (classAttr)
      attrs.splice(0, 0, classAttr);
    if (idAttr)
      attrs.splice(0, 0, idAttr);

Why not use attrs.unshift(value) ?

- In domplate.jsm, in the DomplateTag.addCode() method:

    topBlock.splice(0, topBlock.length);
    topOuts.splice(0, topOuts.length);

Why not clear the arrays with topBlock = topOuts = [] ?

- In domplate.jsm, the DomplateTag.compileDOM() method:

    let self = this;

This is unused.

- There are quite many TODOs in the code. Do these come from domplate in 
  Firebug, or were they added now for the work on the new Inspector tree panel?

- There are some important sections of commented code inside the 
  BaseTemplates.Element object. I think these were commented out because they 
  are strongly related to Firebug. Shouldn't they be removed?

- Overall, domplate is really neat, but I think it's a tad too "hard to read".  
  There should be some documentation on its internals beyond the really great 
  blog posts of Honza about using the domplate code - with neat examples!  
  Nonetheless, I understood, finally, how it works internally - obviously I'd 
  still have to read the code a bit more before I can give a lecture on its 
  internals, hehe. :)

(It would certainly be an interesting exercise for anyone to make domplate work 
in other browsers, beyond the Gecko ones.)

- I am unsure if it's good to put the InsideOutBox in the global scope of the 
  inspector.js file - and thus in the global scope of the chrome window (IIANM).  
  Maybe this should go into an InspectorUtils object, where we can put the 
  InspectorProgressListener of bug 566084 as well. This is where other util 
  functions/methods/objects could live, moved out from the InspectorUI.

I really like the new tree panel - it looks much, much better!

Comment 12

[Julian Viereck]

(In reply to comment #10)
> Created attachment 462776 [details] [diff] [review]
> 
> Have not cleaned up these patches based on previous feedback comments yet. Will
> do soon.

Note: I've started on making the domplate patch following the Firefox styling rules.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Should I hold off on reviewing these patches, given the outstanding feedback (comment 10-comment 12)?

Comment 14

[Mihai Sucan [:msucan]]

Attachment: remove duplication of getNodeTag

As discussed on IRC with Robert I wrote a patch which fixes the problem reported in my feedback comment: this patch removes the duplication of getNodeTag and of the related methods/functions.

This patch should apply cleanly ontop of the DOM tree panel patches.


I would add to my feedback that:

- The getNodeTag() function of domplate.jsm has a second argument (expandAll) that is unused by the function. Why not remove it?

- The getNodeBoxTag() function of domplate.jsm is never used. I recommend removing the function.

Comment 15

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #13)
> Should I hold off on reviewing these patches, given the outstanding feedback
> (comment 10-comment 12)?

just for today. I'll get these things issues addressed by end of day.

Comment 16

[Rob Campbell [:rc] (:robcee)]

Julian, thanks for the review pass. Much-appreciated.

(In reply to comment #6)
> Comment on attachment 460913 [details] [diff] [review]
> domplate
[...]

> >+var DOM = {};
> >+
> >+let setDOM = function(glob)
> >+{
> >+  DOM = glob;
> >+};
> 
> o Don't declare anonymous function. Should be: let setDOM = function
> domplate_setDom(glob)

covered in Mihai's patch.

> o Missing JavaDoc

added.

> o parameter should be aGlob (prefix arguments with "a")
> (points apply for the rest of the patch)

fixed here.

> >+let domplate = function()
> >+{
> >+  let lastSubject;
> >+  for (let i = 0; i < arguments.length; ++i)
> >+    lastSubject = lastSubject ? copyObject(lastSubject, arguments[i]) : arguments[i];
> 
> o ddahl told me not to use let within for loops as TM can't trace them. Use var
> i = 0 ... instead (apply for the rest of the patch).
> o is there a rule to use i++ instead of ++i?

no rule, I think it makes sense in the context of a loop though.

I wasn't aware of the var vs. let thing for tracing.

I fixed it, and now have to revert them all. According to Jason Orendorff in #jsapi:

06:45 <@jorendorff> robcee: totally false
06:45 <@jorendorff> robcee: let traces fine
06:45 <@jorendorff> that's the second time I've heard that rumor

We need to stop propagating that.

> use for (...) {\n ... body ... \n}

For one-liners, I don't think it's terribly critical to have the braces. I remember back during a code style discussion, some were strongly in favor of this, and others (mainly our reviewer) were less critical of it.

You have to watch out for merges sometimes, but in general, I don't think there's much harm in it. I don't expect this file will get modified while it lives in our codebase. Because this is mostly a straight-copy, I'm going to leave these unless our ultimate reviewer decides they want it changed.

[...]

> >+///////////////////////////////////////////////////////////////////////////
> >+//// Base functions
> >+
> >+function DomplateTag(tagName)
> >+{
> >+    this.tagName = tagName;
> 
> Indention is wrong.

thanks. fixed.

> >+function DomplateEmbed()
> >+{
> >+}
[...]
> >+
> >+  compileMarkup: function()
> >+  {
> >+    this.markupArgs = [];
> >+    let topBlock = [], topOuts = [], blocks = [], info = {args: this.markupArgs,
> >+      argIndex: 0};
> 
>     More readable:
> 
>     let topBlock = [], topOuts = [], blocks = [], 
>       info = {args: this.markupArgs, argIndex: 0};

Thanks, fixed.

[...]
> >+    function __loop__(iter, outputs, fn)
> >+    {
> >+      let iterOuts = [];
> >+      outputs.push(iterOuts);
> >+
> >+      if (iter instanceof Array)
> >+        iter = new ArrayIterator(iter);
> >+
> >+      try {
> >+        while (1) {
> >+          let value = iter.next();
> >+          let itemOuts = [0,0];
> 
> Space after ",": let itemOuts = [0, 0];

OK.

[...]
> >+  generateChildDOM: function(path, blocks, args)
> >+  {
> >+    path.push(0);
> >+    for (let i = 0; i < this.children.length; ++i) {
> >+      let child = this.children[i];
> >+      if (isTag(child))
> >+        path[path.length-1] += '+' + child.tag.generateDOM(path, blocks, args);
> 
>   space around "-": path[path.length - 1] +=
> >+      else
> >+        path[path.length-1] += '+1';
> 
>   space around "-": path[path.length - 1] +=

OK, got these.

[...]
> >+
> >+  generateDOM: function(path, blocks, args)
> >+  {
> >+    let embedName = 'e'+path.embedIndex++;
> 
>   space around "+": 'e' + path.embedIndex++;

OK.

[...]

> >+  generateDOM: function(path, blocks, args)
> >+  {
> >+    let iterName = 'd'+path.renderIndex++;
> >+    let counterName = 'i'+path.loopIndex;
> >+    let loopName = 'l'+path.loopIndex++;
> 
>   Add spaces around the "+"
> 
>   let iterName = 'd' + path.renderIndex++;
>   let counterName = 'i '+ path.loopIndex;
>   let loopName = 'l' + path.loopIndex++;

those are ugly. Not sure how I missed them on my original pass.

> >+    if (!path.length)
> >+        path.push(-1, 0);
> 
>   Two spaces to much indented.

got it.

> >+    path.renderIndex = 0;
> >+
> >+    let nodeCount = 0;
> >+
> >+    let subBlocks = [];
> >+    let basePath = path[path.length-1];
> 
>   Add white spaces around "-": path.length - 1

yep.

> >+    for (let i = 0; i < this.children.length; ++i) {
> >+      path[path.length-1] = basePath+'+'+loopName+'+'+nodeCount;
> 
>   Add white spaces around "-" and "+":
>     path[path.length - 1] = basePath + '+' + loopName + '+' + nodeCount;

done.

> >+      let child = this.children[i];
> >+      if (isTag(child))
> >+        nodeCount += '+' + child.tag.generateDOM(path, subBlocks, args);
> >+      else
> >+        nodeCount += '+1';
> >+    }
> >+
> >+    path[path.length-1] = basePath+'+'+loopName;
> 
>   Add white spaces around "-" and "+".

yup.

> >+function generateArg(val, path, args)
> >+{
> >+  if (val instanceof Parts) {
> >+    let vals = [];
> >+    for (let i = 0; i < val.parts.length; ++i) {
> >+      let part = val.parts[i];
> >+      if (part instanceof Variable) {
> >+        let varName = 'd'+path.renderIndex++;
> 
>   Add spaces around "+":  let varName = 'd' + path.renderIndex++;

ok.

> >+        if (part.format) {
> >+          for (let j = 0; j < part.format.length; ++j)
> >+            varName = part.format[j] + '(' + varName + ')';
> >+        }
> >+
> >+        vals.push(varName);
> >+      }
> >+      else
> >+        vals.push('"'+part.replace(/"/g, '\\"')+'"');
> 
>   Add spaces around "+":  vals.push('"' + part.replace(/"/g, '\\"') + '"');

thanks.

[...]

> >+function addParts(val, delim, block, info, escapeIt)
> >+{
> >+  let vals = [];
> >+  if (val instanceof Parts) {
> >+    for (let i = 0; i < val.parts.length; ++i) {
> >+      let part = val.parts[i];
> >+      if (part instanceof Variable) {
> >+        let partName = part.name;
> >+        if (part.format) {
> >+          for (let j = 0; j < part.format.length; ++j)
> >+            partName = part.format[j] + "(" + partName + ")";
> >+        }
> >+
> >+        if (escapeIt)
> >+          vals.push("__escape__(" + partName + ")");
> >+        else
> >+          vals.push(partName);
> >+      }
> >+      else
> >+        vals.push('"'+ part + '"');
> >+    }
> >+  } else if (isTag(val)) {
> >+    info.args.push(val);
> >+    vals.push('s'+info.argIndex++);
> 
>   Add spaces around "+":  vals.push('s' + info.argIndex++);

OK.

> >+  } else
> >+    vals.push('"'+ val + '"');
> 
>   Add spaces around "+": vals.push('"' + val + '"');

got 'em.


> >+function cropString(text, limit, alterText)
> >+{
> >+  if (!alterText)
> >+    alterText = "..."; //…
> >+
> >+  text = text + "";
> >+
> >+  if (!limit)
> >+    limit = 88; // todo
> 
>   "todo" <<< what is the todo?

usually indicates something that needs to be revisited or cleaned up. Not sure what it is in this case, I don't think that's mine.

> >+  var halfLimit = (limit / 2);
> >+  halfLimit -= 2; // adjustment for alterText's increase in size
> >+
> >+  if (text.length > limit)
> >+    return text.substr(0, halfLimit) + alterText + text.substr(text.length-halfLimit);
> 
>   Add spaces around "-": ... + text.substr(text.length - halfLimit);

OK.

[...]
> >+  getTitle: function(object)
> >+  { // todo
> >+    let label = safeToString(object); // eg [object XPCWrappedNative [object foo]]
> >+
> >+    const re =/\[object ([^\]]*)/;
> >+    let m = re.exec(label);
> >+    let n = null;
> >+    if (m)
> >+      n = re.exec(m[1]);  // eg XPCWrappedNative [object foo
> 
>   As there is more then just one match, can you give m and n a more verbose
> name?

used objectMatch and secondObjectMatch since that's what we're looking for.

> >+    if (n)
> >+      return n[1];  // eg foo
> >+    else
> >+      return m ? m[1] : label;
> >+  },
> >+
> >+  getTooltip: function(object)
> >+  {
> >+    return null;
> >+  },
> >+
> >+  /*
> >+  * Called by chrome.onContextMenu to build the context menu when the underlying object has this rep.
> 
>   Far more then 80 characters per line.

that comment's clearly from Firebug anyway. As is the method. Should probably remove these if not used, but correcting for now in the hopes of not breaking anything along the way.

> >+  * See also Panel for a similar function also called by onContextMenu
> >+  * Extensions may monkey patch and chain off this call
> 
>   Missing point at the end of sentence.

I name thee, "The Quibbler"! ;)


> >+  getNodeTextGroups: function(element)
> >+  { // todo
> 
>   todo what?

I donno! Removed the todo comment and the commented out section beneath it.

removing other commented sections as well.

thanks!

Comment 17

[Mihai Sucan [:msucan]]

Attachment: test domplate output from the inspector

This patch should apply cleanly on top of the two main inspector tree panel patches.

What is included:

- An automated test that opens up a web page and checks the tree panel output. The tree panel output is compared to the result we have stored and the result we expect. This is, as discussed on IRC yesterday.

This doesn't only test domplate, it tests the whole inspector works correctly. I am not sure if having the result hard coded is ideal, but ultimately, that's what it all boils down to: testing already known conditions and seeing if we get expected results.

- Found an error which caused for [object Whatever] to show up in the class attribute for elements generated by domplate for displaying doctypes. Fixed.

- Found that the DOM panel fails to show when _showDOMPanel is false, by default. This also caused closeInspectorUI() to throw, because it tries to use the DOM panel when it doesn't exist. Fixed.

That's all! I hope you like it.

Comment 18

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #7)
> Comment on attachment 460378 [details] [diff] [review]
> tree-panel-rollup-20100726
> 
> Note: I've removed the domplate parts of this patch as I reviewed them in the
> other patch already and they are 100% the same. As I'm running out of time, I
> couldn't make it to review all of this patch. I removed the part that I haven't
> reviewed yet.

Incorporated into the domplate patch.

[...]
> > 
> > ///////////////////////////////////////////////////////////////////////////
> >-//// InspectorTreeView
> >+//// InsideOutBox
> > 
> > /**
> >- * TreeView object to manage the view of the DOM tree. Wraps and provides an
> >- * interface to an inIDOMView object
> >+ * InsideOutBoxView is a simple interface definition for views implementing
> >+ * InsideOutBox controls. All implementors must define these methods.
> >+ */
> >+
> 
> Remove newline here so that the comment is a JavaDoc for InsideOutBoxView.

done.

> >+InsideOutBoxView = {
> >+    /**
> >+     * Retrieves the parent object for a given child object.
> >+     */
> >+    getParentObject: function(child) {},
> 
> o function parameter should be prefixed with "a": aChild.
> o not a complete JavaDoc (@param and @returns is missing).
> o functions declare anonymous function, instead:
>       getParentObject: function IOBoxView_getParentObject(child) {},

done and done. (and done)

> This applies for the whole patch file.

u r strict!

> >+    /**
> >+     * Retrieves a given child node.
> >+     *
> >+     * If both index and previousSibling are passed, the implementation
> >+     * may assume that previousSibling will be the return for getChildObject
> >+     * with index-1.
> >+     */
> >+    getChildObject: function(parent, index, previousSibling) {},
> >+
> >+    /**
> >+     * Renders the HTML representation of the object. Should return an HTML
> >+     * object which will be displayed to the user.
> >+     */
> >+    createObjectBox: function(object, isRoot) {}
> 
> Fix indention.

OK.

> >+};
> >+
> >+/**
> >+ * Creates a tree based on objects provided by a separate "view" object.
> >  *
> >- * @param aWindow
> >- *        a top-level window object
> >+ * Construction uses an "inside-out" algorithm, meaning that the view's job is
> >+ * first to tell us the ancestry of each object, and secondarily its
> >+ * descendants.
> >+ *
> >+ * Constructor
> >+ * @param view
> >+ *        The view requiring the InsideOutBox.
> >+ * @param box
> >+ *        The box object containing the InsideOutBox. Required to add/remove
> >+ *        children during box manipulation (toggling opened or closed).
> 
> Missing @returns void. I don't repeat this on every JavaDoc.

I don't think we bother in the case of void returns.

[...]
> >-  getCellText: function ITV_getCellText(aRow, aCol)
> >+  openObject: function IOBox_openObject(aObject)
> >   {
> >-    return this.view.getCellText(aRow, aCol);
> >+    let object = aObject;
> >+    let firstChild = this.view.getChildObject(object, 0);
> >+    if (firstChild)
> >+      object = firstChild;
> 
>       if (firstChild) {
>         object = firstChild;
>       }

not needed.

[...]
> >    */
> >-  set selectedRow(anIndex)
> >+  expandObject: function IOBox_expandObject(aObject)
> >   {
> >-    this.view.selection.select(anIndex);
> >-    this.tree.treeBoxObject.ensureRowIsVisible(anIndex);
> >+    let objectBox = this.createObjectBox(aObject);
> >+    if (objectBox)
> >+      this.expandObjectBox(objectBox);
> 
>       if (objectBox) {
>         this.expandObjectBox(objectBox);
>       }
> 
> >   },
> > 
> >   /**
> >-   * Set the selected node to the specified document node.
> >-   *
> >-   * @param aNode
> >-   *        The document node to select in the tree.
> >+   * Contract the given object in the tree.
> >+   * @param aObject
> >+   *        The tree node to contract.
> >    */
> >-  set selectedNode(aNode)
> >+  contractObject: function IOBox_contractObject(aObject)
> >   {
> >-    let rowIndex = this.view.getRowIndexFromNode(aNode);
> >-    if (rowIndex > -1) {
> >-      this.selectedRow = rowIndex;
> >-    } else {
> >-      this.selectElementInTree(aNode);
> >+    let objectBox = this.createObjectBox(aObject);
> >+    if (objectBox)
> >+      this.contractObjectBox(objectBox);
> 
>       if (objectBox) {
>         this.contractObjectBox(objectBox);
>       }

also not needed

[...]
> >+  /**
> >+   * Highlight the given objectBox in the tree.
> >+   * @param aObjectBox
> >+   *        The objectBox to highlight.
> >+   */
> >+  highlightObjectBox: function IOBox_highlightObjectBox(aObjectBox)
> >+  {
> >+    if (this.highlightedObjectBox) {
> >+      this.view.style.removeClass(this.highlightedObjectBox, "highlighted");
> >+
> >+      let highlightedBox = this.getParentObjectBox(this.highlightedObjectBox);
> >+      for (; highlightedBox; highlightedBox = this.getParentObjectBox(highlightedBox))
> 
>   Can you use a while loop here? 
>         while (highlightedBox = this.getParentObjectBox(highlightedBox)) {

We'll need a do { } while (highlightedBox = getParentObjectBox...) call for this to work. Not sure if that's more readable or not. I find those constructs a little weird.

> >+        this.view.style.removeClass(highlightedBox, "highlightOpen");
> >+    }
> >+
> >+    this.highlightedObjectBox = aObjectBox;
> >+
> >+    if (aObjectBox) {
> >+      this.view.style.setClass(aObjectBox, "highlighted");
> >+
> >+      let highlightedBox = this.getParentObjectBox(aObjectBox);
> >+      for (; highlightedBox; highlightedBox = this.getParentObjectBox(highlightedBox))
> 
>   Can you use a while loop here? 
>         while (highlightedBox = this.getParentObjectBox(highlightedBox)) {

same!

[...]
> >+  /**
> >+   * Open the ancestors of the given object box.
> >+   * @param aObjectBox
> >+   *        The object box to open.
> >+   */
> >+  openObjectBox: function IOBox_openObjectBox(aObjectBox)
> >+  {
> >+    if (aObjectBox) {
> >+      // Set all of the node's ancestors to be permanently open
> >+      let parentBox = this.getParentObjectBox(aObjectBox);
> >+      let labelBox;
> >+      for (; parentBox; parentBox = this.getParentObjectBox(parentBox)) {
> 
> Can you use a while loop here?
>         while (parentBox = this.getParentObjectBox(parentBox)) {

sure!

[...]
> >+  /**
> >+   * Find the child object box for a given repObject within the subtree
> >+   * rooted at aParentNodeBox
> 
> Missing point at the end of the sentence.

quibbler! :)

> >+   * @param aParentNodeBox
> >+   *        root of the subtree in which to search for repObject
> 
> Missing point at the end of the sentence.

sometimes I forget things...

> >+   * @param aRepObject
> >+   *        The object you wish to locate in the subtree.
> >+   * @returns an object box or null
> >+   */
> >+  findChildObjectBox: function IOBox_findChildObjectBox(aParentNodeBox, aRepObject)
> >+  {
> >+    for (let childBox = aParentNodeBox.firstChild; childBox; childBox = childBox.nextSibling) {
> 
> I don't like the for loop. I would write:
> 
>         var childBox = aParentNodeBox.firstChild;
>         while (childBox) {
>           body;
>           childBox = childBox.nextSibling
>         }
> 
> but that might be question of taste.

yeah, that works. Makes for a shorter line too.

[...]
> >+    } while (parentNode);
> >+    return null;
> >   },
> > };
> > 
> > ///////////////////////////////////////////////////////////////////////////
> >+//// Local Helpers
> >+
> >+
> >+
> 
> This section is empty? Can it be removed?

It sure can.

> >+///////////////////////////////////////////////////////////////////////////
> > //// InspectorUI
[...]
> >+
> >+  /**
> >+   * Return the owner panel of the node.
> 
> Missing @param + @returns

added.

> >+   */
> >+  getOwnerPanel: function IUI_getOwnerPanel(node)
> >+  {
> >+    for (; node; node = node.parentNode) {
> >+      if (node.ownerPanel)
> >+        return node.ownerPanel;
> >+    }
> >+  },
> >+
> >+  /**
> >    * Open the inspector's tree panel and initialize it.
> 
> Missing @param + @returns

nulls - unneeded.

[...[
> >+    }
> >+  },
> >+
> >+  getChildObject: function IUI_getChildObject(node, index, previousSibling)
> >+  {
> >+    if (!node)
> >+      return;
> 
>   if (...) { }
>   Lots of them in this function.

We're just gonna leave them for now.

[...]
> >   /**
> >-   * Event fired when a tree row is selected in the tree panel.
> >+   * Handle click events in the html tree panel.
> 
> Missing @param and @returns.

[...]
> > 
> > XPCOMUtils.defineLazyGetter(InspectorUI, "inspectCmd", function () {
> >diff --git a/browser/base/content/stylePanel.jsm b/browser/base/content/stylePanel.jsm
> >--- a/browser/base/content/stylePanel.jsm
> >+++ b/browser/base/content/stylePanel.jsm
> >@@ -311,4 +311,88 @@
> >       this.getStyleProperties(aNode, aRules, aUsedProps, aInherit);
> >     }
> >   },
> >+
[...]
> >+  hasClass: function CSS_hasClass(node, name)
> >+  {
> >+    if (!node || node.nodeType != 1 || !node.className || name == '')
> >+      return false;
> >+
> >+  if (name.indexOf(" ") != -1) {
> 
> Fix indention.

oops!

> >+      let classes = name.split(" "), len = classes.length, found = false;
> >+      for (var i = 0; i < len; i++) {
> >+        let cls = classes[i].trim();
> >+        if (cls != "") {
> >+          if (this.hasClass(node, cls) == false)
> >+            return false;
> >+          found = true;
> >+        }
> >+      }
> >+      return found;
> >+    }
> >+
> >+    let re;
> >+    if (name.indexOf("-") == -1)
> >+      re = new RegExp('(^|\\s)' + name + '(\\s|$)', "g");
> >+    else
> >+      re = new RegExp('(^|\\s)' + name + '(\\s|$)', "g")
> >+    return node.className.search(re) != -1;
> 
> I don't really understand what you're doing with the RegExp here. Would it be
> hard
> to do things without them?

We're looking for classnames within a string of possibly multiple classnames. We could use split(" ") and look for the string in the resultant array. Or look for a substring...

Those regexps are identical though, so I think whatever logic they were doing (if the name has a - in it, do something different?) is no longer needed.

Also, the second one is missing a semi-colon.

I'm just going to coalesce these into one regexp and comment it.

I really wish I could get the classList API to work in here but for some reason I couldn't.

> >+  removeClass: function CSS_removeClass(node, name)
> >+  {
> >+    if (!node || node.nodeType != 1 || node.className == '' || name == '')
> >+      return;
> >+
> >+    if (name.indexOf(" ") != -1) {
> >+      let classes = name.split(" "), len = classes.length;
> >+      for (var i = 0; i < len; i++) {
> >+        let cls = classes[i].trim();
> >+        if (cls != "") {
> >+          if (this.hasClass(node, cls) == false)
> >+            this.removeClass(node, cls);
> >+        }
> >+      }
> >+      return;
> >+    }
> >+
> >+    let re;
> >+    if (name.indexOf("-") == -1)
> >+      re = new RegExp('(^|\\s)' + name + '(\\s|$)', "g");
> >+    else
> >+      re = new RegExp('(^|\\s)' + name + '(\\s|$)', "g");
> >+
> >+    node.className = node.className.replace(re, " ");
> 
> I don't really understand what you're doing with the RegExp here. Would it be
> hard
> to do things without them?

Again, probably not, but I'm going to do as I did in the above and reduce it to the one regexp.

> 
> Haven't to stop here.
> =====================

Thanks for the comments! Most helpful.

Comment 19

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #18)
> We'll need a do { } while (highlightedBox = getParentObjectBox...) call for
> this to work. Not sure if that's more readable or not. I find those constructs
> a little weird.

scratch that. The do {} while; looked funny so I just used a straight while loop.

Comment 20

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #11)
> Hello Robert!
> 
> Below is my review/feedback on the tree panel patches.
> 
> - In inspector.js, the InsideOutBoxView object is unused.

It's an "interface spec" which InspectorUI implements. I added a comment to that effect, but we may just want to rip it out or comment it to avoid namespace pollution.

> The InspectorUI.ioBox is setup by the InspectorUI.treeBrowser onload event 
> handler found in the InspectorUI.openTreePanel() method. There the view of the 
> ioBox is the InspectorUI itself.

That's right.

> - InspectorUI makes use of this.embeddedBrowserParents which is not explained 
> and not defined in at the start of the object definition. I believe it should
> be 
> defined and explain with a short comment.

I think these are used to speed up document traversal when looking for parent/child nodes.

> - InspectorUI.getParentObject() uses nodeType == 9. Why not use the 
> DOCUMENT_NODE constant?

Good call. Done.

> - InspectorUI.createObjectBox(object, isRoot): the second argument is unused.

No, but it's defined in the "spec" above. Maybe it got removed at some point in this method's evolution.

> - InspectorUI.createObjectBox() uses InspectorUI.getNodeTag(). Why not use the 
>   getNodeTag() method of the domplate js module? It seems the InspectorUI
> method 
>   only duplicates the functionality.

this was addressed in your follow-up patch and integrated here. Thanks!

> Not only that, but having copied the getNodeTag() method into the InspectorUI, 
> it also required the copying of several more functions that are now new methods 
> in the InspectorUI: isContainerElement, isWhitespace, isWhitespaceText, 
> isSelfClosing, isEmptyElement, hasNoElementChildren. I believe that if these
> are 
> not duplicated, then the inspector.js diff would be quite smaller and it would 
> really help reviewing.

see above.

> - In the InspectorUI.hasNoElementChildren() method I believe the code should be 
>   trimmed to only check using the element.childElementCount property, because 
>   the Inspector tool will only run inside Firefox 4+ anyway.

* TODO in a domplate followup.

> - In the InspectorUI.defaultSelection getter, the getPreviousElement() method
> is 
>   used, which both look like:
> 
>   get defaultSelection()
>   {
>     let doc = this.win.document;
>     return doc.body ? doc.body :
>       this.getPreviousElement(doc.documentElement.lastChild);
>   },

ok.

>   getPreviousElement: function IUI_getPreviousElement(aNode)
>   {
>     while (aNode && aNode.nodeType != 1)
>       aNode = aNode.previousSibling;
> 
>     return aNode;
>   }
> 
> Why not use doc.documentElement.lastElementChild in the defaultSelection
> getter?
> 
> Also, the getPreviousElement() method is legacy code which can be removed. The 
> previousElementSibling property of the node in question can be used.

right you are. no senders. Removed!

> I would add that the getPreviousElement() method is used only by the 
> defaultSelection getter, making it easy to remove now.

and it is.

> - Applying these patches on top of the patch from bug 561782 makes the DOM
> panel 
>   to fail. Somehow, for some reason, the DOM panel never shows up.

fixed by setting _showDOMPanel = true; Included in this patch.

> - In the InsideOutBox.toggleObjectBox() method:
> 
>     let nodeLabel = aObjectBox.getElementsByClassName("nodeLabel").item(0);
>     let labelBox = nodeLabel.getElementsByClassName('nodeLabelBox').item(0);
> 
> ... then the labelBox variable is never used. I believe both lines can be 
> removed.

Wow. Nice catch. Removed!

> - The InspectorUI.toggleNode() looks like it should belong in the InsideOutBox 
>   object, renamed to toggleObject() - there's already toggleObjectBox() and 
>   there are the expand/contractObject() methods.

oh yes. very nice.

> - In the InsideOutBox object there are unused methods: appendChildBox, 
>   insertChildBoxBefore, and removeChildBox.

probably someone with an over-eager urge to complete an API. Or maybe they anticipated a need that never materialized.

> - In domplate.jsm, in the Renderer object, the replace() method: it uses hard 
>   coded nodeTypes instead of the constants.
> 
> - In domplate.jsm, in the Renderer object, the replace() method: it makes use
> of 
>   the document global object, which I doubt is correct. The document global 
>   should be the treeBrowserDocument. The document object used as-is in the
> code, 
>   may refer to the main browser chrome document object, or it may even be 
>   undefined (the script is a module, it doesn't directly run in the chrome - am 
>   I correct?)
> 
> I believe that, like setDOM() is invoked from the InspectorUI, there's a need
> to 
> tell the domplate script what is the global document object.
> 
> I don't know when this is going to cause a bug, but it might be hard to debug 
> when it hits us. Better to be safe than sorry.

TODO in followup domplate patch

> 
> - In domplate.jsm, in the BaseTemplates.Element.attrIterator method:
> 
>     if (classAttr)
>       attrs.splice(0, 0, classAttr);
>     if (idAttr)
>       attrs.splice(0, 0, idAttr);
> 
> Why not use attrs.unshift(value) ?
> 
> - In domplate.jsm, in the DomplateTag.addCode() method:
> 
>     topBlock.splice(0, topBlock.length);
>     topOuts.splice(0, topOuts.length);
> 
> Why not clear the arrays with topBlock = topOuts = [] ?
> 
> - In domplate.jsm, the DomplateTag.compileDOM() method:
> 
>     let self = this;
> 
> This is unused.
> 
> - There are quite many TODOs in the code. Do these come from domplate in 
>   Firebug, or were they added now for the work on the new Inspector tree panel?

some came from Firebug, some I added while doing the port. I stripped out a bunch of commented code in an earlier sweep.

> 
> - There are some important sections of commented code inside the 
>   BaseTemplates.Element object. I think these were commented out because they 
>   are strongly related to Firebug. Shouldn't they be removed?

yep, done.

> - I am unsure if it's good to put the InsideOutBox in the global scope of the 
>   inspector.js file - and thus in the global scope of the chrome window
> (IIANM).  
>   Maybe this should go into an InspectorUtils object, where we can put the 
>   InspectorProgressListener of bug 566084 as well. This is where other util 
>   functions/methods/objects could live, moved out from the InspectorUI.

There's a bug to modularize inspector.js. We can probably do that then. Should be safe for now.

> I really like the new tree panel - it looks much, much better!

Great! Thanks for the review!

Comment 21

[Rob Campbell [:rc] (:robcee)]

Attachment: inspector tree panel (from feedback)

Comment 22

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 463140 [details] [diff] [review]
remove duplication of getNodeTag

this works well and is incorporated into the new inspector tree panel patch. Thanks!

Comment 23

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #20)
> (In reply to comment #11)
[...]
> > - In the InspectorUI.hasNoElementChildren() method I believe the code should be 
> >   trimmed to only check using the element.childElementCount property, because 
> >   the Inspector tool will only run inside Firefox 4+ anyway.
> 
> * TODO in a domplate followup.

true. Removed.

[...]
> > - In domplate.jsm, in the Renderer object, the replace() method: it uses hard 
> >   coded nodeTypes instead of the constants.

fixed.

> > - In domplate.jsm, in the Renderer object, the replace() method: it makes use
> > of 
> >   the document global object, which I doubt is correct. The document global 
> >   should be the treeBrowserDocument. The document object used as-is in the
> > code, 
> >   may refer to the main browser chrome document object, or it may even be 
> >   undefined (the script is a module, it doesn't directly run in the chrome - am 
> >   I correct?)

It runs in its own scope, so I have no idea what "document" would refer to. Possibly something unique to its context.

> > I believe that, like setDOM() is invoked from the InspectorUI, there's a need
> > to 
> > tell the domplate script what is the global document object.
> > 
> > I don't know when this is going to cause a bug, but it might be hard to debug 
> > when it hits us. Better to be safe than sorry.
> 
> TODO in followup domplate patch

very confusing, and I'm not sure how this would work in Firebug either. it's domplate is wrapped up in with to make the contexts extra confusing. It also lives in chrome.

parent should never be undefined though. We call "replace" with a parent when we want to execute our domplate. I'm tempted to either throw an error or return from this as it's a contractual error we have with domplate.

> > - In domplate.jsm, in the BaseTemplates.Element.attrIterator method:
> > 
> >     if (classAttr)
> >       attrs.splice(0, 0, classAttr);
> >     if (idAttr)
> >       attrs.splice(0, 0, idAttr);
> > 
> > Why not use attrs.unshift(value) ?

I think this was born before unshift was invented. I've fixed these elsewhere too.

Fixed.

> > - In domplate.jsm, in the DomplateTag.addCode() method:
> > 
> >     topBlock.splice(0, topBlock.length);
> >     topOuts.splice(0, topOuts.length);
> > 
> > Why not clear the arrays with topBlock = topOuts = [] ?

good question. Whoever wrote this really likes splice!

> > 
> > - In domplate.jsm, the DomplateTag.compileDOM() method:
> > 
> >     let self = this;
> > 
> > This is unused.

weird. Forgotten closure?

Anyway, these comments have been addressed. Thanks again for the review.

Comment 24

[Rob Campbell [:rc] (:robcee)]

Attachment: domplate

had to re-add the topBlock+topOuts.splices. setting values on arguments that are being passed around in here is a no-no.

Comment 25

[Rob Campbell [:rc] (:robcee)]

Attachment: Inspector tree panel rebased 2010-08-12

Comment 26

[Rob Campbell [:rc] (:robcee)]

note that the order of the patches should be:

* Inspector tree panel
* Domplate
* Test domplate output

Comment 27

[Rob Campbell [:rc] (:robcee)]

Attachment: Inspector tree panel rebased 2010-08-13

fixed domplate initialization.

Comment 28

[Mihai Sucan [:msucan]]

Attachment: inspector tests fixes

As discussed with Robert, I wrote a patch that fixes all Inspector tests.

Note that the order of patches is now:

* Inspector tree panel
* Domplate
* Test domplate output
* tests fixes

(the new tree panel broke older inspector tests, hence the need for fixes)

Comment 29

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #28)
> Created attachment 467888 [details] [diff] [review]
> inspector tests fixes
> 
> As discussed with Robert, I wrote a patch that fixes all Inspector tests.
> 
> Note that the order of patches is now:
> 
> * Inspector tree panel
> * Domplate
> * Test domplate output
> * tests fixes
> 
> (the new tree panel broke older inspector tests, hence the need for fixes)

order should be Domplate then Inspector Tree Panel, though they may apply correctly in either order.

Comment 30

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 465810 [details] [diff] [review]
Inspector tree panel rebased 2010-08-13

Partial comments...

>diff --git a/browser/base/content/stylePanel.jsm b/browser/base/content/stylePanel.jsm

>+  hasClass: function CSS_hasClass(node, name)

Does this actually need to support "name" being a whitespace separated list of classes? It doesn't seem to be used that way, and it otherwise could just be:

let nodeGlobal = Components.utils.getGlobalForObject(el);
return node instanceof nodeGlobal.Element && node.classList.contains(name);

>+  removeClass: function CSS_removeClass(node, name)
>+  setClass: function CSS_setClass(node, name)

Similarly these could use node.classList.add()/remove();

>diff --git a/browser/base/content/test/browser_inspector_domPanel.js b/browser/base/content/test/browser_inspector_domPanel.js

>-function runDOMTests(evt)
>+var runDOMTests = {

This change is unnecessary - you can pass a function object to addObserver since bug 538920. Same comment applies to the other tests.

>diff --git a/browser/themes/gnomestripe/browser/inspector.css b/browser/themes/gnomestripe/browser/inspector.css

>+  background: url(chrome://firebug/skin/group.gif) repeat-x #FFFFFF;

>+  background: url(chrome://firebug/skin/loading_16.gif) no-repeat;

These look wrong...

>diff --git a/browser/themes/winstripe/browser/twistyClosed.png b/browser/themes/winstripe/browser/twistyClosed.png
>diff --git a/browser/themes/winstripe/browser/twistyOpen.png b/browser/themes/winstripe/browser/twistyOpen.png

We already have twisty-clsd.png and twisty-open.png in toolkit/themes/winstripe/global/tree/ that you can use.

Comment 31

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 465810 [details] [diff] [review]
Inspector tree panel rebased 2010-08-13

>diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js

>   openInspectorUI: function IUI_openInspectorUI()

>+    if (!this.domplate) {
>+      Cu.import("resource:///modules/domplate.jsm", this);
>+      let dom = Cu.getGlobalForObject(Node);
>+      this.domplateUtils.setDOM(dom);

The global for "Node" is the current global scope, so this doesn't make much sense. setDOM(window) would be equivalent.

>-    this.inspectCmd.setAttribute("checked", true);
>+    this.toolsInspectCmd.setAttribute("checked", true);

This looks wrong, since AFAICT this.toolsInspectCmd doesn't exist.

>   closeInspectorUI: function IUI_closeInspectorUI()
>   {
>+    let toolsInspectCmd = document.getElementById("Tools:Inspect");

Why introduce this when this.inspectCmd already exists? Maybe this is on top of some other patch I'm missing?

I noticed that this depends on domplate to work, which seems to go against your proposed order in comment 26.

Comment 32

[:Gavin Sharp [email: [e-mail]]]

Oh, I missed comment 29. Ignore that part.

Comment 33

[Rob Campbell [:rc] (:robcee)]

Attachment: Inspector tree panel rebased 2010-08-24

Rolled domplate tests and inspector test fixes into this patch.

Comment 34

[Rob Campbell [:rc] (:robcee)]

just a note that all inspector tests pass currently with these patches in place as of today.

Comment 35

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #30)
> Comment on attachment 465810 [details] [diff] [review]
> Inspector tree panel rebased 2010-08-13
> 
> Partial comments...
> 
> >diff --git a/browser/base/content/stylePanel.jsm b/browser/base/content/stylePanel.jsm
> 
> >+  hasClass: function CSS_hasClass(node, name)
> 
> Does this actually need to support "name" being a whitespace separated list of
> classes? It doesn't seem to be used that way, and it otherwise could just be:
> 
> let nodeGlobal = Components.utils.getGlobalForObject(el);
> return node instanceof nodeGlobal.Element && node.classList.contains(name);
>
> >+  removeClass: function CSS_removeClass(node, name)
> >+  setClass: function CSS_setClass(node, name)
> 
> Similarly these could use node.classList.add()/remove();

I tried using the classList API while back but didn't have any luck with it. Maybe your getGlobalForObject() function is the incantation I need to make it work. I'm happy to get rid of these methods with something system-level.

> >diff --git a/browser/base/content/test/browser_inspector_domPanel.js b/browser/base/content/test/browser_inspector_domPanel.js
> 
> >-function runDOMTests(evt)
> >+var runDOMTests = {
> 
> This change is unnecessary - you can pass a function object to addObserver
> since bug 538920. Same comment applies to the other tests.

That would be a bit of a cleanup, though I don't see it as super-necessary if these work as-implemented. Required change?

> >diff --git a/browser/themes/gnomestripe/browser/inspector.css b/browser/themes/gnomestripe/browser/inspector.css
> 
> >+  background: url(chrome://firebug/skin/group.gif) repeat-x #FFFFFF;
> 
> >+  background: url(chrome://firebug/skin/loading_16.gif) no-repeat;
> 
> These look wrong...

I see what you mean. We'll fix those up.

> >diff --git a/browser/themes/winstripe/browser/twistyClosed.png b/browser/themes/winstripe/browser/twistyClosed.png
> >diff --git a/browser/themes/winstripe/browser/twistyOpen.png b/browser/themes/winstripe/browser/twistyOpen.png
> 
> We already have twisty-clsd.png and twisty-open.png in
> toolkit/themes/winstripe/global/tree/ that you can use.

Ok, I'll take a look.

thanks for the partials!

Comment 36

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #35)
> > This change is unnecessary - you can pass a function object to addObserver
> > since bug 538920. Same comment applies to the other tests.
> 
> That would be a bit of a cleanup, though I don't see it as super-necessary if
> these work as-implemented. Required change?

Well, this patch changes existing code unnecessarily - best to be avoided in general, and the object/method syntax is more complicated. It isn't really hard to revert those changes, is it?

Comment 37

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #36)
> (In reply to comment #35)
> > > This change is unnecessary - you can pass a function object to addObserver
> > > since bug 538920. Same comment applies to the other tests.
> > 
> > That would be a bit of a cleanup, though I don't see it as super-necessary if
> > these work as-implemented. Required change?
> 
> Well, this patch changes existing code unnecessarily - best to be avoided in
> general, and the object/method syntax is more complicated. It isn't really hard
> to revert those changes, is it?

because of the changes to the DOM panel (now PropertyPanel) and the delayed loading of the tree panel, I needed a new mechanism to notify interested parties (i.e., the tests) that the inspector was up and running. Hence the observer. Changing the implementation of the observer to use the simpler observer function is no big deal, but I'm not sure it's really worth it either. I think it's possibly more explicit in what's happening with the tests that use it.

I'll change it if necessary, though.

Comment 38

[Rob Campbell [:rc] (:robcee)]

Attachment: Inspector tree panel 2010-08-24 post comments

Initial comments addressed.

* hasClass, setClass, removeClass replaced with classList methods
* CSS cleaned up based on comments (tested on mac and windows)
* tests reverted to using functions instead of observer objects

Outstanding issue: browser_inspector_output.js is failing 1 test due to differences in output from new classList api. I think it's whitespace but I want mihai to take a look at it.

Comment 39

[Mihai Sucan [:msucan]]

Attachment: browser_inspector_treePanel_output test fix

This patch fixes the tree panel output test. Apply on top of domplate+treepanel patches.

Indeed, it's only a difference of whitespaces.

Comment 40

[Rob Campbell [:rc] (:robcee)]

Attachment: Inspector tree panel 2010-08-25

Updated patch addressing two things:

* rolled mihai's treePanel output test fix patch into this one.
* added close button to tree panel with an event listener to shutdown the inspector

Comment 41

[Rob Campbell [:rc] (:robcee)]

Attachment: Inspector tree panel 2010-08-25.2

rolled some code from my editor patch in bug 575234 into this one to make twisties work better.

Comment 42

[Mihai Sucan [:msucan]]

Attachment: closeInspectorUI() failure fix

The latest tree panel patch introduced a regression: when one calls the InspectorUI.closeInspectorUI() method, it throws, because it closes the tree panel, which in turn is invoked by the popuphiding event. (closeInspectorUI is also the event handler for popuphiding). This bug caused all Inspector tests to fail.

This minimal patch fixes the regression.

Comment 43

[:Gavin Sharp [email: [e-mail]]]

More partial comments:

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

> <window id="main-window"

>+        xmlns:html="http://www.w3.org/1999/xhtml"

This doesn't appear to be needed.

>+      <browser id="inspector-tree-browser"

Can this be type="content"? Might also want disablesecurity="true".

>diff --git a/browser/base/content/inspector.html b/browser/base/content/inspector.html

>+<head>
>+  <title>Inspector</title>

l10n? Or if this isn't used (doesn't appear to be), just remove it.

>diff --git a/browser/themes/gnomestripe/browser/inspector.css b/browser/themes/gnomestripe/browser/inspector.css

>+/* See license.txt for terms of usage */

What are these about? I don't see a license.txt anywhere. Are these styles copied from somewhere? It seems like there are a lot of unused styles. (all the infoTip* stuff, fullPanelEditor, etc.).

>diff --git a/browser/themes/winstripe/browser/jar.mn b/browser/themes/winstripe/browser/jar.mn

>+        skin/classic/aero/browser/twistyClosed.png
>+        skin/classic/aero/browser/twistyOpen.png

Forgot to remove these?

Comment 44

[:Gavin Sharp [email: [e-mail]]]

You should keep hasClass()/removeClass()/addClass() helpers method rather than sprinkling instanceof Element checks all over the place. Are the instanceof checks really all needed though? When are object boxes not Elements?

Comment 45

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 469061 [details] [diff] [review]
Inspector tree panel 2010-08-25.2

>diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js

>+const RE_NOT_WHITESPACE = /[^\s]/;

Might as well just put this in isWhiteSpace, rather than at the global level.

>+/*
>+ * Begin Firebug license section
>+ */

Probably better to put this in a separate file, and preprocessor #include it here, given the different-license issue. It would also make the diff much easier to review (showing removal+additions seperately, rather than interleaved).

>+//// InsideOutBox
> 
> /**
>+ * InsideOutBoxView is a simple interface definition for views implementing
>+ * InsideOutBox controls. All implementors must define these methods.
>+ * Implemented in InspectorUI.
>  */
>+InsideOutBoxView = {

Put this all in a comment, to make it clear that it's an interface definition, rather than dead code?

>+InsideOutBox = function(aView, aBox)

function InsideOutBox(aView, aBox)

>+InsideOutBox.prototype =

>+  openObject: function IOBox_openObject(aObject)

>+    let objectBox = this.createObjectBox(object);
>+    this.openObjectBox(objectBox);
>+    return objectBox;

this.openToObject(object);

>   /**
>+   * Select the given object node in the tree.
>+   * @param aObject
>+   *        The object node to select.
>+   * @param makeBoxVisible
>+   *        Do we open the objectBox?
>+   * @param forceOpen
>+   *        Force the object box open?

The distinction between "open the objectBox" and "force the object box open" is rather confusing!

It seems like forceOpen controls whether we call expandObjectBox, which controls whether children are visible.
makeBoxVisible controls whether we call openObjectBox, which controls whether the we open all of its ancestors.

Consistent terminology throughout wrt "expanded" vs. "open" would be nice... As it is things are just horribly confusing.

>+  /**
>+   * Expand the given object in the tree.
>+   * @param aObject
>+   *        The tree node to expand.
>+   */

"Expands/contracts the given object, depending on its state.", "The tree node to expand/contract."

>+  highlightObjectBox: function IOBox_highlightObjectBox(aObjectBox)

Seems like a generic function for iterating over ancestors would be useful:
function iterateObjectAncestors(aObject, aCallback) {
  let object = aObject;
  while ((object = this.getParentObjectBox(object)))
    callback(object);
}

then this method could do:
let self = this;
iterateHighlightedAncestors(this.highlightedObjectBox, function (box) {
  self.removeClass(box, "highlightOpen");
});
iterateHighlightedAncestors(this.highlightedObjectBox, function (box) {
  self.addClass(box, "highlightOpen");
});

and other methods could use it too (e.g. openObjectBox())
(can be a followup)

>+  selectObjectBox: function IOBox_selectObjectBox(aObjectBox, forceOpen)

>+        // Force it open the first time it is selected
>+        if (forceOpen)
>+          this.toggleObjectBox(aObjectBox, true);

Why not call expandObjectBox() directly?

>+  openObjectBox: function IOBox_openObjectBox(aObjectBox)

>+    if (aObjectBox) {

Use an early return instead, to reduce indentation.

>+  createObjectBox: function IOBox_createObjectBox(aObject)

>+    if (!objectBox)
>+      return null;
>+    else if (aObject == this.rootObject)
>+      return objectBox;
>+    else
>+      return this.populateChildBox(aObject, objectBox.parentNode);

remove else-after-return (here and everywhere else - there are a few)

>+  createObjectBoxes: function IOBox_createObjectBoxes(aObject, aRootObject)

>+  findObjectBox: function IOBox_findObjectBox(aObject)

>+  populateChildBox: function IOBox_populateChildBox(repObject, nodeChildBox)

I find the logic in these really hard to follow - the whole setup (with objects/object boxes etc.) is confusing. I know this mostly isn't your code, though, and it seems to work OK, I guess, so I suppose I won't stress about it too much...

>+  getChildObjectBox: function IOBox_getChildObjectBox(aObjectBox)

>+    return aObjectBox.getElementsByClassName("nodeChildBox").item(0);

objectBox.querySelector(".nodeChildBox")?

>+  getRootNode: function IOBox_getRootNode(aNode)

How about:

let node = aNode;
let tmpNode;
while ((tmpNode = this.view.getParentObject(node)))
  node = tmpNode;

return node;

> var InspectorUI = {

>+  noScrollIntoView: false,

Appears to be unused.

>+  getOwnerPanel: function IUI_getOwnerPanel(aNode)
>+  {
>+    for (; node; node = node.parentNode) {
>+      if (node.ownerPanel)
>+        return node.ownerPanel;
>+    }

undeclared variable "node" - but it turns out this is unused, so we can remove it!

>+  getParentObject: function IUI_getParentObject(node)

Else-after-returns in this function are deadly.

>+    } else {
>+      // Documents have no parentNode; Attr, Document, DocumentFragment, Entity,
>+      // and Notation. top level windows have no parentNode
>+      if (node && node.nodeType == 9) {

Node.DOCUMENT_NODE

>+        if (node.defaultView) {
>+          // generally a reference to the window object for the document,
>+          // however that is not defined in the specification

remove this comment, specification isn't relevant here (defaultView is garanteed to be the window)

>+          let embeddingFrame = node.defaultView.frameElement;
>+          if (embeddingFrame)
>+            return embeddingFrame.parentNode;

Why return the frame's parent rather than the frame itself (as in the case above)?

>+        } else // a Document object without a parentNode or window
>+          return null;  // top level has no parent
>+      }
>     }

>+  getChildObject: function IUI_getChildObject(node, index, previousSibling)
>+  {
>+    if (!node)
>+      return;

return null?

>+    /* if (this.isSourceElement(node)) {
>+      if (index == 0)
>+        return this.getElementSourceText(node);
>+      else
>+        return null;  // no siblings of source elements
>+    } else */

Either remove or uncomment.

>+    } else if (node.getSVGDocument && node.getSVGDocument()) {

(node instanceof GetSVGDocument) (what a weird interface name...)

>+  getFirstChild: function IUI_getFirstChild(node)

How does this differ from node.firstChild?

>+  getNextSibling: function IUI_getNextSibling(node)

And this from node.nextSibling?

>   openInspectorUI: function IUI_openInspectorUI()

>+    if (!this.domplate) {
>+      Cu.import("resource:///modules/domplate.jsm", this);
>+      let dom = Cu.getGlobalForObject(Node);
>+      this.domplateUtils.setDOM(dom);
>+    }

This still hasn't been addressed from my last comments (first part of comment 31).

>   closeInspectorUI: function IUI_closeInspectorUI()

>+    this.inspectCmd.setAttribute("checked", "false");

This line already exists below.

>+  select: function IUI_select(aNode, forceUpdate)

>+      let box = this.ioBox.createObjectBox(this.selection);
>+        box.scrollIntoView(true); // todo scrollIntoCenterView would be nicer

ioBox.select (called via updateSelection) seems to handle this, do you really need to do them pre-emptively?

>+      this.updateSelection(this.selection);

>+  updateSelection: function IUI_updateSelection(aNode)
>+  {
>+    this.ioBox.select(aNode, true, true);

Might as well just inline this...

>   /**
>+   * Get the repObject from the HTML panel's domplate-constructed DOM node.

What does "repObject" represent? Worth adding a comment.

>+  isWhitespaceText: function IUI_isWhitespaceText(node)
>+  {
>+    if (node instanceof HTMLAppletElement)
>+      return false;

What's this for? Seems pretty random...

>+  getClientOffset: function IUI_getClientOffset(aNode)

This looks crazy - can't you just use aNode.getClientBoundingRect()?

>+  _debugTree: function DEBUGTREE()
>+  {
>+    let docEl = this.treeBrowser.contentDocument.documentElement;
>+    this._log(docEl.innerHTML);
>+  },

Remove this?

>diff --git a/browser/themes/gnomestripe/browser/jar.mn b/browser/themes/gnomestripe/browser/jar.mn
>diff --git a/browser/themes/pinstripe/browser/jar.mn b/browser/themes/pinstripe/browser/jar.mn

>+  skin/classic/browser/twistyClosed.png
>+  skin/classic/browser/twistyOpen.png

Rather than add these, you should be able to use "-moz-appearance: treetwistyopen;" on Mac/Linux.

r- just because of the volume of comments - will need to do another pass once these are addressed.

Comment 46

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 469166 [details] [diff] [review]
closeInspectorUI() failure fix

yup.

Comment 47

[Mihai Sucan [:msucan]]

Attachment: Inspector tree panel (rebased 2010-08-26)

Attaching the Inspector tree panel patch rebased on top of the latest mozilla-central default branch. This patch includes closeInspectorUI() fixes, and fixes for the new test code from m-c.

Comment 48

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #43)
> More partial comments:
> 
> >diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
> 
> > <window id="main-window"
> 
> >+        xmlns:html="http://www.w3.org/1999/xhtml"
> 
> This doesn't appear to be needed.

Whups. I forgot to remove that. Yanked.

> >+      <browser id="inspector-tree-browser"
> 
> Can this be type="content"? Might also want disablesecurity="true".

Should work.

> 
> >diff --git a/browser/base/content/inspector.html b/browser/base/content/inspector.html
> 
> >+<head>
> >+  <title>Inspector</title>
> 
> l10n? Or if this isn't used (doesn't appear to be), just remove it.

Removed.

> >diff --git a/browser/themes/gnomestripe/browser/inspector.css b/browser/themes/gnomestripe/browser/inspector.css
> 
> >+/* See license.txt for terms of usage */
> 
> What are these about? I don't see a license.txt anywhere. Are these styles
> copied from somewhere? It seems like there are a lot of unused styles. (all the
> infoTip* stuff, fullPanelEditor, etc.).

These came from Firebug. I stripped out a bunch of them and it looks like I can remove a few more. I'll fix up the license references and try to clean up some of the extraneous selectors.

> >diff --git a/browser/themes/winstripe/browser/jar.mn b/browser/themes/winstripe/browser/jar.mn
> 
> >+        skin/classic/aero/browser/twistyClosed.png
> >+        skin/classic/aero/browser/twistyOpen.png
> 
> Forgot to remove these?

I did! Removing.

Comment 49

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #44)
> You should keep hasClass()/removeClass()/addClass() helpers method rather than
> sprinkling instanceof Element checks all over the place. Are the instanceof
> checks really all needed though? When are object boxes not Elements?

Done this. Converted these methods to simple wrappers around the classList methods.

It turns out that the checks are needed. Not sure if they're textnodes or some other classless element in the DOM but some cases cause failures if the checks aren't included.

Comment 50

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #45)
> Comment on attachment 469061 [details] [diff] [review]
> Inspector tree panel 2010-08-25.2
> 
> >diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js
> 
> >+const RE_NOT_WHITESPACE = /[^\s]/;
> 
> Might as well just put this in isWhiteSpace, rather than at the global level.

done.

> >+/*
> >+ * Begin Firebug license section
> >+ */
> 
> Probably better to put this in a separate file, and preprocessor #include it
> here, given the different-license issue. It would also make the diff much
> easier to review (showing removal+additions seperately, rather than
> interleaved).

Done. Now moved to insideOutBox.js and #included in inspector.js.

> >+//// InsideOutBox
> > 
> > /**
> >+ * InsideOutBoxView is a simple interface definition for views implementing
> >+ * InsideOutBox controls. All implementors must define these methods.
> >+ * Implemented in InspectorUI.
> >  */
> >+InsideOutBoxView = {
> 
> Put this all in a comment, to make it clear that it's an interface definition,
> rather than dead code?

Done.

> >+InsideOutBox = function(aView, aBox)
> 
> function InsideOutBox(aView, aBox)

flipped.

> >+InsideOutBox.prototype =
> 
> >+  openObject: function IOBox_openObject(aObject)
> 
> >+    let objectBox = this.createObjectBox(object);
> >+    this.openObjectBox(objectBox);
> >+    return objectBox;
> 
> this.openToObject(object);

Mm, yeah. Don't forget, I copied this wholesale and only made minor modifications to it. This change makes sense though.

> >   /**
> >+   * Select the given object node in the tree.
> >+   * @param aObject
> >+   *        The object node to select.
> >+   * @param makeBoxVisible
> >+   *        Do we open the objectBox?
> >+   * @param forceOpen
> >+   *        Force the object box open?
> 
> The distinction between "open the objectBox" and "force the object box open" is
> rather confusing!

It is. Very. It's one of the more endearing qualities of the Firebug codebase. I have termed this highly-factored and API-ified code style, "Hewitty". Made more confusing because there were no comments with any of these methods. Hopefully I got the comments right.

> It seems like forceOpen controls whether we call expandObjectBox, which
> controls whether children are visible.

that's right.

> makeBoxVisible controls whether we call openObjectBox, which controls whether
> the we open all of its ancestors.

Right again. Do we expand the ancestors to make the object box in question visible? Would seem to be necessary for both of these to work.

> Consistent terminology throughout wrt "expanded" vs. "open" would be nice... As
> it is things are just horribly confusing.

Agreed. Again though, I cleaned up what I could and left alone what was less obvious. I'm sure there's some refactoring possible with this code, but because a lot of it is highly-recursive, we'd risk breaking it.

> >+  /**
> >+   * Expand the given object in the tree.
> >+   * @param aObject
> >+   *        The tree node to expand.
> >+   */
> 
> "Expands/contracts the given object, depending on its state.", "The tree node
> to expand/contract."

Yes.

> >+  highlightObjectBox: function IOBox_highlightObjectBox(aObjectBox)
> 
> Seems like a generic function for iterating over ancestors would be useful:
> function iterateObjectAncestors(aObject, aCallback) {
>   let object = aObject;
>   while ((object = this.getParentObjectBox(object)))
>     callback(object);
> }
> 
> then this method could do:
> let self = this;
> iterateHighlightedAncestors(this.highlightedObjectBox, function (box) {
>   self.removeClass(box, "highlightOpen");
> });
> iterateHighlightedAncestors(this.highlightedObjectBox, function (box) {
>   self.addClass(box, "highlightOpen");
> });

um, iterateObjectAncestors you mean. And to keep this semantically the same, we need to add some checks around the this.highlightedObjectBox section adn the second section where we set the highlightedObjectBox to be the passed in argument.

But I get your meaning and will update this.

  /**
   * General method for iterating over an object's ancestors and performing
   * some function.
   * @param aObject
   *        The object whose ancestors we wish to iterate over.
   * @param aCallback
   *        The function to call with the object as argument.
   */

  iterateObjectAncestors: function IOBox_iterateObjectAncesors(aObject, aCallback)
  {
    let object = aObject;
    while (object = this.getParentObjectBox(object))
      callback(object);
  },

  /**
   * Highlight the given objectBox in the tree.
   * @param aObjectBox
   *        The objectBox to highlight.
   */
  highlightObjectBox: function IOBox_highlightObjectBox(aObjectBox)
  {
    let self = this;

    if (this.highlightedObjectBox) {
      this.iterateObjectAncestors(this.highlightedObjectBox, function (box) {
        self.view.removeClass(box, "highlightOpen");
      });
    }

    this.highlightedObjectBox = aObjectBox;

    this.iterateObjectAncestors(this.highlightedObjectBox, function (box) {
      self.view.addClass(box, "highlightOpen");
    });

    aObjectBox.scrollIntoView(true);
  },

comme ça? This makes this method a little more concise and readable, I think.

> and other methods could use it too (e.g. openObjectBox())
> (can be a followup)

This might take a bit of extra thinking to unwind. I'm not sure these a11y methods even work in this context as they were bolted on to firebug after-the-fact.

my initial attempt to use the above method didn't work, so I'd prefer a follow-up. Time constraints...

> >+  selectObjectBox: function IOBox_selectObjectBox(aObjectBox, forceOpen)
> 
> >+        // Force it open the first time it is selected
> >+        if (forceOpen)
> >+          this.toggleObjectBox(aObjectBox, true);
> 
> Why not call expandObjectBox() directly?

Good question! Done.

> >+  openObjectBox: function IOBox_openObjectBox(aObjectBox)
> 
> >+    if (aObjectBox) {
> 
> Use an early return instead, to reduce indentation.

Done. I don't know why we'd be passing a null object in there anyway, probably as stop-conditions during recursive calls?

> >+  createObjectBox: function IOBox_createObjectBox(aObject)
> 
> >+    if (!objectBox)
> >+      return null;
> >+    else if (aObject == this.rootObject)
> >+      return objectBox;
> >+    else
> >+      return this.populateChildBox(aObject, objectBox.parentNode);
> 
> remove else-after-return (here and everywhere else - there are a few)

We'll make this make sense or else...

> >+  createObjectBoxes: function IOBox_createObjectBoxes(aObject, aRootObject)
> 
> >+  findObjectBox: function IOBox_findObjectBox(aObject)
> 
> >+  populateChildBox: function IOBox_populateChildBox(repObject, nodeChildBox)
> 
> I find the logic in these really hard to follow - the whole setup (with
> objects/object boxes etc.) is confusing. I know this mostly isn't your code,
> though, and it seems to work OK, I guess, so I suppose I won't stress about it
> too much...

It is confusing. You've probably gleaned that "objects" are the actual DOM nodes and attributes in the originating HTML document we're inspecting and "objectBoxes" are the IOBox nodes that represent them. Yeah, I'm not super keen on these terms either, but they make sense once you've played with them some.

> >+  getChildObjectBox: function IOBox_getChildObjectBox(aObjectBox)
> 
> >+    return aObjectBox.getElementsByClassName("nodeChildBox").item(0);
> 
> objectBox.querySelector(".nodeChildBox")?

nice. Replaced a bunch of these.

> >+  getRootNode: function IOBox_getRootNode(aNode)

(You should've seen this before I reordered it)

> How about:
> 
> let node = aNode;
> let tmpNode;
> while ((tmpNode = this.view.getParentObject(node)))
>   node = tmpNode;
> 
> return node;

Yes, I think that works. Succinct!

> > var InspectorUI = {
> 
> >+  noScrollIntoView: false,
> 
> Appears to be unused.

Must be a leftover.

> 
> >+  getOwnerPanel: function IUI_getOwnerPanel(aNode)
> >+  {
> >+    for (; node; node = node.parentNode) {
> >+      if (node.ownerPanel)
> >+        return node.ownerPanel;
> >+    }
> 
> undeclared variable "node" - but it turns out this is unused, so we can remove
> it!

Removed!

> >+  getParentObject: function IUI_getParentObject(node)
> 
> Else-after-returns in this function are deadly.

I hate this method. Let's fix it!

> 
> >+    } else {
> >+      // Documents have no parentNode; Attr, Document, DocumentFragment, Entity,
> >+      // and Notation. top level windows have no parentNode
> >+      if (node && node.nodeType == 9) {
> 
> Node.DOCUMENT_NODE

fixed.

> >+        if (node.defaultView) {
> >+          // generally a reference to the window object for the document,
> >+          // however that is not defined in the specification
> 
> remove this comment, specification isn't relevant here (defaultView is
> garanteed to be the window)

OK

> 
> >+          let embeddingFrame = node.defaultView.frameElement;
> >+          if (embeddingFrame)
> >+            return embeddingFrame.parentNode;
> 
> Why return the frame's parent rather than the frame itself (as in the case
> above)?

Not sure. I could try it and see, but I know this works.

I flipped these around a bit and got rid of the elses I could. WE could probably clean this up a bit more with some effort, but the braces are starting to wobble under their own power right now so I'm going to leave it as-is.

> >+        } else // a Document object without a parentNode or window
> >+          return null;  // top level has no parent
> >+      }
> >     }
> 
> >+  getChildObject: function IUI_getChildObject(node, index, previousSibling)
> >+  {
> >+    if (!node)
> >+      return;
> 
> return null?

sure.

> 
> >+    /* if (this.isSourceElement(node)) {
> >+      if (index == 0)
> >+        return this.getElementSourceText(node);
> >+      else
> >+        return null;  // no siblings of source elements
> >+    } else */
> 
> Either remove or uncomment.

Let's remove it!

> 
> >+    } else if (node.getSVGDocument && node.getSVGDocument()) {
> 
> (node instanceof GetSVGDocument) (what a weird interface name...)

kinda. It's SVG, man!

> >+  getFirstChild: function IUI_getFirstChild(node)
> 
> How does this differ from node.firstChild?

Good question. getFirstChild and getNextSibling are both using a treeWalker. I'm not sure we couldn't replace those with direct calls to firstChild and nextSibling. Pretty sure this code came into being before those were added to the DOM.

Then again, maybe not. Replacing these with nextSibling and firstChild didn't work.

> >+  getNextSibling: function IUI_getNextSibling(node)
> 
> And this from node.nextSibling?

nope. There's more going on here. They're like Transformers.

You'll note that the instance variable this.treeWalker lives around for awhile. These methods appear to be reusing it for their own nefarious purposes.

> >   openInspectorUI: function IUI_openInspectorUI()
> 
> >+    if (!this.domplate) {
> >+      Cu.import("resource:///modules/domplate.jsm", this);
> >+      let dom = Cu.getGlobalForObject(Node);
> >+      this.domplateUtils.setDOM(dom);
> >+    }
> 
> This still hasn't been addressed from my last comments (first part of comment
> 31).

Oh, sorry. I read that, nodded along with it and then promptly forgot to do it.

> 
> >   closeInspectorUI: function IUI_closeInspectorUI()
> 
> >+    this.inspectCmd.setAttribute("checked", "false");
> 
> This line already exists below.

That's been removed.

> >+  select: function IUI_select(aNode, forceUpdate)
> 
> >+      let box = this.ioBox.createObjectBox(this.selection);
> >+        box.scrollIntoView(true); // todo scrollIntoCenterView would be nicer
> 
> ioBox.select (called via updateSelection) seems to handle this, do you really
> need to do them pre-emptively?

ok, I added an extra argument and updated the callers.

> >+      this.updateSelection(this.selection);
> 
> >+  updateSelection: function IUI_updateSelection(aNode)
> >+  {
> >+    this.ioBox.select(aNode, true, true);
> 
> Might as well just inline this...

Not sure why that got split out.

> >   /**
> >+   * Get the repObject from the HTML panel's domplate-constructed DOM node.
> 
> What does "repObject" represent? Worth adding a comment.

As near as I can tell, it stands for...

The Object being Represented (by the Box Object).

> 
> >+  isWhitespaceText: function IUI_isWhitespaceText(node)
> >+  {
> >+    if (node instanceof HTMLAppletElement)
> >+      return false;
> 
> What's this for? Seems pretty random...

Pshew. I don't know. Maybe inspecting pages with applets on them return things that look like text nodes. In any case, I've removed it.

> >+  getClientOffset: function IUI_getClientOffset(aNode)
> 
> This looks crazy - can't you just use aNode.getClientBoundingRect()?

This is only used by the scrollIntoCenterView method which is currently unsent. These don't work properly so I'm going to pull them out until we're ready to write ones that work.

> 
> >+  _debugTree: function DEBUGTREE()
> >+  {
> >+    let docEl = this.treeBrowser.contentDocument.documentElement;
> >+    this._log(docEl.innerHTML);
> >+  },
> 
> Remove this?

sure thing.
> 
> >diff --git a/browser/themes/gnomestripe/browser/jar.mn b/browser/themes/gnomestripe/browser/jar.mn
> >diff --git a/browser/themes/pinstripe/browser/jar.mn b/browser/themes/pinstripe/browser/jar.mn
> 
> >+  skin/classic/browser/twistyClosed.png
> >+  skin/classic/browser/twistyOpen.png
> 
> Rather than add these, you should be able to use "-moz-appearance:
> treetwistyopen;" on Mac/Linux.

Ah, good. I switched over to those and removed the pngs and references to them in the jar.mns. I also pulled out a little excess selector goop around the twisty parts of the inspector.css files. Works well.

> r- just because of the volume of comments - will need to do another pass once
> these are addressed.

of course. I need to rerun the unittests with this latest patch, but I'm pretty much done for the day. Massive, massive review. Thanks so much.

Comment 51

[Rob Campbell [:rc] (:robcee)]

Attachment: Inspector tree panel 2010-08-26 - post review

Comment 52

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 469667 [details] [diff] [review]
Inspector tree panel 2010-08-26 - post review

>diff --git a/browser/base/content/domplate.jsm b/browser/base/content/domplate.jsm

> HTMLTemplates.HTMLHtmlElement = domplate(BaseTemplates.Element,
> {
>   tag:
>     domplate.DIV({"class":
>         "nodeBox htmlNodeBox containerNodeBox $object|getHidden repIgnore",
>         _repObject: "$object", role: "presentation"},
>-      domplate.DIV({"class": "docType $object"},
>+      domplate.DIV({"class": "docType"},

What's this change about?

>diff --git a/browser/base/content/insideOutBox.js b/browser/base/content/insideOutBox.js

>+InsideOutBox.prototype =

>+  openObject: function IOBox_openObject(aObject)
>+  {
>+    let object = aObject;
>+    let firstChild = this.view.getChildObject(object, 0);
>+    if (firstChild)
>+      object = firstChild;
>+
>+    let objectBox = this.createObjectBox(object);
>+    this.openObjectBox(objectBox);
>+    return objectBox;

this.openToObject(object);

>+   * @param makeBoxVisible
>+   *        Do we open the objectBox?
>+   * @param forceOpen
>+   *        Force the object box open?

Can you revise these as I summarized in the previous review (i.e. mention expanding children vs. ancestors)

>+  iterateObjectAncestors: function IOBox_iterateObjectAncesors(aObject, aCallback)
>+  {
>+    let object = aObject;
>+    while ((object = this.getParentObjectBox(object)))
>+      callback(object);

aCallback? We should have a test that would have caught this.

>+  openObjectBox: function IOBox_openObjectBox(aObjectBox)

>+    let parentBox = this.getParentObjectBox(aObjectBox);
>+    let labelBox;
>+    while (parentBox) {
>+      this.view.addClass(parentBox, "open");
>+      labelBox = parentBox.querySelector(".nodeLabelBox");
>+      if (labelBox)
>+        labelBox.setAttribute('aria-expanded', 'true');
>+      parentBox = this.getParentObjectBox(parentBox);
>+    }

let self = this;
this.iterateObjectAncestors(aObjectBox, function (box) {
  self.view.addClass(box, "open");
  let labelBox = box.querySelector(".nodeLabelBox");
  if (labelBox)
    labelBox.setAttribute("aria-expanded", "true");
});

should work...

>+  expandObjectBox: function IOBox_expandObjectBox(aObjectBox)

>+      labelBox.setAttribute('aria-expanded', 'true');

>+  contractObjectBox: function IOBox_contractObjectBox(aObjectBox)

>+      labelBox.setAttribute('aria-expanded', 'false');

nit: use " rather than '

>+  findChildObjectBox: function IOBox_findChildObjectBox(aParentNodeBox, aRepObject)
>+  {
>+    let childBox = aParentNodeBox.firstChild;
>+    while (childBox) {
>+      if (childBox.repObject == aRepObject)
>+        return childBox;
>+      childBox = childBox.nextSibling;
>+    }
>+    return null; // not found

let foundNode = null;
Array.some(aParentNodeBox.childNodes, function (node) {
  foundNode = node.repObject == aRepObject && node;
});
return foundNode;

but maybe that's too hard to understand.

>diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js

>+  getFirstChild: function IUI_getFirstChild(node)
>+  getNextSibling: function IUI_getNextSibling(node)

I read the treeWalker spec, and I don't see how these are any different than just using nextSibling and firstChild properties. File a followup to investigate?

>+  getChildObject: function IUI_getChildObject(node, index, previousSibling)

>+    if (node.getSVGDocument && node.getSVGDocument()) {

(instanceof GetSVGDocument) didn't work?

>   openInspectorUI: function IUI_openInspectorUI()

>+    if (!this.domplate) {
>+      Cu.import("resource:///modules/domplate.jsm", this);
>+      this.domplateUtils.setDOM(window);
>+    }

Does this actually want this.browser.contentWindow?

>   closeInspectorUI: function IUI_closeInspectorUI(aClearStore)
>   {
>+    if (this.closing || !this.win || !this.browser) {
>+      return;
>+    }
>+
>+    this.closing = true;

Why was this needed? What cases do the win and browse checks catch? Scares me a bit that this would be re-entrant...

>+  isWhitespace: function IUI_isWhitespace(text)
>+  isWhitespaceText: function IUI_isWhitespaceText(node)

These seem to be duplicated in domplate, can they be exposed there instead?

>diff --git a/browser/base/content/test/browser_inspector_domPanel.js b/browser/base/content/test/browser_inspector_domPanel.js

> function nodeGenerator()

>-  let closing = doc.getElementById("#closing");
>+  let closing = doc.getElementById("closing");
>   InspectorUI.inspectNode(closing);

Huh, why didn't this cause a test failure?

>diff --git a/browser/base/content/test/browser_inspector_treePanel_output.js b/browser/base/content/test/browser_inspector_treePanel_output.js

How was the output file for this test generated?

>diff --git a/browser/themes/gnomestripe/browser/inspector.css b/browser/themes/gnomestripe/browser/inspector.css

>+.jumpHighlight {
>+.collapsed,
>+[collapsed="true"] {
>+.obscured {
>+.measureBox {
>+.panelNode {
>+.panelNode[active="true"] {
>+h1.groupHeader {
>+.fullPanelEditor {
>+.offScreen {
>+.memberRow.hasChildren > .memberLabelCell > .memberLabel,
>+.hasHeaders .netHrefLabel {

All of these look unused (applies to all three copies), so should be removed.

>+/* skin/os/panel.css */

This reference and others like it (including empty comments?) in these files don't seem useful.

>+.useA11y

All of these too.

>+html {
>+  background-color: #f3f3f3;
>+}

This seems to override the same selector at the top of the file.

>+body {
>+  border-top: 1px solid #BBB9BA;
>+}

This should be merged with the selector block at the top of the file.

It looks like there is a lot of CSS duplication in the earlier styles. We should get a bug on file to review and clean up all of it.

Comment 53

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #52)
> Comment on attachment 469667 [details] [diff] [review]
> Inspector tree panel 2010-08-26 - post review
> 
> >diff --git a/browser/base/content/domplate.jsm b/browser/base/content/domplate.jsm
> 
> > HTMLTemplates.HTMLHtmlElement = domplate(BaseTemplates.Element,
> > {
> >   tag:
> >     domplate.DIV({"class":
> >         "nodeBox htmlNodeBox containerNodeBox $object|getHidden repIgnore",
> >         _repObject: "$object", role: "presentation"},
> >-      domplate.DIV({"class": "docType $object"},
> >+      domplate.DIV({"class": "docType"},
> 
> What's this change about?

Mihai did that through the course of writing the unittest for the tree.

He discovered there was an unused object parameter in this template so he removed it. Simpler is better, right?

> >diff --git a/browser/base/content/insideOutBox.js b/browser/base/content/insideOutBox.js
> 
> >+InsideOutBox.prototype =
> 
> >+  openObject: function IOBox_openObject(aObject)
> >+  {
> >+    let object = aObject;
> >+    let firstChild = this.view.getChildObject(object, 0);
> >+    if (firstChild)
> >+      object = firstChild;
> >+
> >+    let objectBox = this.createObjectBox(object);
> >+    this.openObjectBox(objectBox);
> >+    return objectBox;
> 
> this.openToObject(object);

I tried that and it didn't work. You can't mix (rep)objects and objectBoxes.

These two methods openObject and openToObject do different things. The first opens the object's box itself. The second one opens the tree of boxes upTo the given object.

> >+   * @param makeBoxVisible
> >+   *        Do we open the objectBox?
> >+   * @param forceOpen
> >+   *        Force the object box open?
> 
> Can you revise these as I summarized in the previous review (i.e. mention
> expanding children vs. ancestors)

I've tried to explain them a bit better. Truth is, there are so many of these methods doing subtly different things that it's difficult to do.

> >+  iterateObjectAncestors: function IOBox_iterateObjectAncesors(aObject, aCallback)
> >+  {
> >+    let object = aObject;
> >+    while ((object = this.getParentObjectBox(object)))
> >+      callback(object);
> 
> aCallback? We should have a test that would have caught this.

Nuts. That explains why my attempt to use that in openObjectBox didn't work last night. Tired eyes.

Had we been using it for openObjectBox, the unit test for treePanel_output would've caught it as there wouldn't have been a complete tree created.

I added a check in the method to check that aCallback is set to something and fixed the typo.

> >+  openObjectBox: function IOBox_openObjectBox(aObjectBox)
> 
> >+    let parentBox = this.getParentObjectBox(aObjectBox);
> >+    let labelBox;
> >+    while (parentBox) {
> >+      this.view.addClass(parentBox, "open");
> >+      labelBox = parentBox.querySelector(".nodeLabelBox");
> >+      if (labelBox)
> >+        labelBox.setAttribute('aria-expanded', 'true');
> >+      parentBox = this.getParentObjectBox(parentBox);
> >+    }
> 
> let self = this;
> this.iterateObjectAncestors(aObjectBox, function (box) {
>   self.view.addClass(box, "open");
>   let labelBox = box.querySelector(".nodeLabelBox");
>   if (labelBox)
>     labelBox.setAttribute("aria-expanded", "true");
> });
> 
> should work...

it should work now that we fixed iterateObjectAncestors. And indeed it does.

> >+  expandObjectBox: function IOBox_expandObjectBox(aObjectBox)
> 
> >+      labelBox.setAttribute('aria-expanded', 'true');
> 
> >+  contractObjectBox: function IOBox_contractObjectBox(aObjectBox)
> 
> >+      labelBox.setAttribute('aria-expanded', 'false');
> 
> nit: use " rather than '

OK.

> >+  findChildObjectBox: function IOBox_findChildObjectBox(aParentNodeBox, aRepObject)
> >+  {
> >+    let childBox = aParentNodeBox.firstChild;
> >+    while (childBox) {
> >+      if (childBox.repObject == aRepObject)
> >+        return childBox;
> >+      childBox = childBox.nextSibling;
> >+    }
> >+    return null; // not found
> 
> let foundNode = null;
> Array.some(aParentNodeBox.childNodes, function (node) {
>   foundNode = node.repObject == aRepObject && node;
> });
> return foundNode;
> 
> but maybe that's too hard to understand.

Yeah, I find that a bit dense to look at. The current method is at least legible.

> >diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js
> 
> >+  getFirstChild: function IUI_getFirstChild(node)
> >+  getNextSibling: function IUI_getNextSibling(node)
> 
> I read the treeWalker spec, and I don't see how these are any different than
> just using nextSibling and firstChild properties. File a followup to
> investigate?
> 
> >+  getChildObject: function IUI_getChildObject(node, index, previousSibling)
> 
> >+    if (node.getSVGDocument && node.getSVGDocument()) {
> 
> (instanceof GetSVGDocument) didn't work?

Oh, sorry, I didn't understand what you were suggesting there. That should work, yes. Replaced.

> 
> >   openInspectorUI: function IUI_openInspectorUI()
> 
> >+    if (!this.domplate) {
> >+      Cu.import("resource:///modules/domplate.jsm", this);
> >+      this.domplateUtils.setDOM(window);
> >+    }
> 
> Does this actually want this.browser.contentWindow?

Any global with visibility into the DOM classes is sufficient here. It's only used for doing things like looking at Node.CONSTANTs and instanceof Element, etc.

> >   closeInspectorUI: function IUI_closeInspectorUI(aClearStore)
> >   {
> >+    if (this.closing || !this.win || !this.browser) {
> >+      return;
> >+    }
> >+
> >+    this.closing = true;
> 
> Why was this needed? What cases do the win and browse checks catch? Scares me a
> bit that this would be re-entrant...

It ended up being a problem with our unittests and afaict, that's the only place it should be an issue. The extra checks don't hurt in case someone manages to close the UI from two locations before things get cleaned up. Should be hard, if not impossible for a human to do. Maybe easier from code or extensions.

> >+  isWhitespace: function IUI_isWhitespace(text)
> >+  isWhitespaceText: function IUI_isWhitespaceText(node)
> 
> These seem to be duplicated in domplate, can they be exposed there instead?

Ah yes, I think so. I missed them when I was moving the helper functions around. Moved them to domplateUtils in domplate.jsm.

> >diff --git a/browser/base/content/test/browser_inspector_domPanel.js b/browser/base/content/test/browser_inspector_domPanel.js
> 
> > function nodeGenerator()
> 
> >-  let closing = doc.getElementById("#closing");
> >+  let closing = doc.getElementById("closing");
> >   InspectorUI.inspectNode(closing);
> 
> Huh, why didn't this cause a test failure?

I'm not sure how long that'd been in there like that. Probably hadn't been run as part of this patch for awhile. I've started using a script to run all the inspector unittests and would definitely catch it now.

> >diff --git a/browser/base/content/test/browser_inspector_treePanel_output.js b/browser/base/content/test/browser_inspector_treePanel_output.js
> 
> How was the output file for this test generated?

InspectorUI.treePanelDiv.innerHTML >> file.

We know it works too. Since changing to the classList API, there were some whitespace differences in the generated HTML in the tree that this test caught. Props to Mihai for setting it up!

And in the future, if we need to update the test, it's a simple matter to grab the contents using this test which spits out errors on the console.

> >diff --git a/browser/themes/gnomestripe/browser/inspector.css b/browser/themes/gnomestripe/browser/inspector.css
> 
> >+.jumpHighlight {
> >+.collapsed,
> >+[collapsed="true"] {
> >+.obscured {
> >+.measureBox {
> >+.panelNode {
> >+.panelNode[active="true"] {
> >+h1.groupHeader {
> >+.fullPanelEditor {
> >+.offScreen {
> >+.memberRow.hasChildren > .memberLabelCell > .memberLabel,
> >+.hasHeaders .netHrefLabel {
> 
> All of these look unused (applies to all three copies), so should be removed.

I've been paring down the CSS. I'll nip these out too.

> 
> >+/* skin/os/panel.css */
> 
> This reference and others like it (including empty comments?) in these files
> don't seem useful.

They were useful when I was porting this code over. The CSS was originally spread across 5 CSS files and overridden for each OS. We're getting to the yummy good stuff now though! There's a lot less than there was.

> >+.useA11y
> 
> All of these too.

OK.

> >+html {
> >+  background-color: #f3f3f3;
> >+}
> 
> This seems to override the same selector at the top of the file.

You're right. Remember where I said these all had overrides nestled in OS-specific directories? Ditching...

> >+body {
> >+  border-top: 1px solid #BBB9BA;
> >+}
> 
> This should be merged with the selector block at the top of the file.
> 
> It looks like there is a lot of CSS duplication in the earlier styles. We
> should get a bug on file to review and clean up all of it.

Absolutely.

Comment 54

[Rob Campbell [:rc] (:robcee)]

Attachment: Inspector tree panel 2010-08-27

Comment 55

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #53)
> (In reply to comment #52)
> > It looks like there is a lot of CSS duplication in the earlier styles. We
> > should get a bug on file to review and clean up all of it.
> 
> Absolutely.

Created https://bugzilla.mozilla.org/show_bug.cgi?id=591466

Comment 56

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #53)
> > >+    let objectBox = this.createObjectBox(object);
> > >+    this.openObjectBox(objectBox);
> > >+    return objectBox;
> > 
> > this.openToObject(object);
> 
> I tried that and it didn't work. You can't mix (rep)objects and objectBoxes.

openToObject contains exactly the same three lines quote above - I was just suggesting replacing those three lines with a call to openToObject.

> It ended up being a problem with our unittests and afaict, that's the only
> place it should be an issue. The extra checks don't hurt in case someone
> manages to close the UI from two locations before things get cleaned up.

I'd kind of like to understand exactly how this could be re-entrant. I didn't review the InspectorStore stuff, so maybe I'm missing something. File a followup, I guess?

> I'm not sure how long that'd been in there like that. Probably hadn't been run
> as part of this patch for awhile. I've started using a script to run all the
> inspector unittests and would definitely catch it now.

Isn't that test checked in and running on tinderbox, though? Without looking into it at all, it seems like it should be failing...

Comment 57

[Mihai Sucan [:msucan]]

(In reply to comment #52)
> Comment on attachment 469667 [details] [diff] [review]
> Inspector tree panel 2010-08-26 - post review
> 
> >diff --git a/browser/base/content/domplate.jsm b/browser/base/content/domplate.jsm
> 
> > HTMLTemplates.HTMLHtmlElement = domplate(BaseTemplates.Element,
> > {
> >   tag:
> >     domplate.DIV({"class":
> >         "nodeBox htmlNodeBox containerNodeBox $object|getHidden repIgnore",
> >         _repObject: "$object", role: "presentation"},
> >-      domplate.DIV({"class": "docType $object"},
> >+      domplate.DIV({"class": "docType"},
> 
> What's this change about?

This change comes because the generated domplate output was something like:

<div class="docType $object.toString()">
&lt;!DOCTYPE html&gt;</div>

where $object was the DOCTYPE node. Thus $object.toString() resulted in something like:

<div class="docType [XPCNativeWrapper [HtmlDocTypeElement ... ]]">
&lt;!DOCTYPE html&gt;</div>

(or something like that - I don't remember the *exact* toString() result)

That was a bug in the domplate template for doctype nodes that needed a fix. That's the fix I chose.


> >   closeInspectorUI: function IUI_closeInspectorUI(aClearStore)
> >   {
> >+    if (this.closing || !this.win || !this.browser) {
> >+      return;
> >+    }
> >+
> >+    this.closing = true;
> 
> Why was this needed? What cases do the win and browse checks catch? Scares me a
> bit that this would be re-entrant...

Robert added onpopuphiding="InspectorUI.closeInspectorUI()" to the #inspector-tree-panel xul:panel element in browser.xul.

Tests code all invoked InspectorUI.closeInspectorUI() - and other functions do that, even inside the Inspector code.

The closeInspectorUI() in turn called hidePopup() for the #inspector-tree-panel. This resulted in the closeInspectorUI() method being called twice, and the second time it has always thrown an exception - some missing objects.

The quick fix I did is to use a flag for the closeInspectorUI() method that tells it's closing.


> > function nodeGenerator()
> 
> >-  let closing = doc.getElementById("#closing");
> >+  let closing = doc.getElementById("closing");
> >   InspectorUI.inspectNode(closing);
> 
> Huh, why didn't this cause a test failure?

Because nobody complained about an non-existing node to inspect. We could make inspectNode() to throw, or maybe simply deselect any currently-selected node.


> >diff --git a/browser/base/content/test/browser_inspector_treePanel_output.js b/browser/base/content/test/browser_inspector_treePanel_output.js
> 
> How was the output file for this test generated?

As Robert said: treePanelDiv.innerHTML. The test code spits out both the innerHTML result when the expected result is wrong.


That's all. I wanted to clarify the things I did. :)

Comment 58

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #56)
> (In reply to comment #53)
> > > >+    let objectBox = this.createObjectBox(object);
> > > >+    this.openObjectBox(objectBox);
> > > >+    return objectBox;
> > > 
> > > this.openToObject(object);
> > 
> > I tried that and it didn't work. You can't mix (rep)objects and objectBoxes.
> 
> openToObject contains exactly the same three lines quote above - I was just
> suggesting replacing those three lines with a call to openToObject.

Ah, I understand. Sorry. Added it.

> 
> > It ended up being a problem with our unittests and afaict, that's the only
> > place it should be an issue. The extra checks don't hurt in case someone
> > manages to close the UI from two locations before things get cleaned up.
> 
> I'd kind of like to understand exactly how this could be re-entrant. I didn't
> review the InspectorStore stuff, so maybe I'm missing something. File a
> followup, I guess?

We were calling InspectorUI.closeInspectorUI() from our test code. That causes the HTML panel to close which in turn calls closeInspectorUI() from the popuphiding listener.

The current isClosing check should be sufficient, no?

Follow-up for a unittest?

> > I'm not sure how long that'd been in there like that. Probably hadn't been run
> > as part of this patch for awhile. I've started using a script to run all the
> > inspector unittests and would definitely catch it now.
> 
> Isn't that test checked in and running on tinderbox, though? Without looking
> into it at all, it seems like it should be failing...

oh, yeah, it is. I'd have to see what's happening in that test to figure out why it's not failing like we'd expect. The fix here fixes it though.

Comment 59

[Rob Campbell [:rc] (:robcee)]

Attachment: Inspector tree panel 2010-08-27.2

added openObject() fix.

Comment 60

[Rob Campbell [:rc] (:robcee)]

Adjustments made to the CSS in winstripe are causing the twisties to not show up at all there. I'm still tweaking this, but we might need a followup bug to get them working properly if I can't fix this in the next 20 minutes or so.

Comment 61

[Rob Campbell [:rc] (:robcee)]

Attachment: [backed-out] Inspector tree panel 2010-08-27.3

Fixed a bug in the close from titlebar button that would cause the inspector to reopen if switched to a new tab after closing.

Comment 62

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 470079 [details] [diff] [review]
[backed-out] Inspector tree panel 2010-08-27.3

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>+      <browser id="inspector-tree-browser"
>+               flex="1"
>+               type="content"
>+               disablesecurity="true"
>+               src="chrome://browser/content/inspector.html"
>+               onclick="InspectorUI.onTreeClick(event);"
>+               disablehistory="true" />

Dolske mentions in bug 582596 comment 78 that perhaps an iframe would do the trick (it's certainly lighter weight) - followup?

>diff --git a/browser/base/content/insideOutBox.js b/browser/base/content/insideOutBox.js

>+InsideOutBoxView = {

I suppose you should add the has/add/removeClass helpers to this? Don't need to be verbose with the comment, one for the three of them should be fine :)

>diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js

>+  getFirstChild: function IUI_getFirstChild(node)
>+  getNextSibling: function IUI_getNextSibling(node)

Don't forget the followup for these.

>   closeInspectorUI: function IUI_closeInspectorUI(aClearStore)
>   {
>+    if (this.closing || !this.win || !this.browser) {
>+      return;
>+    }

And on maybe cleaning this up (or at least being able to remove the win/browser checks)?

Comment 63

[:Gavin Sharp [email: [e-mail]]]

I'm seeing:
Error: repObject is not defined
Source File: chrome://browser/content/browser.js
Line: 2266

Looks like a typo in getRepObject:
      if (child.repObject) {
        if (!target && this.hasClass(repObject, "repIgnore"))

(should be child.repObject?)

Comment 64

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 463349 [details] [diff] [review]
domplate

I'm going to rs this for the sake of getting tree panel in, but I'd really like for us to be sure to followup in bug 591580, and to eventually get this properly reviewed if it ends up staying. It seems like there's more here than is strictly necessary, and this is a lot of code to be dumping in.

Comment 65

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 470079 [details] [diff] [review]
[backed-out] Inspector tree panel 2010-08-27.3

http://hg.mozilla.org/mozilla-central/rev/9cff67c76d75

with minor changes from last comment

Comment 66

[Kevin Dangoor]

This was backed out due to leaks.

Comment 67

[Rob Campbell [:rc] (:robcee)]

example log here:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1283014956.1283016668.20874.gz&fulltext=1

Doing some experiments to track these down.

Comment 68

[Mihai Sucan [:msucan]]

Attachment: fix memory leaks

Attempt to fix the memory leaks.

With this patch, on my system I can run all tests with no memory leak detected. Also, I see no regressions caused by the changes I did.

Comment 69

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 470750 [details] [diff] [review]
fix memory leaks

no leaks!

Comment 70

[Rob Campbell [:rc] (:robcee)]

Attachment: [backed-out] Inspector tree panel (leaky, -r 51696 on mozilla-central)

review carried forward from previous attachment. This is the version that was checked in on Aug 28, @r 51696, mozilla-central.

Comment 71

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 470750 [details] [diff] [review]
fix memory leaks

>diff --git a/browser/base/content/domplate.jsm b/browser/base/content/domplate.jsm

>-    domplate.FOR("char", "$object|getNodeTextGroups",
>-      domplate.SPAN({"class": "$char.class $char.extra"},
>-        "$char.str")));
>+    domplate.FOR("chr", "$object|getNodeTextGroups",
>+      domplate.SPAN({"class": "$chr.class $chr.extra"},
>+        "$chr.str")));

What's this change about?

Comment 72

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 470764 [details] [diff] [review]
[backed-out] Inspector tree panel (leaky, -r 51696 on mozilla-central)

http://hg.mozilla.org/mozilla-central/rev/9d7e7ac57f10

(folded with leak fix)

Comment 73

[Rob Campbell [:rc] (:robcee)]

may be responsible for txul regression. See bug 592525.

Comment 74

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 470764 [details] [diff] [review]
[backed-out] Inspector tree panel (leaky, -r 51696 on mozilla-central)

http://hg.mozilla.org/mozilla-central/rev/19f441a1c262

Comment 75

[Rob Campbell [:rc] (:robcee)]

Attachment: Inspector tree panel iframe

Conversion from <browser> to <iframe>. Fixed up unittests and some InspectorStore logic that didn't seem to fit with the new initialization code.

Hoping this nails the txul regression from bug 592525.

Comment 76

[Rob Campbell [:rc] (:robcee)]

Attachment: Inspector tree panel iframe 2

Based upon feedback from mihai, restored the observer notifications and test code.

Comment 77

[Joe Walker [:jwalker]]

Patch application instructions:

The backed-out panel patch includes the domplate patch, and some minor breakage. Talking to Mihai, the iframe patch needs work, and shouldn't be used just yet.

So the patch application order is:
1. "[backed-out] Inspector tree panel (leaky, -r 51696 on mozilla-central)"
2. "fix memory leaks"

Or if you are using hg mq:
$ hg qimport 'https://bugzilla.mozilla.org/attachment.cgi?id=470764' -n bug572038-patch470764-panel
$ hg qpush
  (the reject can be ignored - inspector.css is already there)
$ rm browser/themes/winstripe/browser/jar.mn.rej
$ hg qrefresh


$ hg qimport 'https://bugzilla.mozilla.org/attachment.cgi?id=470750' -n bug572038-patch470750-leak
$ hg qpush

At least, I think that's right. Flames accepted if I got that wrong.

Comment 78

[Rob Campbell [:rc] (:robcee)]

txul results from latest try patch (with iframes):

http://graphs.mozilla.org/#tests=[[17,23,422],[17,23,395],[17,23,724],[17,23,377],[17,23,367]]&sel=1281689335,1283420691

Comment 79

[Rob Campbell [:rc] (:robcee)]

joe, I think that's right.

Comment 80

[Rob Campbell [:rc] (:robcee)]

Attachment: Inspector tree panel iframe 4

Comment 81

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 471501 [details] [diff] [review]
Inspector tree panel iframe 4

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>+      <iframe id="inspector-tree-browser"
>                flex="1"
>-               type="content"

leave this?

>diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js

>-    if (!InspectorStore.hasID(this.winID) ||
>-      InspectorStore.getValue(this.winID, "inspecting")) {
>+    // Setup the InspectorStore or restore state
>+    this.initializeStore();
>+
>+    if (InspectorStore.getValue(this.winID, "inspecting"))
>       this.startInspecting();
>-    }
> 
>     this.win.document.addEventListener("scroll", this, false);
>     this.win.addEventListener("resize", this, false);
>     this.inspectCmd.setAttribute("checked", true);
>-
>-    if (InspectorStore.isEmpty()) {
>-      gBrowser.tabContainer.addEventListener("TabSelect", this, false);
>-    }
>+    document.addEventListener("popupshown", this, false);
>   },

>   /**
>+   * Initialize the InspectorStore.
>+   */
>+  initializeStore: function IUI_initializeStore()
>+  {
>+    // First time opened, add the TabSelect listener
>+    if (InspectorStore.isEmpty())
>+      gBrowser.tabContainer.addEventListener("TabSelect", this, false);
>+
>+    // Has this windowID been inspected before?
>+    if (InspectorStore.hasID(this.winID)) {
>+      let selectedNode = InspectorStore.getValue(this.winID, "selectedNode");
>+      if (selectedNode) {
>+        this.inspectNode(selectedNode);
>+      }
>+    } else {
>+      // First time inspecting, set state to no selection + live inspection.
>+      InspectorStore.addStore(this.winID);
>+      InspectorStore.setValue(this.winID, "selectedNode", null);
>+      InspectorStore.setValue(this.winID, "inspecting", true);
>+      this.win.addEventListener("pagehide", this, true);
>+    }
>+  },

>   handleEvent: function IUI_handleEvent(event)
>   {
>     let winID = null;
>     let win = null;
>     let inspectorClosed = false;
> 
>     switch (event.type) {
>+      case "popupshown":
>+        if (event.target.id == "inspector-tree-panel" ||
>+            event.target.id == "inspector-style-panel" ||
>+            event.target.id == "inspector-dom-panel")
>+          if (this.isTreePanelOpen && this.isStylePanelOpen && this.isDOMPanelOpen) {
>+            document.removeEventListener("popupshowing", this, false);
>+            Services.obs.notifyObservers(null, "inspector-opened", null);
>+          }
>+        break;
>       case "TabSelect":
>         winID = this.getWindowID(gBrowser.selectedBrowser.contentWindow);
>         if (this.isTreePanelOpen && winID != this.winID) {
>           this.closeInspectorUI(false);
>           inspectorClosed = true;
>         }

>   getValue: function IS_getValue(aID, aKey)
>   {
>-    return aID in this.store ? this.store[aID][aKey] : null;
>+    if (!this.hasID(aID))
>+      return null;
>+    if (aKey in this.store[aID])
>+      return this.store[aID][aKey];
>+    return null;
>   },

I find these changes all rather confusing (not sure what they have to do with switching from a browser to an iframe). Can you elaborate on why they're needed? Can someone more familiar with the InspectorStore sign off on them?

Comment 82

[Mihai Sucan [:msucan]]

(In reply to comment #81)
> Comment on attachment 471501 [details] [diff] [review]
> Inspector tree panel iframe 4
> 
> >diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
> 
> >+      <iframe id="inspector-tree-browser"
> >                flex="1"
> >-               type="content"
> 
> leave this?

Not an expert on this, but I presume we want full chrome privileges inside our panels. If it works without, sure, less privileges, less possible security issues.

> [...]
> >   getValue: function IS_getValue(aID, aKey)
> >   {
> >-    return aID in this.store ? this.store[aID][aKey] : null;
> >+    if (!this.hasID(aID))
> >+      return null;
> >+    if (aKey in this.store[aID])
> >+      return this.store[aID][aKey];
> >+    return null;
> >   },
> 
> I find these changes all rather confusing (not sure what they have to do with
> switching from a browser to an iframe). Can you elaborate on why they're
> needed? Can someone more familiar with the InspectorStore sign off on them?

I wrote the code for InspectorStore and I can sign off on the changes. I discussed these changes with Robert. I haven't tested the code myself - I presume Robert did. The patch looks fine to me.

Comment 83

[Rob Campbell [:rc] (:robcee)]

this bug is approaching browser-stress-test length.

I'll re-add the type="content" attribute. I wasn't sure if that was relevant to an iframe or not.

Regarding the InspectorStore changes, I was finding some weird behavior when switching between tabs. The reactivation wasn't setting live inspection properly on startup and the getValue() method was throwing warnings in debug builds (accessing undefined property).

We made some additional changes based on discussion with Mihai yesterday around the observer notification to keep our tests running properly. The browser.reload() method in IUI_openTreePanel was no longer needed after the switch.

Feel free to ping me in irc if you have any questions!

Comment 84

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 471501 [details] [diff] [review]
Inspector tree panel iframe 4

>diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js

>     this.treeBrowser = document.getElementById("inspector-tree-browser");

Might want to rename to treeIframe for clarity.

This patch adds a notifyObservers("inspector-opened") call, but I don't see one being removed (from the original patch) - is that just an interdiffing artifact, or something?

r=me with that and the type="content" thing addressed.

Comment 85

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #84)
> Comment on attachment 471501 [details] [diff] [review]
> Inspector tree panel iframe 4
> 
> >diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js
> 
> >     this.treeBrowser = document.getElementById("inspector-tree-browser");
> 
> Might want to rename to treeIframe for clarity.

Yeah. Could do.

> This patch adds a notifyObservers("inspector-opened") call, but I don't see one
> being removed (from the original patch) - is that just an interdiffing
> artifact, or something?

yeah, it must be. It was originally in the openTreePanel() method of IUI. It was fired when the tree panel's browser element complete its load event.

> r=me with that and the type="content" thing addressed.

ok! Third time's the charm! Thanks, gavin.

Comment 86

[Rob Campbell [:rc] (:robcee)]

Attachment: [checked-in] Inspector tree panel 2010-09-03

http://hg.mozilla.org/mozilla-central/rev/deeb4cf3baba