Update Marionette client for new WebDriver and browser vendor commands

RESOLVED FIXED in Firefox 60

Status

enhancement
P1
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 wontfix, firefox60 fixed, firefox61 fixed)

Details

Attachments

(5 attachments)

With bug 1451725 we want to remove all the old WebDriver commands. We cannot do that as long as the client doesn't call those.
Hm, so we also use the "WebDriver" prefix for vendor specific features like "GetCurrentChromeWindowHandle"? I thought we want to use "Marionette" for those. Andreas, given that you made that change, can you please explain?
Flags: needinfo?(ato)
That should indeed probably be in the Marionette namespace.

Better still, it should be removed in favour of WebDriver:GetWindowHandle,
but because we haven’t properly segregated chrome- and content
context we can’t really do that yet.
Flags: needinfo?(ato)
Great. So I will get this fixed in my upcoming patch.
Attachment #8965644 - Flags: review?(ato)
Attachment #8965645 - Flags: review?(ato)
Attachment #8965646 - Flags: review?(ato)
Attachment #8965647 - Flags: review?(ato)
Attachment #8965648 - Flags: review?(ato)
Comment on attachment 8965644 [details]
Bug 1451727 - [marionette] Add "WebDriver:AcceptAlert" in favor of "WebDriver:AcceptDialog".

https://reviewboard.mozilla.org/r/234480/#review240144

::: commit-message-62c25:3
(Diff revision 1)
> +To be aligned with the spec the command has to be named
> +"WebDriver:AcceptAlert".

This is not a spec requirement per se, but consistency is good.

::: testing/marionette/driver.js:3527
(Diff revision 1)
>    "reftest:run": GeckoDriver.prototype.runReftest,
>    "reftest:teardown": GeckoDriver.prototype.teardownReftest,
>  
>    // WebDriver service
> -  "WebDriver:AcceptDialog": GeckoDriver.prototype.acceptDialog,
> +  "WebDriver:AcceptAlert": GeckoDriver.prototype.acceptDialog,
> +  "WebDriver:AcceptDialog": GeckoDriver.prototype.acceptDialog,  // deprecated, remove in Firefox 63

As far as I know nothing uses WebDriver:AcceptDialog yet, so it can
be dropped.
Attachment #8965644 - Flags: review?(ato) → review+
Comment on attachment 8965645 [details]
Bug 1451727 - [marionette] Update WebDriver specific commands to use the "WebDriver" prefix.

https://reviewboard.mozilla.org/r/234482/#review240146

Do we not use the Python client with earlier Firefoxen such as 52
ESR for update tests?  If we don’t, this change is fine since the
namespaced commands were introduced in Firefox 56.
Attachment #8965645 - Flags: review?(ato) → review+
Comment on attachment 8965646 [details]
Bug 1451727 - [marionette] Update vendor specific commands to use custom prefixes.

https://reviewboard.mozilla.org/r/234484/#review240148

I suppose this is OK, but I am scared that lumping more things into
the Marionette:* namespace will blur the segregation between
WebDriver-esque behaviour in Marionette and the WebDriver module.
When we introduce more effective command modularisation to Marionette
in bug 1330309 the idea is for behaviour that is WebDriver related
to be separated from other remote protocol features that don’t share
this state.  Some of the commands that this patch moves to the
Marionette:*  namespace shares state with WebDriver behaviour, and
I worry that this will be a problem unless we find an effective way
to remove the non-standard commands soon.
Attachment #8965646 - Flags: review?(ato) → review+
Comment on attachment 8965647 [details]
Bug 1451727 - [marionette] Remove unused execute_js_script method.

https://reviewboard.mozilla.org/r/234486/#review240164
Attachment #8965647 - Flags: review?(ato) → review+
Comment on attachment 8965648 [details]
Bug 1451727 - [marionette] Remove deprecated WebDriver commands.

https://reviewboard.mozilla.org/r/234488/#review240168
Attachment #8965648 - Flags: review?(ato) → review+
Comment on attachment 8965644 [details]
Bug 1451727 - [marionette] Add "WebDriver:AcceptAlert" in favor of "WebDriver:AcceptDialog".

https://reviewboard.mozilla.org/r/234480/#review240144

> As far as I know nothing uses WebDriver:AcceptDialog yet, so it can
> be dropped.

I can drop that but then we would have the following situations:

1) When I'm going to update marionette client, I have to add a fallback similar to the other chrome related commands
2) For geckodriver we would also need a fallback because this end-point will not work with earlier Firefox releases. Personally I would make use of `WebDriver:AcceptDialog` for now, and then update it to `WebDriver:AcceptAlert` in Firefox 63.
Comment on attachment 8965645 [details]
Bug 1451727 - [marionette] Update WebDriver specific commands to use the "WebDriver" prefix.

https://reviewboard.mozilla.org/r/234482/#review240146

We do not support update tests for ESR -> ESR_next. So Firefox 56 is the earliest release which would be tested for Firefox 59, but note that no-one currently runs the update tests.

Getting all that into Firefox 60 is fine.
Comment on attachment 8965646 [details]
Bug 1451727 - [marionette] Update vendor specific commands to use custom prefixes.

https://reviewboard.mozilla.org/r/234484/#review240148

I assume you mean all the chrome window specific commands? I would be happy if we could make the underlying code part of the official WebDriver commands, and just differentiate by the currently selected context. I would be happy to make those changes, if you think that should be quickly done.

In that case we might want to leave the old names, also for backward compatibility of geckodriver and Marionette. I would indeed be helpful.
(In reply to Henrik Skupin (:whimboo) from comment #17)

> I assume you mean all the chrome window specific commands? I would
> be happy if we could make the underlying code part of the official
> WebDriver commands, and just differentiate by the currently
> selected context. I would be happy to make those changes, if you
> think that should be quickly done.

Yes, so I think you’ve got the gist.  The idea is using
Marionette:{GetContext,SetContext} to “contextualise” commands such
as WebDriver:GetWindowHandle so that it would return the chrome
window handle in chrome context and the web window handle in content
context.  This would allow us to remove chrome-only, non-standard
commands such as GetChromeWindowHandle.

Alas I don’t think it is easy to make this change at the moment.
Working on https://bugzil.la/1311041 I’ve discovered multiple
cases where we don’t have a clean separation between chrome- and
content context.  For example some tests will be in chrome context
but expect commands querying content to still work, and vice
versa.  I’ve filed some bugs about this (https://bugzil.la/1426208,
https://bugzil.la/1447024) and documented a few more cases in the
code (will file more bugs later once the patches are done).

In light of this I gave your patch an r+.  Because there is bound
to be additional churn for some of the commands you move into the
Marionette:* namespace you could consider leaving the untouched for
the moment, but doing the best we can of the current situation is
also fine even if it means a couple of more aliases in the future.
Comment on attachment 8965646 [details]
Bug 1451727 - [marionette] Update vendor specific commands to use custom prefixes.

Andreas, mind having another quick look? I think that looks way better now.
Attachment #8965646 - Flags: review?(ato)
Comment on attachment 8965644 [details]
Bug 1451727 - [marionette] Add "WebDriver:AcceptAlert" in favor of "WebDriver:AcceptDialog".

https://reviewboard.mozilla.org/r/234480/#review240144

> I can drop that but then we would have the following situations:
> 
> 1) When I'm going to update marionette client, I have to add a fallback similar to the other chrome related commands
> 2) For geckodriver we would also need a fallback because this end-point will not work with earlier Firefox releases. Personally I would make use of `WebDriver:AcceptDialog` for now, and then update it to `WebDriver:AcceptAlert` in Firefox 63.

Andreas, can we leave that in, or do you still want to see it removed?
Attachment #8965646 - Flags: review?(ato) → review+
(In reply to Henrik Skupin (:whimboo) from comment #25)
> Comment on attachment 8965644 [details]
> Bug 1451727 - [marionette] Add "WebDriver:AcceptAlert" in favor of
> "WebDriver:AcceptDialog".
> 
> https://reviewboard.mozilla.org/r/234480/#review240144
> 
> > I can drop that but then we would have the following situations:
> > 
> > 1) When I'm going to update marionette client, I have to add a fallback similar to the other chrome related commands
> > 2) For geckodriver we would also need a fallback because this end-point will not work with earlier Firefox releases. Personally I would make use of `WebDriver:AcceptDialog` for now, and then update it to `WebDriver:AcceptAlert` in Firefox 63.
> 
> Andreas, can we leave that in, or do you still want to see it removed?

I don’t require any change here.  Do what you think is best.
(In reply to Andreas Tolfsen ‹:ato› from comment #26)
> I don’t require any change here.  Do what you think is best.

You opened an issue for that. So I'm going to drop that. Thanks.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a7c96a481976
[marionette] Add "WebDriver:AcceptAlert" in favor of "WebDriver:AcceptDialog". r=ato
https://hg.mozilla.org/integration/autoland/rev/7f5061bafd97
[marionette] Update WebDriver specific commands to use the "WebDriver" prefix. r=ato
https://hg.mozilla.org/integration/autoland/rev/12196e352ac1
[marionette] Update vendor specific commands to use custom prefixes. r=ato
https://hg.mozilla.org/integration/autoland/rev/e020fe7e691e
[marionette] Remove unused execute_js_script method. r=ato
https://hg.mozilla.org/integration/autoland/rev/c2ddf1e6abb6
[marionette] Remove deprecated WebDriver commands. r=ato
This is a test-only patch which we want to see uplifted to mozilla-beta. Since it landed on autoland all is working fine, so can you please uplift?
Whiteboard: [checkin-needed-beta]
Whiteboard: [checkin-needed-beta]
You need to log in before you can comment on or make changes to this bug.