1691500 - Extract window handling code to a shareable module

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, task, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Handling windows and their ids need to be unique between Marionette, WebDriver BiDi and CDP. As such all the code related to handling Windows in driver.js should be extracted to a new JS module.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1691500 - [marionette] Create a window-manager module

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1691500 - [marionette] Move findWindow to window-manager.js

Depends on D109431

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1691500 - [marionette] Simplify findWindow helper to match against handle

Depends on D109432

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1691500 - [marionette] Move findWindowByHandle and getWindowProperties to window-handler.js

Depends on D109433

Comment 5

[Julian Descottes [:jdescottes]]

I struggle a bit to decide what should be in this shared module or not, so I am uploading my current state and we should discuss if this is going in the right direction or not.

In the stack uploaded here there are still 4 methods in driver.js which obviously deal with windows/window handles:

• getChromeWindowHandle
• getChromeWindowHandles
• getWindowHandle
• getWindowHandles

getWindowHandle/getChromeWindowHandle will both need to be refactored when we switch to ids other than the browsingContextID because they both assume that the window handle is a browsing context id in order to retrieve the current content/chrome window handle. If we start using browser permanentKey + a uuid to identify content browsers, we will have to decide if we still compute this from the browsing context (which is stored in the session) or if we need to store something else in the session.

getWindowHandles/getChromeWindowHandles are simply pretty printing and can be moved to the new module, I just kept them out to limit the changes.

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1691500 - [marionette] Remove empty leftover file from initial marionette-window-tracking attempt

Preliminary cleanup for window-handler module. Left over from https://bugzilla.mozilla.org/show_bug.cgi?id=marionette-window-tracking

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1691500 - [marionette] Remove unused curFrameId getter in marionette browser.js

Depends on D109467

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Julian Descottes [:jdescottes] from comment #5)

I struggle a bit to decide what should be in this shared module or not, so I am uploading my current state and we should discuss if this is going in the right direction or not.

In the stack uploaded here there are still 4 methods in driver.js which obviously deal with windows/window handles:

• getChromeWindowHandle
• getChromeWindowHandles
• getWindowHandle
• getWindowHandles

All those are the entry points for the appropriate WebDriver commands, and as such have to remain in that file. What getChromeWindowHandle and getWindowHandle should basically do is a proper assertion if the current browsing context is still open. The remaining code should then basically come from the window handling module. The other two commands don't need such an assertion, and as such directly rely on the window handling module.

getWindowHandle/getChromeWindowHandle will both need to be refactored when we switch to ids other than the browsingContextID because they both assume that the window handle is a browsing context id in order to retrieve the current content/chrome window handle. If we start using browser permanent + a uuid to identify content browsers, we will have to decide if we still compute this from the browsing context (which is stored in the session) or if we need to store something else in the session.

If we could move out the code that's computing the window handle already to the next module that might be help already, so that we won't have to touch those methods in driver.js in the future anymore. Note that window handles aren't associated to the Session and the browsing context ids.

Comment 9

[Julian Descottes [:jdescottes]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #8)

(In reply to Julian Descottes [:jdescottes] from comment #5)

I struggle a bit to decide what should be in this shared module or not, so I am uploading my current state and we should discuss if this is going in the right direction or not.

In the stack uploaded here there are still 4 methods in driver.js which obviously deal with windows/window handles:

• getChromeWindowHandle
• getChromeWindowHandles
• getWindowHandle
• getWindowHandles

All those are the entry points for the appropriate WebDriver commands, and as such have to remain in that file. What getChromeWindowHandle and getWindowHandle should basically do is a proper assertion if the current browsing context is still open. The remaining code should then basically come from the window handling module. The other two commands don't need such an assertion, and as such directly rely on the window handling module.

getWindowHandle/getChromeWindowHandle will both need to be refactored when we switch to ids other than the browsingContextID because they both assume that the window handle is a browsing context id in order to retrieve the current content/chrome window handle. If we start using browser permanent + a uuid to identify content browsers, we will have to decide if we still compute this from the browsing context (which is stored in the session) or if we need to store something else in the session.

If we could move out the code that's computing the window handle already to the next module that might be help already, so that we won't have to touch those methods in driver.js in the future anymore. Note that window handles aren't associated to the Session and the browsing context ids.

Thanks for the feedback Henrik!

I agree with the general and initially I wanted to also move the non-session related part of getWindowHandle. What stopped me was that it meant exposing something like getIdForBrowsingContext, and I was not sure if this API would make sense in the long run.

Note that window handles aren't associated to the Session and the

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: Bug 1691500 - [marionette] Move open/close/focusWindow from browser module to window-manager.js

Depends on D109432

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ef57d408e66
[marionette] Remove empty leftover file from initial marionette-window-tracking attempt r=marionette-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/6ca17a62910c
[marionette] Remove unused curFrameId getter in marionette browser.js r=marionette-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/0bc8628153e4
[marionette] Create a window-manager module r=marionette-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/e9cfcb6308fe
[marionette] Move findWindow to window-manager.js r=marionette-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/e51e9f4989fe
[marionette] Move open/close/focusWindow from browser module to window-manager.js r=marionette-reviewers,whimboo

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/3ef57d408e66
https://hg.mozilla.org/mozilla-central/rev/6ca17a62910c
https://hg.mozilla.org/mozilla-central/rev/0bc8628153e4
https://hg.mozilla.org/mozilla-central/rev/e9cfcb6308fe
https://hg.mozilla.org/mozilla-central/rev/e51e9f4989fe