Closed
Bug 1442543
Opened 6 years ago
Closed 6 years ago
Add type checks to WebDriver:{ExecuteScript,ExecuteAsyncScript}
Categories
(Remote Protocol :: Marionette, enhancement)
Remote Protocol
Marionette
Tracking
(firefox60 fixed)
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: ato, Assigned: ato)
Details
Attachments
(1 file)
Most of the arguments for GeckoDriver#executeScript and #executeAsyncScript are missing type checks. This is a follow-up from https://bugzil.la/1329184.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → ato
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
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`.
Attachment #8955476 -
Flags: review?(hskupin) → review+
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.
Comment hidden (mozreview-request) |
Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b57bed48434f Add type checks for WebDriver:{ExecuteScript,ExecuteAsyncScript}. r=whimboo
Comment 6•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b57bed48434f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•6 years ago
|
status-firefox60:
--- → fixed
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•