1452024 - Update geckodriver/webdriver commands for "WebDriver" prefix

Status: RESOLVED FIXED

(Testing :: geckodriver, enhancement, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Similar to bug 1451727 we also have to update geckodriver/webdriver to use the new endpoints with the `WebDriver` prefix.

If possible we should still get this landed for Firefox 60, so we have it in the ESR release.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1452024 - [geckodriver] Sort webdriver command list.

Review commit: https://reviewboard.mozilla.org/r/235650/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235650/

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1452024 - [geckodriver] Update WebDriver specific commands to use the "WebDriver" prefix.

Review commit: https://reviewboard.mozilla.org/r/235652/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235652/

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1452024 - [geckodriver] Update vendor specific commands to use custom prefixes.

Commands which are not defined by the WebDriver spec have to
use the "Marionette" prefix. Exceptions are only allowed for
chrome specific commands which share the same request URL with
the content one.

Review commit: https://reviewboard.mozilla.org/r/235654/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235654/

Comment 4

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8966965 [details]
Bug 1452024 - [geckodriver] Sort webdriver command list.

https://reviewboard.mozilla.org/r/235650/#review241414

Almost impossible to tell if anything really changed here, but
assuming this is fine.

Comment 5

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8966966 [details]
Bug 1452024 - [geckodriver] Update WebDriver specific commands to use the "WebDriver" prefix.

https://reviewboard.mozilla.org/r/235652/#review241416

::: testing/geckodriver/src/marionette.rs:1016
(Diff revision 1)
>                                msg: &WebDriverMessage<GeckoExtensionRoute>)
>                                -> WebDriverResult<MarionetteCommand> {
>          let (opt_name, opt_parameters) = match msg.command {
>              Status => panic!("Got status command that should already have been handled"),
> -            AcceptAlert => (Some("acceptDialog"), None),
> -            AddCookie(ref x) => (Some("addCookie"), Some(x.to_marionette())),
> +            AcceptAlert => {
> +                // Needs to be updated to "WebDriver:AcceptAlert" for Firefox 63

You will want to highlight that geckodriver minimum Firefox version
has now changed in both README.md and CHANGES.md.

::: testing/geckodriver/src/marionette.rs:1147
(Diff revision 1)
> +            GoBack => {
> +                (Some("WebDriver:Back"), None)
> +            },

What is the point of the new scope here?  Is this something rustfmt
insists on?

Comment 6

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8966967 [details]
Bug 1452024 - [geckodriver] Update vendor specific commands to use custom prefixes.

https://reviewboard.mozilla.org/r/235654/#review241418

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8966965 [details]
Bug 1452024 - [geckodriver] Sort webdriver command list.

https://reviewboard.mozilla.org/r/235650/#review241414

Yes, I left all in. I promise!

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8966966 [details]
Bug 1452024 - [geckodriver] Update WebDriver specific commands to use the "WebDriver" prefix.

https://reviewboard.mozilla.org/r/235652/#review241416

> You will want to highlight that geckodriver minimum Firefox version
> has now changed in both README.md and CHANGES.md.

Sure. I will push a separate commit for that.

> What is the point of the new scope here?  Is this something rustfmt
> insists on?

I tried to follow what we have already here: https://dxr.mozilla.org/mozilla-central/rev/0a2dae2d8cf9f628c55668514c54a23da446d5de/testing/geckodriver/src/marionette.rs#1165-1170

Is that not necessary?

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1452024 - [geckodriver] Update changelog for command prefix changes.

Review commit: https://reviewboard.mozilla.org/r/235674/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235674/

Comment 10

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8967008 [details]
Bug 1452024 - [geckodriver] Update changelog for command prefix changes.

https://reviewboard.mozilla.org/r/235674/#review241458

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8966966 [details]
Bug 1452024 - [geckodriver] Update WebDriver specific commands to use the "WebDriver" prefix.

https://reviewboard.mozilla.org/r/235652/#review241416

> I tried to follow what we have already here: https://dxr.mozilla.org/mozilla-central/rev/0a2dae2d8cf9f628c55668514c54a23da446d5de/testing/geckodriver/src/marionette.rs#1165-1170
> 
> Is that not necessary?

rustfmt doesn't complain about it. Andreas, if you think I should get the scope removed from those commands which only have a single line please let me know. Otherwise we could get this landed as is.

Comment 12

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8966966 [details]
Bug 1452024 - [geckodriver] Update WebDriver specific commands to use the "WebDriver" prefix.

https://reviewboard.mozilla.org/r/235652/#review241416

> rustfmt doesn't complain about it. Andreas, if you think I should get the scope removed from those commands which only have a single line please let me know. Otherwise we could get this landed as is.

When I run this function through rustfmt, I end up without scopes/blocks.

This isn’t a blocker; we can fix the style at a later point.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8966966 [details]
Bug 1452024 - [geckodriver] Update WebDriver specific commands to use the "WebDriver" prefix.

https://reviewboard.mozilla.org/r/235652/#review241416

> When I run this function through rustfmt, I end up without scopes/blocks.
> 
> This isn’t a blocker; we can fix the style at a later point.

Hm, then I'm not sure what I'm doing wrong locally. I also installed the Sublime rustfmt plugin and don't see modifications. So let me see, given that I don't want to push code which cause us more reverting changes later and makes it hard for blame.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8966966 [details]
Bug 1452024 - [geckodriver] Update WebDriver specific commands to use the "WebDriver" prefix.

https://reviewboard.mozilla.org/r/235652/#review241416

> Hm, then I'm not sure what I'm doing wrong locally. I also installed the Sublime rustfmt plugin and don't see modifications. So let me see, given that I don't want to push code which cause us more reverting changes later and makes it hard for blame.

Hah, I got it working and will update the two commits for correct formatting. It looks like that we have to revert to the style as used before.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8966966 [details]
Bug 1452024 - [geckodriver] Update WebDriver specific commands to use the "WebDriver" prefix.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235652/diff/1-2/

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8966967 [details]
Bug 1452024 - [geckodriver] Update vendor specific commands to use custom prefixes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235654/diff/1-2/

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8967008 [details]
Bug 1452024 - [geckodriver] Update changelog for command prefix changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235674/diff/1-2/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17eb0cdbc7fc
[geckodriver] Sort webdriver command list. r=ato
https://hg.mozilla.org/integration/autoland/rev/0f6830a8e7c7
[geckodriver] Update WebDriver specific commands to use the "WebDriver" prefix. r=ato
https://hg.mozilla.org/integration/autoland/rev/722cbb7a90c3
[geckodriver] Update vendor specific commands to use custom prefixes. r=ato
https://hg.mozilla.org/integration/autoland/rev/679b1667e2eb
[geckodriver] Update changelog for command prefix changes. r=ato

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/17eb0cdbc7fc
https://hg.mozilla.org/mozilla-central/rev/0f6830a8e7c7
https://hg.mozilla.org/mozilla-central/rev/722cbb7a90c3
https://hg.mozilla.org/mozilla-central/rev/679b1667e2eb