Closed Bug 1451727 Opened 7 years ago Closed 7 years ago

Update Marionette client for new WebDriver and browser vendor commands

Categories

(Remote Protocol :: Marionette, enhancement, P1)

Product:
Remote Protocol
Component:
Marionette
Type:
enhancement

Tracking

(firefox59 wontfix, firefox60 fixed, firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(5 files)

Bug 1451727 - [marionette] Add "WebDriver:AcceptAlert" in favor of "WebDriver:AcceptDialog".
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1451727 - [marionette] Update WebDriver specific commands to use the "WebDriver" prefix.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1451727 - [marionette] Update vendor specific commands to use custom prefixes.
59 bytes, text/x-review-board-request
ato
: review+
ato
: review+
Details
Bug 1451727 - [marionette] Remove unused execute_js_script method.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1451727 - [marionette] Remove deprecated WebDriver commands.
59 bytes, text/x-review-board-request
ato
: review+
Details
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.
See Also: → 1452024
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.
Blocks: 1452024
(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?
status-firefox59: --- → wontfix
status-firefox60: --- → affected
Whiteboard: [checkin-needed-beta]
Whiteboard: [checkin-needed-beta]
Blocks: 1480107
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: