More updates
authorDave Camp <dcamp@mozilla.com>
Tue, 11 Jun 2013 20:43:31 -0700
changeset 3 58e81b95dfa169d51b91296c095455bcb9322428
parent 2 5f41b41d8c38887600e1bea247fb495751425606
child 4 65c86d4ac2bbfce6c0ee4fdf98cfd4646efd944c
push id4
push userdcamp@campd.org
push dateWed, 12 Jun 2013 03:43:40 +0000
More updates
inspector-queue-mutations.diff
inspector-retain.diff
protocol-helpers-reviewcomments.diff
protocol-helpers.diff
series
new file mode 100644
--- /dev/null
+++ b/inspector-queue-mutations.diff
@@ -0,0 +1,109 @@
+# HG changeset patch
+# Parent b06d55e94d577f83f27e6bf596fe90ea057281c3
+
+diff --git a/toolkit/devtools/server/actors/inspector.js b/toolkit/devtools/server/actors/inspector.js
+--- a/toolkit/devtools/server/actors/inspector.js
++++ b/toolkit/devtools/server/actors/inspector.js
+@@ -1134,27 +1134,35 @@ var WalkerActor = protocol.ActorClass({
+     request: {
+       cleanup: Option(0)
+     },
+     response: {
+       mutations: RetVal("array:dommutation")
+     }
+   }),
+ 
++  queueMutation: function(mutation) {
++    // We only send the `new-mutations` notification once, until the client
++    // fetches mutations with the `getMutations` packet.
++    let needEvent = this._pendingMutations.length === 0;
++
++    this._pendingMutations.push(mutation);
++
++    if (needEvent) {
++      events.emit(this, "new-mutations");
++    }
++  },
++
+   /**
+    * Handles mutations from the DOM mutation observer API.
+    *
+    * @param array[MutationRecord] mutations
+    *    See https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver#MutationRecord
+    */
+   onMutations: function(mutations) {
+-    // We only send the `new-mutations` notification once, until the client
+-    // fetches mutations with the `getMutations` packet.
+-    let needEvent = this._pendingMutations.length === 0;
+-
+     for (let change of mutations) {
+       let targetActor = this._refMap.get(change.target);
+       if (!targetActor) {
+         continue;
+       }
+       let targetNode = change.target;
+       let mutation = {
+         type: change.type,
+@@ -1201,58 +1209,47 @@ var WalkerActor = protocol.ActorClass({
+           // to date.
+           this._orphaned.delete(addedActor);
+           addedActors.push(addedActor.actorID);
+         }
+         mutation.numChildren = change.target.childNodes.length;
+         mutation.removed = removedActors;
+         mutation.added = addedActors;
+       }
+-      this._pendingMutations.push(mutation);
+-    }
+-    if (needEvent) {
+-      events.emit(this, "new-mutations");
++      this.queueMutation(mutation);
+     }
+   },
+ 
+   onFrameLoad: function(window) {
+     let frame = window.frameElement;
+     let frameActor = this._refMap.get(frame);
+     if (!frameActor) {
+       return;
+     }
+-    let needEvent = this._pendingMutations.length === 0;
+-    this._pendingMutations.push({
++
++    this.queueMutation({
+       type: "frameLoad",
+       target: frameActor.actorID,
+       added: [],
+       removed: []
+     });
+-
+-    if (needEvent) {
+-      events.emit(this, "new-mutations");
+-    }
+   },
+ 
+   onFrameUnload: function(window) {
+     let doc = window.document;
+     let documentActor = this._refMap.get(doc);
+     if (!documentActor) {
+       return;
+     }
+ 
+-    let needEvent = this._pendingMutations.length === 0;
+-    this._pendingMutations.push({
++    this.queueMutation({
+       type: "documentUnload",
+       target: documentActor.actorID
+     });
+     this.releaseNode(documentActor);
+-    if (needEvent) {
+-      events.emit(this, "new-mutations");
+-    }
+   }
+ });
+ 
+ /**
+  * Client side of the DOM walker.
+  */
+ var WalkerFront = exports.WalkerFront = protocol.FrontClass(WalkerActor, {
+   // Set to true if cleanup should be requested after every mutation list.
--- a/inspector-retain.diff
+++ b/inspector-retain.diff
@@ -1,20 +1,20 @@
 # HG changeset patch
 # User Dave Camp <dcamp@mozilla.com>
 # Date 1370924325 25200
 #      Mon Jun 10 21:18:45 2013 -0700
 # Node ID 3f5f55d861811c26bf757a7d40a2780ee8a4d074
-# Parent  e163463312617c7f367e3abff148098d570af2e3
+# Parent ebe41f0088d327951bb6260597f9f15353f156d0
 imported patch inspector-retain.diff
 
 diff --git a/toolkit/devtools/server/actors/inspector.js b/toolkit/devtools/server/actors/inspector.js
 --- a/toolkit/devtools/server/actors/inspector.js
 +++ b/toolkit/devtools/server/actors/inspector.js
-@@ -227,29 +227,31 @@ let NodeFront = protocol.FrontClass(Node
+@@ -204,29 +204,31 @@ let NodeFront = protocol.FrontClass(Node
    initialize: function(conn, form, detail, ctx) {
      this._parent = null; // The parent node
      this._child = null;  // The first child of this node.
      this._next = null;   // The next sibling of this node.
      this._prev = null;   // The previous sibling of this node.
      protocol.Front.prototype.initialize.call(this, conn, form, detail, ctx);
    },
  
@@ -41,19 +41,19 @@ diff --git a/toolkit/devtools/server/act
      protocol.Front.prototype.destroy.call(this);
    },
  
    // Update the object given a form representation off the wire.
    form: function(form, detail, ctx) {
      // Shallow copy of the form.  We could just store a reference, but
      // eventually we'll want to update some of the data.
      this._form = object.merge(form);
-@@ -674,16 +676,21 @@ var WalkerActor = protocol.ActorClass({
+@@ -640,16 +642,21 @@ var WalkerActor = protocol.ActorClass({
+     this._refMap = new Map();
      this._pendingMutations = [];
-     this._activePseudoClassLocks = new Set();
  
      // Nodes which have been removed from the client's known
      // ownership tree are considered "orphaned", and stored in
      // this set.
      this._orphaned = new Set();
  
 +    // The client can tell the walker that it is interested in a node
 +    // even when it is orphaned with the `retainNode` method.  This
@@ -63,17 +63,17 @@ diff --git a/toolkit/devtools/server/act
      this.onMutations = this.onMutations.bind(this);
      this.onFrameLoad = this.onFrameLoad.bind(this);
      this.onFrameUnload = this.onFrameUnload.bind(this);
  
      this.progressListener = ProgressListener(webProgress);
  
      events.on(this.progressListener, "windowchange-start", this.onFrameUnload);
      events.on(this.progressListener, "windowchange-stop", this.onFrameLoad);
-@@ -825,33 +832,85 @@ var WalkerActor = protocol.ActorClass({
+@@ -786,33 +793,85 @@ var WalkerActor = protocol.ActorClass({
      let parent = walker.parentNode();
      if (parent) {
        return this._ref(parent);
      }
      return null;
    },
  
    /**
@@ -152,17 +152,17 @@ diff --git a/toolkit/devtools/server/act
    }),
  
    /**
     * Add any nodes between `node` and the walker's root node that have not
     * yet been seen by the client.
     */
    ensurePathToRoot: function(node, newParents=new Set()) {
      if (!node) {
-@@ -1367,16 +1426,18 @@ var WalkerActor = protocol.ActorClass({
+@@ -1119,16 +1178,18 @@ var WalkerActor = protocol.ActorClass({
     * in the new set of children it needs to issue a `children` request.
     */
    getMutations: method(function(options={}) {
      let pending = this._pendingMutations || [];
      this._pendingMutations = [];
  
      if (options.cleanup) {
        for (let node of this._orphaned) {
@@ -171,18 +171,18 @@ diff --git a/toolkit/devtools/server/act
          this.releaseNode(node);
        }
        this._orphaned = new Set();
      }
  
      return pending;
    }, {
      request: {
-@@ -1477,41 +1538,79 @@ var WalkerActor = protocol.ActorClass({
-     this._queueMutation({
+@@ -1228,41 +1289,79 @@ var WalkerActor = protocol.ActorClass({
+     this.queueMutation({
        type: "frameLoad",
        target: frameActor.actorID,
        added: [],
        removed: []
      });
    },
  
 +  // Returns true if domNode is in window or a subframe.
@@ -208,33 +208,32 @@ diff --git a/toolkit/devtools/server/act
 +          this._childOfWindow(window, retained.rawNode)) {
 +        this._retainedOrphans.delete(retained);
 +        releasedOrphans.push(retained.actorID);
 +        this.releaseNode(retained, { force: true });
 +      }
 +    }
 +
 +    if (releasedOrphans.length > 0) {
-+      this._queueMutation({
++      this.queueMutation({
 +        target: this.rootNode.actorID,
 +        type: "unretained",
 +        nodes: releasedOrphans
 +      });
 +    }
 +
      let doc = window.document;
      let documentActor = this._refMap.get(doc);
      if (!documentActor) {
        return;
      }
  
-     this._queueMutation({
+     this.queueMutation({
        type: "documentUnload",
--      target: documentActor.actorID
-+      target: documentActor.actorID,
+       target: documentActor.actorID
      });
 -    this.releaseNode(documentActor);
 +
 +    // Need to force a release of this node, because those nodes can't
 +    // be accessed anymore.
 +    this.releaseNode(documentActor, { force: true });
    }
  });
@@ -253,17 +252,17 @@ diff --git a/toolkit/devtools/server/act
    },
  
    destroy: function() {
      protocol.Front.prototype.destroy.call(this);
    },
  
    // Update the object given a form representation off the wire.
    form: function(json) {
-@@ -1531,21 +1630,61 @@ var WalkerFront = exports.WalkerFront = 
+@@ -1282,34 +1381,96 @@ var WalkerFront = exports.WalkerFront = 
      let front = this.get(id);
      if (front) {
        return front;
      }
  
      return types.getType("domnode").read({ actor: id }, this, "standin");
    },
  
@@ -317,24 +316,20 @@ diff --git a/toolkit/devtools/server/act
      return this._releaseNode({ actorID: actorID });
    }, {
      impl: "_releaseNode"
    }),
  
    querySelector: protocol.custom(function(queryNode, selector) {
      return this._querySelector(queryNode, selector).then(response => {
        return response.node;
-@@ -1556,16 +1695,38 @@ var WalkerFront = exports.WalkerFront = 
- 
-   /**
-    * Return a new AttributeModificationList for this node.
-    */
-   startModifyingAttributes: function(node) {
-     return AttributeModificationList(this, node);
-   },
+     });
+   }, {
+     impl: "_querySelector"
+   }),
  
 +  _releaseFront: function(node, force) {
 +    if (node.retained && !force) {
 +      node.reparent(null);
 +      this._retainedOrphans.add(node);
 +      return;
 +    }
 +
@@ -356,17 +351,17 @@ diff --git a/toolkit/devtools/server/act
    /**
     * Get any unprocessed mutation records and process them.
     */
    getMutations: protocol.custom(function(options={}) {
      return this._getMutations(options).then(mutations => {
        let emitMutations = [];
        for (let change of mutations) {
          // The target is only an actorID, get the associated front.
-@@ -1622,27 +1783,38 @@ var WalkerFront = exports.WalkerFront = 
+@@ -1366,27 +1527,38 @@ var WalkerFront = exports.WalkerFront = 
              if (child.nodeType === Ci.nsIDOMNode.DOCUMENT_NODE) {
                console.trace("Got an unexpected frameLoad in the inspector, please file a bug on bugzilla.mozilla.org!");
              }
            }
          } else if (change.type === "documentUnload") {
            // We try to give fronts instead of actorIDs, but these fronts need
            // to be destroyed now.
            emittedMutation.target = targetFront.actorID;
@@ -400,37 +395,36 @@ diff --git a/toolkit/devtools/server/act
  
        events.emit(this, "mutations", emitMutations);
      });
    }, {
      impl: "_getMutations"
 diff --git a/toolkit/devtools/server/tests/mochitest/Makefile.in b/toolkit/devtools/server/tests/mochitest/Makefile.in
 --- a/toolkit/devtools/server/tests/mochitest/Makefile.in
 +++ b/toolkit/devtools/server/tests/mochitest/Makefile.in
-@@ -16,15 +16,16 @@ MOCHITEST_CHROME_FILES	= \
+@@ -14,14 +14,15 @@ include $(DEPTH)/config/autoconf.mk
+ MOCHITEST_CHROME_FILES	= \
+ 	inspector-helpers.js \
  	inspector-traversal-data.html \
- 	test_inspector-changeattrs.html \
- 	test_inspector-changevalue.html \
  	test_inspector-mutations-attr.html \
  	test_inspector-mutations-childlist.html \
  	test_inspector-mutations-frameload.html \
  	test_inspector-mutations-value.html \
  	test_inspector-release.html \
 +	test_inspector-retain.html \
- 	test_inspector-pseudoclass-lock.html \
  	test_inspector-traversal.html \
  	test_unsafeDereference.html \
  	nonchrome_unsafeDereference.html \
  	$(NULL)
  
  include $(topsrcdir)/config/rules.mk
 diff --git a/toolkit/devtools/server/tests/mochitest/inspector-helpers.js b/toolkit/devtools/server/tests/mochitest/inspector-helpers.js
 --- a/toolkit/devtools/server/tests/mochitest/inspector-helpers.js
 +++ b/toolkit/devtools/server/tests/mochitest/inspector-helpers.js
-@@ -117,31 +117,33 @@ function serverOwnershipSubtree(walker, 
+@@ -105,31 +105,33 @@ function serverOwnershipSubtree(walker, 
  }
  
  function serverOwnershipTree(walker) {
    let serverConnection = walker.conn._transport._serverConnection;
    let serverWalker = serverConnection.getActor(walker.actorID);
  
    return {
      root: serverOwnershipSubtree(serverWalker, serverWalker.rootDoc ),
@@ -456,17 +450,17 @@ diff --git a/toolkit/devtools/server/tes
    }
  }
  
  function ownershipTreeSize(tree) {
    let size = 1;
    for (let child of tree.children) {
      size += ownershipTreeSize(child);
    }
-@@ -168,26 +170,114 @@ function checkMissing(client, actorID) {
+@@ -156,26 +158,114 @@ function checkMissing(client, actorID) {
      type: "request",
    }, response => {
      is(response.error, "noSuchActor", "node list actor should no longer be contactable.");
      deferred.resolve(undefined);
    });
    return deferred.promise;
  }
  
deleted file mode 100644
--- a/protocol-helpers-reviewcomments.diff
+++ /dev/null
@@ -1,677 +0,0 @@
-# HG changeset patch
-# User Dave Camp <dcamp@mozilla.com>
-# Date 1370924227 25200
-#      Mon Jun 10 21:17:07 2013 -0700
-# Node ID 6b240990833678387343d3c62f6ea3882f5c1cad
-# Parent  9ffffc80410871c3fb5f77862bc9c72367be4772
-imported patch protocol-helpers-reviewcomments.diff
-
-diff --git a/toolkit/devtools/server/protocol.js b/toolkit/devtools/server/protocol.js
---- a/toolkit/devtools/server/protocol.js
-+++ b/toolkit/devtools/server/protocol.js
-@@ -34,18 +34,18 @@ function promiseDone(err) {
-  * Types are referred to with a typestring.  Basic types are
-  * registered by name using addType, and more complex types can
-  * be generated by adding detail to the type name.
-  */
- 
- let types = Object.create(null);
- exports.types = types;
- 
--let registeredTypes = Object.create(null);
--let registeredLifetimes = Object.create(null);
-+let registeredTypes = new Map();
-+let registeredLifetimes = new Map();
- 
- /**
-  * Return the type object associated with a given typestring.
-  * If passed a type object, it will be returned unchanged.
-  *
-  * Types can be registered with addType, or can be created on
-  * the fly with typestrings.  Examples:
-  *
-@@ -65,75 +65,87 @@ types.getType = function(type) {
-     return types.Primitive;
-   }
- 
-   if (typeof(type) !== "string") {
-     return type;
-   }
- 
-   // If already registered, we're done here.
--  let reg = registeredTypes[type];
-+  let reg = registeredTypes.get(type);
-   if (reg) return reg;
- 
-   // New type, see if it's a collection/lifetime type:
-   let sep = type.indexOf(":");
-   if (sep >= 0) {
-     let collection = type.substring(0, sep);
-     let subtype = types.getType(type.substring(sep + 1));
- 
-     if (collection === "array") {
-       return types.addArrayType(subtype);
-     }
- 
--    if (collection in registeredLifetimes) {
-+    if (registeredLifetimes.has(collection)) {
-       return types.addLifetimeType(collection, subtype);
-     }
- 
-     throw Error("Unknown collection type: " + collection);
-   }
- 
-   // Not a collection, might be actor detail
-   let pieces = type.split("#", 2);
-   if (pieces.length > 1) {
-     return types.addActorDetail(type, pieces[0], pieces[1]);
-   }
- 
-   // Might be a lazily-loaded type
-   if (type === "longstring") {
-     require("devtools/server/actors/string");
--    return registeredTypes["longstring"];
-+    return registeredTypes.get("longstring");
-   }
- 
-   throw Error("Unknown type: " + type);
- }
- 
- /**
-  * Add a type to the type system.
-  *
-+ * When registering a type, you can provide `read` and `write` methods.
-+ *
-+ * The `read` method will be passed a JS object value from the JSON
-+ * packet and must return a native representation.  The `write` method will
-+ * be passed a native representation and should provide a JSONable value.
-+ *
-+ * These methods will both be passed a context.  The context is the object
-+ * performing or servicing the request - on the server side it will be
-+ * an Actor, on the client side it will be a Front.
-+ *
-  * @param typestring name
-  *    Name to register
-- * @param object methods
-- *    The type's read and write methods.  Either can be blank
-- *    to use an identity function.
-+ * @param object typeObject
-+ *    An object whose properties will be stored in the type, including
-+ *    the `read` and `write` methods.
-  * @param object options
-  *    Can specify `thawed` to prevent the type from being frozen.
-+ *
-+ * @returns a type object that can be used in protocol definitions.
-  */
--types.addType = function(name, methods={}, options={}) {
--  if (name in registeredTypes) {
-+types.addType = function(name, typeObject={}, options={}) {
-+  if (registeredTypes.has(name)) {
-     throw Error("Type '" + name + "' already exists.");
-   }
- 
-   let type = object.merge({
-     name: name,
--    primitive: !(methods.read || methods.write),
-+    primitive: !(typeObject.read || typeObject.write),
-     read: v => v,
-     write: v => v
--  }, methods);
-+  }, typeObject);
- 
--  registeredTypes[name] = type;
-+  registeredTypes.set(name, type);
- 
-   if (!options.thawed) {
-     Object.freeze(type);
-   }
- 
-   return type;
- };
- 
-@@ -215,17 +227,17 @@ types.addActorType = function(name) {
-     _actor: true,
-     read: (v, ctx, detail) => {
-       // If we're reading a request on the server side, just
-       // find the actor registered with this actorID.
-       if (ctx instanceof Actor) {
-         return ctx.conn.getActor(v);
-       }
- 
--      // Reading a response from the client side, check for an
-+      // Reading a response on the client side, check for an
-       // existing front on the connection, and create the front
-       // if it isn't found.
-       let front = ctx.conn.getActor(v.actor);
-       if (front) {
-         front.form(v, detail, ctx);
-       } else {
-         front = new type.frontClass(ctx.conn, v, detail, ctx)
-         front.actorID = v.actor;
-@@ -253,25 +265,25 @@ types.addActorType = function(name) {
-   });
-   return type;
- }
- 
- /**
-  * Register an actor implementation for a previously-added actor type.
-  */
- types.addActorImpl = function(name, actorClass) {
--  registeredTypes[name].actorClass = actorClass;
-+  registeredTypes.get(name).actorClass = actorClass;
- }
- 
- /**
-  * Register a front implementation for a previously-added actor type.
-  */
- types.addActorFront = function(name, frontClass) {
--  registeredTypes[name].frontClass = frontClass;
--  Object.freeze(registeredTypes[name]);
-+  registeredTypes.get(name).frontClass = frontClass;
-+  Object.freeze(registeredTypes.get(name));
- }
- 
- /**
-  * Register an actor detail type.  This is just like an actor type, but
-  * will pass a detail hint to the actor's form method during serialization/
-  * deserialization.
-  *
-  * This is called by getType() when passed an 'actorType#detail' string.
-@@ -285,35 +297,35 @@ types.addActorFront = function(name, fro
-  */
- types.addActorDetail = function(name, actorType, detail) {
-   actorType = types.getType(actorType);
-   if (!actorType._actor) {
-     throw Error("Details only apply to actor types, tried to add detail '" + detail + "'' to " + actorType.name + "\n");
-   }
-   return types.addType(name, {
-     _actor: true,
--    read: (v, ctx, detail) => actorType.read(v, ctx, detail),
-+    read: (v, ctx) => actorType.read(v, ctx, detail),
-     write: (v, ctx) => actorType.write(v, ctx, detail)
-   });
- }
- 
- /**
-  * Register an actor lifetime.  This lets the type system find a parent
-  * actor that differs from the actor fulfilling the request.
-  *
-  * @param string name
-  *    The lifetime name to use in typestrings.
-  * @param string prop
-  *    The property of the actor that holds the parent that should be used.
-  */
- types.addLifetime = function(name, prop) {
--  if (name in registeredLifetimes) {
-+  if (registeredLifetimes.has(name)) {
-     throw Error("Lifetime '" + name + "' already registered.");
-   }
--  registeredLifetimes[name] = prop;
-+  registeredLifetimes.set(name, prop);
- }
- 
- /**
-  * Register a lifetime type.  This creates an actor type tied to the given
-  * lifetime.
-  *
-  * This is called by getType() when passed a '<lifetimeType>:<actorType>'
-  * typestring.
-@@ -323,17 +335,17 @@ types.addLifetime = function(name, prop)
-  * @param type subtype
-  *    An actor type
-  */
- types.addLifetimeType = function(lifetime, subtype) {
-   subtype = types.getType(subtype);
-   if (!subtype._actor) {
-     throw Error("Lifetimes only apply to actor types, tried to apply lifetime '" + lifetime + "'' to " + subtype.name);
-   }
--  let prop = registeredLifetimes[lifetime];
-+  let prop = registeredLifetimes.get(lifetime);
-   return types.addType(lifetime + ":" + subtype.name, {
-     read: (value, ctx) => subtype.read(value, ctx[prop]),
-     write: (value, ctx) => subtype.write(value, ctx[prop])
-   })
- }
- 
- // Add a few named primitive types.
- types.Primitive = types.addType("primitive");
-@@ -349,45 +361,35 @@ types.JSON = types.addType("json");
-  * Arg and Option placeholders where arguments should be
-  * placed.
-  *
-  * Reponse packets are also specified as json templates,
-  * with a RetVal placeholder where the return value should be
-  * placed.
-  */
- 
--function cloneArg(arg) {
--  let cloned = arg.constructor(arg.index, arg.type);
--  cloned.clone(arg);
--  return cloned;
--}
--
- /**
-  * Placeholder for simple arguments.
-  *
-  * @param number index
-  *    The argument index to place at this position.
-  * @param type type
-  *    The argument should be marshalled as this type.
-  * @param object options
-  *    Argument options:
-  *      optional: true if the argument can be undefined or null.
-  * @constructor
-  */
--exports.Arg = Class({
-+let Arg = Class({
-   initialize: function(index, type, options={}) {
-     this.index = index;
-     this.type = types.getType(type);
-     this.optional = !!options.optional;
-   },
- 
--  clone: function(other) {
--    this.optional = other.optional;
--  },
--
-   write: function(arg, ctx) {
-     if (typeof(arg) === "undefined" || arg === null) {
-       if (!this.optional) throw Error("Required argument " + this.name + " not specified.");
-       return undefined;
-     }
-     return this.type.write(arg, ctx);
-   },
- 
-@@ -395,16 +397,17 @@ exports.Arg = Class({
-     if (typeof(v)  === "undefined" || v === null) {
-       if (!this.optional) throw Error("Required argument " + this.name + " not specified.");
-       outArgs[this.index] = v;
-       return;
-     }
-     outArgs[this.index] = this.type.read(v, ctx);
-   }
- });
-+exports.Arg = Arg;
- 
- /**
-  * Placeholder for an options argument value that should be hoisted
-  * into the packet.
-  *
-  * If provided in a method specification:
-  *
-  *   { optionArg: Option(1)}
-@@ -413,59 +416,55 @@ exports.Arg = Class({
-  * value's place.
-  *
-  * @param number index
-  *    The argument index of the options value.
-  * @param type type
-  *    The argument should be marshalled as this type.
-  * @constructor
-  */
--exports.Option = Class({
--  extends: exports.Arg,
-+let Option = Class({
-+  extends: Arg,
-   initialize: function(index, type) {
--    exports.Arg.prototype.initialize.call(this, index, type)
-+    Arg.prototype.initialize.call(this, index, type)
-   },
- 
--  clone: function(other) {
--    this.name = other.name;
--  },
--
--  name: null, // Will be set during actor generation
--
--  write: function(arg, ctx) {
-+  write: function(arg, ctx, name) {
-     if (!arg) {
-       return undefined;
-     }
--    let v = arg[this.name] || undefined;
-+    let v = arg[name] || undefined;
-     if (typeof(v) === "undefined") {
-       return undefined;
-     }
-     return this.type.write(v, ctx);
-   },
--  read: function(v, ctx, outArgs) {
-+  read: function(v, ctx, outArgs, name) {
-+    if (typeof(outArgs[this.index]) === "undefined") {
-+      outArgs[this.index] = {};
-+    }
-     if (typeof(v) === "undefined") {
-       return;
-     }
--    if (typeof(outArgs[this.index]) === "undefined") {
--      outArgs[this.index] = {};
--    }
--    outArgs[this.index][this.name] = this.type.read(v, ctx);
-+    outArgs[this.index][name] = this.type.read(v, ctx);
-   }
- });
- 
-+exports.Option = Option;
-+
- /**
-  * Placeholder for return values in a response template.
-  *
-  * @param type type
-  *    The return value should be marshalled as this type.
-  * @param object options
-  *    Argument options:
-  *      optional: true if the argument can be undefined or null.
-  */
--exports.RetVal = Class({
-+let RetVal = Class({
-   initialize: function(type, options={}) {
-     this.type = types.getType(type);
-     this.optional = !!options.optional;
-   },
- 
-   write: function(v, ctx) {
-     if (typeof(v) != "undefined" && v != null) {
-       return this.type.write(v, ctx);
-@@ -482,209 +481,163 @@ exports.RetVal = Class({
-     }
-     if (!this.optional) {
-       throw Error("Return value not specified.");
-     }
-     return v;
-   }
- });
- 
--function cloneRetVal(other) {
--  let cloned = new exports.RetVal(other.type);
--  cloned.optional = other.optional;
--  return cloned;
--}
-+exports.RetVal = RetVal;
- 
- /* Template handling functions */
- 
- /**
-- * Clone a template json object.
-- */
--function cloneTemplate(template) {
--  // Better way to do this?
--  return JSON.parse(JSON.stringify(template));
--}
--
--/**
-  * Get the value at a given path, or undefined if not found.
-  */
- function getPath(obj, path) {
-   for (let name of path) {
-     if (!(name in obj)) {
-       return undefined;
-     }
-     obj = obj[name];
-   }
-   return obj;
- }
- 
- /**
-- * Set the value at a given path.  Will create
-- * objects in the path if they're missing.
-+ * Find Placeholders in the template and save them along with their
-+ * paths.
-  */
--function setPath(obj, path, v) {
--  for (let i = 0; i < path.length - 1; i++) {
--    let name = path[i];
--    if (!(name in obj)) {
--      obj[name] = {};
--    }
--    obj = obj[name];
-+function findPlaceholders(template, constructor, path=[], placeholders=[]) {
-+  if (!template || typeof(template) != "object") {
-+    return placeholders;
-   }
--  obj[path[path.length - 1]] = v;
-+
-+  if (template instanceof constructor) {
-+    placeholders.push({ placeholder: template, path: [p for (p of path)] });
-+    return placeholders;
-+  }
-+
-+  for (let name in template) {
-+    path.push(name);
-+    findPlaceholders(template[name], constructor, path, placeholders);
-+    path.pop();
-+  }
-+
-+  return placeholders;
- }
- 
-+
- /**
-  * Manages a request template.
-  *
-  * @param object template
-  *    The request template.
-  * @construcor
-  */
- let Request = Class({
-   initialize: function(template={}) {
-     this.type = template.type;
--    this.args = [];
--    this.template = this.processTemplate(template);
-+    this.template = template;
-+    this.args = findPlaceholders(template, Arg);
-   },
- 
-   /**
-    * Write a request.
-    *
-    * @param array fnArgs
-    *    The function arguments to place in the request.
-    * @param object ctx
-    *    The object making the request.
-    * @returns a request packet.
-    */
-   write: function(fnArgs, ctx) {
--    if (!this.template) {
--      return {};
--    }
--    let packet = cloneTemplate(this.template);
--    for (let templateArg of this.args) {
--      setPath(packet, templateArg.path, templateArg.write(fnArgs[templateArg.index], ctx));
--    }
--    return packet;
-+    let str = JSON.stringify(this.template, (key, value) => {
-+      if (value instanceof Arg) {
-+        return value.write(fnArgs[value.index], ctx, key);
-+      }
-+      return value;
-+    });
-+    return JSON.parse(str);
-   },
- 
-   /**
-    * Read a request.
-    *
-    * @param object packet
-    *    The request packet.
-    * @param object ctx
-    *    The object making the request.
-    * @returns an arguments array
-    */
-   read: function(packet, ctx) {
-     let fnArgs = [];
-     for (let templateArg of this.args) {
--      templateArg.read(getPath(packet, templateArg.path), ctx, fnArgs);
-+      let arg = templateArg.placeholder;
-+      let path = templateArg.path;
-+      let name = path[path.length - 1];
-+      arg.read(getPath(packet, path), ctx, fnArgs, name);
-     }
-     return fnArgs;
-   },
--
--  /**
--   * Create a clone of the request template, saving any Arg instances
--   * found in the template along with their path.
--   */
--  processTemplate: function(template, path=[]) {
--    if (!template || typeof(template) != "object") {
--      return template;
--    }
--
--    if (template instanceof exports.Arg) {
--      let arg = cloneArg(template);
--      arg.name = path[path.length - 1];
--      arg.path = [p for (p of path)];
--      this.args.push(arg);
--      return undefined;
--    }
--
--    let newTemplate = {};
--    for (let name in template) {
--      path.push(name);
--      newTemplate[name] = this.processTemplate(template[name], path);
--      path.pop();
--    }
--    return newTemplate;
--  },
- });
- 
- /**
-  * Manages a response template.
-  *
-  * @param object template
-  *    The response template.
-  * @construcor
-  */
- let Response = Class({
-   initialize: function(template={}) {
--    // This will set this.path and this.retVal
--    this.template = this.processTemplate(template);
-+    this.template = template;
-+    let placeholders = findPlaceholders(template, RetVal);
-+    if (placeholders.length > 1) {
-+      throw Error("More than one RetVal specified in response");
-+    }
-+    let placeholder = placeholders.shift();
-+    if (placeholder) {
-+      this.retVal = placeholder.placeholder;
-+      this.path = placeholder.path;
-+    }
-   },
- 
-   /**
-    * Write a response for the given return value.
-    *
-    * @param val ret
-    *    The return value.
-    * @param object ctx
-    *    The object writing the response.
-    */
-   write: function(ret, ctx) {
--    if (!this.retVal) {
--      return {};
--    }
--    let val = this.retVal.write(ret, ctx);
--    if (this.path.length === 0) {
--      return val;
--    }
--    let packet = cloneTemplate(this.template);
--    setPath(packet, this.path, val);
--    return packet;
-+    return JSON.parse(JSON.stringify(this.template, function(key, value) {
-+      if (value instanceof RetVal) {
-+        return value.write(ret, ctx);
-+      }
-+      return value;
-+    }));
-   },
- 
-   /**
-    * Read a return value from the given response.
-    *
-    * @param object packet
-    *    The response packet.
-    * @param object ctx
-    *    The object reading the response.
-    */
-   read: function(packet, ctx) {
-     if (!this.retVal) {
-       return undefined;
-     }
--
-     let v = getPath(packet, this.path);
-     return this.retVal.read(v, ctx);
--  },
--
--  /**
--   * Create a clone of the response template, saving the RetVal
--   * instance in the template along with its path.
--   */
--  processTemplate: function(template, path=[]) {
--    if (typeof(template) != "object") {
--      return template;
--    }
--    if (template instanceof exports.RetVal) {
--      this.retVal = cloneRetVal(template);
--      this.path = [p for (p of path)];
--      return undefined;
--    }
--    let newTemplate = {};
--    for (let name in template) {
--      path.push(name);
--      newTemplate[name] = this.processTemplate(template[name], path);
--      path.pop();
--    }
--    return newTemplate;
-   }
- });
- 
- /**
-  * Actor and Front implementations
-  */
- 
- /**
-@@ -988,17 +941,17 @@ exports.actorProto = function(actorProto
-  *    The object prototype.  Must have a 'typeName' property,
-  *    should have method definitions, can have event definitions.
-  */
- exports.ActorClass = function(proto) {
-   if (!proto.typeName) {
-     throw Error("Actor prototype must have a typeName member.");
-   }
-   proto.extends = Actor;
--  if (!(proto.typeName in registeredTypes)) {
-+  if (!registeredTypes.has(proto.typeName)) {
-     types.addActorType(proto.typeName);
-   }
-   let cls = Class(exports.actorProto(proto));
-   types.addActorImpl(proto.typeName, cls);
-   return cls;
- };
- 
- /**
-diff --git a/toolkit/devtools/server/tests/unit/test_protocol_simple.js b/toolkit/devtools/server/tests/unit/test_protocol_simple.js
---- a/toolkit/devtools/server/tests/unit/test_protocol_simple.js
-+++ b/toolkit/devtools/server/tests/unit/test_protocol_simple.js
-@@ -181,16 +181,20 @@ function run_test()
-       do_check_eq(ret.c, 3);
-       return rootClient.optionArgs({
-         "option1": 5,
-         "option2": 10
-       });
-     }).then(ret => {
-       do_check_eq(ret.option1, 5);
-       do_check_eq(ret.option2, 10);
-+      return rootClient.optionArgs({});
-+    }).then(ret => {
-+      do_check_true(typeof(ret.option1) === "undefined");
-+      do_check_true(typeof(ret.option2) === "undefined");
-       // Explicitly call an optional argument...
-       return rootClient.optionalArgs(5, 10);
-     }).then(ret => {
-       do_check_eq(ret, 10);
-       // Now don't pass the optional argument, expect the default.
-       return rootClient.optionalArgs(5);
-     }).then(ret => {
-       do_check_eq(ret, 200);
--- a/protocol-helpers.diff
+++ b/protocol-helpers.diff
@@ -1,15 +1,17 @@
 # HG changeset patch
 # User Dave Camp <dcamp@mozilla.com>
 # Date 1370546967 25200
 #      Thu Jun 06 12:29:27 2013 -0700
 # Node ID 9ffffc80410871c3fb5f77862bc9c72367be4772
-# Parent  d6d00b7871faa4bfc8408f54d8a43875a25c9591
+# Parent 5b25d41c55f4b6347423b59fa37459ed1ed28f6b
 [mq]: protocol-helpers.diff
+* * *
+imported patch protocol-helpers-reviewcomments.diff
 
 diff --git a/toolkit/devtools/client/dbg-client.jsm b/toolkit/devtools/client/dbg-client.jsm
 --- a/toolkit/devtools/client/dbg-client.jsm
 +++ b/toolkit/devtools/client/dbg-client.jsm
 @@ -223,16 +223,17 @@ this.DebuggerClient = function DebuggerC
    this._activeRequests = {};
    this._eventsEnabled = true;
  
@@ -383,17 +385,17 @@ diff --git a/toolkit/devtools/server/mai
                          '" does not recognize the packet type "' +
                          aPacket.type + '"') };
      }
  
 diff --git a/toolkit/devtools/server/protocol.js b/toolkit/devtools/server/protocol.js
 new file mode 100644
 --- /dev/null
 +++ b/toolkit/devtools/server/protocol.js
-@@ -0,0 +1,1265 @@
+@@ -0,0 +1,1223 @@
 +/* This Source Code Form is subject to the terms of the Mozilla Public
 + * License, v. 2.0. If a copy of the MPL was not distributed with this
 + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 +
 +"use strict";
 +
 +let {Cu} = require("chrome");
 +
@@ -425,18 +427,18 @@ new file mode 100644
 + * Types are referred to with a typestring.  Basic types are
 + * registered by name using addType, and more complex types can
 + * be generated by adding detail to the type name.
 + */
 +
 +let types = Object.create(null);
 +exports.types = types;
 +
-+let registeredTypes = Object.create(null);
-+let registeredLifetimes = Object.create(null);
++let registeredTypes = new Map();
++let registeredLifetimes = new Map();
 +
 +/**
 + * Return the type object associated with a given typestring.
 + * If passed a type object, it will be returned unchanged.
 + *
 + * Types can be registered with addType, or can be created on
 + * the fly with typestrings.  Examples:
 + *
@@ -456,75 +458,87 @@ new file mode 100644
 +    return types.Primitive;
 +  }
 +
 +  if (typeof(type) !== "string") {
 +    return type;
 +  }
 +
 +  // If already registered, we're done here.
-+  let reg = registeredTypes[type];
++  let reg = registeredTypes.get(type);
 +  if (reg) return reg;
 +
 +  // New type, see if it's a collection/lifetime type:
 +  let sep = type.indexOf(":");
 +  if (sep >= 0) {
 +    let collection = type.substring(0, sep);
 +    let subtype = types.getType(type.substring(sep + 1));
 +
 +    if (collection === "array") {
 +      return types.addArrayType(subtype);
 +    }
 +
-+    if (collection in registeredLifetimes) {
++    if (registeredLifetimes.has(collection)) {
 +      return types.addLifetimeType(collection, subtype);
 +    }
 +
 +    throw Error("Unknown collection type: " + collection);
 +  }
 +
 +  // Not a collection, might be actor detail
 +  let pieces = type.split("#", 2);
 +  if (pieces.length > 1) {
 +    return types.addActorDetail(type, pieces[0], pieces[1]);
 +  }
 +
 +  // Might be a lazily-loaded type
 +  if (type === "longstring") {
 +    require("devtools/server/actors/string");
-+    return registeredTypes["longstring"];
++    return registeredTypes.get("longstring");
 +  }
 +
 +  throw Error("Unknown type: " + type);
 +}
 +
 +/**
 + * Add a type to the type system.
 + *
++ * When registering a type, you can provide `read` and `write` methods.
++ *
++ * The `read` method will be passed a JS object value from the JSON
++ * packet and must return a native representation.  The `write` method will
++ * be passed a native representation and should provide a JSONable value.
++ *
++ * These methods will both be passed a context.  The context is the object
++ * performing or servicing the request - on the server side it will be
++ * an Actor, on the client side it will be a Front.
++ *
 + * @param typestring name
 + *    Name to register
-+ * @param object methods
-+ *    The type's read and write methods.  Either can be blank
-+ *    to use an identity function.
++ * @param object typeObject
++ *    An object whose properties will be stored in the type, including
++ *    the `read` and `write` methods.
 + * @param object options
 + *    Can specify `thawed` to prevent the type from being frozen.
++ *
++ * @returns a type object that can be used in protocol definitions.
 + */
-+types.addType = function(name, methods={}, options={}) {
-+  if (name in registeredTypes) {
++types.addType = function(name, typeObject={}, options={}) {
++  if (registeredTypes.has(name)) {
 +    throw Error("Type '" + name + "' already exists.");
 +  }
 +
 +  let type = object.merge({
 +    name: name,
-+    primitive: !(methods.read || methods.write),
++    primitive: !(typeObject.read || typeObject.write),
 +    read: v => v,
 +    write: v => v
-+  }, methods);
++  }, typeObject);
 +
-+  registeredTypes[name] = type;
++  registeredTypes.set(name, type);
 +
 +  if (!options.thawed) {
 +    Object.freeze(type);
 +  }
 +
 +  return type;
 +};
 +
@@ -606,17 +620,17 @@ new file mode 100644
 +    _actor: true,
 +    read: (v, ctx, detail) => {
 +      // If we're reading a request on the server side, just
 +      // find the actor registered with this actorID.
 +      if (ctx instanceof Actor) {
 +        return ctx.conn.getActor(v);
 +      }
 +
-+      // Reading a response from the client side, check for an
++      // Reading a response on the client side, check for an
 +      // existing front on the connection, and create the front
 +      // if it isn't found.
 +      let front = ctx.conn.getActor(v.actor);
 +      if (front) {
 +        front.form(v, detail, ctx);
 +      } else {
 +        front = new type.frontClass(ctx.conn, v, detail, ctx)
 +        front.actorID = v.actor;
@@ -644,25 +658,25 @@ new file mode 100644
 +  });
 +  return type;
 +}
 +
 +/**
 + * Register an actor implementation for a previously-added actor type.
 + */
 +types.addActorImpl = function(name, actorClass) {
-+  registeredTypes[name].actorClass = actorClass;
++  registeredTypes.get(name).actorClass = actorClass;
 +}
 +
 +/**
 + * Register a front implementation for a previously-added actor type.
 + */
 +types.addActorFront = function(name, frontClass) {
-+  registeredTypes[name].frontClass = frontClass;
-+  Object.freeze(registeredTypes[name]);
++  registeredTypes.get(name).frontClass = frontClass;
++  Object.freeze(registeredTypes.get(name));
 +}
 +
 +/**
 + * Register an actor detail type.  This is just like an actor type, but
 + * will pass a detail hint to the actor's form method during serialization/
 + * deserialization.
 + *
 + * This is called by getType() when passed an 'actorType#detail' string.
@@ -676,35 +690,35 @@ new file mode 100644
 + */
 +types.addActorDetail = function(name, actorType, detail) {
 +  actorType = types.getType(actorType);
 +  if (!actorType._actor) {
 +    throw Error("Details only apply to actor types, tried to add detail '" + detail + "'' to " + actorType.name + "\n");
 +  }
 +  return types.addType(name, {
 +    _actor: true,
-+    read: (v, ctx, detail) => actorType.read(v, ctx, detail),
++    read: (v, ctx) => actorType.read(v, ctx, detail),
 +    write: (v, ctx) => actorType.write(v, ctx, detail)
 +  });
 +}
 +
 +/**
 + * Register an actor lifetime.  This lets the type system find a parent
 + * actor that differs from the actor fulfilling the request.
 + *
 + * @param string name
 + *    The lifetime name to use in typestrings.
 + * @param string prop
 + *    The property of the actor that holds the parent that should be used.
 + */
 +types.addLifetime = function(name, prop) {
-+  if (name in registeredLifetimes) {
++  if (registeredLifetimes.has(name)) {
 +    throw Error("Lifetime '" + name + "' already registered.");
 +  }
-+  registeredLifetimes[name] = prop;
++  registeredLifetimes.set(name, prop);
 +}
 +
 +/**
 + * Register a lifetime type.  This creates an actor type tied to the given
 + * lifetime.
 + *
 + * This is called by getType() when passed a '<lifetimeType>:<actorType>'
 + * typestring.
@@ -714,17 +728,17 @@ new file mode 100644
 + * @param type subtype
 + *    An actor type
 + */
 +types.addLifetimeType = function(lifetime, subtype) {
 +  subtype = types.getType(subtype);
 +  if (!subtype._actor) {
 +    throw Error("Lifetimes only apply to actor types, tried to apply lifetime '" + lifetime + "'' to " + subtype.name);
 +  }
-+  let prop = registeredLifetimes[lifetime];
++  let prop = registeredLifetimes.get(lifetime);
 +  return types.addType(lifetime + ":" + subtype.name, {
 +    read: (value, ctx) => subtype.read(value, ctx[prop]),
 +    write: (value, ctx) => subtype.write(value, ctx[prop])
 +  })
 +}
 +
 +// Add a few named primitive types.
 +types.Primitive = types.addType("primitive");
@@ -740,62 +754,53 @@ new file mode 100644
 + * Arg and Option placeholders where arguments should be
 + * placed.
 + *
 + * Reponse packets are also specified as json templates,
 + * with a RetVal placeholder where the return value should be
 + * placed.
 + */
 +
-+function cloneArg(arg) {
-+  let cloned = arg.constructor(arg.index, arg.type);
-+  cloned.clone(arg);
-+  return cloned;
-+}
-+
 +/**
 + * Placeholder for simple arguments.
 + *
 + * @param number index
 + *    The argument index to place at this position.
 + * @param type type
 + *    The argument should be marshalled as this type.
 + * @param object options
 + *    Argument options:
 + *      optional: true if the argument can be undefined or null.
 + * @constructor
 + */
-+exports.Arg = Class({
++let Arg = Class({
 +  initialize: function(index, type, options={}) {
 +    this.index = index;
 +    this.type = types.getType(type);
 +    this.optional = !!options.optional;
 +  },
 +
-+  clone: function(other) {
-+    this.optional = other.optional;
-+  },
-+
 +  write: function(arg, ctx) {
-+    if (typeof(arg) === "undefined" || arg === null) {
++    if (arg === undefined || arg === null) {
 +      if (!this.optional) throw Error("Required argument " + this.name + " not specified.");
 +      return undefined;
 +    }
 +    return this.type.write(arg, ctx);
 +  },
 +
 +  read: function(v, ctx, outArgs) {
-+    if (typeof(v)  === "undefined" || v === null) {
++    if (v === undefined || v === null) {
 +      if (!this.optional) throw Error("Required argument " + this.name + " not specified.");
 +      outArgs[this.index] = v;
 +      return;
 +    }
 +    outArgs[this.index] = this.type.read(v, ctx);
 +  }
 +});
++exports.Arg = Arg;
 +
 +/**
 + * Placeholder for an options argument value that should be hoisted
 + * into the packet.
 + *
 + * If provided in a method specification:
 + *
 + *   { optionArg: Option(1)}
@@ -804,278 +809,228 @@ new file mode 100644
 + * value's place.
 + *
 + * @param number index
 + *    The argument index of the options value.
 + * @param type type
 + *    The argument should be marshalled as this type.
 + * @constructor
 + */
-+exports.Option = Class({
-+  extends: exports.Arg,
++let Option = Class({
++  extends: Arg,
 +  initialize: function(index, type) {
-+    exports.Arg.prototype.initialize.call(this, index, type)
++    Arg.prototype.initialize.call(this, index, type)
 +  },
 +
-+  clone: function(other) {
-+    this.name = other.name;
-+  },
-+
-+  name: null, // Will be set during actor generation
-+
-+  write: function(arg, ctx) {
++  write: function(arg, ctx, name) {
 +    if (!arg) {
 +      return undefined;
 +    }
-+    let v = arg[this.name] || undefined;
-+    if (typeof(v) === "undefined") {
++    let v = arg[name] || undefined;
++    if (v === undefined) {
 +      return undefined;
 +    }
 +    return this.type.write(v, ctx);
 +  },
-+  read: function(v, ctx, outArgs) {
-+    if (typeof(v) === "undefined") {
++  read: function(v, ctx, outArgs, name) {
++    if (outArgs[this.index] === undefined) {
++      outArgs[this.index] = {};
++    }
++    if (v === undefined) {
 +      return;
 +    }
-+    if (typeof(outArgs[this.index]) === "undefined") {
-+      outArgs[this.index] = {};
-+    }
-+    outArgs[this.index][this.name] = this.type.read(v, ctx);
++    outArgs[this.index][name] = this.type.read(v, ctx);
 +  }
 +});
 +
++exports.Option = Option;
++
 +/**
 + * Placeholder for return values in a response template.
 + *
 + * @param type type
 + *    The return value should be marshalled as this type.
 + * @param object options
 + *    Argument options:
 + *      optional: true if the argument can be undefined or null.
 + */
-+exports.RetVal = Class({
++let RetVal = Class({
 +  initialize: function(type, options={}) {
 +    this.type = types.getType(type);
 +    this.optional = !!options.optional;
 +  },
 +
 +  write: function(v, ctx) {
-+    if (typeof(v) != "undefined" && v != null) {
++    if (v !== undefined && v != null) {
 +      return this.type.write(v, ctx);
 +    }
 +    if (!this.optional) {
 +      throw Error("Return value not specified.");
 +    }
 +    return v;
 +  },
 +
 +  read: function(v, ctx) {
-+    if (typeof(v) != "undefined" && v != null) {
++    if (v !== undefined && v != null) {
 +      return this.type.read(v, ctx);
 +    }
 +    if (!this.optional) {
 +      throw Error("Return value not specified.");
 +    }
 +    return v;
 +  }
 +});
 +
-+function cloneRetVal(other) {
-+  let cloned = new exports.RetVal(other.type);
-+  cloned.optional = other.optional;
-+  return cloned;
-+}
++exports.RetVal = RetVal;
 +
 +/* Template handling functions */
 +
 +/**
-+ * Clone a template json object.
-+ */
-+function cloneTemplate(template) {
-+  // Better way to do this?
-+  return JSON.parse(JSON.stringify(template));
-+}
-+
-+/**
 + * Get the value at a given path, or undefined if not found.
 + */
 +function getPath(obj, path) {
 +  for (let name of path) {
 +    if (!(name in obj)) {
 +      return undefined;
 +    }
 +    obj = obj[name];
 +  }
 +  return obj;
 +}
 +
 +/**
-+ * Set the value at a given path.  Will create
-+ * objects in the path if they're missing.
++ * Find Placeholders in the template and save them along with their
++ * paths.
 + */
-+function setPath(obj, path, v) {
-+  for (let i = 0; i < path.length - 1; i++) {
-+    let name = path[i];
-+    if (!(name in obj)) {
-+      obj[name] = {};
-+    }
-+    obj = obj[name];
++function findPlaceholders(template, constructor, path=[], placeholders=[]) {
++  if (!template || typeof(template) != "object") {
++    return placeholders;
++  }
++
++  if (template instanceof constructor) {
++    placeholders.push({ placeholder: template, path: [p for (p of path)] });
++    return placeholders;
 +  }
-+  obj[path[path.length - 1]] = v;
++
++  for (let name in template) {
++    path.push(name);
++    findPlaceholders(template[name], constructor, path, placeholders);
++    path.pop();
++  }
++
++  return placeholders;
 +}
 +
++
 +/**
 + * Manages a request template.
 + *
 + * @param object template
 + *    The request template.
 + * @construcor
 + */
 +let Request = Class({
 +  initialize: function(template={}) {
 +    this.type = template.type;
-+    this.args = [];
-+    this.template = this.processTemplate(template);
++    this.template = template;
++    this.args = findPlaceholders(template, Arg);
 +  },
 +
 +  /**
 +   * Write a request.
 +   *
 +   * @param array fnArgs
 +   *    The function arguments to place in the request.
 +   * @param object ctx
 +   *    The object making the request.
 +   * @returns a request packet.
 +   */
 +  write: function(fnArgs, ctx) {
-+    if (!this.template) {
-+      return {};
-+    }
-+    let packet = cloneTemplate(this.template);
-+    for (let templateArg of this.args) {
-+      setPath(packet, templateArg.path, templateArg.write(fnArgs[templateArg.index], ctx));
-+    }
-+    return packet;
++    let str = JSON.stringify(this.template, (key, value) => {
++      if (value instanceof Arg) {
++        return value.write(fnArgs[value.index], ctx, key);
++      }
++      return value;
++    });
++    return JSON.parse(str);
 +  },
 +
 +  /**
 +   * Read a request.
 +   *
 +   * @param object packet
 +   *    The request packet.
 +   * @param object ctx
 +   *    The object making the request.
 +   * @returns an arguments array
 +   */
 +  read: function(packet, ctx) {
 +    let fnArgs = [];
 +    for (let templateArg of this.args) {
-+      templateArg.read(getPath(packet, templateArg.path), ctx, fnArgs);
++      let arg = templateArg.placeholder;
++      let path = templateArg.path;
++      let name = path[path.length - 1];
++      arg.read(getPath(packet, path), ctx, fnArgs, name);
 +    }
 +    return fnArgs;
 +  },
-+
-+  /**
-+   * Create a clone of the request template, saving any Arg instances
-+   * found in the template along with their path.
-+   */
-+  processTemplate: function(template, path=[]) {
-+    if (!template || typeof(template) != "object") {
-+      return template;
-+    }
-+
-+    if (template instanceof exports.Arg) {
-+      let arg = cloneArg(template);
-+      arg.name = path[path.length - 1];
-+      arg.path = [p for (p of path)];
-+      this.args.push(arg);
-+      return undefined;
-+    }
-+
-+    let newTemplate = {};
-+    for (let name in template) {
-+      path.push(name);
-+      newTemplate[name] = this.processTemplate(template[name], path);
-+      path.pop();
-+    }
-+    return newTemplate;
-+  },
 +});
 +
 +/**
 + * Manages a response template.
 + *
 + * @param object template
 + *    The response template.
 + * @construcor
 + */
 +let Response = Class({
 +  initialize: function(template={}) {
-+    // This will set this.path and this.retVal
-+    this.template = this.processTemplate(template);
++    this.template = template;
++    let placeholders = findPlaceholders(template, RetVal);
++    if (placeholders.length > 1) {
++      throw Error("More than one RetVal specified in response");
++    }
++    let placeholder = placeholders.shift();
++    if (placeholder) {
++      this.retVal = placeholder.placeholder;
++      this.path = placeholder.path;
++    }
 +  },
 +
 +  /**
 +   * Write a response for the given return value.
 +   *
 +   * @param val ret
 +   *    The return value.
 +   * @param object ctx
 +   *    The object writing the response.
 +   */
 +  write: function(ret, ctx) {
-+    if (!this.retVal) {
-+      return {};
-+    }
-+    let val = this.retVal.write(ret, ctx);
-+    if (this.path.length === 0) {
-+      return val;
-+    }
-+    let packet = cloneTemplate(this.template);
-+    setPath(packet, this.path, val);
-+    return packet;
++    return JSON.parse(JSON.stringify(this.template, function(key, value) {
++      if (value instanceof RetVal) {
++        return value.write(ret, ctx);
++      }
++      return value;
++    }));
 +  },
 +
 +  /**
 +   * Read a return value from the given response.
 +   *
 +   * @param object packet
 +   *    The response packet.
 +   * @param object ctx
 +   *    The object reading the response.
 +   */
 +  read: function(packet, ctx) {
 +    if (!this.retVal) {
 +      return undefined;
 +    }
-+
 +    let v = getPath(packet, this.path);
 +    return this.retVal.read(v, ctx);
-+  },
-+
-+  /**
-+   * Create a clone of the response template, saving the RetVal
-+   * instance in the template along with its path.
-+   */
-+  processTemplate: function(template, path=[]) {
-+    if (typeof(template) != "object") {
-+      return template;
-+    }
-+    if (template instanceof exports.RetVal) {
-+      this.retVal = cloneRetVal(template);
-+      this.path = [p for (p of path)];
-+      return undefined;
-+    }
-+    let newTemplate = {};
-+    for (let name in template) {
-+      path.push(name);
-+      newTemplate[name] = this.processTemplate(template[name], path);
-+      path.pop();
-+    }
-+    return newTemplate;
 +  }
 +});
 +
 +/**
 + * Actor and Front implementations
 + */
 +
 +/**
@@ -1140,17 +1095,17 @@ new file mode 100644
 +  /**
 +   * Remove an actor as a child of this pool.
 +   */
 +  unmanage: function(actor) {
 +    this.__poolMap.delete(actor.actorID);
 +  },
 +
 +  // true if the given actor ID exists in the pool.
-+  has: function(actorID) this._poolMap.has(actorID),
++  has: function(actorID) this._poolMap && this._poolMap.has(actorID),
 +
 +  // The actor for a given actor id stored in this pool
 +  actor: function(actorID) this._poolMap.get(actorID),
 +
 +  // Same as actor, should update debugger connection to use 'actor'
 +  // and then remove this.
 +  get: function(actorID) this._poolMap.get(actorID),
 +
@@ -1164,28 +1119,28 @@ new file mode 100644
 +  destroy: function() {
 +    let parent = this.parent();
 +    if (parent) {
 +      parent.unmanage(this);
 +    }
 +    if (!this.__poolMap) {
 +      return;
 +    }
-+    for (actor in this.__poolMap) {
++    for (let actor of this.__poolMap.values()) {
 +      let destroy = actor.destroy;
 +      if (destroy) {
 +        // Disconnect destroy while we're destroying in case of self-owned
 +        // or (misbehaving) circular ownership.
 +        actor.destroy = null;
 +        destroy.call(actor);
 +        actor.destroy = destroy;
 +      }
 +    };
 +    this.conn.removeActorPool(this);
-+    this._poolMap.clear();
++    this.__poolMap.clear();
 +    this.__poolMap = null;
 +  },
 +
 +  /**
 +   * For getting along with the debugger server pools, should be removable
 +   * eventually.
 +   */
 +  cleanup: function() {
@@ -1212,30 +1167,31 @@ new file mode 100644
 +   *   conn can be null if the subclass provides a conn property.
 +   * @constructor
 +   */
 +  initialize: function(conn) {
 +    Pool.prototype.initialize.call(this, conn);
 +
 +    // Forward events to the connection.
 +    if (this._actorSpec && this._actorSpec.events) {
-+      Object.getOwnPropertyNames(this._actorSpec.events).forEach((name) => {
++      for (let key of this._actorSpec.events.keys()) {
++        let name = key;
 +        this.on(name, (...args) => {
 +          this._sendEvent.apply(this, [name].concat(args));
 +        });
-+      });
++      }
 +    }
 +  },
 +
 +  _sendEvent: function(name, ...args) {
-+    if (!(name in this._actorSpec.events)) {
++    if (!this._actorSpec.events.has(name)) {
 +      // It's ok to emit events that don't go over the wire.
 +      return;
 +    }
-+    let request = this._actorSpec.events[name];
++    let request = this._actorSpec.events.get(name);
 +    let packet = request.write(args, this);
 +    packet.from = packet.from || this.actorID;
 +    this.conn.send(packet);
 +  },
 +
 +  destroy: function() {
 +    Pool.prototype.destroy.call(this);
 +    this.actorID = null;
@@ -1316,21 +1272,21 @@ new file mode 100644
 +      spec.oneway = frozenSpec.oneway;
 +
 +      protoSpec.methods.push(spec);
 +    }
 +  }
 +
 +  // Find event specifications
 +  if (actorProto.events) {
-+    protoSpec.events = {};
++    protoSpec.events = new Map();
 +    for (let name in actorProto.events) {
 +      let eventRequest = actorProto.events[name];
 +      Object.freeze(eventRequest);
-+      protoSpec.events[name] = Request(object.merge({type: name}, eventRequest));
++      protoSpec.events.set(name, Request(object.merge({type: name}, eventRequest)));
 +    }
 +  }
 +
 +  // Generate request handlers for each method definition
 +  actorProto.requestTypes = Object.create(null);
 +  protoSpec.methods.forEach(spec => {
 +    let handler = function(packet, conn) {
 +      try {
@@ -1379,17 +1335,17 @@ new file mode 100644
 + *    The object prototype.  Must have a 'typeName' property,
 + *    should have method definitions, can have event definitions.
 + */
 +exports.ActorClass = function(proto) {
 +  if (!proto.typeName) {
 +    throw Error("Actor prototype must have a typeName member.");
 +  }
 +  proto.extends = Actor;
-+  if (!(proto.typeName in registeredTypes)) {
++  if (!registeredTypes.has(proto.typeName)) {
 +    types.addActorType(proto.typeName);
 +  }
 +  let cls = Class(exports.actorProto(proto));
 +  types.addActorImpl(proto.typeName, cls);
 +  return cls;
 +};
 +
 +/**
@@ -1463,18 +1419,18 @@ new file mode 100644
 +    return deferred.promise;
 +  },
 +
 +  /**
 +   * Handler for incoming packets from the client's actor.
 +   */
 +  onPacket: function(packet) {
 +    // Pick off event packets
-+    if (this._clientSpec.events && packet.type in this._clientSpec.events) {
-+      let event = this._clientSpec.events[packet.type];
++    if (this._clientSpec.events && this._clientSpec.events.has(packet.type)) {
++      let event = this._clientSpec.events.get(packet.type);
 +      let args = event.request.read(packet, this);
 +      (event.pre || []).forEach((pre) => pre.apply(this, args));
 +      events.emit.apply(null, [this, event.name].concat(args));
 +      return;
 +    }
 +
 +    // Remaining packets must be responses.
 +    if (this._requests.length === 0) {
@@ -1536,17 +1492,17 @@ new file mode 100644
 +  let methods = proto._actorSpec.methods;
 +  methods.forEach(spec => {
 +    let name = spec.name;
 +
 +    // If there's already a property by this name in the front, it must
 +    // be a custom front method.
 +    if (name in proto) {
 +      let custom = proto[spec.name]._customFront;
-+      if (typeof(custom) === "undefined") {
++      if (custom === undefined) {
 +        throw Error("Existing method for " + spec.name + " not marked customFront while processing " + actorType.typeName + ".");
 +      }
 +      // If the user doesn't need the impl don't generate it.
 +      if (!custom.impl) {
 +        return;
 +      }
 +      name = custom.impl;
 +    }
@@ -1605,38 +1561,42 @@ new file mode 100644
 +
 +
 +  // Process event specifications
 +  proto._clientSpec = {};
 +
 +  let events = proto._actorSpec.events;
 +  if (events) {
 +    // This actor has events, scan the prototype for preEvent handlers...
-+    let preHandlers = Object.create(null);
++    let preHandlers = new Map();
 +    for (let name of Object.getOwnPropertyNames(proto)) {
 +      let desc = Object.getOwnPropertyDescriptor(proto, name);
 +      if (!desc.value) {
 +        continue;
 +      }
 +      if (desc.value._preEvent) {
 +        let preEvent = desc.value._preEvent;
-+        preHandlers[preEvent] = preHandlers[preEvent] || [];
-+        preHandlers[preEvent].push(desc.value);
++        let handlers = preHandlers.get(preEvent);
++        if (!handlers) {
++          handlers = [];
++          preHandlers.set(preEvent, handlers);
++        }
++        handlers.push(desc.value);
 +      }
 +    }
 +
-+    proto._clientSpec.events = Object.create(null);
++    proto._clientSpec.events = new Map();
 +
-+    for (let name in events) {
-+      let request = events[name]
-+      proto._clientSpec.events[request.type] = {
++    for (let name of events.keys()) {
++      let request = events.get(name);
++      proto._clientSpec.events.set(request.type, {
 +        name: name,
 +        request: request,
-+        pre: preHandlers[name]
-+      }
++        pre: preHandlers.get(name)
++      });
 +    }
 +
 +  }
 +  return proto;
 +}
 +
 +/**
 + * Create a front class for the given actor class, with the given prototype.
@@ -1674,17 +1634,17 @@ diff --git a/toolkit/devtools/server/tes
    } catch(ex) {
      return {throw: ex}
    }
  }
 diff --git a/toolkit/devtools/server/tests/unit/test_protocol_children.js b/toolkit/devtools/server/tests/unit/test_protocol_children.js
 new file mode 100644
 --- /dev/null
 +++ b/toolkit/devtools/server/tests/unit/test_protocol_children.js
-@@ -0,0 +1,368 @@
+@@ -0,0 +1,388 @@
 +/* Any copyright is dedicated to the Public Domain.
 +   http://creativecommons.org/publicdomain/zero/1.0/ */
 +
 +/**
 + * Test simple requests using the protocol helpers.
 + */
 +let protocol = devtools.require("devtools/server/protocol");
 +let {method, preEvent, types, Arg, Option, RetVal} = protocol;
@@ -1710,18 +1670,23 @@ new file mode 100644
 +  typeName: "childActor",
 +
 +  // Actors returned by this actor should be owned by the root actor.
 +  defaultParent: function() { return this.parent() },
 +
 +  toString: function() "[ChildActor " + this.childID + "]",
 +
 +  initialize: function(conn, id) {
++    protocol.Actor.prototype.initialize.call(this, conn);
 +    this.childID = id;
-+    protocol.Actor.prototype.initialize.call(this, conn);
++  },
++
++  destroy: function() {
++    protocol.Actor.prototype.destroy.call(this);
++    this.destroyed = true;
 +  },
 +
 +  form: function(detail) {
 +    return {
 +      actor: this.actorID,
 +      childID: this.childID,
 +      detail: detail
 +    };
@@ -1790,16 +1755,21 @@ new file mode 100644
 +  }
 +});
 +
 +let ChildFront = protocol.FrontClass(ChildActor, {
 +  initialize: function(client, form) {
 +    protocol.Front.prototype.initialize.call(this, client, form);
 +  },
 +
++  destroy: function() {
++    this.destroyed = true;
++    protocol.Front.prototype.destroy.call(this);
++  },
++
 +  defaultParent: function() { return this.parent() },
 +
 +  toString: function() "[child front " + this.childID + "]",
 +
 +  form: function(form) {
 +    this.childID = form.childID;
 +    this.detail = form.detail;
 +  },
@@ -1961,27 +1931,37 @@ new file mode 100644
 +    }).then(ret => {
 +      expectRootChildren(2);
 +    }).then(ret => {
 +      return rootFront.getTemporaryChild("temp1").then(temp1 => {
 +        // At this point we expect two direct children, plus the temporary holder
 +        // which should hold 1 itself.
 +        do_check_eq(rootActor._temporaryHolder.__poolMap.size, 1);
 +        do_check_eq(rootFront._temporaryHolder.__poolMap.size, 1);
++
 +        expectRootChildren(3);
 +        return rootFront.getTemporaryChild("temp2").then(temp2 => {
 +          // Same amount of direct children, and an extra in the temporary holder.
 +          expectRootChildren(3);
 +          do_check_eq(rootActor._temporaryHolder.__poolMap.size, 2);
 +          do_check_eq(rootFront._temporaryHolder.__poolMap.size, 2);
++
++          // Get the children of the temporary holder...
++          let checkActors = [entry[1] for (entry of rootActor._temporaryHolder.__poolMap)];
++          let checkFronts = [entry[1] for (entry of rootFront._temporaryHolder.__poolMap)];
++
 +          // Now release the temporary holders and expect them to drop again.
 +          return rootFront.clearTemporaryChildren().then(() => {
 +            expectRootChildren(2);
 +            do_check_false(!!rootActor._temporaryHolder);
 +            do_check_false(!!rootFront._temporaryHolder);
++            for (let checkActor of checkActors) {
++              do_check_true(checkActor.destroyed);
++              do_check_true(checkActor.destroyed);
++            }
 +          });
 +        });
 +      })
 +    }).then(ret => {
 +      return rootFront.getChildren(["child1", "child2"]);
 +    }).then(ret => {
 +      expectRootChildren(3);
 +      do_check_true(ret[0] === childFront);
@@ -2212,17 +2192,17 @@ new file mode 100644
 +    });
 +  });
 +  do_test_pending();
 +}
 diff --git a/toolkit/devtools/server/tests/unit/test_protocol_simple.js b/toolkit/devtools/server/tests/unit/test_protocol_simple.js
 new file mode 100644
 --- /dev/null
 +++ b/toolkit/devtools/server/tests/unit/test_protocol_simple.js
-@@ -0,0 +1,224 @@
+@@ -0,0 +1,228 @@
 +/* Any copyright is dedicated to the Public Domain.
 +   http://creativecommons.org/publicdomain/zero/1.0/ */
 +
 +/**
 + * Test simple requests using the protocol helpers.
 + */
 +
 +let protocol = devtools.require("devtools/server/protocol");
@@ -2401,16 +2381,20 @@ new file mode 100644
 +      do_check_eq(ret.c, 3);
 +      return rootClient.optionArgs({
 +        "option1": 5,
 +        "option2": 10
 +      });
 +    }).then(ret => {
 +      do_check_eq(ret.option1, 5);
 +      do_check_eq(ret.option2, 10);
++      return rootClient.optionArgs({});
++    }).then(ret => {
++      do_check_true(typeof(ret.option1) === "undefined");
++      do_check_true(typeof(ret.option2) === "undefined");
 +      // Explicitly call an optional argument...
 +      return rootClient.optionalArgs(5, 10);
 +    }).then(ret => {
 +      do_check_eq(ret, 10);
 +      // Now don't pass the optional argument, expect the default.
 +      return rootClient.optionalArgs(5);
 +    }).then(ret => {
 +      do_check_eq(ret, 200);
--- a/series
+++ b/series
@@ -1,27 +1,27 @@
 register-actors.diff
 transport-null-other.diff
 protocol-helpers.diff
-protocol-helpers-reviewcomments.diff
 inspector-actor.diff
 inspector-pathtoroot.diff
 inspector-node-tree.diff
 inspector-simple-mutations.diff
 inspector-childlist-mutations.diff
 inspector-page-navigation.diff
 inspector-drop-orphans.diff
+inspector-queue-mutations.diff
+inspector-retain.diff
 inspector-sibling-traversal.diff
 inspector-actor-pseudoclass.diff
 target-inspector-changes.diff
 inspector-front-islocal.diff
 inspector-emit-on-destroy.diff
 inspector-innerhtml.diff
 inspector-set-attributes.diff
 inspector-set-nodevalue.diff
-inspector-retain.diff
 inspector-remote-target.diff
 inspector-remote-breadcrumbs.diff
 inspector-remote-pseudoclass.diff
 search-box-remote.diff
 remote-markup.diff
 inspector-retain-root.diff #+obsolete
 protocol-clientserver-marshallers.diff #+experimental