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 676761 ebad552e29c7e1aa27ef3b52201761380dd0fe04
parent 676760 4d2d091759054721e6276942bd1dc0bffb923f19
child 735062 c09f474fa6b03cb23d374b6283181b73a3120514
push id83632
push userbmo:ato@sny.no
push dateMon, 09 Oct 2017 17:09:49 +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
@@ -233,35 +233,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:
@@ -279,23 +281,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;
         }
@@ -310,17 +310,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
@@ -77,17 +77,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
@@ -43,34 +43,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), DEFAULT_TIMEOUT);
 });
 
 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);
 });