feat: add 'dom-ready' event to WebFrameMain

[samuelmaddock]

Description of Change

resolves #27344

WebContents now emits a 'frame-dom-ready' event when either the top-level frame or a sub frame's document is ready.

WebFrameMain now emits a 'dom-ready' event when the frame's document is ready.

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• tests are changed or added
• relevant documentation is changed or added
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes:

• Added 'dom-ready' event to WebFrameMain which emits when the frame's document is ready.
• Added 'frame-created' event to WebContents which emits when a frame is created in the page.

[MarshallOfSound]

Should this be a dom-ready event on the WebFrameMain class instead? Seems frame specific things should go their now we have it

[samuelmaddock]

Should this be a dom-ready event on the WebFrameMain class instead? Seems frame specific things should go their now we have it

I see frame discovery as a potential limitation of that approach. Currently we don't have enough events provided to know the full lifecycle of a frame. There's no frame-created or destroyed events yet which might be needed first.

Adding frame-dom-ready is admittedly a convenience event for frames not necessarily being tracked. Without lifecycle events for frames, this is the easiest approach. Do you see many downsides to this event? WebContents event bloat?

Also, as an aside, does dom-ready-in-frame read better? 🤔

[zcbenz]

API looks good to me.

[samuelmaddock]

@MarshallOfSound Here's a proposal for how I could see providing equivalent functionality with events moved into the frame itself.

interface WebContents {
  /** Emitted when an iframe is created within the page. */
  on(event: 'frame-created', listener: (event: Event, frame: WebFrameMain) => void): this;
}

interface WebFrameMain {
  /** Emitted when the DOM has finished loading. */
  on(event: 'dom-ready', listener: (event: Event) => void): this;
}

Example usage:

const browserWindow = new BrowserWindow()

browserWindow.webContents.on('frame-created', (e, frame) => {
  frame.on('dom-ready', () => {})
})

I'd be fine making these changes if the proposal looks okay.

[MarshallOfSound]

My concern is with on(event: 'frame-created', listener: (event: Event, frame: WebFrameMain) => void): this; is the slight overhead of creating a new WebFrameMain instance for every frame. Seems like a price that every app will pay even though 99% of apps won't use this API.

I've had a thing on the backlog for a while to not emit events that don't have listeners (to avoid paying gin converter costs) but without that work this would contain an annoying memory / performance hit for folks with lots of frames

[samuelmaddock]

That's a fair point, and likely pushes the API into a better design overall. How would you feel about a details object with the IDs to lookup the frame?

interface FrameCreatedDetails {
  processId: number;
  routingId: number;
  /** When we eventually feel more comfortable, we can extend the details to include the actual frame. */
  // frame: Frame;
}

interface WebContents {
  /** Emitted when an iframe is created within the page. */
  on(event: 'frame-created', listener: (event: Event, details: FrameCreatedDetails) => void): this;
}

interface WebFrameMain {
  /** Emitted when the DOM has finished loading. */
  on(event: 'dom-ready', listener: (event: Event) => void): this;
}

const browserWindow = new BrowserWindow()

browserWindow.webContents.on('frame-created', (e, details) => {
  const frame = webFrameMain.fromId(details.processId, details.routingId)
  frame.on('dom-ready', () => {})
})

[MarshallOfSound]

That seems more reasonable although slightly less easy to use for folks that do really want that frame 🤷 I think it's the better side of the trade off though

[samuelmaddock]

That seems more reasonable although slightly less easy to use for folks that do really want that frame 🤷 I think it's the better side of the trade off though

Do we have precedence for events that use getters? e.g.

const details: FrameCreatedDetails = {
  get frame(): WebFrameMain {
    // lazily performs gin conversion
  }
}

I'm not familiar with how this could be done with v8, but I believe Chrome does this for a few of their globals such as chrome.*.

[MarshallOfSound]

Do we have precedence for events that use getters? e.g.

No but doing this lazily sounds like a nice middle ground

[samuelmaddock]

@MarshallOfSound @zcbenz @miniak The contents of this PR has changed to reflect the discussion above.

Instead of emitting a 'frame-dom-ready' event from WebContents, 'dom-ready' is now emitted from WebFrameMain itself. To listen for newly created frames, 'frame-created' is now emitted from WebContents.

[miniak]

@samuelmaddock does it make sense to also have a destroyed event on the frame?

[nornagon]

What's the status of this PR @samuelmaddock ?

[samuelmaddock]

What's the status of this PR @samuelmaddock ?

It's now unblocked by #30076 so I'm planning to rebase and retest sometime this week hopefully.

[samuelmaddock]

This PR is ready for review again. WebContent's frame-created event is now emitted correctly as defined in the added docs.

[nornagon]

API LGTM.

[nornagon]

Build failures are unrelated. This has two API LGTMs, one from @zcbenz and one from myself, so I'm going to go ahead and merge.

[release-clerk]

Release Notes Persisted

• Added 'dom-ready' event to WebFrameMain which emits when the frame's document is ready.
• Added 'frame-created' event to WebContents which emits when a frame is created in the page.

[trop]

I have automatically backported this PR to "15-x-y", please check out #30801