Andreas Tolfsen ❲:ato❳ Assignee
	Description • 6 years ago

Most of the arguments for GeckoDriver#executeScript and
#executeAsyncScript are missing type checks.

This is a follow-up from https://bugzil.la/1329184.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 2 • 6 years ago
mozreview-review

Comment on attachment 8955476 [details]
Bug 1442543 - Add type checks for WebDriver:{ExecuteScript,ExecuteAsyncScript}.

https://reviewboard.mozilla.org/r/224650/#review231178

::: testing/marionette/driver.js:976
(Diff revision 1)
> +      newSandbox = false,
> +      file = "",
> +      line = 0,
> +      debug = false,
> +      async = false,
> +    } = {}) {

You know my position on this styling. It completely breaks the an easy understanding of the function parameter list. Also not sure how the Js documentation will look like.

IMHO we should really find something better.

::: testing/marionette/driver.js:984
(Diff revision 1)
> +    timeout = this.timeouts.script;
> +  }
> +
> +  assert.open(this.getCurrentWindow());
> +
> +  assert.string(script, pprint`Expected script to be a string: ${script}`);

Can we put quotes around the actual parameter name as we do in session.js? This reads hard.

::: testing/marionette/driver.js:986
(Diff revision 1)
> +
> +  assert.open(this.getCurrentWindow());
> +
> +  assert.string(script, pprint`Expected script to be a string: ${script}`);
> +  assert.array(args, pprint`Expected script args to be an array: ${args}`);
> +  assert.positiveInteger(timeout, pprint`Expected script timeout to be an integer: ${timeout}`);

`to be a positive integer`.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 3 • 6 years ago
mozreview-review-reply

Comment on attachment 8955476 [details]
Bug 1442543 - Add type checks for WebDriver:{ExecuteScript,ExecuteAsyncScript}.

https://reviewboard.mozilla.org/r/224650/#review231178

> You know my position on this styling. It completely breaks the an easy understanding of the function parameter list. Also not sure how the Js documentation will look like.
> 
> IMHO we should really find something better.

“[C]ompletely breaks” are strong words.  The problem is that
the argument list is so long that it isn’t easily formatting in
any sensible way.

See my proposal in https://bugzilla.mozilla.org/show_bug.cgi?id=1330309#c3
which could help improve this in the future.

	Pulsebot
	Comment 5 • 6 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b57bed48434f
Add type checks for WebDriver:{ExecuteScript,ExecuteAsyncScript}. r=whimboo

	Dorel Luca [:dluca]
	Comment 6 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/b57bed48434f