1722464 - MessageHandler should support custom root folder to load modules

Status: NEW

(Remote Protocol :: WebDriver BiDi, task, P3)

Description

[Julian Descottes [:jdescottes]]

For the initial implementation of the MessageHandler, we will assume that MessageHandler modules should live in remote/webdriver-bidi/modules

However, if we want to start using MessageHandler for other protocols such as DevTools or CDP, we should decide how we organize modules for those other protocols.

One option is to have a different root module folder for each protocol. This means ModuleCache should be aware of which protocol it's been created for. We should have a plan to still share code between the various protocols.

If we have a Log module implementation in root & windowglobal for CDP & BiDi, this leads to the following folder layout:

remote
 |_ webdriver_bidi
    |_ modules
       |_ root
       |  |_ Log.jsm
       |_ windowglobal
          |_ Log.jsm
 |_ cdp
    |_ modules
       |_ root
       |  |_ Log.jsm
       |_ windowglobal
          |_ Log.jsm

Or we can keep all modules under the same root folder. If protocol specific modules are needed, they will need have different name. For the same example it would lead to:

remote
 |_ shared
    |_ messagehandler
       |_ modules
          |_ root
          |  |_ BiDiLog.jsm
          |  |_ CDPLog.jsm
          |_ windowglobal
             |_ BiDiLog.jsm
             |_ CDPLog.jsm

Comment 1

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

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

However, if we want to start using MessageHandler for other protocols such as DevTools or CDP, we should decide how we organize modules for those other protocols.

IMO having a clear separation should be a requirement. Mixing files of totally different protocols under the same root folder wouldn't evolve into the right direction due to the following reasons:

• Shared code shouldn't embed/include files from specific implementations.
• We would have to implement a protection that message handlers cannot instantiate and use modules from a different protocol.
• It's harder to find the right code when working on a specific protocol and having to switch between root folders.

One option is to have a different root module folder for each protocol. This means ModuleCache should be aware of which protocol it's been created for. We should have a plan to still share code between the various protocols.

If we have a Log module implementation in root & windowglobal for CDP & BiDi, this leads to the following folder layout:

remote
 |_ webdriver_bidi
    |_ modules
       |_ root
       |  |_ Log.jsm
       |_ windowglobal
          |_ Log.jsm
 |_ cdp
    |_ modules
       |_ root
       |  |_ Log.jsm
       |_ windowglobal
          |_ Log.jsm

That would be my favorite, yes. Shared code can still be located under /remote/shared and called from protocol specific implementations, eg. registered network observers under /remote/shared/observers/Network.jsm. Protocol specific implementations will remain in the appropriate modules.

Comment 2

[Julian Descottes [:jdescottes]]

I looked a bit into that, and having different root folders for modules could also be interesting for tests.
In case we want to have browser mochitests for messagehandlers, similar to the ones I currently have at https://phabricator.services.mozilla.com/D120084, we could actually have the additional modules as support-files. Then we would just have to ensure the MessageHandler network is started with a modulesRoot set to something like "chrome://mochitests/content/browser/remote/shared/messagehandler/test/browser/resources/modules" and we could then have test-only modules, not packaged with Firefox.

Even though wdspec tests are great, I think that actual tests asserting the complete behavior (including edge cases) are a must. Otherwise our coverage will be bound by the current surface used by the commands we implemented. And we could regress without noticing.

That being said, having the ability to pass a modulesRoot to ModuleCache instances basically means that we have to add some meta data to the session and make that available to all processes. While we could implement it right away, I think it would make more sense to wait for Bug 1713443 - MessageHandler: Support shared global session data that supports BiDi and CDP which will allow to easily pass such information to all processes.

Therefore I'm going to block this on Bug 1713443, but I think I agree that having custom root modules would overall be a good move.

Comment 3

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

(In reply to Julian Descottes [:jdescottes] from comment #2)
ensure the MessageHandler network is started with a modulesRoot set to something like "chrome://mochitests/content/browser/remote/shared/messagehandler/test/browser/resources/modules" and we could then have test-only modules, not packaged with Firefox.

This would be great and we should definitely support that. So yes, lets polish that and get the appropriate browser chrome tests added.

That being said, having the ability to pass a modulesRoot to ModuleCache instances basically means that we have to add some meta data to the session and make that available to all processes. While we could implement it right away, I think it would make more sense to wait for Bug 1713443 - MessageHandler: Support shared global session data that supports BiDi and CDP which will allow to easily pass such information to all processes.

In terms of propagation through the message handler network this makes total sense. So yes, lets wait until that's possible.

Thanks for having a deeper look into that topic.

Comment 4

[Julian Descottes [:jdescottes]]

Renamed to match the latest discussion