- Notifications
You must be signed in to change notification settings - Fork16.2k
refactor: replace webFrame.routingId with sync IPC#47717
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
| ): Electron.WebFrame | null { | ||
| findFrameByRoutingIdDeprecated(); | ||
| const frameToken = ipcRendererUtils.invokeSync<string | undefined>( | ||
| IPC_MESSAGES.BROWSER_GET_FRAME_TOKEN_SYNC, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It hadn't occurred to me that this workaround would require one roundtrip to get the routingId, and then a second roundtrip to convert it right back into a frame token 😅
That said, this still seems like the best way to give apps more time to migrate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's only one roundtrip so it's not too bad!FindFrameByToken only looks within the current renderer process afaik. If the frame is in another process, it won't be returned from theWebFrame API.
electron/shell/renderer/api/electron_api_web_frame.cc
Lines 823 to 826 ina33b599
| content::RenderFrame* render_frame = | |
| content::RenderFrame::FromWebFrame(web_frame); | |
| if (render_frame) | |
| returnWebFrameRenderer::Create(isolate, render_frame).ToV8(); |
| *`frameToken` string - A`string` representing the unique frame token in the | ||
| current renderer process. | ||
| Returns`WebFrameMain | undefined` - A frame with the given process and frame token, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is there a reason thatWebFrameMain returnsundefined butWebFrame returnsnull? Or is that just an artifact of the APIs evolving independently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The latter. Definitely open to changing this at some point later on 😄
typings/internal-ambient.d.ts Outdated
| @@ -133,7 +133,8 @@ declare namespace NodeJS { | |||
| interfaceWebFrameMainBinding{ | |||
| WebFrameMain:typeofElectron.WebFrameMain; | |||
| fromId(processId:number,routingId:number):Electron.WebFrameMain; | |||
| fromId(processId:number,routingId:number):Electron.WebFrameMain|null; | |||
| fromFrameToken(processId:number,frameToken:string):Electron.WebFrameMain|null; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Seems like this is typed as| null here, but inweb-frame-main.md it's| undefined?
3da63d1 tob61f468CompareI'll wait for the roll to be merged before fixing up this PR. |
4db9fea tod3690a9Comparea33b599 tobc7c1c0Compare
Uh oh!
There was an error while loading.Please reload this page.
Description of Change
#47616
Replaces
webFrame.routingIdandwebFrame.findFrameByRoutingId()with internal synchronous IPC implementations. This allows us to avoid immediate breaking changes and allows application developers time to upgrade.Checklist
npm testpassesRelease Notes
Notes:
webFrameMain.fromFrameToken(processId, frameToken)to get aWebFrameMaininstance from its frame token.