Optional command arguments should be passed-through without setting a default

RESOLVED FIXED in Firefox 64

Status

enhancement
P2
normal
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

(Depends on 1 bug)

63 Branch
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 wontfix, firefox64 fixed)

Details

Attachments

(2 attachments)

As we noticed for a couple commands geckodriver sets default values for arguments if those are not present. Instead it should ignore those and pass-through transparently to Marionette.

That would avoid us from having to maintain multiple places for default values.
Currently it looks like that only SetWindowRect, ExecuteScript, and ExecuteAsyncScript seem to send null or no longer needed values to Marionette. All other Webdriver commands are using Optional as expected.
Priority: P1 → P2
The argument "specialPowers" is no longer used in Marionette
and as such can be removed from the serialization.

The default value for "newSandbox" in Marionette is false,
so it doesn't need to be explicitely set in geckodriver.

By removing those two properties a custom serialization
is no longer neccessary.

Depends on D7210
Andreas, I already uploaded the patch with a review for you. In case it looks all fine and gets your r+ maybe you can land it if you are around at the weekend. Thanks.
Comment on attachment 9012970 [details]
Bug 1494637 - [geckodriver] Only serialize non null arguments for Marionette. r?ato

Andreas Tolfsen ❲:ato❳ has approved the revision.
Attachment #9012970 - Flags: review+
Comment on attachment 9012972 [details]
Bug 1494637 - [geckodriver] Remove custom serialization of JavascriptCommandParameters for Marionette. r?ato

Andreas Tolfsen ❲:ato❳ has approved the revision.
Attachment #9012972 - Flags: review+
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/77cf6aa75d7e
[geckodriver] Only serialize non null arguments for Marionette. r=ato
https://hg.mozilla.org/mozilla-central/rev/77cf6aa75d7e
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Actually those are only cosmetic changes. As such I don't think that we have to uplift this patch.
Attachment #9012972 - Attachment is obsolete: true
So I actually missed to land the second commit of this patch which now has been made obsolete. Andreas, did you include it somewhere else? If not I will file a new bug to get it landed.
Flags: needinfo?(ato)
Maybe Phabricator lets you push the missing commit with Lando?  It
should be possible to re-open (“reclaim” in Phabricator vernacular)
a review if Lando doesn’t work on obsoleted diffs.  Otherwise pushing
to inbound with this same bug number works too.
Flags: needinfo?(ato)
Attachment #9012972 - Attachment is obsolete: false
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7b5cc9b80a3e
[geckodriver] Remove custom serialization of JavascriptCommandParameters for Marionette. r=ato
You need to log in before you can comment on or make changes to this bug.