Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Description • 6 years ago

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.

	Andreas Tolfsen ❲:ato❳
	Comment 4 • 6 years ago
mozreview-review

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.

	Andreas Tolfsen ❲:ato❳
	Comment 5 • 6 years ago
mozreview-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?

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

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

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

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

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!

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

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?

	Andreas Tolfsen ❲:ato❳
	Comment 10 • 6 years ago
mozreview-review

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

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

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

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.

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

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.

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

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.

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

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.

	Pulsebot
	Comment 18 • 6 years ago

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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 19 • 6 years ago
bugherder

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