Bug 1404946 - Have PollPromise accept an options dictionary. r?whimboo draft
authorAndreas Tolfsen <ato@sny.no>
Mon, 02 Oct 2017 17:13:57 +0100
changeset 673437 202f523c081e1ba8b8f1185a63681e39e7566253
parent 673436 7bc5480eb08346cc50dcc6cb1261ebba735d10e1
child 734106 d48c23a6b5b466095a4187c6d30087b837e74639
push id82580
push userbmo:ato@sny.no
push dateMon, 02 Oct 2017 16:15:39 +0000
reviewerswhimboo
bugs1404946
milestone58.0a1
Bug 1404946 - Have PollPromise accept an options dictionary. r?whimboo This patch moves the "timeout" and "interval" positional arguments on PollPromise to an options dictionary. In the following code example it is hard to know which argument means what because they are not named: new PollPromise(resolve => resolve(), 100, 100); Named arguments can be achieved in JavaScript using option dictionaries, and this patch changes the input PollPromise takes so that the above example looks like this: new PollPromise(resolve => resolve(), {timeout: 100, interval: 100}; This plays especially well with code in testing/marionette/element.js as we already have named arguments that we can pass directly in through an object literal, making the code more readable and more compact: let timeout = 42; new PollPromise(resolve => resolve(), {timeout}); MozReview-Commit-ID: GFWNGQAeWk1
testing/marionette/element.js
testing/marionette/sync.js
testing/marionette/test_sync.js
--- a/testing/marionette/element.js
+++ b/testing/marionette/element.js
@@ -240,35 +240,37 @@ element.Store = class {
  *     If <var>strategy</var> is unknown.
  * @throws InvalidSelectorError
  *     If <var>selector</var> is malformed.
  * @throws NoSuchElementError
  *     If a single element is requested, this error will throw if the
  *     element is not found.
  */
 element.find = function(container, strategy, selector, opts = {}) {
-  opts.all = !!opts.all;
-  opts.timeout = opts.timeout || 0;
+  let all = !!opts.all;
+  let timeout = opts.timeout || 0;
+  let startNode = opts.startNode;
 
   let searchFn;
   if (opts.all) {
     searchFn = findElements.bind(this);
   } else {
     searchFn = findElement.bind(this);
   }
 
   return new Promise((resolve, reject) => {
     let findElements = new PollPromise((resolve, reject) => {
-      let res = find_(container, strategy, selector, searchFn, opts);
+      let res = find_(container, strategy, selector, searchFn,
+          {all, startNode});
       if (res.length > 0) {
         resolve(Array.from(res));
       } else {
         reject([]);
       }
-    }, opts.timeout);
+    }, {timeout});
 
     findElements.then(foundEls => {
       // the following code ought to be moved into findElement
       // and findElements when bug 1254486 is addressed
       if (!opts.all && (!foundEls || foundEls.length == 0)) {
         let msg;
         switch (strategy) {
           case element.Strategy.AnonAttribute:
@@ -286,23 +288,21 @@ element.find = function(container, strat
       if (opts.all) {
         resolve(foundEls);
       }
       resolve(foundEls[0]);
     }, reject);
   });
 };
 
-function find_(container, strategy, selector, searchFn, opts) {
+function find_(container, strategy, selector, searchFn,
+    {startNode = null, all = false} = {}) {
   let rootNode = container.shadowRoot || container.frame.document;
-  let startNode;
 
-  if (opts.startNode) {
-    startNode = opts.startNode;
-  } else {
+  if (!startNode) {
     switch (strategy) {
       // For anonymous nodes the start node needs to be of type
       // DOMElement, which will refer to :root in case of a DOMDocument.
       case element.Strategy.Anon:
       case element.Strategy.AnonAttribute:
         if (rootNode instanceof Ci.nsIDOMDocument) {
           startNode = rootNode.documentElement;
         }
@@ -317,17 +317,17 @@ function find_(container, strategy, sele
   try {
     res = searchFn(strategy, selector, rootNode, startNode);
   } catch (e) {
     throw new InvalidSelectorError(
         `Given ${strategy} expression "${selector}" is invalid: ${e}`);
   }
 
   if (res) {
-    if (opts.all) {
+    if (all) {
       return res;
     }
     return [res];
   }
   return [];
 }
 
 /**
--- a/testing/marionette/sync.js
+++ b/testing/marionette/sync.js
@@ -76,17 +76,17 @@ const {TYPE_ONE_SHOT, TYPE_REPEATING_SLA
  *
  * @return {Promise.<*>}
  *     Yields the value passed to <var>func</var>'s
  *     <code>resolve</code> or <code>reject</code> callbacks.
  *
  * @throws {*}
  *     If <var>func</var> throws, its error is propagated.
  */
-function PollPromise(func, timeout = 2000, interval = 10) {
+function PollPromise(func, {timeout = 2000, interval = 10} = {}) {
   const timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
 
   return new Promise((resolve, reject) => {
     const start = new Date().getTime();
     const end = start + timeout;
 
     let evalFn = () => {
       new Promise(func).then(resolve, rejected => {
--- a/testing/marionette/test_sync.js
+++ b/testing/marionette/test_sync.js
@@ -41,34 +41,34 @@ add_task(async function test_PollPromise
 
 add_task(async function test_PollPromise_noTimeout() {
   // run at least once when timeout is 0
   let nevals = 0;
   let start = new Date().getTime();
   await new PollPromise((resolve, reject) => {
     ++nevals;
     reject();
-  }, 0);
+  }, {timeout: 0});
   let end = new Date().getTime();
   equal(1, nevals);
   less((end - start), 2000);
 });
 
 add_task(async function test_PollPromise_timeout() {
   let nevals = 0;
   let start = new Date().getTime();
   await new PollPromise((resolve, reject) => {
     ++nevals;
     reject();
-  }, 100);
+  }, {timeout: 100});
   let end = new Date().getTime();
   greater(nevals, 1);
   greaterOrEqual((end - start), 100);
 });
 
 add_task(async function test_PollPromise_interval() {
   let nevals = 0;
   await new PollPromise((resolve, reject) => {
     ++nevals;
     reject();
-  }, 100, 100);
+  }, {timeout: 100, interval: 100});
   equal(2, nevals);
 });