Bug 896270 - Replace optional arguments with nullable types in protocol.js, part 2. r=jimb
authorDave Camp <dcamp@mozilla.com>
Sun, 21 Jul 2013 10:38:40 -0700
changeset 140251 bbb8b9bf2578384b389264f33eaca9ce6f934b47
parent 140250 dbaacbf32f6c7eac6a83f079640990fe04df5578
child 140252 caddb1d95e348d4c1fa96c4e3c0b80968586ffa0
push id25021
push userryanvm@gmail.com
push dateSun, 28 Jul 2013 01:51:56 +0000
treeherdermozilla-central@015135250e06 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjimb
bugs896270
milestone25.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 896270 - Replace optional arguments with nullable types in protocol.js, part 2. r=jimb
toolkit/devtools/server/actors/inspector.js
toolkit/devtools/server/protocol.js
toolkit/devtools/server/tests/unit/test_protocol_simple.js
--- a/toolkit/devtools/server/actors/inspector.js
+++ b/toolkit/devtools/server/actors/inspector.js
@@ -591,18 +591,18 @@ var NodeListActor = exports.NodeListActo
       this.walker.ensurePathToRoot(item, newParents);
     }
     return {
       nodes: items,
       newParents: [node for (node of newParents)]
     }
   }, {
     request: {
-      start: Arg(0, "number", { optional: true }),
-      end: Arg(1, "number", { optional: true })
+      start: Arg(0, "nullable:number"),
+      end: Arg(1, "nullable:number")
     },
     response: RetVal("disconnectedNodeArray")
   }),
 
   release: method(function() {}, { release: true })
 });
 
 /**
@@ -659,17 +659,17 @@ let nodeArrayMethod = {
 };
 
 let traversalMethod = {
   request: {
     node: Arg(0, "domnode"),
     whatToShow: Option(1)
   },
   response: {
-    node: RetVal("domnode", {optional: true})
+    node: RetVal("nullable:domnode")
   }
 }
 
 /**
  * We need to know when a document is navigating away so that we can kill
  * the nodes underneath it.  We also need to know when a document is
  * navigated to so that we can send a mutation event for the iframe node.
  *
@@ -834,32 +834,32 @@ var WalkerActor = protocol.ActorClass({
    * @param NodeActor node
    *        The node whose document is needed, or null to
    *        return the root.
    */
   document: method(function(node) {
     let doc = node ? nodeDocument(node.rawNode) : this.rootDoc;
     return this._ref(doc);
   }, {
-    request: { node: Arg(0, "domnode", {optional: true}) },
+    request: { node: Arg(0, "nullable:domnode") },
     response: { node: RetVal("domnode") },
   }),
 
   /**
    * Return the documentElement for the document containing the
    * given node.
    * @param NodeActor node
    *        The node whose documentElement is requested, or null
    *        to use the root document.
    */
   documentElement: method(function(node) {
     let elt = node ? nodeDocument(node.rawNode).documentElement : this.rootDoc.documentElement;
     return this._ref(elt);
   }, {
-    request: { node: Arg(0, "domnode", {optional: true}) },
+    request: { node: Arg(0, "nullable:domnode") },
     response: { node: RetVal("domnode") },
   }),
 
   /**
    * Return all parents of the given node, ordered from immediate parent
    * to root.
    * @param NodeActor node
    *    The node whose parents are requested.
@@ -1352,17 +1352,17 @@ var WalkerActor = protocol.ActorClass({
       for (let locked of this._activePseudoClassLocks) {
         DOMUtils.clearPseudoClassLocks(locked.rawNode);
         this._activePseudoClassLocks.delete(locked);
         this._queuePseudoClassMutation(locked);
       }
     }
   }, {
     request: {
-      node: Arg(0, "domnode", { optional: true }),
+      node: Arg(0, "nullable:domnode")
     },
     response: {}
   }),
 
   /**
    * Get a node's innerHTML property.
    */
   innerHTML: method(function(node) {
@@ -1407,30 +1407,30 @@ var WalkerActor = protocol.ActorClass({
       // Mutation events will take care of the rest.
     }
     return nextSibling;
   }, {
     request: {
       node: Arg(0, "domnode")
     },
     response: {
-      nextSibling: RetVal("domnode", { optional: true })
+      nextSibling: RetVal("nullable:domnode")
     }
   }),
 
   /**
    * Insert a node into the DOM.
    */
   insertBefore: method(function(node, parent, sibling) {
     parent.rawNode.insertBefore(node.rawNode, sibling ? sibling.rawNode : null);
   }, {
     request: {
       node: Arg(0, "domnode"),
       parent: Arg(1, "domnode"),
-      sibling: Arg(2, "domnode", { optional: true })
+      sibling: Arg(2, "nullable:domnode")
     },
     response: {}
   }),
 
   /**
    * Get any pending mutation records.  Must be called by the client after
    * the `new-mutations` notification is received.  Returns an array of
    * mutation records.
--- a/toolkit/devtools/server/protocol.js
+++ b/toolkit/devtools/server/protocol.js
@@ -103,16 +103,27 @@ types.getType = function(type) {
     require("devtools/server/actors/string");
     return registeredTypes.get("longstring");
   }
 
   throw Error("Unknown type: " + type);
 }
 
 /**
+ * Don't allow undefined when writing primitive types to packets.  If
+ * you want to allow undefined, use a nullable type.
+ */
+function identityWrite(v) {
+  if (v === undefined) {
+    throw Error("undefined passed where a value is required");
+  }
+  return v;
+}
+
+/**
  * 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.
  *
@@ -133,18 +144,18 @@ types.getType = function(type) {
 types.addType = function(name, typeObject={}, options={}) {
   if (registeredTypes.has(name)) {
     throw Error("Type '" + name + "' already exists.");
   }
 
   let type = object.merge({
     name: name,
     primitive: !(typeObject.read || typeObject.write),
-    read: v => v,
-    write: v => v
+    read: identityWrite,
+    write: identityWrite
   }, typeObject);
 
   registeredTypes.set(name, type);
 
   if (!options.thawed) {
     Object.freeze(type);
   }
 
@@ -379,42 +390,29 @@ types.JSON = types.addType("json");
 
 /**
  * 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
  */
 let Arg = Class({
-  initialize: function(index, type, options={}) {
+  initialize: function(index, type) {
     this.index = index;
     this.type = types.getType(type);
-    this.optional = !!options.optional;
   },
 
   write: function(arg, ctx) {
-    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 (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.
@@ -461,44 +459,28 @@ let Option = Class({
 
 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.
  */
 let RetVal = Class({
-  initialize: function(type, options={}) {
+  initialize: function(type) {
     this.type = types.getType(type);
-    this.optional = !!options.optional;
   },
 
   write: function(v, ctx) {
-    if (v !== undefined && v != null) {
-      return this.type.write(v, ctx);
-    }
-    if (!this.optional) {
-      throw Error("Return value not specified.");
-    }
-    return v;
+    return this.type.write(v, ctx);
   },
 
   read: function(v, ctx) {
-    if (v !== undefined && v != null) {
-      return this.type.read(v, ctx);
-    }
-    if (!this.optional) {
-      throw Error("Return value not specified.");
-    }
-    return v;
+    return this.type.read(v, ctx);
   }
 });
 
 exports.RetVal = RetVal;
 
 /* Template handling functions */
 
 /**
--- a/toolkit/devtools/server/tests/unit/test_protocol_simple.js
+++ b/toolkit/devtools/server/tests/unit/test_protocol_simple.js
@@ -76,17 +76,17 @@ let RootActor = protocol.ActorClass({
     response: RetVal()
   }),
 
   optionalArgs: method(function(a, b=200) {
     return b;
   }, {
     request: {
       a: Arg(0),
-      b: Arg(1, "number", { optional: true })
+      b: Arg(1, "nullable:number")
     },
     response: {
       value: RetVal("number")
     },
   }),
 
   arrayArgs: method(function(a) {
     return a;
@@ -183,16 +183,21 @@ function run_test()
       do_check_eq(ret, 1);
     }).then(() => {
       return rootClient.promiseReturn();
     }).then(ret => {
       trace.expectSend({"type":"promiseReturn","to":"<actorid>"});
       trace.expectReceive({"value":1,"from":"<actorid>"});
       do_check_eq(ret, 1);
     }).then(() => {
+      // Missing argument should throw an exception
+      check_except(() => {
+        rootClient.simpleArgs(5);
+      });
+
       return rootClient.simpleArgs(5, 10)
     }).then(ret => {
       trace.expectSend({"type":"simpleArgs","firstArg":5,"secondArg":10,"to":"<actorid>"});
       trace.expectReceive({"firstResponse":6,"secondResponse":11,"from":"<actorid>"});
       do_check_eq(ret.firstResponse, 6);
       do_check_eq(ret.secondResponse, 11);
     }).then(() => {
       return rootClient.nestedArgs(1, 2, 3);