1756610 - Implement "browsingContext.domContentLoaded" event

Status: RESOLVED FIXED

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

Description

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

We should be able to fully implement the event. But if it turns out that the navigation id is causing us problems we could move it to a follow-up bug. See the browsingContext.navigationStarted event (bug 1756595) for further details.

Comment 1

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

Now that basic navigation has been implemented I wonder if we can take this bug into the M3 milestone which is about navigation.

Comment 2

[Julian Descottes [:jdescottes]]

Discussed this morning with Henrik about this, summary and thoughts below.

Current usage

We already monitor the DOMContentLoaded event as an internal event to support browsingContext.navigate. Let's first review how this is handled.
In the parent process, the root browsingContext module uses the MessageHandler's EventDispatcher to subscribe to browsingContext.domContentLoaded (link). When calling EventDispatcher.on, we add a session data item corresponding to this event so that we can setup the event listeners as soon as possible when new windows are created. For this particular use case, the session data item is not using the global "ALL" context descriptor, but instead only monitors events from a given tab (see context descriptor definition).

In content processes, when a new window global is created, the MessageHandler initialization checks the content of the SessionData, and if it finds a session data item which corresponds to the browsingContext.DOMContentLoaded event, it will automatically spawn a windowglobal browsingContext module. The constructor of this module creates a "LoadListener", which is one of our low-level listener classes, and "starts" the listener when applying the session data item.

We now need to expose DOMContentLoaded as a protocol event. Also note the case difference, the spec uses domContentLoaded, while we have been using DOMContentLoaded for the internal event. Whatever we decide, we might want to rename the internal event to something else, to avoid confusion?

Option 1: content process

Since the event comes from the content process, we could follow the same pattern as for log.entryAdded. The root browsingContext modules should update _subscribeEvent to add a session data item for the event (see current implementation for the log module).

In the content process, the windowglobal browsingContext will then have to handle this event similarly to what is done by the log module. We can reuse the LoadListener in order to listen for the event. But we have to solve a few problems:

• a single windowglobal browsingContext module might listen to both the internal and the protocol DOMContentLoaded events. Our usual listener APIs (with start/stop) typically don't work with several consumers, so we need to fix this.
• we have two code paths dealing with almost identical events, but one is internal, one is protocol, which might be confusing

Option 2: parent process

Alternatively, we can probably fully implement the protocol event in the parent process. The root browsingContext module has to implement _subscribeEvent anyway to support subscribing to events coming from session.subscribe. From there, we could use this.messageHandler.eventsDispatcher.on to add another callback for the internal DOMContentLoaded event. And the callback would emit the protocol event.

Comment 3

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

As discussed we want to include this bug into M4 to have unblocked work available.

Comment 4

[Julian Descottes [:jdescottes]]

Given that this event overlaps with an event currently marked as internal, I think we should block this work on Bug 1783177.
Once this Bug is fixed, we can simply update the browsingContext.DOMContentLoaded event to have the right name and data.

Comment 5

[Julian Descottes [:jdescottes]]

I am also blocking on Bug 1741834, because domContentLoaded is currently monitored for a specific browsing context, when used internally. So when we start supporting it as a protocol event, we also need to take care of scenarios such as:

• consumer A subscribes to domContentLoaded for browsing context 1
• consumer B subscribes to domContentLoaded for ALL browsing contexts
• consumer B unsubscribes from domContentLoaded for ALL browsing contexts

At that point, we need to make sure that domContentLoaded is still monitored for browsing context 1, which is probably not the case with the way we broadcast our SessionData updates. I believe that at step 2 for now we will simply send a broadcast about the removal of domContentLoaded, which will lead the MessageHandler for browsing context 1 to remove the listeners.

We need one layer to be responsible of handling the logic related to overlapping context descriptors (ie ContextDescriptorType.All vs ContextDescriptorType.TopBrowsingContext).

I will start experimenting with various approaches and report back on Bug 1741834. My initial feeling is that SessionData should not try to overcomplicate things, and either MessageHandler or individual modules should be smart enough to check what they actually need to do. Eg if a module receives the information that domContentLoaded is no longer monitored for ALL, but they can still see that it is monitored for their specific context, they should ignore the update.

Comment 6

[Julian Descottes [:jdescottes]]

Unblocking from Bug 1741834. Reusing the internal event might bring other problems, such as payload differences (internally we need more information than what the spec expects for the public event).

So it might just be better to implement it as a completely different event. If we do that, the scenario described in comment #5 is no longer relevant.

Comment 7

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1756610 - [bidi] Rename internal "browsingContext.DOMContentLoaded" event.

Comment 8

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1756610 - [bidi] Implement "browsingContext.domContentLoaded" event.

Depends on D160763

Comment 9

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1756610 - [wdspec] Add tests for "browsingContext.domContentLoaded" event.

Depends on D160764

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3465ee8f3c0
[bidi] Rename internal "browsingContext.DOMContentLoaded" event. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/93fe7d48f8f4
[bidi] Implement "browsingContext.domContentLoaded" event. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/5763d84e51c3
[wdspec] Add tests for "browsingContext.domContentLoaded" event. r=webdriver-reviewers,jdescottes,whimboo

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/36821 for changes under testing/web-platform/tests

Comment 12

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/d3465ee8f3c0
https://hg.mozilla.org/mozilla-central/rev/93fe7d48f8f4
https://hg.mozilla.org/mozilla-central/rev/5763d84e51c3

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot