Closed Bug 1452024 Opened Last year Closed Last year

Update geckodriver/webdriver commands for "WebDriver" prefix

Categories

(Testing :: geckodriver, enhancement, P1)

enhancement

Tracking

(firefox60 wontfix, firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(4 files)

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.
Depends on: 1451727
Attachment #8966965 - Flags: review?(ato)
Attachment #8966966 - Flags: review?(ato)
Attachment #8966967 - Flags: review?(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.
Attachment #8966965 - Flags: review?(ato) → review+
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?
Attachment #8966966 - Flags: review?(ato) → review+
Comment on attachment 8966967 [details]
Bug 1452024 - [geckodriver] Update vendor specific commands to use custom prefixes.

https://reviewboard.mozilla.org/r/235654/#review241418
Attachment #8966967 - Flags: review?(ato) → review+
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 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 on attachment 8967008 [details]
Bug 1452024 - [geckodriver] Update changelog for command prefix changes.

https://reviewboard.mozilla.org/r/235674/#review241458
Attachment #8967008 - Flags: review?(ato) → review+
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.
Flags: needinfo?(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.
Flags: needinfo?(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

> 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 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.
Pushed by hskupin@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.