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

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.

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

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?

	Andreas Tolfsen ❲:ato❳
	Comment 2 • 6 years ago

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.

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

Great. So I will get this fixed in my upcoming patch.

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

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.

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

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

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

Comment on attachment 8965647 [details]
Bug 1451727 - [marionette] Remove unused execute_js_script method.

https://reviewboard.mozilla.org/r/234486/#review240164

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

Comment on attachment 8965648 [details]
Bug 1451727 - [marionette] Remove deprecated WebDriver commands.

https://reviewboard.mozilla.org/r/234488/#review240168

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

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.

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

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.

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

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.

	Andreas Tolfsen ❲:ato❳
	Comment 18 • 6 years ago

(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.

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

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.

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

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?

	Andreas Tolfsen ❲:ato❳
	Comment 26 • 6 years ago

(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.

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

(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.

	Pulsebot
	Comment 28 • 6 years ago

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

	Raul Gurzau (:RaulG)
	Comment 29 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a7c96a481976
https://hg.mozilla.org/mozilla-central/rev/7f5061bafd97
https://hg.mozilla.org/mozilla-central/rev/12196e352ac1
https://hg.mozilla.org/mozilla-central/rev/e020fe7e691e
https://hg.mozilla.org/mozilla-central/rev/c2ddf1e6abb6

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

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?

	Andreea Pavel [:apavel]
	Comment 31 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/def0f8799e4f
https://hg.mozilla.org/releases/mozilla-beta/rev/1bcba4cdfa86
https://hg.mozilla.org/releases/mozilla-beta/rev/3d457aab66d4
https://hg.mozilla.org/releases/mozilla-beta/rev/0c798753bf07
https://hg.mozilla.org/releases/mozilla-beta/rev/5b7aa80c2619