Closed Bug 1691500 Opened 8 months ago Closed 5 months ago

Extract window handling code to a shareable module

Categories

(Testing :: Marionette, task, P2)

Default
task
Points:
8

Tracking

(firefox90 fixed)

RESOLVED FIXED
90 Branch
Tracking Status
firefox90 --- fixed

People

(Reporter: whimboo, Assigned: jdescottes)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [bidi-m1-mvp])

Attachments

(5 files, 2 obsolete files)

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.

Blocks: 1691501
Points: --- → 2
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED

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.

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

Attachment #9210875 - Attachment description: Bug 1691500 - [marionette] Create a window-handler module → Bug 1691500 - [marionette] Create a window-manager module

(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

Points: 2 → ---
Blocks: 1704394
Attachment #9210878 - Attachment is obsolete: true
Attachment #9210879 - Attachment is obsolete: true
Attachment #9210877 - Attachment description: Bug 1691500 - [marionette] Stop using driver::findWindow with a truthy callback → Bug 1691500 - [marionette] Move findWindow to window-manager.js
Points: --- → 8
See Also: → 1704779
Pushed by jdescottes@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.